This series fixes some of the fallout after a recent series adding
support for DMA transfers to the Qualcomm geni serial driver.
Most importantly it fixes a hang during reboot when using a serial
console and the getty is stopped during reboot.
Doug just posted an equivalent fix here:
https://lore.kernel.org/lkml/20230307073155.1.Iaab0159b8d268060a0e131ebb27125af4750ef99@changeid
but the commit message only mentions the regression with respect to
kgdb, which is not as widely used serial consoles generally, so I
figured I'd post my version for completeness.
Either version of that fix should address the immediate regression, but
fixing the underlying problems which have been there since the driver
was first merged is going to be a bit more involved.
The rest of the series fixes a few bugs in the new DMA support that I
found while investigating the console regression.
Johan
Johan Hovold (4):
serial: qcom-geni: fix console shutdown hang
serial: qcom-geni: fix DMA mapping leak on shutdown
serial: qcom-geni: fix mapping of empty DMA buffer
serial: qcom-geni: drop bogus uart_write_wakeup()
drivers/tty/serial/qcom_geni_serial.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
--
2.39.2
A recent commit added back the calls top stop tx and rx to shutdown()
which had previously been removed by commit e83766334f96 ("tty: serial:
qcom_geni_serial: No need to stop tx/rx on UART shutdown") in order to
be able to use kgdb after stopping the getty.
Not only did this again break kgdb, but it also broke serial consoles
more generally by hanging TX when stopping the getty during reboot.
The underlying problem has been there since the driver was first merged
and fixing it is going to be a bit involved so simply stop calling the
broken stop functions during shutdown for consoles for now.
Fixes: d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in progress at shutdown")
Cc: Bartosz Golaszewski <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/tty/serial/qcom_geni_serial.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index d69592e5e2ec..11da05d8f848 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1070,6 +1070,10 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
static void qcom_geni_serial_shutdown(struct uart_port *uport)
{
disable_irq(uport->irq);
+
+ if (uart_console(uport))
+ return;
+
qcom_geni_serial_stop_tx(uport);
qcom_geni_serial_stop_rx(uport);
}
--
2.39.2
Make sure that there is data in the ring buffer before trying to set up
a zero-length DMA transfer.
This specifically fixes the following warning when unmapping the empty
buffer on the sc8280xp-crd:
WARNING: CPU: 0 PID: 138 at drivers/iommu/dma-iommu.c:1046 iommu_dma_unmap_page+0xbc/0xd8
...
Call trace:
iommu_dma_unmap_page+0xbc/0xd8
dma_unmap_page_attrs+0x30/0x1c8
geni_se_tx_dma_unprep+0x28/0x38
qcom_geni_serial_isr+0x358/0x75c
Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
Cc: Bartosz Golaszewski <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/tty/serial/qcom_geni_serial.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 2aa3872e6283..9871225b2f9b 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -631,6 +631,9 @@ static void qcom_geni_serial_start_tx_dma(struct uart_port *uport)
if (port->tx_dma_addr)
return;
+ if (uart_circ_empty(xmit))
+ return;
+
xmit_size = uart_circ_chars_pending(xmit);
if (xmit_size < WAKEUP_CHARS)
uart_write_wakeup(uport);
--
2.39.2
Fix what appears to be a copy-paste error that can lead to a leaked DMA
mapping on close() and failure to restart TX after the port is reopened.
Note that rx_dma_addr is generally NULL when
qcom_geni_serial_stop_tx_dma() is called as part of shutdown() (but
tx_dma_addr need not be).
Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
Cc: Bartosz Golaszewski <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/tty/serial/qcom_geni_serial.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 11da05d8f848..2aa3872e6283 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -596,7 +596,7 @@ static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport)
if (!qcom_geni_serial_main_active(uport))
return;
- if (port->rx_dma_addr) {
+ if (port->tx_dma_addr) {
geni_se_tx_dma_unprep(&port->se, port->tx_dma_addr,
port->tx_remaining);
port->tx_dma_addr = 0;
--
2.39.2
Drop the bogus uart_write_wakeup() from when setting up a new DMA
transfer, which does not free up any more space in the ring buffer.
Any pending writers will be woken up when the transfer completes.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/tty/serial/qcom_geni_serial.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 9871225b2f9b..28fbc927a546 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -634,10 +634,6 @@ static void qcom_geni_serial_start_tx_dma(struct uart_port *uport)
if (uart_circ_empty(xmit))
return;
- xmit_size = uart_circ_chars_pending(xmit);
- if (xmit_size < WAKEUP_CHARS)
- uart_write_wakeup(uport);
-
xmit_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
qcom_geni_serial_setup_tx(uport, xmit_size);
--
2.39.2
On Tue, 7 Mar 2023 at 17:43, Johan Hovold <[email protected]> wrote:
>
> This series fixes some of the fallout after a recent series adding
> support for DMA transfers to the Qualcomm geni serial driver.
>
> Most importantly it fixes a hang during reboot when using a serial
> console and the getty is stopped during reboot.
>
> Doug just posted an equivalent fix here:
>
> https://lore.kernel.org/lkml/20230307073155.1.Iaab0159b8d268060a0e131ebb27125af4750ef99@changeid
>
> but the commit message only mentions the regression with respect to
> kgdb, which is not as widely used serial consoles generally, so I
> figured I'd post my version for completeness.
>
> Either version of that fix should address the immediate regression, but
> fixing the underlying problems which have been there since the driver
> was first merged is going to be a bit more involved.
>
> The rest of the series fixes a few bugs in the new DMA support that I
> found while investigating the console regression.
>
> Johan
>
>
> Johan Hovold (4):
> serial: qcom-geni: fix console shutdown hang
> serial: qcom-geni: fix DMA mapping leak on shutdown
> serial: qcom-geni: fix mapping of empty DMA buffer
> serial: qcom-geni: drop bogus uart_write_wakeup()
>
> drivers/tty/serial/qcom_geni_serial.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> --
> 2.39.2
>
Hey Johan,
Douglas and Srini beat you to these fixes but thanks!
Bart
On Tue, 7 Mar 2023 at 17:44, Bartosz Golaszewski
<[email protected]> wrote:
>
> On Tue, 7 Mar 2023 at 17:43, Johan Hovold <[email protected]> wrote:
> >
> > This series fixes some of the fallout after a recent series adding
> > support for DMA transfers to the Qualcomm geni serial driver.
> >
> > Most importantly it fixes a hang during reboot when using a serial
> > console and the getty is stopped during reboot.
> >
> > Doug just posted an equivalent fix here:
> >
> > https://lore.kernel.org/lkml/20230307073155.1.Iaab0159b8d268060a0e131ebb27125af4750ef99@changeid
> >
> > but the commit message only mentions the regression with respect to
> > kgdb, which is not as widely used serial consoles generally, so I
> > figured I'd post my version for completeness.
> >
> > Either version of that fix should address the immediate regression, but
> > fixing the underlying problems which have been there since the driver
> > was first merged is going to be a bit more involved.
> >
> > The rest of the series fixes a few bugs in the new DMA support that I
> > found while investigating the console regression.
> >
> > Johan
> >
> >
> > Johan Hovold (4):
> > serial: qcom-geni: fix console shutdown hang
> > serial: qcom-geni: fix DMA mapping leak on shutdown
> > serial: qcom-geni: fix mapping of empty DMA buffer
> > serial: qcom-geni: drop bogus uart_write_wakeup()
> >
> > drivers/tty/serial/qcom_geni_serial.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > --
> > 2.39.2
> >
>
> Hey Johan,
>
> Douglas and Srini beat you to these fixes but thanks!
>
> Bart
Nevermind, I read your other message now. And also patch 3/4 looks right.
Bart
On Tue, Mar 07, 2023 at 05:47:27PM +0100, Bartosz Golaszewski wrote:
> On Tue, 7 Mar 2023 at 17:44, Bartosz Golaszewski
> <[email protected]> wrote:
> >
> > On Tue, 7 Mar 2023 at 17:43, Johan Hovold <[email protected]> wrote:
> > >
> > > This series fixes some of the fallout after a recent series adding
> > > support for DMA transfers to the Qualcomm geni serial driver.
> > >
> > > Most importantly it fixes a hang during reboot when using a serial
> > > console and the getty is stopped during reboot.
> > >
> > > Doug just posted an equivalent fix here:
> > >
> > > https://lore.kernel.org/lkml/20230307073155.1.Iaab0159b8d268060a0e131ebb27125af4750ef99@changeid
> > >
> > > but the commit message only mentions the regression with respect to
> > > kgdb, which is not as widely used serial consoles generally, so I
> > > figured I'd post my version for completeness.
> > >
> > > Either version of that fix should address the immediate regression, but
> > > fixing the underlying problems which have been there since the driver
> > > was first merged is going to be a bit more involved.
> > >
> > > The rest of the series fixes a few bugs in the new DMA support that I
> > > found while investigating the console regression.
> > >
> > > Johan
> > >
> > >
> > > Johan Hovold (4):
> > > serial: qcom-geni: fix console shutdown hang
> > > serial: qcom-geni: fix DMA mapping leak on shutdown
> > > serial: qcom-geni: fix mapping of empty DMA buffer
> > > serial: qcom-geni: drop bogus uart_write_wakeup()
> > >
> > > drivers/tty/serial/qcom_geni_serial.c | 11 +++++++----
> > > 1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > --
> > > 2.39.2
> > >
> >
> > Hey Johan,
> >
> > Douglas and Srini beat you to these fixes but thanks!
> Nevermind, I read your other message now. And also patch 3/4 looks right.
Heh, this hang has been in linux-next for over a month and I've
actively tried to not spend time on investigating it in the hope that
someone else would be beat me to it before I moved to 6.3-rc. :)
Obviously I may be a bit biased, but I prefer this series over the
alternate fixes as the commit messages are a bit more complete and my
version of the empty DMA buffer fix is a bit cleaner.
Johan
Hi,
On Tue, Mar 7, 2023 at 8:43 AM Johan Hovold <[email protected]> wrote:
>
> A recent commit added back the calls top stop tx and rx to shutdown()
> which had previously been removed by commit e83766334f96 ("tty: serial:
> qcom_geni_serial: No need to stop tx/rx on UART shutdown") in order to
> be able to use kgdb after stopping the getty.
>
> Not only did this again break kgdb, but it also broke serial consoles
> more generally by hanging TX when stopping the getty during reboot.
>
> The underlying problem has been there since the driver was first merged
> and fixing it is going to be a bit involved so simply stop calling the
> broken stop functions during shutdown for consoles for now.
>
> Fixes: d8aca2f96813 ("tty: serial: qcom-geni-serial: stop operations in progress at shutdown")
> Cc: Bartosz Golaszewski <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 4 ++++
> 1 file changed, 4 insertions(+)
I'm fine with either this change or my change [1] landing.
Reviewed-by: Douglas Anderson <[email protected]>
[1] https://lore.kernel.org/r/20230307073155.1.Iaab0159b8d268060a0e131ebb27125af4750ef99@changeid
Hi,
On Tue, Mar 7, 2023 at 8:43 AM Johan Hovold <[email protected]> wrote:
>
> Fix what appears to be a copy-paste error that can lead to a leaked DMA
> mapping on close() and failure to restart TX after the port is reopened.
>
> Note that rx_dma_addr is generally NULL when
> qcom_geni_serial_stop_tx_dma() is called as part of shutdown() (but
> tx_dma_addr need not be).
>
> Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
> Cc: Bartosz Golaszewski <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Tue, Mar 7, 2023 at 8:43 AM Johan Hovold <[email protected]> wrote:
>
> Make sure that there is data in the ring buffer before trying to set up
> a zero-length DMA transfer.
>
> This specifically fixes the following warning when unmapping the empty
> buffer on the sc8280xp-crd:
>
> WARNING: CPU: 0 PID: 138 at drivers/iommu/dma-iommu.c:1046 iommu_dma_unmap_page+0xbc/0xd8
> ...
> Call trace:
> iommu_dma_unmap_page+0xbc/0xd8
> dma_unmap_page_attrs+0x30/0x1c8
> geni_se_tx_dma_unprep+0x28/0x38
> qcom_geni_serial_isr+0x358/0x75c
>
> Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
> Cc: Bartosz Golaszewski <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 2aa3872e6283..9871225b2f9b 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -631,6 +631,9 @@ static void qcom_geni_serial_start_tx_dma(struct uart_port *uport)
> if (port->tx_dma_addr)
> return;
>
> + if (uart_circ_empty(xmit))
> + return;
I guess you could remove the uart_circ_empty() test in
qcom_geni_serial_handle_tx_dma() now? In any case, with or without
that:
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Tue, Mar 7, 2023 at 8:43 AM Johan Hovold <[email protected]> wrote:
>
> Drop the bogus uart_write_wakeup() from when setting up a new DMA
> transfer, which does not free up any more space in the ring buffer.
>
> Any pending writers will be woken up when the transfer completes.
>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 4 ----
> 1 file changed, 4 deletions(-)
Reviewed-by: Douglas Anderson <[email protected]>
On Tue, Mar 07, 2023 at 10:41:46AM -0800, Doug Anderson wrote:
> Hi,
>
> On Tue, Mar 7, 2023 at 8:43 AM Johan Hovold <[email protected]> wrote:
> >
> > Make sure that there is data in the ring buffer before trying to set up
> > a zero-length DMA transfer.
> >
> > This specifically fixes the following warning when unmapping the empty
> > buffer on the sc8280xp-crd:
> >
> > WARNING: CPU: 0 PID: 138 at drivers/iommu/dma-iommu.c:1046 iommu_dma_unmap_page+0xbc/0xd8
> > ...
> > Call trace:
> > iommu_dma_unmap_page+0xbc/0xd8
> > dma_unmap_page_attrs+0x30/0x1c8
> > geni_se_tx_dma_unprep+0x28/0x38
> > qcom_geni_serial_isr+0x358/0x75c
> >
> > Fixes: 2aaa43c70778 ("tty: serial: qcom-geni-serial: add support for serial engine DMA")
> > Cc: Bartosz Golaszewski <[email protected]>
> > Signed-off-by: Johan Hovold <[email protected]>
> > ---
> > drivers/tty/serial/qcom_geni_serial.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > index 2aa3872e6283..9871225b2f9b 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -631,6 +631,9 @@ static void qcom_geni_serial_start_tx_dma(struct uart_port *uport)
> > if (port->tx_dma_addr)
> > return;
> >
> > + if (uart_circ_empty(xmit))
> > + return;
>
> I guess you could remove the uart_circ_empty() test in
> qcom_geni_serial_handle_tx_dma() now?
I considered that, but decided to leave it in as it makes the flow in
qcom_geni_serial_handle_tx_dma() a bit more obvious (and that function
already handles the related uart_write_wakeup() which the check could
potentially be combined with).
> In any case, with or without that:
>
> Reviewed-by: Douglas Anderson <[email protected]>
Thanks for reviewing.
Johan
On 07/03/2023 17:03, Johan Hovold wrote:
> On Tue, Mar 07, 2023 at 05:47:27PM +0100, Bartosz Golaszewski wrote:
>> On Tue, 7 Mar 2023 at 17:44, Bartosz Golaszewski
>> <[email protected]> wrote:
>>>
>>> On Tue, 7 Mar 2023 at 17:43, Johan Hovold <[email protected]> wrote:
>>>>
>>>> This series fixes some of the fallout after a recent series adding
>>>> support for DMA transfers to the Qualcomm geni serial driver.
>>>>
>>>> Most importantly it fixes a hang during reboot when using a serial
>>>> console and the getty is stopped during reboot.
>>>>
>>>> Doug just posted an equivalent fix here:
>>>>
>>>> https://lore.kernel.org/lkml/20230307073155.1.Iaab0159b8d268060a0e131ebb27125af4750ef99@changeid
>>>>
>>>> but the commit message only mentions the regression with respect to
>>>> kgdb, which is not as widely used serial consoles generally, so I
>>>> figured I'd post my version for completeness.
>>>>
>>>> Either version of that fix should address the immediate regression, but
>>>> fixing the underlying problems which have been there since the driver
>>>> was first merged is going to be a bit more involved.
>>>>
>>>> The rest of the series fixes a few bugs in the new DMA support that I
>>>> found while investigating the console regression.
>>>>
>>>> Johan
>>>>
>>>>
>>>> Johan Hovold (4):
>>>> serial: qcom-geni: fix console shutdown hang
>>>> serial: qcom-geni: fix DMA mapping leak on shutdown
>>>> serial: qcom-geni: fix mapping of empty DMA buffer
>>>> serial: qcom-geni: drop bogus uart_write_wakeup()
>>>>
>>>> drivers/tty/serial/qcom_geni_serial.c | 11 +++++++----
>>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> --
>>>> 2.39.2
>>>>
>>>
>>> Hey Johan,
>>>
>>> Douglas and Srini beat you to these fixes but thanks!
>
>> Nevermind, I read your other message now. And also patch 3/4 looks right.
>
> Heh, this hang has been in linux-next for over a month and I've
> actively tried to not spend time on investigating it in the hope that
> someone else would be beat me to it before I moved to 6.3-rc. :)
>
> Obviously I may be a bit biased, but I prefer this series over the
> alternate fixes as the commit messages are a bit more complete and my
> version of the empty DMA buffer fix is a bit cleaner.
I don't mind, as long as the bugs are fixed.
--srini
>
> Johan
On 07/03/2023 16:44, Johan Hovold wrote:
> This series fixes some of the fallout after a recent series adding
> support for DMA transfers to the Qualcomm geni serial driver.
>
> Most importantly it fixes a hang during reboot when using a serial
> console and the getty is stopped during reboot.
>
> Doug just posted an equivalent fix here:
>
> https://lore.kernel.org/lkml/20230307073155.1.Iaab0159b8d268060a0e131ebb27125af4750ef99@changeid
>
> but the commit message only mentions the regression with respect to
> kgdb, which is not as widely used serial consoles generally, so I
> figured I'd post my version for completeness.
>
> Either version of that fix should address the immediate regression, but
> fixing the underlying problems which have been there since the driver
> was first merged is going to be a bit more involved.
>
> The rest of the series fixes a few bugs in the new DMA support that I
> found while investigating the console regression.
>
> Johan
>
>
> Johan Hovold (4):
> serial: qcom-geni: fix console shutdown hang
> serial: qcom-geni: fix DMA mapping leak on shutdown
> serial: qcom-geni: fix mapping of empty DMA buffer
> serial: qcom-geni: drop bogus uart_write_wakeup()
>
Tested this series on RB5
Reviewed-by: Srinivas Kandagatla <[email protected]>
Tested-by: Srinivas Kandagatla <[email protected]>
--srini
> drivers/tty/serial/qcom_geni_serial.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
On Tue, Mar 07, 2023 at 05:44:01PM +0100, Johan Hovold wrote:
> This series fixes some of the fallout after a recent series adding
> support for DMA transfers to the Qualcomm geni serial driver.
>
> Most importantly it fixes a hang during reboot when using a serial
> console and the getty is stopped during reboot.
>
> Doug just posted an equivalent fix here:
>
> https://lore.kernel.org/lkml/20230307073155.1.Iaab0159b8d268060a0e131ebb27125af4750ef99@changeid
>
> but the commit message only mentions the regression with respect to
> kgdb, which is not as widely used serial consoles generally, so I
> figured I'd post my version for completeness.
>
> Either version of that fix should address the immediate regression, but
> fixing the underlying problems which have been there since the driver
> was first merged is going to be a bit more involved.
>
> The rest of the series fixes a few bugs in the new DMA support that I
> found while investigating the console regression.
>
> Johan
>
>
> Johan Hovold (4):
> serial: qcom-geni: fix console shutdown hang
> serial: qcom-geni: fix DMA mapping leak on shutdown
> serial: qcom-geni: fix mapping of empty DMA buffer
> serial: qcom-geni: drop bogus uart_write_wakeup()
>
> drivers/tty/serial/qcom_geni_serial.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> --
> 2.39.2
>
Realized this has been affecting me (with me blaming it on something
else prior) off and on. Thanks for the fix!
Reviewed-by: Andrew Halaney <[email protected]>
Tested-by: Andrew Halaney <[email protected]> # sa8540p-ride