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