On 3/7/25 11:53 AM, Shannon Nelson wrote: > In preparation for adding a new auxiliary_device for the > PF, make the vif type an argument to pdsc_auxbus_dev_add(). > We also now pass in the address to where we'll keep the new > padev pointer so that the caller can specify where to save it > but we can still change it under the mutex and keep the mutex > usage within the function. Please consider changing the commit log to use imperative language and avoid using pronouns such as "we". https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#describe-your-changes https://kernelnewbies.org/PatchPhilosophy#:~:text=Please%20read%20that%20whole%20section,it%20into%20the%20git%20history. Maybe something like: Pass in the address of the padev pointer so the caller can specify where to save it. ... > > Reviewed-by: Leon Romanovsky <leonro@xxxxxxxxxx> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > Signed-off-by: Shannon Nelson <shannon.nelson@xxxxxxx> Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx> > --- > drivers/net/ethernet/amd/pds_core/auxbus.c | 37 ++++++++++----------- > drivers/net/ethernet/amd/pds_core/core.h | 7 ++-- > drivers/net/ethernet/amd/pds_core/devlink.c | 5 +-- > drivers/net/ethernet/amd/pds_core/main.c | 11 +++--- > 4 files changed, 33 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c > index 78fba368e797..563de9e7ce0a 100644 > --- a/drivers/net/ethernet/amd/pds_core/auxbus.c > +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c > @@ -175,29 +175,32 @@ static struct pds_auxiliary_dev *pdsc_auxbus_dev_register(struct pdsc *cf, > return padev; > } > > -void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf) > +void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf, > + struct pds_auxiliary_dev **pd_ptr) > { > struct pds_auxiliary_dev *padev; > > + if (!*pd_ptr) > + return; > + > mutex_lock(&pf->config_lock); > > - padev = pf->vfs[cf->vf_id].padev; > - if (padev) { > - pds_client_unregister(pf, padev->client_id); > - auxiliary_device_delete(&padev->aux_dev); > - auxiliary_device_uninit(&padev->aux_dev); > - padev->client_id = 0; > - } > - pf->vfs[cf->vf_id].padev = NULL; > + padev = *pd_ptr; > + pds_client_unregister(pf, padev->client_id); > + auxiliary_device_delete(&padev->aux_dev); > + auxiliary_device_uninit(&padev->aux_dev); > + padev->client_id = 0; > + *pd_ptr = NULL; > > mutex_unlock(&pf->config_lock); > } > > -int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf) > +int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf, > + enum pds_core_vif_types vt, > + struct pds_auxiliary_dev **pd_ptr) > { > struct pds_auxiliary_dev *padev; > char devname[PDS_DEVNAME_LEN]; > - enum pds_core_vif_types vt; > unsigned long mask; > u16 vt_support; > int client_id; > @@ -206,6 +209,9 @@ int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf) > if (!cf) > return -ENODEV; > > + if (vt >= PDS_DEV_TYPE_MAX) > + return -EINVAL; > + > mutex_lock(&pf->config_lock); > > mask = BIT_ULL(PDSC_S_FW_DEAD) | > @@ -217,17 +223,10 @@ int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf) > goto out_unlock; > } > > - /* We only support vDPA so far, so it is the only one to > - * be verified that it is available in the Core device and > - * enabled in the devlink param. In the future this might > - * become a loop for several VIF types. > - */ > - > /* Verify that the type is supported and enabled. It is not > * an error if there is no auxbus device support for this > * VF, it just means something else needs to happen with it. > */ > - vt = PDS_DEV_TYPE_VDPA; > vt_support = !!le16_to_cpu(pf->dev_ident.vif_types[vt]); > if (!(vt_support && > pf->viftype_status[vt].supported && > @@ -253,7 +252,7 @@ int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf) > err = PTR_ERR(padev); > goto out_unlock; > } > - pf->vfs[cf->vf_id].padev = padev; > + *pd_ptr = padev; > > out_unlock: > mutex_unlock(&pf->config_lock); > diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h > index 631a59cfdd7e..f075e68c64db 100644 > --- a/drivers/net/ethernet/amd/pds_core/core.h > +++ b/drivers/net/ethernet/amd/pds_core/core.h > @@ -303,8 +303,11 @@ void pdsc_health_thread(struct work_struct *work); > int pdsc_register_notify(struct notifier_block *nb); > void pdsc_unregister_notify(struct notifier_block *nb); > void pdsc_notify(unsigned long event, void *data); > -int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf); > -void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf); > +int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf, > + enum pds_core_vif_types vt, > + struct pds_auxiliary_dev **pd_ptr); > +void pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf, > + struct pds_auxiliary_dev **pd_ptr); > > void pdsc_process_adminq(struct pdsc_qcq *qcq); > void pdsc_work_thread(struct work_struct *work); > diff --git a/drivers/net/ethernet/amd/pds_core/devlink.c b/drivers/net/ethernet/amd/pds_core/devlink.c > index 4e2b92ddef6f..c5c787df61a4 100644 > --- a/drivers/net/ethernet/amd/pds_core/devlink.c > +++ b/drivers/net/ethernet/amd/pds_core/devlink.c > @@ -57,9 +57,10 @@ int pdsc_dl_enable_set(struct devlink *dl, u32 id, > struct pdsc *vf = pdsc->vfs[vf_id].vf; > > if (ctx->val.vbool) > - err = pdsc_auxbus_dev_add(vf, pdsc); > + err = pdsc_auxbus_dev_add(vf, pdsc, vt_entry->vif_id, > + &pdsc->vfs[vf_id].padev); > else > - pdsc_auxbus_dev_del(vf, pdsc); > + pdsc_auxbus_dev_del(vf, pdsc, &pdsc->vfs[vf_id].padev); > } > > return err; > diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c > index 660268ff9562..a3a68889137b 100644 > --- a/drivers/net/ethernet/amd/pds_core/main.c > +++ b/drivers/net/ethernet/amd/pds_core/main.c > @@ -190,7 +190,8 @@ static int pdsc_init_vf(struct pdsc *vf) > devl_unlock(dl); > > pf->vfs[vf->vf_id].vf = vf; > - err = pdsc_auxbus_dev_add(vf, pf); > + err = pdsc_auxbus_dev_add(vf, pf, PDS_DEV_TYPE_VDPA, > + &pf->vfs[vf->vf_id].padev); > if (err) { > devl_lock(dl); > devl_unregister(dl); > @@ -417,7 +418,7 @@ static void pdsc_remove(struct pci_dev *pdev) > > pf = pdsc_get_pf_struct(pdsc->pdev); > if (!IS_ERR(pf)) { > - pdsc_auxbus_dev_del(pdsc, pf); > + pdsc_auxbus_dev_del(pdsc, pf, &pf->vfs[pdsc->vf_id].padev); > pf->vfs[pdsc->vf_id].vf = NULL; > } > } else { > @@ -482,7 +483,8 @@ static void pdsc_reset_prepare(struct pci_dev *pdev) > > pf = pdsc_get_pf_struct(pdsc->pdev); > if (!IS_ERR(pf)) > - pdsc_auxbus_dev_del(pdsc, pf); > + pdsc_auxbus_dev_del(pdsc, pf, > + &pf->vfs[pdsc->vf_id].padev); > } > > pdsc_unmap_bars(pdsc); > @@ -527,7 +529,8 @@ static void pdsc_reset_done(struct pci_dev *pdev) > > pf = pdsc_get_pf_struct(pdsc->pdev); > if (!IS_ERR(pf)) > - pdsc_auxbus_dev_add(pdsc, pf); > + pdsc_auxbus_dev_add(pdsc, pf, PDS_DEV_TYPE_VDPA, > + &pf->vfs[pdsc->vf_id].padev); > } > } >
