| Message ID | E1rvIcO-006bQQ-Md@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: felix: provide own phylink MAC operations | expand |
On Fri, Apr 12, 2024 at 04:15:08PM +0100, Russell King (Oracle) wrote: > Convert felix to provide its own phylink MAC operations, thus > avoiding the shim layer in DSA's port.c. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > diff --git a/drivers/net/dsa/ocelot/ocelot_ext.c b/drivers/net/dsa/ocelot/ocelot_ext.c > index 22187d831c4b..a8927dc7aca4 100644 > --- a/drivers/net/dsa/ocelot/ocelot_ext.c > +++ b/drivers/net/dsa/ocelot/ocelot_ext.c > @@ -96,6 +96,7 @@ static int ocelot_ext_probe(struct platform_device *pdev) > ds->num_tx_queues = felix->info->num_tx_queues; > > ds->ops = &felix_switch_ops; > + ds->phylink_mac_ops = &felix_phylink_mac_ops; There are actually 2 more places which need this: felix_vsc9959.c, seville_vsc9953.c. > ds->priv = ocelot; > felix->ds = ds; > felix->tag_proto = DSA_TAG_PROTO_OCELOT; > -- > 2.30.2 > --- pw-bot: cr
Hi Russell,
On Fri, Apr 12, 2024 at 04:15:08PM +0100, Russell King (Oracle) wrote:
> drivers/net/dsa/ocelot/ocelot_ext.c | 1 +
The patch looks good. I'm chasing down an unrelated regression so I
don't know if I'll be able to test it on hardware until after EOSS.
Colin Foster
On Mon, Apr 15, 2024 at 01:34:53PM +0300, Vladimir Oltean wrote: > On Fri, Apr 12, 2024 at 04:15:08PM +0100, Russell King (Oracle) wrote: > > Convert felix to provide its own phylink MAC operations, thus > > avoiding the shim layer in DSA's port.c. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > diff --git a/drivers/net/dsa/ocelot/ocelot_ext.c b/drivers/net/dsa/ocelot/ocelot_ext.c > > index 22187d831c4b..a8927dc7aca4 100644 > > --- a/drivers/net/dsa/ocelot/ocelot_ext.c > > +++ b/drivers/net/dsa/ocelot/ocelot_ext.c > > @@ -96,6 +96,7 @@ static int ocelot_ext_probe(struct platform_device *pdev) > > ds->num_tx_queues = felix->info->num_tx_queues; > > > > ds->ops = &felix_switch_ops; > > + ds->phylink_mac_ops = &felix_phylink_mac_ops; > > There are actually 2 more places which need this: felix_vsc9959.c, > seville_vsc9953.c. 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; 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. 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?
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
On Mon, Apr 15, 2024 at 07:01:50PM +0300, Vladimir Oltean wrote: > 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 Sounds like there's an opportunity to beneficially clean this driver up before I make this change, so I'll hold off this patch until that's happened. I probably don't have the spare cycles for that.
On Mon, Apr 15, 2024 at 05:49:01PM +0100, Russell King (Oracle) wrote: > Sounds like there's an opportunity to beneficially clean this driver > up before I make this change Yes, it is. > so I'll hold off this patch until that's happened. I probably don't > have the spare cycles for that. Alternatively to you waiting, I could pick up this patch and include it as the last one in the cleanup series, if you're ok with that.
On Mon, Apr 15, 2024 at 11:13:59PM +0300, Vladimir Oltean wrote: > On Mon, Apr 15, 2024 at 05:49:01PM +0100, Russell King (Oracle) wrote: > > Sounds like there's an opportunity to beneficially clean this driver > > up before I make this change > > Yes, it is. > > > so I'll hold off this patch until that's happened. I probably don't > > have the spare cycles for that. > > Alternatively to you waiting, I could pick up this patch and include it > as the last one in the cleanup series, if you're ok with that. Sure, please do. Thanks.
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index 61e95487732d..3aa66bf9eafc 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -1050,24 +1050,32 @@ static void felix_phylink_get_caps(struct dsa_switch *ds, int port, config->supported_interfaces); } -static void felix_phylink_mac_config(struct dsa_switch *ds, int port, +static void felix_phylink_mac_config(struct phylink_config *config, unsigned int mode, const struct phylink_link_state *state) { - struct ocelot *ocelot = ds->priv; - struct felix *felix = ocelot_to_felix(ocelot); + struct dsa_port *dp = dsa_phylink_to_port(config); + struct ocelot *ocelot = dp->ds->priv; + int port = dp->index; + struct felix *felix; + + felix = ocelot_to_felix(ocelot); if (felix->info->phylink_mac_config) felix->info->phylink_mac_config(ocelot, port, mode, state); } -static struct phylink_pcs *felix_phylink_mac_select_pcs(struct dsa_switch *ds, - int port, - phy_interface_t iface) +static struct phylink_pcs * +felix_phylink_mac_select_pcs(struct phylink_config *config, + phy_interface_t iface) { - struct ocelot *ocelot = ds->priv; - struct felix *felix = ocelot_to_felix(ocelot); + struct dsa_port *dp = dsa_phylink_to_port(config); + struct ocelot *ocelot = dp->ds->priv; struct phylink_pcs *pcs = NULL; + int port = dp->index; + struct felix *felix; + + felix = ocelot_to_felix(ocelot); if (felix->pcs && felix->pcs[port]) pcs = felix->pcs[port]; @@ -1075,11 +1083,13 @@ static struct phylink_pcs *felix_phylink_mac_select_pcs(struct dsa_switch *ds, return pcs; } -static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port, +static void felix_phylink_mac_link_down(struct phylink_config *config, unsigned int link_an_mode, phy_interface_t interface) { - struct ocelot *ocelot = ds->priv; + struct dsa_port *dp = dsa_phylink_to_port(config); + struct ocelot *ocelot = dp->ds->priv; + int port = dp->index; struct felix *felix; felix = ocelot_to_felix(ocelot); @@ -1088,15 +1098,19 @@ static void felix_phylink_mac_link_down(struct dsa_switch *ds, int port, felix->info->quirks); } -static void felix_phylink_mac_link_up(struct dsa_switch *ds, int port, +static void felix_phylink_mac_link_up(struct phylink_config *config, + struct phy_device *phydev, unsigned int link_an_mode, phy_interface_t interface, - struct phy_device *phydev, int speed, int duplex, bool tx_pause, bool rx_pause) { - struct ocelot *ocelot = ds->priv; - struct felix *felix = ocelot_to_felix(ocelot); + struct dsa_port *dp = dsa_phylink_to_port(config); + struct ocelot *ocelot = dp->ds->priv; + int port = dp->index; + struct felix *felix; + + felix = ocelot_to_felix(ocelot); ocelot_phylink_mac_link_up(ocelot, port, phydev, link_an_mode, interface, speed, duplex, tx_pause, rx_pause, @@ -2083,6 +2097,14 @@ static void felix_get_mm_stats(struct dsa_switch *ds, int port, ocelot_port_get_mm_stats(ocelot, port, stats); } +const struct phylink_mac_ops felix_phylink_mac_ops = { + .mac_select_pcs = felix_phylink_mac_select_pcs, + .mac_config = felix_phylink_mac_config, + .mac_link_down = felix_phylink_mac_link_down, + .mac_link_up = felix_phylink_mac_link_up, +}; +EXPORT_SYMBOL_GPL(felix_phylink_mac_ops); + const struct dsa_switch_ops felix_switch_ops = { .get_tag_protocol = felix_get_tag_protocol, .change_tag_protocol = felix_change_tag_protocol, @@ -2104,10 +2126,6 @@ const struct dsa_switch_ops felix_switch_ops = { .get_sset_count = felix_get_sset_count, .get_ts_info = felix_get_ts_info, .phylink_get_caps = felix_phylink_get_caps, - .phylink_mac_config = felix_phylink_mac_config, - .phylink_mac_select_pcs = felix_phylink_mac_select_pcs, - .phylink_mac_link_down = felix_phylink_mac_link_down, - .phylink_mac_link_up = felix_phylink_mac_link_up, .port_enable = felix_port_enable, .port_fast_age = felix_port_fast_age, .port_fdb_dump = felix_fdb_dump, diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h index dbf5872fe367..4d3489aaa659 100644 --- a/drivers/net/dsa/ocelot/felix.h +++ b/drivers/net/dsa/ocelot/felix.h @@ -82,6 +82,7 @@ struct felix_tag_proto_ops { struct netlink_ext_ack *extack); }; +extern const struct phylink_mac_ops felix_phylink_mac_ops; extern const struct dsa_switch_ops felix_switch_ops; /* DSA glue / front-end for struct ocelot */ diff --git a/drivers/net/dsa/ocelot/ocelot_ext.c b/drivers/net/dsa/ocelot/ocelot_ext.c index 22187d831c4b..a8927dc7aca4 100644 --- a/drivers/net/dsa/ocelot/ocelot_ext.c +++ b/drivers/net/dsa/ocelot/ocelot_ext.c @@ -96,6 +96,7 @@ static int ocelot_ext_probe(struct platform_device *pdev) ds->num_tx_queues = felix->info->num_tx_queues; ds->ops = &felix_switch_ops; + ds->phylink_mac_ops = &felix_phylink_mac_ops; ds->priv = ocelot; felix->ds = ds; felix->tag_proto = DSA_TAG_PROTO_OCELOT;
Convert felix to provide its own phylink MAC operations, thus avoiding the shim layer in DSA's port.c. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/ocelot/felix.c | 54 +++++++++++++++++++---------- drivers/net/dsa/ocelot/felix.h | 1 + drivers/net/dsa/ocelot/ocelot_ext.c | 1 + 3 files changed, 38 insertions(+), 18 deletions(-)