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;
