| Message ID | 20250211234854.52277-3-shannon.nelson@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | pds_fwctl: fwctl for AMD/Pensando core devices | expand |
On Tue, 11 Feb 2025 15:48:51 -0800 Shannon Nelson <shannon.nelson@amd.com> 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@amd.com> Hi Shannon, > diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c > index a3a68889137b..7f20c3f5f349 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_teardown; > + If you fail after this point, do you not want to remove this device? Also superficially shouldn't this be goto err_out_stop? I think the call is just after pdsc_start(pdsc) though maybe I'm looking at an old tree. > dl = priv_to_devlink(pdsc); > devl_lock(dl); > err = devl_params_register(dl, pdsc_dl_params,
On Tue, 11 Feb 2025 15:48:51 -0800 Shannon Nelson <shannon.nelson@amd.com> wrote: > Add support for a new fwctl-based auxiliary_device for creating a > channel for fwctl support into the AMD/Pensando DSC. auxiliary_device in title.
On 2/12/2025 4:02 AM, Jonathan Cameron wrote: > On Tue, 11 Feb 2025 15:48:51 -0800 > Shannon Nelson <shannon.nelson@amd.com> 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@amd.com> > > Hi Shannon, > >> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c >> index a3a68889137b..7f20c3f5f349 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_teardown; >> + > > If you fail after this point, do you not want to remove this device? > Also superficially shouldn't this be goto err_out_stop? I think > the call is just after pdsc_start(pdsc) though maybe I'm looking at > an old tree. Thanks, I'll fix this. sln > > >> dl = priv_to_devlink(pdsc); >> devl_lock(dl); >> err = devl_params_register(dl, pdsc_dl_params, >
On 2/12/2025 4:03 AM, Jonathan Cameron wrote: > On Tue, 11 Feb 2025 15:48:51 -0800 > Shannon Nelson <shannon.nelson@amd.com> wrote: > >> Add support for a new fwctl-based auxiliary_device for creating a >> channel for fwctl support into the AMD/Pensando DSC. > > auxiliary_device in title. > Good catch - I almost had them all fixed... sln
On Tue, Feb 11, 2025 at 03:48:51PM -0800, Shannon Nelson 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@amd.com> > --- > 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 | 10 ++++++++++ > include/linux/pds/pds_common.h | 2 ++ > 5 files changed, 21 insertions(+), 2 deletions(-) <...> My comment is only slightly related to the patch itself, but worth to write it anyway. > 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, This enum and defines below should be cleaned from unsupported types. I don't see any code for RDMA, LM and ETH. Thanks > > /* 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 > -- > 2.17.1 > >
On 2/18/2025 11:28 AM, Leon Romanovsky wrote: > > On Tue, Feb 11, 2025 at 03:48:51PM -0800, Shannon Nelson 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@amd.com> >> --- >> 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 | 10 ++++++++++ >> include/linux/pds/pds_common.h | 2 ++ >> 5 files changed, 21 insertions(+), 2 deletions(-) > > <...> > > My comment is only slightly related to the patch itself, but worth to > write it anyway. > >> 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, > > This enum and defines below should be cleaned from unsupported types. > I don't see any code for RDMA, LM and ETH. > > Thanks I've looked at those a few times over the life of this code, but I continue to leave them there because they are part of the firmware interface definition, whether we use them or not. You're right, there is no ETH or RDMA type code, they exist as historical artifacts of the early interface design. The LM type underlies the device used by the pds-vfio-pci driver and is a value that pds_core will see in the device identity information gathered from the firmware if there are VFIO VFs configured in the FW. I'd rather not mess with this enum for this patchset, but I'll keep this in mind for future cleanup work. Thanks, sln > >> >> /* 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 >> -- >> 2.17.1 >> >>
On Tue, Feb 18, 2025 at 12:00:52PM -0800, Nelson, Shannon wrote: > On 2/18/2025 11:28 AM, Leon Romanovsky wrote: > > > > On Tue, Feb 11, 2025 at 03:48:51PM -0800, Shannon Nelson 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@amd.com> > > > --- > > > 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 | 10 ++++++++++ > > > include/linux/pds/pds_common.h | 2 ++ > > > 5 files changed, 21 insertions(+), 2 deletions(-) > > > > <...> > > > > My comment is only slightly related to the patch itself, but worth to > > write it anyway. > > > > > 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, > > > > This enum and defines below should be cleaned from unsupported types. > > I don't see any code for RDMA, LM and ETH. > > > > Thanks > > I've looked at those a few times over the life of this code, but I continue > to leave them there because they are part of the firmware interface > definition, whether we use them or not. How? You are passing some number which FW is not aware of it. You can pass any number you want there. Even it is not true, you can PDS_DEV_TYPE_FWCTL = 6 here, but remove rest of enums and *_STR defines. > > You're right, there is no ETH or RDMA type code, they exist as historical > artifacts of the early interface design. This make me wonder why netdev merged this code which has nothing to do with netdev subsystem at all. Thanks
On 2/19/2025 12:24 AM, Leon Romanovsky wrote: > > On Tue, Feb 18, 2025 at 12:00:52PM -0800, Nelson, Shannon wrote: >> On 2/18/2025 11:28 AM, Leon Romanovsky wrote: >>> >>> On Tue, Feb 11, 2025 at 03:48:51PM -0800, Shannon Nelson 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@amd.com> >>>> --- >>>> 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 | 10 ++++++++++ >>>> include/linux/pds/pds_common.h | 2 ++ >>>> 5 files changed, 21 insertions(+), 2 deletions(-) >>> >>> <...> >>> >>> My comment is only slightly related to the patch itself, but worth to >>> write it anyway. >>> >>>> 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, >>> >>> This enum and defines below should be cleaned from unsupported types. >>> I don't see any code for RDMA, LM and ETH. >>> >>> Thanks >> >> I've looked at those a few times over the life of this code, but I continue >> to leave them there because they are part of the firmware interface >> definition, whether we use them or not. > > How? You are passing some number which FW is not aware of it. You can > pass any number you want there. Even it is not true, you can > PDS_DEV_TYPE_FWCTL = 6 here, but remove rest of enums and *_STR defines. When pds_core starts up it gets ident/config information from the firmware in a struct pds_core_dev_identity, which includes the vif_types[] array which tells us how many of each PDS_DEV_TYPE_xx are supported in the FW. This is indexed by enum pds_core_vif_types. > >> >> You're right, there is no ETH or RDMA type code, they exist as historical >> artifacts of the early interface design. > > This make me wonder why netdev merged this code which has nothing to do > with netdev subsystem at all. The pds_core was originally brought in for supporting pds_vdpa and pds_vfio_pci. At the time, it was essentially following the example of the mlx core module; at the time there wasn't any push back or suggestions of a different place to land. Maybe further fwctl and "core" related discussions will suggest another approach. sln > > Thanks
diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c index 0a3035adda52..857697ae1e4b 100644 --- a/drivers/net/ethernet/amd/pds_core/auxbus.c +++ b/drivers/net/ethernet/amd/pds_core/auxbus.c @@ -229,8 +229,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. */ 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 065031dd5af6..218bb9c4c780 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..7f20c3f5f349 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_teardown; + dl = priv_to_devlink(pdsc); devl_lock(dl); err = devl_params_register(dl, pdsc_dl_params, @@ -427,6 +431,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 +490,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 +538,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
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@amd.com> --- 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 | 10 ++++++++++ include/linux/pds/pds_common.h | 2 ++ 5 files changed, 21 insertions(+), 2 deletions(-)