Re: [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

[Guenter Roeck <linux@roeck-us.net>]

On 7/7/20 3:10 AM, Enric Balletbo i Serra wrote:
> Hi Prashant and Guenter,
> 
> Thank you to spend your time on look at this.
> 
> On 4/7/20 16:26, Guenter Roeck wrote:
>> The EC reports a variety of error codes. Most of those, with the exception
>> of EC_RES_INVALID_VERSION, are converted to -EPROTO. As result, the actual
>> error code gets lost. Convert all EC errors to Linux error codes to report
>> a more meaningful error to the caller to aid debugging.
>>
>> Cc: Yu-Hsuan Hsu <yuhsuan@chromium.org>
>> Cc: Prashant Malani <pmalani@chromium.org>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
> 
> For me the patch looks good as is. Said that, I'd like to discuss a bit more
> (but not too much) about it :-). My "concerns" are:
> 
> - It is not clear to me if some EC errors match directly to a linux kernel error
> codes, the importance of them, and which action should be taken in that case or
> if just reporting is enough.
> 
> - The EC result can be obtained, either, enabling more debug traces or using the
> linux kernel tracing tools. So, IMO mapping _all_ the errors has very little value.
> 
> Right now, the policy I followed is return -EPROTO for all EC errors and
> introduce/match a new error when someone (another kernel driver or userspace
> needs to know about it, i.e [1], where some EC drivers do different actions if
> -ENOTSUPP is returned). We introduced that error because we had a use case for it.
> 
> The future, I'd like to maintain this policy if it makes sense to you. And
> introduce a new error when we have a use case for it. I.e if at some point a
> kernel driver needs to know when the EC is busy (-EBUSY) because the driver
> waits and retries again, I'll be fine to introduce this new error/match code.
> Otherwise, I feel that has no really value.
> 
> Said that, if you still feel, that this will help you for debugging purposes, I
> am completely fine to pick the patch.
> 
> Thoughts?
> 

Yes, I did feel it was useful, error codes are never perfect, it is rarely
possible to enable tracing in situations in which the error that was observed,
and I think it would have been and would be useful (having it would have saved
hours of analysis and debugging time), but not for the cost of spending hours
of argument about it. Please drop.

Guenter