[PATCH v2 1/2] serial: 8250_lpss: Balance reference count for PCI DMA device

[PATCH v2 1/2] serial: 8250_lpss: Balance reference count for PCI DMA device

[Andy Shevchenko]

The pci_get_slot() increases its reference count, the caller
must decrement the reference count by calling pci_dev_put().

Fixes: 9a1870ce812e ("serial: 8250: don't use slave_id of dma_slave_config")
Depends-on: a13e19cf3dc1 ("serial: 8250_lpss: split LPSS driver to separate module")
Reported-by: Qing Wang <wangqing@vivo.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: made sure we bump reference count only on success (Jiri)
 drivers/tty/serial/8250/8250_lpss.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index d3bafec7619d..0f5af061e0b4 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -117,8 +117,7 @@ static int byt_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 {
 	struct dw_dma_slave *param = &lpss->dma_param;
 	struct pci_dev *pdev = to_pci_dev(port->dev);
-	unsigned int dma_devfn = PCI_DEVFN(PCI_SLOT(pdev->devfn), 0);
-	struct pci_dev *dma_dev = pci_get_slot(pdev->bus, dma_devfn);
+	struct pci_dev *dma_dev;
 
 	switch (pdev->device) {
 	case PCI_DEVICE_ID_INTEL_BYT_UART1:
@@ -137,6 +136,8 @@ static int byt_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 		return -EINVAL;
 	}
 
+	dma_dev = pci_get_slot(pdev->bus, PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
+
 	param->dma_dev = &dma_dev->dev;
 	param->m_master = 0;
 	param->p_master = 1;
@@ -152,6 +153,14 @@ static int byt_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 	return 0;
 }
 
+static void byt_serial_exit(struct lpss8250 *lpss)
+{
+	struct dw_dma_slave *param = &lpss->dma_param;
+
+	/* Paired with pci_get_slot() in the byt_serial_setup() above */
+	put_device(param->dma_dev);
+}
+
 static int ehl_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 {
 	struct uart_8250_dma *dma = &lpss->data.dma;
@@ -170,6 +179,13 @@ static int ehl_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 	return 0;
 }
 
+static void ehl_serial_exit(struct lpss8250 *lpss)
+{
+	struct uart_8250_port *up = serial8250_get_port(lpss->data.line);
+
+	up->dma = NULL;
+}
+
 #ifdef CONFIG_SERIAL_8250_DMA
 static const struct dw_dma_platform_data qrk_serial_dma_pdata = {
 	.nr_channels = 2,
@@ -344,8 +360,7 @@ static int lpss8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return 0;
 
 err_exit:
-	if (lpss->board->exit)
-		lpss->board->exit(lpss);
+	lpss->board->exit(lpss);
 	pci_free_irq_vectors(pdev);
 	return ret;
 }
@@ -356,8 +371,7 @@ static void lpss8250_remove(struct pci_dev *pdev)
 
 	serial8250_unregister_port(lpss->data.line);
 
-	if (lpss->board->exit)
-		lpss->board->exit(lpss);
+	lpss->board->exit(lpss);
 	pci_free_irq_vectors(pdev);
 }
 
@@ -365,12 +379,14 @@ static const struct lpss8250_board byt_board = {
 	.freq = 100000000,
 	.base_baud = 2764800,
 	.setup = byt_serial_setup,
+	.exit = byt_serial_exit,
 };
 
 static const struct lpss8250_board ehl_board = {
 	.freq = 200000000,
 	.base_baud = 12500000,
 	.setup = ehl_serial_setup,
+	.exit = ehl_serial_exit,
 };
 
 static const struct lpss8250_board qrk_board = {
-- 
2.34.1

[PATCH v2 2/2] serial: 8250_lpss: Switch to pcim_iomap() instead of pci_ioremap_bar()

[Andy Shevchenko]

The pci_iounmap() doesn't cover all the cases where resource should
be unmapped. Instead of spreading it more, replace the pci_ioremap_bar()
with pcim_iomap() which uses managed resource approach.

Fixes: fecdef932b00 ("serial: 8250_lpss: enable DMA on Intel Quark UART")
Depends-on: ea5ab2e422de ("8250_lpss: check null return when calling pci_ioremap_bar")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new fix (by code inspection due to previous patch)
 drivers/tty/serial/8250/8250_lpss.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 0f5af061e0b4..a9fc1d7d9c37 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -209,7 +209,7 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 	chip->dev = &pdev->dev;
 	chip->id = pdev->devfn;
 	chip->irq = pci_irq_vector(pdev, 0);
-	chip->regs = pci_ioremap_bar(pdev, 1);
+	chip->regs = pcim_iomap(pdev, 1, 0);
 	if (!chip->regs)
 		return;
 
@@ -241,8 +241,6 @@ static void qrk_serial_exit_dma(struct lpss8250 *lpss)
 		return;
 
 	dw_dma_remove(chip);
-
-	pci_iounmap(to_pci_dev(chip->dev), chip->regs);
 }
 #else	/* CONFIG_SERIAL_8250_DMA */
 static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port) {}
-- 
2.34.1

Re: [PATCH v2 2/2] serial: 8250_lpss: Switch to pcim_iomap() instead of pci_ioremap_bar()

[Christoph Hellwig]

On Tue, Feb 15, 2022 at 03:43:59PM +0200, Andy Shevchenko wrote:
> The pci_iounmap() doesn't cover all the cases where resource should
> be unmapped. Instead of spreading it more, replace the pci_ioremap_bar()
> with pcim_iomap() which uses managed resource approach.

pcim_iomap requires the use of ioreadX/iowriteX and thus runtime
overhead.  So in doubt please add a pcim_ioremap_bar instead of forcing
the legacy iomap/ioread/iowrite API onto modern drivers tht can't
support legacy port I/O.

Re: [PATCH v2 2/2] serial: 8250_lpss: Switch to pcim_iomap() instead of pci_ioremap_bar()

[Jiri Slaby]

On 16. 02. 22, 9:53, Christoph Hellwig wrote:
> On Tue, Feb 15, 2022 at 03:43:59PM +0200, Andy Shevchenko wrote:
>> The pci_iounmap() doesn't cover all the cases where resource should
>> be unmapped. Instead of spreading it more, replace the pci_ioremap_bar()
>> with pcim_iomap() which uses managed resource approach.
> 
> pcim_iomap requires the use of ioreadX/iowriteX and thus runtime
> overhead.  So in doubt please add a pcim_ioremap_bar instead of forcing
> the legacy iomap/ioread/iowrite API onto modern drivers tht can't
> support legacy port I/O.

Hmm, the driver combines pci_ioremap_bar with pci_iounmap. pci_iounmap 
does the right thing after all, but is that correct? And this driver is 
not alone, this shows more:
git grep -E 'pci_iounmap|pci_ioremap_bar' `git grep -l pci_iounmap \`git 
grep -l pci_ioremap_bar\``

-- 
js
suse labs

Re: [PATCH v2 2/2] serial: 8250_lpss: Switch to pcim_iomap() instead of pci_ioremap_bar()

[Christoph Hellwig]

On Tue, Feb 22, 2022 at 10:14:16AM +0100, Jiri Slaby wrote:
> On 16. 02. 22, 9:53, Christoph Hellwig wrote:
> > On Tue, Feb 15, 2022 at 03:43:59PM +0200, Andy Shevchenko wrote:
> > > The pci_iounmap() doesn't cover all the cases where resource should
> > > be unmapped. Instead of spreading it more, replace the pci_ioremap_bar()
> > > with pcim_iomap() which uses managed resource approach.
> > 
> > pcim_iomap requires the use of ioreadX/iowriteX and thus runtime
> > overhead.  So in doubt please add a pcim_ioremap_bar instead of forcing
> > the legacy iomap/ioread/iowrite API onto modern drivers tht can't
> > support legacy port I/O.
> 
> Hmm, the driver combines pci_ioremap_bar with pci_iounmap. pci_iounmap does
> the right thing after all, but is that correct? And this driver is not
> alone, this shows more:
> git grep -E 'pci_iounmap|pci_ioremap_bar' `git grep -l pci_iounmap \`git
> grep -l pci_ioremap_bar\``

I think it is wrong.  It is not actively harmful unlike the the
combination of pci_iomap and then later use of accessors from the
ioremap family, but still not exactly a good idea.

In a perfect world we'd have some different annotation from __iomem
for the whole iomap family of functions.

Re: [PATCH v2 2/2] serial: 8250_lpss: Switch to pcim_iomap() instead of pci_ioremap_bar()

[Andy Shevchenko]

On Tue, Feb 22, 2022 at 08:02:10AM -0800, Christoph Hellwig wrote:
> On Tue, Feb 22, 2022 at 10:14:16AM +0100, Jiri Slaby wrote:
> > On 16. 02. 22, 9:53, Christoph Hellwig wrote:
> > > On Tue, Feb 15, 2022 at 03:43:59PM +0200, Andy Shevchenko wrote:
> > > > The pci_iounmap() doesn't cover all the cases where resource should
> > > > be unmapped. Instead of spreading it more, replace the pci_ioremap_bar()
> > > > with pcim_iomap() which uses managed resource approach.
> > > 
> > > pcim_iomap requires the use of ioreadX/iowriteX and thus runtime
> > > overhead.  So in doubt please add a pcim_ioremap_bar instead of forcing
> > > the legacy iomap/ioread/iowrite API onto modern drivers tht can't
> > > support legacy port I/O.
> > 
> > Hmm, the driver combines pci_ioremap_bar with pci_iounmap. pci_iounmap does
> > the right thing after all, but is that correct? And this driver is not
> > alone, this shows more:
> > git grep -E 'pci_iounmap|pci_ioremap_bar' `git grep -l pci_iounmap \`git
> > grep -l pci_ioremap_bar\``
> 
> I think it is wrong.  It is not actively harmful unlike the the
> combination of pci_iomap and then later use of accessors from the
> ioremap family, but still not exactly a good idea.
> 
> In a perfect world we'd have some different annotation from __iomem
> for the whole iomap family of functions.

So, what would be your suggestion for a) backportable change b) cleanup for
the current and future drivers?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] serial: 8250_lpss: Switch to pcim_iomap() instead of pci_ioremap_bar()

[Greg Kroah-Hartman]

On Wed, Feb 23, 2022 at 01:08:07PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 22, 2022 at 08:02:10AM -0800, Christoph Hellwig wrote:
> > On Tue, Feb 22, 2022 at 10:14:16AM +0100, Jiri Slaby wrote:
> > > On 16. 02. 22, 9:53, Christoph Hellwig wrote:
> > > > On Tue, Feb 15, 2022 at 03:43:59PM +0200, Andy Shevchenko wrote:
> > > > > The pci_iounmap() doesn't cover all the cases where resource should
> > > > > be unmapped. Instead of spreading it more, replace the pci_ioremap_bar()
> > > > > with pcim_iomap() which uses managed resource approach.
> > > > 
> > > > pcim_iomap requires the use of ioreadX/iowriteX and thus runtime
> > > > overhead.  So in doubt please add a pcim_ioremap_bar instead of forcing
> > > > the legacy iomap/ioread/iowrite API onto modern drivers tht can't
> > > > support legacy port I/O.
> > > 
> > > Hmm, the driver combines pci_ioremap_bar with pci_iounmap. pci_iounmap does
> > > the right thing after all, but is that correct? And this driver is not
> > > alone, this shows more:
> > > git grep -E 'pci_iounmap|pci_ioremap_bar' `git grep -l pci_iounmap \`git
> > > grep -l pci_ioremap_bar\``
> > 
> > I think it is wrong.  It is not actively harmful unlike the the
> > combination of pci_iomap and then later use of accessors from the
> > ioremap family, but still not exactly a good idea.
> > 
> > In a perfect world we'd have some different annotation from __iomem
> > for the whole iomap family of functions.
> 
> So, what would be your suggestion for a) backportable change b) cleanup for
> the current and future drivers?

Worry about getting it right first.  Only after that should you even
consider stable tree backports.  There's usually no reason you can't
just take the same change there as well.  And if not, we will work
through it :)

thanks,

greg k-h

Re: [PATCH v2 2/2] serial: 8250_lpss: Switch to pcim_iomap() instead of pci_ioremap_bar()

[Andy Shevchenko]

On Wed, Feb 23, 2022 at 12:13:07PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Feb 23, 2022 at 01:08:07PM +0200, Andy Shevchenko wrote:
> > On Tue, Feb 22, 2022 at 08:02:10AM -0800, Christoph Hellwig wrote:
> > > On Tue, Feb 22, 2022 at 10:14:16AM +0100, Jiri Slaby wrote:
> > > > On 16. 02. 22, 9:53, Christoph Hellwig wrote:
> > > > > On Tue, Feb 15, 2022 at 03:43:59PM +0200, Andy Shevchenko wrote:
> > > > > > The pci_iounmap() doesn't cover all the cases where resource should
> > > > > > be unmapped. Instead of spreading it more, replace the pci_ioremap_bar()
> > > > > > with pcim_iomap() which uses managed resource approach.
> > > > > 
> > > > > pcim_iomap requires the use of ioreadX/iowriteX and thus runtime
> > > > > overhead.  So in doubt please add a pcim_ioremap_bar instead of forcing
> > > > > the legacy iomap/ioread/iowrite API onto modern drivers tht can't
> > > > > support legacy port I/O.
> > > > 
> > > > Hmm, the driver combines pci_ioremap_bar with pci_iounmap. pci_iounmap does
> > > > the right thing after all, but is that correct? And this driver is not
> > > > alone, this shows more:
> > > > git grep -E 'pci_iounmap|pci_ioremap_bar' `git grep -l pci_iounmap \`git
> > > > grep -l pci_ioremap_bar\``
> > > 
> > > I think it is wrong.  It is not actively harmful unlike the the
> > > combination of pci_iomap and then later use of accessors from the
> > > ioremap family, but still not exactly a good idea.
> > > 
> > > In a perfect world we'd have some different annotation from __iomem
> > > for the whole iomap family of functions.
> > 
> > So, what would be your suggestion for a) backportable change b) cleanup for
> > the current and future drivers?
> 
> Worry about getting it right first.  Only after that should you even
> consider stable tree backports.  There's usually no reason you can't
> just take the same change there as well.  And if not, we will work
> through it :)

Okay, so if I read this thread correctly Christoph suggests to introduce
pcim_ioremap_bar() and then use it. Am I right?

Christoph, since we are on the topic about pcim_*() APIs, can you chime in
the discussion [1] about IRQ vectors allocation?

[1]: https://lore.kernel.org/all/20210607153916.1021016-1-zhengdejin5@gmail.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] serial: 8250_lpss: Switch to pcim_iomap() instead of pci_ioremap_bar()

[Christoph Hellwig]

On Wed, Feb 23, 2022 at 01:37:18PM +0200, Andy Shevchenko wrote:
> Okay, so if I read this thread correctly Christoph suggests to introduce
> pcim_ioremap_bar() and then use it. Am I right?

Yes.

> 
> Christoph, since we are on the topic about pcim_*() APIs, can you chime in
> the discussion [1] about IRQ vectors allocation?
> 
> [1]: https://lore.kernel.org/all/20210607153916.1021016-1-zhengdejin5@gmail.com/

Did you intend to link to a 8 month old series or is there something
else this should point to?

Re: [PATCH v2 2/2] serial: 8250_lpss: Switch to pcim_iomap() instead of pci_ioremap_bar()

[Andy Shevchenko]

On Wed, Feb 23, 2022 at 04:20:16AM -0800, Christoph Hellwig wrote:
> On Wed, Feb 23, 2022 at 01:37:18PM +0200, Andy Shevchenko wrote:
> > Okay, so if I read this thread correctly Christoph suggests to introduce
> > pcim_ioremap_bar() and then use it. Am I right?
> 
> Yes.

Thanks for clarification!

> > Christoph, since we are on the topic about pcim_*() APIs, can you chime in
> > the discussion [1] about IRQ vectors allocation?
> > 
> > [1]: https://lore.kernel.org/all/20210607153916.1021016-1-zhengdejin5@gmail.com/
> 
> Did you intend to link to a 8 month old series or is there something
> else this should point to?

Yes, because it seems stalled.

-- 
With Best Regards,
Andy Shevchenko