| Message ID | 20240116193542.711482-1-tmenninger@purestorage.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: Make *_c45 callbacks agree with phy_*_c45 callbacks | expand |
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
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
> 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
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?
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.
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
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).
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.
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.
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?).
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.
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,
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(-)