On Thu, Feb 20, 2025 at 03:20:14PM -0800, Nelson, Shannon wrote:
> 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@xxxxxxx>
> > > > > ---
> > > > > 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.
So just leave in the kernel, the PDS_DEV_TYPE_XXX which you support.
>
> >
> > >
> > > 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.
The understanding of that "core" drivers don't belong to netdev was
always there.
Thanks
>
> sln
>
> >
> > Thanks
>
>
