Re: [PATCH net-next] net: mana: Add new device attributes for mana

[Shradha Gupta]

On Tue, Apr 16, 2024 at 08:09:35PM +0200, Andrew Lunn wrote:
> On Tue, Apr 16, 2024 at 06:27:04AM +0200, Zhu Yanjun wrote:
> > ??? 2024/4/15 18:13, Jason Gunthorpe ??????:
> > > On Mon, Apr 15, 2024 at 02:49:49AM -0700, Shradha Gupta wrote:
> > > > Add new device attributes to view multiport, msix, and adapter MTU
> > > > setting for MANA device.
> > > > 
> > > > Signed-off-by: Shradha Gupta <shradhagupta@xxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >   .../net/ethernet/microsoft/mana/gdma_main.c   | 74 +++++++++++++++++++
> > > >   include/net/mana/gdma.h                       |  9 +++
> > > >   2 files changed, 83 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > > > index 1332db9a08eb..6674a02cff06 100644
> > > > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > > > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > > > @@ -1471,6 +1471,65 @@ static bool mana_is_pf(unsigned short dev_id)
> > > >   	return dev_id == MANA_PF_DEVICE_ID;
> > > >   }
> > > > +static ssize_t mana_attr_show(struct device *dev,
> > > > +			      struct device_attribute *attr, char *buf)
> > > > +{
> > > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > > > +	struct gdma_context *gc = pci_get_drvdata(pdev);
> > > > +	struct mana_context *ac = gc->mana.driver_data;
> > > > +
> > > > +	if (strcmp(attr->attr.name, "mport") == 0)
> > > > +		return snprintf(buf, PAGE_SIZE, "%d\n", ac->num_ports);
> > > > +	else if (strcmp(attr->attr.name, "adapter_mtu") == 0)
> > > > +		return snprintf(buf, PAGE_SIZE, "%d\n", gc->adapter_mtu);
> > > > +	else if (strcmp(attr->attr.name, "msix") == 0)
> > > > +		return snprintf(buf, PAGE_SIZE, "%d\n", gc->max_num_msix);
> > > > +	else
> > > > +		return -EINVAL;
> > > > +
> > > 
> > > That is not how sysfs should be implemented at all, please find a
> > > good example to copy from. Every attribute should use its own function
> > > with the macros to link it into an attributes group and sysfs_emit
> > > should be used for printing
> > 
> > Not sure if this file drivers/infiniband/hw/usnic/usnic_ib_sysfs.c is a good
> > example or not.
> 
> The first question should be, what are these values used for? You can
> then decide on debugfs or sysfs. debugfs is easier to use, and you
> avoid any ABI, which will make long term support easier.

Hi Andrew,
We want to eventually use these attributes to make the device settings configurable
and also improve debuggability for MANA devices. I feel having these attributes 
in sysfs would make more sense as we plan to extend the attribute list and also make
them settable.

Regards,
Shradha.
> 
>       Andrew