| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <itatpahlgullmutoxdwybzcpxvg3xklyeqnq7wjztr36xeuon6@4owugtmenlaj>
Date: Mon, 16 Dec 2024 10:52:04 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Philipp Stanner <pstanner@...hat.com>
Cc: Alvaro Karsz <alvaro.karsz@...id-run.com>,
"Michael S. Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez <eperezma@...hat.com>,
virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] vdpa: solidrun: Replace deprecated PCI functions
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@...hat.com>
>> > ---
>> > 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
>> >
>>
>
Powered by blists - more mailing lists