From: <Alex_Gagniuc@Dellteam.com>
To: <lukas@wunner.de>
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>
Subject: Re: [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence comes up after link
Date: Sun, 24 Feb 2019 22:27:53 +0000 [thread overview]
Message-ID: <b54916c90e044d93a06a3f0d297ee30f@ausx13mps321.AMER.DELL.COM> (raw)
In-Reply-To: 20190223064926.mh24zzlj4xykwcjf@wunner.de
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
next prev parent reply other threads:[~2019-02-24 22:28 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-20 1:20 [PATCH RFC v2 0/4] PCI: pciehp: Do not turn off slot if presence comes up after link Alexandru Gagniuc
2019-02-20 1:20 ` [PATCH RFC v2 1/4] PCI: hotplug: Add support for disabling in-band presence Alexandru Gagniuc
2019-02-21 7:19 ` Lukas Wunner
2019-02-21 18:05 ` Alex_Gagniuc
2019-02-20 1:20 ` [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence comes up after link Alexandru Gagniuc
2019-02-21 7:36 ` Lukas Wunner
2019-02-22 19:56 ` Alex_Gagniuc
2019-02-23 6:49 ` Lukas Wunner
2019-02-24 22:27 ` Alex_Gagniuc [this message]
2019-02-20 1:20 ` [PATCH RFC v2 3/4] PCI: hotplug: Wait for PDS when in-band presence is disabled Alexandru Gagniuc
2019-02-20 1:20 ` [PATCH RFC v2 4/4] PCI: hotplug: Add quirk For Dell nvme pcie switches Alexandru Gagniuc
2019-02-21 7:56 ` Lukas Wunner
2019-02-21 18:35 ` Alex_Gagniuc
2019-02-22 1:20 ` Joe Perches
2019-02-22 2:04 ` Oliver
2019-02-22 19:19 ` Alex_Gagniuc
2019-02-21 15:38 ` [PATCH RFC v2 0/4] PCI: pciehp: Do not turn off slot if presence comes up after link Lukas Wunner
2019-02-21 18:17 ` Alex_Gagniuc
2019-04-19 0:01 ` Bjorn Helgaas
2019-04-19 15:22 ` [PATCH v3 " Alexandru Gagniuc
2019-04-19 15:22 ` [PATCH v3 1/4] PCI: hotplug: Add support for disabling in-band presence Alexandru Gagniuc
2019-04-19 15:22 ` [PATCH v3 2/4] PCI: pciehp: Do not turn off slot if presence comes up after link Alexandru Gagniuc
2019-04-19 15:22 ` [PATCH v3 3/4] PCI: hotplug: Wait for PDS when in-band presence is disabled Alexandru Gagniuc
2019-04-19 15:22 ` [PATCH v3 4/4] PCI: hotplug: Add quirk For Dell nvme pcie switches Alexandru Gagniuc
2019-04-19 21:00 ` Alan J. Wylie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b54916c90e044d93a06a3f0d297ee30f@ausx13mps321.AMER.DELL.COM \
--to=alex_gagniuc@dellteam.com \
--cc=Austin.Bolen@dell.com \
--cc=Shyam.Iyer@dell.com \
--cc=bhelgaas@google.com \
--cc=gustavo@embeddedor.com \
--cc=keith.busch@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=mr.nuke.me@gmail.com \
--cc=okaya@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).