mbox series

[v2,0/6] Configurable VLAN mode for NCSI driver

Message ID 20220610165940.2326777-1-jiaqing.zhao@linux.intel.com
Headers show
Series Configurable VLAN mode for NCSI driver | expand

Message

Jiaqing Zhao June 10, 2022, 4:59 p.m. UTC
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

v2:
- Fix indentation

Jiaqing Zhao (6):
  net/ncsi: Fix value of NCSI_CAP_VLAN_ANY
  net/ncsi: Rename NCSI_CAP_VLAN_NO to NCSI_CAP_VLAN_FILTERED
  net/ncsi: Enable VLAN filtering when callback is registered
  ftgmac100: Remove enable NCSI VLAN filtering
  dt-bindings: net: Add NCSI bindings
  net/ncsi: Support VLAN mode configuration

 .../devicetree/bindings/net/ncsi.yaml         | 34 ++++++++++++++
 MAINTAINERS                                   |  2 +
 drivers/net/ethernet/faraday/ftgmac100.c      |  3 --
 include/dt-bindings/net/ncsi.h                | 15 ++++++
 net/ncsi/internal.h                           |  5 +-
 net/ncsi/ncsi-manage.c                        | 46 ++++++++++++++++---
 6 files changed, 93 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/ncsi.yaml
 create mode 100644 include/dt-bindings/net/ncsi.h

Comments

Jakub Kicinski June 10, 2022, 8:09 p.m. UTC | #1
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.
Jiaqing Zhao June 11, 2022, 3:25 a.m. UTC | #2
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.
Jakub Kicinski June 11, 2022, 4:45 a.m. UTC | #3
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?
Jiaqing Zhao June 11, 2022, 5:18 a.m. UTC | #4
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.
Jakub Kicinski June 11, 2022, 5:44 a.m. UTC | #5
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 ?
Jiaqing Zhao June 11, 2022, 9:58 a.m. UTC | #6
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.