Re: [PATCH] PCI: pciehp: Simplify Attention Button logging

[Bjorn Helgaas <helgaas@kernel.org>]

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;