diff mbox series

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

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

Commit Message

Bitao Hu May 24, 2024, 6:30 a.m. UTC
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 <yaoma@linux.alibaba.com>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Lukas Wunner May 24, 2024, 7:53 a.m. UTC | #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
Bitao Hu May 26, 2024, 2:45 p.m. UTC | #2
Hi,

> 2024年5月24日 15:53,Lukas Wunner <lukas@wunner.de> 写道:
> 
> 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
Lukas Wunner May 27, 2024, 8:50 a.m. UTC | #3
On Sun, May 26, 2024 at 10:45:36PM +0800, yaoma wrote:
> > 2024 5 24 15:53 Lukas Wunner <lukas@wunner.de>
> > 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 <lukas@wunner.de>

...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
Bitao Hu May 27, 2024, 9:43 a.m. UTC | #4
Hi,

> 2024年5月27日 16:50,Lukas Wunner <lukas@wunner.de> 写道:
> 
> On Sun, May 26, 2024 at 10:45:36PM +0800, yaoma wrote:
>>> 2024 5 24 15:53 Lukas Wunner <lukas@wunner.de>
>>> 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 <lukas@wunner.de>
> 
> ...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
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);