diff mbox series

[PATCHv2] PCI: pciehp: Use appropriate conditions to check the hotplug controller status

Message ID 20240528064200.87762-1-yaoma@linux.alibaba.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series [PATCHv2] PCI: pciehp: Use appropriate conditions to check the hotplug controller status | expand

Commit Message

Bitao Hu May 28, 2024, 6:42 a.m. UTC 
"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 <yaoma@linux.alibaba.com>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
---
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(-)

Comments

Ilpo Järvinen May 28, 2024, 10:54 a.m. UTC | #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 <yaoma@linux.alibaba.com>
> Reviewed-by: Lukas Wunner <lukas@wunner.de>

Thanks for updaring the description.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Bjorn Helgaas June 14, 2024, 6:41 p.m. UTC | #2 
[+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 <yaoma@linux.alibaba.com>
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
> ---
> 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)
>
Lukas Wunner June 14, 2024, 7:36 p.m. UTC | #3 
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
Bjorn Helgaas June 14, 2024, 10:03 p.m. UTC | #4 
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
Lukas Wunner June 15, 2024, 10:06 a.m. UTC | #5 
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
diff mbox series

Patch

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);