[PATCH] nvme: Use pci_iomap instead of ioremap

[PATCH] nvme: Use pci_iomap instead of ioremap

[Pankaj Raghav]

Use the better checked pci_iomap instead of ioremap.

Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 drivers/nvme/host/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d8585df2c2fd..6c8aad9ff7db 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1800,7 +1800,7 @@ static int nvme_remap_bar(struct nvme_dev *dev, unsigned long size)
 		return -ENOMEM;
 	if (dev->bar)
 		iounmap(dev->bar);
-	dev->bar = ioremap(pci_resource_start(pdev, 0), size);
+	dev->bar = pci_iomap(pdev, 0, size);
 	if (!dev->bar) {
 		dev->bar_mapped_size = 0;
 		return -ENOMEM;
-- 
2.25.1

Re: [PATCH] nvme: Use pci_iomap instead of ioremap

[Christoph Hellwig]

pci_iomap goes with the ioread/iowrite accessors which we absolutely
do not want in nvme.

Re: [PATCH] nvme: Use pci_iomap instead of ioremap

[Keith Busch]

On Wed, Jan 26, 2022 at 05:33:06PM +0100, Christoph Hellwig wrote:
> pci_iomap goes with the ioread/iowrite accessors which we absolutely
> do not want in nvme.

It looks like the action pci_iomap() uses is based on the
pci_resource_flags(), and should call the same "ioremap()" function we
currently use.

The one problem I see with this, though, is that pci_iomap() will
automatically clamp the mapped length to the resource's length, which
may be shorter than the potential doorbell registers that the driver
thinks it's getting.

Re: [PATCH] nvme: Use pci_iomap instead of ioremap

[Christoph Hellwig]

On Wed, Jan 26, 2022 at 09:22:04AM -0800, Keith Busch wrote:
> On Wed, Jan 26, 2022 at 05:33:06PM +0100, Christoph Hellwig wrote:
> > pci_iomap goes with the ioread/iowrite accessors which we absolutely
> > do not want in nvme.
> 
> It looks like the action pci_iomap() uses is based on the
> pci_resource_flags(), and should call the same "ioremap()" function we
> currently use.

Yes.  But the rule is: use ioremap when you use read*/write*, and use
pci_iomap when using ioread/iowrite.  But they should never be mixed.

Re: [PATCH] nvme: Use pci_iomap instead of ioremap

[Pankaj Raghav]

On Wed, Jan 26, 2022 at 05:33:06PM +0100, Christoph Hellwig wrote:
> pci_iomap goes with the ioread/iowrite accessors which we absolutely
> do not want in nvme.
Is it because of ioread/iowrite has a conditional check everytime we do
a read/write with IO_COND? Or is there any other reason?

--
Pankaj

Re: [PATCH] nvme: Use pci_iomap instead of ioremap

[Pankaj Raghav]

On 2022-01-26 22:52, Keith Busch wrote:
> On Wed, Jan 26, 2022 at 05:33:06PM +0100, Christoph Hellwig wrote:
> The one problem I see with this, though, is that pci_iomap() will
> automatically clamp the mapped length to the resource's length, which
> may be shorter than the potential doorbell registers that the driver
> thinks it's getting.

The current implementation disallow the size to be greater than
the pci_resource_len(pdev, 0).

if (size > pci_resource_len(pdev, 0))
    return -ENOMEM;

So, clamping shouldn't be an issue when we use pci_iomap.

-- 
Regards,
Pankaj

Re: [PATCH] nvme: Use pci_iomap instead of ioremap

[Christoph Hellwig]

On Thu, Jan 27, 2022 at 06:35:41PM +0530, Pankaj Raghav wrote:
> On Wed, Jan 26, 2022 at 05:33:06PM +0100, Christoph Hellwig wrote:
> > pci_iomap goes with the ioread/iowrite accessors which we absolutely
> > do not want in nvme.
> Is it because of ioread/iowrite has a conditional check everytime we do
> a read/write with IO_COND? Or is there any other reason?

Yes, ioread* and iowrite* are more expensive.  And nvme doesn't support
port I/O anyway so there is no point.

Re: [PATCH] nvme: Use pci_iomap instead of ioremap

[Pankaj Raghav]

On 2022-01-27 20:15, Christoph Hellwig wrote:
> On Thu, Jan 27, 2022 at 06:35:41PM +0530, Pankaj Raghav wrote:
>> On Wed, Jan 26, 2022 at 05:33:06PM +0100, Christoph Hellwig wrote:
>>> pci_iomap goes with the ioread/iowrite accessors which we absolutely
>>> do not want in nvme.
>> Is it because of ioread/iowrite has a conditional check everytime we do
>> a read/write with IO_COND? Or is there any other reason?
> 
> Yes, ioread* and iowrite* are more expensive.  And nvme doesn't support
> port I/O anyway so there is no point.

Ok. 

`ioremap` has a comment to prefer pci_iomap if the area we are trying 
to map is a PCI BAR. Does it makes sense then to have a comment in 
the nvme pci driver as to why ioremap is preferred over pci_iomap?

-- 
Regards,
Pankaj

Re: [PATCH] nvme: Use pci_iomap instead of ioremap

[Christoph Hellwig]

On Thu, Jan 27, 2022 at 09:26:28PM +0530, Pankaj Raghav wrote:
> `ioremap` has a comment to prefer pci_iomap if the area we are trying 
> to map is a PCI BAR. Does it makes sense then to have a comment in 
> the nvme pci driver as to why ioremap is preferred over pci_iomap?

I don't think that comment is correct.  pci_iomap is not a replacement
for ioremap, but an interface that helps drivers that need to
transparently deal with MMIO and Port I/O - something that is mostly
of historic interest these days.