Re: [PATCH v2 10/17] iio: cros_ec: Use cros_ec_cmd()

[Enric Balletbo i Serra <enric.balletbo@collabora.com>]

Hi Prashant,

Could you base your next series on top of [1]. Also, if you can give your
feedback and test those, would be much appreciated ;-)

BTW, I think you need to fix your sendmail as the series are not threaded and
appear as independent patches in patchwork, which is a bit hard to follow.

Thanks,
 Enric

[1] https://lore.kernel.org/patchwork/cover/1197210/


On 18/2/20 19:30, Prashant Malani wrote:
> Hi All,
> 
> Just thought I'd ping this thread since it's been a week since the last
> email.
> 
> On Mon, Feb 10, 2020 at 12:14:01PM -0800, Prashant Malani wrote:
>> Hi All (trimming most code parts of the thread for the sake of brevity),
>>
>> Thanks for listing the points Enric, Please see my notes inline:
>>
>> On Mon, Feb 10, 2020 at 3:03 AM Enric Balletbo i Serra
>> <enric.balletbo@collabora.com> wrote:
>>>
>>> Hi Gwendal, Prashant et all
>>>
>>> On 7/2/20 19:47, Gwendal Grignou wrote:
>>>> On Thu, Feb 6, 2020 at 10:50 AM Prashant Malani <pmalani@chromium.org> wrote:
>>>>>
>>>>> Hi Enric,
>>>>>
>>>>> Thanks for taking a look at the patch. Please see my response inline:
>> ....
>>>>>>>> @@ -171,9 +162,11 @@ int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *state,
>>>>>>>>
>>>>>>>>      memcpy(state->msg->data, &state->param, sizeof(state->param));
>>>>>>>>
>>>>>>>> -    ret = cros_ec_cmd_xfer_status(state->ec, state->msg);
>>>>>>>> +    ret = cros_ec_cmd_xfer(state->ec, state->msg);
>>>>>>>>      if (ret < 0)
>>>>>>>>              return ret;
>>>>>>>> +    else if (state->msg->result != EC_RES_SUCCESS)
>>>>>>>> +            return -EPROTO;
>>>>>>>>
>>>>>>
>>>>>> There is no way to use the new cros_ec_cmd here?
>>>> When the EC does not support sensor fifo,
>>>> cros_ec_motion_send_host_cmd() is on the data path. For instance, it
>>>> is called 2 times every 10ms by chrome to calculate the lid angle. I
>>>> would be reluctant to call malloc. Given it is well encapsulated into
>>>> the sensor stack. Does it make sense to call cros_ec_cmd_xfer
>>>> directly?
>>>>
>>>
>>> Thanks Gwendal for pointing this, it makes totally sense, and I suspect this can
>>> happen on other cases.
>>>
>>> Just to make clear, my concern is not about not using the new 'cros_ec_cmd'
>>> here, is about changing 'cros_ec_cmd_xfer_status' for 'cros_ec_cmd_xfer'. Also,
>>> my other concern is how useful is the new 'cros_ec_cmd' replacing what we have
>>> now if cannot replace all current uses.
>>>
>>> My points of view are this:
>>>
>>> * Actually we have cros_ec_cmd_xfer and cros_ec_cmd_xfer_status, use the second
>>> one is better, in fact, we tried to move all the cros_ec_cmd_xfer to the _status
>>> version in the past because makes the code and error handling cleaner. So I'm
>>> reticent to get back to use cros_ec_cmd_xfer instead of cros_ec_cmd_xfer_status.
>>>
>>> * The users of the cros-ec protocol sometimes they mallocing/freeing at runtime,
>>> and sometimes they don't. IMHO *non* mallocing/freeing is usually better, more
>>> efficient and faster. Would be nice to standardize this.
>>
>> I think we should look at latency (I am assuming that is one of the
>> concerns Gwendal was referring to).
>> We should certainly do more rigorous measurements, but I did a crude
>> measurement across a devm_kzalloc() used on one of the EC commands
>> inside platform/chrome for struct EC command:
>> - Used ktime_get_ns() to record time before and after the devm_kzalloc()
>> - Used ktime_sub to subtract the "after" and "before" values:
>>
>>         struct cros_ec_command *msg;
>>         int ret;
>> +       ktime_t start, end, diff;
>>
>> +       start = ktime_get_ns();
>>         msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
>> +       end = ktime_get_ns();
>>         if (!msg)
>>                 return -ENOMEM;
>>
>> +       diff = ktime_sub(end, start);
>> +       printk("%s(): TEST: kzalloc took: %lld\n", __func__, ktime_to_ns(diff));
>>
>> On an i5 1.6 GHz system, across 16 call measurements I got the
>> following latency values (in ns):
>> - Count, N:16
>> - Average: 72.375
>> - Std. Dev : 28.768
>> - Max: 143
>> - Min:  51
>>
>> Are these values significant for the various call-sites? I think the
>> driver authors might be able to comment better there (unfortunately I
>> don't have enough context for each case).
>> Of course there will be other overhead (memcpy) but I think this is a
>> good starting point for the discussion.
>> (My apologies if this measurement method is incorrect/inaccurate.)
> 
> Any thoughts / comments here?
> 
> On an overall note, I think keeping cros_ec_cmd_xfer() and cros_ec_cmd()
> might be a good starting point.
> 
> In this way, we are not introducing any extra function. Also, we can
> begin converting the cros_ec_cmd_xfer() use cases (a few call-sites may
> need to be investigated from a latency perspective). The
> cros_ec_cmd_xfer() conversions are better handled in separate patch
> series.
> 
> Best regards,
> 
> -Prashant
>>
>>>
>>> * If we want to introduce a new 'cros_ec_cmd', this should make the code cleaner
>>> and ideally should be the way we tell the users they should use to communicate
>>> with the cros-ec and not open coding constantly. Ideally, should be a
>>> replacement of all current 'cros_ec_cmd_xfer*' versions.
>>
>> As I mentioned previously, I think all calls of cros_ec_cmd_xfer() can
>> be converted to use cros_ec_cmd() (especially since the new API has a
>> *result pointer),
>> but I think it should be staged out a bit more (since cases like iio:
>> cros_ec driver require non-trivial refactoring which I think is better
>> in a patch/series).
>>
>>>
>>> * If 'cros_ec_cmd' *cannot* replace all the cases, it should be clear to the
>>> user in which cases he should use this function and in which cases shouldn't use
>>> this function.
>>
>> This seems like a good compromise, but my expectation is that if there
>> is a "fast" and "slow" version of the same functionality, developers
>> would be inclined to use the "fast" version always?
>>
>>
>>> * Finally, what pointed Gwendal, what's the best approach to send commands to
>>> the EC by default, is better use dynamic memory? or is better use the stack? is
>>> it always safe use the stack? is always efficient use allocated memory?
>>>
>>> As you can see I have a lot of questions still around, but taking in
>>> consideration that this will be an important change I think that makes sense
>>> spend some time discussing it.
>>>
>>> What do you think?
>>>
>>> Enric
>>>
>>>
>>>> Gwendal.
>>>>>
>>>>> I think it is doable. From looking at the code I felt the factors we
>>>>> need to be careful about are:
>>>>> - The function cros_ec_motion_send_host_cmd() is called from a few
>>>>> other files, each of which set up the struct cros_ec_command
>>>>> differently (reference:
>>>>> https://elixir.bootlin.com/linux/latest/ident/cros_ec_motion_send_host_cmd)
>>>>> - It is not clear to me how readability will be affected by making the
>>>>> change to cros_ec_cmd().
>>>>>
>>>>> Due to the above two factors, but primarily because I wanted to avoid
>>>>> making such an involved large change in this 17 patch series, I
>>>>> reasoned it would be better to make the transition to cros_ec_cmd()
>>>>> for these files in a separate patch/series.
>>>>> My plan after this patch series is to work on this driver(perhaps we
>>>>> can eliminate cros_ec_motion_send_host_cmd() itself?), and then remove
>>>>> cros_ec_cmd_xfer() usage.
>>>>>
>>>>> WDYT?
>>>>>
>>>>> Best regards,
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>>      if (ret &&
>>>>>>>>          state->resp != (struct ec_response_motion_sense *)state->msg->data)
>>>>>>>