netdev - Re: [RFC PATCH 2/2] net: selpcimac: Add driver for SEL PCIe network adapter

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: <20241029174939.1f7306df@fedora.home>
Date: Tue, 29 Oct 2024 17:49:39 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Robert Joslyn <robert_joslyn@...inc.com>
Cc: <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
 <lee@...nel.org>
Subject: Re: [RFC PATCH 2/2] net: selpcimac: Add driver for SEL PCIe network
 adapter

Hello,

On Mon, 28 Oct 2024 15:35:08 -0700
Robert Joslyn <robert_joslyn@...inc.com> wrote:

> Add support for SEL FPGA based network adapters. The network device is
> implemented as an FPGA IP core and enumerated by the selpvmf driver.
> This is used on multiple devices, including:
>  - SEL-3350 mainboard
>  - SEL-3390E4 card
>  - SEL-3390T card

Make sure that you get the right people as recipients for this
patchset. You would need at least the net maintainers, make sure to use
the scripts/get_maintainers.pl tool to know who to send the patch to.

> 
> Signed-off-by: Robert Joslyn <robert_joslyn@...inc.com>
> ---
>  MAINTAINERS                                  |   1 +
>  drivers/net/ethernet/Kconfig                 |   1 +
>  drivers/net/ethernet/Makefile                |   1 +
>  drivers/net/ethernet/sel/Kconfig             |  31 +
>  drivers/net/ethernet/sel/Makefile            |  22 +
>  drivers/net/ethernet/sel/ethtool.c           | 404 ++++++++
>  drivers/net/ethernet/sel/ethtool.h           |  17 +
>  drivers/net/ethernet/sel/hw_interface.c      | 410 ++++++++
>  drivers/net/ethernet/sel/hw_interface.h      |  46 +
>  drivers/net/ethernet/sel/mac_main.c          | 155 +++
>  drivers/net/ethernet/sel/mdio.c              | 166 ++++
>  drivers/net/ethernet/sel/mdio.h              |  15 +
>  drivers/net/ethernet/sel/mii.c               | 422 +++++++++
>  drivers/net/ethernet/sel/mii.h               |  21 +
>  drivers/net/ethernet/sel/mii_interface.c     | 133 +++
>  drivers/net/ethernet/sel/mii_interface.h     |  23 +
>  drivers/net/ethernet/sel/netdev.c            | 946 +++++++++++++++++++
>  drivers/net/ethernet/sel/netdev.h            |  24 +
>  drivers/net/ethernet/sel/netdev_isr.c        | 245 +++++
>  drivers/net/ethernet/sel/netdev_isr.h        |  20 +
>  drivers/net/ethernet/sel/netdev_rx.c         | 785 +++++++++++++++
>  drivers/net/ethernet/sel/netdev_rx.h         |  17 +
>  drivers/net/ethernet/sel/netdev_tx.c         | 647 +++++++++++++
>  drivers/net/ethernet/sel/netdev_tx.h         |  22 +
>  drivers/net/ethernet/sel/pci_mac.h           | 290 ++++++
>  drivers/net/ethernet/sel/pci_mac_hw_regs.h   | 370 ++++++++
>  drivers/net/ethernet/sel/pci_mac_sysfs.c     | 642 +++++++++++++
>  drivers/net/ethernet/sel/pci_mac_sysfs.h     |  14 +
>  drivers/net/ethernet/sel/sel_pci_mac_ioctl.h |  25 +
>  drivers/net/ethernet/sel/sel_phy.h           |  91 ++
>  drivers/net/ethernet/sel/sel_phy_broadcom.c  | 316 +++++++
>  drivers/net/ethernet/sel/sel_phy_broadcom.h  |  15 +
>  drivers/net/ethernet/sel/sel_phy_marvell.c   | 458 +++++++++
>  drivers/net/ethernet/sel/sel_phy_marvell.h   |  15 +
>  drivers/net/ethernet/sel/sel_phy_ti.c        | 419 ++++++++
>  drivers/net/ethernet/sel/sel_phy_ti.h        |  14 +
>  drivers/net/ethernet/sel/sel_soft_phy.c      |  98 ++
>  drivers/net/ethernet/sel/sel_soft_phy.h      |  15 +
>  drivers/net/ethernet/sel/semaphore.h         |  85 ++
>  drivers/net/ethernet/sel/sfp.c               | 615 ++++++++++++
>  drivers/net/ethernet/sel/sfp.h               |  61 ++

I haven't reviewed the code itself as this is a biiiiig patch, I
suggest you try to split it into more digestable patches, focusing on
individual aspects of the driver.

One thing is the PHY support as you mention in the cover-letter, in the
current state this driver re-implements PHY drivers from what I
understand. You definitely need to use the kernel infra for PHY
handling.

As it seems this driver also re-implements SFP entirely, I suggest you
look into phylink [1]. This will help you supporting the PHYs and SFPs.
You can take a look at the mvneta.c and mvpp2 drivers for examples.

Make sure you handle the mdio bus control using the dedicated framework
(see mii_bus et al.).

I'd be happy to give you more reviews, but this would be a more
manageable task with smaller patches :)

Best regards,

Maxime