Re: [PATCH v3] PCI: pciehp: Fix the slot in BLINKINGON_STATE when Presence Detect Changed event occurred

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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. 




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux