Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Cc:     <mr.nuke.me@gmail.com>, <bhelgaas@google.com>,
        <Austin.Bolen@dell.com>, <keith.busch@intel.com>,
        <Shyam.Iyer@dell.com>, <okaya@kernel.org>,
        <linux-pci@vger.kernel.org>, <gustavo@embeddedor.com>,
        <linux-kernel@vger.kernel.org>
From:   <Alex_Gagniuc@Dellteam.com>
To:     <lukas@wunner.de>
Subject: Re: [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence
 comes up after link
Thread-Topic: [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence
 comes up after link
Thread-Index: AQHUyLqR+ed3yG3ljUu42ub7Hwypsg==
Date:   Sun, 24 Feb 2019 22:27:53 +0000
Message-ID: <b54916c90e044d93a06a3f0d297ee30f@ausx13mps321.AMER.DELL.COM>
References: <20190220012031.10741-1-mr.nuke.me@gmail.com>
 <20190220012031.10741-3-mr.nuke.me@gmail.com>
 <20190221073634.snvazxlzgvjhilaz@wunner.de>
 <dfeadc6798984cce965e2202e90fe0b0@ausx13mps321.AMER.DELL.COM>
 <20190223064926.mh24zzlj4xykwcjf@wunner.de>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2/23/19 12:50 AM, Lukas Wunner wrote:=0A=
> =0A=
> [EXTERNAL EMAIL]=0A=
> =0A=
> On Fri, Feb 22, 2019 at 07:56:28PM +0000, Alex_Gagniuc@Dellteam.com wrote=
:=0A=
>> On 2/21/19 1:36 AM, Lukas Wunner wrote:=0A=
>>> On Tue, Feb 19, 2019 at 07:20:28PM -0600, Alexandru Gagniuc wrote:=0A=
>>>>    	mutex_lock(&ctrl->state_lock);=0A=
>>>> +	present =3D pciehp_card_present(ctrl);=0A=
>>>> +	link_active =3D pciehp_check_link_active(ctrl);=0A=
>>>>    	switch (ctrl->state) {=0A=
>>>=0A=
>>> These two assignments appear to be superfluous as you're also performin=
g=0A=
>>> them in pciehp_check_link_active().=0A=
>>=0A=
>> Not sure. Between the first check, and this check, you can have several=
=0A=
>> seconds elapse depending on whether the driver's .probe()/remove() is=0A=
>> invoked. Whatever you got at the beginning would be stale. If you had a=
=0A=
>> picture dictionary and looked up 'bad idea', it would have a picture of=
=0A=
>> the above code with the second check removed.=0A=
> =0A=
> I don't quite follow.  You're no longer using the "present" and=0A=
> "link_active" variables in pciehp_handle_presence_or_link_change(),=0A=
> the variables are set again further down in the function and you're=0A=
> *also* reading PDS and DLLLA in is_delayed_presence_up_event().=0A=
> So the above-quoted assignments are superfluous.  Am I missing something?=
=0A=
> =0A=
> (Sorry, I had copy-pasted the wrong function name, I meant=0A=
> is_delayed_presence_up_event() above, not pciehp_check_link_active().=0A=
=0A=
=0A=
I see what I did. You're right. I should remove the following lines from =
=0A=
the patch. I'll have that fixed when I re-submit this.=0A=
=0A=
+       present =3D pciehp_card_present(ctrl);=0A=
+       link_active =3D pciehp_check_link_active(ctrl);=0A=
=0A=
> =0A=
>> I've got all the other review comments addressed in my local branch. I'm=
=0A=
>> waiting on Lord Helgass' decision on which solution is better.=0A=
>               ^^^^^^^^^^^^=0A=
> =0A=
> Can we keep this discussion in a neutral tone please?=0A=
=0A=
I'm sorry. I thought comparing linux to feudalism would be hillarious, =0A=
but I now see not everyone agrees. Sorry, Bjorn.=0A=
=0A=
Alex=0A=