On Fri, Dec 13, 2024 at 07:39:46PM +0100, Philipp Stanner wrote:
On Fri, 2024-12-13 at 16:10 +0100, Stefano Garzarella wrote:
On Wed, Dec 11, 2024 at 11:47:05AM +0100, Philipp Stanner wrote:
> The PCI functions
>
> pcim_iomap_regions()
> pcim_iounmap_regions()
> pcim_iomap_table()
>
> have been deprecated by the PCI subsystem.
>
> Replace these functions with their successors pcim_iomap_region()
> and
> pcim_iounmap_region().
>
> Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> ---
> Changes in v3:
> - Move __iomem *io declaration into the loop. (Stefano)
>
> Changes in v2:
> - Fix build warning because of dead variable.
> - Make "bars_found" a boolean, since only true or false are
> relevant.
> ---
> drivers/vdpa/solidrun/snet_main.c | 56 ++++++++++++++--------------
> ---
> 1 file changed, 26 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/vdpa/solidrun/snet_main.c
> b/drivers/vdpa/solidrun/snet_main.c
> index c8b74980dbd1..643e335f00f1 100644
> --- a/drivers/vdpa/solidrun/snet_main.c
> +++ b/drivers/vdpa/solidrun/snet_main.c
> @@ -556,59 +556,58 @@ static const struct vdpa_config_ops
> snet_config_ops = {
> static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet
> *psnet)
> {
> char *name;
> - int ret, i, mask = 0;
> + unsigned short i;
> + bool bars_found = false;
> +
> + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-
> bars", pci_name(pdev));
> + if (!name)
> + return -ENOMEM;
> +
> /* 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);
> + void __iomem *io;
> +
> + if (pci_resource_len(pdev, i) == 0)
> + continue;
> +
> + io = pcim_iomap_region(pdev, i, name);
> + if (IS_ERR(io)) {
> + SNET_ERR(pdev, "Failed to request and map
> PCI BARs\n");
> + return PTR_ERR(io);
> + }
> +
> + psnet->bars[i] = io;
> + bars_found = true;
> }
>
> /* No BAR can be used.. */
> - if (!mask) {
> + if (!bars_found) {
> SNET_ERR(pdev, "Failed to find a PCI BAR\n");
> return -ENODEV;
> }
>
> - name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "psnet[%s]-
> bars", pci_name(pdev));
> - if (!name)
> - return -ENOMEM;
> -
> - ret = pcim_iomap_regions(pdev, mask, name);
> - if (ret) {
> - SNET_ERR(pdev, "Failed to request and map PCI
> BARs\n");
> - return ret;
> - }
> -
> - for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> - if (mask & (1 << i))
> - psnet->bars[i] =
> pcim_iomap_table(pdev)[i];
> - }
> -
> return 0;
> }
>
> static int snet_open_vf_bar(struct pci_dev *pdev, struct snet
> *snet)
> {
> char *name;
> - int ret;
>
> name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "snet[%s]-
> bars", pci_name(pdev));
> if (!name)
> return -ENOMEM;
>
> /* 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);
Could we also use a temporary variable here as we did in
psnet_open_pf_bar()?
We can. Some maintainers want it so, others so
It seems to me that no one uses `!snet->bar` for now to check whether
they are configured or not, but maybe better to prevent, WDYT?
So it seems that this driver is a bit special as its probe() function
can take different paths running into one of the respective request
functions.
Doesn't look to me as if pcim_iomap_region() could fail and the driver
would still load?
yeah, it's not super clear to me how physical and virtual functions are
managed.
Anyways, better safe than sorry. If you think it's safer I can modify
it so those pointers are valid or NULL.
Just because this way we leave the behavior the same as before this
change, but I don't have a strong opinion, actually it's the error path
of the probe, so maybe the device is completely deallocated.
Thanks,
Stefano
P.
Thanks,
Stefano
> + 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;
> }
>
> @@ -656,15 +655,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;
> + unsigned short 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 */
> --
> 2.47.1
>
