On 5/11/23 10:43 PM, Lukas Wunner wrote:
> On Fri, May 05, 2023 at 09:38:59AM +0800, Rongguang Wei wrote:
>> On 4/29/23 10:30 PM, Lukas Wunner wrote:
>>> On Fri, Apr 21, 2023 at 10:56:41AM +0800, Rongguang Wei wrote:
>>>> --- a/drivers/pci/hotplug/pciehp_ctrl.c
>>>> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
>>>> @@ -256,6 +256,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
>>>> present = pciehp_card_present(ctrl);
>>>> link_active = pciehp_check_link_active(ctrl);
>>>> if (present <= 0 && link_active <= 0) {
>>>> + ctrl->state = OFF_STATE;
>>>> mutex_unlock(&ctrl->state_lock);
>>>> return;
>>>> }
>>>
>>> It has occurred to me that we also need to turn off the Power Indicator,
>>> lest it continues blinking after the 5 second interval. Sorry for not
>>> spotting this earlier. And I'm thinking that making the state change
>>> conditional on BLINKINGON_STATE would serve clarity.
>>>
>>> So could you please amend the above as follows and verify it fixes the
>>> issue for you?
>>
>> It works fine.
>>
>>> if (present <= 0 && link_active <= 0) {
>>> + if (ctrl->state == BLINKINGON_STATE) {
>>
>> I have a little question and is there necessary to add
>> "cancel_delayed_work(&ctrl->button_work);" here?
>
> Right, that seems reasonable. pciehp_queue_pushbutton_work() becomes
> a no-op once the state has been changed to OFF_STATE, so there's no
> real harm if the work item is left in the queue, but canceling it is
> definitely cleaner.
>
> And a message to the user might be helpful as well to indicate that
> the button press hasn't resulted in slot bringup due to it being
> empty. So maybe something like the below?
>
>
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 529c34808440..32baba1b7f13 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -256,6 +256,14 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
> present = pciehp_card_present(ctrl);
> link_active = pciehp_check_link_active(ctrl);
> if (present <= 0 && link_active <= 0) {
> + if (ctrl->state == BLINKINGON_STATE) {
> + ctrl->state = OFF_STATE;
> + cancel_delayed_work(&ctrl->button_work);
> + pciehp_set_indicators(ctrl, PCI_EXP_SLTCTL_PWR_IND_OFF,
> + INDICATOR_NOOP);
> + ctrl_info(ctrl, "Slot(%s): Card not present\n",
> + slot_name(ctrl));
> + }
> mutex_unlock(&ctrl->state_lock);
> return;
> }
>
Yep! It looks more focused on this issue and I will send the new version patch base on this.
Thanks.
