Add new device attributes to view multiport, msix, and adapter MTU
setting for MANA device.

Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
---
 .../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;
+}
+
+static int mana_gd_setup_sysfs(struct pci_dev *pdev)
+{
+	struct gdma_context *gc = pci_get_drvdata(pdev);
+	int retval = 0;
+
+	gc->mana_attributes.mana_mport_attr.attr.name = "mport";
+	gc->mana_attributes.mana_mport_attr.attr.mode = 0444;
+	gc->mana_attributes.mana_mport_attr.show = mana_attr_show;
+	sysfs_attr_init(&gc->mana_attributes.mana_mport_attr);
+	retval = device_create_file(&pdev->dev,
+				    &gc->mana_attributes.mana_mport_attr);
+	if (retval < 0)
+		return retval;
+
+	gc->mana_attributes.mana_adapter_mtu_attr.attr.name = "adapter_mtu";
+	gc->mana_attributes.mana_adapter_mtu_attr.attr.mode = 0444;
+	gc->mana_attributes.mana_adapter_mtu_attr.show = mana_attr_show;
+	sysfs_attr_init(&gc->mana_attributes.mana_adapter_mtu_attr);
+	retval = device_create_file(&pdev->dev,
+				    &gc->mana_attributes.mana_adapter_mtu_attr);
+	if (retval < 0)
+		goto mtu_attr_error;
+
+	gc->mana_attributes.mana_msix_attr.attr.name = "msix";
+	gc->mana_attributes.mana_msix_attr.attr.mode = 0444;
+	gc->mana_attributes.mana_msix_attr.show = mana_attr_show;
+	sysfs_attr_init(&gc->mana_attributes.mana_msix_attr);
+	retval = device_create_file(&pdev->dev,
+				    &gc->mana_attributes.mana_msix_attr);
+	if (retval < 0)
+		goto msix_attr_error;
+
+	return retval;
+msix_attr_error:
+	device_remove_file(&pdev->dev,
+			   &gc->mana_attributes.mana_adapter_mtu_attr);
+mtu_attr_error:
+	device_remove_file(&pdev->dev,
+			   &gc->mana_attributes.mana_mport_attr);
+	return retval;
+}
+
 static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct gdma_context *gc;
@@ -1519,6 +1578,10 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	gc->bar0_va = bar0_va;
 	gc->dev = &pdev->dev;
 
+	err = mana_gd_setup_sysfs(pdev);
+	if (err < 0)
+		goto free_gc;
+
 	err = mana_gd_setup(pdev);
 	if (err)
 		goto unmap_bar;
@@ -1544,6 +1607,15 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	return err;
 }
 
+static void mana_cleanup_sysfs_files(struct pci_dev *pdev,
+				     struct gdma_context *gc)
+{
+	device_remove_file(&pdev->dev, &gc->mana_attributes.mana_msix_attr);
+	device_remove_file(&pdev->dev,
+			   &gc->mana_attributes.mana_adapter_mtu_attr);
+	device_remove_file(&pdev->dev, &gc->mana_attributes.mana_mport_attr);
+}
+
 static void mana_gd_remove(struct pci_dev *pdev)
 {
 	struct gdma_context *gc = pci_get_drvdata(pdev);
@@ -1552,6 +1624,8 @@ static void mana_gd_remove(struct pci_dev *pdev)
 
 	mana_gd_cleanup(pdev);
 
+	mana_cleanup_sysfs_files(pdev, gc);
+
 	pci_iounmap(pdev, gc->bar0_va);
 
 	vfree(gc);
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 27684135bb4d..ea636959164c 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -354,6 +354,12 @@ struct gdma_irq_context {
 	char name[MANA_IRQ_NAME_SZ];
 };
 
+struct mana_device_attributes {
+	struct device_attribute mana_mport_attr;
+	struct device_attribute mana_adapter_mtu_attr;
+	struct device_attribute mana_msix_attr;
+};
+
 struct gdma_context {
 	struct device		*dev;
 
@@ -395,6 +401,9 @@ struct gdma_context {
 
 	/* Azure RDMA adapter */
 	struct gdma_dev		mana_ib;
+
+	/* device attributes */
+	struct mana_device_attributes mana_attributes;
 };
 
 #define MAX_NUM_GDMA_DEVICES	4
-- 
2.34.1

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@linux.microsoft.com>
> ---
>  .../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;
> +}
> +
> +static int mana_gd_setup_sysfs(struct pci_dev *pdev)
> +{
> +	struct gdma_context *gc = pci_get_drvdata(pdev);
> +	int retval = 0;
> +
> +	gc->mana_attributes.mana_mport_attr.attr.name = "mport";
> +	gc->mana_attributes.mana_mport_attr.attr.mode = 0444;
> +	gc->mana_attributes.mana_mport_attr.show = mana_attr_show;
> +	sysfs_attr_init(&gc->mana_attributes.mana_mport_attr);
> +	retval = device_create_file(&pdev->dev,
> +				    &gc->mana_attributes.mana_mport_attr);

if you can use .dev_groups, sysfs creation and removal will be lot more
simplified for the driver.

- Saurabh

On Mon, Apr 15, 2024 at 09:38:32AM -0700, Saurabh Singh Sengar wrote:
> 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@linux.microsoft.com>
> > ---
> >  .../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;
> > +}
> > +
> > +static int mana_gd_setup_sysfs(struct pci_dev *pdev)
> > +{
> > +	struct gdma_context *gc = pci_get_drvdata(pdev);
> > +	int retval = 0;
> > +
> > +	gc->mana_attributes.mana_mport_attr.attr.name = "mport";
> > +	gc->mana_attributes.mana_mport_attr.attr.mode = 0444;
> > +	gc->mana_attributes.mana_mport_attr.show = mana_attr_show;
> > +	sysfs_attr_init(&gc->mana_attributes.mana_mport_attr);
> > +	retval = device_create_file(&pdev->dev,
> > +				    &gc->mana_attributes.mana_mport_attr);
> 
> if you can use .dev_groups, sysfs creation and removal will be lot more
> simplified for the driver.
Sure Saurabh, I think we can do this too.
> 
> - Saurabh

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@linux.microsoft.com>
> ---
>  .../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

Jason

在 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@linux.microsoft.com>
>> ---
>>   .../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.

Zhu Yanjun

> 
> Jason

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@linux.microsoft.com>
> >>---
> >>  .../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.
> 
> Zhu Yanjun
Thanks for the reference, Zhu.
> 
> >
> >Jason

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@linux.microsoft.com>
> > > ---
> > >   .../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

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@linux.microsoft.com>
> > > > ---
> > > >   .../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

On Wed, Apr 17, 2024 at 11:01:08PM -0700, Shradha Gupta wrote:

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

From an RDMA perspective this is all available from other APIs already
at least and I wouldn't want to see new sysfs unless there is a netdev
justification.

Jason

> >From an RDMA perspective this is all available from other APIs already
> at least and I wouldn't want to see new sysfs unless there is a netdev
> justification.

It is unlikely there is a netdev justification. Configuration happens
via netlink, not sysfs.

    Andrew

On Thu, Apr 18, 2024 at 08:42:59PM +0200, Andrew Lunn wrote:
> > >From an RDMA perspective this is all available from other APIs already
> > at least and I wouldn't want to see new sysfs unless there is a netdev
> > justification.
> 
> It is unlikely there is a netdev justification. Configuration happens
> via netlink, not sysfs.
> 
>     Andrew

Thanks. Sure, it makes sense to make the generic attribute configurable
through the netdevice ops or netlink implementation. I will keep that in
mind while adding the next set of configuration attributes for the driver.
These attributes(from the patch) however, are hardware specific(that show
the maximum supported values by the hardware in most cases). We want them
to be a part of sysfs so that they are readily available in the production
for improving debuggability. I will change the names of these attribute to
indicate the same to avoid possible confusion.

Regards,
Shradha.

On Fri, Apr 19, 2024 at 09:59:26AM -0700, Shradha Gupta wrote:
> On Thu, Apr 18, 2024 at 08:42:59PM +0200, Andrew Lunn wrote:
> > > >From an RDMA perspective this is all available from other APIs already
> > > at least and I wouldn't want to see new sysfs unless there is a netdev
> > > justification.
> > 
> > It is unlikely there is a netdev justification. Configuration happens
> > via netlink, not sysfs.
> > 
> >     Andrew
> 
> Thanks. Sure, it makes sense to make the generic attribute configurable
> through the netdevice ops or netlink implementation. I will keep that in
> mind while adding the next set of configuration attributes for the driver.
> These attributes(from the patch) however, are hardware specific(that show
> the maximum supported values by the hardware in most cases).

        ndev->max_mtu = gc->adapter_mtu - ETH_HLEN;
        ndev->min_mtu = ETH_MIN_MTU;

This does not appear to be specific to your device. This is very
generic. We already have /sys/class/net/eth42/mtu, why not add
/sys/class/net/eth42/max_mtu and /sys/class/net/eth42/min_mtu for
every driver?

Are these values really hardware specific? Are they really unique to
your hardware? I have to wounder because you clearly did not think
much about MTU, and how it is actually generic...

     Andrew

On Fri, Apr 19, 2024 at 08:51:02PM +0200, Andrew Lunn wrote:
> On Fri, Apr 19, 2024 at 09:59:26AM -0700, Shradha Gupta wrote:
> > On Thu, Apr 18, 2024 at 08:42:59PM +0200, Andrew Lunn wrote:
> > > > >From an RDMA perspective this is all available from other APIs already
> > > > at least and I wouldn't want to see new sysfs unless there is a netdev
> > > > justification.
> > > 
> > > It is unlikely there is a netdev justification. Configuration happens
> > > via netlink, not sysfs.
> > > 
> > >     Andrew
> > 
> > Thanks. Sure, it makes sense to make the generic attribute configurable
> > through the netdevice ops or netlink implementation. I will keep that in
> > mind while adding the next set of configuration attributes for the driver.
> > These attributes(from the patch) however, are hardware specific(that show
> > the maximum supported values by the hardware in most cases).
> 
>         ndev->max_mtu = gc->adapter_mtu - ETH_HLEN;
>         ndev->min_mtu = ETH_MIN_MTU;
> 
> This does not appear to be specific to your device. This is very
> generic. We already have /sys/class/net/eth42/mtu, why not add
> /sys/class/net/eth42/max_mtu and /sys/class/net/eth42/min_mtu for
> every driver?
> 
> Are these values really hardware specific? Are they really unique to
> your hardware? I have to wounder because you clearly did not think
> much about MTU, and how it is actually generic...
> 
>      Andrew
That makes sense. I will make these as generic attributes in the next version.
Thanks.

On Mon, Apr 15, 2024 at 01:13:05PM -0300, Jason Gunthorpe wrote:
> 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@linux.microsoft.com>
> > ---
> >  .../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
> 
> Jason
Thanks Jason, I will make the appropriate changes in the next version.

Regards,
Shradha.