On Fri, Feb 22, 2019 at 07:56:28PM +0000, Alex_Gagniuc@xxxxxxxxxxxx wrote:
> On 2/21/19 1:36 AM, Lukas Wunner wrote:
> > On Tue, Feb 19, 2019 at 07:20:28PM -0600, Alexandru Gagniuc wrote:
> >> mutex_lock(&ctrl->state_lock);
> >> + present = pciehp_card_present(ctrl);
> >> + link_active = pciehp_check_link_active(ctrl);
> >> switch (ctrl->state) {
> >
> > These two assignments appear to be superfluous as you're also performing
> > them in pciehp_check_link_active().
>
> Not sure. Between the first check, and this check, you can have several
> seconds elapse depending on whether the driver's .probe()/remove() is
> invoked. Whatever you got at the beginning would be stale. If you had a
> picture dictionary and looked up 'bad idea', it would have a picture of
> the above code with the second check removed.
I don't quite follow. You're no longer using the "present" and
"link_active" variables in pciehp_handle_presence_or_link_change(),
the variables are set again further down in the function and you're
*also* reading PDS and DLLLA in is_delayed_presence_up_event().
So the above-quoted assignments are superfluous. Am I missing something?
(Sorry, I had copy-pasted the wrong function name, I meant
is_delayed_presence_up_event() above, not pciehp_check_link_active().
> I've got all the other review comments addressed in my local branch. I'm
> waiting on Lord Helgass' decision on which solution is better.
^^^^^^^^^^^^
Can we keep this discussion in a neutral tone please?
Thanks,
Lukas
