Thu, Aug 30, 2018 at 12:13:40PM CEST, mar...@holtmann.org wrote:
>Hi Jiri,
>
>>>>> The name value from SET_NETDEV_DEVTYPE only ended up in the uevent sysfs
>>>>> file as DEVTYPE= information. To avoid any kind of race conditions
>>>>> between netlink messages and reading from sysfs, it is useful to add the
>>>>> same string as new IFLA_DEVTYPE attribute included in the RTM_NEWLINK
>>>>> messages.
>>>>>
>>>>> For network managing daemons that have to classify ARPHRD_ETHER network
>>>>> devices into different types (like Wireless LAN, Bluetooth etc.), this
>>>>> avoids the extra round trip to sysfs and parsing of the uevent file.
>>>>>
>>>>> Signed-off-by: Marcel Holtmann <mar...@holtmann.org>
>>>>> ---
>>>>> include/uapi/linux/if_link.h | 2 ++
>>>>> net/core/rtnetlink.c | 12 ++++++++++++
>>>>> 2 files changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>>>>> index 43391e2d1153..781294972bb4 100644
>>>>> --- a/include/uapi/linux/if_link.h
>>>>> +++ b/include/uapi/linux/if_link.h
>>>>> @@ -166,6 +166,8 @@ enum {
>>>>> IFLA_NEW_IFINDEX,
>>>>> IFLA_MIN_MTU,
>>>>> IFLA_MAX_MTU,
>>>>> + IFLA_DEVTYPE, /* Name value from SET_NETDEV_DEVTYPE */
>>>>
>>>> This is not something netdev-related. dev->dev.type is struct device_type.
>>>> This is a generic "device" thing. Incorrect to expose over
>>>> netdev-specific API. Please use "device" API for this.
>>>
>>> it is not just "device" related since this is a sub-classification of
>>> ARPHRD_ETHER type as a wrote above. Don't get hang up that this information
>>> is part of struct device. The dev->dev.type contains strings like "wlan",
>>> "bluetooth", "wimax", "gadget" etc. so that you can tell what kind of
>>> Ethernet card you have.
>>
>> I understand. But using dev->dev.type for that purpose seems like an
>> abuse. If this is subtype of netdev, it should not be stored in parent
>> device. I believe that you need to re-introduce this as a part of struct
>> net_device - preferably an enum - and that you can expose over rtnl (and
>> net-sysfs).
>
>it is not stored in the parent device. It is stored in the device of the
>netdev.
Yeah. That is what I ment. "Device struct" associated with the
netdevice.
>
>As Stephen said, there is no device API. And why would I add the same info
>again and bloat netdev?
>
>drivers/net/bonding/bond_main.c: SET_NETDEV_DEVTYPE(bond_dev,
>&bond_type);
>drivers/net/geneve.c: SET_NETDEV_DEVTYPE(dev, &geneve_type);
>drivers/net/macsec.c: SET_NETDEV_DEVTYPE(dev, &macsec_type);
>drivers/net/ppp/ppp_generic.c: SET_NETDEV_DEVTYPE(dev, &ppp_type);
>drivers/net/usb/hso.c: SET_NETDEV_DEVTYPE(net, &hso_type);
>drivers/net/usb/usbnet.c: SET_NETDEV_DEVTYPE(net, &wlan_type);
>drivers/net/usb/usbnet.c: SET_NETDEV_DEVTYPE(net, &wwan_type);
>drivers/net/vxlan.c: SET_NETDEV_DEVTYPE(dev, &vxlan_type);
>drivers/net/wimax/i2400m/usb.c: SET_NETDEV_DEVTYPE(net_dev, &i2400mu_type);
>drivers/net/wireless/intersil/prism54/islpci_dev.c:
>SET_NETDEV_DEVTYPE(ndev, &wlan_type);
>drivers/staging/gdm724x/gdm_lte.c: SET_NETDEV_DEVTYPE(net,
>&wwan_type);
>drivers/usb/gadget/function/u_ether.c: SET_NETDEV_DEVTYPE(net, &gadget_type);
>drivers/usb/gadget/function/u_ether.c: SET_NETDEV_DEVTYPE(net, &gadget_type);
>net/8021q/vlan_dev.c: SET_NETDEV_DEVTYPE(dev, &vlan_type);
>net/bluetooth/6lowpan.c: SET_NETDEV_DEVTYPE(netdev, &bt_type);
>net/bluetooth/bnep/core.c: SET_NETDEV_DEVTYPE(dev, &bnep_type);
>net/bridge/br_device.c: SET_NETDEV_DEVTYPE(dev, &br_type);
>net/dsa/slave.c: SET_NETDEV_DEVTYPE(slave_dev, &dsa_type);
>net/hsr/hsr_device.c: SET_NETDEV_DEVTYPE(dev, &hsr_type);
>net/l2tp/l2tp_eth.c: SET_NETDEV_DEVTYPE(dev, &l2tpeth_type);
>net/wireless/core.c: SET_NETDEV_DEVTYPE(dev, &wiphy_type);
>
>So for all these devices we have to add duplicate information now?
The fact the things are done now it a wrong way does not justify
extending UAPI in the same wrong way. So yes, change them all if you
need it. The "ethernet-subtype" should not an attribute of "struct
device" but rather "struct net_device".
>
>I actually don't like that idea. I can rename it to IFLA_FOO or any other
>proposal, but adding the same information twice is pointless.
So don't expose the "struct device" attribute over rtnetlink. Use
"struct device"-specific interface for that.
>
>>> We can revive the patches for adding this information as IFLA_INFO_KIND,
>>> but last time they where reverted since NetworkManager did all the wrong
>>> decisions based on that. That part is fixed now and we can put that back
>>> and declare the sub-type classification of Ethernet device in two places.
>>> Or we just take the information that we added a long time ago for exactly
>>> this sub-classification and provide them to userspace via RTNL.
>>
>> IFLA_INFO_KIND serves for different purpose. It is for drivers that
>> register rtnl_link_ops. I don't think it would be wise to mix it with
>> this.
>
>We could register rtnl_link_ops for these Ethernet drivers, but then we are
>duplicating the information as well.
That is not the purpose of "rtnl_link_ops".
>
>Regards
>
>Marcel
>
