Messages in this thread | | | Date | Thu, 20 Apr 2023 19:34:53 +0300 | From | Vladimir Oltean <> | Subject | Re: [PATCH v2 net-next 3/9] net: enetc: only commit preemptible TCs to hardware when MM TX is active |
| |
On Thu, Apr 20, 2023 at 04:42:52PM +0200, Simon Horman wrote: > > + /* This will time out after the standard value of 3 verification > > + * attempts. To not sleep forever, it relies on a non-zero verify_time, > > + * guarantee which is provided by the ethtool nlattr policy. > > + */ > > + return read_poll_timeout(enetc_port_rd, val, > > + ENETC_MMCSR_GET_VSTS(val) == 3, > > nit: 3 is doing a lot of work here. > As a follow-up, perhaps it could become part of an enum?
IMHO it's easy to abuse enums, when numbers could do just fine. I think that in context (seeing the entire enetc_ethtool.c), this is not as bad as just this patch makes it to be, because the other occurrence of ENETC_MMCSR_GET_VSTS() is:
switch (ENETC_MMCSR_GET_VSTS(val)) { case 0: state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; break; case 2: state->verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING; break; case 3: state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED; break; case 4: state->verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED; break; case 5: default: state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN; break; }
so it's immediately clear what the 3 represents (in vim I just press '*' to see the other occurrences of ENETC_MMCSR_GET_VSTS).
I considered it, but I don't feel an urgent necessity to add an enum here. Doing that would essentially transform the code into:
return read_poll_timeout(enetc_port_rd, val, ENETC_MMCSR_GET_VSTS(val) == ENETC_MM_VSTS_SUCCEEDED,
switch (ENETC_MMCSR_GET_VSTS(val)) { case ENETC_MMCSR_VSTS_DISABLED: state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED; break; case ENETC_MMCSR_VSTS_VERIFYING: state->verify_status = ETHTOOL_MM_VERIFY_STATUS_VERIFYING; break; case ENETC_MMCSR_VSTS_SUCCEEDED: state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED; break; case ENETC_MMCSR_VSTS_FAILED: state->verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED; break; case ENETC_MMCSR_VSTS_UNKNOWN: default: state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN; break; }
which to my eye is more bloated.
| |