netdev - RE: [PATCH v3 2/3] net: ethernet: renesas: Add Ethernet Switch driver

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, 27 Sep 2022 05:59:25 +0000
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH v3 2/3] net: ethernet: renesas: Add Ethernet Switch driver

Hi Andrew,

> From: Andrew Lunn, Sent: Tuesday, September 27, 2022 4:18 AM
> 
> On Mon, Sep 26, 2022 at 08:12:14AM +0000, Yoshihiro Shimoda wrote:
> > Hi Andrew,
> >
> > > From: Andrew Lunn, Sent: Friday, September 23, 2022 10:12 PM
> > >
> > > > +/* Forwarding engine block (MFWD) */
> > > > +static void rswitch_fwd_init(struct rswitch_private *priv)
> > > > +{
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < RSWITCH_NUM_HW; i++) {
> > > > +		iowrite32(FWPC0_DEFAULT, priv->addr + FWPC0(i));
> > > > +		iowrite32(0, priv->addr + FWPBFC(i));
> > > > +	}
> > >
> > > What is RSWITCH_NUM_HW?
> >
> > I think the name is unclear...
> > Anyway, this hardware has 3 ethernet ports and 2 CPU ports.
> > So that the RSWITCH_NUM_HW is 5. Perhaps, RSWITCH_NUM_ALL_PORTS
> > is better name.
> 
> How do the CPU ports differ to the other ports? When you mention CPU
> ports, it makes me wonder if this should be a DSA driver?

I compared a DSA diagram [1] and this Ethernet Switch and then
this switch differs than the DSA diagram:
- This switch has a feature which accesses DRAM directly like an "ethernet controller".
  I called this feature as "cpu port", but it might be incorrect.
- This switch has doesn't have any "control path". Instead of that, this switch
  is controled by registers via APB (internal bus) directly.

So, IIUC, this should not be a DSA driver.

[1] https://bootlin.com/blog/tag/dsa/

> Is there a public data sheet for this device?

Unfortunately, we have no public data sheet for this device.
But, I tried to figure this switch diagram about control/data paths as below:

Control path:
+--- R-Car S4-8 SoC -------------------------+
|                                            |
| CPU ---(APB bus)---+--- Ethernet Switch ---|---(MDIO)--------------+
|                    |                       |                       |
|                    +--- Ethernet SERDES    |              External Ethernet PHY --- RJ45
|                                            |
+--------------------------------------------+
Notes: The switch and SERDES have 3 ports of MDIO and SGMII.

Data Path:
+--- R-Car S4-8 SoC ---------------------------------------------------------+
|                                                                            |
| CPU ---(AXI bus)---+--- DRAM        +--------+                             |
|                    +---(cpu port)---|        |---(ether port)--- SERDES ---|---(SGMII)--- PHY --- RJ45
|                    |                | Switch |---(ether port)--- SERDES ---|---(SGMII)--- PHY --- RJ45
|                    +---(cpu port)---|        |---(ether port)--- SERDES ---|---(SGMII)--- PHY --- RJ45
|                                     +--------|                             |
+----------------------------------------------------------------------------+

The current driver only supports one of MDIO, cpu port and ethernet port, and it acts as an ethernet device.

> > Perhaps, since the current driver supports 1 ethernet port and 1 CPU port only,
> > I should modify this driver for the current condition strictly.
> 
> I would suggest you support all three user ports. For an initial
> driver you don't need to support any sort of acceleration. You don't
> need any hardware bridging etc. That can be added later. Just three
> separated ports.

Thank you for your suggestion. However, supporting three user ports
is required to modify an external ethernet PHY driver.
(For now, a boot loader initialized the PHY, but one port only.)

The PHY is 88E2110 on my environment, so Linux has a driver in
drivers/net/phy/marvell10g.c. However, I guess this is related to
configuration of the PHY chip on the board, it needs to change
the host 7interface mode, but the driver doesn't support it for now.

So, I'd like to support three user ports later if possible...

> > > > +
> > > > +	for (i = 0; i < RSWITCH_NUM_ETHA; i++) {
> > >
> > > RSWITCH_NUM_ETHA appears to be the number of ports?
> >
> > Yes, this is number of ethernet ports.
> 
> In the DSA world we call these user ports.

I got it.
However, when I read the dsa document of Linux kernel,
it seems to call DSA ports. Perhaps, I could misunderstand the document though...
https://docs.kernel.org/networking/dsa/dsa.html

> > > > +	kfree(c->skb);
> > > > +	c->skb = NULL;
> > >
> > > When i see code like this, i wonder why an API call like
> > > dev_kfree_skb() is not being used. I would suggest reaming this to
> > > something other than skb, which has a very well understood meaning.
> >
> > Perhaps, c->skbs is better name than just c->skb.
> 
> Yes, that is O.K.

I got it. Thanks!

Best regards,
Yoshihiro Shimoda

>      Andrew