Re: [PATCH v2 2/2] usb: uas: add US_FL_QDEPTH_075 quirk to cap queue depth at probe time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb-storage: truncation of US_FL_QDEPTH_075 on 32-bit architectures
- [Medium] usb-storage: UAS queue depth cap overrides and violates 2-tag reserve for small qdepths
--

commit e6046cf877ab5ed09e6d0dc6b5f9a570cdfb3bd0
Author: Luca Cecchi <luca.cecchi.info@gmail.com>

usb: uas: add US_FL_QDEPTH_075 quirk to cap queue depth at probe time

This patch introduces the US_FL_QDEPTH_075 quirk to cap the queue depth of
specific USB attached SCSI devices at 3/4 of their reported maximum during
probe time. It applies this quirk to the Lexar ES3 enclosure to prevent
firmware lockups under sustained deep command queueing.

> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index f745c9ee02d35..975598c6a6613 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -1085,6 +1085,16 @@ static int uas_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	 */
>  	shost->can_queue = devinfo->qdepth - 2;
>  
> +	/*
> +	 * Some bridge chips lock up under sustained deep command queueing
> +	 * even within the margin above. For those, US_FL_QDEPTH_075 caps
> +	 * the depth to 3/4 of the device's reported maximum at probe time,
> +	 * closing the window a udev-triggered .change_queue_depth write
> +	 * would otherwise leave open between probe and udev acting.
> +	 */
> +	if (devinfo->flags & US_FL_QDEPTH_075)
> +		shost->can_queue = (devinfo->qdepth * 3) / 4;

[Severity: Medium]
Does this unconditionally override the previously calculated safe queue depth,
violating the 2-tag reserve for devices with small queue depths?

If a device or host controller limits qdepth to a small value, for example 4,
the original logic safely restricts can_queue to 2 to reserve 1 tag for
untagged commands and 1 tag for firmware off-by-one errors.

With this change, (4 * 3) / 4 evaluates to 3, which accidentally inflates
can_queue instead of capping it, leaving only 1 tag reserved.

Could this use min_t(int, shost->can_queue, (devinfo->qdepth * 3) / 4) to
ensure the baseline reservations are always respected?

> diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h
> index d6f86d5db3bf2..d41f206faea13 100644
> --- a/drivers/usb/storage/unusual_uas.h
> +++ b/drivers/usb/storage/unusual_uas.h
[ ... ]
> +/* Reported-by: Luca Cecchi <luca.cecchi.info@gmail.com> */
> +UNUSUAL_DEV(0x21c4, 0x0003, 0x0000, 0x9999,
> +		"Lexar",
> +		"ES3",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_QDEPTH_075),

[Severity: High]
Will the US_FL_QDEPTH_075 quirk be silently discarded on 32-bit architectures?

The usb_device_id struct uses kernel_ulong_t for the driver_info field,
which is a 32-bit unsigned long on 32-bit platforms.

Because US_FL_QDEPTH_075 is defined as a 33-bit value:

include/linux/usb_usual.h:
    US_FLAG(QDEPTH_075, 0x100000000ULL)

When UNUSUAL_DEV sets this flag in the table here, casting the 64-bit value
to kernel_ulong_t will silently truncate the upper bits to 0 on 32-bit
platforms. This leaves affected devices exposed to the firmware lockups this
patch intends to fix.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008122604.1862534-1-luca.cecchi.info@gmail.com?part=2