| Message ID | 1588793212-5586-2-git-send-email-sagar.kadam@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tty: serial: add missing spin_lock_init for SiFive UART | expand |
On Wed, May 6, 2020 at 12:27 PM Sagar Shrikant Kadam <sagar.kadam@sifive.com> wrote: > > Uninitialised spin lock on sifive serial port (ssp) raised > a race condition and resulted in spin lock bad magic as > reported and discussed here [1] > Initialising the spin lock resolves the issue. > > The fix is tested on HiFive Unleashed A00 board with Linux 5.7-rc4 > and OpenSBI v0.7 > > [1] http://lists.infradead.org/pipermail/linux-riscv/2020-May/009713.html > > Fixes: 45c054d0815b ("tty: serial: add driver for the SiFive UART") > Reported-by: Atish Patra <Atish.Patra@wdc.com> > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> > --- > drivers/tty/serial/sifive.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c > index 13eadcb..b061bdb 100644 > --- a/drivers/tty/serial/sifive.c > +++ b/drivers/tty/serial/sifive.c > @@ -989,6 +989,7 @@ static int sifive_serial_probe(struct platform_device *pdev) > ssp->clk = clk; > ssp->clk_notifier.notifier_call = sifive_serial_clk_notifier; > > + spin_lock_init(&ssp->port.lock); > r = clk_notifier_register(ssp->clk, &ssp->clk_notifier); > if (r) { > dev_err(&pdev->dev, "could not register clock notifier: %d\n", > -- > 2.7.4 > > Looks good. I have verified the fix on unleashed. Tested-by: Atish Patra <atish.patra@wdc.com>
HI Atish, > -----Original Message----- > From: Atish Patra <atishp@atishpatra.org> > Sent: Thursday, May 7, 2020 4:41 AM > To: Sagar Kadam <sagar.kadam@sifive.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Palmer Dabbelt > <palmer@dabbelt.com>; Atish Patra <Atish.Patra@wdc.com>; linux-riscv > <linux-riscv@lists.infradead.org>; Anup Patel <Anup.Patel@wdc.com>; Paul > Walmsley <paul.walmsley@sifive.com> > Subject: Re: [PATCH] tty: serial: add missing spin_lock_init for SiFive UART > > [External Email] Do not click links or attachments unless you recognize the > sender and know the content is safe > > On Wed, May 6, 2020 at 12:27 PM Sagar Shrikant Kadam > <sagar.kadam@sifive.com> wrote: > > > > Uninitialised spin lock on sifive serial port (ssp) raised a race > > condition and resulted in spin lock bad magic as reported and > > discussed here [1] Initialising the spin lock resolves the issue. > > > > The fix is tested on HiFive Unleashed A00 board with Linux 5.7-rc4 and > > OpenSBI v0.7 > > > > [1] > > http://lists.infradead.org/pipermail/linux-riscv/2020-May/009713.html > > > > Fixes: 45c054d0815b ("tty: serial: add driver for the SiFive UART") > > Reported-by: Atish Patra <Atish.Patra@wdc.com> > > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> > > --- > > drivers/tty/serial/sifive.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c > > index 13eadcb..b061bdb 100644 > > --- a/drivers/tty/serial/sifive.c > > +++ b/drivers/tty/serial/sifive.c > > @@ -989,6 +989,7 @@ static int sifive_serial_probe(struct > platform_device *pdev) > > ssp->clk = clk; > > ssp->clk_notifier.notifier_call = sifive_serial_clk_notifier; > > > > + spin_lock_init(&ssp->port.lock); > > r = clk_notifier_register(ssp->clk, &ssp->clk_notifier); > > if (r) { > > dev_err(&pdev->dev, "could not register clock > > notifier: %d\n", > > -- > > 2.7.4 > > > > > > Looks good. I have verified the fix on unleashed. > > Tested-by: Atish Patra <atish.patra@wdc.com> > Thanks for testing the patch, and confirming that it's fixed. BR, Sagar Kadam > -- > Regards, > Atish
On Wed, May 06, 2020 at 12:26:52PM -0700, Sagar Shrikant Kadam wrote: > Uninitialised spin lock on sifive serial port (ssp) raised > a race condition and resulted in spin lock bad magic as > reported and discussed here [1] > Initialising the spin lock resolves the issue. > > The fix is tested on HiFive Unleashed A00 board with Linux 5.7-rc4 > and OpenSBI v0.7 > > [1] http://lists.infradead.org/pipermail/linux-riscv/2020-May/009713.html In the future, can you link to lore.kernel.org instead? > > Fixes: 45c054d0815b ("tty: serial: add driver for the SiFive UART") So this should also go to stable kernels, right? > Reported-by: Atish Patra <Atish.Patra@wdc.com> > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> > --- > drivers/tty/serial/sifive.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c > index 13eadcb..b061bdb 100644 > --- a/drivers/tty/serial/sifive.c > +++ b/drivers/tty/serial/sifive.c > @@ -989,6 +989,7 @@ static int sifive_serial_probe(struct platform_device *pdev) > ssp->clk = clk; > ssp->clk_notifier.notifier_call = sifive_serial_clk_notifier; > > + spin_lock_init(&ssp->port.lock); Shouldn't the port lock be initialized by the tty core instead? I think this is the second time I've seen this type of fix needed recently... thanks, greg k-h
On Wed, 06 May 2020 23:49:58 PDT (-0700), Greg KH wrote: > On Wed, May 06, 2020 at 12:26:52PM -0700, Sagar Shrikant Kadam wrote: >> Uninitialised spin lock on sifive serial port (ssp) raised >> a race condition and resulted in spin lock bad magic as >> reported and discussed here [1] >> Initialising the spin lock resolves the issue. >> >> The fix is tested on HiFive Unleashed A00 board with Linux 5.7-rc4 >> and OpenSBI v0.7 >> >> [1] http://lists.infradead.org/pipermail/linux-riscv/2020-May/009713.html > > In the future, can you link to lore.kernel.org instead? > >> >> Fixes: 45c054d0815b ("tty: serial: add driver for the SiFive UART") > > So this should also go to stable kernels, right? > >> Reported-by: Atish Patra <Atish.Patra@wdc.com> >> Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> >> --- >> drivers/tty/serial/sifive.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c >> index 13eadcb..b061bdb 100644 >> --- a/drivers/tty/serial/sifive.c >> +++ b/drivers/tty/serial/sifive.c >> @@ -989,6 +989,7 @@ static int sifive_serial_probe(struct platform_device *pdev) >> ssp->clk = clk; >> ssp->clk_notifier.notifier_call = sifive_serial_clk_notifier; >> >> + spin_lock_init(&ssp->port.lock); > > Shouldn't the port lock be initialized by the tty core instead? I think > this is the second time I've seen this type of fix needed recently... Ya, that's what was blocking my reviewed-by. It looks like the spinlock isn't initialized for console. IDK why that's the case, but assuming that's the way it's supposed to work we should only initialize the spinlock if we set up a console port, which we do in __ssp_add_console_port(). In other words, something like this diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c index 13eadcb8aec4..0b5110dad051 100644 --- a/drivers/tty/serial/sifive.c +++ b/drivers/tty/serial/sifive.c @@ -883,6 +883,7 @@ console_initcall(sifive_console_init); static void __ssp_add_console_port(struct sifive_serial_port *ssp) { + spin_lock_init(&ssp->port.lock); sifive_serial_console_ports[ssp->port.line] = ssp; } > > thanks, > > greg k-h
Hello, > -----Original Message----- > From: Palmer Dabbelt <palmer@dabbelt.com> > Sent: Thursday, May 7, 2020 11:03 PM > To: Greg KH <gregkh@linuxfoundation.org> > Cc: Sagar Kadam <sagar.kadam@sifive.com>; Paul Walmsley > <paul.walmsley@sifive.com>; linux-riscv@lists.infradead.org; Atish Patra > <Atish.Patra@wdc.com>; Anup Patel <Anup.Patel@wdc.com> > Subject: Re: [PATCH] tty: serial: add missing spin_lock_init for SiFive UART > > [External Email] Do not click links or attachments unless you recognize the > sender and know the content is safe > > On Wed, 06 May 2020 23:49:58 PDT (-0700), Greg KH wrote: > > On Wed, May 06, 2020 at 12:26:52PM -0700, Sagar Shrikant Kadam wrote: > >> Uninitialised spin lock on sifive serial port (ssp) raised a race > >> condition and resulted in spin lock bad magic as reported and > >> discussed here [1] Initialising the spin lock resolves the issue. > >> > >> The fix is tested on HiFive Unleashed A00 board with Linux 5.7-rc4 > >> and OpenSBI v0.7 > >> > >> [1] > >> http://lists.infradead.org/pipermail/linux-riscv/2020-May/009713.html > > > > In the future, can you link to lore.kernel.org instead? > > Thanks for suggestions, Greg. Yes, will update link from lore.kernel.org > >> > >> Fixes: 45c054d0815b ("tty: serial: add driver for the SiFive UART") > > > > So this should also go to stable kernels, right? > > Yes, I will include linux-kernel and linux-serial as well in V2 > >> Reported-by: Atish Patra <Atish.Patra@wdc.com> > >> Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> > >> --- > >> drivers/tty/serial/sifive.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/tty/serial/sifive.c > >> b/drivers/tty/serial/sifive.c index 13eadcb..b061bdb 100644 > >> --- a/drivers/tty/serial/sifive.c > >> +++ b/drivers/tty/serial/sifive.c > >> @@ -989,6 +989,7 @@ static int sifive_serial_probe(struct > platform_device *pdev) > >> ssp->clk = clk; > >> ssp->clk_notifier.notifier_call = sifive_serial_clk_notifier; > >> > >> + spin_lock_init(&ssp->port.lock); > > > > Shouldn't the port lock be initialized by the tty core instead? I > > think this is the second time I've seen this type of fix needed recently... > > Ya, that's what was blocking my reviewed-by. It looks like the spinlock isn't > initialized for console. IDK why that's the case, but assuming that's the way > it's supposed to work we should only initialize the spinlock if we set up a > console port, which we do in __ssp_add_console_port(). In other words, > something like this > Thanks Greg and Palmer, your inputs are valuable. I could see that the tty core does initialise the port spin lock via tty_port_init while registering the uart_driver, while there isn't any lock initialized for console. The serial_core ensure the spin lock for console is initialised (uart_port_spin_lock_init) and since it is missing in our case we encounter the spin_lock error. I have verified that spin_lock init for ssp console works, and will submit a v2. Thanks & BR, Sagar Kadam > diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c index > 13eadcb8aec4..0b5110dad051 100644 > --- a/drivers/tty/serial/sifive.c > +++ b/drivers/tty/serial/sifive.c > @@ -883,6 +883,7 @@ console_initcall(sifive_console_init); > > static void __ssp_add_console_port(struct sifive_serial_port *ssp) { > + spin_lock_init(&ssp->port.lock); > sifive_serial_console_ports[ssp->port.line] = ssp; } > > > > > thanks, > > > > greg k-h
diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c index 13eadcb..b061bdb 100644 --- a/drivers/tty/serial/sifive.c +++ b/drivers/tty/serial/sifive.c @@ -989,6 +989,7 @@ static int sifive_serial_probe(struct platform_device *pdev) ssp->clk = clk; ssp->clk_notifier.notifier_call = sifive_serial_clk_notifier; + spin_lock_init(&ssp->port.lock); r = clk_notifier_register(ssp->clk, &ssp->clk_notifier); if (r) { dev_err(&pdev->dev, "could not register clock notifier: %d\n",
Uninitialised spin lock on sifive serial port (ssp) raised a race condition and resulted in spin lock bad magic as reported and discussed here [1] Initialising the spin lock resolves the issue. The fix is tested on HiFive Unleashed A00 board with Linux 5.7-rc4 and OpenSBI v0.7 [1] http://lists.infradead.org/pipermail/linux-riscv/2020-May/009713.html Fixes: 45c054d0815b ("tty: serial: add driver for the SiFive UART") Reported-by: Atish Patra <Atish.Patra@wdc.com> Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com> --- drivers/tty/serial/sifive.c | 1 + 1 file changed, 1 insertion(+)