| Message ID | 20240819165148.58201-10-pstanner@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Remove pcim_iounmap_regions() | expand |
Le 19/08/2024 à 18:51, Philipp Stanner a écrit : > solidrun utilizes pcim_iomap_regions(), which has been deprecated by the > PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate > pcim_iomap_table(), pcim_iomap_regions_request_all()"), among other > things because it forces usage of quite a complicated bitmask mechanism. > The bitmask handling code can entirely be removed by replacing > pcim_iomap_regions() and pcim_iomap_table(). > > Replace pcim_iomap_regions() and pcim_iomap_table() with > pci_iomap_region(). > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > --- > drivers/vdpa/solidrun/snet_main.c | 47 +++++++++++-------------------- > 1 file changed, 16 insertions(+), 31 deletions(-) > > diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c > index 99428a04068d..abf027ca35e1 100644 > --- a/drivers/vdpa/solidrun/snet_main.c > +++ b/drivers/vdpa/solidrun/snet_main.c > @@ -556,33 +556,24 @@ static const struct vdpa_config_ops snet_config_ops = { > static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet) > { > char name[50]; > - int ret, i, mask = 0; > + int i; > + > + snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev)); > + > /* We don't know which BAR will be used to communicate.. > * We will map every bar with len > 0. > * > * Later, we will discover the BAR and unmap all other BARs. > */ > for (i = 0; i < PCI_STD_NUM_BARS; i++) { > - if (pci_resource_len(pdev, i)) > - mask |= (1 << i); > - } > - > - /* No BAR can be used.. */ > - if (!mask) { > - SNET_ERR(pdev, "Failed to find a PCI BAR\n"); > - return -ENODEV; > - } > - > - snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev)); > - ret = pcim_iomap_regions(pdev, mask, name); > - if (ret) { > - SNET_ERR(pdev, "Failed to request and map PCI BARs\n"); > - return ret; > - } > + if (pci_resource_len(pdev, i)) { > + psnet->bars[i] = pcim_iomap_region(pdev, i, name); Hi, Unrelated to the patch, but is is safe to have 'name' be on the stack? pcim_iomap_region() --> __pcim_request_region() --> __pcim_request_region_range() --> request_region() or __request_mem_region() --> __request_region() --> __request_region_locked() --> res->name = name; So an address on the stack ends in the 'name' field of a "struct resource". According to a few grep, it looks really unusual. I don't know if it is used, but it looks strange to me. If it is an issue, it was apparently already there before this patch. > + if (IS_ERR(psnet->bars[i])) { > + SNET_ERR(pdev, "Failed to request and map PCI BARs\n"); > + return PTR_ERR(psnet->bars[i]); > + } > + } > > - for (i = 0; i < PCI_STD_NUM_BARS; i++) { > - if (mask & (1 << i)) > - psnet->bars[i] = pcim_iomap_table(pdev)[i]; > } > > return 0; > @@ -591,18 +582,15 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet) > static int snet_open_vf_bar(struct pci_dev *pdev, struct snet *snet) > { > char name[50]; > - int ret; > > snprintf(name, sizeof(name), "snet[%s]-bar", pci_name(pdev)); > /* Request and map BAR */ > - ret = pcim_iomap_regions(pdev, BIT(snet->psnet->cfg.vf_bar), name); > - if (ret) { > + snet->bar = pcim_iomap_region(pdev, snet->psnet->cfg.vf_bar, name); Same Just my 2c. CJ > + if (IS_ERR(snet->bar)) { > SNET_ERR(pdev, "Failed to request and map PCI BAR for a VF\n"); > - return ret; > + return PTR_ERR(snet->bar); > } > > - snet->bar = pcim_iomap_table(pdev)[snet->psnet->cfg.vf_bar]; > - > return 0; > } > > @@ -650,15 +638,12 @@ static int psnet_detect_bar(struct psnet *psnet, u32 off) > > static void psnet_unmap_unused_bars(struct pci_dev *pdev, struct psnet *psnet) > { > - int i, mask = 0; > + int i; > > for (i = 0; i < PCI_STD_NUM_BARS; i++) { > if (psnet->bars[i] && i != psnet->barno) > - mask |= (1 << i); > + pcim_iounmap_region(pdev, i); > } > - > - if (mask) > - pcim_iounmap_regions(pdev, mask); > } > > /* Read SNET config from PCI BAR */
On Mon, Aug 19, 2024 at 06:51:48PM +0200, Philipp Stanner wrote: > solidrun utilizes pcim_iomap_regions(), which has been deprecated by the > PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate > pcim_iomap_table(), pcim_iomap_regions_request_all()"), among other > things because it forces usage of quite a complicated bitmask mechanism. > The bitmask handling code can entirely be removed by replacing > pcim_iomap_regions() and pcim_iomap_table(). > > Replace pcim_iomap_regions() and pcim_iomap_table() with > pci_iomap_region(). ... > - int ret, i, mask = 0; > + int i; Make it signed? ... > for (i = 0; i < PCI_STD_NUM_BARS; i++) { > + if (pci_resource_len(pdev, i)) { > + psnet->bars[i] = pcim_iomap_region(pdev, i, name); > + if (IS_ERR(psnet->bars[i])) { > + SNET_ERR(pdev, "Failed to request and map PCI BARs\n"); > + return PTR_ERR(psnet->bars[i]); > + } > + } > Blank line leftover. > }
On Mon, Aug 19, 2024 at 08:19:28PM +0200, Christophe JAILLET wrote: > Le 19/08/2024 à 18:51, Philipp Stanner a écrit : ... > Unrelated to the patch, but is is safe to have 'name' be on the stack? > > pcim_iomap_region() > --> __pcim_request_region() > --> __pcim_request_region_range() > --> request_region() or __request_mem_region() > --> __request_region() > --> __request_region_locked() > --> res->name = name; > > So an address on the stack ends in the 'name' field of a "struct resource". > > According to a few grep, it looks really unusual. > > I don't know if it is used, but it looks strange to me. It might be used when printing /proc/iomem, but I don't remember by heart. > If it is an issue, it was apparently already there before this patch. This series seems to reveal a lot of issues with the probe/remove in many drivers. I think it's better to make fixes of them before this series for the sake of easier backporting. If here is a problem, the devm_kasprintf() should be used.
On Mon, Aug 19, 2024 at 09:32:00PM +0300, Andy Shevchenko wrote: > On Mon, Aug 19, 2024 at 06:51:48PM +0200, Philipp Stanner wrote: ... > > for (i = 0; i < PCI_STD_NUM_BARS; i++) { > > + if (pci_resource_len(pdev, i)) { Btw, looking at this you may invert the conditional for (i = 0; i < PCI_STD_NUM_BARS; i++) { if (!pci_resource_len(pdev, i)) continue; It might make patch neater. > > + psnet->bars[i] = pcim_iomap_region(pdev, i, name); > > + if (IS_ERR(psnet->bars[i])) { > > + SNET_ERR(pdev, "Failed to request and map PCI BARs\n"); > > + return PTR_ERR(psnet->bars[i]); > > + } > > + } > > > > Blank line leftover. > > > }
On Mon, 2024-08-19 at 20:19 +0200, Christophe JAILLET wrote: > Le 19/08/2024 à 18:51, Philipp Stanner a écrit : > > solidrun utilizes pcim_iomap_regions(), which has been deprecated > > by the > > PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate > > pcim_iomap_table(), pcim_iomap_regions_request_all()"), among other > > things because it forces usage of quite a complicated bitmask > > mechanism. > > The bitmask handling code can entirely be removed by replacing > > pcim_iomap_regions() and pcim_iomap_table(). > > > > Replace pcim_iomap_regions() and pcim_iomap_table() with > > pci_iomap_region(). > > > > Signed-off-by: Philipp Stanner <pstanner@redhat.com> > > --- > > drivers/vdpa/solidrun/snet_main.c | 47 +++++++++++--------------- > > ----- > > 1 file changed, 16 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/vdpa/solidrun/snet_main.c > > b/drivers/vdpa/solidrun/snet_main.c > > index 99428a04068d..abf027ca35e1 100644 > > --- a/drivers/vdpa/solidrun/snet_main.c > > +++ b/drivers/vdpa/solidrun/snet_main.c > > @@ -556,33 +556,24 @@ static const struct vdpa_config_ops > > snet_config_ops = { > > static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet > > *psnet) > > { > > char name[50]; > > - int ret, i, mask = 0; > > + int i; > > + > > + snprintf(name, sizeof(name), "psnet[%s]-bars", > > pci_name(pdev)); > > + > > /* We don't know which BAR will be used to communicate.. > > * We will map every bar with len > 0. > > * > > * Later, we will discover the BAR and unmap all other > > BARs. > > */ > > for (i = 0; i < PCI_STD_NUM_BARS; i++) { > > - if (pci_resource_len(pdev, i)) > > - mask |= (1 << i); > > - } > > - > > - /* No BAR can be used.. */ > > - if (!mask) { > > - SNET_ERR(pdev, "Failed to find a PCI BAR\n"); > > - return -ENODEV; > > - } > > - > > - snprintf(name, sizeof(name), "psnet[%s]-bars", > > pci_name(pdev)); > > - ret = pcim_iomap_regions(pdev, mask, name); > > - if (ret) { > > - SNET_ERR(pdev, "Failed to request and map PCI > > BARs\n"); > > - return ret; > > - } > > + if (pci_resource_len(pdev, i)) { > > + psnet->bars[i] = pcim_iomap_region(pdev, > > i, name); > > Hi, > > Unrelated to the patch, but is is safe to have 'name' be on the > stack? > > pcim_iomap_region() > --> __pcim_request_region() > --> __pcim_request_region_range() > --> request_region() or __request_mem_region() > --> __request_region() > --> __request_region_locked() > --> res->name = name; > > So an address on the stack ends in the 'name' field of a "struct > resource". Oh oh... > > According to a few grep, it looks really unusual. > > I don't know if it is used, but it looks strange to me. I have seen it used in the kernel ringbuffer log when you try to request something that's already owned. I think it's here, right in __request_region_locked(): /* * mm/hmm.c reserves physical addresses which then * become unavailable to other users. Conflicts are * not expected. Warn to aid debugging if encountered. */ if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) { pr_warn("Unaddressable device %s %pR conflicts with %pR", conflict->name, conflict, res); } Assuming I interpret the code correctly: The conflicting resource is found when a new caller (e.g. another driver) tries to get the same region. So conflict->name on the original requester's stack is by now gone and you do get UB. Very unlikely UB, since only rarely drivers race for the same resource, but still UB. But there's also a few other places. Grep for "conflict->name". > > > If it is an issue, it was apparently already there before this patch. I think this has to be fixed. Question would just be whether one wants to fix it locally in this driver, or prevent it from happening globally by making the common infrastructure copy the string. P. > > > + if (IS_ERR(psnet->bars[i])) { > > + SNET_ERR(pdev, "Failed to request > > and map PCI BARs\n"); > > + return PTR_ERR(psnet->bars[i]); > > + } > > + } > > > > - for (i = 0; i < PCI_STD_NUM_BARS; i++) { > > - if (mask & (1 << i)) > > - psnet->bars[i] = > > pcim_iomap_table(pdev)[i]; > > } > > > > return 0; > > @@ -591,18 +582,15 @@ static int psnet_open_pf_bar(struct pci_dev > > *pdev, struct psnet *psnet) > > static int snet_open_vf_bar(struct pci_dev *pdev, struct snet > > *snet) > > { > > char name[50]; > > - int ret; > > > > snprintf(name, sizeof(name), "snet[%s]-bar", > > pci_name(pdev)); > > /* Request and map BAR */ > > - ret = pcim_iomap_regions(pdev, BIT(snet->psnet- > > >cfg.vf_bar), name); > > - if (ret) { > > + snet->bar = pcim_iomap_region(pdev, snet->psnet- > > >cfg.vf_bar, name); > > Same > > Just my 2c. > > CJ > > > + if (IS_ERR(snet->bar)) { > > SNET_ERR(pdev, "Failed to request and map PCI BAR > > for a VF\n"); > > - return ret; > > + return PTR_ERR(snet->bar); > > } > > > > - snet->bar = pcim_iomap_table(pdev)[snet->psnet- > > >cfg.vf_bar]; > > - > > return 0; > > } > > > > @@ -650,15 +638,12 @@ static int psnet_detect_bar(struct psnet > > *psnet, u32 off) > > > > static void psnet_unmap_unused_bars(struct pci_dev *pdev, struct > > psnet *psnet) > > { > > - int i, mask = 0; > > + int i; > > > > for (i = 0; i < PCI_STD_NUM_BARS; i++) { > > if (psnet->bars[i] && i != psnet->barno) > > - mask |= (1 << i); > > + pcim_iounmap_region(pdev, i); > > } > > - > > - if (mask) > > - pcim_iounmap_regions(pdev, mask); > > } > > > > /* Read SNET config from PCI BAR */ > >
On Mon, 2024-08-19 at 21:34 +0300, Andy Shevchenko wrote: > On Mon, Aug 19, 2024 at 08:19:28PM +0200, Christophe JAILLET wrote: > > Le 19/08/2024 à 18:51, Philipp Stanner a écrit : > > > ... > > > Unrelated to the patch, but is is safe to have 'name' be on the > > stack? > > > > pcim_iomap_region() > > --> __pcim_request_region() > > --> __pcim_request_region_range() > > --> request_region() or __request_mem_region() > > --> __request_region() > > --> __request_region_locked() > > --> res->name = name; > > > > So an address on the stack ends in the 'name' field of a "struct > > resource". > > > > According to a few grep, it looks really unusual. > > > > I don't know if it is used, but it looks strange to me. > > It might be used when printing /proc/iomem, but I don't remember by > heart. > > > If it is an issue, it was apparently already there before this > > patch. > > This series seems to reveal a lot of issues with the probe/remove in > many > drivers. I think it's better to make fixes of them before this series > for > the sake of easier backporting. Just so we're in sync: I think the only real bug here so far is the one found by Christophe. The usages of pci_disable_device(), pcim_iounmap_regions() and the like in remove() and error unwind paths are not elegant and make devres kind of useless – but they are not bugs. So I wouldn't backport them. P. > > If here is a problem, the devm_kasprintf() should be used. >
On Tue, Aug 20, 2024 at 10:13:40AM +0200, Philipp Stanner wrote: > On Mon, 2024-08-19 at 21:34 +0300, Andy Shevchenko wrote: > > On Mon, Aug 19, 2024 at 08:19:28PM +0200, Christophe JAILLET wrote: > > > Le 19/08/2024 à 18:51, Philipp Stanner a écrit : ... > > > Unrelated to the patch, but is is safe to have 'name' be on the > > > stack? > > > > > > pcim_iomap_region() > > > --> __pcim_request_region() > > > --> __pcim_request_region_range() > > > --> request_region() or __request_mem_region() > > > --> __request_region() > > > --> __request_region_locked() > > > --> res->name = name; > > > > > > So an address on the stack ends in the 'name' field of a "struct > > > resource". > > > > > > According to a few grep, it looks really unusual. > > > > > > I don't know if it is used, but it looks strange to me. > > > > It might be used when printing /proc/iomem, but I don't remember by > > heart. > > > > > If it is an issue, it was apparently already there before this > > > patch. > > > > This series seems to reveal a lot of issues with the probe/remove in > > many > > drivers. I think it's better to make fixes of them before this series > > for > > the sake of easier backporting. > > Just so we're in sync: > I think the only real bug here so far is the one found by Christophe. > > The usages of pci_disable_device(), pcim_iounmap_regions() and the like > in remove() and error unwind paths are not elegant and make devres kind > of useless – but they are not bugs. So I wouldn't backport them. Agree. > > If here is a problem, the devm_kasprintf() should be used.
Le 20/08/2024 à 10:09, Philipp Stanner a écrit : >>> @@ -556,33 +556,24 @@ static const struct vdpa_config_ops >>> snet_config_ops = { >>> static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet >>> *psnet) >>> { >>> char name[50]; >>> - int ret, i, mask = 0; >>> + int i; >>> + >>> + snprintf(name, sizeof(name), "psnet[%s]-bars", >>> pci_name(pdev)); >>> + >>> /* We don't know which BAR will be used to communicate.. >>> * We will map every bar with len > 0. >>> * >>> * Later, we will discover the BAR and unmap all other >>> BARs. >>> */ >>> for (i = 0; i < PCI_STD_NUM_BARS; i++) { >>> - if (pci_resource_len(pdev, i)) >>> - mask |= (1 << i); >>> - } >>> - >>> - /* No BAR can be used.. */ >>> - if (!mask) { >>> - SNET_ERR(pdev, "Failed to find a PCI BAR\n"); >>> - return -ENODEV; >>> - } >>> - >>> - snprintf(name, sizeof(name), "psnet[%s]-bars", >>> pci_name(pdev)); >>> - ret = pcim_iomap_regions(pdev, mask, name); >>> - if (ret) { >>> - SNET_ERR(pdev, "Failed to request and map PCI >>> BARs\n"); >>> - return ret; >>> - } >>> + if (pci_resource_len(pdev, i)) { >>> + psnet->bars[i] = pcim_iomap_region(pdev, >>> i, name); >> >> Hi, >> >> Unrelated to the patch, but is is safe to have 'name' be on the >> stack? >> >> pcim_iomap_region() >> --> __pcim_request_region() >> --> __pcim_request_region_range() >> --> request_region() or __request_mem_region() >> --> __request_region() >> --> __request_region_locked() >> --> res->name = name; >> >> So an address on the stack ends in the 'name' field of a "struct >> resource". > > Oh oh... > >> >> According to a few grep, it looks really unusual. >> >> I don't know if it is used, but it looks strange to me. > > > I have seen it used in the kernel ringbuffer log when you try to > request something that's already owned. I think it's here, right in > __request_region_locked(): > > /* > * mm/hmm.c reserves physical addresses which then > * become unavailable to other users. Conflicts are > * not expected. Warn to aid debugging if encountered. > */ > if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) { > pr_warn("Unaddressable device %s %pR conflicts with %pR", > conflict->name, conflict, res); > } > > > Assuming I interpret the code correctly: > The conflicting resource is found when a new caller (e.g. another > driver) tries to get the same region. So conflict->name on the original > requester's stack is by now gone and you do get UB. > > Very unlikely UB, since only rarely drivers race for the same resource, > but still UB. > > But there's also a few other places. Grep for "conflict->name". > >> >> >> If it is an issue, it was apparently already there before this patch. > > I think this has to be fixed. > > Question would just be whether one wants to fix it locally in this > driver, or prevent it from happening globally by making the common > infrastructure copy the string. > > > P. > Not a perfect script, but the below coccinelle script only find this place, so I would +1 only fixing things here only. Agree? CJ @@ identifier name; expression x; constant N; @@ char name[N]; ... ( * pcim_iomap_region(..., name, ...); | * pcim_iomap_regions(..., name, ...); | * request_region(..., name, ...); | * x = pcim_iomap_region(..., name, ...); | * x = pcim_iomap_regions(..., name, ...); | * x = request_region(..., name, ...); )
On Tue, 2024-08-20 at 12:50 +0200, Christophe JAILLET wrote: > Le 20/08/2024 à 10:09, Philipp Stanner a écrit : > > > > @@ -556,33 +556,24 @@ static const struct vdpa_config_ops > > > > snet_config_ops = { > > > > static int psnet_open_pf_bar(struct pci_dev *pdev, struct > > > > psnet > > > > *psnet) > > > > { > > > > char name[50]; > > > > - int ret, i, mask = 0; > > > > + int i; > > > > + > > > > + snprintf(name, sizeof(name), "psnet[%s]-bars", > > > > pci_name(pdev)); > > > > + > > > > /* We don't know which BAR will be used to > > > > communicate.. > > > > * We will map every bar with len > 0. > > > > * > > > > * Later, we will discover the BAR and unmap all other > > > > BARs. > > > > */ > > > > for (i = 0; i < PCI_STD_NUM_BARS; i++) { > > > > - if (pci_resource_len(pdev, i)) > > > > - mask |= (1 << i); > > > > - } > > > > - > > > > - /* No BAR can be used.. */ > > > > - if (!mask) { > > > > - SNET_ERR(pdev, "Failed to find a PCI BAR\n"); > > > > - return -ENODEV; > > > > - } > > > > - > > > > - snprintf(name, sizeof(name), "psnet[%s]-bars", > > > > pci_name(pdev)); > > > > - ret = pcim_iomap_regions(pdev, mask, name); > > > > - if (ret) { > > > > - SNET_ERR(pdev, "Failed to request and map PCI > > > > BARs\n"); > > > > - return ret; > > > > - } > > > > + if (pci_resource_len(pdev, i)) { > > > > + psnet->bars[i] = > > > > pcim_iomap_region(pdev, > > > > i, name); > > > > > > Hi, > > > > > > Unrelated to the patch, but is is safe to have 'name' be on the > > > stack? > > > > > > pcim_iomap_region() > > > --> __pcim_request_region() > > > --> __pcim_request_region_range() > > > --> request_region() or __request_mem_region() > > > --> __request_region() > > > --> __request_region_locked() > > > --> res->name = name; > > > > > > So an address on the stack ends in the 'name' field of a "struct > > > resource". > > > > Oh oh... > > > > > > > > According to a few grep, it looks really unusual. > > > > > > I don't know if it is used, but it looks strange to me. > > > > > > I have seen it used in the kernel ringbuffer log when you try to > > request something that's already owned. I think it's here, right in > > __request_region_locked(): > > > > /* > > * mm/hmm.c reserves physical addresses which then > > * become unavailable to other users. Conflicts are > > * not expected. Warn to aid debugging if encountered. > > */ > > if (conflict->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY) { > > pr_warn("Unaddressable device %s %pR conflicts with %pR", > > conflict->name, conflict, res); > > } > > > > > > Assuming I interpret the code correctly: > > The conflicting resource is found when a new caller (e.g. another > > driver) tries to get the same region. So conflict->name on the > > original > > requester's stack is by now gone and you do get UB. > > > > Very unlikely UB, since only rarely drivers race for the same > > resource, > > but still UB. > > > > But there's also a few other places. Grep for "conflict->name". > > > > > > > > > > > If it is an issue, it was apparently already there before this > > > patch. > > > > I think this has to be fixed. > > > > Question would just be whether one wants to fix it locally in this > > driver, or prevent it from happening globally by making the common > > infrastructure copy the string. > > > > > > P. > > > > Not a perfect script, but the below coccinelle script only find this > place, so I would +1 only fixing things here only. > > Agree? Yup, sounds good. Copying the string would cause trouble (GFP flags) anyways. I'll provide a fix in v2. Thanks, P. > > CJ > > > > @@ > identifier name; > expression x; > constant N; > @@ > char name[N]; > ... > ( > * pcim_iomap_region(..., name, ...); > > > * pcim_iomap_regions(..., name, ...); > > > * request_region(..., name, ...); > > > * x = pcim_iomap_region(..., name, ...); > > > * x = pcim_iomap_regions(..., name, ...); > > > * x = request_region(..., name, ...); > ) > >
diff --git a/drivers/vdpa/solidrun/snet_main.c b/drivers/vdpa/solidrun/snet_main.c index 99428a04068d..abf027ca35e1 100644 --- a/drivers/vdpa/solidrun/snet_main.c +++ b/drivers/vdpa/solidrun/snet_main.c @@ -556,33 +556,24 @@ static const struct vdpa_config_ops snet_config_ops = { static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet) { char name[50]; - int ret, i, mask = 0; + int i; + + snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev)); + /* We don't know which BAR will be used to communicate.. * We will map every bar with len > 0. * * Later, we will discover the BAR and unmap all other BARs. */ for (i = 0; i < PCI_STD_NUM_BARS; i++) { - if (pci_resource_len(pdev, i)) - mask |= (1 << i); - } - - /* No BAR can be used.. */ - if (!mask) { - SNET_ERR(pdev, "Failed to find a PCI BAR\n"); - return -ENODEV; - } - - snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev)); - ret = pcim_iomap_regions(pdev, mask, name); - if (ret) { - SNET_ERR(pdev, "Failed to request and map PCI BARs\n"); - return ret; - } + if (pci_resource_len(pdev, i)) { + psnet->bars[i] = pcim_iomap_region(pdev, i, name); + if (IS_ERR(psnet->bars[i])) { + SNET_ERR(pdev, "Failed to request and map PCI BARs\n"); + return PTR_ERR(psnet->bars[i]); + } + } - for (i = 0; i < PCI_STD_NUM_BARS; i++) { - if (mask & (1 << i)) - psnet->bars[i] = pcim_iomap_table(pdev)[i]; } return 0; @@ -591,18 +582,15 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet) static int snet_open_vf_bar(struct pci_dev *pdev, struct snet *snet) { char name[50]; - int ret; snprintf(name, sizeof(name), "snet[%s]-bar", pci_name(pdev)); /* Request and map BAR */ - ret = pcim_iomap_regions(pdev, BIT(snet->psnet->cfg.vf_bar), name); - if (ret) { + snet->bar = pcim_iomap_region(pdev, snet->psnet->cfg.vf_bar, name); + if (IS_ERR(snet->bar)) { SNET_ERR(pdev, "Failed to request and map PCI BAR for a VF\n"); - return ret; + return PTR_ERR(snet->bar); } - snet->bar = pcim_iomap_table(pdev)[snet->psnet->cfg.vf_bar]; - return 0; } @@ -650,15 +638,12 @@ static int psnet_detect_bar(struct psnet *psnet, u32 off) static void psnet_unmap_unused_bars(struct pci_dev *pdev, struct psnet *psnet) { - int i, mask = 0; + int i; for (i = 0; i < PCI_STD_NUM_BARS; i++) { if (psnet->bars[i] && i != psnet->barno) - mask |= (1 << i); + pcim_iounmap_region(pdev, i); } - - if (mask) - pcim_iounmap_regions(pdev, mask); } /* Read SNET config from PCI BAR */
solidrun utilizes pcim_iomap_regions(), which has been deprecated by the PCI subsystem in commit e354bb84a4c1 ("PCI: Deprecate pcim_iomap_table(), pcim_iomap_regions_request_all()"), among other things because it forces usage of quite a complicated bitmask mechanism. The bitmask handling code can entirely be removed by replacing pcim_iomap_regions() and pcim_iomap_table(). Replace pcim_iomap_regions() and pcim_iomap_table() with pci_iomap_region(). Signed-off-by: Philipp Stanner <pstanner@redhat.com> --- drivers/vdpa/solidrun/snet_main.c | 47 +++++++++++-------------------- 1 file changed, 16 insertions(+), 31 deletions(-)