netdev - Re: [PATCH net-next] net: dsa: felix: provide own phylink MAC operations

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]

Message-ID: <20240415160150.yejcazpjqvn7vhxu@skbuf>
Date: Mon, 15 Apr 2024 19:01:50 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Vladimir Oltean <olteanv@...il.com>, Andrew Lunn <andrew@...n.ch>,
	Claudiu Manoil <claudiu.manoil@....com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	UNGLinuxDriver@...rochip.com,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Colin Foster <colin.foster@...advantage.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: felix: provide own phylink MAC
 operations

On Mon, Apr 15, 2024 at 04:24:45PM +0100, Russell King (Oracle) wrote:
> Looking at these three, isn't there good reason to merge the allocation
> and initialisation of struct dsa_switch together in all three drivers?
> All three are basically doing the same thing:
> 
> felix_vsc9959.c:
>         ds->dev = &pdev->dev;
>         ds->num_ports = felix->info->num_ports;
>         ds->num_tx_queues = felix->info->num_tx_queues;
>         ds->ops = &felix_switch_ops;
>         ds->priv = ocelot;
>         felix->ds = ds;
> 
> ocelot_ext.c:
>         ds->dev = dev;
>         ds->num_ports = felix->info->num_ports;
>         ds->num_tx_queues = felix->info->num_tx_queues;
>         ds->ops = &felix_switch_ops;
>         ds->priv = ocelot;
>         felix->ds = ds;
> 
> seville_vsc9953.c:
>         ds->dev = &pdev->dev;
>         ds->num_ports = felix->info->num_ports;
>         ds->ops = &felix_switch_ops;
>         ds->priv = ocelot;
>         felix->ds = ds;

Yes, there is :)

If dev_set_drvdata() were to be used instead of platform_set_drvdata()/
pci_set_drvdata(), there's room for even more common code.

> Also, I note that felix->info->num_tx_queues on seville_vsc9953.c
> is set to OCELOT_NUM_TC, which is defined to be 8, and is the same
> value for ocelot_ext and felix_vsc9959. Presumably this unintentionally
> missing from seville_vsc9953.c... because why initialise a private
> struct member to a non-zero value and then not use it.
> 
> An alternative would be to initialise .num_tx_queues in seville_vsc9953.c
> to zero.

It makes me wonder why felix->info->num_tx_queues even exists...

It was introduced by commit de143c0e274b ("net: dsa: felix: Configure
Time-Aware Scheduler via taprio offload") at a time when vsc9959
(LS1028A) was the only switch supported by the driver. It seems
unnecessary.

8 traffic classes, and 1 queue per traffic class, is a common
architectural feature of all switches in the family. So they could all
just set OCELOT_NUM_TC in the common allocation function and be fine
(and remove felix->info->num_tx_queues).

When num_tx_queues=0, this is implicitly converted to 1 by dsa_user_create(),
and this is good enough for basic operation for a switch port. The tc
qdisc offload layer works with netdev TX queues, so for QoS offload we
need to pretend we have multiple TX queues. The VSC9953, like ocelot_ext,
doesn't export QoS offload, so it doesn't really matter. But we can
definitely set num_tx_queues=8 for all switches.

> If we had common code doing this initialisation, then it wouldn't be
> missed... and neither would have _this_ addition of the phylink MAC
> ops missed the other two drivers - so I think that's something which
> should be done as a matter of course - and thus there will be no need
> to export these two data structures, just an initialisation (and
> destruction) function. I don't think we would even need the destruction
> function if we used devm_kzalloc().
> 
> Good idea?

Looking again at the driver, I see it's not very consistent in its use of
devres... It is used elsewhere, including in felix_pci_probe() itself:
devm_request_threaded_irq().

Yes, I think the use of devres here would be an improvement.

Note that felix_pci_probe() will still have to call pci_disable_device()
on the error teardown path.

For even more consistency, it would be great if the error teardown
labels were called after what they do, rather than after the path that
triggered them. Example:
- goto err_pci_enable -> goto out
- goto err_alloc_felix -> goto out_pci_disable