| Message ID | 1713174589-29243-1-git-send-email-shradhagupta@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: mana: Add new device attributes for mana | expand |
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
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 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.
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
在 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. 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 <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 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
> -----Original Message----- > From: Shradha Gupta <shradhagupta@linux.microsoft.com> > Sent: Monday, April 15, 2024 5:50 AM > To: linux-kernel@vger.kernel.org; linux-hyperv@vger.kernel.org; linux- > rdma@vger.kernel.org; netdev@vger.kernel.org > Cc: Shradha Gupta <shradhagupta@linux.microsoft.com>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; Ajay Sharma <sharmaajay@microsoft.com>; Leon > Romanovsky <leon@kernel.org>; Thomas Gleixner <tglx@linutronix.de>; > Sebastian Andrzej Siewior <bigeasy@linutronix.de>; KY Srinivasan > <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Wei Liu > <wei.liu@kernel.org>; Dexuan Cui <decui@microsoft.com>; Long Li > <longli@microsoft.com>; Michael Kelley <mikelley@microsoft.com>; Shradha > Gupta <shradhagupta@microsoft.com>; Yury Norov <yury.norov@gmail.com>; > Konstantin Taranov <kotaranov@microsoft.com>; Souradeep Chakrabarti > <schakrabarti@linux.microsoft.com> > 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 <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; > + 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.
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
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(+)