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

[Andrew Lunn]

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.

      Andrew