| Message ID | 1623161227-29930-3-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/4] rtnetlink: add alloc() method to rtnl_link_ops | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | warning | Series does not have a cover letter |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 5 maintainers not CCed: laniel_francis@privacyrequired.com zhudi21@huawei.com cong.wang@bytedance.com avagin@gmail.com roopa@cumulusnetworks.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 11 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
> From: Loic Poulain <loic.poulain@linaro.org> > Sent: Tuesday, June 8, 2021 7:37 PM > > From: Sergey Ryazanov <ryazanov.s.a@gmail.com> > > Return a parent device using the FLA_PARENT_DEV_NAME attribute during > links dump. This should help a user figure out which links belong to a > particular HW device. E.g. what data channels exists on a specific WWAN > modem. > Please add the output sample in the commit message, for this additional field possibly for a more common netdevice of a pci device or some other one. > Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> > --- > net/core/rtnetlink.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 56ac16a..120887c > 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -1819,6 +1819,11 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, > if (rtnl_fill_prop_list(skb, dev)) > goto nla_put_failure; > > + if (dev->dev.parent && > + nla_put_string(skb, IFLA_PARENT_DEV_NAME, > + dev_name(dev->dev.parent))) > + goto nla_put_failure; > + A device name along with device bus establishes a unique identity in the system. Hence you should add IFLA_PARENT_DEV_BUS_NAME and return it optionally if the device is on a bus. If (dev->dev.parent->bus) return parent->bus->name string. > nlmsg_end(skb, nlh); > return 0; > > -- > 2.7.4
Hello Parav, On Tue, Jun 8, 2021 at 7:54 PM Parav Pandit <parav@nvidia.com> wrote: >> From: Loic Poulain <loic.poulain@linaro.org> >> Sent: Tuesday, June 8, 2021 7:37 PM >> >> From: Sergey Ryazanov <ryazanov.s.a@gmail.com> >> >> Return a parent device using the FLA_PARENT_DEV_NAME attribute during >> links dump. This should help a user figure out which links belong to a >> particular HW device. E.g. what data channels exists on a specific WWAN >> modem. >> > Please add the output sample in the commit message, for this additional field possibly for a more common netdevice of a pci device or some other one. > >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> >> --- >> net/core/rtnetlink.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 56ac16a..120887c >> 100644 >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -1819,6 +1819,11 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, >> if (rtnl_fill_prop_list(skb, dev)) >> goto nla_put_failure; >> >> + if (dev->dev.parent && >> + nla_put_string(skb, IFLA_PARENT_DEV_NAME, >> + dev_name(dev->dev.parent))) >> + goto nla_put_failure; >> + > A device name along with device bus establishes a unique identity in the system. Sure. To uniquely identify an abstract device we need a full path, including a device parent. In sysfs it will be a device bus. But IFLA_PARENT_DEV_NAME was introduced to identify the parent device within a scope of a specific subsystem, which is specified by the IFLA_INFO_KIND attribute. IFLA_PARENT_DEV_NAME should become a sane alternative for the IFLA_LINK usage when the link parent is not a netdev themself. You can find more details in the description of IFLA_PARENT_DEV_NAME introduction patch [1], my explanation, why we need to make the attribute common [2] and see a usage example in the wwan interface creation patch [3]. 1. https://lore.kernel.org/netdev/1623161227-29930-2-git-send-email-loic.poulain@linaro.org/ 2. https://lore.kernel.org/netdev/CAHNKnsTKfFF9EckwSnLrwaPdH_tkjvdB3PVraMD-OLqFdLmp_Q@mail.gmail.com/ 3. https://lore.kernel.org/netdev/1623161227-29930-4-git-send-email-loic.poulain@linaro.org/ > Hence you should add IFLA_PARENT_DEV_BUS_NAME and return it optionally if the device is on a bus. > If (dev->dev.parent->bus) > return parent->bus->name string. Looks like we are able to export the device bus name. Do you have a use case for this attribute? And even so, should we bloat this simple patch with auxiliary attributes? I would even prefer that this patch was merged with the attribute introduction patch into a single patch. This way the purpose of the attribute will become more clear.
Hi Sergey, > From: Sergey Ryazanov <ryazanov.s.a@gmail.com> > Sent: Wednesday, June 9, 2021 5:00 AM > > Hello Parav, > > On Tue, Jun 8, 2021 at 7:54 PM Parav Pandit <parav@nvidia.com> wrote: > >> From: Loic Poulain <loic.poulain@linaro.org> > >> Sent: Tuesday, June 8, 2021 7:37 PM > >> > >> From: Sergey Ryazanov <ryazanov.s.a@gmail.com> > >> > >> Return a parent device using the FLA_PARENT_DEV_NAME attribute > during > >> links dump. This should help a user figure out which links belong to > >> a particular HW device. E.g. what data channels exists on a specific > >> WWAN modem. > >> > > Please add the output sample in the commit message, for this additional > field possibly for a more common netdevice of a pci device or some other > one. > > > >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com> > >> --- > >> net/core/rtnetlink.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index > >> 56ac16a..120887c > >> 100644 > >> --- a/net/core/rtnetlink.c > >> +++ b/net/core/rtnetlink.c > >> @@ -1819,6 +1819,11 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, > >> if (rtnl_fill_prop_list(skb, dev)) > >> goto nla_put_failure; > >> > >> + if (dev->dev.parent && > >> + nla_put_string(skb, IFLA_PARENT_DEV_NAME, > >> + dev_name(dev->dev.parent))) > >> + goto nla_put_failure; > >> + > > A device name along with device bus establishes a unique identity in the > system. > > Sure. To uniquely identify an abstract device we need a full path, including a > device parent. In sysfs it will be a device bus. But IFLA_PARENT_DEV_NAME > was introduced to identify the parent device within a scope of a specific > subsystem, which is specified by the IFLA_INFO_KIND attribute. IFLA_INFO_KIND is not set for many types of netdevices which are not created by ip link add command. Such as pci devices, auxiliary bus pci sf devices and possibly others. IFLA_PARENT_DEV_NAME is returned for all netdevices whichever has it valid. For example, for a netdevice with PCI parent, it will return as 0000:03:00.0. This number string is useless without telling that it is pci device name. So if you prefer to add PARENT_DEV_NAME, it needs to accompany along with its BUS_NAME too. IFLA_INF_KIND for pci and other device is null. So only PARENT_DEV is not sufficient. > IFLA_PARENT_DEV_NAME should become a sane alternative for the > IFLA_LINK usage when the link parent is not a netdev themself. > Ok. but not enough. It needs to accompany with the bus name too. > You can find more details in the description of IFLA_PARENT_DEV_NAME > introduction patch [1], my explanation, why we need to make the attribute > common [2] and see a usage example in the wwan interface creation patch > [3]. > I am not against making it common. I just say that if you prefer to expose this duplicate (and useful) info, please accompany with device bus name too. Btw: parent device info is available via ethool such as ethtool -i enp1s0f0 | grep bus-info bus-info: 0000:01:00.0 This is left to the individual driver to fill up. Compare to that generic way like this in this patch is desired with bus name. > 1. https://lore.kernel.org/netdev/1623161227-29930-2-git-send-email- > loic.poulain@linaro.org/ > 2. > https://lore.kernel.org/netdev/CAHNKnsTKfFF9EckwSnLrwaPdH_tkjvdB3PVra > MD-OLqFdLmp_Q@mail.gmail.com/ > 3. https://lore.kernel.org/netdev/1623161227-29930-4-git-send-email- > loic.poulain@linaro.org/ > > > Hence you should add IFLA_PARENT_DEV_BUS_NAME and return it > optionally if the device is on a bus. > > If (dev->dev.parent->bus) > > return parent->bus->name string. > > Looks like we are able to export the device bus name. Do you have a use case > for this attribute? The one I explained above. > And even so, should we bloat this simple patch with > auxiliary attributes? > Not sure which one. I don’t see any more to add other that bus name and parent device name. A similar patch [4] to include parent device for rdma networking devices didn't make through because its readily available in sysfs. [4] https://lore.kernel.org/linux-rdma/BY5PR12MB432246F3155BC1D440857629DCEB0@BY5PR12MB4322.namprd12.prod.outlook.com/
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 56ac16a..120887c 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1819,6 +1819,11 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, if (rtnl_fill_prop_list(skb, dev)) goto nla_put_failure; + if (dev->dev.parent && + nla_put_string(skb, IFLA_PARENT_DEV_NAME, + dev_name(dev->dev.parent))) + goto nla_put_failure; + nlmsg_end(skb, nlh); return 0;