Re: [PATCH v4 1/2] ALSA: usb-audio: expose the Topping M62's analogue gains as mixer controls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Memory reclaim deadlock potential due to GFP_KERNEL usage in system suspend/resume path.
- [Low] Bogus URB resubmit error logged during driver unbind due to unhandled -EPERM.
--

--- Patch [1]: [PATCH v4 1/2] ALSA: usb-audio: expose the Topping M62's analogue gains as mixer controls ---
commit 08a702649defd38485537f408f6392de558fea68
Author: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>

ALSA: usb-audio: expose the Topping M62's analogue gains as mixer controls

This commit adds mixer controls for the Topping M62 audio interface by reading
and writing to its vendor HID channel. It introduces a new file to handle the
specific protocol, managing the device's state via USB interrupt URBs. It claims
the HID interface and handles suspend/resume by resubmitting the URBs and
renewing the subscription.

> diff --git a/sound/usb/mixer_topping.c b/sound/usb/mixer_topping.c
> new file mode 100644
> index 0000000000000..2f2365b9b4762
> --- /dev/null
> +++ b/sound/usb/mixer_topping.c
[ ... ]
> +static int topping_send(struct topping_mixer *tm, u8 target, u8 prop,
> +			s32 value)
> +{
> +	u8 *buf __free(kfree) = kzalloc(TOPPING_EP_BUF, GFP_KERNEL);

[Severity: Medium]
Could this trigger a memory reclaim deadlock?

During system resume, the PM core calls the USB driver's resume callback, which
executes topping_resume(). topping_resume() then invokes topping_send(), which
allocates memory with GFP_KERNEL here.

Since this happens in the suspend/resume path, allocating with GFP_KERNEL could
trigger memory reclaim that waits on block devices that are still suspended.
Should this use GFP_NOIO instead?

[ ... ]
> +static void topping_urb_complete(struct urb *urb)
> +{
[ ... ]
> +resubmit:
> +	err = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (err < 0 && err != -ENODEV && err != -ESHUTDOWN)
> +		usb_audio_err(tm->mixer->chip,
> +			      "Topping: cannot resubmit: %d\n", err);
> +}

[Severity: Low]
Does this error path log a bogus error during driver unbind?

If usb_kill_urb() is called concurrently during device unplug, it sets
urb->reject. When this completion handler subsequently attempts to resubmit the
URB, it will fail with -EPERM.

Should this explicit check for -ENODEV and -ESHUTDOWN also include -EPERM to
avoid logging a spurious error message when the device is removed?

[ ... ]
> +static int topping_resume(struct usb_mixer_interface *mixer)
> +{
> +	struct topping_mixer *tm = mixer->private_data;
> +	int err;
> +
> +	if (!tm)
> +		return 0;
> +
> +	if (tm->urb) {
> +		err = usb_submit_urb(tm->urb, GFP_KERNEL);

[Severity: Medium]
Similar to the allocation in topping_send(), could submitting this URB with
GFP_KERNEL in the resume path also trigger a memory reclaim deadlock?

Should this use GFP_NOIO instead, since it is called synchronously during
system resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823222946.171345-1-mikhail.v.gavrilov@gmail.com?part=1