Re: [PATCH v3 0/7] Hexdump Enhancements
On Wed, 19 Jun 2019, Joe Perches wrote:
> On Thu, 2019-06-20 at 11:14 +1000, Alastair D'Silva wrote:
>> On Wed, 2019-06-19 at 17:35 -0700, Joe Perches wrote:
>> > On Thu, 2019-06-20 at 09:15 +1000, Alastair D'Silva wrote:
>> > > On Wed, 2019-06-19 at 09:31 -0700, Joe Perches wrote:
>> > > > On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
>> > > > > From: Alastair D'Silva
>> > > > >
>> > > > > Apologies for the large CC list, it's a heads up for those
>> > > > > responsible
>> > > > > for subsystems where a prototype change in generic code causes
>> > > > > a
>> > > > > change
>> > > > > in those subsystems.
>> > > > >
>> > > > > This series enhances hexdump.
>> > > >
>> > > > Still not a fan of these patches.
>> > >
>> > > I'm afraid there's not too much action I can take on that, I'm
>> > > happy to
>> > > address specific issues though.
>> > >
>> > > > > These improve the readability of the dumped data in certain
>> > > > > situations
>> > > > > (eg. wide terminals are available, many lines of empty bytes
>> > > > > exist,
>> > > > > etc).
>> >
>> > I think it's generally overkill for the desired uses.
>>
>> I understand where you're coming from, however, these patches make it a
>> lot easier to work with large chucks of binary data. I think it makes
>> more sense to have these patches upstream, even though committed code
>> may not necessarily have all the features enabled, as it means that
>> devs won't have to apply out-of-tree patches during development to make
>> larger dumps manageable.
>>
>> > > > Changing hexdump's last argument from bool to int is odd.
>> > > >
>> > >
>> > > Think of it as replacing a single boolean with many booleans.
>> >
>> > I understand it. It's odd.
>> >
>> > I would rather not have a mixture of true, false, and apparently
>> > random collections of bitfields like 0xd or 0b1011 or their
>> > equivalent or'd defines.
>> >
>>
>> Where's the mixture? What would you propose instead?
>
> create a hex_dump_to_buffer_ext with a new argument
> and a new static inline for the old hex_dump_to_buffer
> without modifying the argument list that calls
> hex_dump_to_buffer with whatever added argument content
> you need.
>
> Something like:
>
> static inline
> int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> int groupsize, char *linebuf, size_t linebuflen,
> bool ascii)
> {
> return hex_dump_to_buffer_ext(buf, len, rowsize, groupsize,
> linebuf, linebuflen, ascii, 0);
> }
>
> and remove EXPORT_SYMBOL(hex_dump_to_buffer)
If you decide to do something like this, I'd actually suggest you drop
the bool ascii parameter from hex_dump_to_buffer() altogether, and
replace the callers that do require ascii with
hex_dump_to_buffer_ext(..., HEXDUMP_ASCII). Even if that also requires
touching all callers.
But no strong opinions, really.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 4/7] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags
On Mon, 17 Jun 2019, "Alastair D'Silva" wrote: > From: Alastair D'Silva > > In order to support additional features in hex_dump_to_buffer, replace > the ascii bool parameter with flags. > > Signed-off-by: Alastair D'Silva > --- > drivers/gpu/drm/i915/intel_engine_cs.c| 2 +- > drivers/isdn/hardware/mISDN/mISDNisar.c | 6 -- > drivers/mailbox/mailbox-test.c| 2 +- > drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 2 +- > drivers/net/ethernet/synopsys/dwc-xlgmac-common.c | 2 +- > drivers/net/wireless/ath/ath10k/debug.c | 3 ++- > drivers/net/wireless/intel/iwlegacy/3945-mac.c| 2 +- > drivers/platform/chrome/wilco_ec/debugfs.c| 2 +- > drivers/scsi/scsi_logging.c | 8 +++- > drivers/staging/fbtft/fbtft-core.c| 2 +- > fs/seq_file.c | 3 ++- > include/linux/printk.h| 8 > lib/hexdump.c | 15 --- > lib/test_hexdump.c| 5 +++-- > 14 files changed, 33 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c > b/drivers/gpu/drm/i915/intel_engine_cs.c > index eea9bec04f1b..5df5fffdb848 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -1340,7 +1340,7 @@ static void hexdump(struct drm_printer *m, const void > *buf, size_t len) > WARN_ON_ONCE(hex_dump_to_buffer(buf + pos, len - pos, > rowsize, sizeof(u32), > line, sizeof(line), > - false) >= sizeof(line)); > + 0) >= sizeof(line)); > drm_printf(m, "[%04zx] %s\n", pos, line); > > prev = buf + pos; On i915, Acked-by: Jani Nikula -- Jani Nikula, Intel Open Source Graphics Center ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 4/7] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags
On Wed, 08 May 2019, Alastair D'Silva wrote:
> From: Alastair D'Silva
>
> In order to support additional features in hex_dump_to_buffer, replace
> the ascii bool parameter with flags.
>
> Signed-off-by: Alastair D'Silva
> ---
> drivers/gpu/drm/i915/intel_engine_cs.c | 2 +-
For i915,
Acked-by: Jani Nikula
> drivers/isdn/hardware/mISDN/mISDNisar.c | 6 --
> drivers/mailbox/mailbox-test.c| 2 +-
> drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 2 +-
> drivers/net/ethernet/synopsys/dwc-xlgmac-common.c | 2 +-
> drivers/net/wireless/ath/ath10k/debug.c | 3 ++-
> drivers/net/wireless/intel/iwlegacy/3945-mac.c| 2 +-
> drivers/platform/chrome/wilco_ec/debugfs.c| 2 +-
> drivers/scsi/scsi_logging.c | 8 +++-
> drivers/staging/fbtft/fbtft-core.c| 2 +-
> fs/seq_file.c | 3 ++-
> include/linux/printk.h| 8
> lib/hexdump.c | 15 ---
> lib/test_hexdump.c| 5 +++--
> 14 files changed, 33 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 49fa43ff02ba..fb133e729f9a 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1318,7 +1318,7 @@ static void hexdump(struct drm_printer *m, const void
> *buf, size_t len)
> WARN_ON_ONCE(hex_dump_to_buffer(buf + pos, len - pos,
> rowsize, sizeof(u32),
> line, sizeof(line),
> - false) >= sizeof(line));
> + 0) >= sizeof(line));
> drm_printf(m, "[%04zx] %s\n", pos, line);
>
> prev = buf + pos;
> diff --git a/drivers/isdn/hardware/mISDN/mISDNisar.c
> b/drivers/isdn/hardware/mISDN/mISDNisar.c
> index 386731ec2489..f13f34db6c17 100644
> --- a/drivers/isdn/hardware/mISDN/mISDNisar.c
> +++ b/drivers/isdn/hardware/mISDN/mISDNisar.c
> @@ -84,7 +84,8 @@ send_mbox(struct isar_hw *isar, u8 his, u8 creg, u8 len, u8
> *msg)
>
> while (l < (int)len) {
> hex_dump_to_buffer(msg + l, len - l, 32, 1,
> -isar->log, 256, 1);
> +isar->log, 256,
> +HEXDUMP_ASCII);
> pr_debug("%s: %s %02x: %s\n", isar->name,
>__func__, l, isar->log);
> l += 32;
> @@ -113,7 +114,8 @@ rcv_mbox(struct isar_hw *isar, u8 *msg)
>
> while (l < (int)isar->clsb) {
> hex_dump_to_buffer(msg + l, isar->clsb - l, 32,
> -1, isar->log, 256, 1);
> +1, isar->log, 256,
> +HEXDUMP_ASCII);
> pr_debug("%s: %s %02x: %s\n", isar->name,
>__func__, l, isar->log);
> l += 32;
> diff --git a/drivers/mailbox/mailbox-test.c b/drivers/mailbox/mailbox-test.c
> index 4e4ac4be6423..2f9a094d0259 100644
> --- a/drivers/mailbox/mailbox-test.c
> +++ b/drivers/mailbox/mailbox-test.c
> @@ -213,7 +213,7 @@ static ssize_t mbox_test_message_read(struct file *filp,
> char __user *userbuf,
> hex_dump_to_buffer(ptr,
> MBOX_BYTES_PER_LINE,
> MBOX_BYTES_PER_LINE, 1, touser + l,
> -MBOX_HEXDUMP_LINE_LEN, true);
> +MBOX_HEXDUMP_LINE_LEN, HEXDUMP_ASCII);
>
> ptr += MBOX_BYTES_PER_LINE;
> l += MBOX_HEXDUMP_LINE_LEN;
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> index 0cc911f928b1..e954a31cee0c 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
> @@ -2992,7 +2992,7 @@ void xgbe_print_pkt(struct net_device *netdev, struct
> sk_buff *skb, bool tx_rx)
> unsigned int len = min(skb->len - i, 32U);
>
> hex_dump_to_buffer(&skb->data[i], len, 32, 1,
> -
Re: [-next PATCH 0/4] sysfs and DEVICE_ATTR_
On Tue, 19 Dec 2017, Joe Perches wrote: > drivers/gpu/drm/i915/i915_sysfs.c | 12 ++-- For i915, Acked-by: Jani Nikula -- Jani Nikula, Intel Open Source Technology Center ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Intel-gfx] [PATCH 0/3] Kconfig dependencies: acpi-video, backlight and thermal
On Wed, 26 Jul 2017, Daniel Vetter wrote: > On Wed, Jul 26, 2017 at 03:53:09PM +0200, Arnd Bergmann wrote: >> Hi everyone, >> >> It took me a while to figure this out properly, as I kept getting >> circular or missing dependencies with video drivers. >> >> This set of three patches should simplify the situation a bit, >> mostly by cleaning up the dependencies around CONFIG_ACPI_VIDEO. >> With all three patches applied, I no longer run into those related >> warnings. If everyone agrees on the general direction, I hope >> we can merge all three through the DRM tree. >> >> I originally had another larger patch in the series to replace all >> of the 'select BACKLIGHT_LCD_SUPPORT; select BACKLIGHT_CLASS_DEVICE' >> statements with 'depends on LCD_CLASS_DEVICE', that would clean >> it up some more, but it is also a more invasive change that we >> can do separately at some point. > > Looks reasonable, but I think it'd be good to get Jani Nikula's explicit > ack on this, since he dugg around a lot in this area. And he's on vacation > this week. I didn't dig through all the details, but looks good to me and definitely an improvement in drm Kconfigs. Acked-by: Jani Nikula > -Daniel > >> >>Arnd >> >> Arnd Bergmann (3): >> backlight: always select BACKLIGHT_LCD_SUPPORT for >> BACKLIGHT_CLASS_DEVICE >> ACPI/DRM: rework ACPI_VIDEO Kconfig dependencies >> drm/etnaviv: add thermal dependency >> >> drivers/acpi/Kconfig | 7 +-- >> drivers/gpu/drm/etnaviv/Kconfig | 1 + >> drivers/gpu/drm/gma500/Kconfig| 5 + >> drivers/gpu/drm/i915/Kconfig | 7 +-- >> drivers/gpu/drm/nouveau/Kconfig | 10 ++ >> drivers/platform/x86/Kconfig | 9 - >> drivers/staging/olpc_dcon/Kconfig | 1 + >> 7 files changed, 15 insertions(+), 25 deletions(-) >> >> To: dri-de...@lists.freedesktop.org >> Cc: "Rafael J. Wysocki" >> Cc: Len Brown >> Cc: Lucas Stach >> Cc: Russell King >> Cc: Christian Gmeiner >> Cc: David Airlie >> Cc: Patrik Jakobsson >> Cc: Daniel Vetter >> Cc: Jani Nikula >> Cc: Ben Skeggs >> Cc: Darren Hart >> Cc: Andy Shevchenko >> Cc: Jens Frederich >> Cc: Daniel Drake >> Cc: Jon Nettleton >> Cc: Greg Kroah-Hartman >> Cc: linux-a...@vger.kernel.org >> Cc: linux-ker...@vger.kernel.org >> Cc: etna...@lists.freedesktop.org >> Cc: intel-...@lists.freedesktop.org >> Cc: nouv...@lists.freedesktop.org >> Cc: platform-driver-...@vger.kernel.org >> Cc: de...@driverdev.osuosl.org >> >> -- >> 2.9.0 >> >> ___ >> Intel-gfx mailing list >> intel-...@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] Documentation: Move visorbus documentation from staging to Documentation/
On Mon, 05 Jun 2017, David Kershner wrote: > This patch simply does a git mv of the > drivers/staging/unisys/Documentation directory to Documentation. Up to Jon, of course, but I wouldn't add any new files directly under Documentation, and especially not something as specific as this. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] format-security: move static strings to const
On Thu, 06 Apr 2017, Kees Cook wrote:
> While examining output from trial builds with -Wformat-security enabled,
> many strings were found that should be defined as "const", or as a char
> array instead of char pointer. This makes some static analysis easier,
> by producing fewer false positives.
>
> As these are all trivial changes, it seemed best to put them all in
> a single patch rather than chopping them up per maintainer.
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index f6d4d9700734..1ff9d5912b83 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2331,7 +2331,7 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
> int __init drm_fb_helper_modinit(void)
> {
> #if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
> - const char *name = "fbcon";
> + const char name[] = "fbcon";
I'd always write the former out of habit. Why should I start using the
latter? What makes it better?
What keeps the kernel from accumulating tons more of the former?
Here's an interesting comparison of the generated code. I'm a bit
surprised by what gcc does, I would have expected no difference, like
clang. https://godbolt.org/g/OdqUvN
The other changes adding const in this patch are, of course, good.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/3] doc: add documentation for uio-hv-generic
On Mon, 17 Oct 2016, Stephen Hemminger wrote: > From: Stephen Hemminger > > Update UIO documentation to include basic information about > uio_hv_generic. How about converting to Sphinx/reStructuredText first...? It's not a big file... BR, Jani. > > Signed-off-by: Stephen Hemminger > --- > Documentation/DocBook/uio-howto.tmpl | 62 > > 1 file changed, 62 insertions(+) > > diff --git a/Documentation/DocBook/uio-howto.tmpl > b/Documentation/DocBook/uio-howto.tmpl > index cd0e452..5210f8a 100644 > --- a/Documentation/DocBook/uio-howto.tmpl > +++ b/Documentation/DocBook/uio-howto.tmpl > @@ -46,6 +46,13 @@ GPL version 2. > > > > + 0.10 > + 2016-10-17 > + sch > + Added generic hyperv driver > + > + > + > 0.9 > 2009-07-16 > mst > @@ -1033,6 +1040,61 @@ int main() > > > > + > + > +Generic Hyper-V UIO driver > + > + The generic driver is a kernel module named uio_hv_generic. > + It supports devices on the Hyper-V VMBus similar to uio_pci_generic > + on PCI bus. > + > + > + > +Making the driver recognize the device > + > +Since the driver does not declare any device GUID's, it will not get loaded > +automatically and will not automatically bind to any devices, you must load > it > +and allocate id to the driver yourself. For example, to use the network > device > +GUID: > + > + modprobe uio_hv_generic > + echo "f8615163-df3e-46c5-913f-f2d2f965ed0e" > > /sys/bus/vmbus/drivers/uio_hv_generic/new_id > + > + > + > +If there already is a hardware specific kernel driver for the device, the > +generic driver still won't bind to it, in this case if you want to use the > +generic driver (why would you?) you'll have to manually unbind the hardware > +specific driver and bind the generic driver, like this: > + > + echo -n vmbus-ed963694-e847-4b2a-85af-bc9cfc11d6f3 > > /sys/bus/vmbus/drivers/hv_netvsc/unbind > + echo -n vmbus-ed963694-e847-4b2a-85af-bc9cfc11d6f3 > > /sys/bus/vmbus/drivers/uio_hv_generic/bind > + > + > + > +You can verify that the device has been bound to the driver > +by looking for it in sysfs, for example like the following: > + > +ls -l > /sys/bus/vmbus/devices/vmbus-ed963694-e847-4b2a-85af-bc9cfc11d6f3/driver > + > +Which if successful should print > + > + .../vmbus-ed963694-e847-4b2a-85af-bc9cfc11d6f3/driver -> > ../../../bus/vmbus/drivers/uio_hv_generic > + > + > + > + > + > +Things to know about uio_hv_generic > + > +On each interrupt, uio_hv_generic sets the Interrupt Disable bit. > +This prevents the device from generating further interrupts > +until the bit is cleared. The userspace driver should clear this > +bit before blocking and waiting for more interrupts. > + > + > + > + > > Further information > -- Jani Nikula, Intel Open Source Technology Center ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/6] lib: string: add function strtolower()
On Fri, 01 Jul 2016, Markus Mayer wrote:
> On 1 July 2016 at 03:52, Jani Nikula wrote:
>> On Fri, 01 Jul 2016, Markus Mayer wrote:
>>> Add a function called strtolower() to convert strings to lower case
>>> in-place, overwriting the original string.
>>>
>>> This seems to be a recurring requirement in the kernel that is
>>> currently being solved by several duplicated implementations doing the
>>> same thing.
>>>
>>> Signed-off-by: Markus Mayer
>>> ---
>>> include/linux/string.h | 1 +
>>> lib/string.c | 14 ++
>>> 2 files changed, 15 insertions(+)
>>>
>>> diff --git a/include/linux/string.h b/include/linux/string.h
>>> index 26b6f6a..aad605e 100644
>>> --- a/include/linux/string.h
>>> +++ b/include/linux/string.h
>>> @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
>>> #endif
>>> void *memchr_inv(const void *s, int c, size_t n);
>>> char *strreplace(char *s, char old, char new);
>>> +char *strtolower(char *s);
>>>
>>> extern void kfree_const(const void *x);
>>>
>>> diff --git a/lib/string.c b/lib/string.c
>>> index ed83562..6e3b560 100644
>>> --- a/lib/string.c
>>> +++ b/lib/string.c
>>> @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new)
>>> return s;
>>> }
>>> EXPORT_SYMBOL(strreplace);
>>> +
>>
>> This needs a kernel-doc comment right here.
>
> Will add it.
>
>>> +char *strtolower(char *s)
>>> +{
>>> + char *p;
>>> +
>>> +if (unlikely(!s))
>>> +return NULL;
>>
>> Using spaces for indentation? See scripts/checkpatch.pl.
>
> Not on purpose. Thanks for spotting it.
>
>>> +
>>> + for (p = s; *p; p++)
>>> + *p = tolower(*p);
>>> +
>>> + return s;
>>
>> Why does it return a value? Could be void?
>
> It could be void, but I thought that would make the function's use
> less flexible. As is, the return value is there if anybody wants it,
> but it can be ignored if it is not needed. Also, it seems customary
> for string functions to be returning the string that was passed in.
>
> I'll change it to void if there are strong opinions leaning that way.
> Personally, I like that it returns a char * better.
I don't have strong opinions on this. Just a general aversion to
returning something redundant. Avoids questions like, does it allocate a
new string, should I use the return value instead of the string I passed
in, should I check the return value or can I ignore it, should I check
both the string I pass in and the return value for != NULL, etc. But I
could be persuaded either way.
BR,
Jani.
>
>> BR,
>> Jani.
>>
>>> +}
>>> +EXPORT_SYMBOL(strtolower);
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
--
Jani Nikula, Intel Open Source Technology Center
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/6] lib: string: add function strtolower()
On Fri, 01 Jul 2016, Markus Mayer wrote:
> Add a function called strtolower() to convert strings to lower case
> in-place, overwriting the original string.
>
> This seems to be a recurring requirement in the kernel that is
> currently being solved by several duplicated implementations doing the
> same thing.
>
> Signed-off-by: Markus Mayer
> ---
> include/linux/string.h | 1 +
> lib/string.c | 14 ++
> 2 files changed, 15 insertions(+)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 26b6f6a..aad605e 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -116,6 +116,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
> #endif
> void *memchr_inv(const void *s, int c, size_t n);
> char *strreplace(char *s, char old, char new);
> +char *strtolower(char *s);
>
> extern void kfree_const(const void *x);
>
> diff --git a/lib/string.c b/lib/string.c
> index ed83562..6e3b560 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -952,3 +952,17 @@ char *strreplace(char *s, char old, char new)
> return s;
> }
> EXPORT_SYMBOL(strreplace);
> +
This needs a kernel-doc comment right here.
> +char *strtolower(char *s)
> +{
> + char *p;
> +
> +if (unlikely(!s))
> +return NULL;
Using spaces for indentation? See scripts/checkpatch.pl.
> +
> + for (p = s; *p; p++)
> + *p = tolower(*p);
> +
> + return s;
Why does it return a value? Could be void?
BR,
Jani.
> +}
> +EXPORT_SYMBOL(strtolower);
--
Jani Nikula, Intel Open Source Technology Center
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
