linux-kernel - Re: [PATCH v2 1/5] serial: 8250: extract serial8250_init

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: <b7f6eaa3-f3f0-4cc8-b11a-1bb2daa57f5d@kernel.org>
Date: Wed, 25 Jun 2025 07:59:02 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 linux-serial <linux-serial@...r.kernel.org>,
 LKML <linux-kernel@...r.kernel.org>,
 Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH v2 1/5] serial: 8250: extract serial8250_init_mctrl()

On 24. 06. 25, 13:15, Ilpo Järvinen wrote:
> On Tue, 24 Jun 2025, Ilpo Järvinen wrote:
> 
>> On Tue, 24 Jun 2025, Jiri Slaby (SUSE) wrote:
>>
>>> After commit 795158691cc0 ("serial: 8250: extract
>>> serial8250_initialize()"), split serial8250_initialize() even more --
>>> the mctrl part of this code can be separated into
>>> serial8250_init_mctrl() -- done now.
>>>
>>> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@...nel.org>
>>> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
>>
>> Heh, I didn't even realize I was suggesting this :-D but it's good
>> nonetheless.
>>
>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
>>
>>> Cc: Andy Shevchenko <andy.shevchenko@...il.com>
>>> ---
>>> [v2]
>>> * use port-> directly.
>>> * do not remove curly braces.
>>> Both rebase errors -- noticed by Andy.
>>> ---
>>>   drivers/tty/serial/8250/8250_port.c | 21 +++++++++++++--------
>>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>>> index 48c30e158cb8..0f85a2f292fc 100644
>>> --- a/drivers/tty/serial/8250/8250_port.c
>>> +++ b/drivers/tty/serial/8250/8250_port.c
>>> @@ -2216,15 +2216,8 @@ static void serial8250_THRE_test(struct uart_port *port)
>>>   		up->bugs |= UART_BUG_THRE;
>>>   }
>>>   
>>> -static void serial8250_initialize(struct uart_port *port)
>>> +static void serial8250_init_mctrl(struct uart_port *port)
>>>   {
>>> -	struct uart_8250_port *up = up_to_u8250p(port);
>>> -	unsigned long flags;
>>> -	bool lsr_TEMT, iir_NOINT;
>>> -
>>> -	serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
>>> -
>>> -	uart_port_lock_irqsave(port, &flags);
>>>   	if (port->flags & UPF_FOURPORT) {
> 
> I should have also added what I meant with my earlier suggestion. AFAICT,
> this UPF_FOURPORT thing can only occur if SERIAL_8250_FOURPORT is enabled.
> 
> The challenge obviously are the if/else constructs but there are a few
> places that do port->flags & UPF_FOURPORT specific thing and something
> else otherwise. That hw-specific code could be placed into the hw-specific
> 8250_fourport.c file if the hw-specific function is made to return true
> if it did match, and the generic code runs otherwise.

So you can brew a patch for this, right :)? Every time I read some code 
the same you describe, I end up with 10+ patches. I don't want for now :P.

> I also have no idea why serial/sunsu.c checks UPF_FOURPORT, perhaps that's
> copy-paste in action. :-)

Yes, AFAI looked, it was during the switch of sunsu to serial driver. If 
you feel confident (it really appears apparent), drop that too ;).

thanks,
-- 
js
suse labs