[PATCH v1] serial: imx: Initialize lock for non-registered console

[PATCH v1] serial: imx: Initialize lock for non-registered console

[Andy Shevchenko]

The commit a3cb39d258ef
("serial: core: Allow detach and attach serial device for console")
changed a bit logic behind lock initialization since for most of the console
driver it's supposed to have lock already initialized even if console is not
enabled. However, it's not the case for Freescale IMX console.

Initialize lock explicitly in the ->probe().

Note, there is still an open question should or shouldn't not this driver
register console properly.

Fixes: a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/imx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 986d902fb7fe..6b078e395931 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2404,6 +2404,9 @@ static int imx_uart_probe(struct platform_device *pdev)
 		}
 	}
 
+	/* We need to initialize lock even for non-registered console */
+	spin_lock_init(&sport->port.lock);
+
 	imx_uart_ports[sport->port.line] = sport;
 
 	platform_set_drvdata(pdev, sport);
-- 
2.26.2

Re: [PATCH v1] serial: imx: Initialize lock for non-registered console

[Guenter Roeck]

On 5/25/20 3:59 AM, Andy Shevchenko wrote:
> The commit a3cb39d258ef
> ("serial: core: Allow detach and attach serial device for console")
> changed a bit logic behind lock initialization since for most of the console
> driver it's supposed to have lock already initialized even if console is not
> enabled. However, it's not the case for Freescale IMX console.
> 
> Initialize lock explicitly in the ->probe().
> 
> Note, there is still an open question should or shouldn't not this driver
> register console properly.
> 
> Fixes: a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I really wonder if this is the correct fix, especially since it looks like
there are several other drivers which don't call register_console() either
but implement a console device.

Guenter

> ---
>  drivers/tty/serial/imx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 986d902fb7fe..6b078e395931 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -2404,6 +2404,9 @@ static int imx_uart_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	/* We need to initialize lock even for non-registered console */
> +	spin_lock_init(&sport->port.lock);
> +
>  	imx_uart_ports[sport->port.line] = sport;
>  
>  	platform_set_drvdata(pdev, sport);
> 

Re: [PATCH v1] serial: imx: Initialize lock for non-registered console

[Greg Kroah-Hartman]

On Mon, May 25, 2020 at 07:01:13AM -0700, Guenter Roeck wrote:
> On 5/25/20 3:59 AM, Andy Shevchenko wrote:
> > The commit a3cb39d258ef
> > ("serial: core: Allow detach and attach serial device for console")
> > changed a bit logic behind lock initialization since for most of the console
> > driver it's supposed to have lock already initialized even if console is not
> > enabled. However, it's not the case for Freescale IMX console.
> > 
> > Initialize lock explicitly in the ->probe().
> > 
> > Note, there is still an open question should or shouldn't not this driver
> > register console properly.
> > 
> > Fixes: a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I really wonder if this is the correct fix, especially since it looks like
> there are several other drivers which don't call register_console() either
> but implement a console device.
> 
> Guenter
> 
> > ---
> >  drivers/tty/serial/imx.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 986d902fb7fe..6b078e395931 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -2404,6 +2404,9 @@ static int imx_uart_probe(struct platform_device *pdev)
> >  		}
> >  	}
> >  
> > +	/* We need to initialize lock even for non-registered console */
> > +	spin_lock_init(&sport->port.lock);

Why are we having to do this for all console drivers recently?
Shouldn't the original patch that required this gone through and fixed
up all drivers?

I'm with Guenter here, let's fix them all at once please.

thanks,

greg k-h

Re: [PATCH v1] serial: imx: Initialize lock for non-registered console

[Guenter Roeck]

On 5/25/20 7:07 AM, Greg Kroah-Hartman wrote:
> On Mon, May 25, 2020 at 07:01:13AM -0700, Guenter Roeck wrote:
>> On 5/25/20 3:59 AM, Andy Shevchenko wrote:
>>> The commit a3cb39d258ef
>>> ("serial: core: Allow detach and attach serial device for console")
>>> changed a bit logic behind lock initialization since for most of the console
>>> driver it's supposed to have lock already initialized even if console is not
>>> enabled. However, it's not the case for Freescale IMX console.
>>>
>>> Initialize lock explicitly in the ->probe().
>>>
>>> Note, there is still an open question should or shouldn't not this driver
>>> register console properly.
>>>
>>> Fixes: a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
>>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>
>> I really wonder if this is the correct fix, especially since it looks like
>> there are several other drivers which don't call register_console() either
>> but implement a console device.
>>
>> Guenter
>>
>>> ---
>>>  drivers/tty/serial/imx.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>>> index 986d902fb7fe..6b078e395931 100644
>>> --- a/drivers/tty/serial/imx.c
>>> +++ b/drivers/tty/serial/imx.c
>>> @@ -2404,6 +2404,9 @@ static int imx_uart_probe(struct platform_device *pdev)
>>>  		}
>>>  	}
>>>  
>>> +	/* We need to initialize lock even for non-registered console */
>>> +	spin_lock_init(&sport->port.lock);
> 
> Why are we having to do this for all console drivers recently?
> Shouldn't the original patch that required this gone through and fixed
> up all drivers?
> 

Good that I am getting paid for bisecting and reporting issues like this.

Oh, wait, I am not. Bummer.

Guenter

Re: [PATCH v1] serial: imx: Initialize lock for non-registered console

[Andy Shevchenko]

On Mon, May 25, 2020 at 04:07:52PM +0200, Greg Kroah-Hartman wrote:
> On Mon, May 25, 2020 at 07:01:13AM -0700, Guenter Roeck wrote:
> > On 5/25/20 3:59 AM, Andy Shevchenko wrote:
> > > The commit a3cb39d258ef
> > > ("serial: core: Allow detach and attach serial device for console")
> > > changed a bit logic behind lock initialization since for most of the console
> > > driver it's supposed to have lock already initialized even if console is not
> > > enabled. However, it's not the case for Freescale IMX console.
> > > 
> > > Initialize lock explicitly in the ->probe().
> > > 
> > > Note, there is still an open question should or shouldn't not this driver
> > > register console properly.
> > > 
> > > Fixes: a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
> > > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > I really wonder if this is the correct fix, especially since it looks like
> > there are several other drivers which don't call register_console() either
> > but implement a console device.
> > 
> > Guenter
> > 
> > > ---
> > >  drivers/tty/serial/imx.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > index 986d902fb7fe..6b078e395931 100644
> > > --- a/drivers/tty/serial/imx.c
> > > +++ b/drivers/tty/serial/imx.c
> > > @@ -2404,6 +2404,9 @@ static int imx_uart_probe(struct platform_device *pdev)
> > >  		}
> > >  	}
> > >  
> > > +	/* We need to initialize lock even for non-registered console */
> > > +	spin_lock_init(&sport->port.lock);
> 
> Why are we having to do this for all console drivers recently?
> Shouldn't the original patch that required this gone through and fixed
> up all drivers?
> 
> I'm with Guenter here, let's fix them all at once please.

Okay. Do you want series per driver or one for all?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] serial: imx: Initialize lock for non-registered console

[Greg Kroah-Hartman]

On Mon, May 25, 2020 at 06:34:18PM +0300, Andy Shevchenko wrote:
> On Mon, May 25, 2020 at 04:07:52PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, May 25, 2020 at 07:01:13AM -0700, Guenter Roeck wrote:
> > > On 5/25/20 3:59 AM, Andy Shevchenko wrote:
> > > > The commit a3cb39d258ef
> > > > ("serial: core: Allow detach and attach serial device for console")
> > > > changed a bit logic behind lock initialization since for most of the console
> > > > driver it's supposed to have lock already initialized even if console is not
> > > > enabled. However, it's not the case for Freescale IMX console.
> > > > 
> > > > Initialize lock explicitly in the ->probe().
> > > > 
> > > > Note, there is still an open question should or shouldn't not this driver
> > > > register console properly.
> > > > 
> > > > Fixes: a3cb39d258ef ("serial: core: Allow detach and attach serial device for console")
> > > > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > 
> > > I really wonder if this is the correct fix, especially since it looks like
> > > there are several other drivers which don't call register_console() either
> > > but implement a console device.
> > > 
> > > Guenter
> > > 
> > > > ---
> > > >  drivers/tty/serial/imx.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > > > index 986d902fb7fe..6b078e395931 100644
> > > > --- a/drivers/tty/serial/imx.c
> > > > +++ b/drivers/tty/serial/imx.c
> > > > @@ -2404,6 +2404,9 @@ static int imx_uart_probe(struct platform_device *pdev)
> > > >  		}
> > > >  	}
> > > >  
> > > > +	/* We need to initialize lock even for non-registered console */
> > > > +	spin_lock_init(&sport->port.lock);
> > 
> > Why are we having to do this for all console drivers recently?
> > Shouldn't the original patch that required this gone through and fixed
> > up all drivers?
> > 
> > I'm with Guenter here, let's fix them all at once please.
> 
> Okay. Do you want series per driver or one for all?

one per driver is always easier to apply :)

thanks,

greg k-h

Re: [PATCH v1] serial: imx: Initialize lock for non-registered console

[Andy Shevchenko]

On Mon, May 25, 2020 at 05:41:07PM +0200, Greg Kroah-Hartman wrote:
> On Mon, May 25, 2020 at 06:34:18PM +0300, Andy Shevchenko wrote:
> > On Mon, May 25, 2020 at 04:07:52PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, May 25, 2020 at 07:01:13AM -0700, Guenter Roeck wrote:

...

> > > I'm with Guenter here, let's fix them all at once please.
> > 
> > Okay. Do you want series per driver or one for all?
> 
> one per driver is always easier to apply :)

Will do during this week. Please, apply this one as it's ready to go.

-- 
With Best Regards,
Andy Shevchenko