Re: [RFC PATCH fwctl 2/5] pds_core: add new fwctl auxilary_device

[Leon Romanovsky <leon@kernel.org>]

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@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.

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
> 
>