[PATCH 0/4] 8250: DMA Fixes

[PATCH 0/4] 8250: DMA Fixes

[Ilpo Järvinen]

Here are a number of 8250 DMA related fixes. The last one seems the
most serious problem able to corrupt the payload ordering.

Ilpo Järvinen (4):
  serial: 8250: Fall back to non-DMA Rx if IIR_RDI occurs
  serial: 8250_lpss: Configure DMA also w/o DMA filter
  serial: 8250_lpss: Use 16B DMA burst with Elkhart Lake
  serial: 8250: Flush DMA Rx on RLSI

 drivers/tty/serial/8250/8250_lpss.c | 18 +++++++++++++-----
 drivers/tty/serial/8250/8250_port.c |  7 +++++--
 2 files changed, 18 insertions(+), 7 deletions(-)

-- 
2.30.2

[PATCH 1/4] serial: 8250: Fall back to non-DMA Rx if IIR_RDI occurs

[Ilpo Järvinen]

DW UART sometimes triggers IIR_RDI during DMA Rx when IIR_RX_TIMEOUT
should have been triggered instead. Since IIR_RDI has higher priority
than IIR_RX_TIMEOUT, this causes the Rx to hang into interrupt loop.
The problem seems to occur at least with some combinations of
small-sized transfers (I've reproduced the problem on Elkhart Lake PSE
UARTs).

If there's already an on-going Rx DMA and IIR_RDI triggers, fall
graciously back to non-DMA Rx. That is, behave as if IIR_RX_TIMEOUT had
occurred.

8250_omap already considers IIR_RDI similar to this change so its
nothing unheard of.

Fixes: 75df022b5f89 ("serial: 8250_dma: Fix RX handling")
Cc: <stable@vger.kernel.org>
Co-developed-by: Srikanth Thokala <srikanth.thokala@intel.com>
Signed-off-by: Srikanth Thokala <srikanth.thokala@intel.com>
Co-developed-by: Aman Kumar <aman.kumar@intel.com>
Signed-off-by: Aman Kumar <aman.kumar@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index fe8662cd9402..92dd18716169 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1897,6 +1897,10 @@ EXPORT_SYMBOL_GPL(serial8250_modem_status);
 static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
 {
 	switch (iir & 0x3f) {
+	case UART_IIR_RDI:
+		if (!up->dma->rx_running)
+			break;
+		fallthrough;
 	case UART_IIR_RX_TIMEOUT:
 		serial8250_rx_dma_flush(up);
 		fallthrough;
-- 
2.30.2

[PATCH 2/4] serial: 8250_lpss: Configure DMA also w/o DMA filter

[Ilpo Järvinen]

If the platform doesn't use DMA device filter (as is the case with
Elkhart Lake), whole lpss8250_dma_setup() setup is skipped. This
results in skipping also *_maxburst setup which is undesirable.
Refactor lpss8250_dma_setup() to configure DMA even if filter is not
setup.

Cc: stable <stable@kernel.org>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_lpss.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 44cc755b1a29..ed281445a97d 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -277,8 +277,10 @@ static int lpss8250_dma_setup(struct lpss8250 *lpss, struct uart_8250_port *port
 	struct dw_dma_slave *rx_param, *tx_param;
 	struct device *dev = port->port.dev;
 
-	if (!lpss->dma_param.dma_dev)
-		return 0;
+	if (!lpss->dma_param.dma_dev) {
+		dma = port->dma;
+		goto confonly;
+	}
 
 	rx_param = devm_kzalloc(dev, sizeof(*rx_param), GFP_KERNEL);
 	if (!rx_param)
@@ -289,16 +291,20 @@ static int lpss8250_dma_setup(struct lpss8250 *lpss, struct uart_8250_port *port
 		return -ENOMEM;
 
 	*rx_param = lpss->dma_param;
-	dma->rxconf.src_maxburst = lpss->dma_maxburst;
-
 	*tx_param = lpss->dma_param;
-	dma->txconf.dst_maxburst = lpss->dma_maxburst;
 
 	dma->fn = lpss8250_dma_filter;
 	dma->rx_param = rx_param;
 	dma->tx_param = tx_param;
 
 	port->dma = dma;
+
+confonly:
+	if (dma) {
+		dma->rxconf.src_maxburst = lpss->dma_maxburst;
+		dma->txconf.dst_maxburst = lpss->dma_maxburst;
+	}
+
 	return 0;
 }
 
-- 
2.30.2

[PATCH 3/4] serial: 8250_lpss: Use 16B DMA burst with Elkhart Lake

[Ilpo Järvinen]

Configure DMA to use 16B burst size with Elkhart Lake. This makes the
bus use more efficient and works around an issue which occurs with the
previously used 1B.

Fixes: 0a9410b981e9 ("serial: 8250_lpss: Enable DMA on Intel Elkhart Lake")
Cc: <stable@vger.kernel.org> # serial: 8250_lpss: Configure DMA also w/o DMA filter
Reported-by: Wentong Wu <wentong.wu@intel.com>
Co-developed-by: Srikanth Thokala <srikanth.thokala@intel.com>
Signed-off-by: Srikanth Thokala <srikanth.thokala@intel.com>
Co-developed-by: Aman Kumar <aman.kumar@intel.com>
Signed-off-by: Aman Kumar <aman.kumar@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

---
I know the list of Co-dev-bys & Sob seems a bit odd for a oneliner.
The reason is that I cleaned up this from a more complex patch using
the earlier change that I authored myself so only this oneliner
remained in this patch.
---
 drivers/tty/serial/8250/8250_lpss.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index ed281445a97d..e0b4e1446eac 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -174,6 +174,8 @@ static int ehl_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 	 */
 	up->dma = dma;
 
+	lpss->dma_maxburst = 16;
+
 	port->set_termios = dw8250_do_set_termios;
 
 	return 0;
-- 
2.30.2

[PATCH 4/4] serial: 8250: Flush DMA Rx on RLSI

[Ilpo Järvinen]

Returning true from handle_rx_dma() without flushing DMA first creates
a data ordering hazard. If DMA Rx has handled any character at the
point when RLSI occurs, the non-DMA path handles any pending characters
jumping them ahead of those characters that are pending under DMA.

Fixes: 75df022b5f89 ("serial: 8250_dma: Fix RX handling")
Cc: <stable@vger.kernel.org>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
Cc: Gilles BULOZ <gilles.buloz@kontron.com>

 drivers/tty/serial/8250/8250_port.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 92dd18716169..388172289627 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1901,10 +1901,9 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
 		if (!up->dma->rx_running)
 			break;
 		fallthrough;
+	case UART_IIR_RLSI:
 	case UART_IIR_RX_TIMEOUT:
 		serial8250_rx_dma_flush(up);
-		fallthrough;
-	case UART_IIR_RLSI:
 		return true;
 	}
 	return up->dma->rx_dma(up);
-- 
2.30.2

Re: [PATCH 0/4] 8250: DMA Fixes

[Andy Shevchenko]

On Mon, Nov 07, 2022 at 01:07:04PM +0200, Ilpo Järvinen wrote:
> Here are a number of 8250 DMA related fixes. The last one seems the
> most serious problem able to corrupt the payload ordering.
> 
> Ilpo Järvinen (4):
>   serial: 8250: Fall back to non-DMA Rx if IIR_RDI occurs

8250_port?

>   serial: 8250_lpss: Configure DMA also w/o DMA filter
>   serial: 8250_lpss: Use 16B DMA burst with Elkhart Lake
>   serial: 8250: Flush DMA Rx on RLSI

8250_port?

> 
>  drivers/tty/serial/8250/8250_lpss.c | 18 +++++++++++++-----
>  drivers/tty/serial/8250/8250_port.c |  7 +++++--
>  2 files changed, 18 insertions(+), 7 deletions(-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] serial: 8250_lpss: Configure DMA also w/o DMA filter

[Andy Shevchenko]

On Mon, Nov 07, 2022 at 01:07:06PM +0200, Ilpo Järvinen wrote:
> If the platform doesn't use DMA device filter (as is the case with
> Elkhart Lake), whole lpss8250_dma_setup() setup is skipped. This
> results in skipping also *_maxburst setup which is undesirable.
> Refactor lpss8250_dma_setup() to configure DMA even if filter is not
> setup.

...


> +	if (!lpss->dma_param.dma_dev) {
> +		dma = port->dma;
> +		goto confonly;

Perhaps

		if (dma)
			goto out_configuration_only;

		return 0;

(note the label naming as well).

> +	}

...

> +confonly:

> +	if (dma) {

But you know that previous code has dma != NULL. See above.

> +		dma->rxconf.src_maxburst = lpss->dma_maxburst;
> +		dma->txconf.dst_maxburst = lpss->dma_maxburst;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] serial: 8250_lpss: Use 16B DMA burst with Elkhart Lake

[Andy Shevchenko]

On Mon, Nov 07, 2022 at 01:07:07PM +0200, Ilpo Järvinen wrote:
> Configure DMA to use 16B burst size with Elkhart Lake. This makes the
> bus use more efficient and works around an issue which occurs with the
> previously used 1B.
> 
> Fixes: 0a9410b981e9 ("serial: 8250_lpss: Enable DMA on Intel Elkhart Lake")
> Cc: <stable@vger.kernel.org> # serial: 8250_lpss: Configure DMA also w/o DMA filter
> Reported-by: Wentong Wu <wentong.wu@intel.com>
> Co-developed-by: Srikanth Thokala <srikanth.thokala@intel.com>
> Signed-off-by: Srikanth Thokala <srikanth.thokala@intel.com>
> Co-developed-by: Aman Kumar <aman.kumar@intel.com>
> Signed-off-by: Aman Kumar <aman.kumar@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> ---
> I know the list of Co-dev-bys & Sob seems a bit odd for a oneliner.
> The reason is that I cleaned up this from a more complex patch using
> the earlier change that I authored myself so only this oneliner
> remained in this patch.

If you changed more than 70% of the code, I would suggest to drop the rest of
(Co)authors and if you want to pay a credit, just mention them in the commit
message in a free form.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] serial: 8250: Flush DMA Rx on RLSI

[Andy Shevchenko]

On Mon, Nov 07, 2022 at 01:07:08PM +0200, Ilpo Järvinen wrote:
> Returning true from handle_rx_dma() without flushing DMA first creates
> a data ordering hazard. If DMA Rx has handled any character at the
> point when RLSI occurs, the non-DMA path handles any pending characters
> jumping them ahead of those characters that are pending under DMA.
> 
> Fixes: 75df022b5f89 ("serial: 8250_dma: Fix RX handling")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> Cc: Gilles BULOZ <gilles.buloz@kontron.com>

Maybe instead you can do --cc Guilles ... for entire series (and drop from
other commit messages)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] 8250: DMA Fixes

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

On Mon, 7 Nov 2022, Andy Shevchenko wrote:

> On Mon, Nov 07, 2022 at 01:07:04PM +0200, Ilpo Järvinen wrote:
> > Here are a number of 8250 DMA related fixes. The last one seems the
> > most serious problem able to corrupt the payload ordering.
> > 
> > Ilpo Järvinen (4):
> >   serial: 8250: Fall back to non-DMA Rx if IIR_RDI occurs
> 
> 8250_port?
> 
> >   serial: 8250_lpss: Configure DMA also w/o DMA filter
> >   serial: 8250_lpss: Use 16B DMA burst with Elkhart Lake
> >   serial: 8250: Flush DMA Rx on RLSI
> 
> 8250_port?

Why?

To me this 8250_core/port split is still integral part of the same 
8250 even if they're in the end technically loaded into different modules
or the code is in a different file. There's even some trickery to access 
internals of the other part to workaround the circular module dependency 
logic that would otherwise prevent the split (like we learned not so long 
time ago with that setup_irq change).

I can start to use 8250_port if you insist but it seems pointless 5 extra 
characters out from a resource that is scarse to begin with, IMHO (the 
summary line is not that long).


-- 
 i.

Re: [PATCH 0/4] 8250: DMA Fixes

[Andy Shevchenko]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 1430 bytes --]

On Mon, Nov 07, 2022 at 01:50:38PM +0200, Ilpo J�rvinen wrote:
> On Mon, 7 Nov 2022, Andy Shevchenko wrote:
> 
> > On Mon, Nov 07, 2022 at 01:07:04PM +0200, Ilpo Järvinen wrote:
> > > Here are a number of 8250 DMA related fixes. The last one seems the
> > > most serious problem able to corrupt the payload ordering.
> > > 
> > > Ilpo Järvinen (4):
> > >   serial: 8250: Fall back to non-DMA Rx if IIR_RDI occurs
> > 
> > 8250_port?
> > 
> > >   serial: 8250_lpss: Configure DMA also w/o DMA filter
> > >   serial: 8250_lpss: Use 16B DMA burst with Elkhart Lake
> > >   serial: 8250: Flush DMA Rx on RLSI
> > 
> > 8250_port?
> 
> Why?

> To me this 8250_core/port split is still integral part of the same 
> 8250 even if they're in the end technically loaded into different modules
> or the code is in a different file. There's even some trickery to access 
> internals of the other part to workaround the circular module dependency 
> logic that would otherwise prevent the split (like we learned not so long 
> time ago with that setup_irq change).

> I can start to use 8250_port if you insist but it seems pointless 5 extra 
> characters out from a resource that is scarse to begin with, IMHO (the 
> summary line is not that long).

No strong opinion here, I just follow (in my patches) the following logic:
- if it touches a single file, I use full prefix,
- otherwise I use 8250

-- 
With Best Regards,
Andy Shevchenko