linux-kernel - Re: [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence comes up after link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sun, 24 Feb 2019 22:27:53 +0000
From:   <Alex_Gagniuc@...lteam.com>
To:     <lukas@...ner.de>
Cc:     <mr.nuke.me@...il.com>, <bhelgaas@...gle.com>,
        <Austin.Bolen@...l.com>, <keith.busch@...el.com>,
        <Shyam.Iyer@...l.com>, <okaya@...nel.org>,
        <linux-pci@...r.kernel.org>, <gustavo@...eddedor.com>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence
 comes up after link

On 2/23/19 12:50 AM, Lukas Wunner wrote:
> 
> [EXTERNAL EMAIL]
> 
> On Fri, Feb 22, 2019 at 07:56:28PM +0000, Alex_Gagniuc@...lteam.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