lkml.org 
[lkml]   [2019]   [Feb]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence comes up after link
    Date
    On 2/23/19 12:50 AM, Lukas Wunner wrote:
    >
    > [EXTERNAL EMAIL]
    >
    > On Fri, Feb 22, 2019 at 07:56:28PM +0000, Alex_Gagniuc@Dellteam.com 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 see what I did. You're right. I should remove the following lines from
    the patch. I'll have that fixed when I re-submit this.

    + present = pciehp_card_present(ctrl);
    + link_active = pciehp_check_link_active(ctrl);

    >
    >> 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?

    I'm sorry. I thought comparing linux to feudalism would be hillarious,
    but I now see not everyone agrees. Sorry, Bjorn.

    Alex

    \
     
     \ /
      Last update: 2019-02-24 23:29    [W:4.779 / U:0.308 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site