Re: [PATCH v2 1/2] Input: xpad - query MS OS descriptor for BTP-KP20D

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The variable `error` holds positive values on success, violating subsystem naming conventions.
- [Low] Missed opportunity to use modern cleanup primitives for the descriptor buffer.
--

--- Patch [1]: [PATCH v2 1/2] Input: xpad - query MS OS descriptor for BTP-KP20D ---
commit eab43c4126d684c849995489e35829f7af7fb51c
Author: Aaron Ma <[email protected]>

Input: xpad - query MS OS descriptor for BTP-KP20D

This commit adds a quirk to query the MS OS descriptor for the Beitong
BTP-KP20D dongle during probe. This allows the device to stay in Xbox 360
mode.

> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index feb8f368f834e..1616047e65f5e 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
[ ... ]
> @@ -786,6 +788,24 @@ static int xpad_start_input(struct usb_xpad *xpad);
>  static void xpadone_ack_mode_report(struct usb_xpad *xpad, u8 seq_num);
>  static void xpad360w_poweroff_controller(struct usb_xpad *xpad);
>  
> +static void xpad_query_ms_os_descriptor(struct usb_device *udev)
> +{
> +	u8 *descriptor;
> +	int error;

[Severity: Low]
This isn't a bug, but does the error variable receive a positive value here?
The input subsystem maintainer prefers that variables named error or err
should only hold negative error codes or 0 for success. Since
usb_get_descriptor() returns the number of bytes transferred on success,
perhaps a different variable name would be better?

> +
> +	descriptor = kmalloc(18, GFP_KERNEL);

[Severity: Low]
This isn't a bug, but could this use the __free(kfree) primitive for
the allocation to automate cleanup and remove the explicit kfree(descriptor)
below, as encouraged by the maintainer preferences for new code?

> +	if (!descriptor)
> +		return;
> +
> +	error = usb_get_descriptor(udev, USB_DT_STRING, 0xee,
> +				   descriptor, 18);
> +	if (error != 18)
> +		dev_warn(&udev->dev,
> +			 "unable to query MS OS descriptor: %d\n", error);
> +
> +	kfree(descriptor);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1