Add new device attributes to view multiport, msix, and adapter MTU
setting for MANA device.
Signed-off-by: Shradha Gupta <[email protected]>
---
.../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 <[email protected]>
> ---
> .../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
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 <[email protected]>
> ---
> .../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 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 <[email protected]>
> > ---
> > .../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.
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 <[email protected]>
> > ---
> > .../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
在 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 <[email protected]>
>> ---
>> .../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 <[email protected]>
> > > ---
> > > .../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 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 <[email protected]>
> >>---
> >> .../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 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 <[email protected]>
> > > > ---
> > > > .../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
> -----Original Message-----
> From: Shradha Gupta <[email protected]>
> Sent: Monday, April 15, 2024 5:50 AM
> To: [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Cc: Shradha Gupta <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> <[email protected]>; Ajay Sharma <[email protected]>; Leon
> Romanovsky <[email protected]>; Thomas Gleixner <[email protected]>;
> Sebastian Andrzej Siewior <[email protected]>; KY Srinivasan
> <[email protected]>; Haiyang Zhang <[email protected]>; Wei Liu
> <[email protected]>; Dexuan Cui <[email protected]>; Long Li
> <[email protected]>; Michael Kelley <[email protected]>; Shradha
> Gupta <[email protected]>; Yury Norov <[email protected]>;
> Konstantin Taranov <[email protected]>; Souradeep Chakrabarti
> <[email protected]>
> Subject: [PATCH net-next] net: mana: Add new device attributes for mana
>
> Add new device attributes to view multiport, msix, and adapter MTU
> setting for MANA device.
>
> Signed-off-by: Shradha Gupta <[email protected]>
> ---
> .../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;
> +
Regarding examples, vmbus_drv.c has a number of sysfs variables:
static ssize_t in_read_bytes_avail_show(struct device *dev,
struct device_attribute *dev_attr,
char *buf)
{
struct hv_device *hv_dev = device_to_hv_device(dev);
struct hv_ring_buffer_debug_info inbound;
int ret;
if (!hv_dev->channel)
return -ENODEV;
ret = hv_ringbuffer_get_debuginfo(&hv_dev->channel->inbound, &inbound);
if (ret < 0)
return ret;
return sprintf(buf, "%d\n", inbound.bytes_avail_toread);
}
static DEVICE_ATTR_RO(in_read_bytes_avail);
Thanks,
- Haiyang
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.