diff mbox series

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

Message ID 20230725142343.1724130-2-hugo@hugovil.com
State New
Headers
Series serial: sc16is7xx: fix GPIO regression and rs485 improvements |

Commit Message

Hugo Villeneuve July 25, 2023, 2:23 p.m. UTC 
  From: Hugo Villeneuve <hvilleneuve@dimonoff.com>

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")
Cc: <stable@vger.kernel.org> # 6.1.x
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
---
 drivers/tty/serial/sc16is7xx.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Greg KH July 31, 2023, 3:52 p.m. UTC | #1 
On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> 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@vger.kernel.org> # 6.1.x

But you say this should only go to 6.1.y?  Why?  What is wrong with the
older kernels?

> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> ---
>  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.

confused,

greg k-h
Hugo Villeneuve Aug. 1, 2023, 5:16 p.m. UTC | #2 
On Mon, 31 Jul 2023 17:52:26 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > 
> > 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@vger.kernel.org> # 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.

Maybe it would be best to simply drop for now all the "Cc:
<stable@vger.kernel.org>" tags for this series, and following Option 2,
I send an email to stable@vger.kernel.org once the patches have been
merged to Linus' tree?


> > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > ---
> >  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;


Also, keep in mind that in the original discussion about that patch,
there was the following comment from Ilpo Järvinen:

------
This changelog was really well describing the problem! :-)
Yeah, some other solution should be devised. Maybe we should add
another .iotype for thse kind of devices. But I'm fine with this for
this fix. After editing the changelog, feel free to add:
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
------

If wou want, I could also add the same comment to the max310 driver?

Hugo.
Greg KH Aug. 3, 2023, 7:54 a.m. UTC | #3 
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@linuxfoundation.org> wrote:
> 
> > On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > 
> > > 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@vger.kernel.org> # 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@vger.kernel.org>" tags for this series, and following Option 2,
> I send an email to stable@vger.kernel.org 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@dimonoff.com>
> > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > ---
> > >  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.

thanks,

greg k-h
Hugo Villeneuve Aug. 3, 2023, 1:04 p.m. UTC | #4 
On Thu, 3 Aug 2023 09:54:37 +0200
Greg KH <gregkh@linuxfoundation.org> 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@linuxfoundation.org> wrote:
> > 
> > > On Tue, Jul 25, 2023 at 10:23:33AM -0400, Hugo Villeneuve wrote:
> > > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > 
> > > > 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@vger.kernel.org> # 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@vger.kernel.org>" tags for this series, and following Option 2,
> > I send an email to stable@vger.kernel.org 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@dimonoff.com>
> > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com>
> > > > ---
> > > >  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.
diff mbox series

Patch

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;
 		s->p[i].port.iotype	= UPIO_PORT;
 		s->p[i].port.uartclk	= freq;
 		s->p[i].port.rs485_config = sc16is7xx_config_rs485;