| Message ID | 20230330234628.14627-11-shannon.nelson@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | pds_core driver | expand |
On Thu, Mar 30, 2023 at 04:46:24PM -0700, Shannon Nelson wrote: > An auxiliary_bus device is created for each vDPA type VF at VF probe > and destroyed at VF remove. The VFs are always removed on PF remove, so > there should be no issues with VFs trying to access missing PF structures. > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > --- > drivers/net/ethernet/amd/pds_core/Makefile | 1 + > drivers/net/ethernet/amd/pds_core/auxbus.c | 142 +++++++++++++++++++++ > drivers/net/ethernet/amd/pds_core/core.h | 6 + > drivers/net/ethernet/amd/pds_core/main.c | 36 +++++- > include/linux/pds/pds_auxbus.h | 16 +++ > include/linux/pds/pds_common.h | 1 + > 6 files changed, 200 insertions(+), 2 deletions(-) > create mode 100644 drivers/net/ethernet/amd/pds_core/auxbus.c > create mode 100644 include/linux/pds/pds_auxbus.h I feel that this auxbus usage is still not correct. The idea of auxiliary devices is to partition physical device (for example PCI device) to different sub-devices, where every sub-device belongs to different sub-system. It is not intended to create per-VF devices. In your case, you should create XXX vDPA auxiliary devices which are connected in one-to-one scheme to their PCI VF counterpart. Thanks
On 4/1/23 11:27 AM, Leon Romanovsky wrote: > > On Thu, Mar 30, 2023 at 04:46:24PM -0700, Shannon Nelson wrote: >> An auxiliary_bus device is created for each vDPA type VF at VF probe >> and destroyed at VF remove. The VFs are always removed on PF remove, so >> there should be no issues with VFs trying to access missing PF structures. >> >> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >> --- >> drivers/net/ethernet/amd/pds_core/Makefile | 1 + >> drivers/net/ethernet/amd/pds_core/auxbus.c | 142 +++++++++++++++++++++ >> drivers/net/ethernet/amd/pds_core/core.h | 6 + >> drivers/net/ethernet/amd/pds_core/main.c | 36 +++++- >> include/linux/pds/pds_auxbus.h | 16 +++ >> include/linux/pds/pds_common.h | 1 + >> 6 files changed, 200 insertions(+), 2 deletions(-) >> create mode 100644 drivers/net/ethernet/amd/pds_core/auxbus.c >> create mode 100644 include/linux/pds/pds_auxbus.h > > I feel that this auxbus usage is still not correct. > > The idea of auxiliary devices is to partition physical device (for > example PCI device) to different sub-devices, where every sub-device > belongs to different sub-system. It is not intended to create per-VF > devices. > > In your case, you should create XXX vDPA auxiliary devices which are > connected in one-to-one scheme to their PCI VF counterpart. I don't understand - first I read "It is not intended to create per-VF devices" and then "you should create XXX vDPA auxiliary devices which are connected in one-to-one scheme to their PCI VF counterpart." These seem at first to be directly contradictory statements, so maybe I'm missing some nuance. We have a PF device that has an adminq, VF devices that don't have an adminq, and the adminq is needed for some basic setup before the rest of the vDPA driver can use the VF. To access the PF's adminq we set up an auxiliary device per feature in each VF - but currently only offer one feature (vDPA) and no sub-devices yet. We're trying to plan for the future. Is it that we only have one feature per VF so far is what is causing the discomfort? sln
On Sat, Apr 01, 2023 at 01:15:03PM -0700, Shannon Nelson wrote: > On 4/1/23 11:27 AM, Leon Romanovsky wrote: > > > > On Thu, Mar 30, 2023 at 04:46:24PM -0700, Shannon Nelson wrote: > > > An auxiliary_bus device is created for each vDPA type VF at VF probe > > > and destroyed at VF remove. The VFs are always removed on PF remove, so > > > there should be no issues with VFs trying to access missing PF structures. > > > > > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > > --- > > > drivers/net/ethernet/amd/pds_core/Makefile | 1 + > > > drivers/net/ethernet/amd/pds_core/auxbus.c | 142 +++++++++++++++++++++ > > > drivers/net/ethernet/amd/pds_core/core.h | 6 + > > > drivers/net/ethernet/amd/pds_core/main.c | 36 +++++- > > > include/linux/pds/pds_auxbus.h | 16 +++ > > > include/linux/pds/pds_common.h | 1 + > > > 6 files changed, 200 insertions(+), 2 deletions(-) > > > create mode 100644 drivers/net/ethernet/amd/pds_core/auxbus.c > > > create mode 100644 include/linux/pds/pds_auxbus.h > > > > I feel that this auxbus usage is still not correct. > > > > The idea of auxiliary devices is to partition physical device (for > > example PCI device) to different sub-devices, where every sub-device > > belongs to different sub-system. It is not intended to create per-VF > > devices. > > > > In your case, you should create XXX vDPA auxiliary devices which are > > connected in one-to-one scheme to their PCI VF counterpart. > > I don't understand - first I read > "It is not intended to create per-VF devices" > and then > "you should create XXX vDPA auxiliary devices which are > connected in one-to-one scheme to their PCI VF counterpart." > These seem at first to be directly contradictory statements, so maybe I'm > missing some nuance. It is not, as I'm looking in the code and don't expect to see the code like this. It gives me a sense that auxdevice is not created properly as nothing shouldn't be happen from these checks. + if (pf->state) { + dev_warn(vf->dev, "%s: PF in a transition state (%lu)\n", + __func__, pf->state); + err = -EBUSY; + } else if (!pf->vfs) { + dev_warn(vf->dev, "%s: PF vfs array not ready\n", + __func__); + err = -ENOTTY; + } else if (vf->vf_id >= pf->num_vfs) { + dev_warn(vf->dev, "%s: vfid %d out of range\n", + __func__, vf->vf_id); + err = -ERANGE; + } else if (pf->vfs[vf->vf_id].padev) { + dev_warn(vf->dev, "%s: vfid %d already running\n", + __func__, vf->vf_id); + err = -ENODEV; + } > > We have a PF device that has an adminq, VF devices that don't have an > adminq, and the adminq is needed for some basic setup before the rest of the > vDPA driver can use the VF. To access the PF's adminq we set up an > auxiliary device per feature in each VF - but currently only offer one > feature (vDPA) and no sub-devices yet. We're trying to plan for the future. It looks like premature effort to me. > > Is it that we only have one feature per VF so far is what is causing the > discomfort? This whole patch is not easy for me. Thanks > > sln >
On 4/2/23 11:18 PM, Leon Romanovsky wrote: > > On Sat, Apr 01, 2023 at 01:15:03PM -0700, Shannon Nelson wrote: >> On 4/1/23 11:27 AM, Leon Romanovsky wrote: >>> >>> On Thu, Mar 30, 2023 at 04:46:24PM -0700, Shannon Nelson wrote: >>>> An auxiliary_bus device is created for each vDPA type VF at VF probe >>>> and destroyed at VF remove. The VFs are always removed on PF remove, so >>>> there should be no issues with VFs trying to access missing PF structures. >>>> >>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >>>> --- >>>> drivers/net/ethernet/amd/pds_core/Makefile | 1 + >>>> drivers/net/ethernet/amd/pds_core/auxbus.c | 142 +++++++++++++++++++++ >>>> drivers/net/ethernet/amd/pds_core/core.h | 6 + >>>> drivers/net/ethernet/amd/pds_core/main.c | 36 +++++- >>>> include/linux/pds/pds_auxbus.h | 16 +++ >>>> include/linux/pds/pds_common.h | 1 + >>>> 6 files changed, 200 insertions(+), 2 deletions(-) >>>> create mode 100644 drivers/net/ethernet/amd/pds_core/auxbus.c >>>> create mode 100644 include/linux/pds/pds_auxbus.h >>> >>> I feel that this auxbus usage is still not correct. >>> >>> The idea of auxiliary devices is to partition physical device (for >>> example PCI device) to different sub-devices, where every sub-device >>> belongs to different sub-system. It is not intended to create per-VF >>> devices. >>> >>> In your case, you should create XXX vDPA auxiliary devices which are >>> connected in one-to-one scheme to their PCI VF counterpart. >> >> I don't understand - first I read >> "It is not intended to create per-VF devices" >> and then >> "you should create XXX vDPA auxiliary devices which are >> connected in one-to-one scheme to their PCI VF counterpart." >> These seem at first to be directly contradictory statements, so maybe I'm >> missing some nuance. > > It is not, as I'm looking in the code and don't expect to see the code > like this. It gives me a sense that auxdevice is not created properly > as nothing shouldn't be happen from these checks. > > + if (pf->state) { > + dev_warn(vf->dev, "%s: PF in a transition state (%lu)\n", > + __func__, pf->state); > + err = -EBUSY; > + } else if (!pf->vfs) { > + dev_warn(vf->dev, "%s: PF vfs array not ready\n", > + __func__); > + err = -ENOTTY; > + } else if (vf->vf_id >= pf->num_vfs) { > + dev_warn(vf->dev, "%s: vfid %d out of range\n", > + __func__, vf->vf_id); > + err = -ERANGE; > + } else if (pf->vfs[vf->vf_id].padev) { > + dev_warn(vf->dev, "%s: vfid %d already running\n", > + __func__, vf->vf_id); > + err = -ENODEV; > + } > >> >> We have a PF device that has an adminq, VF devices that don't have an >> adminq, and the adminq is needed for some basic setup before the rest of the >> vDPA driver can use the VF. To access the PF's adminq we set up an >> auxiliary device per feature in each VF - but currently only offer one >> feature (vDPA) and no sub-devices yet. We're trying to plan for the future. > > It looks like premature effort to me. > >> >> Is it that we only have one feature per VF so far is what is causing the >> discomfort? > > This whole patch is not easy for me. Yes, those are extraneous checks left from testing the new driver organization. They are no longer needed, and can come out in the next round. In addition to spreading out the pds_core.rst creation across the patchset and adding more to the commit descriptions, I'll see if there are some other nips and tucks I can do to possibly make the patchset more palatable. sln
On Tue, Apr 04, 2023 at 02:44:34PM -0700, Shannon Nelson wrote: > On 4/2/23 11:18 PM, Leon Romanovsky wrote: > > > > On Sat, Apr 01, 2023 at 01:15:03PM -0700, Shannon Nelson wrote: > > > On 4/1/23 11:27 AM, Leon Romanovsky wrote: > > > > > > > > On Thu, Mar 30, 2023 at 04:46:24PM -0700, Shannon Nelson wrote: > > > > > An auxiliary_bus device is created for each vDPA type VF at VF probe > > > > > and destroyed at VF remove. The VFs are always removed on PF remove, so > > > > > there should be no issues with VFs trying to access missing PF structures. > > > > > > > > > > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > > > > > --- > > > > > drivers/net/ethernet/amd/pds_core/Makefile | 1 + > > > > > drivers/net/ethernet/amd/pds_core/auxbus.c | 142 +++++++++++++++++++++ > > > > > drivers/net/ethernet/amd/pds_core/core.h | 6 + > > > > > drivers/net/ethernet/amd/pds_core/main.c | 36 +++++- > > > > > include/linux/pds/pds_auxbus.h | 16 +++ > > > > > include/linux/pds/pds_common.h | 1 + > > > > > 6 files changed, 200 insertions(+), 2 deletions(-) > > > > > create mode 100644 drivers/net/ethernet/amd/pds_core/auxbus.c > > > > > create mode 100644 include/linux/pds/pds_auxbus.h > > > > > > > > I feel that this auxbus usage is still not correct. > > > > > > > > The idea of auxiliary devices is to partition physical device (for > > > > example PCI device) to different sub-devices, where every sub-device > > > > belongs to different sub-system. It is not intended to create per-VF > > > > devices. > > > > > > > > In your case, you should create XXX vDPA auxiliary devices which are > > > > connected in one-to-one scheme to their PCI VF counterpart. > > > > > > I don't understand - first I read > > > "It is not intended to create per-VF devices" > > > and then > > > "you should create XXX vDPA auxiliary devices which are > > > connected in one-to-one scheme to their PCI VF counterpart." > > > These seem at first to be directly contradictory statements, so maybe I'm > > > missing some nuance. > > > > It is not, as I'm looking in the code and don't expect to see the code > > like this. It gives me a sense that auxdevice is not created properly > > as nothing shouldn't be happen from these checks. > > > > + if (pf->state) { > > + dev_warn(vf->dev, "%s: PF in a transition state (%lu)\n", > > + __func__, pf->state); > > + err = -EBUSY; > > + } else if (!pf->vfs) { > > + dev_warn(vf->dev, "%s: PF vfs array not ready\n", > > + __func__); > > + err = -ENOTTY; > > + } else if (vf->vf_id >= pf->num_vfs) { > > + dev_warn(vf->dev, "%s: vfid %d out of range\n", > > + __func__, vf->vf_id); > > + err = -ERANGE; > > + } else if (pf->vfs[vf->vf_id].padev) { > > + dev_warn(vf->dev, "%s: vfid %d already running\n", > > + __func__, vf->vf_id); > > + err = -ENODEV; > > + } > > > > > > > > We have a PF device that has an adminq, VF devices that don't have an > > > adminq, and the adminq is needed for some basic setup before the rest of the > > > vDPA driver can use the VF. To access the PF's adminq we set up an > > > auxiliary device per feature in each VF - but currently only offer one > > > feature (vDPA) and no sub-devices yet. We're trying to plan for the future. > > > > It looks like premature effort to me. > > > > > > > > Is it that we only have one feature per VF so far is what is causing the > > > discomfort? > > > > This whole patch is not easy for me. > > Yes, those are extraneous checks left from testing the new driver > organization. They are no longer needed, and can come out in the next > round. > > In addition to spreading out the pds_core.rst creation across the patchset > and adding more to the commit descriptions, I'll see if there are some other > nips and tucks I can do to possibly make the patchset more palatable. You also need to rework your auxdevice id creation scheme to be more like other drivers. This line assumes that you have only one PF device in the system. + aux_dev->id = vf->id; Thanks > > sln
On 4/4/23 11:07 PM, Leon Romanovsky wrote: > > On Tue, Apr 04, 2023 at 02:44:34PM -0700, Shannon Nelson wrote: >> On 4/2/23 11:18 PM, Leon Romanovsky wrote: >>> >>> On Sat, Apr 01, 2023 at 01:15:03PM -0700, Shannon Nelson wrote: >>>> On 4/1/23 11:27 AM, Leon Romanovsky wrote: >>>>> >>>>> On Thu, Mar 30, 2023 at 04:46:24PM -0700, Shannon Nelson wrote: >>>>>> An auxiliary_bus device is created for each vDPA type VF at VF probe >>>>>> and destroyed at VF remove. The VFs are always removed on PF remove, so >>>>>> there should be no issues with VFs trying to access missing PF structures. >>>>>> >>>>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >>>>>> --- >>>>>> drivers/net/ethernet/amd/pds_core/Makefile | 1 + >>>>>> drivers/net/ethernet/amd/pds_core/auxbus.c | 142 +++++++++++++++++++++ >>>>>> drivers/net/ethernet/amd/pds_core/core.h | 6 + >>>>>> drivers/net/ethernet/amd/pds_core/main.c | 36 +++++- >>>>>> include/linux/pds/pds_auxbus.h | 16 +++ >>>>>> include/linux/pds/pds_common.h | 1 + >>>>>> 6 files changed, 200 insertions(+), 2 deletions(-) >>>>>> create mode 100644 drivers/net/ethernet/amd/pds_core/auxbus.c >>>>>> create mode 100644 include/linux/pds/pds_auxbus.h >>>>> >>>>> I feel that this auxbus usage is still not correct. >>>>> >>>>> The idea of auxiliary devices is to partition physical device (for >>>>> example PCI device) to different sub-devices, where every sub-device >>>>> belongs to different sub-system. It is not intended to create per-VF >>>>> devices. >>>>> >>>>> In your case, you should create XXX vDPA auxiliary devices which are >>>>> connected in one-to-one scheme to their PCI VF counterpart. >>>> >>>> I don't understand - first I read >>>> "It is not intended to create per-VF devices" >>>> and then >>>> "you should create XXX vDPA auxiliary devices which are >>>> connected in one-to-one scheme to their PCI VF counterpart." >>>> These seem at first to be directly contradictory statements, so maybe I'm >>>> missing some nuance. >>> >>> It is not, as I'm looking in the code and don't expect to see the code >>> like this. It gives me a sense that auxdevice is not created properly >>> as nothing shouldn't be happen from these checks. >>> >>> + if (pf->state) { >>> + dev_warn(vf->dev, "%s: PF in a transition state (%lu)\n", >>> + __func__, pf->state); >>> + err = -EBUSY; >>> + } else if (!pf->vfs) { >>> + dev_warn(vf->dev, "%s: PF vfs array not ready\n", >>> + __func__); >>> + err = -ENOTTY; >>> + } else if (vf->vf_id >= pf->num_vfs) { >>> + dev_warn(vf->dev, "%s: vfid %d out of range\n", >>> + __func__, vf->vf_id); >>> + err = -ERANGE; >>> + } else if (pf->vfs[vf->vf_id].padev) { >>> + dev_warn(vf->dev, "%s: vfid %d already running\n", >>> + __func__, vf->vf_id); >>> + err = -ENODEV; >>> + } >>> >>>> >>>> We have a PF device that has an adminq, VF devices that don't have an >>>> adminq, and the adminq is needed for some basic setup before the rest of the >>>> vDPA driver can use the VF. To access the PF's adminq we set up an >>>> auxiliary device per feature in each VF - but currently only offer one >>>> feature (vDPA) and no sub-devices yet. We're trying to plan for the future. >>> >>> It looks like premature effort to me. >>> >>>> >>>> Is it that we only have one feature per VF so far is what is causing the >>>> discomfort? >>> >>> This whole patch is not easy for me. >> >> Yes, those are extraneous checks left from testing the new driver >> organization. They are no longer needed, and can come out in the next >> round. >> >> In addition to spreading out the pds_core.rst creation across the patchset >> and adding more to the commit descriptions, I'll see if there are some other >> nips and tucks I can do to possibly make the patchset more palatable. > > You also need to rework your auxdevice id creation scheme to be more > like other drivers. > > This line assumes that you have only one PF device in the system. > + aux_dev->id = vf->id; > Yes, that really should be an ida_alloc(), thanks. sln
On Sat, Apr 01, 2023 at 01:15:03PM -0700, Shannon Nelson wrote: > We have a PF device that has an adminq, VF devices that don't have an > adminq, and the adminq is needed for some basic setup before the rest of the > vDPA driver can use the VF. To access the PF's adminq we set up an > auxiliary device per feature in each VF - but currently only offer one > feature (vDPA) and no sub-devices yet. We're trying to plan for the future. It is the same remark I gave for the VFIO integration, the bound VF driver should use pci_iov_get_pf_drvdata() to reach to its PF driver and operate the admin queue. Make sure to read the comment of the function to lock it properly. There should not be a parallel aux device for the VF device, the purpose of the aux device is not to allow you to find things like admin queue objects, it is to represent a slice of device functionality that doesn't already have a struct device representation. Drivers should not attempt to 'dual bind' aux device and pci device as the original vfio patches tried to do. Jason
diff --git a/drivers/net/ethernet/amd/pds_core/Makefile b/drivers/net/ethernet/amd/pds_core/Makefile index 6d1d6c58a1fa..0abc33ce826c 100644 --- a/drivers/net/ethernet/amd/pds_core/Makefile +++ b/drivers/net/ethernet/amd/pds_core/Makefile @@ -5,6 +5,7 @@ obj-$(CONFIG_PDS_CORE) := pds_core.o pds_core-y := main.o \ devlink.o \ + auxbus.o \ dev.o \ adminq.o \ core.o \ diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c new file mode 100644 index 000000000000..7c6a84009558 --- /dev/null +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c @@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2023 Advanced Micro Devices, Inc */ + +#include <linux/pci.h> + +#include "core.h" +#include <linux/pds/pds_auxbus.h> + +static void pdsc_auxbus_dev_release(struct device *dev) +{ + struct pds_auxiliary_dev *padev = + container_of(dev, struct pds_auxiliary_dev, aux_dev.dev); + + kfree(padev); +} + +static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *vf, + struct pdsc *pf, + char *name) +{ + struct auxiliary_device *aux_dev; + struct pds_auxiliary_dev *padev; + int err; + + padev = kzalloc(sizeof(*padev), GFP_KERNEL); + if (!padev) + return ERR_PTR(-ENOMEM); + + padev->vf_pdev = vf->pdev; + padev->pf_pdev = pf->pdev; + + aux_dev = &padev->aux_dev; + aux_dev->name = name; + aux_dev->id = vf->id; + aux_dev->dev.parent = vf->dev; + aux_dev->dev.release = pdsc_auxbus_dev_release; + + err = auxiliary_device_init(aux_dev); + if (err < 0) { + dev_warn(vf->dev, "auxiliary_device_init of %s id %d failed: %pe\n", + name, vf->id, ERR_PTR(err)); + goto err_out; + } + + err = auxiliary_device_add(aux_dev); + if (err) { + dev_warn(vf->dev, "auxiliary_device_add of %s id %d failed: %pe\n", + name, vf->id, ERR_PTR(err)); + auxiliary_device_uninit(aux_dev); + goto err_out; + } + + return padev; + +err_out: + kfree(padev); + return ERR_PTR(err); +} + +int pdsc_auxbus_dev_del_vf(struct pdsc *vf, struct pdsc *pf) +{ + struct pds_auxiliary_dev *padev; + int err = 0; + + mutex_lock(&pf->config_lock); + if (pf->state) { + dev_warn(vf->dev, "%s: PF in a transition state (%lu)\n", + __func__, pf->state); + err = -EBUSY; + } else if (vf->vf_id >= pf->num_vfs) { + dev_warn(vf->dev, "%s: vfid %d out of range\n", + __func__, vf->vf_id); + err = -ERANGE; + } + if (err) + goto out_unlock; + + padev = pf->vfs[vf->vf_id].padev; + if (padev) { + auxiliary_device_delete(&padev->aux_dev); + auxiliary_device_uninit(&padev->aux_dev); + } + pf->vfs[vf->vf_id].padev = NULL; + +out_unlock: + mutex_unlock(&pf->config_lock); + return err; +} + +int pdsc_auxbus_dev_add_vf(struct pdsc *vf, struct pdsc *pf) +{ + struct pds_auxiliary_dev *padev; + enum pds_core_vif_types vt; + int err = 0; + + mutex_lock(&pf->config_lock); + if (pf->state) { + dev_warn(vf->dev, "%s: PF in a transition state (%lu)\n", + __func__, pf->state); + err = -EBUSY; + } else if (!pf->vfs) { + dev_warn(vf->dev, "%s: PF vfs array not ready\n", + __func__); + err = -ENOTTY; + } else if (vf->vf_id >= pf->num_vfs) { + dev_warn(vf->dev, "%s: vfid %d out of range\n", + __func__, vf->vf_id); + err = -ERANGE; + } else if (pf->vfs[vf->vf_id].padev) { + dev_warn(vf->dev, "%s: vfid %d already running\n", + __func__, vf->vf_id); + err = -ENODEV; + } + if (err) + goto out_unlock; + + for (vt = 0; vt < PDS_DEV_TYPE_MAX; vt++) { + u16 vt_support; + + /* Verify that the type is supported and enabled */ + vt_support = !!le16_to_cpu(pf->dev_ident.vif_types[vt]); + if (!(vt_support && + pf->viftype_status[vt].supported && + pf->viftype_status[vt].enabled)) + continue; + + padev = pdsc_auxbus_dev_register(vf, pf, + pf->viftype_status[vt].name); + if (IS_ERR(padev)) { + err = PTR_ERR(padev); + goto out_unlock; + } + pf->vfs[vf->vf_id].padev = padev; + + /* We only support a single type per VF, so jump out here */ + break; + } + +out_unlock: + mutex_unlock(&pf->config_lock); + return err; +} diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h index bfe0274942d1..27aaffdc5056 100644 --- a/drivers/net/ethernet/amd/pds_core/core.h +++ b/drivers/net/ethernet/amd/pds_core/core.h @@ -30,8 +30,11 @@ struct pdsc_dev_bar { int res_index; }; +struct pdsc; + struct pdsc_vf { struct pds_auxiliary_dev *padev; + struct pdsc *vf; u16 index; __le16 vif_types[PDS_DEV_TYPE_MAX]; }; @@ -301,6 +304,9 @@ int pdsc_start(struct pdsc *pdsc); void pdsc_stop(struct pdsc *pdsc); void pdsc_health_thread(struct work_struct *work); +int pdsc_auxbus_dev_add_vf(struct pdsc *vf, struct pdsc *pf); +int pdsc_auxbus_dev_del_vf(struct pdsc *vf, struct pdsc *pf); + void pdsc_process_adminq(struct pdsc_qcq *qcq); void pdsc_work_thread(struct work_struct *work); irqreturn_t pdsc_adminq_isr(int irq, void *data); diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c index 313057a57deb..900158135938 100644 --- a/drivers/net/ethernet/amd/pds_core/main.c +++ b/drivers/net/ethernet/amd/pds_core/main.c @@ -201,6 +201,12 @@ static int pdsc_sriov_configure(struct pci_dev *pdev, int num_vfs) static int pdsc_init_vf(struct pdsc *vf) { struct devlink *dl; + struct pdsc *pf; + int err; + + pf = pdsc_get_pf_struct(vf->pdev); + if (IS_ERR_OR_NULL(pf)) + return PTR_ERR(pf) ?: -1; vf->vf_id = pci_iov_vf_id(vf->pdev); @@ -209,7 +215,15 @@ static int pdsc_init_vf(struct pdsc *vf) devl_register(dl); devl_unlock(dl); - return 0; + pf->vfs[vf->vf_id].vf = vf; + err = pdsc_auxbus_dev_add_vf(vf, pf); + if (err) { + devl_lock(dl); + devl_unregister(dl); + devl_unlock(dl); + } + + return err; } static const struct devlink_health_reporter_ops pdsc_fw_reporter_ops = { @@ -400,7 +414,19 @@ static void pdsc_remove(struct pci_dev *pdev) } devl_unlock(dl); - if (!pdev->is_virtfn) { + if (pdev->is_virtfn) { + struct pdsc *pf; + + pf = pdsc_get_pf_struct(pdsc->pdev); + if (!IS_ERR(pf)) { + pdsc_auxbus_dev_del_vf(pdsc, pf); + pf->vfs[pdsc->vf_id].vf = NULL; + } + } else { + /* Remove the VFs and their aux_bus connections before other + * cleanup so that the clients can use the AdminQ to cleanly + * shut themselves down. + */ pdsc_sriov_configure(pdev, 0); del_timer_sync(&pdsc->wdtimer); @@ -440,6 +466,12 @@ static struct pci_driver pdsc_driver = { .sriov_configure = pdsc_sriov_configure, }; +void *pdsc_get_pf_struct(struct pci_dev *vf_pdev) +{ + return pci_iov_get_pf_drvdata(vf_pdev, &pdsc_driver); +} +EXPORT_SYMBOL_GPL(pdsc_get_pf_struct); + static int __init pdsc_init_module(void) { pdsc_debugfs_create(); diff --git a/include/linux/pds/pds_auxbus.h b/include/linux/pds/pds_auxbus.h new file mode 100644 index 000000000000..aa0192af4a29 --- /dev/null +++ b/include/linux/pds/pds_auxbus.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright(c) 2023 Advanced Micro Devices, Inc */ + +#ifndef _PDSC_AUXBUS_H_ +#define _PDSC_AUXBUS_H_ + +#include <linux/auxiliary_bus.h> + +struct pds_auxiliary_dev { + struct auxiliary_device aux_dev; + struct pci_dev *vf_pdev; + struct pci_dev *pf_pdev; + u16 client_id; + void *priv; +}; +#endif /* _PDSC_AUXBUS_H_ */ diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h index 350295091d9d..898f3c7b14b7 100644 --- a/include/linux/pds/pds_common.h +++ b/include/linux/pds/pds_common.h @@ -91,4 +91,5 @@ enum pds_core_logical_qtype { PDS_CORE_QTYPE_MAX = 16 /* don't change - used in struct size */ }; +void *pdsc_get_pf_struct(struct pci_dev *vf_pdev); #endif /* _PDS_COMMON_H_ */
An auxiliary_bus device is created for each vDPA type VF at VF probe and destroyed at VF remove. The VFs are always removed on PF remove, so there should be no issues with VFs trying to access missing PF structures. Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> --- drivers/net/ethernet/amd/pds_core/Makefile | 1 + drivers/net/ethernet/amd/pds_core/auxbus.c | 142 +++++++++++++++++++++ drivers/net/ethernet/amd/pds_core/core.h | 6 + drivers/net/ethernet/amd/pds_core/main.c | 36 +++++- include/linux/pds/pds_auxbus.h | 16 +++ include/linux/pds/pds_common.h | 1 + 6 files changed, 200 insertions(+), 2 deletions(-) create mode 100644 drivers/net/ethernet/amd/pds_core/auxbus.c create mode 100644 include/linux/pds/pds_auxbus.h