Message ID | 20220610165940.2326777-1-jiaqing.zhao@linux.intel.com |
---|---|
Headers | show |
Series | Configurable VLAN mode for NCSI driver | expand |
On Sat, 11 Jun 2022 00:59:34 +0800 Jiaqing Zhao wrote: > Currently kernel NCSI driver only supports the "VLAN + non-VLAN" mode > (Mode #2), but this mode is an optional mode [1] defined in NCSI spec > and some NCSI devices like Intel E810 Network Adapter [2] does not > support that mode. This patchset adds a new "ncsi,vlan-mode" device > tree property for configuring the VLAN mode of NCSI device. > > [1] Table 58 - VLAN Enable Modes > https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.0.0.pdf > [2] 12.6.5.4.3 VLAN > https://cdrdv2.intel.com/v1/dl/getContent/613875 Please don't post the same patches more than once a day. You posted the same patches 3 times within 15 minutes with no major difference :/ Why is "ncsi,vlan-mode" set via the device tree? Looks like something that can be configured at runtime.
On 2022-06-11 04:09, Jakub Kicinski wrote: > On Sat, 11 Jun 2022 00:59:34 +0800 Jiaqing Zhao wrote: >> Currently kernel NCSI driver only supports the "VLAN + non-VLAN" mode >> (Mode #2), but this mode is an optional mode [1] defined in NCSI spec >> and some NCSI devices like Intel E810 Network Adapter [2] does not >> support that mode. This patchset adds a new "ncsi,vlan-mode" device >> tree property for configuring the VLAN mode of NCSI device. >> >> [1] Table 58 - VLAN Enable Modes >> https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.0.0.pdf >> [2] 12.6.5.4.3 VLAN >> https://cdrdv2.intel.com/v1/dl/getContent/613875 > > Please don't post the same patches more than once a day. You posted the > same patches 3 times within 15 minutes with no major difference :/ Got it, sorry for misusing the mailing list. > Why is "ncsi,vlan-mode" set via the device tree? Looks like something > that can be configured at runtime. Actually this cannot be configured at runtime, the NCSI spec defines no command or register to determine which mode is supported by the device. If kernel want to enable VLAN on the NCSI device, either "Filtered tagged + Untagged" (current default) or "Any tagged + untagged" mode should be enabled, but unfortunately both of these two modes are documented to be optionally supported in the spec. And in real cases, there are devices that only supports one of them, or neither of them. So I added the device tree property to configure which mode to use.
On Sat, 11 Jun 2022 11:25:03 +0800 Jiaqing Zhao wrote: > > Why is "ncsi,vlan-mode" set via the device tree? Looks like something > > that can be configured at runtime. > > Actually this cannot be configured at runtime, the NCSI spec defines no > command or register to determine which mode is supported by the device. To be clear I'm not saying that it should be auto-detected and auto-configured. Just that user space can issue a command to change the config. > If kernel want to enable VLAN on the NCSI device, either "Filtered tagged > + Untagged" (current default) or "Any tagged + untagged" mode should be > enabled, but unfortunately both of these two modes are documented to be > optionally supported in the spec. And in real cases, there are devices > that only supports one of them, or neither of them. So I added the device > tree property to configure which mode to use. But for a given device its driver knows what modes are supported. Is it not possible to make the VLAN mode passed thru ncsi-netlink? Better still, can't "Filtered tagged + Untagged" vs "Any tagged + untagged" be decided based on netdev features being enabled like it is for normal netdevs?
On 2022-06-11 12:45, Jakub Kicinski wrote: > On Sat, 11 Jun 2022 11:25:03 +0800 Jiaqing Zhao wrote: >>> Why is "ncsi,vlan-mode" set via the device tree? Looks like something >>> that can be configured at runtime. >> >> Actually this cannot be configured at runtime, the NCSI spec defines no >> command or register to determine which mode is supported by the device. > > To be clear I'm not saying that it should be auto-detected and > auto-configured. Just that user space can issue a command to change > the config. > >> If kernel want to enable VLAN on the NCSI device, either "Filtered tagged >> + Untagged" (current default) or "Any tagged + untagged" mode should be >> enabled, but unfortunately both of these two modes are documented to be >> optionally supported in the spec. And in real cases, there are devices >> that only supports one of them, or neither of them. So I added the device >> tree property to configure which mode to use. > > But for a given device its driver knows what modes are supported. > Is it not possible to make the VLAN mode passed thru ncsi-netlink? > > Better still, can't "Filtered tagged + Untagged" vs "Any tagged + > untagged" be decided based on netdev features being enabled like it > is for normal netdevs? All ncsi devices uses the same driver as they uses same command set, so the driver doesn't know what modes are supported. And in current driver, the vlan related parameters are configured when registering the device, adding an ncsi-netlink command to do so seems to be unsuitable. And adding a netlink command requires extra application in userspace to switch the mode. In my opinion, it would be more user-friendly to make it usable on boot. Netdev also does not work as the ncsi device itself does not have its own netdev, the netdev comes from the mac device. For different vlan modes, the netdev feature set of its parent mac device are the same.
On Sat, 11 Jun 2022 13:18:51 +0800 Jiaqing Zhao wrote: > All ncsi devices uses the same driver as they uses same command set, > so the driver doesn't know what modes are supported. And in current > driver, the vlan related parameters are configured when registering > the device, adding an ncsi-netlink command to do so seems to be > unsuitable. Maybe you could draw a diagram? NC-SI is a bit confusing. > And adding a netlink command requires extra application in userspace > to switch the mode. In my opinion, it would be more user-friendly to > make it usable on boot. Unfortunately convenience is not reason to start adding system config into DT. > Netdev also does not work as the ncsi device itself does not have > its own netdev, the netdev comes from the mac device. For different > vlan modes, the netdev feature set of its parent mac device are the > same. You say that, yet the command handling already takes into account the VLAN list: if (list_empty(&ndp->vlan_vids)) { which come from the MAC netdev. What's wrong with setting the filtering mode based on NETIF_F_HW_VLAN_CTAG_FILTER ?
On 2022-06-11 13:44, Jakub Kicinski wrote: > On Sat, 11 Jun 2022 13:18:51 +0800 Jiaqing Zhao wrote: >> All ncsi devices uses the same driver as they uses same command set, >> so the driver doesn't know what modes are supported. And in current >> driver, the vlan related parameters are configured when registering >> the device, adding an ncsi-netlink command to do so seems to be >> unsuitable. > > Maybe you could draw a diagram? NC-SI is a bit confusing. Yes I admit NC-SI is confusing as its design is not as straightforward as the MAC-PHY structure. In NC-SI, there are two macs like below. Packets + NCSI commands Packets MAC-------------------------External controller MAC---------PHY The NCSI commands are used to set the behavior of the External controller MAC, like it's MAC address filter, VLAN filters. Those filtered packets will be transferred back to the MAC. Unlike PHY has standard registers to determine its model and capabilities, NC-SI seems does not have such way. >> And adding a netlink command requires extra application in userspace >> to switch the mode. In my opinion, it would be more user-friendly to >> make it usable on boot. > > Unfortunately convenience is not reason to start adding system config > into DT. Currently there is already a DT config "use-ncsi" is used to choose using MDIO PHY or NCSI stack in the MAC driver with NCSI support like ftgmac100. That's why I choose adding another DT option here. >> Netdev also does not work as the ncsi device itself does not have >> its own netdev, the netdev comes from the mac device. For different >> vlan modes, the netdev feature set of its parent mac device are the >> same. > > You say that, yet the command handling already takes into account the > VLAN list: > > if (list_empty(&ndp->vlan_vids)) { > > which come from the MAC netdev. What's wrong with setting the filtering > mode based on NETIF_F_HW_VLAN_CTAG_FILTER ? When configuring the mac driver, there might be two net_device_ops sets for MDIO or NC-SI. When using NC-SI, some features need to be delegated to the external controller MAC, like VLAN hardware filtering, different ndo_vlan_rx_{add,kill}_vid callbacks need to be assigned. The filtering mode is an optional mode defined in NC-SI spec, some devices does not support it. In this case, to support VLAN, I would personally in favor of using the "Any VLAN" mode to let the external MAC pass all packets to the internal one, and let the internal MAC handle it either with its own hardware filter or software filter. In this case, the VLAN list in NC-SI driver (used for setting the external MAC filter) is not used.