netdev - Re: [PATCH v2 0/8] Add support for ICSSG-based Ethernet on SR1.0 devices

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]

Date: Tue, 23 Jan 2024 14:15:47 +0200
From: Roger Quadros <rogerq@...nel.org>
To: Diogo Ivo <diogo.ivo@...mens.com>, danishanwar@...com,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, andrew@...n.ch, dan.carpenter@...aro.org,
 grygorii.strashko@...com, jacob.e.keller@...el.com, robh@...nel.org,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
 devicetree@...r.kernel.org
Cc: jan.kiszka@...mens.com
Subject: Re: [PATCH v2 0/8] Add support for ICSSG-based Ethernet on SR1.0
 devices

Hello Diogo,

On 17/01/2024 18:14, Diogo Ivo wrote:
> Hello,
> 
> This series extends the current ICSSG-based Ethernet driver to support
> Silicon Revision 1.0 devices.
> 
> Notable differences between the Silicon Revisions are that there is
> no TX core in SR1.0 with this being handled by the firmware, requiring
> extra DMA channels to communicate commands to the firmware (with the
> firmware being different as well) and in the packet classifier.
> 
> The motivation behind it is that a significant number of Siemens
> devices containing SR1.0 silicon have been deployed in the field
> and need to be supported and updated to newer kernel versions
> without losing functionality.

Adding SR1.0 support with all its ifdefs makes the driver more complicated
than it should be.

I think we need to treat SR1.0 and SR2.0 as different devices with their
own independent drivers. While the data path is pretty much the same, 
also like in am65-cpsw-nuss.c, the initialization, firmware and other
runtime logic is significantly different.

How about introducing a new icssg_prueth_sr1.c and putting all the SR1 stuff
there? You could still re-use the other helper files in net/ti/icssg/.
It also warrants for it's own Kconfig symbol so it can be built only
if required.
Any common logic could still be moved to icssg_common.c and re-used in both drivers.

> 
> This series is based on TI's 5.10 SDK [1].
> 
> The first version of this patch series can be found in [2].
> 
> [1]: https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/tree/?h=ti-linux-5.10.y
> [2]: https://lore.kernel.org/all/20231219174548.3481-1-diogo.ivo@siemens.com/
> 
> Changes in v2:
>  - Addressed Krzysztof's comments on the dt-binding
>  - Removed explicit references to SR2.0
>  - Added static keyword as indicated by the kernel test robot
> 
> Diogo Ivo (8):
>   dt-bindings: net: Add support for AM65x SR1.0 in ICSSG
>   net: ti: icssg-config: add SR1.0-specific configuration bits
>   net: ti: icssg-prueth: add SR1.0-specific configuration bits
>   net: ti: icssg-classifier: Add support for SR1.0
>   net: ti: icssg-config: Add SR1.0 configuration functions
>   net: ti: icssg-ethtool: Adjust channel count for SR1.0
>   net: ti: iccsg-prueth: Add necessary functions for SR1.0 support
>   net: ti: icssg-prueth: Wire up support for SR1.0
> 
>  .../bindings/net/ti,icssg-prueth.yaml         |  29 +-
>  .../net/ethernet/ti/icssg/icssg_classifier.c  | 113 +++-
>  drivers/net/ethernet/ti/icssg/icssg_config.c  |  86 ++-
>  drivers/net/ethernet/ti/icssg/icssg_config.h  |  55 ++
>  drivers/net/ethernet/ti/icssg/icssg_ethtool.c |  10 +-
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 556 ++++++++++++++++--
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h  |  21 +-
>  7 files changed, 788 insertions(+), 82 deletions(-)
> 

-- 
cheers,
-roger