On Thu, Apr 20, 2023 at 07:34:53PM +0300, Vladimir Oltean wrote:
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).
Thanks.
I did see the code above, and I do agree it is informational wrt the meaning of the values.
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.
I guess it's subjective. I certainly don't feel strongly about this. And I appreciate you taking the time to respond to my idea.
I have no objections to leaving this patch as is (with '3').