Re: [PATCH 3/3] usb: chipidea: core: convert ci_role_switch to local variable

["Peter Chen (CIX)" <peter.chen@kernel.org>]

On 26-04-21 16:24:36, Xu Yang wrote:
> When a system contains multiple USB controllers, the global ci_role_switch
> variable may be overwritten by subsequent driver initialization code.
> 
> This can cause issues in the following cases:
>  - The 2nd ci_hdrc_probe() sees ci_role_switch.fwnode as non-NULL even
>    though the "usb-role-switch" property is not present for the controller.
>  - When the ci_hdrc device is unbound and bound again, ci_role_switch
>    fwnode will not be reassigned, and the old value will be used instead.
> 
> Convert ci_role_switch to a local variable to fix these issues.
> 
> Fixes: 05559f10ed79 ("usb: chipidea: add role switch class support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

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

Peter
> ---
>  drivers/usb/chipidea/core.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 95d9db159ce8..07563be0013f 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -655,12 +655,6 @@ static enum ci_role ci_get_role(struct ci_hdrc *ci)
>  	return role;
>  }
>  
> -static struct usb_role_switch_desc ci_role_switch = {
> -	.set = ci_usb_role_switch_set,
> -	.get = ci_usb_role_switch_get,
> -	.allow_userspace_control = true,
> -};
> -
>  static int ci_get_platdata(struct device *dev,
>  		struct ci_hdrc_platform_data *platdata)
>  {
> @@ -787,9 +781,6 @@ static int ci_get_platdata(struct device *dev,
>  			cable->connected = false;
>  	}
>  
> -	if (device_property_read_bool(dev, "usb-role-switch"))
> -		ci_role_switch.fwnode = dev->fwnode;
> -
>  	platdata->pctl = devm_pinctrl_get(dev);
>  	if (!IS_ERR(platdata->pctl)) {
>  		struct pinctrl_state *p;
> @@ -1033,6 +1024,7 @@ ATTRIBUTE_GROUPS(ci);
>  
>  static int ci_hdrc_probe(struct platform_device *pdev)
>  {
> +	struct usb_role_switch_desc ci_role_switch = {};
>  	struct device	*dev = &pdev->dev;
>  	struct ci_hdrc	*ci;
>  	struct resource	*res;
> @@ -1179,7 +1171,11 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	if (ci_role_switch.fwnode) {
> +	if (device_property_read_bool(dev, "usb-role-switch")) {
> +		ci_role_switch.set = ci_usb_role_switch_set;
> +		ci_role_switch.get = ci_usb_role_switch_get;
> +		ci_role_switch.allow_userspace_control = true;
> +		ci_role_switch.fwnode = dev_fwnode(dev);
>  		ci_role_switch.driver_data = ci;
>  		ci->role_switch = usb_role_switch_register(dev,
>  					&ci_role_switch);
> -- 
> 2.34.1
> 

-- 

Best regards,
Peter