[PATCH v3] PCI: pciehp: Fix the slot in BLINKINGON_STATE when Presence Detect Changed event occurred

[PATCH v3] PCI: pciehp: Fix the slot in BLINKINGON_STATE when Presence Detect Changed event occurred

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

pciehp's behavior is incorrect if the Attention Button is pressed
on an unoccupied slot.

When a Presence Detect Changed event has occurred, the slot status
in either BLINKINGOFF_STATE or OFF_STATE, turn it off unconditionally.
But if the slot status is in BLINKINGON_STATE and the slot is currently
empty, the slot status was staying in BLINKINGON_STATE.

The message print like this:
    pcieport 0000:00:01.5: pciehp: Slot(0-5): Attention button pressed
    pcieport 0000:00:01.5: pciehp: Slot(0-5) Powering on due to button press
    pcieport 0000:00:01.5: pciehp: Slot(0-5): Attention button pressed
    pcieport 0000:00:01.5: pciehp: Slot(0-5): Button cancel
    pcieport 0000:00:01.5: pciehp: Slot(0-5): Action canceled due to button press

It cause the next Attention Button Pressed event become Button cancel
and missing the Presence Detect Changed event with this button press
though this button presses event is occurred after 5s.

According to the Commit d331710ea78f ("PCI: pciehp: Become resilient
to missed events"), if the slot is currently occupied, turn it on and
if the slot is empty, it need to set in OFF_STATE rather than stay in
current status when pciehp_handle_presence_or_link_change() bails out.

V2: Update to simple code and avoid gratuitous message.
V3: Add Suggested-by.

Fixes: d331710ea78f ("PCI: pciehp: Become resilient to missed events")
Link: https://lore.kernel.org/linux-pci/20230403054619.19163-1-clementwei90@163.com/
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 529c34808440..1f78e401e9b6 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -256,6 +256,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 	present = pciehp_card_present(ctrl);
 	link_active = pciehp_check_link_active(ctrl);
 	if (present <= 0 && link_active <= 0) {
+		ctrl->state = OFF_STATE;
 		mutex_unlock(&ctrl->state_lock);
 		return;
 	}
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH v3] PCI: pciehp: Fix the slot in BLINKINGON_STATE when Presence Detect Changed event occurred

[Lukas Wunner]

On Fri, Apr 21, 2023 at 10:56:41AM +0800, Rongguang Wei wrote:
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -256,6 +256,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  	present = pciehp_card_present(ctrl);
>  	link_active = pciehp_check_link_active(ctrl);
>  	if (present <= 0 && link_active <= 0) {
> +		ctrl->state = OFF_STATE;
>  		mutex_unlock(&ctrl->state_lock);
>  		return;
>  	}

It has occurred to me that we also need to turn off the Power Indicator,
lest it continues blinking after the 5 second interval.  Sorry for not
spotting this earlier.  And I'm thinking that making the state change
conditional on BLINKINGON_STATE would serve clarity.

So could you please amend the above as follows and verify it fixes the
issue for you?

 	if (present <= 0 && link_active <= 0) {
+		if (ctrl->state == BLINKINGON_STATE) {
+			ctrl->state = OFF_STATE;
+			pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
+					      INDICATOR_NOOP);
+		}
 		mutex_unlock(&ctrl->state_lock);
 		return;
 	}

Thanks!

Lukas

Re: [PATCH v3] PCI: pciehp: Fix the slot in BLINKINGON_STATE when Presence Detect Changed event occurred

[Rongguang Wei]

Hi

On 4/29/23 10:30 PM, Lukas Wunner wrote:
> On Fri, Apr 21, 2023 at 10:56:41AM +0800, Rongguang Wei wrote:
>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>> @@ -256,6 +256,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>>  	present = pciehp_card_present(ctrl);
>>  	link_active = pciehp_check_link_active(ctrl);
>>  	if (present <= 0 && link_active <= 0) {
>> +		ctrl->state = OFF_STATE;
>>  		mutex_unlock(&ctrl->state_lock);
>>  		return;
>>  	}
> 
> It has occurred to me that we also need to turn off the Power Indicator,
> lest it continues blinking after the 5 second interval.  Sorry for not
> spotting this earlier.  And I'm thinking that making the state change
> conditional on BLINKINGON_STATE would serve clarity.
> 
> So could you please amend the above as follows and verify it fixes the
> issue for you?
> 
It works fine.
>  	if (present <= 0 && link_active <= 0) {
> +		if (ctrl->state == BLINKINGON_STATE) {
I have a little question and is there necessary to add "cancel_delayed_work(&ctrl->button_work);" here?
> +			ctrl->state = OFF_STATE;
> +			pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> +					      INDICATOR_NOOP);
> +		}
>  		mutex_unlock(&ctrl->state_lock);
>  		return;
>  	}
> 
> Thanks!
> 
> Lukas
> 
Thanks!

Re: [PATCH v3] PCI: pciehp: Fix the slot in BLINKINGON_STATE when Presence Detect Changed event occurred

[Lukas Wunner]

On Fri, May 05, 2023 at 09:38:59AM +0800, Rongguang Wei wrote:
> On 4/29/23 10:30 PM, Lukas Wunner wrote:
> > On Fri, Apr 21, 2023 at 10:56:41AM +0800, Rongguang Wei wrote:
> > > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> > > @@ -256,6 +256,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> > >  	present = pciehp_card_present(ctrl);
> > >  	link_active = pciehp_check_link_active(ctrl);
> > >  	if (present <= 0 && link_active <= 0) {
> > > +		ctrl->state = OFF_STATE;
> > >  		mutex_unlock(&ctrl->state_lock);
> > >  		return;
> > >  	}
> > 
> > It has occurred to me that we also need to turn off the Power Indicator,
> > lest it continues blinking after the 5 second interval.  Sorry for not
> > spotting this earlier.  And I'm thinking that making the state change
> > conditional on BLINKINGON_STATE would serve clarity.
> > 
> > So could you please amend the above as follows and verify it fixes the
> > issue for you?
> 
> It works fine.
> 
> >  	if (present <= 0 && link_active <= 0) {
> > +		if (ctrl->state == BLINKINGON_STATE) {
> 
> I have a little question and is there necessary to add
> "cancel_delayed_work(&ctrl->button_work);" here?

Right, that seems reasonable.  pciehp_queue_pushbutton_work() becomes
a no-op once the state has been changed to OFF_STATE, so there's no
real harm if the work item is left in the queue, but canceling it is
definitely cleaner.

And a message to the user might be helpful as well to indicate that
the button press hasn't resulted in slot bringup due to it being
empty.  So maybe something like the below?


diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 529c34808440..32baba1b7f13 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -256,6 +256,14 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 	present = pciehp_card_present(ctrl);
 	link_active = pciehp_check_link_active(ctrl);
 	if (present <= 0 && link_active <= 0) {
+		if (ctrl->state == BLINKINGON_STATE) {
+			ctrl->state = OFF_STATE;
+			cancel_delayed_work(&ctrl->button_work);
+			pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
+					      INDICATOR_NOOP);
+			ctrl_info(ctrl, "Slot(%s): Card not present\n",
+				  slot_name(ctrl));
+		}
 		mutex_unlock(&ctrl->state_lock);
 		return;
 	}

Re: [PATCH v3] PCI: pciehp: Fix the slot in BLINKINGON_STATE when Presence Detect Changed event occurred

[Rongguang Wei]

On 5/11/23 10:43 PM, Lukas Wunner wrote:
> On Fri, May 05, 2023 at 09:38:59AM +0800, Rongguang Wei wrote:
>> On 4/29/23 10:30 PM, Lukas Wunner wrote:
>>> On Fri, Apr 21, 2023 at 10:56:41AM +0800, Rongguang Wei wrote:
>>>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>>>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>>>> @@ -256,6 +256,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>>>>  	present = pciehp_card_present(ctrl);
>>>>  	link_active = pciehp_check_link_active(ctrl);
>>>>  	if (present <= 0 && link_active <= 0) {
>>>> +		ctrl->state = OFF_STATE;
>>>>  		mutex_unlock(&ctrl->state_lock);
>>>>  		return;
>>>>  	}
>>>
>>> It has occurred to me that we also need to turn off the Power Indicator,
>>> lest it continues blinking after the 5 second interval.  Sorry for not
>>> spotting this earlier.  And I'm thinking that making the state change
>>> conditional on BLINKINGON_STATE would serve clarity.
>>>
>>> So could you please amend the above as follows and verify it fixes the
>>> issue for you?
>>
>> It works fine.
>>
>>>  	if (present <= 0 && link_active <= 0) {
>>> +		if (ctrl->state == BLINKINGON_STATE) {
>>
>> I have a little question and is there necessary to add
>> "cancel_delayed_work(&ctrl->button_work);" here?
> 
> Right, that seems reasonable.  pciehp_queue_pushbutton_work() becomes
> a no-op once the state has been changed to OFF_STATE, so there's no
> real harm if the work item is left in the queue, but canceling it is
> definitely cleaner.
> 
> And a message to the user might be helpful as well to indicate that
> the button press hasn't resulted in slot bringup due to it being
> empty.  So maybe something like the below?
> 
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 529c34808440..32baba1b7f13 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -256,6 +256,14 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  	present = pciehp_card_present(ctrl);
>  	link_active = pciehp_check_link_active(ctrl);
>  	if (present <= 0 && link_active <= 0) {
> +		if (ctrl->state == BLINKINGON_STATE) {
> +			ctrl->state = OFF_STATE;
> +			cancel_delayed_work(&ctrl->button_work);
> +			pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> +					      INDICATOR_NOOP);
> +			ctrl_info(ctrl, "Slot(%s): Card not present\n",
> +				  slot_name(ctrl));
> +		}
>  		mutex_unlock(&ctrl->state_lock);
>  		return;
>  	}
> 

Yep! It looks more focused on this issue and I will send the new version patch base on this.

Thanks.