Date: Mon, 29 Jan 2024 10:55:51 -0800
From: Florian Fainelli <f.fainelli@...il.com>
To: Tim Menninger <tmenninger@...estorage.com>,
 Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with
 phy_*_c45 callbacks

On 1/29/24 10:53, Tim Menninger wrote:
> On Tue, Jan 23, 2024 at 7:27 AM Vladimir Oltean <olteanv@...il.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

Powered by blists - more mailing lists