Message-ID: <363f681cf8ff4bdf0b8c1db68ca6ada6@codeaurora.org>
Date:   Mon, 04 Mar 2019 11:48:45 +0800
From:   xiaofeis@...eaurora.org
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Vinod Koul <vkoul@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        linux-arm-msm@...r.kernel.org,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Niklas Cassel <niklas.cassel@...aro.org>,
        netdev@...r.kernel.org
Subject: Re: [PATCH] net: dsa: read mac address from DT for slave device

On 2019-02-28 11:54, Florian Fainelli wrote:
> On 2/27/2019 6:23 PM, xiaofeis@...eaurora.org wrote:
>> On 2019-02-27 11:13, Florian Fainelli wrote:
>>> On 2/26/2019 6:04 PM, xiaofeis@...eaurora.org wrote:
>>>> On 2019-02-26 15:45, xiaofeis@...eaurora.org wrote:
>>>>> On 2019-02-26 01:27, Florian Fainelli wrote:
>>>>>> On 2/25/19 5:28 AM, xiaofeis@...eaurora.org wrote:
>>>>>>> Hi Florian
>>>>>>> 
>>>>>>> We have two slave DSA interfaces, wan0 and lan0, one is for wan 
>>>>>>> port,
>>>>>>> and the other is for lan port. Customer has it's mac address 
>>>>>>> pool,
>>>>>>> they
>>>>>>> want
>>>>>>> to assign the mac address from the pool on wan0, lan0, and other
>>>>>>> interfaces like
>>>>>>> wifi, bt. Coreboot/uboot will populate it to the DTS node, so the
>>>>>>> driver
>>>>>>> can
>>>>>>> get it from it's node. For DSA slave interface, it already has
>>>>>>> it's own
>>>>>>> DTS node, it's
>>>>>>> easy to just add one porperty "local-mac-address" there for the
>>>>>>> usage in
>>>>>>> DSA driver.
>>>>>>> 
>>>>>>> If not use DSA framework, normally we will use eth0.x and eth0.y 
>>>>>>> for
>>>>>>> wan
>>>>>>> and lan.
>>>>>>> On this case, customer usually also assign the MAC address on 
>>>>>>> these
>>>>>>> logical interface
>>>>>>> from it's pool.
>>>>>> 
>>>>>> OK, but this is not necessary per my previous explanation: the CPU 
>>>>>> <=>
>>>>>> WAN pipe is a separate broadcast domain (otherwise it is a 
>>>>>> security
>>>>>> hole
>>>>>> since you exposing LAN machines to the outside world), and so 
>>>>>> there is
>>>>>> no need for a separate MAC address. It might be convenient to have
>>>>>> one,
>>>>>> especially for the provider, if they run a management software 
>>>>>> (e.g.:
>>>>>> TR69), but it is not required per-se.
>>>>>> 
>>>>>> Let me ask a secondary question here, how many Ethernet MACs
>>>>>> connect to
>>>>>> the switch in this configuration? Is there one that is supposed to 
>>>>>> be
>>>>>> assigned all LAN traffic and one that is supposed to be assigned
>>>>>> all WAN
>>>>>> traffic? If so, then what you are doing makes even less
>>>>>> 
>>>>> 
>>>>> Only one MAC connected to switch cpu port, both lan0 and wan0 are 
>>>>> on
>>>>> the top of
>>>>> same interface(eth0).
>>>>> 
>>>> Customer doesn't care about the MAC controller's MAC address, just 
>>>> leave
>>>> it as the driver
>>>> randomly generated. They just want to assign the MAC address on wan 
>>>> and
>>>> lan DSA logical
>>>> interface.
>>>> 
>>>> Many customer doesn't use DSA, for example, they use eth0.1/eth0.2 
>>>> for
>>>> lan/wan with one MAC controller.
>>>> They configure switch wan port in vlan2 group, and lan port in vlan1
>>>> group, they usually assign mac address
>>>> on the logical interface(eth0.1&eth0.2), i think this is similar 
>>>> with
>>>> DSA slave interfaces.
>>> 
>>> Yes it is a similar use case, and in both cases there is no really a
>>> functional need for a separate MAC address for lan/eth0.1 or 
>>> wan/eth0.2
>>> since the switch should be configured to perform IVL (Individual VLAN
>>> Learning) and would determine the egress port just fine based on the 
>>> MAC
>>> DA. Because it is an established practice does not mean we should not
>>> challenge it :).
>>> 
>>> My issue with your change is that because DSA is meant to be a 
>>> flexible
>>> framework we do not know the type/nature of DSA master network device
>>> that is going to be used. That DSA master network device may or may 
>>> not
>>> have it own MAC DA filtering capability. Having to filter its own MAC
>>> address is fine and a well established behavior, having to filter for
>>> more than one unicast address starts to be questionable and eats up
>>> filter space that could be better used for filtering MC addresses
>>> instead. Another possible concern is a station trying to spoof the 
>>> MAC
>>> address, some switches may support protecting only one UC/management 
>>> MAC
>>> address, so having more than one could create security attack 
>>> surfaces.
>>> 
>>> To give you an example, I work with 3 generations of DSA master 
>>> network
>>> controllers (bcmgenet and bcmsysport drivers).
>>> 
>>> - GENET supports 17 perfect filters, but we must include its own MAC
>>> address, the broadcast address and that leaves only 15 filters for MC
>>> 
>>> - SYSTEMPORT is always attached to a switch but supports filtering 
>>> the
>>> MAC DA based on its own MAC and then it is in promiscuous mode
>>> 
>>> So with your scheme, we would leave only 13 filters for MC on GENET 
>>> and
>>> we would putting the interface in promiscuous mode for SYSTEMPORT.
>>> 
>>> Until we have a better switch-side filtering framework (and this is
>>> being worked on right now), I would prefer that we defer accepting 
>>> those
>>> type of features. Andrew and Vivien might feel differently about that
>>> though.
>> This patch is just add one more option, if there is valid mac address
>> populated
>> in the DTS, then use it or else still inherti from master. I don't 
>> think
>> it will
>> break the DSA flexible framework. I think this change make DSA more
>> flexible on
>> MAC address setting.
>> Many cusomter use some of our QCA chips, some direclty use DSA, some 
>> use
>> internal similar
>> mechanism(one netdevice for each switch port with swtichdev), we 
>> didn't
>> see any limiation
>> when they populiate the mac address for each port in DTS with only one
>> HW mac controller.
>> So my  opinion is this patch is want to add a option which is already
>> used in many
>> products, this change does not break anything, developer/customer can
>> chose use or not.
> 
> As I wrote, I am not totally opposed to it, I would prefer we had a
> better infrastructure for UC/MC filtering in place before landing this
> change but that is not there yet, and won't happen over night. So 
> please
> address Andrew's feedback to provide an update to the DSA binding
> document, repost and I will certainly Ack it this time.
> 
> Please be considerate of people giving you feedback and do not try to
> circle back to your use case and just explaining in a different way 
> than
> "works for me, accept it", because that's not going to work really well
> in the long run.
> 
> Looking forward for more contributions on the qca8k driver, thanks!

Thanks Florian, I have reply Andres's mail to provide DSA binding 
document change.

Powered by blists - more mailing lists