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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx/
> >
> > 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