From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 799D62D94B5 for ; Fri, 27 Feb 2026 09:03:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.45 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772182989; cv=none; b=aOEH/Q3IPHIkH9JOXrebUpQzFqarVjIVhcKrN9NzxPi2h43m+5yT56PKpClrxgzFb8L9TfpQYWUsclSQnAYDEaUbvo2WkPRR49U8Jb6ManyEyBvXura27Ri3Ul7tnN6u6dshJliARH9CJonaLQZwvW2IB2FcPZ+eGcpQI0rf+c4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772182989; c=relaxed/simple; bh=/rdJVoGRmXCk0Y9KPBj+F9umll76N+hl7u2yJyEUBnM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=K/m1R5wid8/mXk2eExIju+o8KhH8gXIfyRtc2EHxtsb0YfqqhrK1PsgidAAqQQGetxptp1SZB+dVLsCMLPv1S/r579fqxXbPsWyCYikEATox5y6KldH4nTK/W54quG7FYTDY1Ymi4Yp+XAVwidwAEZf8cTM7e/are8xdIMN5Q8c= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=Bj+33bCA; arc=none smtp.client-ip=209.85.128.45 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="Bj+33bCA" Received: by mail-wm1-f45.google.com with SMTP id 5b1f17b1804b1-4834826e5a0so19230675e9.2 for ; Fri, 27 Feb 2026 01:03:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1772182987; x=1772787787; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=LK3kLvJU/BNP7pb7k7IBNvzbVws1LlhpsC95zzr3Y4E=; b=Bj+33bCAuIYYIMfe+GaVw/IBmjb3WUHGmahe/KHlqq6ObtGZ6HSBW4EUaTv3ExbT83 vxOmpP9pg3HF1Ahgr6BfUqe7mNa/YTTpru5ss86hIFuW6m941qkRfrqnUpveGkhSwFgZ 5vKCxulsZxeRcbzzVRCZQx6TxZVUWP1HcDZ/6OgmCeF3+g9UZ4Ldv93W7mQ+4fcsgRJB FAyKW1idFrqqzetGbbU7lTQncD8h+vGBEPm+aVQw2bv23No4R+CFbs3RGABY+YA3tObi NaeYBnRPg2lCEzcXZ40SiAQKaZiMnjtyl35D8ehBDkROiSUNHWli9l6OnOJnzSs9++/i YRZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1772182987; x=1772787787; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=LK3kLvJU/BNP7pb7k7IBNvzbVws1LlhpsC95zzr3Y4E=; b=rBhMzHu1SSnmuR2D1dPsHHj3YX2zByj3GrgGt4xU/aYQuDen+GTu5CVv1lWy/fQk+W b7d2vEMSWzXkUb8Y65+Qw71jPCBhC8X+A5M1FTT/HhKDdmFCPO8BYmIiXuwL6Yc7FCZC SQ2ztBEzdMvZLsNIt2z2M7spo+ZTx+NeuIjB17vkah/8kStsn6DmDvURwHOXYwcE9e8g 1FDawdKA9Sg6KhzXhsAxjtKAeFF0Cs0yqDdNbjBfK4dj/oMJUyhvGqf+QHrHlu0DkWa3 ApAEGPkSelMJ7sghY6PNxhXB8XYoQXgaLmjHYG5FSAOamYHyCzxx9udXyZiMiWIXcsrk Ua/g== X-Forwarded-Encrypted: i=1; AJvYcCXoqE8n/3NZG/4+PDSNnKTX3CqDuslTZKJS9BygF5s4iuoldmaH2/fZkdnjs7zvPZiUyEV//eXJB1D7P4Q=@vger.kernel.org X-Gm-Message-State: AOJu0Yzexe9qVcDnCZsopPoH/jpZ1v+iLkKACldH1prjjlrpQ0uAKjxx NfI9dH9Ici49P24fnjDyCRXMWRQQGtlpj8/OyP5UvrnM/BRZbIt42byjWiWc3+TWABLYYpzgMFQ BRId+ X-Gm-Gg: ATEYQzxVOn34/1Fwua3NHPBwnoA7rX67VIMmq2qfluyG1q4fLrqWGUPLPDc/WKjpZq7 x9kgKs7WDxQ4xWfuCpKMUwgpu0V7hEvNhBrlItySlEjTMizrEDG5OKIUvvAsRN373akLaDzmKe2 g9GQfUkdbLhyoL9FRCqVeI17rtXhZmQ12F+5FhnlXw+OPJ3KpwimWYf4JoTkDJIQZ1fUnUjSaVX QKdxfNfYKaGg9hakggAjY85yU2AQJ1k1P5HK81O1ibG+zFS0wgqt/Ck47dnVzRHZBO9HIk1/mTe b6hyIZ6JJQEy7UNfQcSqUc6kLU1nBT0sOK1NrqgqKV9j98B6nqgE3s2iCAE/M/SXqX3Xa+OXJrp fUFQ5bZ9F607LS1K47DQAG9VP2fxRmke80d+R62OoMskBw5CGjZphINQ7BqTnpN2AvLKG61pAOY st9sm9fDKqyrWjTGWa8Ma5XzAdqnd4 X-Received: by 2002:a05:600c:3512:b0:480:7162:fa48 with SMTP id 5b1f17b1804b1-483c9bb1fcfmr29700645e9.13.1772182986616; Fri, 27 Feb 2026 01:03:06 -0800 (PST) Received: from localhost ([196.207.164.177]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-483c3b3d24dsm80363625e9.5.2026.02.27.01.03.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 27 Feb 2026 01:03:06 -0800 (PST) Date: Fri, 27 Feb 2026 12:03:02 +0300 From: Dan Carpenter To: Shubham Chakraborty Cc: gregkh@linuxfoundation.org, dtwlin@gmail.com, johan@kernel.org, elder@kernel.org, greybus-dev@lists.linaro.org, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] staging: greybus: uart: add descriptive lock comments Message-ID: References: <20260227065220.8039-1-chakrabortyshubham66@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260227065220.8039-1-chakrabortyshubham66@gmail.com> 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 > --- 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