netdev - Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for HSR frame forward offload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <040b3b26-a7ef-47c7-845d-068a0c734e61@ti.com>
Date: Mon, 2 Sep 2024 11:21:08 +0530
From: "Anwar, Md Danish" <a0501179@...com>
To: Andrew Lunn <andrew@...n.ch>, Roger Quadros <rogerq@...nel.org>
CC: MD Danish Anwar <danishanwar@...com>,
        Dan Carpenter
	<dan.carpenter@...aro.org>,
        Jan Kiszka <jan.kiszka@...mens.com>,
        Javier
 Carrasco <javier.carrasco.cruz@...il.com>,
        Jacob Keller
	<jacob.e.keller@...el.com>,
        Diogo Ivo <diogo.ivo@...mens.com>, Simon Horman
	<horms@...nel.org>,
        Richard Cochran <richardcochran@...il.com>,
        Paolo Abeni
	<pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet
	<edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <srk@...com>,
        Vignesh Raghavendra
	<vigneshr@...com>
Subject: Re: [PATCH net-next v3 3/6] net: ti: icssg-prueth: Add support for
 HSR frame forward offload



On 8/30/2024 7:30 PM, Andrew Lunn wrote:
> On Fri, Aug 30, 2024 at 04:27:34PM +0300, Roger Quadros wrote:
>>
>>
>> On 28/08/2024 12:18, MD Danish Anwar wrote:
>>> Add support for offloading HSR port-to-port frame forward to hardware.
>>> When the slave interfaces are added to the HSR interface, the PRU cores
>>> will be stopped and ICSSG HSR firmwares will be loaded to them.
>>>
>>> Similarly, when HSR interface is deleted, the PRU cores will be stopped
>>> and dual EMAC firmware will be loaded to them.
>>
>> And what happens if we first started with switch mode and then switched to HSR mode?
>> Is this case possible and if yes should it revert to the last used mode
>> instead of forcing to dual EMAC mode?
>>
>>>
>>> This commit also renames some APIs that are common between switch and
>>> hsr mode with '_fw_offload' suffix.
>>>
>>> Signed-off-by: MD Danish Anwar <danishanwar@...com>
>>> ---

[...]

>>
>> Can you please check that if we are not in dual emac mode then we should
>> error out if any HSR feature is requested to be set.
> 
> This is where all the shenanigans with firmware makes things complex.
> 
> One of these options say 'If the interface is used for HSR, offload it
> to hardware if possible'. You should be able to set this flag
> anytime. It only has any effect when an interface is put into HSR
> mode, or if it is already in HSR mode. Hence, the firmware running
> right now should not matter.
> 
> I suspect the same is true for many of these flags.
> 

Andrew that is correct. Ideally the current running firmware should not
matter. But the firmware team only recommends dual EMAC -> HSR offload
and vise versa transitions. They suspect some configuration issues when
switch -> HSR transition happen. That is why they have recommended not
to do HSR hw offloading from switch mode. We can however keep doing SW
offload as suggested by you in v2.

>> As you mentioned there are some contstraints on what HSR features can be
>> enabled individually.
>> "2) Inorder to enable hsr-tag-ins-offload, hsr-dup-offload
>>    must also be enabled as these are tightly coupled in
>>    the firmware implementation."
>> You could do this check there by setting/clearing both features in tandem
>> if either one was set/cleared.
> 
> Software HSR should always work. Offloading is generally thought as
> accelerating this, if the hardware supports the current
> configuration. When offloading, if the hardware cannot support the
> current configuration, in general it should return -EOPNOTSUPP, and
> the software will keep doing the work.
> 
> It is not particularly friendly, more of a documentation issue, but
> the user needs to set the options the correct way for offload to
> work. Otherwise it keeps chugging along in software. I would not
> expect to see any error messages when offload is not possible.
> 

Yes, and I have already added this in this series based on your feedback
on v2.

I have one question though, in emac_ndo_set_features() should I change
these HSR related features irrespective of the current mode?

AFAIK, if NETIF_F_HW_HSR_FWD is set, the forwarding is offloaded to HW.
If NETIF_F_HW_HSR_FWD is not set the forwarding is not offloaded to HW
and is done in SW.

So, I don't see any need to enable this features if we are currently in
switch mode. Let me know what do you think. Should I still enable this
feature irrespective of current mode and later handle this in
prueth_hsr_port_link / unlink()?

> 	Andrew

-- 
Thanks and Regards,
Md Danish Anwar