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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87v7yz50ry.fsf@geanix.com>
Date: Fri, 13 Sep 2024 12:02:57 +0200
From: Esben Haabendal <esben@...nix.com>
To: Alexander Stein <alexander.stein@...tq-group.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,  Jiri Slaby
 <jirislaby@...nel.org>,  Shawn Guo <shawnguo@...nel.org>,  Sascha Hauer
 <s.hauer@...gutronix.de>,  Pengutronix Kernel Team
 <kernel@...gutronix.de>,  Fabio Estevam <festevam@...il.com>,  John Ogness
 <john.ogness@...uxtronix.de>,  linux-arm-kernel@...ts.infradead.org,
  linux-kernel@...r.kernel.org,  linux-serial@...r.kernel.org,
  imx@...ts.linux.dev
Subject: Re: [PATCH 2/2] serial: imx: Add more comments on port lock status

Alexander Stein <alexander.stein@...tq-group.com> 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@...nix.com>
>> ---
>>  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@geanix.com/),
as that will change the code paths this code will be used in.

/Esben