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

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

[Russell King (Oracle)]

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(-)

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;
-- 
2.30.2

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

[Vladimir Oltean]

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

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

[Russell King (Oracle)]

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?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Vladimir Oltean]

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

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

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Vladimir Oltean]

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.

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

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Colin Foster]

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

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

[Russell King (Oracle)]

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>
---
Back in the thread for the previous posting
https://lore.kernel.org/r/Zh1GvcOTXqb7CpQt@shell.armlinux.org.uk
it was identified that two other sub-drivers also needed an update,
and through that identical code in each was identified. In the
final message of the sub-thread from Vladimir, Vladimir volunteered
to pick this up and I agreed. However, I haven't seen anything yet.
I guess Vladimir's attention is elsewhere, so I've done the minimal
fixup for this driver.

 drivers/net/dsa/ocelot/felix.c           | 54 ++++++++++++++++--------
 drivers/net/dsa/ocelot/felix.h           |  1 +
 drivers/net/dsa/ocelot/felix_vsc9959.c   |  1 +
 drivers/net/dsa/ocelot/ocelot_ext.c      |  1 +
 drivers/net/dsa/ocelot/seville_vsc9953.c |  1 +
 5 files changed, 40 insertions(+), 18 deletions(-)

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/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 85952d841f28..d4799a908abc 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -2717,6 +2717,7 @@ static int felix_pci_probe(struct pci_dev *pdev,
 	ds->num_ports = felix->info->num_ports;
 	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;
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;
diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
index 049930da0521..5ac8897e232b 100644
--- a/drivers/net/dsa/ocelot/seville_vsc9953.c
+++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
@@ -1009,6 +1009,7 @@ static int seville_probe(struct platform_device *pdev)
 	ds->dev = &pdev->dev;
 	ds->num_ports = felix->info->num_ports;
 	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_SEVILLE;
-- 
2.30.2

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

[Vladimir Oltean]

On Tue, May 28, 2024 at 04:15:42PM +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>
> ---
> Back in the thread for the previous posting
> https://lore.kernel.org/r/Zh1GvcOTXqb7CpQt@shell.armlinux.org.uk
> it was identified that two other sub-drivers also needed an update,
> and through that identical code in each was identified. In the
> final message of the sub-thread from Vladimir, Vladimir volunteered
> to pick this up and I agreed. However, I haven't seen anything yet.
> I guess Vladimir's attention is elsewhere, so I've done the minimal
> fixup for this driver.

Indeed, sorry.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 28 May 2024 16:15:42 +0100 you 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>
> ---
> Back in the thread for the previous posting
> https://lore.kernel.org/r/Zh1GvcOTXqb7CpQt@shell.armlinux.org.uk
> it was identified that two other sub-drivers also needed an update,
> and through that identical code in each was identified. In the
> final message of the sub-thread from Vladimir, Vladimir volunteered
> to pick this up and I agreed. However, I haven't seen anything yet.
> I guess Vladimir's attention is elsewhere, so I've done the minimal
> fixup for this driver.
> 
> [...]

Here is the summary with links:
  - [net-next] net: dsa: felix: provide own phylink MAC operations
    https://git.kernel.org/netdev/net-next/c/ef0e51dccdc4

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html