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@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?).