Re: [PATCH v3 3/6] pds_core: add new fwctl auxiliary_device

["Nelson, Shannon" <shannon.nelson@amd.com>]

On 3/10/2025 10:33 AM, Dave Jiang wrote:
> 
> On 3/7/25 11:53 AM, Shannon Nelson wrote:
>> Add support for a new fwctl-based auxiliary_device for creating a
>> channel for fwctl support into the AMD/Pensando DSC.
>>
>> Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> 
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> 
> minor comment below
> 
>> ---
>>   drivers/net/ethernet/amd/pds_core/auxbus.c |  4 ++--
>>   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   | 14 +++++++++++++-
>>   include/linux/pds/pds_common.h             |  2 ++
>>   5 files changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
>> index 563de9e7ce0a..c9aeb56e8174 100644
>> --- a/drivers/net/ethernet/amd/pds_core/auxbus.c
>> +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
>> @@ -224,8 +224,8 @@ 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 the firmware doesn't support the feature, we
>> +      * just won't set up an auxiliary_device for it.
> 
> s/, we just/; the driver/

Got it, thanks,
sln

> 
> DJ
> 
> 
> 
>>         */
>>        vt_support = !!le16_to_cpu(pf->dev_ident.vif_types[vt]);
>>        if (!(vt_support &&
>> diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
>> index 536635e57727..1eb0d92786f7 100644
>> --- a/drivers/net/ethernet/amd/pds_core/core.c
>> +++ b/drivers/net/ethernet/amd/pds_core/core.c
>> @@ -402,6 +402,9 @@ static int pdsc_core_init(struct pdsc *pdsc)
>>   }
>>
>>   static struct pdsc_viftype pdsc_viftype_defaults[] = {
>> +     [PDS_DEV_TYPE_FWCTL] = { .name = PDS_DEV_TYPE_FWCTL_STR,
>> +                              .vif_id = PDS_DEV_TYPE_FWCTL,
>> +                              .dl_id = -1 },
>>        [PDS_DEV_TYPE_VDPA] = { .name = PDS_DEV_TYPE_VDPA_STR,
>>                                .vif_id = PDS_DEV_TYPE_VDPA,
>>                                .dl_id = DEVLINK_PARAM_GENERIC_ID_ENABLE_VNET },
>> @@ -428,6 +431,10 @@ static int pdsc_viftypes_init(struct pdsc *pdsc)
>>
>>                /* See what the Core device has for support */
>>                vt_support = !!le16_to_cpu(pdsc->dev_ident.vif_types[vt]);
>> +
>> +             if (vt == PDS_DEV_TYPE_FWCTL)
>> +                     pdsc->viftype_status[vt].enabled = true;
>> +
>>                dev_dbg(pdsc->dev, "VIF %s is %ssupported\n",
>>                        pdsc->viftype_status[vt].name,
>>                        vt_support ? "" : "not ");
>> diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
>> index f075e68c64db..0bf320c43083 100644
>> --- a/drivers/net/ethernet/amd/pds_core/core.h
>> +++ b/drivers/net/ethernet/amd/pds_core/core.h
>> @@ -156,6 +156,7 @@ struct pdsc {
>>        struct dentry *dentry;
>>        struct device *dev;
>>        struct pdsc_dev_bar bars[PDS_CORE_BARS_MAX];
>> +     struct pds_auxiliary_dev *padev;
>>        struct pdsc_vf *vfs;
>>        int num_vfs;
>>        int vf_id;
>> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
>> index a3a68889137b..4843f9249a31 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,
>> @@ -273,7 +277,7 @@ static int pdsc_init_pf(struct pdsc *pdsc)
>>                devl_unlock(dl);
>>                dev_warn(pdsc->dev, "Failed to register devlink params: %pe\n",
>>                         ERR_PTR(err));
>> -             goto err_out_stop;
>> +             goto err_out_del_dev;
>>        }
>>
>>        hr = devl_health_reporter_create(dl, &pdsc_fw_reporter_ops, 0, pdsc);
>> @@ -296,6 +300,8 @@ static int pdsc_init_pf(struct pdsc *pdsc)
>>   err_out_unreg_params:
>>        devlink_params_unregister(dl, pdsc_dl_params,
>>                                  ARRAY_SIZE(pdsc_dl_params));
>> +err_out_del_dev:
>> +     pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
>>   err_out_stop:
>>        pdsc_stop(pdsc);
>>   err_out_teardown:
>> @@ -427,6 +433,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 +492,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 +540,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);
>>        }
>>   }
>>
>> diff --git a/include/linux/pds/pds_common.h b/include/linux/pds/pds_common.h
>> index 5802e1deef24..b193adbe7cc3 100644
>> --- a/include/linux/pds/pds_common.h
>> +++ b/include/linux/pds/pds_common.h
>> @@ -29,6 +29,7 @@ enum pds_core_vif_types {
>>        PDS_DEV_TYPE_ETH        = 3,
>>        PDS_DEV_TYPE_RDMA       = 4,
>>        PDS_DEV_TYPE_LM         = 5,
>> +     PDS_DEV_TYPE_FWCTL      = 6,
>>
>>        /* new ones added before this line */
>>        PDS_DEV_TYPE_MAX        = 16   /* don't change - used in struct size */
>> @@ -40,6 +41,7 @@ enum pds_core_vif_types {
>>   #define PDS_DEV_TYPE_ETH_STR "Eth"
>>   #define PDS_DEV_TYPE_RDMA_STR        "RDMA"
>>   #define PDS_DEV_TYPE_LM_STR  "LM"
>> +#define PDS_DEV_TYPE_FWCTL_STR       "fwctl"
>>
>>   #define PDS_VDPA_DEV_NAME    PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_VDPA_STR
>>   #define PDS_VFIO_LM_DEV_NAME PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_LM_STR "." PDS_DEV_TYPE_VFIO_STR
>