Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bf66a840-594a-d90a-2ca6-595e95c09514@linux.intel.com>
Date:   Sat, 11 Jun 2022 17:58:46 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH v2 0/6] Configurable VLAN mode for NCSI driver
Content-Language: en-US
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     Samuel Mendoza-Jonas <sam@mendozajonas.com>,
        "David S . Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        openbmc@lists.ozlabs.org
References: <20220610165940.2326777-1-jiaqing.zhao@linux.intel.com>
 <20220610130903.0386c0d9@kernel.org>
 <3c9fa928-f416-3526-be23-12644d18db3b@linux.intel.com>
 <20220610214506.74c3f89c@kernel.org>
 <6f067302-74a8-702f-bf38-4477a805a528@linux.intel.com>
 <20220610224407.4e58dc5a@kernel.org>
From:   Jiaqing Zhao <jiaqing.zhao@linux.intel.com>
In-Reply-To: <20220610224407.4e58dc5a@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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.