netdev - RE: [PATCH net-next v4] dsa: lan9303: Add 3 ethtool stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 1 Dec 2022 16:42:10 +0000
From:   <Jerry.Ray@...rochip.com>
To:     <olteanv@...il.com>
CC:     <andrew@...n.ch>, <f.fainelli@...il.com>, <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-next v4] dsa: lan9303: Add 3 ethtool stats

>>  static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
>>                                     uint64_t *data)
>>  {
>>       struct lan9303 *chip = ds->priv;
>>       unsigned int u;
>>
>>       for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
>>               u32 reg;
>>               int ret;
>>
>>               ret = lan9303_read_switch_port(
>>                       chip, port, lan9303_mib[u].offset, &reg);
>>
>> -             if (ret)
>> +             if (ret) {
>>                       dev_warn(chip->dev, "Reading status port %d reg %u failed\n",
>>                                port, lan9303_mib[u].offset);
>> +                     reg = 0;
>> +             }
>
>This part of the change still is unrelated and affects existing code.
>Bug fixes to existing code are submitted as separate patches. In some
>kernel trees, they are at the very least tagged with a Fixes: tag and
>put before other development work. In netdev, they are sent to a different
>git tree (net.git) which eventually lands in a different set of branches
>than net-next.git. You need to not mix bug fixes with development code.
>Andrew also suggested that you separate each logical change into a
>separate patch.
>
>This, plus the fact that Jakub asked you to also provide standardized
>counters, not just free-form ones, which you found it ok to disregard.
>
>I hope that only a misunderstanding is involved, because if it isn't,
>then Jakub will know you, alright, but as the person who disregards
>review feedback and expects that it'll just disappear. I think Jakub
>has pretty solid grounds to not expect that you'll come back with what
>has been requested.
>

I was hoping to get at least this change upstreamed this cycle and address
the standardized counters down the road.  get_stats64 will require PHYLINK.
I replied as such and was hoping to get the benefit of the doubt.
I have a patch set under internal review for migrating to phylink and adding
support for the port_max_mtu api.

I have been asked to, and have plans for, adding VLAN support and then adding
Hardware Timestamping support for the LAN9354 variant.  I was hoping to show
some progress with this patchset, but if I need to pull it and pick it up
later then I'll shuffle things around. I wasn't disregarding Jakub's request
and apologize if was viewed that way. I'm simply trying to generate a correct
patchset on something relatively simple before moving on to more complicated
patchsets.

>Sorry, this patch has a NACK from me at least until you come back with
>some clarifications, and split the change.
>
>>               data[u] = reg;
>>       }

My plan is to correct the issues pointed out in this patchset, but not extend
it to cover standardized counters. get_stats64 is a separate beast requiring
a background thread to keep the stats stored in the driver accumulated and
up to date.  The current patch simply reads a few more registers from the
hardware when asked.

Regards,
Jerry.