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]

Message-ID: <YzH65W3r1IV+rHFW@lunn.ch>
Date:   Mon, 26 Sep 2022 21:17:57 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
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

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?

Is there a public data sheet for this 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.

> > > +
> > > +	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. 

> > > +	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.

     Andrew