netdev - Re: [RFC 2/3] net: Provide switchdev driver for NXP's More Than IP L2 switch

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 23 Jun 2021 22:01:46 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Lukasz Majewski <lukma@...x.de>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Joakim Zhang <qiangqing.zhang@....com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>, netdev@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>,
        Mark Einon <mark.einon@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        linux-kernel@...ts.infradead.org
Subject: Re: [RFC 2/3] net: Provide switchdev driver for NXP's More Than IP
 L2 switch

> > Using volatile is generally wrong. Why do you need it?
> 
> This was the code, which I took from the legacy driver. I will adjust
> it.

It is called 'vendor crap' for a reason.

> > > +	for_each_available_child_of_node(p, port) {
> > > +		if (of_property_read_u32(port, "reg", &port_num))
> > > +			continue;
> > > +
> > > +		priv->n_ports = port_num;
> > > +
> > > +		fep_np = of_parse_phandle(port, "phy-handle", 0);  
> > 
> > As i said, phy-handle points to a phy. It minimum, you need to call
> > this mac-handle. But that then makes this switch driver very different
> > to every other switch driver.
> 
> Other drivers (DSA for example) use "ethernet" or "link" properties.
> Maybe those can be reused?

Not really. They have well known meanings and they are nothing like
what you are trying to do. You need a new name. Maybe 'mac-handle'?


> > > +		pdev = of_find_device_by_node(fep_np);
> > > +		ndev = platform_get_drvdata(pdev);
> > > +		priv->fep[port_num - 1] = netdev_priv(ndev);  
> > 
> > What happens when somebody puts reg=<42>; in DT?
> 
> I do guess that this will break the code.
> 
> However, DSA DT descriptions also rely on the exact numbering [1] (via
> e.g. reg property) of the ports. I've followed this paradigm.

DSA does a range check:

        for_each_available_child_of_node(ports, port) {
                err = of_property_read_u32(port, "reg", &reg);
                if (err)
                        goto out_put_node;

                if (reg >= ds->num_ports) {
                        dev_err(ds->dev, "port %pOF index %u exceeds num_ports (%zu)\n",
                                port, reg, ds->num_ports);
                        err = -EINVAL;
                        goto out_put_node;
                }

> > I would say, your basic structure needs to change, to make it more
> > like other switchdev drivers. You need to replace the two FEC device
> > instances with one switchdev driver.
> 
> I've used the cpsw_new.c as the example.
> 
> > The switchdev driver will then
> > instantiate the two netdevs for the two external MACs.
> 
> Then there is a question - what about eth[01], which already exists?

They don't exist. cpsw_new is not used at the same time as cpsw, it
replaces it. This new driver would replace the FEC driver. cpsw_new
makes use of some of the code in the cpsw driver to implement two
netdevs. This new FEC switch driver would do the same, make use of
some of the low level code, e.g. for DMA access, MDIO bus, etc.

> To be honest - such driver for L2 switch already has been forward
> ported by me [2] to v4.19.

Which is fine, you can do whatever you want in your own fork. But for
mainline, we need a clean architecture. I'm not convinced your code is
that clean, and how well future features can be added. Do you have
support for VLANS? Adding and removing entries to the lookup tables?
How will IGMP snooping work? How will STP work?

    Andrew