[PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Tim Menninger]

Set the read_c45 callback in the mii_bus struct in mv88e6xxx only if there
is a non-NULL phy_read_c45 callback on the chip mv88e6xxx_ops. Similarly
for write_c45 and phy_write_c45.

In commit 743a19e38d02 ("net: dsa: mv88e6xxx: Separate C22 and C45 transactions")
the MDIO bus driver split its API to separate C22 and C45 transfers.

In commit 1a136ca2e089 ("net: mdio: scan bus based on bus capabilities for C22 and C45")
we do a C45 mdio bus scan based on existence of the read_c45 callback
rather than checking MDIO bus capabilities then in
commit da099a7fb13d ("net: phy: Remove probe_capabilities") we remove the
probe_capabilities from the mii_bus struct.

The combination of the above results in a scenario (e.g. mv88e6185)
where we take a non-NULL read_c45 callback on the mii_bus struct to mean
we can perform a C45 read and proceed with a C45 MDIO bus scan. The scan
encounters a NULL phy_read_c45 callback in the mv88e6xxx_ops which implies
we can NOT perform a C45 read and fails with EOPNOTSUPP. The read_c45
callback should be NULL if phy_read_c45 is NULL, and similarly for
write_c45 and phy_write_c45.

Fixes: 1a136ca2e089 ("net: mdio: scan bus based on bus capabilities for C22 and C45")
Fixes: da099a7fb13d ("net: phy: Remove probe_capabilities")

Signed-off-by: Tim Menninger <tmenninger@purestorage.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 383b3c4d6f59..ba972d5427e5 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3739,8 +3739,10 @@ static int mv88e6xxx_mdio_register(struct mv88e6xxx_chip *chip,
 
 	bus->read = mv88e6xxx_mdio_read;
 	bus->write = mv88e6xxx_mdio_write;
-	bus->read_c45 = mv88e6xxx_mdio_read_c45;
-	bus->write_c45 = mv88e6xxx_mdio_write_c45;
+	bus->read_c45 = chip->info->ops->phy_read_c45
+		? mv88e6xxx_mdio_read_c45 : NULL;
+	bus->write_c45 = chip->info->ops->phy_write_c45
+		? mv88e6xxx_mdio_write_c45 : NULL;
 	bus->parent = chip->dev;
 	bus->phy_mask = ~GENMASK(chip->info->phy_base_addr +
 				 mv88e6xxx_num_ports(chip) - 1,
-- 
2.34.1

Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Andrew Lunn]

On Tue, Jan 16, 2024 at 07:35:42PM +0000, Tim Menninger wrote:
> Set the read_c45 callback in the mii_bus struct in mv88e6xxx only if there
> is a non-NULL phy_read_c45 callback on the chip mv88e6xxx_ops. Similarly
> for write_c45 and phy_write_c45.
> 
> In commit 743a19e38d02 ("net: dsa: mv88e6xxx: Separate C22 and C45 transactions")
> the MDIO bus driver split its API to separate C22 and C45 transfers.
> 
> In commit 1a136ca2e089 ("net: mdio: scan bus based on bus capabilities for C22 and C45")
> we do a C45 mdio bus scan based on existence of the read_c45 callback
> rather than checking MDIO bus capabilities then in
> commit da099a7fb13d ("net: phy: Remove probe_capabilities") we remove the
> probe_capabilities from the mii_bus struct.
> 
> The combination of the above results in a scenario (e.g. mv88e6185)
> where we take a non-NULL read_c45 callback on the mii_bus struct to mean
> we can perform a C45 read and proceed with a C45 MDIO bus scan. The scan
> encounters a NULL phy_read_c45 callback in the mv88e6xxx_ops which implies
> we can NOT perform a C45 read and fails with EOPNOTSUPP. The read_c45
> callback should be NULL if phy_read_c45 is NULL, and similarly for
> write_c45 and phy_write_c45.

Hi Tim

What does phylib do with the return of -EOPNOTSUPP? I've not tested
it, but i would expect it just keeps going with the scan? It treats it
as if there is no device there? And since it never accesses the
hardware, this should be fast?

Or is my assumption wrong? Do you see the EPOPNOTSUPP getting reported
back to user space, and the probe failing?

     Andrew

Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Tim Menninger]

On Tue, Jan 16, 2024 at 11:59 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Jan 16, 2024 at 07:35:42PM +0000, Tim Menninger wrote:
> > Set the read_c45 callback in the mii_bus struct in mv88e6xxx only if there
> > is a non-NULL phy_read_c45 callback on the chip mv88e6xxx_ops. Similarly
> > for write_c45 and phy_write_c45.
> >
> > In commit 743a19e38d02 ("net: dsa: mv88e6xxx: Separate C22 and C45 transactions")
> > the MDIO bus driver split its API to separate C22 and C45 transfers.
> >
> > In commit 1a136ca2e089 ("net: mdio: scan bus based on bus capabilities for C22 and C45")
> > we do a C45 mdio bus scan based on existence of the read_c45 callback
> > rather than checking MDIO bus capabilities then in
> > commit da099a7fb13d ("net: phy: Remove probe_capabilities") we remove the
> > probe_capabilities from the mii_bus struct.
> >
> > The combination of the above results in a scenario (e.g. mv88e6185)
> > where we take a non-NULL read_c45 callback on the mii_bus struct to mean
> > we can perform a C45 read and proceed with a C45 MDIO bus scan. The scan
> > encounters a NULL phy_read_c45 callback in the mv88e6xxx_ops which implies
> > we can NOT perform a C45 read and fails with EOPNOTSUPP. The read_c45
> > callback should be NULL if phy_read_c45 is NULL, and similarly for
> > write_c45 and phy_write_c45.
>
> Hi Tim
>
> What does phylib do with the return of -EOPNOTSUPP? I've not tested
> it, but i would expect it just keeps going with the scan? It treats it
> as if there is no device there? And since it never accesses the
> hardware, this should be fast?
>
> Or is my assumption wrong? Do you see the EPOPNOTSUPP getting reported
> back to user space, and the probe failing?
>
>      Andrew
>

Hi Andrew,

It bubbles up as EIO (the translation happens in get_phy_c45_ids when
get_phy_c45_devs_in_pkg fails) and ultimately causes the probe to fail.

The EIO causes the scan to stop and fail immediately - the way I read
mdiobus_scan_bus_c45, only ENODEV is permissible.

From logs:
$ dmesg | grep mv88e6
[   12.951149] mv88e6085 ixgbe-mdio-0000:05:00.0:00: switch 0x1a70
detected: Marvell 88E6185, revision 2
[   13.272812] mv88e6085 ixgbe-mdio-0000:05:00.0:00: Cannot register
MDIO bus (-5)
[   13.401140] mv88e6085: probe of ixgbe-mdio-0000:05:00.0:00 failed
with error -5
[   13.413105] mv88e6085 ixgbe-mdio-0000:05:00.1:00: switch 0x1a70
detected: Marvell 88E6185, revision 2
[   13.730227] mv88e6085 ixgbe-mdio-0000:05:00.1:00: Cannot register
MDIO bus (-5)
[   13.858336] mv88e6085: probe of ixgbe-mdio-0000:05:00.1:00 failed
with error -5

Tim

Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Andrew Lunn]

> Hi Andrew,
> 
> It bubbles up as EIO (the translation happens in get_phy_c45_ids when
> get_phy_c45_devs_in_pkg fails) and ultimately causes the probe to fail.
> 
> The EIO causes the scan to stop and fail immediately - the way I read
> mdiobus_scan_bus_c45, only ENODEV is permissible.

O.K. At minimum, this should be added to the commit message.

However, i'm wondering if this is the correct fix. I would prefer that
the scan code just acts on the -EOPNOTSUPP the same was as
-ENODEV. Maybe the error code from phy_c45_probe_present() should be
returned as is. And mdiobus_scan_bus_c45() is extended to handle
-EOPNOTSUPP ?

	    Andrew

Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Tim Menninger]

On Tue, Jan 16, 2024 at 3:21 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Hi Andrew,
> >
> > It bubbles up as EIO (the translation happens in get_phy_c45_ids when
> > get_phy_c45_devs_in_pkg fails) and ultimately causes the probe to fail.
> >
> > The EIO causes the scan to stop and fail immediately - the way I read
> > mdiobus_scan_bus_c45, only ENODEV is permissible.
>
> O.K. At minimum, this should be added to the commit message.
>
> However, i'm wondering if this is the correct fix. I would prefer that
> the scan code just acts on the -EOPNOTSUPP the same was as
> -ENODEV. Maybe the error code from phy_c45_probe_present() should be
> returned as is. And mdiobus_scan_bus_c45() is extended to handle
> -EOPNOTSUPP ?
>
>             Andrew

Noted about the commit message.

To return -EOPNOTSUPP high enough up that the mdiobus_scan function(s) can
directly handle it would mean at minimum, these functions have -EOPNOTSUPP
added to their respective list of possible return values:
    - get_phy_c45_ids (static)
    - phy_get_c45_ids
    - get_phy_device
    - mdiobus_scan (static)
    - mdiobus_scan_c22 (static)
    - mdiobus_scan_c45 (static)

I didn't look beyond to see whether any callers of phy_get_c45_ids or
get_phy_device also return errors as-are, but it feels a little broad in
scope to me.

My impression is still that the read_c45 function should agree with the
phy_read_c45 function, but that isn't a hill I care to die on if you still
think otherwise. Thoughts?

Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Vladimir Oltean]

On Tue, Jan 16, 2024 at 05:51:13PM -0800, Tim Menninger wrote:
> My impression is still that the read_c45 function should agree with the
> phy_read_c45 function, but that isn't a hill I care to die on if you still
> think otherwise. Thoughts?

FWIW, Tim's approach is consistent with what drivers/net/mdio/mdio-mux.c does.

		if (parent_bus->read)
			cb->mii_bus->read = mdio_mux_read;
		if (parent_bus->write)
			cb->mii_bus->write = mdio_mux_write;
		if (parent_bus->read_c45)
			cb->mii_bus->read_c45 = mdio_mux_read_c45;
		if (parent_bus->write_c45)
			cb->mii_bus->write_c45 = mdio_mux_write_c45;

My only objection to his patch (apart from the commit message which
should indeed be more detailed) is that I would have preferred the same
"if" syntax rather than the use of a ternary operator with NULL.

Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Andrew Lunn]

On Mon, Jan 22, 2024 at 02:33:49PM +0200, Vladimir Oltean wrote:
> On Tue, Jan 16, 2024 at 05:51:13PM -0800, Tim Menninger wrote:
> > My impression is still that the read_c45 function should agree with the
> > phy_read_c45 function, but that isn't a hill I care to die on if you still
> > think otherwise. Thoughts?
> 
> FWIW, Tim's approach is consistent with what drivers/net/mdio/mdio-mux.c does.
> 
> 		if (parent_bus->read)
> 			cb->mii_bus->read = mdio_mux_read;
> 		if (parent_bus->write)
> 			cb->mii_bus->write = mdio_mux_write;
> 		if (parent_bus->read_c45)
> 			cb->mii_bus->read_c45 = mdio_mux_read_c45;
> 		if (parent_bus->write_c45)
> 			cb->mii_bus->write_c45 = mdio_mux_write_c45;
> 
> My only objection to his patch (apart from the commit message which
> should indeed be more detailed) is that I would have preferred the same
> "if" syntax rather than the use of a ternary operator with NULL.

I agree it could be fixed this way. But what i don't like about the
current code is how C22 and C45 do different things with error
codes. Since the current code is trying to use an error code, i would
prefer to fix that error code handling, rather than swap to a
different way to indicate its not supported.

	  Andrew

Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Vladimir Oltean]

On Mon, Jan 22, 2024 at 03:30:20PM +0100, Andrew Lunn wrote:
> On Mon, Jan 22, 2024 at 02:33:49PM +0200, Vladimir Oltean wrote:
> > On Tue, Jan 16, 2024 at 05:51:13PM -0800, Tim Menninger wrote:
> > > My impression is still that the read_c45 function should agree with the
> > > phy_read_c45 function, but that isn't a hill I care to die on if you still
> > > think otherwise. Thoughts?
> > 
> > FWIW, Tim's approach is consistent with what drivers/net/mdio/mdio-mux.c does.
> > 
> > 		if (parent_bus->read)
> > 			cb->mii_bus->read = mdio_mux_read;
> > 		if (parent_bus->write)
> > 			cb->mii_bus->write = mdio_mux_write;
> > 		if (parent_bus->read_c45)
> > 			cb->mii_bus->read_c45 = mdio_mux_read_c45;
> > 		if (parent_bus->write_c45)
> > 			cb->mii_bus->write_c45 = mdio_mux_write_c45;
> > 
> > My only objection to his patch (apart from the commit message which
> > should indeed be more detailed) is that I would have preferred the same
> > "if" syntax rather than the use of a ternary operator with NULL.
> 
> I agree it could be fixed this way. But what i don't like about the
> current code is how C22 and C45 do different things with error
> codes. Since the current code is trying to use an error code, i would
> prefer to fix that error code handling, rather than swap to a
> different way to indicate its not supported.
> 
> 	  Andrew

You did write in commit da099a7fb13d ("net: phy: Remove probe_capabilities")
that the MDIO bus API is now this: "Deciding if to probe of PHYs using
C45 is now determine by if the bus provides the C45 read method."

Do you not agree that Tim's approach is the more straightforward
solution overall to skip C45 PHY probing, given this API, both code wise
and runtime wise? Are there downsides to it?

I have no objection to the C22 vs C45 error code handling inconsistency.
It can be improved, sure. But it also does not matter here, if we agree
that this problem can be sorted out in a more straightforward way with
no negative consequences.

I sort of don't understand the desire to have the smallest patch in
terms of lines of code, when the end result will end up being suboptimal
compared to something with just a little more lines (1 vs 4).

Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Tim Menninger]

On Mon, Jan 22, 2024 at 7:12 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Jan 22, 2024 at 03:30:20PM +0100, Andrew Lunn wrote:
> > On Mon, Jan 22, 2024 at 02:33:49PM +0200, Vladimir Oltean wrote:
> > > On Tue, Jan 16, 2024 at 05:51:13PM -0800, Tim Menninger wrote:
> > > > My impression is still that the read_c45 function should agree with the
> > > > phy_read_c45 function, but that isn't a hill I care to die on if you still
> > > > think otherwise. Thoughts?
> > >
> > > FWIW, Tim's approach is consistent with what drivers/net/mdio/mdio-mux.c does.
> > >
> > >             if (parent_bus->read)
> > >                     cb->mii_bus->read = mdio_mux_read;
> > >             if (parent_bus->write)
> > >                     cb->mii_bus->write = mdio_mux_write;
> > >             if (parent_bus->read_c45)
> > >                     cb->mii_bus->read_c45 = mdio_mux_read_c45;
> > >             if (parent_bus->write_c45)
> > >                     cb->mii_bus->write_c45 = mdio_mux_write_c45;
> > >
> > > My only objection to his patch (apart from the commit message which
> > > should indeed be more detailed) is that I would have preferred the same
> > > "if" syntax rather than the use of a ternary operator with NULL.
> >
> > I agree it could be fixed this way. But what i don't like about the
> > current code is how C22 and C45 do different things with error
> > codes. Since the current code is trying to use an error code, i would
> > prefer to fix that error code handling, rather than swap to a
> > different way to indicate its not supported.
> >
> >         Andrew
>
> You did write in commit da099a7fb13d ("net: phy: Remove probe_capabilities")
> that the MDIO bus API is now this: "Deciding if to probe of PHYs using
> C45 is now determine by if the bus provides the C45 read method."
>
> Do you not agree that Tim's approach is the more straightforward
> solution overall to skip C45 PHY probing, given this API, both code wise
> and runtime wise? Are there downsides to it?
>
> I have no objection to the C22 vs C45 error code handling inconsistency.
> It can be improved, sure. But it also does not matter here, if we agree
> that this problem can be sorted out in a more straightforward way with
> no negative consequences.
>
> I sort of don't understand the desire to have the smallest patch in
> terms of lines of code, when the end result will end up being suboptimal
> compared to something with just a little more lines (1 vs 4).


Andrew, would you feel differently if I added to the patch the same
logic for C22 ops? Perhaps that symmetry should have existed
in the initial patch, e.g.

    bus->read = chip->info->ops->phy_read
        ? mv88e6xxx_mdio_read : NULL;
    bus->write = chip->info->ops->phy_write
        ? mv88e6xxx_mdio_write : NULL;
    bus->read_c45 = chip->info->ops->phy_read_c45
        ? mv88e6xxx_mdio_read_c45 : NULL;
    bus->write_c45 = chip->info->ops->phy_write_c45
        ? mv88e6xxx_mdio_write_c45 : NULL;

Vladimir, as far as style I have no objections moving to straightlined
if's. I most prefer to follow the convention the rest of the code follows
and can change my patch accordingly.

Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Vladimir Oltean]

On Mon, Jan 22, 2024 at 07:46:06AM -0800, Tim Menninger wrote:
> Andrew, would you feel differently if I added to the patch the same
> logic for C22 ops? Perhaps that symmetry should have existed
> in the initial patch, e.g.
> 
>     bus->read = chip->info->ops->phy_read
>         ? mv88e6xxx_mdio_read : NULL;
>     bus->write = chip->info->ops->phy_write
>         ? mv88e6xxx_mdio_write : NULL;
>     bus->read_c45 = chip->info->ops->phy_read_c45
>         ? mv88e6xxx_mdio_read_c45 : NULL;
>     bus->write_c45 = chip->info->ops->phy_write_c45
>         ? mv88e6xxx_mdio_write_c45 : NULL;

Here it's me who would disagree, for the simple fact that it's not
needed, and we shouldn't complicate the code with things that are not
needed (and also, bug fixes should not make more logical changes than
strictly necessary). All mv88e6xxx_ops structure provide the C22
phy_read() and phy_write(). As listed below, in order:

static const struct mv88e6xxx_ops mv88e6085_ops = {
	.phy_read = mv88e6185_phy_ppu_read,
	.phy_write = mv88e6185_phy_ppu_write,
};

static const struct mv88e6xxx_ops mv88e6095_ops = {
	.phy_read = mv88e6185_phy_ppu_read,
	.phy_write = mv88e6185_phy_ppu_write,
};

static const struct mv88e6xxx_ops mv88e6097_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6123_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6131_ops = {
	.phy_read = mv88e6185_phy_ppu_read,
	.phy_write = mv88e6185_phy_ppu_write,
};

static const struct mv88e6xxx_ops mv88e6141_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6161_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6165_ops = {
	.phy_read = mv88e6165_phy_read,
	.phy_write = mv88e6165_phy_write,
};

static const struct mv88e6xxx_ops mv88e6171_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6172_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6175_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6176_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6185_ops = {
	.phy_read = mv88e6185_phy_ppu_read,
	.phy_write = mv88e6185_phy_ppu_write,
};

static const struct mv88e6xxx_ops mv88e6190_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6190x_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6191_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6240_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6250_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6290_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6320_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6321_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6341_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6350_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6351_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6352_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6390_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6390x_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

static const struct mv88e6xxx_ops mv88e6393x_ops = {
	.phy_read = mv88e6xxx_g2_smi_phy_read_c22,
	.phy_write = mv88e6xxx_g2_smi_phy_write_c22,
	.phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
	.phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
};

> Vladimir, as far as style I have no objections moving to straightlined
> if's. I most prefer to follow the convention the rest of the code follows
> and can change my patch accordingly.

Yes, so my objections have to do with code style and with the structure
of the commit message.

It should have been a more linear description of: user impact of the
problem -> identify the cause -> why the existing mechanism to prevent
the issue does not work -> what can be done to resolve the problem ->
see if this is consistent with what is done elsewhere -> why the
proposed change does not break other things -> optionally consider
alternative solutions and explain why this one is better.

Basically be as preemptive as possible w.r.t. questions that might be
crossing readers' minds as they read the commit. You should view any
clarification question you receive during review as a potential
improvement you could make to the commit message or comments.

Also, the commit title should focus on what is being fixed from a user
impact perspective. And the Fixes: tag should normally be a single one,
which coincides with what 'git blame' finds (corollary: bugs which have
no user visible impact are not treated like bugs, and are fixed as part
of the "net-next" tree).

Also, there should be no blank lines between the Fixes: and Signed-off-by:
tags. And the next patch revision should be generated with git
format-patch --subject-prefix "PATCH net v2" to clarify it is targeted
to the https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
tree for fixes. See the warning here (Target tree name not specified in
the subject).
https://patchwork.kernel.org/project/netdevbpf/patch/20240116193542.711482-1-tmenninger@purestorage.com/

The space beneath the "---" line in the formatted patch is not processed
by git when applying the patch. You can use it for extra info such as
change log compared to v1, and a link to v1 on lore.kernel.org.

Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Tim Menninger]

On Tue, Jan 23, 2024 at 7:27 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Jan 22, 2024 at 07:46:06AM -0800, Tim Menninger wrote:
> > Andrew, would you feel differently if I added to the patch the same
> > logic for C22 ops? Perhaps that symmetry should have existed
> > in the initial patch, e.g.
> >
> >     bus->read = chip->info->ops->phy_read
> >         ? mv88e6xxx_mdio_read : NULL;
> >     bus->write = chip->info->ops->phy_write
> >         ? mv88e6xxx_mdio_write : NULL;
> >     bus->read_c45 = chip->info->ops->phy_read_c45
> >         ? mv88e6xxx_mdio_read_c45 : NULL;
> >     bus->write_c45 = chip->info->ops->phy_write_c45
> >         ? mv88e6xxx_mdio_write_c45 : NULL;
>
> Here it's me who would disagree, for the simple fact that it's not
> needed, and we shouldn't complicate the code with things that are not
> needed (and also, bug fixes should not make more logical changes than
> strictly necessary). All mv88e6xxx_ops structure provide the C22
> phy_read() and phy_write(). As listed below, in order:
>
> static const struct mv88e6xxx_ops mv88e6085_ops = {
>         .phy_read = mv88e6185_phy_ppu_read,
>         .phy_write = mv88e6185_phy_ppu_write,
> };
>
> static const struct mv88e6xxx_ops mv88e6095_ops = {
>         .phy_read = mv88e6185_phy_ppu_read,
>         .phy_write = mv88e6185_phy_ppu_write,
> };
>
> static const struct mv88e6xxx_ops mv88e6097_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6123_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6131_ops = {
>         .phy_read = mv88e6185_phy_ppu_read,
>         .phy_write = mv88e6185_phy_ppu_write,
> };
>
> static const struct mv88e6xxx_ops mv88e6141_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6161_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6165_ops = {
>         .phy_read = mv88e6165_phy_read,
>         .phy_write = mv88e6165_phy_write,
> };
>
> static const struct mv88e6xxx_ops mv88e6171_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6172_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6175_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6176_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6185_ops = {
>         .phy_read = mv88e6185_phy_ppu_read,
>         .phy_write = mv88e6185_phy_ppu_write,
> };
>
> static const struct mv88e6xxx_ops mv88e6190_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6190x_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6191_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6240_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6250_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6290_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6320_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6321_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6341_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6350_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6351_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6352_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6390_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6390x_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> static const struct mv88e6xxx_ops mv88e6393x_ops = {
>         .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>         .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>         .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>         .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
> };
>
> > Vladimir, as far as style I have no objections moving to straightlined
> > if's. I most prefer to follow the convention the rest of the code follows
> > and can change my patch accordingly.
>
> Yes, so my objections have to do with code style and with the structure
> of the commit message.
>
> It should have been a more linear description of: user impact of the
> problem -> identify the cause -> why the existing mechanism to prevent
> the issue does not work -> what can be done to resolve the problem ->
> see if this is consistent with what is done elsewhere -> why the
> proposed change does not break other things -> optionally consider
> alternative solutions and explain why this one is better.
>
> Basically be as preemptive as possible w.r.t. questions that might be
> crossing readers' minds as they read the commit. You should view any
> clarification question you receive during review as a potential
> improvement you could make to the commit message or comments.
>
> Also, the commit title should focus on what is being fixed from a user
> impact perspective. And the Fixes: tag should normally be a single one,
> which coincides with what 'git blame' finds (corollary: bugs which have
> no user visible impact are not treated like bugs, and are fixed as part
> of the "net-next" tree).
>
> Also, there should be no blank lines between the Fixes: and Signed-off-by:
> tags. And the next patch revision should be generated with git
> format-patch --subject-prefix "PATCH net v2" to clarify it is targeted
> to the https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
> tree for fixes. See the warning here (Target tree name not specified in
> the subject).
> https://patchwork.kernel.org/project/netdevbpf/patch/20240116193542.711482-1-tmenninger@purestorage.com/
>
> The space beneath the "---" line in the formatted patch is not processed
> by git when applying the patch. You can use it for extra info such as
> change log compared to v1, and a link to v1 on lore.kernel.org.

Thank you for all the feedback.

Since there's the other thread, am I to follow through with this patch?

If so, I'll clean it up and resubmit (should it be the same thread or
a new one?).

Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks

[Florian Fainelli]

On 1/29/24 10:53, Tim Menninger wrote:
> On Tue, Jan 23, 2024 at 7:27 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>>
>> On Mon, Jan 22, 2024 at 07:46:06AM -0800, Tim Menninger wrote:
>>> Andrew, would you feel differently if I added to the patch the same
>>> logic for C22 ops? Perhaps that symmetry should have existed
>>> in the initial patch, e.g.
>>>
>>>      bus->read = chip->info->ops->phy_read
>>>          ? mv88e6xxx_mdio_read : NULL;
>>>      bus->write = chip->info->ops->phy_write
>>>          ? mv88e6xxx_mdio_write : NULL;
>>>      bus->read_c45 = chip->info->ops->phy_read_c45
>>>          ? mv88e6xxx_mdio_read_c45 : NULL;
>>>      bus->write_c45 = chip->info->ops->phy_write_c45
>>>          ? mv88e6xxx_mdio_write_c45 : NULL;
>>
>> Here it's me who would disagree, for the simple fact that it's not
>> needed, and we shouldn't complicate the code with things that are not
>> needed (and also, bug fixes should not make more logical changes than
>> strictly necessary). All mv88e6xxx_ops structure provide the C22
>> phy_read() and phy_write(). As listed below, in order:
>>
>> static const struct mv88e6xxx_ops mv88e6085_ops = {
>>          .phy_read = mv88e6185_phy_ppu_read,
>>          .phy_write = mv88e6185_phy_ppu_write,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6095_ops = {
>>          .phy_read = mv88e6185_phy_ppu_read,
>>          .phy_write = mv88e6185_phy_ppu_write,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6097_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6123_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6131_ops = {
>>          .phy_read = mv88e6185_phy_ppu_read,
>>          .phy_write = mv88e6185_phy_ppu_write,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6141_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6161_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6165_ops = {
>>          .phy_read = mv88e6165_phy_read,
>>          .phy_write = mv88e6165_phy_write,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6171_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6172_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6175_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6176_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6185_ops = {
>>          .phy_read = mv88e6185_phy_ppu_read,
>>          .phy_write = mv88e6185_phy_ppu_write,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6190_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6190x_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6191_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6240_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6250_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6290_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6320_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6321_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6341_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6350_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6351_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6352_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6390_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6390x_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>> static const struct mv88e6xxx_ops mv88e6393x_ops = {
>>          .phy_read = mv88e6xxx_g2_smi_phy_read_c22,
>>          .phy_write = mv88e6xxx_g2_smi_phy_write_c22,
>>          .phy_read_c45 = mv88e6xxx_g2_smi_phy_read_c45,
>>          .phy_write_c45 = mv88e6xxx_g2_smi_phy_write_c45,
>> };
>>
>>> Vladimir, as far as style I have no objections moving to straightlined
>>> if's. I most prefer to follow the convention the rest of the code follows
>>> and can change my patch accordingly.
>>
>> Yes, so my objections have to do with code style and with the structure
>> of the commit message.
>>
>> It should have been a more linear description of: user impact of the
>> problem -> identify the cause -> why the existing mechanism to prevent
>> the issue does not work -> what can be done to resolve the problem ->
>> see if this is consistent with what is done elsewhere -> why the
>> proposed change does not break other things -> optionally consider
>> alternative solutions and explain why this one is better.
>>
>> Basically be as preemptive as possible w.r.t. questions that might be
>> crossing readers' minds as they read the commit. You should view any
>> clarification question you receive during review as a potential
>> improvement you could make to the commit message or comments.
>>
>> Also, the commit title should focus on what is being fixed from a user
>> impact perspective. And the Fixes: tag should normally be a single one,
>> which coincides with what 'git blame' finds (corollary: bugs which have
>> no user visible impact are not treated like bugs, and are fixed as part
>> of the "net-next" tree).
>>
>> Also, there should be no blank lines between the Fixes: and Signed-off-by:
>> tags. And the next patch revision should be generated with git
>> format-patch --subject-prefix "PATCH net v2" to clarify it is targeted
>> to the https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
>> tree for fixes. See the warning here (Target tree name not specified in
>> the subject).
>> https://patchwork.kernel.org/project/netdevbpf/patch/20240116193542.711482-1-tmenninger@purestorage.com/
>>
>> The space beneath the "---" line in the formatted patch is not processed
>> by git when applying the patch. You can use it for extra info such as
>> change log compared to v1, and a link to v1 on lore.kernel.org.
> 
> Thank you for all the feedback.
> 
> Since there's the other thread, am I to follow through with this patch?
> 
> If so, I'll clean it up and resubmit (should it be the same thread or
> a new one?).

Your original approach IMHO scales better and is consistent with other 
parts of the code, it simply lacks a better commit message as pointed 
out by Vladimir. Andrew being both a DSA subsystem maintainer and 
mv88e6xxx driver maintainer might see things differently however.
-- 
Florian