| Message ID | 20230403054619.19163-1-clementwei90@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v1] PCI: pciehp: Fix the slot in BLINKINGON_STATE when Presence Detect Changed event occurred | expand |
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); >
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 >
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
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
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
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; }
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
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. :)
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);