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