Re: [PATCH 2/2] drm/msm/hdmi: add hdmi hdcp support

[Bjorn Andersson]

On Wed, Dec 3, 2014 at 9:16 AM,  <jilaiw@xxxxxxxxxxxxxx> wrote:
[..]
>>>> +       enum hdmi_hdcp_state hdcp_state;
>>>> +       struct mutex state_mutex;
>>>> +       struct delayed_work hdcp_reauth_work;
>>>> +       struct delayed_work hdcp_auth_part1_1_work;
>>>> +       struct delayed_work hdcp_auth_part1_2_work;
>>>> +       struct work_struct hdcp_auth_part1_3_work;
>>>> +       struct delayed_work hdcp_auth_part2_1_work;
>>>> +       struct delayed_work hdcp_auth_part2_2_work;
>>>> +       struct delayed_work hdcp_auth_part2_3_work;
>>>> +       struct delayed_work hdcp_auth_part2_4_work;
>>>> +       struct work_struct hdcp_auth_prepare_work;
>>>
>>> You shouldn't use "work" as a way to express states in your state
>>> machine.
>>> Better have 1 auth work function that does all these steps, probably
>>> having
>>> them split in functions just like you do now.
>>>
>>> That way you can have 1 function running the pass of authentication,
>>> starting
>>> by checking if you're reauthing or not then processing each step one by
>>> one,
>>> sleeping inbetween them. You can have the functions return -EAGAIN to
>>> indicate
>>> that you need to retry the current operation and so on.
>>>
>>> This would split the state machine from the state executioners and
>>> simplify
>>> your code.
>>
>> As a side note (disclaimer, I'm not an hdcp expert), but I wonder if
>> eventually some of that should be extracted out into some helpers in
>> drm core.  I guess that is something we'll figure out when a 2nd
>> driver gains upstream HDCP support.  One big work w/ msleep()'s does
>> sound like it would be easier to understand (and therefore easier to
>> refactor out into helpers).
>>
>> [snip]
>>
>
> The reason that I break the partI/PartII work into these small works
> because I want to avoid to use msleep in work.
> Otherwise cancel a HDCP work may cause long delay up to several seconds.
>

I definitely think the steps are nice size and make things easy to understand.

If you make the steps that require a retry return out to the main
state handling function with a -EAGAIN, then you can have check if you
should retry or abort based on cancellation. Giving you the worst case
cancellation time of the longest sleep...

As Rob suggest you could use wait_event_timeout() or something to
improve this, but on the other hand the worst case here seems to be
the HZ/2 after prepare_auth; others are HZ/8, HZ/20 and HZ/50; not
"seconds".

So I would start with a simple msleep() for implementation simplicity
and then enhance that in a follow up commit (if needed)...

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/