diff mbox series

PCI: pciehp: Simplify Attention Button logging

Message ID 20230522214051.619337-1-helgaas@kernel.org
State New
Headers show
Series PCI: pciehp: Simplify Attention Button logging | expand

Commit Message

Bjorn Helgaas May 22, 2023, 9:40 p.m. UTC 
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(-)

Comments

Bjorn Helgaas May 22, 2023, 9:53 p.m. UTC | #1 
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
>
Lukas Wunner May 23, 2023, 5:29 a.m. UTC | #2 
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
Bjorn Helgaas May 23, 2023, 3:40 p.m. UTC | #3 
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;
Lukas Wunner May 24, 2023, 10:08 a.m. UTC | #4 
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;
Bjorn Helgaas May 24, 2023, 4:54 p.m. UTC | #5 
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!
diff mbox series

Patch

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) {