The values of 'present' and 'link_active' have similar meanings:
the value is %1 if the status is ready, and %0 if it is not. If the
hotplug controller itself is not available, the value should be
%-ENODEV. However, both %1 and %-ENODEV are considered true, which
obviously does not meet expectations. 'Slot(xx): Card present' and
'Slot(xx): Link Up' should only be output when the value is %1.
Signed-off-by: Bitao Hu <[email protected]>
---
drivers/pci/hotplug/pciehp_ctrl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index dcdbfcf404dd..6adfdbb70150 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -276,10 +276,10 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
case OFF_STATE:
ctrl->state = POWERON_STATE;
mutex_unlock(&ctrl->state_lock);
- if (present)
+ if (present > 0)
ctrl_info(ctrl, "Slot(%s): Card present\n",
slot_name(ctrl));
- if (link_active)
+ if (link_active > 0)
ctrl_info(ctrl, "Slot(%s): Link Up\n",
slot_name(ctrl));
ctrl->request_result = pciehp_enable_slot(ctrl);
--
2.37.1 (Apple Git-137.1)
On Fri, May 24, 2024 at 02:30:23PM +0800, Bitao Hu wrote:
> The values of 'present' and 'link_active' have similar meanings:
> the value is %1 if the status is ready, and %0 if it is not. If the
> hotplug controller itself is not available, the value should be
> %-ENODEV. However, both %1 and %-ENODEV are considered true, which
> obviously does not meet expectations. 'Slot(xx): Card present' and
> 'Slot(xx): Link Up' should only be output when the value is %1.
[...]
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -276,10 +276,10 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> case OFF_STATE:
> ctrl->state = POWERON_STATE;
> mutex_unlock(&ctrl->state_lock);
> - if (present)
> + if (present > 0)
> ctrl_info(ctrl, "Slot(%s): Card present\n",
> slot_name(ctrl));
> - if (link_active)
> + if (link_active > 0)
> ctrl_info(ctrl, "Slot(%s): Link Up\n",
> slot_name(ctrl));
> ctrl->request_result = pciehp_enable_slot(ctrl);
We already handle the "<= 0" case immediately above this code excerpt:
if (present <= 0 && link_active <= 0) {
...
}
So neither "present" nor "link_active" can be < 0 at this point.
Hence I don't quite understand what motivates the proposed code change?
Thanks,
Lukas
Hi,
> 2024年5月24日 15:53，Lukas Wunner <[email protected]> 写道：
>
> On Fri, May 24, 2024 at 02:30:23PM +0800, Bitao Hu wrote:
>> The values of 'present' and 'link_active' have similar meanings:
>> the value is %1 if the status is ready, and %0 if it is not. If the
>> hotplug controller itself is not available, the value should be
>> %-ENODEV. However, both %1 and %-ENODEV are considered true, which
>> obviously does not meet expectations. 'Slot(xx): Card present' and
>> 'Slot(xx): Link Up' should only be output when the value is %1.
> [...]
>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>> @@ -276,10 +276,10 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>> case OFF_STATE:
>> ctrl->state = POWERON_STATE;
>> mutex_unlock(&ctrl->state_lock);
>> - if (present)
>> + if (present > 0)
>> ctrl_info(ctrl, "Slot(%s): Card present\n",
>> slot_name(ctrl));
>> - if (link_active)
>> + if (link_active > 0)
>> ctrl_info(ctrl, "Slot(%s): Link Up\n",
>> slot_name(ctrl));
>> ctrl->request_result = pciehp_enable_slot(ctrl);
>
> We already handle the "<= 0" case immediately above this code excerpt:
>
> if (present <= 0 && link_active <= 0) {
> ...
> }
I'm not sure if the following scenarios would occur in actual production environment,
but from the code level, there is the possibility of “present <= 0 && link_active > 0”
or “present > 0 && link_active <= 0”. In these cases, the “<= 0” conditions will
not be properly handled, and “ctrl_info” will output incorrect prompt messages.
> So neither "present" nor "link_active" can be < 0 at this point.
>
Best Regards,
Bitao Hu
On Sun, May 26, 2024 at 10:45:36PM +0800, yaoma wrote:
> > 2024 5 24 15:53 Lukas Wunner <[email protected]>
> > On Fri, May 24, 2024 at 02:30:23PM +0800, Bitao Hu wrote:
> > > The values of 'present' and 'link_active' have similar meanings:
> > > the value is %1 if the status is ready, and %0 if it is not. If the
> > > hotplug controller itself is not available, the value should be
> > > %-ENODEV. However, both %1 and %-ENODEV are considered true, which
> > > obviously does not meet expectations. 'Slot(xx): Card present' and
> > > 'Slot(xx): Link Up' should only be output when the value is %1.
> > [...]
> > > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> > > @@ -276,10 +276,10 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> > > case OFF_STATE:
> > > ctrl->state = POWERON_STATE;
> > > mutex_unlock(&ctrl->state_lock);
> > > - if (present)
> > > + if (present > 0)
> > > ctrl_info(ctrl, "Slot(%s): Card present\n",
> > > slot_name(ctrl));
> > > - if (link_active)
> > > + if (link_active > 0)
> > > ctrl_info(ctrl, "Slot(%s): Link Up\n",
> > > slot_name(ctrl));
> > > ctrl->request_result = pciehp_enable_slot(ctrl);
> >
> > We already handle the "<= 0" case immediately above this code excerpt:
> >
> > if (present <= 0 && link_active <= 0) {
> > ...
> > }
>
> I'm not sure if the following scenarios would occur in actual production
> environment, but from the code level, there is the possibility of
> "present <= 0 && link_active > 0" or "present > 0 && link_active <= 0".
> In these cases, the "<= 0" conditions will not be properly handled,
> and "ctrl_info" will output incorrect prompt messages.
I see, that makes sense.
"present" and "link_active" can be -ENODEV if reading the config space
of the hotplug port failed. That's typically the case if the hotplug
port itself was hot-removed, which happens all the time with
Thunderbolt/USB4.
E.g. pciehp_card_present() may return 1 and pciehp_check_link_active()
may return -ENODEV because the hotplug port was hot-removed in-between
the two function calls. In that case we'll emit both "Card present"
*and* "Link Up". The latter is uncalled for and is supressed by your
patch.
So your code change is
Reviewed-by: Lukas Wunner <[email protected]>
..but it would be good if you could respin the patch and explain the
rationale of the code change in the commit message more clearly.
Basically summarize what you and I have explained above.
Also, the percent sign % in front of 0, 1, -ENODEV is unnecessary in
commit messages. It only has special meaning in kernel-doc.
Thanks,
Lukas
Hi,
> 2024年5月27日 16:50，Lukas Wunner <[email protected]> 写道：
>
> On Sun, May 26, 2024 at 10:45:36PM +0800, yaoma wrote:
>>> 2024 5 24 15:53 Lukas Wunner <[email protected]>
>>> On Fri, May 24, 2024 at 02:30:23PM +0800, Bitao Hu wrote:
>>>> The values of 'present' and 'link_active' have similar meanings:
>>>> the value is %1 if the status is ready, and %0 if it is not. If the
>>>> hotplug controller itself is not available, the value should be
>>>> %-ENODEV. However, both %1 and %-ENODEV are considered true, which
>>>> obviously does not meet expectations. 'Slot(xx): Card present' and
>>>> 'Slot(xx): Link Up' should only be output when the value is %1.
>>> [...]
>>>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>>>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>>>> @@ -276,10 +276,10 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>>>> case OFF_STATE:
>>>> ctrl->state = POWERON_STATE;
>>>> mutex_unlock(&ctrl->state_lock);
>>>> - if (present)
>>>> + if (present > 0)
>>>> ctrl_info(ctrl, "Slot(%s): Card present\n",
>>>> slot_name(ctrl));
>>>> - if (link_active)
>>>> + if (link_active > 0)
>>>> ctrl_info(ctrl, "Slot(%s): Link Up\n",
>>>> slot_name(ctrl));
>>>> ctrl->request_result = pciehp_enable_slot(ctrl);
>>>
>>> We already handle the "<= 0" case immediately above this code excerpt:
>>>
>>> if (present <= 0 && link_active <= 0) {
>>> ...
>>> }
>>
>> I'm not sure if the following scenarios would occur in actual production
>> environment, but from the code level, there is the possibility of
>> "present <= 0 && link_active > 0" or "present > 0 && link_active <= 0".
>> In these cases, the "<= 0" conditions will not be properly handled,
>> and "ctrl_info" will output incorrect prompt messages.
>
> I see, that makes sense.
>
> "present" and "link_active" can be -ENODEV if reading the config space
> of the hotplug port failed. That's typically the case if the hotplug
> port itself was hot-removed, which happens all the time with
> Thunderbolt/USB4.
>
> E.g. pciehp_card_present() may return 1 and pciehp_check_link_active()
> may return -ENODEV because the hotplug port was hot-removed in-between
> the two function calls. In that case we'll emit both "Card present"
> *and* "Link Up". The latter is uncalled for and is supressed by your
> patch.
>
> So your code change is
> Reviewed-by: Lukas Wunner <[email protected]>
>
> ...but it would be good if you could respin the patch and explain the
> rationale of the code change in the commit message more clearly.
> Basically summarize what you and I have explained above.
>
> Also, the percent sign % in front of 0, 1, -ENODEV is unnecessary in
> commit messages. It only has special meaning in kernel-doc.
>
Thanks for your analysis. I will make the the rationale of the code change
more clearly in next patch.
Best Regards,
Bitao Hu
"present" and "link_active" can be 1 if the status is ready, and 0 if
it is not. Both of them can be -ENODEV if reading the config space
of the hotplug port failed. That's typically the case if the hotplug
port itself was hot-removed. Therefore, this situation can occur:
pciehp_card_present() may return 1 and pciehp_check_link_active()
may return -ENODEV because the hotplug port was hot-removed in-between
the two function calls. In that case we'll emit both "Card present"
*and* "Link Up" since both 1 and -ENODEV are considered "true". This
is not the expected behavior. Those messages should be emited when
"present" and "link_active" are positive.
Signed-off-by: Bitao Hu <[email protected]>
Reviewed-by: Lukas Wunner <[email protected]>
---
v1 -> v2:
1. Explain the rationale of the code change in the commit message
more clearly.
2. Add the "Reviewed-by" tag of Lukas.
---
drivers/pci/hotplug/pciehp_ctrl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index dcdbfcf404dd..6adfdbb70150 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -276,10 +276,10 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
case OFF_STATE:
ctrl->state = POWERON_STATE;
mutex_unlock(&ctrl->state_lock);
- if (present)
+ if (present > 0)
ctrl_info(ctrl, "Slot(%s): Card present\n",
slot_name(ctrl));
- if (link_active)
+ if (link_active > 0)
ctrl_info(ctrl, "Slot(%s): Link Up\n",
slot_name(ctrl));
ctrl->request_result = pciehp_enable_slot(ctrl);
--
2.37.1 (Apple Git-137.1)
On Tue, 28 May 2024, Bitao Hu wrote:
> "present" and "link_active" can be 1 if the status is ready, and 0 if
> it is not. Both of them can be -ENODEV if reading the config space
> of the hotplug port failed. That's typically the case if the hotplug
> port itself was hot-removed. Therefore, this situation can occur:
> pciehp_card_present() may return 1 and pciehp_check_link_active()
> may return -ENODEV because the hotplug port was hot-removed in-between
> the two function calls. In that case we'll emit both "Card present"
> *and* "Link Up" since both 1 and -ENODEV are considered "true". This
> is not the expected behavior. Those messages should be emited when
> "present" and "link_active" are positive.
>
> Signed-off-by: Bitao Hu <[email protected]>
> Reviewed-by: Lukas Wunner <[email protected]>
Thanks for updaring the description.
Reviewed-by: Ilpo J?rvinen <[email protected]>
--
i.
> ---
> v1 -> v2:
> 1. Explain the rationale of the code change in the commit message
> more clearly.
> 2. Add the "Reviewed-by" tag of Lukas.
> ---
> drivers/pci/hotplug/pciehp_ctrl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index dcdbfcf404dd..6adfdbb70150 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -276,10 +276,10 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> case OFF_STATE:
> ctrl->state = POWERON_STATE;
> mutex_unlock(&ctrl->state_lock);
> - if (present)
> + if (present > 0)
> ctrl_info(ctrl, "Slot(%s): Card present\n",
> slot_name(ctrl));
> - if (link_active)
> + if (link_active > 0)
> ctrl_info(ctrl, "Slot(%s): Link Up\n",
> slot_name(ctrl));
> ctrl->request_result = pciehp_enable_slot(ctrl);
>
[+cc Ilpo]
On Tue, May 28, 2024 at 02:42:00PM +0800, Bitao Hu wrote:
> "present" and "link_active" can be 1 if the status is ready, and 0 if
> it is not. Both of them can be -ENODEV if reading the config space
> of the hotplug port failed. That's typically the case if the hotplug
> port itself was hot-removed. Therefore, this situation can occur:
> pciehp_card_present() may return 1 and pciehp_check_link_active()
> may return -ENODEV because the hotplug port was hot-removed in-between
> the two function calls. In that case we'll emit both "Card present"
> *and* "Link Up" since both 1 and -ENODEV are considered "true". This
> is not the expected behavior. Those messages should be emited when
> "present" and "link_active" are positive.
>
> Signed-off-by: Bitao Hu <[email protected]>
> Reviewed-by: Lukas Wunner <[email protected]>
> ---
> v1 -> v2:
> 1. Explain the rationale of the code change in the commit message
> more clearly.
> 2. Add the "Reviewed-by" tag of Lukas.
> ---
> drivers/pci/hotplug/pciehp_ctrl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index dcdbfcf404dd..6adfdbb70150 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -276,10 +276,10 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> case OFF_STATE:
> ctrl->state = POWERON_STATE;
> mutex_unlock(&ctrl->state_lock);
> - if (present)
> + if (present > 0)
I completely agree that this is a problem and this patch addresses it.
But ...
It seems a little bit weird to me that we even get to this switch
statement if we got -ENODEV from either pciehp_card_present() or
pciehp_check_link_active(). If that happens, a config read failed,
but we're going to go ahead and call pciehp_enable_slot(), which is
going to do a bunch more config accesses, potentially try to power up
the slot, etc.
If a config read failed, it seems like we might want to avoid doing
some of this stuff.
> ctrl_info(ctrl, "Slot(%s): Card present\n",
> slot_name(ctrl));
> - if (link_active)
> + if (link_active > 0)
> ctrl_info(ctrl, "Slot(%s): Link Up\n",
> slot_name(ctrl));
These are cases where we misinterpreted -ENODEV as "device is present"
or "link is active".
pciehp_ignore_dpc_link_change() and pciehp_slot_reset() also call
pciehp_check_link_active(), and I think they also interpret -ENODEV as
"link is active".
Do we need similar changes there?
> ctrl->request_result = pciehp_enable_slot(ctrl);
> --
> 2.37.1 (Apple Git-137.1)
>
On Fri, Jun 14, 2024 at 01:41:20PM -0500, Bjorn Helgaas wrote:
> On Tue, May 28, 2024 at 02:42:00PM +0800, Bitao Hu wrote:
> > "present" and "link_active" can be 1 if the status is ready, and 0 if
> > it is not. Both of them can be -ENODEV if reading the config space
> > of the hotplug port failed. That's typically the case if the hotplug
> > port itself was hot-removed. Therefore, this situation can occur:
> > pciehp_card_present() may return 1 and pciehp_check_link_active()
> > may return -ENODEV because the hotplug port was hot-removed in-between
> > the two function calls. In that case we'll emit both "Card present"
> > *and* "Link Up" since both 1 and -ENODEV are considered "true". This
> > is not the expected behavior. Those messages should be emited when
> > "present" and "link_active" are positive.
[...]
> > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> > @@ -276,10 +276,10 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> > case OFF_STATE:
> > ctrl->state = POWERON_STATE;
> > mutex_unlock(&ctrl->state_lock);
> > - if (present)
> > + if (present > 0)
>
> I completely agree that this is a problem and this patch addresses it.
> But ...
>
> It seems a little bit weird to me that we even get to this switch
> statement if we got -ENODEV from either pciehp_card_present() or
> pciehp_check_link_active(). If that happens, a config read failed,
> but we're going to go ahead and call pciehp_enable_slot(), which is
> going to do a bunch more config accesses, potentially try to power up
> the slot, etc.
>
> If a config read failed, it seems like we might want to avoid doing
> some of this stuff.
Hm, good point. I guess we should change the logical expression instead:
- if (present <= 0 && link_active <= 0) {
+ if (present < 0 || link_active < 0 || (!present && !link_active)) {
> > - if (link_active)
> > + if (link_active > 0)
> > ctrl_info(ctrl, "Slot(%s): Link Up\n",
> > slot_name(ctrl));
>
> These are cases where we misinterpreted -ENODEV as "device is present"
> or "link is active".
>
> pciehp_ignore_dpc_link_change() and pciehp_slot_reset() also call
> pciehp_check_link_active(), and I think they also interpret -ENODEV as
> "link is active".
>
> Do we need similar changes there?
Another good observation, both need to check for <= 0 instead of == 0.
Do you want to fix that yourself or would you prefer me (or someone else)
to submit a patch?
Thanks,
Lukas
On Fri, Jun 14, 2024 at 09:36:57PM +0200, Lukas Wunner wrote:
> On Fri, Jun 14, 2024 at 01:41:20PM -0500, Bjorn Helgaas wrote:
> > On Tue, May 28, 2024 at 02:42:00PM +0800, Bitao Hu wrote:
> > > "present" and "link_active" can be 1 if the status is ready, and 0 if
> > > it is not. Both of them can be -ENODEV if reading the config space
> > > of the hotplug port failed. That's typically the case if the hotplug
> > > port itself was hot-removed. Therefore, this situation can occur:
> > > pciehp_card_present() may return 1 and pciehp_check_link_active()
> > > may return -ENODEV because the hotplug port was hot-removed in-between
> > > the two function calls. In that case we'll emit both "Card present"
> > > *and* "Link Up" since both 1 and -ENODEV are considered "true". This
> > > is not the expected behavior. Those messages should be emited when
> > > "present" and "link_active" are positive.
> [...]
> > > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> > > @@ -276,10 +276,10 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> > > case OFF_STATE:
> > > ctrl->state = POWERON_STATE;
> > > mutex_unlock(&ctrl->state_lock);
> > > - if (present)
> > > + if (present > 0)
> >
> > I completely agree that this is a problem and this patch addresses it.
> > But ...
> >
> > It seems a little bit weird to me that we even get to this switch
> > statement if we got -ENODEV from either pciehp_card_present() or
> > pciehp_check_link_active(). If that happens, a config read failed,
> > but we're going to go ahead and call pciehp_enable_slot(), which is
> > going to do a bunch more config accesses, potentially try to power up
> > the slot, etc.
> >
> > If a config read failed, it seems like we might want to avoid doing
> > some of this stuff.
>
> Hm, good point. I guess we should change the logical expression instead:
>
> - if (present <= 0 && link_active <= 0) {
> + if (present < 0 || link_active < 0 || (!present && !link_active)) {
It gets to be a fairly complicated expression, and I'm not 100% sure
we should handle the config read failure the same as the "!present &&
!link_active" case. The config read failure probably means the
Downstream Port is gone, the other case means the device *below* that
port is gone.
We likely want to cancel the delayed work in both cases, but what
about the indicators? If the Downstream Port is gone, we're not going
to be able to change them. Do we want the same message for both?
Maybe we should handle the config failures separately first? These
error conditions make everything so ugly.
> > > - if (link_active)
> > > + if (link_active > 0)
> > > ctrl_info(ctrl, "Slot(%s): Link Up\n",
> > > slot_name(ctrl));
> >
> > These are cases where we misinterpreted -ENODEV as "device is present"
> > or "link is active".
> >
> > pciehp_ignore_dpc_link_change() and pciehp_slot_reset() also call
> > pciehp_check_link_active(), and I think they also interpret -ENODEV as
> > "link is active".
> >
> > Do we need similar changes there?
>
> Another good observation, both need to check for <= 0 instead of == 0.
> Do you want to fix that yourself or would you prefer me (or someone else)
> to submit a patch?
It'd be great if you or somebody else could do that.
Bjorn
On Fri, Jun 14, 2024 at 05:03:27PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 14, 2024 at 09:36:57PM +0200, Lukas Wunner wrote:
> > Hm, good point. I guess we should change the logical expression instead:
> >
> > - if (present <= 0 && link_active <= 0) {
> > + if (present < 0 || link_active < 0 || (!present && !link_active)) {
>
> It gets to be a fairly complicated expression, and I'm not 100% sure
> we should handle the config read failure the same as the "!present &&
> !link_active" case. The config read failure probably means the
> Downstream Port is gone, the other case means the device *below* that
> port is gone.
>
> We likely want to cancel the delayed work in both cases, but what
> about the indicators? If the Downstream Port is gone, we're not going
> to be able to change them. Do we want the same message for both?
>
> Maybe we should handle the config failures separately first? These
> error conditions make everything so ugly.
To keep the code simple, I'm leaning towards not making the call to
pciehp_set_indicators() conditional. The worst thing that can happen
is that pciehp waits 1 sec for a previous write to the Slot Control
register to time out.
> > > These are cases where we misinterpreted -ENODEV as "device is present"
> > > or "link is active".
> > >
> > > pciehp_ignore_dpc_link_change() and pciehp_slot_reset() also call
> > > pciehp_check_link_active(), and I think they also interpret -ENODEV as
> > > "link is active".
> > >
> > > Do we need similar changes there?
> >
> > Another good observation, both need to check for <= 0 instead of == 0.
> > Do you want to fix that yourself or would you prefer me (or someone else)
> > to submit a patch?
>
> It'd be great if you or somebody else could do that.
After looking at this with a fresh pair of eyeballs, I'm thinking now
that the code is actually fine the way it is:
- pciehp_ignore_dpc_link_change():
If pciehp_check_link_active() returns -ENODEV, it means we recovered
from DPC but immediately afterwards the hotplug port became inaccessible,
perhaps because it was hot-removed or because a DPC event occurred
further up in the hierarchy. In neither case would it be called for
to synthesize a Data Link Layer State Changed event:
If the hotplug port was hot-removed, it's better to let the hotplug port
in its ancestry handle the de-enumeration of its sub-hierarchy and not
interfere with that by trying to concurrently remove a portion of that
sub-hierarchy.
If a DPC event occurred further up, it's better to let the DPC-capable
port in the ancestry handle the recovery and not interfere with that.
- pciehp_slot_reset():
If pciehp_check_link_active() returns -ENODEV, it means a Hot Reset
was propagated down the hierarchy after which the hotplug port is
no longer accessible. Perhaps the hotplug port was hot removed by
the user, in which case we should let the hotplug port in the
ancestry handle de-enumeration. Another possibility is that reset
recovery failed. I don't think we should try to de-enumerate devices
below the hotplug port in that case. Maybe another error occurred
which triggered another reset and things will be fine after we've
recovered from that.
Thanks,
Lukas