Re: [PATCH] HID: use standard debug APIs

[Thomas Weißschuh]

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@xxxxxxxxxxxxxx>
> ---
> 
> 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")

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

Thanks,
Thomas