Message-ID: <2474ab138c5fbea089bd787d7a885a96e6a57bd1.camel@hadess.net>
Subject: Re: [PATCH] HID: use standard debug APIs
From:   Bastien Nocera <hadess@hadess.net>
To:     Thomas =?ISO-8859-1?Q?Wei=DFschuh?= <linux@weissschuh.net>,
        Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc:     linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Thu, 02 Feb 2023 11:15:17 +0100
In-Reply-To: <20230129215352.oglktngi2rvp6sfd@t-8ch.de>
References: <20221223-hid-dbg-v1-1-5dcf8794f7f9@weissschuh.net>
         <20230129215352.oglktngi2rvp6sfd@t-8ch.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Sun, 2023-01-29 at 21:53 +0000, Thomas Weißschuh wrote:
> Hi Jiri, hi Benjamin,
> 
> On Fri, Dec 23, 2022 at 09:30:19PM +0000, Thomas Weißschuh wrote:
> > The custom "debug" module parameter is fairly inflexible.
> > It can only manage debugging for all calls dbg_hid() at the same
> > time.
> > 
> > Furthermore it creates a mismatch between calls to hid_dbg() which
> > can
> > be managed by CONFIG_DYNAMIC_DEBUG and dbg_hid() which is managed
> > by the
> > module parameter.
> > 
> > Furthermore the change to pr_debug() allows the debugging
> > statements to
> > be completely compiled-out if desired.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> > 
> > Note: This removes the possibility to enable debugging for the HID
> > core
> > and all drivers at the same time.
> > If this is still desirable it could probably be implemented with
> > the new
> > DYNAMIC_DEBUG class feature.
> > ---
> >  drivers/hid/hid-core.c | 9 ---------
> >  include/linux/hid.h    | 8 +-------
> >  2 files changed, 1 insertion(+), 16 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index bd47628da6be..4facfb446986 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -41,11 +41,6 @@
> >  
> >  #define DRIVER_DESC "HID core driver"
> >  
> > -int hid_debug = 0;
> > -module_param_named(debug, hid_debug, int, 0600);
> > -MODULE_PARM_DESC(debug, "toggle HID debugging messages");
> > -EXPORT_SYMBOL_GPL(hid_debug);
> > -
> >  static int hid_ignore_special_drivers = 0;
> >  module_param_named(ignore_special_drivers,
> > hid_ignore_special_drivers, int, 0600);
> >  MODULE_PARM_DESC(ignore_special_drivers, "Ignore any special
> > drivers and handle all devices by generic driver");
> > @@ -2909,10 +2904,6 @@ static int __init hid_init(void)
> >  {
> >         int ret;
> >  
> > -       if (hid_debug)
> > -               pr_warn("hid_debug is now used solely for parser
> > and driver debugging.\n"
> > -                       "debugfs is now used for inspecting the
> > device (report descriptor, reports)\n");
> > -
> >         ret = bus_register(&hid_bus_type);
> >         if (ret) {
> >                 pr_err("can't register hid bus\n");
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index 8677ae38599e..676f501507aa 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -882,8 +882,6 @@ static inline bool hid_is_usb(struct hid_device
> > *hdev)
> >  
> >  /* HID core API */
> >  
> > -extern int hid_debug;
> > -
> >  extern bool hid_ignore(struct hid_device *);
> >  extern int hid_add_device(struct hid_device *);
> >  extern void hid_destroy_device(struct hid_device *);
> > @@ -1191,11 +1189,7 @@ int hid_pidff_init(struct hid_device *hid);
> >  #define hid_pidff_init NULL
> >  #endif
> >  
> > -#define dbg_hid(fmt,
> > ...)                                              \
> > -do
> > {                                                                  
> >  \
> > -       if
> > (hid_debug)                                                  \
> > -               printk(KERN_DEBUG "%s: " fmt, __FILE__,
> > ##__VA_ARGS__); \
> > -} while (0)
> > +#define dbg_hid(fmt, ...) pr_debug("%s: " fmt, __FILE__,
> > ##__VA_ARGS__)
> >  
> >  #define hid_err(hid, fmt, ...)                         \
> >         dev_err(&(hid)->dev, fmt, ##__VA_ARGS__)
> 
> any feedback on this patch?
> 
> Please note that this is *not* the same as the already merged
> 34ba3657a503 ("HID: i2c-hid: switch to standard debugging APIs")

You can add:
Tested-by: Bastien Nocera <hadess@hadess.net>
to this one

I tested it as a way to debug the Logitech G903 regression.

I should note that it's pretty weird to have a dbg_hid() and a
hid_dbg() that do pretty much the same thing, but that was a pre-
existing problem.

> 
> > ---
> > base-commit: 51094a24b85e29138b7fa82ef1e1b4fe19c90046
> > change-id: 20221223-hid-dbg-2f3eeddddd53
> > 
> > Best regards,
> > -- 
> > Thomas Weißschuh <linux@weissschuh.net>
> 
> Thanks,
> Thomas