Re: [PATCH] usb: chipidea: core: handle power lost in workqueue

[Peter Chen <peter.chen@kernel.org>]

On 24-01-19 20:35:37, Xu Yang wrote:
> When power is recycled in usb controller during system power management,
> the controller will recognize it and switch role if role has been changed
> during power lost. In current design, it will be completed in resume()
> function. However, this may bring issues since usb class devices have
> their pm operations too and these device's resume() functions are still
> not being called at this point. When usb controller recognized host role
> should be stopped, these usb class devices will be removed at this point.
> But these usb class devices can't be removed in some cases, such as scsi
> devices. Since scsi driver may sync data to U-disk, however it will block
> there because scsi drvier can only handle pm request when is in suspended
> state. Therefore, there may exist a dependency between ci_resume() and usb
> class device's resume(). To break this potential dependency, we need to
> handle power lost work in a workqueue.
> 
> Fixes: 74494b33211d ("usb: chipidea: core: add controller resume support when controller is powered off")
> cc: <stable@vger.kernel.org>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Acked-by: Peter Chen <peter.chen@kernel.org>

> ---
>  drivers/usb/chipidea/ci.h   |  2 ++
>  drivers/usb/chipidea/core.c | 44 ++++++++++++++++++++-----------------
>  2 files changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index d9bb3d3f026e..2a38e1eb6546 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -176,6 +176,7 @@ struct hw_bank {
>   * @enabled_otg_timer_bits: bits of enabled otg timers
>   * @next_otg_timer: next nearest enabled timer to be expired
>   * @work: work for role changing
> + * @power_lost_work: work for power lost handling
>   * @wq: workqueue thread
>   * @qh_pool: allocation pool for queue heads
>   * @td_pool: allocation pool for transfer descriptors
> @@ -226,6 +227,7 @@ struct ci_hdrc {
>  	enum otg_fsm_timer		next_otg_timer;
>  	struct usb_role_switch		*role_switch;
>  	struct work_struct		work;
> +	struct work_struct		power_lost_work;
>  	struct workqueue_struct		*wq;
>  
>  	struct dma_pool			*qh_pool;
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 41014f93cfdf..835bf2428dc6 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -856,6 +856,27 @@ static int ci_extcon_register(struct ci_hdrc *ci)
>  	return 0;
>  }
>  
> +static void ci_power_lost_work(struct work_struct *work)
> +{
> +	struct ci_hdrc *ci = container_of(work, struct ci_hdrc, power_lost_work);
> +	enum ci_role role;
> +
> +	disable_irq_nosync(ci->irq);
> +	pm_runtime_get_sync(ci->dev);
> +	if (!ci_otg_is_fsm_mode(ci)) {
> +		role = ci_get_role(ci);
> +
> +		if (ci->role != role) {
> +			ci_handle_id_switch(ci);
> +		} else if (role == CI_ROLE_GADGET) {
> +			if (ci->is_otg && hw_read_otgsc(ci, OTGSC_BSV))
> +				usb_gadget_vbus_connect(&ci->gadget);
> +		}
> +	}
> +	pm_runtime_put_sync(ci->dev);
> +	enable_irq(ci->irq);
> +}
> +
>  static DEFINE_IDA(ci_ida);
>  
>  struct platform_device *ci_hdrc_add_device(struct device *dev,
> @@ -1045,6 +1066,8 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&ci->lock);
>  	mutex_init(&ci->mutex);
> +	INIT_WORK(&ci->power_lost_work, ci_power_lost_work);
> +
>  	ci->dev = dev;
>  	ci->platdata = dev_get_platdata(dev);
>  	ci->imx28_write_fix = !!(ci->platdata->flags &
> @@ -1396,25 +1419,6 @@ static int ci_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static void ci_handle_power_lost(struct ci_hdrc *ci)
> -{
> -	enum ci_role role;
> -
> -	disable_irq_nosync(ci->irq);
> -	if (!ci_otg_is_fsm_mode(ci)) {
> -		role = ci_get_role(ci);
> -
> -		if (ci->role != role) {
> -			ci_handle_id_switch(ci);
> -		} else if (role == CI_ROLE_GADGET) {
> -			if (ci->is_otg && hw_read_otgsc(ci, OTGSC_BSV))
> -				usb_gadget_vbus_connect(&ci->gadget);
> -		}
> -	}
> -
> -	enable_irq(ci->irq);
> -}
> -
>  static int ci_resume(struct device *dev)
>  {
>  	struct ci_hdrc *ci = dev_get_drvdata(dev);
> @@ -1446,7 +1450,7 @@ static int ci_resume(struct device *dev)
>  		ci_role(ci)->resume(ci, power_lost);
>  
>  	if (power_lost)
> -		ci_handle_power_lost(ci);
> +		queue_work(system_freezable_wq, &ci->power_lost_work);
>  
>  	if (ci->supports_runtime_pm) {
>  		pm_runtime_disable(dev);
> -- 
> 2.34.1
> 
> 

-- 

Thanks,
Peter Chen