Re: [PATCH v9 01/10] serial: sc16is7xx: fix broken port 0 uart init

[Hugo Villeneuve <hugo@xxxxxxxxxxx>]

On Thu, 3 Aug 2023 09:54:37 +0200
Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:

> On Tue, Aug 01, 2023 at 01:16:55PM -0400, Hugo Villeneuve wrote:
> > On Mon, 31 Jul 2023 17:52:26 +0200
> > Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > 
> > > On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> > > > From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> > > > 
> > > > The sc16is7xx_config_rs485() function is called only for the second
> > > > port (index 1, channel B), causing initialization problems for the
> > > > first port.
> > > > 
> > > > For the sc16is7xx driver, port->membase and port->mapbase are not set,
> > > > and their default values are 0. And we set port->iobase to the device
> > > > index. This means that when the first device is registered using the
> > > > uart_add_one_port() function, the following values will be in the port
> > > > structure:
> > > >     port->membase = 0
> > > >     port->mapbase = 0
> > > >     port->iobase  = 0
> > > > 
> > > > Therefore, the function uart_configure_port() in serial_core.c will
> > > > exit early because of the following check:
> > > > 	/*
> > > > 	 * If there isn't a port here, don't do anything further.
> > > > 	 */
> > > > 	if (!port->iobase && !port->mapbase && !port->membase)
> > > > 		return;
> > > > 
> > > > Typically, I2C and SPI drivers do not set port->membase and
> > > > port->mapbase.
> > > > 
> > > > The max310x driver sets port->membase to ~0 (all ones). By
> > > > implementing the same change in this driver, uart_configure_port() is
> > > > now correctly executed for all ports.
> > > > 
> > > > Fixes: dfeae619d781 ("serial: sc16is7xx")
> > > 
> > > That commit is in a very old 3.x release.
> > > 
> > > > Cc: <stable@xxxxxxxxxxxxxxx> # 6.1.x
> > > 
> > > But you say this should only go to 6.1.y?  Why?  What is wrong with the
> > > older kernels?
> > 
> > Hi Greg,
> > I have read (and reread a couple of times)
> > Documentation/process/stable-kernel-rules.rst to try to understand how
> > to format the tags, but unfortunately it doesn't contain "Everything
> > you ever wanted to know about Linux -stable releases" as the title
> > claims :)
> > 
> > In particular, it doesn't explain or advise which older version we
> > should target, that is why since I was not sure I specified 6.1.y
> > because I could test it properly, but not v3.x.
> 
> If you think this fixes an issue back to 3.x, then just leave it at
> that, there's no need to have to test all of these.  If when I apply the
> patch to the stable trees, and it does not go back to all of the
> active versions specified by Fixes: then you will get an email saying
> so and can handle it then if you want to.
> 
> > Maybe it would be best to simply drop for now all the "Cc:
> > <stable@xxxxxxxxxxxxxxx>" tags for this series, and following Option 2,
> > I send an email to stable@xxxxxxxxxxxxxxx once the patches have been
> > merged to Linus' tree?
> 
> That will just mean more work for both of us, leave it as is, just drop
> the "# 6.1.x" portion please.
> 
> > > > Signed-off-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > > > Reviewed-by: Lech Perczak <lech.perczak@xxxxxxxxxxxxxxx>
> > > > Tested-by: Lech Perczak <lech.perczak@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/tty/serial/sc16is7xx.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > > > index 2e7e7c409cf2..8ae2afc76a9b 100644
> > > > --- a/drivers/tty/serial/sc16is7xx.c
> > > > +++ b/drivers/tty/serial/sc16is7xx.c
> > > > @@ -1436,6 +1436,7 @@ static int sc16is7xx_probe(struct device *dev,
> > > >  		s->p[i].port.fifosize	= SC16IS7XX_FIFO_SIZE;
> > > >  		s->p[i].port.flags	= UPF_FIXED_TYPE | UPF_LOW_LATENCY;
> > > >  		s->p[i].port.iobase	= i;
> > > > +		s->p[i].port.membase	= (void __iomem *)~0;
> > > 
> > > That's a magic value, some comment should be added here to explain why
> > > setting all bits is ok.  Why does this work exactly?  You only say that
> > > the max310x driver does this, but not why it does this at all.
> > 
> > I do not understand, because my commit log message is quite long
> > and, it seems to me, well documenting why this works the way it
> > does when calling uart_configure_port() in serial_core.c?
> > 
> > I say that the max310x driver also does this, because there is also no
> > comment in the max310x driver for using the (void __iomem *)~0;
> > construct. I also located the original commit message for the max310x
> > driver but no comments were usefull there also.
> > 
> > So, what about adding this comment:
> > 
> > /*
> >  * Use all ones as membase to make sure uart_configure_port() in
> >  * serial_core.c does not abort for SPI/I2C devices where the
> >  * membase address is not applicable.
> >  */
> >  s->p[i].port.membase	= (void __iomem *)~0;
> 
> Yes, that would be good, thank you.
> 
> > If wou want, I could also add the same comment to the max310 driver?
> 
> Yes please.

Hi Greg,
I will send a separate patch specifically for this.

Thank you, Hugo.