On Fri, 28 Feb 2025 17:35:51 -0800
Shannon Nelson <shannon.nelson@xxxxxxx> wrote:
> Add support for a new fwctl-based auxiliary_device for creating a
> channel for fwctl support into the AMD/Pensando DSC.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@xxxxxxx>
Hi Shannon,
A few really minor comments inline from a fresh read through.
Thanks,
Jonathan
> ---
> drivers/net/ethernet/amd/pds_core/auxbus.c | 3 +--
> drivers/net/ethernet/amd/pds_core/core.c | 7 +++++++
> drivers/net/ethernet/amd/pds_core/core.h | 1 +
> drivers/net/ethernet/amd/pds_core/main.c | 11 +++++++++++
> include/linux/pds/pds_common.h | 2 ++
> 5 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
> index db950a9c9d30..ac6f76c161f2 100644
> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
> @@ -225,8 +225,7 @@ int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf,
> }
>
> /* 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.
> + * an error if there is no auxbus device support.
Comment feels a bit general. Is this no auxbus support for this device
or none at all in the kernel?
> */
> vt_support = !!le16_to_cpu(pf->dev_ident.vif_types[vt]);
> if (!(vt_support &&
> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
> index a3a68889137b..41575c7a148d 100644
> --- a/drivers/net/ethernet/amd/pds_core/main.c
> +++ b/drivers/net/ethernet/amd/pds_core/main.c
> @@ -265,6 +265,10 @@ static int pdsc_init_pf(struct pdsc *pdsc)
>
> mutex_unlock(&pdsc->config_lock);
>
> + err = pdsc_auxbus_dev_add(pdsc, pdsc, PDS_DEV_TYPE_FWCTL, &pdsc->padev);
> + if (err)
> + goto err_out_stop;
> +
> dl = priv_to_devlink(pdsc);
> devl_lock(dl);
> err = devl_params_register(dl, pdsc_dl_params,
> @@ -297,6 +301,7 @@ static int pdsc_init_pf(struct pdsc *pdsc)
> devlink_params_unregister(dl, pdsc_dl_params,
> ARRAY_SIZE(pdsc_dl_params));
> err_out_stop:
> + pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
This doesn't smell right (by which I mean I had to go look at the
implementation to be sure it wasn't a bug) In my ideal
world that should be obvious on a more local basis.
I'd expect a new label here. pdsc_auxbus_dev_add() should be and is
side effect free if it fails. That is it should not make sense
to call pdsc_auxbus_dev_del() if it fails.
It isn't a bug today as that becomes a noop due to
&pdsc->padev being NULL but that is a detail I shouldn't
ideally need to know when reading this code.
I'd put err_out_stop label here and rename previous
one to err_out_auxbus_del + replace the existing
goto err_out_stop;
with
goto err_out_auxbus_del;
> pdsc_stop(pdsc);
> err_out_teardown:
> pdsc_teardown(pdsc, PDSC_TEARDOWN_REMOVING);
> @@ -427,6 +432,7 @@ static void pdsc_remove(struct pci_dev *pdev)
> * shut themselves down.
> */
> pdsc_sriov_configure(pdev, 0);
> + pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
>
> timer_shutdown_sync(&pdsc->wdtimer);
> if (pdsc->wq)
> @@ -485,6 +491,8 @@ static void pdsc_reset_prepare(struct pci_dev *pdev)
> if (!IS_ERR(pf))
> pdsc_auxbus_dev_del(pdsc, pf,
> &pf->vfs[pdsc->vf_id].padev);
> + } else {
> + pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
> }
>
> pdsc_unmap_bars(pdsc);
> @@ -531,6 +539,9 @@ static void pdsc_reset_done(struct pci_dev *pdev)
> if (!IS_ERR(pf))
> pdsc_auxbus_dev_add(pdsc, pf, PDS_DEV_TYPE_VDPA,
> &pf->vfs[pdsc->vf_id].padev);
> + } else {
> + pdsc_auxbus_dev_add(pdsc, pdsc, PDS_DEV_TYPE_FWCTL,
> + &pdsc->padev);
> }
> }
