[PATCH] tty/nozomi: use pci_iomap instead of ioremap_nocache

[PATCH] tty/nozomi: use pci_iomap instead of ioremap_nocache

[Hugo Lefeuvre]

Use pci_iomap instead of ioremap_nocache in nozomi_card_init(). This
is a cleaner way to do PCI MMIO (performs additional checks) and
allows to drop the manual call to pci_resource_start.

pci_iomap relies on ioremap for MMIO and thus has uncached behavior.

Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
---
 drivers/tty/nozomi.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/tty/nozomi.c b/drivers/tty/nozomi.c
index fed820e9ab9d..3214e22e79f3 100644
--- a/drivers/tty/nozomi.c
+++ b/drivers/tty/nozomi.c
@@ -1317,7 +1317,6 @@ static void remove_sysfs_files(struct nozomi *dc)
 static int nozomi_card_init(struct pci_dev *pdev,
 				      const struct pci_device_id *ent)
 {
-	resource_size_t start;
 	int ret;
 	struct nozomi *dc = NULL;
 	int ndev_idx;
@@ -1357,17 +1356,10 @@ static int nozomi_card_init(struct pci_dev *pdev,
 		goto err_disable_device;
 	}
 
-	start = pci_resource_start(dc->pdev, 0);
-	if (start == 0) {
-		dev_err(&pdev->dev, "No I/O address for card detected\n");
-		ret = -ENODEV;
-		goto err_rel_regs;
-	}
-
 	/* Find out what card type it is */
 	nozomi_get_card_type(dc);
 
-	dc->base_addr = ioremap_nocache(start, dc->card_type);
+	dc->base_addr = pci_iomap(dc->pdev, 0, dc->card_type);
 	if (!dc->base_addr) {
 		dev_err(&pdev->dev, "Unable to map card MMIO\n");
 		ret = -ENODEV;
-- 
2.20.1

Re: [PATCH] tty/nozomi: use pci_iomap instead of ioremap_nocache

[Hugo Lefeuvre]

[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]

> Use pci_iomap instead of ioremap_nocache in nozomi_card_init(). This
> is a cleaner way to do PCI MMIO (performs additional checks) and
> allows to drop the manual call to pci_resource_start.
> 
> pci_iomap relies on ioremap for MMIO and thus has uncached behavior.

there's still something unclear to me about dc->card_type being used as
size argument to ioremap_nocache().

dc->base_addr is the sum of all six io region lengths, not the size of
region 0 which we are trying to map here. Why not using the size of region
0 instead ?

If the goal is to map all six regions "at once", I'm not sure how this is
supposed to work. Is there any kind of guarantee that all six regions will
be adjacent?

If this is a bug then this patch "somehow" already adresses it since
pci_iomap calls pci_resource_len itself. Otherwise this patch is broken.

-- 
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] tty/nozomi: use pci_iomap instead of ioremap_nocache

[Greg Kroah-Hartman]

On Sun, Feb 10, 2019 at 10:45:00PM +0100, Hugo Lefeuvre wrote:
> > Use pci_iomap instead of ioremap_nocache in nozomi_card_init(). This
> > is a cleaner way to do PCI MMIO (performs additional checks) and
> > allows to drop the manual call to pci_resource_start.
> > 
> > pci_iomap relies on ioremap for MMIO and thus has uncached behavior.
> 
> there's still something unclear to me about dc->card_type being used as
> size argument to ioremap_nocache().
> 
> dc->base_addr is the sum of all six io region lengths, not the size of
> region 0 which we are trying to map here. Why not using the size of region
> 0 instead ?

No idea, that might just be how the card is layed out.

> If the goal is to map all six regions "at once", I'm not sure how this is
> supposed to work. Is there any kind of guarantee that all six regions will
> be adjacent?

For some reason, it must happen that way, otherwise the driver would not
work very well :)

> If this is a bug then this patch "somehow" already adresses it since
> pci_iomap calls pci_resource_len itself. Otherwise this patch is broken.

Let's apply it and see if anyone screams...

thanks,

greg k-h

Re: [PATCH] tty/nozomi: use pci_iomap instead of ioremap_nocache

[Hugo Lefeuvre]

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]

(cc-ing Paul Hardwick, mentioned as maintainer of the driver)

Hi,

> > there's still something unclear to me about dc->card_type being used as
> > size argument to ioremap_nocache().
> > 
> > dc->base_addr is the sum of all six io region lengths, not the size of
> > region 0 which we are trying to map here. Why not using the size of region
> > 0 instead ?
> 
> No idea, that might just be how the card is layed out.
> 
> > If the goal is to map all six regions "at once", I'm not sure how this is
> > supposed to work. Is there any kind of guarantee that all six regions will
> > be adjacent?
> 
> For some reason, it must happen that way, otherwise the driver would not
> work very well :)
> 
> > If this is a bug then this patch "somehow" already adresses it since
> > pci_iomap calls pci_resource_len itself. Otherwise this patch is broken.
> 
> Let's apply it and see if anyone screams...

Hum, it looks very much like the intention here was to map all bars at
once.

The offsets corresponding to the downlink, uplink, etc. regions are
retrieved as part of the config table by nozomi_read_config_table().

Unfortunately I don't own test devices, so I will not be able to verify it
myself. This is easy to test, though: if I am right, this patch breaks the
driver.

I guess the right way to map all bars in a single buffer would look like
what the hifn_795x driver does[0].

I will provide a patch if somebody is available to test it.

regards,
 Hugo

[0] https://elixir.bootlin.com/linux/latest/source/drivers/crypto/hifn_795x.c#L2504

-- 
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] tty/nozomi: use pci_iomap instead of ioremap_nocache

[Hugo Lefeuvre]

> I will provide a patch if somebody is available to test it.

FTR, I have found test devices and should be able to run some tests by the
end of next week.

regards,
 Hugo

-- 
                Hugo Lefeuvre (hle)    |    www.owl.eu.com
RSA4096_ 360B 03B3 BF27 4F4D 7A3F D5E8 14AA 1EB8 A247 3DFD
ed25519_ 37B2 6D38 0B25 B8A2 6B9F 3A65 A36F 5357 5F2D DC4C