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

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

[Rongguang Wei]

From: Rongguang Wei <weirongguang@kylinos.cn>

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. So the slot which status in BLINKINGON_STATE is also
turn off unconditionally.

The message print like this after the patch:
    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): Card not present
    pcieport 0000:00:01.5: pciehp: Slot(0-5): Already disabled
    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): Card not present
    pcieport 0000:00:01.5: pciehp: Slot(0-5): Already disabled
    pcieport 0000:00:01.5: pciehp: Slot(0-5): Card present
    pcieport 0000:00:01.5: pciehp: Slot(0-5): Link Up

After that, the next Attention Button Pressed event would power on
the slot normally.

Fixes: d331710ea78f ("PCI: pciehp: Become resilient to missed events")
Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
---
 drivers/pci/hotplug/pciehp_ctrl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 529c34808440..86fc9342be68 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -232,6 +232,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 	 */
 	mutex_lock(&ctrl->state_lock);
 	switch (ctrl->state) {
+	case BLINKINGON_STATE:
 	case BLINKINGOFF_STATE:
 		cancel_delayed_work(&ctrl->button_work);
 		fallthrough;
@@ -261,9 +262,6 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 	}
 
 	switch (ctrl->state) {
-	case BLINKINGON_STATE:
-		cancel_delayed_work(&ctrl->button_work);
-		fallthrough;
 	case OFF_STATE:
 		ctrl->state = POWERON_STATE;
 		mutex_unlock(&ctrl->state_lock);
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

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

[Rongguang Wei]

RESEND

On 4/3/23 1:46 PM, Rongguang Wei wrote:
> From: Rongguang Wei <weirongguang@kylinos.cn>
> 
> 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. So the slot which status in BLINKINGON_STATE is also
> turn off unconditionally.
> 
> The message print like this after the patch:
>     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): Card not present
>     pcieport 0000:00:01.5: pciehp: Slot(0-5): Already disabled
>     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): Card not present
>     pcieport 0000:00:01.5: pciehp: Slot(0-5): Already disabled
>     pcieport 0000:00:01.5: pciehp: Slot(0-5): Card present
>     pcieport 0000:00:01.5: pciehp: Slot(0-5): Link Up
> 
> After that, the next Attention Button Pressed event would power on
> the slot normally.
> 
> Fixes: d331710ea78f ("PCI: pciehp: Become resilient to missed events")
> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
> ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 529c34808440..86fc9342be68 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -232,6 +232,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  	 */
>  	mutex_lock(&ctrl->state_lock);
>  	switch (ctrl->state) {
> +	case BLINKINGON_STATE:
>  	case BLINKINGOFF_STATE:
>  		cancel_delayed_work(&ctrl->button_work);
>  		fallthrough;
> @@ -261,9 +262,6 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  	}
>  
>  	switch (ctrl->state) {
> -	case BLINKINGON_STATE:
> -		cancel_delayed_work(&ctrl->button_work);
> -		fallthrough;
>  	case OFF_STATE:
>  		ctrl->state = POWERON_STATE;
>  		mutex_unlock(&ctrl->state_lock);
> 

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

[Bjorn Helgaas]

On Mon, Apr 03, 2023 at 01:46:19PM +0800, Rongguang Wei wrote:
> From: Rongguang Wei <weirongguang@kylinos.cn>
> 
> 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. So the slot which status in BLINKINGON_STATE is also
> turn off unconditionally.
> 
> The message print like this after the patch:
>     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): Card not present
>     pcieport 0000:00:01.5: pciehp: Slot(0-5): Already disabled
>     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): Card not present
>     pcieport 0000:00:01.5: pciehp: Slot(0-5): Already disabled
>     pcieport 0000:00:01.5: pciehp: Slot(0-5): Card present
>     pcieport 0000:00:01.5: pciehp: Slot(0-5): Link Up
> 
> After that, the next Attention Button Pressed event would power on
> the slot normally.

Lukas, any comment?

> Fixes: d331710ea78f ("PCI: pciehp: Become resilient to missed events")
> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>
> ---
>  drivers/pci/hotplug/pciehp_ctrl.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 529c34808440..86fc9342be68 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -232,6 +232,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  	 */
>  	mutex_lock(&ctrl->state_lock);
>  	switch (ctrl->state) {
> +	case BLINKINGON_STATE:
>  	case BLINKINGOFF_STATE:
>  		cancel_delayed_work(&ctrl->button_work);
>  		fallthrough;
> @@ -261,9 +262,6 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  	}
>  
>  	switch (ctrl->state) {
> -	case BLINKINGON_STATE:
> -		cancel_delayed_work(&ctrl->button_work);
> -		fallthrough;
>  	case OFF_STATE:
>  		ctrl->state = POWERON_STATE;
>  		mutex_unlock(&ctrl->state_lock);
> -- 
> 2.25.1
> 
> 
> No virus found
> 		Checked by Hillstone Network AntiVirus
> 

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

[Lukas Wunner]

On Mon, Apr 03, 2023 at 01:46:19PM +0800, Rongguang Wei wrote:
> 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.

I see what you mean.

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

Upon a button press, pciehp_queue_pushbutton_work() is scheduled to run
after 5 seconds.  It synthesizes a Presence Detect Changed event,
whereupon pciehp_handle_presence_or_link_change() runs.

Should the slot be empty, pciehp_handle_presence_or_link_change() just
bails out and the state incorrectly remains in BLINKINGON_STATE.


> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -232,6 +232,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>  	 */
>  	mutex_lock(&ctrl->state_lock);
>  	switch (ctrl->state) {
> +	case BLINKINGON_STATE:
>  	case BLINKINGOFF_STATE:
>  		cancel_delayed_work(&ctrl->button_work);
>  		fallthrough;

This solution has the disadvantage that a gratuitous "Card not present"
message is emitted even if the slot is occupied.

I'd prefer the following simpler solution:

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 529c348..e680444 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 = POWEROFF_STATE;
 		mutex_unlock(&ctrl->state_lock);
 		return;
 	}

Optionally the assignment can be made conditional on
"if (ctrl->state == BLINKINGON_STATE)" for clarity.

Likewise, a "Card not present" message can optionally be emitted here.

Thanks,

Lukas

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

[Rongguang Wei]

Hi,

On 4/16/23 11:18 PM, Lukas Wunner wrote:
> On Mon, Apr 03, 2023 at 01:46:19PM +0800, Rongguang Wei wrote:
>> 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.
> 
> I see what you mean.
> 
> pciehp's behavior is incorrect if the Attention Button is pressed
> on an unoccupied slot:
> 
> Upon a button press, pciehp_queue_pushbutton_work() is scheduled to run
> after 5 seconds.  It synthesizes a Presence Detect Changed event,
> whereupon pciehp_handle_presence_or_link_change() runs.
> 
> Should the slot be empty, pciehp_handle_presence_or_link_change() just
> bails out and the state incorrectly remains in BLINKINGON_STATE.
> 
> 
>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>> @@ -232,6 +232,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>>  	 */
>>  	mutex_lock(&ctrl->state_lock);
>>  	switch (ctrl->state) {
>> +	case BLINKINGON_STATE:
>>  	case BLINKINGOFF_STATE:
>>  		cancel_delayed_work(&ctrl->button_work);
>>  		fallthrough;
> 
> This solution has the disadvantage that a gratuitous "Card not present"
> message is emitted even if the slot is occupied.
> 
Thank you for your advice.

I think when the "Card not present" is emitted, it may not consider the slot status
from the beginning. If the slot is in ON_STATE and is occupied, turn the slot off and then back on. The message is also emitted at first.

> I'd prefer the following simpler solution:
> 
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 529c348..e680444 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 = POWEROFF_STATE;
>  		mutex_unlock(&ctrl->state_lock);
>  		return;
>  	}
>> Optionally the assignment can be made conditional on
> "if (ctrl->state == BLINKINGON_STATE)" for clarity.
> 
> Likewise, a "Card not present" message can optionally be emitted here.
It should set crtl->state = OFF_STATE in direct and add cancel_delayed_work(&ctrl->button_work). And add message here looks a bit redundancy.
> 
> Thanks,
> 
> Lukas
> 
Maybe I can rework to add like this to prevent the gratuitous message:

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 86fc9342be68..8dbf767a65ac 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -239,12 +239,6 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 	case ON_STATE:
 		ctrl->state = POWEROFF_STATE;
 		mutex_unlock(&ctrl->state_lock);
-		if (events & PCI_EXP_SLTSTA_DLLSC)
-			ctrl_info(ctrl, "Slot(%s): Link Down\n",
-				  slot_name(ctrl));
-		if (events & PCI_EXP_SLTSTA_PDC)
-			ctrl_info(ctrl, "Slot(%s): Card not present\n",
-				  slot_name(ctrl));
 		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
 		break;
 	default:
@@ -257,6 +251,12 @@ 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) {
 		mutex_unlock(&ctrl->state_lock);
+		if (events & PCI_EXP_SLTSTA_DLLSC)
+			ctrl_info(ctrl, "Slot(%s): Link Down\n",
+				  slot_name(ctrl));
+		if (events & PCI_EXP_SLTSTA_PDC)
+			ctrl_info(ctrl, "Slot(%s): Card not present\n",
+				  slot_name(ctrl));
 		return;
 	}

Thanks,

Wei

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

[Lukas Wunner]

On Mon, Apr 17, 2023 at 11:04:31AM +0800, Rongguang Wei wrote:
> On 4/16/23 11:18 PM, Lukas Wunner wrote:
> > On Mon, Apr 03, 2023 at 01:46:19PM +0800, Rongguang Wei wrote:
> >> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> >> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> >> @@ -232,6 +232,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> >>  	 */
> >>  	mutex_lock(&ctrl->state_lock);
> >>  	switch (ctrl->state) {
> >> +	case BLINKINGON_STATE:
> >>  	case BLINKINGOFF_STATE:
> >>  		cancel_delayed_work(&ctrl->button_work);
> >>  		fallthrough;
> > 
> > This solution has the disadvantage that a gratuitous "Card not present"
> > message is emitted even if the slot is occupied.
> 
> I think when the "Card not present" is emitted, it may not consider the
> slot status from the beginning.

I don't quite follow.  With the change you're proposing, if the Attention
Button has been pressed and there's a card in the slot, after five seconds
you'll emit an erroneous "Card not present" message.  Erroneous because
there's a card in the slot.


> If the slot is in ON_STATE and is occupied, turn the slot off and then
> back on. The message is also emitted at first.

That's intentional.  If the slot is occupied and a Presence Detect Changed
event was received, it means the card in the slot may be a different one.
So the "Card not present" message relates to the card that was
*previously* in the slot.

If the slot is still (or again) occupied, we'll then try to bring it up
and that will lead to a subsequent "Card present" message.


> Maybe I can rework to add like this to prevent the gratuitous message:

Could you just test if the 1-line change I suggested in my previous e-mail
fixes the issue for you?

Thanks,

Lukas

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

[Rongguang Wei]

Hi

On 4/17/23 3:11 PM, Lukas Wunner wrote:
> On Mon, Apr 17, 2023 at 11:04:31AM +0800, Rongguang Wei wrote:
>> On 4/16/23 11:18 PM, Lukas Wunner wrote:
>>> On Mon, Apr 03, 2023 at 01:46:19PM +0800, Rongguang Wei wrote:
>>>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>>>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>>>> @@ -232,6 +232,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>>>>  	 */
>>>>  	mutex_lock(&ctrl->state_lock);
>>>>  	switch (ctrl->state) {
>>>> +	case BLINKINGON_STATE:
>>>>  	case BLINKINGOFF_STATE:
>>>>  		cancel_delayed_work(&ctrl->button_work);
>>>>  		fallthrough;
>>>
>>> This solution has the disadvantage that a gratuitous "Card not present"
>>> message is emitted even if the slot is occupied.
>>
>> I think when the "Card not present" is emitted, it may not consider the
>> slot status from the beginning.
> 
> I don't quite follow.  With the change you're proposing, if the Attention
> Button has been pressed and there's a card in the slot, after five seconds
> you'll emit an erroneous "Card not present" message.  Erroneous because
> there's a card in the slot.
> 
> 
>> If the slot is in ON_STATE and is occupied, turn the slot off and then
>> back on. The message is also emitted at first.
> 
> That's intentional.  If the slot is occupied and a Presence Detect Changed
> event was received, it means the card in the slot may be a different one.
> So the "Card not present" message relates to the card that was
> *previously* in the slot.
> 
> If the slot is still (or again) occupied, we'll then try to bring it up
> and that will lead to a subsequent "Card present" message.
> 
> 
>> Maybe I can rework to add like this to prevent the gratuitous message:
> 
> Could you just test if the 1-line change I suggested in my previous e-mail
> fixes the issue for you?
> 
> Thanks,
> 
> Lukas
>
I test the 1-line change and it make the test failed. The dmesg like this:

pcieport 0000:00:01.5: pciehp: Slot(0-5): Attention button pressed
pcieport 0000:00:01.5: pciehp: Slot(0-5): Powering off 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): Ignoring invalid state 0x4

all ABP event are print "Ignoring invalid state 0x4". 

I was add 1 line to disable slot and it works. This looks like what was done
before.

diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 529c34808440..462a61fc7313 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -256,7 +256,9 @@ 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 = POWEROFF_STATE;
 		mutex_unlock(&ctrl->state_lock);
+		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
 		return;
 	}

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

[Lukas Wunner]

On Wed, Apr 19, 2023 at 10:58:33AM +0800, Rongguang Wei wrote:
> I test the 1-line change and it make the test failed. The dmesg like this:
> 
> pcieport 0000:00:01.5: pciehp: Slot(0-5): Attention button pressed
> pcieport 0000:00:01.5: pciehp: Slot(0-5): Powering off 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): Ignoring invalid state 0x4
[...]
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 529c34808440..462a61fc7313 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -256,7 +256,9 @@ 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 = POWEROFF_STATE;
>  		mutex_unlock(&ctrl->state_lock);

My apologies, I made a mistake.  I meant OFF_STATE, not POWEROFF_STATE.
Could you try that, without the addition below:

> +		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
>  		return;
>  	}

Sorry again and thanks,

Lukas

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

[Rongguang Wei]

Hi,

On 4/19/23 3:48 PM, Lukas Wunner wrote:
> On Wed, Apr 19, 2023 at 10:58:33AM +0800, Rongguang Wei wrote:
>> I test the 1-line change and it make the test failed. The dmesg like this:
>>
>> pcieport 0000:00:01.5: pciehp: Slot(0-5): Attention button pressed
>> pcieport 0000:00:01.5: pciehp: Slot(0-5): Powering off 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): Ignoring invalid state 0x4
> [...]
>> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
>> index 529c34808440..462a61fc7313 100644
>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>> @@ -256,7 +256,9 @@ 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 = POWEROFF_STATE;
>>  		mutex_unlock(&ctrl->state_lock);
> 
> My apologies, I made a mistake.  I meant OFF_STATE, not POWEROFF_STATE.
> Could you try that, without the addition below:
> 
>> +		pciehp_disable_slot(ctrl, SURPRISE_REMOVAL);
>>  		return;
>>  	}
> 
> Sorry again and thanks,
> 
> Lukas
> 

It test good. I will rework the patch and send V2. 
Thank you for your help. :)