diff mbox series

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

Message ID 20220215134359.78169-2-andriy.shevchenko@linux.intel.com
State New
Headers show
Series [v2,1/2] serial: 8250_lpss: Balance reference count for PCI DMA device | expand

Commit Message

Andy Shevchenko Feb. 15, 2022, 1:43 p.m. UTC
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(-)

Comments

Christoph Hellwig Feb. 16, 2022, 8:53 a.m. UTC | #1
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.
Jiri Slaby Feb. 22, 2022, 9:14 a.m. UTC | #2
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\``
Christoph Hellwig Feb. 22, 2022, 4:02 p.m. UTC | #3
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.
Andy Shevchenko Feb. 23, 2022, 11:08 a.m. UTC | #4
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?
Greg KH Feb. 23, 2022, 11:13 a.m. UTC | #5
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
Andy Shevchenko Feb. 23, 2022, 11:37 a.m. UTC | #6
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/
Christoph Hellwig Feb. 23, 2022, 12:20 p.m. UTC | #7
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?
Andy Shevchenko Feb. 23, 2022, 2:23 p.m. UTC | #8
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.
diff mbox series

Patch

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) {}