Re: [PATCH 2/2] serial: imx: Add more comments on port lock status

Alexander Stein [off-list ref] writes:

Hi,

Am Freitag, 13. September 2024, 10:39:50 CEST schrieb Esben Haabendal:

quoted

Comments regarding status of port.lock on internal functions is useful when
reviewing correct handling of registers that must be protected by this
lock.

Signed-off-by: Esben Haabendal <redacted>
---
 drivers/tty/serial/imx.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index efa3eb3a2c57..bea4510743ef 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c

@@ -370,6 +370,7 @@ static void imx_uart_soft_reset(struct imx_port *sport)
 	sport->idle_counter = 0;
 }
 
+/* called with port.lock taken and irqs off */
 static void imx_uart_disable_loopback_rs485(struct imx_port *sport)
 {
 	unsigned int uts;

I think you are referring to sport.lock.

Yes.

On the other hand, instead of just adding comments, wouldn't it be
better to make it explicit?
Adding

quoted

lockdep_assert_held(&sport->port->lock);

and/or sparse annoations

quoted

__must_hold(&sport->port->lock)

seems more reasonable to me than adding non-enforcing comments.

I fear that due to the way that legacy console works, assertations might
trigger in special situations, such as printk during panic.

Converting comments to assertations could definitely be a good idea, but
I think it might be better to wait with that until the driver has been
converted to NBCON (in progress, see
https://lore.kernel.org/all/20240913-serial-imx-nbcon-v3-1-4c627302335b@geanix.com/ (local)),
as that will change the code paths this code will be used in.

/Esben