* [PATCH] PCI: pciehp: Simplify Attention Button logging
@ 2023-05-22 21:40 Bjorn Helgaas
2023-05-22 21:53 ` Bjorn Helgaas
2023-05-23 5:29 ` Lukas Wunner
0 siblings, 2 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2023-05-22 21:40 UTC (permalink / raw)
To: Rongguang Wei, Rongguang Wei, Lukas Wunner; +Cc: linux-pci, Bjorn Helgaas
From: Bjorn Helgaas <bhelgaas@google.com>
Previously, pressing the Attention Button always logged two lines, the
first from pciehp_ist() and the second from pciehp_handle_button_press():
Attention button pressed
Powering on due to button press
Since pciehp_handle_button_press() always logs the more detailed message,
remove the generic "Attention button pressed" message. Reword the
pciehp_handle_button_press() to be of the form:
Button press: powering on
Button press: powering off
Button press: canceling poweron
Button press: canceling poweroff
Button press: ignoring invalid state %#x
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/hotplug/pciehp_ctrl.c | 20 +++++++++-----------
drivers/pci/hotplug/pciehp_hpc.c | 5 +----
2 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 32baba1b7f13..ff725104bf2b 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -164,15 +164,13 @@ void pciehp_handle_button_press(struct controller *ctrl)
switch (ctrl->state) {
case OFF_STATE:
case ON_STATE:
- if (ctrl->state == ON_STATE) {
+ if (ctrl->state == ON_STATE)
ctrl->state = BLINKINGOFF_STATE;
- ctrl_info(ctrl, "Slot(%s): Powering off due to button press\n",
- slot_name(ctrl));
- } else {
+ else
ctrl->state = BLINKINGON_STATE;
- ctrl_info(ctrl, "Slot(%s) Powering on due to button press\n",
- slot_name(ctrl));
- }
+ ctrl_info(ctrl, "Slot(%s): Button press: powering %s\n",
+ slot_name(ctrl),
+ ctrl->state == BLINKINGON_STATE ? "on" : "off");
/* blink power indicator and turn off attention */
pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
PCI_EXP_SLTCTL_ATTN_IND_OFF);
@@ -185,7 +183,6 @@ void pciehp_handle_button_press(struct controller *ctrl)
* press the attention again before the 5 sec. limit
* expires to cancel hot-add or hot-remove
*/
- ctrl_info(ctrl, "Slot(%s): Button cancel\n", slot_name(ctrl));
cancel_delayed_work(&ctrl->button_work);
if (ctrl->state == BLINKINGOFF_STATE) {
ctrl->state = ON_STATE;
@@ -196,11 +193,12 @@ void pciehp_handle_button_press(struct controller *ctrl)
pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
PCI_EXP_SLTCTL_ATTN_IND_OFF);
}
- ctrl_info(ctrl, "Slot(%s): Action canceled due to button press\n",
- slot_name(ctrl));
+ ctrl_info(ctrl, "Slot(%s): Button press: canceling power%s\n",
+ slot_name(ctrl),
+ ctrl->state == ON_STATE ? "off" : "on");
break;
default:
- ctrl_err(ctrl, "Slot(%s): Ignoring invalid state %#x\n",
+ ctrl_err(ctrl, "Slot(%s): Button press: ignoring invalid state %#x\n",
slot_name(ctrl), ctrl->state);
break;
}
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index f8c70115b691..379d2af5c51d 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -722,11 +722,8 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
}
/* Check Attention Button Pressed */
- if (events & PCI_EXP_SLTSTA_ABP) {
- ctrl_info(ctrl, "Slot(%s): Attention button pressed\n",
- slot_name(ctrl));
+ if (events & PCI_EXP_SLTSTA_ABP)
pciehp_handle_button_press(ctrl);
- }
/* Check Power Fault Detected */
if (events & PCI_EXP_SLTSTA_PFD) {
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pciehp: Simplify Attention Button logging
2023-05-22 21:40 [PATCH] PCI: pciehp: Simplify Attention Button logging Bjorn Helgaas
@ 2023-05-22 21:53 ` Bjorn Helgaas
2023-05-23 5:29 ` Lukas Wunner
1 sibling, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2023-05-22 21:53 UTC (permalink / raw)
To: Rongguang Wei, Rongguang Wei, Lukas Wunner; +Cc: linux-pci, Bjorn Helgaas
On Mon, May 22, 2023 at 04:40:51PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Previously, pressing the Attention Button always logged two lines, the
> first from pciehp_ist() and the second from pciehp_handle_button_press():
>
> Attention button pressed
> Powering on due to button press
>
> Since pciehp_handle_button_press() always logs the more detailed message,
> remove the generic "Attention button pressed" message. Reword the
> pciehp_handle_button_press() to be of the form:
>
> Button press: powering on
> Button press: powering off
> Button press: canceling poweron
> Button press: canceling poweroff
> Button press: ignoring invalid state %#x
If nobody objects to this patch, I'd likely reorder it before your
patch, Rongguang, and update that commit log to mention the new
messages.
> ---
> drivers/pci/hotplug/pciehp_ctrl.c | 20 +++++++++-----------
> drivers/pci/hotplug/pciehp_hpc.c | 5 +----
> 2 files changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 32baba1b7f13..ff725104bf2b 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -164,15 +164,13 @@ void pciehp_handle_button_press(struct controller *ctrl)
> switch (ctrl->state) {
> case OFF_STATE:
> case ON_STATE:
> - if (ctrl->state == ON_STATE) {
> + if (ctrl->state == ON_STATE)
> ctrl->state = BLINKINGOFF_STATE;
> - ctrl_info(ctrl, "Slot(%s): Powering off due to button press\n",
> - slot_name(ctrl));
> - } else {
> + else
> ctrl->state = BLINKINGON_STATE;
> - ctrl_info(ctrl, "Slot(%s) Powering on due to button press\n",
> - slot_name(ctrl));
> - }
> + ctrl_info(ctrl, "Slot(%s): Button press: powering %s\n",
> + slot_name(ctrl),
> + ctrl->state == BLINKINGON_STATE ? "on" : "off");
> /* blink power indicator and turn off attention */
> pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_BLINK,
> PCI_EXP_SLTCTL_ATTN_IND_OFF);
> @@ -185,7 +183,6 @@ void pciehp_handle_button_press(struct controller *ctrl)
> * press the attention again before the 5 sec. limit
> * expires to cancel hot-add or hot-remove
> */
> - ctrl_info(ctrl, "Slot(%s): Button cancel\n", slot_name(ctrl));
> cancel_delayed_work(&ctrl->button_work);
> if (ctrl->state == BLINKINGOFF_STATE) {
> ctrl->state = ON_STATE;
> @@ -196,11 +193,12 @@ void pciehp_handle_button_press(struct controller *ctrl)
> pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> PCI_EXP_SLTCTL_ATTN_IND_OFF);
> }
> - ctrl_info(ctrl, "Slot(%s): Action canceled due to button press\n",
> - slot_name(ctrl));
> + ctrl_info(ctrl, "Slot(%s): Button press: canceling power%s\n",
> + slot_name(ctrl),
> + ctrl->state == ON_STATE ? "off" : "on");
> break;
> default:
> - ctrl_err(ctrl, "Slot(%s): Ignoring invalid state %#x\n",
> + ctrl_err(ctrl, "Slot(%s): Button press: ignoring invalid state %#x\n",
> slot_name(ctrl), ctrl->state);
> break;
> }
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index f8c70115b691..379d2af5c51d 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -722,11 +722,8 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
> }
>
> /* Check Attention Button Pressed */
> - if (events & PCI_EXP_SLTSTA_ABP) {
> - ctrl_info(ctrl, "Slot(%s): Attention button pressed\n",
> - slot_name(ctrl));
> + if (events & PCI_EXP_SLTSTA_ABP)
> pciehp_handle_button_press(ctrl);
> - }
>
> /* Check Power Fault Detected */
> if (events & PCI_EXP_SLTSTA_PFD) {
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pciehp: Simplify Attention Button logging
2023-05-22 21:40 [PATCH] PCI: pciehp: Simplify Attention Button logging Bjorn Helgaas
2023-05-22 21:53 ` Bjorn Helgaas
@ 2023-05-23 5:29 ` Lukas Wunner
2023-05-23 15:40 ` Bjorn Helgaas
1 sibling, 1 reply; 6+ messages in thread
From: Lukas Wunner @ 2023-05-23 5:29 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Rongguang Wei, Rongguang Wei, linux-pci, Bjorn Helgaas
On Mon, May 22, 2023 at 04:40:51PM -0500, Bjorn Helgaas wrote:
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -164,15 +164,13 @@ void pciehp_handle_button_press(struct controller *ctrl)
> switch (ctrl->state) {
> case OFF_STATE:
> case ON_STATE:
> - if (ctrl->state == ON_STATE) {
> + if (ctrl->state == ON_STATE)
> ctrl->state = BLINKINGOFF_STATE;
> - ctrl_info(ctrl, "Slot(%s): Powering off due to button press\n",
> - slot_name(ctrl));
> - } else {
> + else
> ctrl->state = BLINKINGON_STATE;
> - ctrl_info(ctrl, "Slot(%s) Powering on due to button press\n",
> - slot_name(ctrl));
> - }
> + ctrl_info(ctrl, "Slot(%s): Button press: powering %s\n",
> + slot_name(ctrl),
> + ctrl->state == BLINKINGON_STATE ? "on" : "off");
This results in double checks of ctrl->state (because the ctrl_info()
is pulled out of the if/else statement), so is slightly less
efficient than before. Not a huge issue, but noting nonetheless.
I think the "powering on" (and "powering off") message is (and
has always been) confusing because it's present participle, yet
the powering on / off occurs in the future, hence "powering on in 5 sec"
or "will power on" or "power on pending" would probably be more
more correct.
Thanks,
Lukas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pciehp: Simplify Attention Button logging
2023-05-23 5:29 ` Lukas Wunner
@ 2023-05-23 15:40 ` Bjorn Helgaas
2023-05-24 10:08 ` Lukas Wunner
0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2023-05-23 15:40 UTC (permalink / raw)
To: Lukas Wunner; +Cc: Rongguang Wei, Rongguang Wei, linux-pci, Bjorn Helgaas
On Tue, May 23, 2023 at 07:29:57AM +0200, Lukas Wunner wrote:
> On Mon, May 22, 2023 at 04:40:51PM -0500, Bjorn Helgaas wrote:
> > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> > @@ -164,15 +164,13 @@ void pciehp_handle_button_press(struct controller *ctrl)
> > switch (ctrl->state) {
> > case OFF_STATE:
> > case ON_STATE:
> > - if (ctrl->state == ON_STATE) {
> > + if (ctrl->state == ON_STATE)
> > ctrl->state = BLINKINGOFF_STATE;
> > - ctrl_info(ctrl, "Slot(%s): Powering off due to button press\n",
> > - slot_name(ctrl));
> > - } else {
> > + else
> > ctrl->state = BLINKINGON_STATE;
> > - ctrl_info(ctrl, "Slot(%s) Powering on due to button press\n",
> > - slot_name(ctrl));
> > - }
> > + ctrl_info(ctrl, "Slot(%s): Button press: powering %s\n",
> > + slot_name(ctrl),
> > + ctrl->state == BLINKINGON_STATE ? "on" : "off");
>
> This results in double checks of ctrl->state (because the ctrl_info()
> is pulled out of the if/else statement), so is slightly less
> efficient than before. Not a huge issue, but noting nonetheless.
>
> I think the "powering on" (and "powering off") message is (and
> has always been) confusing because it's present participle, yet
> the powering on / off occurs in the future, hence "powering on in 5 sec"
> or "will power on" or "power on pending" would probably be more
> more correct.
Absolutely right on both counts; thank you very much!
I was trying to make it OFF/ON case parallel to the BLINKING case that
only has one ctrl_info(), but I think that makes it a little harder to
read in addition to being less efficient.
And the language is definitely confusing. How about this?
@@ -166,11 +166,11 @@ void pciehp_handle_button_press(struct controller *ctrl)
case ON_STATE:
if (ctrl->state == ON_STATE) {
ctrl->state = BLINKINGOFF_STATE;
- ctrl_info(ctrl, "Slot(%s): Powering off due to button press\n",
+ ctrl_info(ctrl, "Slot(%s): Button press: will power off in 5 sec\n",
slot_name(ctrl));
} else {
ctrl->state = BLINKINGON_STATE;
- ctrl_info(ctrl, "Slot(%s) Powering on due to button press\n",
+ ctrl_info(ctrl, "Slot(%s): Button press: will power on in 5 sec\n",
slot_name(ctrl));
}
/* blink power indicator and turn off attention */
@@ -185,22 +185,23 @@ void pciehp_handle_button_press(struct controller *ctrl)
* press the attention again before the 5 sec. limit
* expires to cancel hot-add or hot-remove
*/
- ctrl_info(ctrl, "Slot(%s): Button cancel\n", slot_name(ctrl));
cancel_delayed_work(&ctrl->button_work);
if (ctrl->state == BLINKINGOFF_STATE) {
ctrl->state = ON_STATE;
pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_ON,
PCI_EXP_SLTCTL_ATTN_IND_OFF);
+ ctrl_info(ctrl, "Slot(%s): Button press: canceling request to power off\n",
+ slot_name(ctrl));
} else {
ctrl->state = OFF_STATE;
pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
PCI_EXP_SLTCTL_ATTN_IND_OFF);
+ ctrl_info(ctrl, "Slot(%s): Button press: canceling request to power on\n",
+ slot_name(ctrl));
}
- ctrl_info(ctrl, "Slot(%s): Action canceled due to button press\n",
- slot_name(ctrl));
break;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pciehp: Simplify Attention Button logging
2023-05-23 15:40 ` Bjorn Helgaas
@ 2023-05-24 10:08 ` Lukas Wunner
2023-05-24 16:54 ` Bjorn Helgaas
0 siblings, 1 reply; 6+ messages in thread
From: Lukas Wunner @ 2023-05-24 10:08 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Rongguang Wei, Rongguang Wei, linux-pci, Bjorn Helgaas
On Tue, May 23, 2023 at 10:40:39AM -0500, Bjorn Helgaas wrote:
> I was trying to make it OFF/ON case parallel to the BLINKING case that
> only has one ctrl_info(), but I think that makes it a little harder to
> read in addition to being less efficient.
>
> And the language is definitely confusing. How about this?
Perfect, feel free to add my
Reviewed-by: Lukas Wunner <lukas@wunner.de>
to commit 6d433b9ddfda on pci/hotplug.
Thanks a lot!
Lukas
> @@ -166,11 +166,11 @@ void pciehp_handle_button_press(struct controller *ctrl)
> case ON_STATE:
> if (ctrl->state == ON_STATE) {
> ctrl->state = BLINKINGOFF_STATE;
> - ctrl_info(ctrl, "Slot(%s): Powering off due to button press\n",
> + ctrl_info(ctrl, "Slot(%s): Button press: will power off in 5 sec\n",
> slot_name(ctrl));
> } else {
> ctrl->state = BLINKINGON_STATE;
> - ctrl_info(ctrl, "Slot(%s) Powering on due to button press\n",
> + ctrl_info(ctrl, "Slot(%s): Button press: will power on in 5 sec\n",
> slot_name(ctrl));
> }
> /* blink power indicator and turn off attention */
> @@ -185,22 +185,23 @@ void pciehp_handle_button_press(struct controller *ctrl)
> * press the attention again before the 5 sec. limit
> * expires to cancel hot-add or hot-remove
> */
> - ctrl_info(ctrl, "Slot(%s): Button cancel\n", slot_name(ctrl));
> cancel_delayed_work(&ctrl->button_work);
> if (ctrl->state == BLINKINGOFF_STATE) {
> ctrl->state = ON_STATE;
> pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_ON,
> PCI_EXP_SLTCTL_ATTN_IND_OFF);
> + ctrl_info(ctrl, "Slot(%s): Button press: canceling request to power off\n",
> + slot_name(ctrl));
> } else {
> ctrl->state = OFF_STATE;
> pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> PCI_EXP_SLTCTL_ATTN_IND_OFF);
> + ctrl_info(ctrl, "Slot(%s): Button press: canceling request to power on\n",
> + slot_name(ctrl));
> }
> - ctrl_info(ctrl, "Slot(%s): Action canceled due to button press\n",
> - slot_name(ctrl));
> break;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pciehp: Simplify Attention Button logging
2023-05-24 10:08 ` Lukas Wunner
@ 2023-05-24 16:54 ` Bjorn Helgaas
0 siblings, 0 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2023-05-24 16:54 UTC (permalink / raw)
To: Lukas Wunner; +Cc: Rongguang Wei, Rongguang Wei, linux-pci, Bjorn Helgaas
On Wed, May 24, 2023 at 12:08:23PM +0200, Lukas Wunner wrote:
> On Tue, May 23, 2023 at 10:40:39AM -0500, Bjorn Helgaas wrote:
> > I was trying to make it OFF/ON case parallel to the BLINKING case that
> > only has one ctrl_info(), but I think that makes it a little harder to
> > read in addition to being less efficient.
> >
> > And the language is definitely confusing. How about this?
>
> Perfect, feel free to add my
>
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
>
> to commit 6d433b9ddfda on pci/hotplug.
Thanks, added!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-05-24 16:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-22 21:40 [PATCH] PCI: pciehp: Simplify Attention Button logging Bjorn Helgaas
2023-05-22 21:53 ` Bjorn Helgaas
2023-05-23 5:29 ` Lukas Wunner
2023-05-23 15:40 ` Bjorn Helgaas
2023-05-24 10:08 ` Lukas Wunner
2023-05-24 16:54 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox