RE: [PATCH net-next v4] dsa: lan9303: Add 3 ethtool stats
From: Jerry.Ray
Date: Thu Dec 01 2022 - 11:42:19 EST
>> 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, ®);
>>
>> - 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.