[PATCH v3 1/3] HID: core: reformat and reduce hid_printk macros

[PATCH v3 1/3] HID: core: reformat and reduce hid_printk macros

[stillcompiling]

From: Joshua Clayton <stillcompiling@gmail.com>

Reformat hid_printk macros to use standard __VA_ARGS__ syntax.
Per Joe Perches hid_printk(), hid_emerg(), hid_crit(), and hid_alert() are
unlikely ever to be used. Remove them.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>

diff --git a/include/linux/hid.h b/include/linux/hid.h
index d770ab1a0479..e6c7efdb0458 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -1154,29 +1154,21 @@ int hid_pidff_init(struct hid_device *hid);
 #define hid_pidff_init NULL
 #endif
 
-#define dbg_hid(format, arg...)						\
+#define dbg_hid(fmt, ...)						\
 do {									\
 	if (hid_debug)							\
-		printk(KERN_DEBUG "%s: " format, __FILE__, ##arg);	\
+		printk(KERN_DEBUG "%s: " fmt, __FILE__, ##__VA_ARGS__);	\
 } while (0)
 
-#define hid_printk(level, hid, fmt, arg...)		\
-	dev_printk(level, &(hid)->dev, fmt, ##arg)
-#define hid_emerg(hid, fmt, arg...)			\
-	dev_emerg(&(hid)->dev, fmt, ##arg)
-#define hid_crit(hid, fmt, arg...)			\
-	dev_crit(&(hid)->dev, fmt, ##arg)
-#define hid_alert(hid, fmt, arg...)			\
-	dev_alert(&(hid)->dev, fmt, ##arg)
-#define hid_err(hid, fmt, arg...)			\
-	dev_err(&(hid)->dev, fmt, ##arg)
-#define hid_notice(hid, fmt, arg...)			\
-	dev_notice(&(hid)->dev, fmt, ##arg)
-#define hid_warn(hid, fmt, arg...)			\
-	dev_warn(&(hid)->dev, fmt, ##arg)
-#define hid_info(hid, fmt, arg...)			\
-	dev_info(&(hid)->dev, fmt, ##arg)
-#define hid_dbg(hid, fmt, arg...)			\
-	dev_dbg(&(hid)->dev, fmt, ##arg)
+#define hid_err(hid, fmt, ...)				\
+	dev_err(&(hid)->dev, fmt, ##__VA_ARGS__)
+#define hid_notice(hid, fmt, ...)			\
+	dev_notice(&(hid)->dev, fmt, ##__VA_ARGS__)
+#define hid_warn(hid, fmt, ...)				\
+	dev_warn(&(hid)->dev, fmt, ##__VA_ARGS__)
+#define hid_info(hid, fmt, ...)				\
+	dev_info(&(hid)->dev, fmt, ##__VA_ARGS__)
+#define hid_dbg(hid, fmt, ...)				\
+	dev_dbg(&(hid)->dev, fmt, ##__VA_ARGS__)
 
 #endif
-- 
2.21.0

[PATCH v3 2/3] HID: core: Add printk_once variants to hid_warn() etc

[stillcompiling]

From: Joshua Clayton <stillcompiling@gmail.com>

hid_warn_once() is needed. Add the others as part of the block.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>

diff --git a/include/linux/hid.h b/include/linux/hid.h
index e6c7efdb0458..cd41f209043f 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -1171,4 +1171,15 @@ do {									\
 #define hid_dbg(hid, fmt, ...)				\
 	dev_dbg(&(hid)->dev, fmt, ##__VA_ARGS__)
 
+#define hid_err_once(hid, fmt, ...)			\
+	dev_err_once(&(hid)->dev, fmt, ##__VA_ARGS__)
+#define hid_notice_once(hid, fmt, ...)			\
+	dev_notice_once(&(hid)->dev, fmt, ##__VA_ARGS__)
+#define hid_warn_once(hid, fmt, ...)			\
+	dev_warn_once(&(hid)->dev, fmt, ##__VA_ARGS__)
+#define hid_info_once(hid, fmt, ...)			\
+	dev_info_once(&(hid)->dev, fmt, ##__VA_ARGS__)
+#define hid_dbg_once(hid, fmt, ...)			\
+	dev_dbg_once(&(hid)->dev, fmt, ##__VA_ARGS__)
+
 #endif
-- 
2.21.0

[PATCH v3 3/3] HID: core: fix dmesg flooding if report field larger than 32bit

[stillcompiling]

From: Joshua Clayton <stillcompiling@gmail.com>

Only warn once of oversize hid report value field

On HP spectre x360 convertible the message:
hid-sensor-hub 001F:8087:0AC2.0002: hid_field_extract() called with n (192) > 32! (kworker/1:2)
is continually printed many times per second, crowding out all else.
Protect dmesg by printing the warning only one time.

The size of the hid report field data structure should probably be increased.
The data structure is treated as a u32 in Linux, but an unlimited number
of bits in the USB hid spec, so there is some rearchitecture needed now that
devices are sending more than 32 bits.

Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 210b81a56e1a..3eaee2c37931 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1311,8 +1311,8 @@ u32 hid_field_extract(const struct hid_device *hid, u8 *report,
 			unsigned offset, unsigned n)
 {
 	if (n > 32) {
-		hid_warn(hid, "hid_field_extract() called with n (%d) > 32! (%s)\n",
-			 n, current->comm);
+		hid_warn_once(hid, "%s() called with n (%d) > 32! (%s)\n",
+			      __func__, n, current->comm);
 		n = 32;
 	}
 
-- 
2.21.0

Re: [PATCH v3 3/3] HID: core: fix dmesg flooding if report field larger than 32bit

[Joshua Clayton]

ping?
I'd love to see this get in.
with distro kernel I have effectively no dmesg due to this issue

On Mon, Aug 12, 2019 at 9:20 AM <stillcompiling@gmail.com> wrote:
>
> From: Joshua Clayton <stillcompiling@gmail.com>
>
> Only warn once of oversize hid report value field
>
> On HP spectre x360 convertible the message:
> hid-sensor-hub 001F:8087:0AC2.0002: hid_field_extract() called with n (192) > 32! (kworker/1:2)
> is continually printed many times per second, crowding out all else.
> Protect dmesg by printing the warning only one time.
>
> The size of the hid report field data structure should probably be increased.
> The data structure is treated as a u32 in Linux, but an unlimited number
> of bits in the USB hid spec, so there is some rearchitecture needed now that
> devices are sending more than 32 bits.
>
> Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 210b81a56e1a..3eaee2c37931 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1311,8 +1311,8 @@ u32 hid_field_extract(const struct hid_device *hid, u8 *report,
>                         unsigned offset, unsigned n)
>  {
>         if (n > 32) {
> -               hid_warn(hid, "hid_field_extract() called with n (%d) > 32! (%s)\n",
> -                        n, current->comm);
> +               hid_warn_once(hid, "%s() called with n (%d) > 32! (%s)\n",
> +                             __func__, n, current->comm);
>                 n = 32;
>         }
>
> --
> 2.21.0
>

Re: [PATCH v3 3/3] HID: core: fix dmesg flooding if report field larger than 32bit

[Benjamin Tissoires]

On Thu, Aug 29, 2019 at 1:26 AM Joshua Clayton <stillcompiling@gmail.com> wrote:
>
> ping?
> I'd love to see this get in.
> with distro kernel I have effectively no dmesg due to this issue

Apologies for the delay.

I really thought we should find a better way of fixing this, until I
got a laptop affected by it. This series is a must have.

Applied to for-5.4/core

Cheers,
Benjamin

>
> On Mon, Aug 12, 2019 at 9:20 AM <stillcompiling@gmail.com> wrote:
> >
> > From: Joshua Clayton <stillcompiling@gmail.com>
> >
> > Only warn once of oversize hid report value field
> >
> > On HP spectre x360 convertible the message:
> > hid-sensor-hub 001F:8087:0AC2.0002: hid_field_extract() called with n (192) > 32! (kworker/1:2)
> > is continually printed many times per second, crowding out all else.
> > Protect dmesg by printing the warning only one time.
> >
> > The size of the hid report field data structure should probably be increased.
> > The data structure is treated as a u32 in Linux, but an unlimited number
> > of bits in the USB hid spec, so there is some rearchitecture needed now that
> > devices are sending more than 32 bits.
> >
> > Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 210b81a56e1a..3eaee2c37931 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -1311,8 +1311,8 @@ u32 hid_field_extract(const struct hid_device *hid, u8 *report,
> >                         unsigned offset, unsigned n)
> >  {
> >         if (n > 32) {
> > -               hid_warn(hid, "hid_field_extract() called with n (%d) > 32! (%s)\n",
> > -                        n, current->comm);
> > +               hid_warn_once(hid, "%s() called with n (%d) > 32! (%s)\n",
> > +                             __func__, n, current->comm);
> >                 n = 32;
> >         }
> >
> > --
> > 2.21.0
> >

Re: [PATCH v3 3/3] HID: core: fix dmesg flooding if report field larger than 32bit

[Joshua Clayton]

Thanks!
It means a lot to have this accepted.
I actually started working on it, thinking "how hard can it be to
increase the size of a data structure"? It only has to be forward
compatible anyway.
My gut feeling is the existing code is working way too hard to do what
should be a memcpy, and the impulse to "fix" it is strong, despite my
absolute lack of usb-hid experience.

But the history of this little bit of code is already fraught with
complaints about big endian breakage.
I'm tempted to make it much simpler for size>32 bits (fix it only for
future users), or just way simpler for little endian,
But what do I know about usb and big endian? I sure don't have the
equipment to test it. And I worry a little I might be forgetting some
oddball non-byte-aligned data structure, which the spec would
theoretically allow.
Perhaps I'll have to time and courage to take another stab.

~Joshua

On Wed, Sep 18, 2019 at 11:35 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Thu, Aug 29, 2019 at 1:26 AM Joshua Clayton <stillcompiling@gmail.com> wrote:
> >
> > ping?
> > I'd love to see this get in.
> > with distro kernel I have effectively no dmesg due to this issue
>
> Apologies for the delay.
>
> I really thought we should find a better way of fixing this, until I
> got a laptop affected by it. This series is a must have.
>
> Applied to for-5.4/core
>
> Cheers,
> Benjamin
>
> >
> > On Mon, Aug 12, 2019 at 9:20 AM <stillcompiling@gmail.com> wrote:
> > >
> > > From: Joshua Clayton <stillcompiling@gmail.com>
> > >
> > > Only warn once of oversize hid report value field
> > >
> > > On HP spectre x360 convertible the message:
> > > hid-sensor-hub 001F:8087:0AC2.0002: hid_field_extract() called with n (192) > 32! (kworker/1:2)
> > > is continually printed many times per second, crowding out all else.
> > > Protect dmesg by printing the warning only one time.
> > >
> > > The size of the hid report field data structure should probably be increased.
> > > The data structure is treated as a u32 in Linux, but an unlimited number
> > > of bits in the USB hid spec, so there is some rearchitecture needed now that
> > > devices are sending more than 32 bits.
> > >
> > > Signed-off-by: Joshua Clayton <stillcompiling@gmail.com>
> > >
> > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > index 210b81a56e1a..3eaee2c37931 100644
> > > --- a/drivers/hid/hid-core.c
> > > +++ b/drivers/hid/hid-core.c
> > > @@ -1311,8 +1311,8 @@ u32 hid_field_extract(const struct hid_device *hid, u8 *report,
> > >                         unsigned offset, unsigned n)
> > >  {
> > >         if (n > 32) {
> > > -               hid_warn(hid, "hid_field_extract() called with n (%d) > 32! (%s)\n",
> > > -                        n, current->comm);
> > > +               hid_warn_once(hid, "%s() called with n (%d) > 32! (%s)\n",
> > > +                             __func__, n, current->comm);
> > >                 n = 32;
> > >         }
> > >
> > > --
> > > 2.21.0
> > >
>