Re: [PATCH v2] staging: greybus: uart: add descriptive lock comments

[Dan Carpenter <dan.carpenter@linaro.org>]

On Fri, Feb 27, 2026 at 12:22:20PM +0530, Shubham Chakraborty wrote:
> Replace vague lock comments with specific descriptions of
> what data each lock protects:
> - read_lock: protects iocount and oldcount
> - write_lock: protects write_fifo and credits
> - mutex: protects disconnected state
> 
> Signed-off-by: Shubham Chakraborty <chakrabortyshubham66@gmail.com>
> ---

When you're writing a v2 patch, you should first start from a fresh
kernel tree.  This v2 assumes that we applied the v1 patch.

https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/

>  drivers/staging/greybus/uart.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c
> index fe554eba555a..1e51818e34a8 100644
> --- a/drivers/staging/greybus/uart.c
> +++ b/drivers/staging/greybus/uart.c
> @@ -50,12 +50,12 @@ struct gb_tty {
>  	unsigned int minor;
>  	unsigned char clocal;
>  	bool disconnected;
> -	spinlock_t read_lock;		/* protects read operations */
> -	spinlock_t write_lock;		/* protects write operations */
> +	spinlock_t read_lock;		/* protects iocount and oldcount */
> +	spinlock_t write_lock;		/* protects write_fifo and credits */
>  	struct async_icount iocount;
>  	struct async_icount oldcount;
>  	wait_queue_head_t wioctl;
> -	struct mutex mutex;		/* serializes port operations */
> +	struct mutex mutex;		/* protects disconnected state */
>  	u8 ctrlin;	/* input control lines */


To be honest, I'm likely never going to be happy with a four word
explanation of what these locks do...  Probably a paragraph would be
better.

Don't just think about it so narrowly...  For example, does the name
read_lock make sense?  What does the word "read" have anything to do
with "iocount and oldcount"?

Why do we need two separate locks for read and write?  I understand
how write means write_fifo but why do "write_fifo and credits" go
together?  If write_lock protects credits, then why are there several
places where we access gb_tty->credits without taking the lock?

Then as you go along, you're going to notice weird things.  For
example, in set_serial_info() it allows non-admin uses to set the
close_delay and closing_wait to the existing values.  What is the
point of that?  Do other .set_serial() functions allow that?  I
haven't looked, so maybe there is a good reason!  But when you
look at this code with an open mind then you'll find all kinds of
questions like that.

What does protect really mean?  What would happen if we removed
the locking?  Is the locking even correct?  If read_lock "protects
iocount" then why do we not take it in gb_tty_get_icount()?

The locking around disconnect probably is meant to prevent a use after
free.  What are all the variables we allocate and how do we free them?
Does the disconnect process work?  Go through the probe make a list
of allocations.  When is the earliest we can call disconnect?  What
if we set ->disconnected = true but we haven't called
gb_connection_disable_rx() so we're still receiving packets?  I haven't
looked at it so I don't know!

regards,
dan carpenter