On Wed, Dec 3, 2014 at 9:16 AM, jilaiw@codeaurora.org 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