> 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 > The worst wait time could be 5 seconds if the downstream device is a REPEATER. The maximum retry time is set to 100 and the delay for each time is HZ/20, then the maximum wait time before abort will be 5 seconds. As you suggested, I can use the flag to notify the retry process to abort earlier in case this work needs to be canceled which can reduce the delay to HZ/20 then. Or as Rob's suggestion to wait for hpd event. Thanks, Jilai _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
