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

[Esben Haabendal]

Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx> writes:

> Hi,
>
> Am Freitag, 13. September 2024, 10:39:50 CEST schrieb Esben Haabendal:
>> 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 <esben@xxxxxxxxxx>
>> ---
>>  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
>> lockdep_assert_held(&sport->port->lock);
> and/or sparse annoations
>> __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@xxxxxxxxxx/),
as that will change the code paths this code will be used in.

/Esben