| Message ID | 20170224065435.14954-1-yinghai@kernel.org |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Yinghai On Thu, Feb 23, 2017 at 10:54:35PM -0800, Yinghai Lu wrote: > +++ linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c > @@ -71,9 +71,6 @@ static void set_slot_off(struct controll > */ > msleep(1000); > } > - > - pciehp_green_led_off(pslot); > - pciehp_set_attention_status(pslot, 1); Re using set_slot_off() in remove_board() make sense.. but i'm not sure why these are pulled out? It seems to be functionally complete when these are done in set_slot_off(). > } > > /** > @@ -126,6 +123,8 @@ static int board_added(struct slot *p_sl > > err_exit: > set_slot_off(ctrl, p_slot); > + pciehp_green_led_off(p_slot); > + pciehp_set_attention_status(p_slot, 1); > return retval; > } > > @@ -142,16 +141,7 @@ static int remove_board(struct slot *p_s > if (retval) > return retval; > > - if (POWER_CTRL(ctrl)) { > - pciehp_power_off_slot(p_slot); > - > - /* > - * After turning power off, we must wait for at least 1 second > - * before taking any action that relies on power having been > - * removed from the slot/adapter. > - */ > - msleep(1000); > - } > + set_slot_off(ctrl, p_slot); > > /* turn off Green LED */ > pciehp_green_led_off(p_slot); Don't we need the pciehp_set_attention_status() here? Cheers, Ashok
On Fri, Feb 24, 2017 at 9:17 AM, Raj, Ashok <ashok.raj@intel.com> wrote: > On Thu, Feb 23, 2017 at 10:54:35PM -0800, Yinghai Lu wrote: >> +++ linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c >> @@ -71,9 +71,6 @@ static void set_slot_off(struct controll >> */ >> msleep(1000); >> } >> - >> - pciehp_green_led_off(pslot); >> - pciehp_set_attention_status(pslot, 1); > > Re using set_slot_off() in remove_board() make sense.. but i'm not sure > why these are pulled out? It seems to be functionally complete > when these are done in set_slot_off(). Just don't want to the led operation is wrapped too deep inside. In board_added(), has led operation code directly for success path. so put led operation code directly for error path make them more symmetrically. > >> } >> >> /** >> @@ -126,6 +123,8 @@ static int board_added(struct slot *p_sl >> >> err_exit: >> set_slot_off(ctrl, p_slot); >> + pciehp_green_led_off(p_slot); >> + pciehp_set_attention_status(p_slot, 1); >> return retval; >> } >> >> @@ -142,16 +141,7 @@ static int remove_board(struct slot *p_s >> if (retval) >> return retval; >> >> - if (POWER_CTRL(ctrl)) { >> - pciehp_power_off_slot(p_slot); >> - >> - /* >> - * After turning power off, we must wait for at least 1 second >> - * before taking any action that relies on power having been >> - * removed from the slot/adapter. >> - */ >> - msleep(1000); >> - } >> + set_slot_off(ctrl, p_slot); >> >> /* turn off Green LED */ >> pciehp_green_led_off(p_slot); > Don't we need the pciehp_set_attention_status() here? that attention led could be on if previous power on is not done successfully Thanks Yinghai.
Index: linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c =================================================================== --- linux-2.6.orig/drivers/pci/hotplug/pciehp_ctrl.c +++ linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c @@ -71,9 +71,6 @@ static void set_slot_off(struct controll */ msleep(1000); } - - pciehp_green_led_off(pslot); - pciehp_set_attention_status(pslot, 1); } /** @@ -126,6 +123,8 @@ static int board_added(struct slot *p_sl err_exit: set_slot_off(ctrl, p_slot); + pciehp_green_led_off(p_slot); + pciehp_set_attention_status(p_slot, 1); return retval; } @@ -142,16 +141,7 @@ static int remove_board(struct slot *p_s if (retval) return retval; - if (POWER_CTRL(ctrl)) { - pciehp_power_off_slot(p_slot); - - /* - * After turning power off, we must wait for at least 1 second - * before taking any action that relies on power having been - * removed from the slot/adapter. - */ - msleep(1000); - } + set_slot_off(ctrl, p_slot); /* turn off Green LED */ pciehp_green_led_off(p_slot);
Now set_slot_off() is used in board_added() err path. We could reuse in remove_board. Also need to move green_led and attention_status out it. and make code more readable. Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- drivers/pci/hotplug/pciehp_ctrl.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)