| 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 |
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.
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\``
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.
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?
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
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/
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?
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 --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) {}
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(-)