Re: [PATCH v2] usb: adutux: take buflock when resetting read_buffer_length in adu_open()

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Jul 13, 2026 at 01:26:16PM +0530, Kanishka De Silva wrote:
> dev->read_buffer_length is otherwise only ever touched under
> dev->buflock (by adu_interrupt_in_callback() and adu_read()), per the
> locking scheme documented in the comment above struct adu_device.
> adu_open() resets it to 0 without holding buflock.
> 
> In the current code this is not a reachable race. adu_open() and
> adu_release() are fully serialized by adutux_mutex, and
> adu_release_internal() calls usb_kill_urb() (via
> adu_abort_transfers()) before that mutex is dropped, which blocks
> until any in-flight adu_interrupt_in_callback() has returned. The
> write in adu_open() also precedes the urb (re)submission in program
> order, so the callback cannot observe or race with it there either.
> 
> This change brings the assignment under buflock purely so the
> field's locking is locally consistent with the driver's documented
> scheme, making the invariant easy to verify without having to reason
> across adu_open(), adu_release_internal(), and usb_kill_urb()'s
> blocking semantics. No behavioral or functional change intended.
> 
> v2:
>  - Retitled and reframed from "fix unlocked read_buffer_length write
>    in adu_open() (data race)" to a lock-discipline consistency
>    change. Discussion on the RFC (with Oliver Neukum) established
>    that adutux_mutex serialization plus usb_kill_urb()'s blocking
>    semantics rule out any runtime-reachable race here, so this is
>    no longer presented as a bugfix. (Oliver Neukum, Greg
>    Kroah-Hartman)

Version info goes below the --- line please.

> 
> Signed-off-by: Kanishka De Silva <kpskanna1915@gmail.com>
> ---
>  drivers/usb/misc/adutux.c | 3 ++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
> index 1111111..2222222 100644
> --- a/drivers/usb/misc/adutux.c
> +++ b/drivers/usb/misc/adutux.c
> @@ -337,7 +337,8 @@ static int adu_open(struct inode *inode, struct file *file)
>  	file->private_data = dev;
>  
>  	/* initialize in direction */
> -	dev->read_buffer_length = 0;
> +	spin_lock_irq(&dev->buflock);
> +	dev->read_buffer_length = 0;
> +	spin_unlock_irq(&dev->buflock);

This makes no sense, the single write here isn't going to need a lock,
it can't "tear" and if someone else is doing something with it it can
change right after the lock is released, right?

So what exactly is this supposed to be "fixing"?

Ah, your changelog says "not really fixing anything", so then it's not
needed at all, so why add this at all?

thanks,

greg k-h