Re: [PATCH v2 2/3] usb: chipidea: core: fix possible concurrent when switch role

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

On 22-11-30 16:12:30, Xu Yang wrote:
> The user may call role_store() when driver is handling
> ci_handle_id_switch() which is triggerred by otg event or power lost
> event. Unfortunately, the controller may go into chaos in this case.
> Fix this by protecting it with mutex lock.
> 
> Fixes: a932a8041ff9 ("usb: chipidea: core: add sysfs group")
> cc: <stable@vger.kernel.org>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

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

Peter
> 
> ---
> changes since v1:
> - modify description for mutex member
> - wrap role variable in ci_handle_id_switch() too
> ---
>  drivers/usb/chipidea/ci.h   | 2 ++
>  drivers/usb/chipidea/core.c | 8 +++++++-
>  drivers/usb/chipidea/otg.c  | 5 ++++-
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 005c67cb3afb..f210b7489fd5 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -208,6 +208,7 @@ struct hw_bank {
>   * @in_lpm: if the core in low power mode
>   * @wakeup_int: if wakeup interrupt occur
>   * @rev: The revision number for controller
> + * @mutex: protect code from concorrent running when doing role switch
>   */
>  struct ci_hdrc {
>  	struct device			*dev;
> @@ -260,6 +261,7 @@ struct ci_hdrc {
>  	bool				in_lpm;
>  	bool				wakeup_int;
>  	enum ci_revision		rev;
> +	struct mutex                    mutex;
>  };
>  
>  static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index fcb175b22188..d7efde30d59f 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -987,8 +987,12 @@ static ssize_t role_store(struct device *dev,
>  	if (role == CI_ROLE_END)
>  		return -EINVAL;
>  
> -	if (role == ci->role)
> +	mutex_lock(&ci->mutex);
> +
> +	if (role == ci->role) {
> +		mutex_unlock(&ci->mutex);
>  		return n;
> +	}
>  
>  	pm_runtime_get_sync(dev);
>  	disable_irq(ci->irq);
> @@ -998,6 +1002,7 @@ static ssize_t role_store(struct device *dev,
>  		ci_handle_vbus_change(ci);
>  	enable_irq(ci->irq);
>  	pm_runtime_put_sync(dev);
> +	mutex_unlock(&ci->mutex);
>  
>  	return (ret == 0) ? n : ret;
>  }
> @@ -1033,6 +1038,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	spin_lock_init(&ci->lock);
> +	mutex_init(&ci->mutex);
>  	ci->dev = dev;
>  	ci->platdata = dev_get_platdata(dev);
>  	ci->imx28_write_fix = !!(ci->platdata->flags &
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> index 622c3b68aa1e..f5490f2a5b6b 100644
> --- a/drivers/usb/chipidea/otg.c
> +++ b/drivers/usb/chipidea/otg.c
> @@ -167,8 +167,10 @@ static int hw_wait_vbus_lower_bsv(struct ci_hdrc *ci)
>  
>  void ci_handle_id_switch(struct ci_hdrc *ci)
>  {
> -	enum ci_role role = ci_otg_role(ci);
> +	enum ci_role role;
>  
> +	mutex_lock(&ci->mutex);
> +	role = ci_otg_role(ci);
>  	if (role != ci->role) {
>  		dev_dbg(ci->dev, "switching from %s to %s\n",
>  			ci_role(ci)->name, ci->roles[role]->name);
> @@ -198,6 +200,7 @@ void ci_handle_id_switch(struct ci_hdrc *ci)
>  		if (role == CI_ROLE_GADGET)
>  			ci_handle_vbus_change(ci);
>  	}
> +	mutex_unlock(&ci->mutex);
>  }
>  /**
>   * ci_otg_work - perform otg (vbus/id) event handle
> -- 
> 2.34.1
> 

-- 

Thanks,
Peter Chen