Messages in this thread | | | From | <> | Subject | RE: [PATCH net-next v4] dsa: lan9303: Add 3 ethtool stats | Date | Thu, 1 Dec 2022 16:42:10 +0000 |
| |
>> 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.
| |