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
