linux-kernel - Re: [PATCH v2 5/9] usb: roles: Provide the switch drivers handle to the switch in the API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 2 Mar 2020 09:43:03 +0200
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Peter Chen <peter.chen@....com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Benson Leung <bleung@...omium.org>,
        Prashant Malani <pmalani@...omium.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        Felipe Balbi <balbi@...nel.org>,
        Chunfeng Yun <chunfeng.yun@...iatek.com>,
        Bin Liu <b-liu@...com>
Subject: Re: [PATCH v2 5/9] usb: roles: Provide the switch drivers handle to
 the switch in the API

On Mon, Mar 02, 2020 at 06:22:59AM +0000, Peter Chen wrote:
> On 20-02-24 13:14:40, Peter Chen wrote:
> > >   */
> > > -static int cdns3_role_set(struct device *dev, enum usb_role role)
> > > +static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
> > >  {
> > > -	struct cdns3 *cdns = dev_get_drvdata(dev);
> > > +	struct cdns3 *cdns = usb_role_switch_get_drvdata(sw);
> > >  	int ret = 0;
> > >  
> > >  	pm_runtime_get_sync(cdns->dev);
> > > @@ -423,12 +423,6 @@ static int cdns3_role_set(struct device *dev, enum usb_role role)
> > >  	return ret;
> > >  }
> > >  
> > > -static const struct usb_role_switch_desc cdns3_switch_desc = {
> > > -	.set = cdns3_role_set,
> > > -	.get = cdns3_role_get,
> > > -	.allow_userspace_control = true,
> > > -};
> > > -
> > >  /**
> > >   * cdns3_probe - probe for cdns3 core device
> > >   * @pdev: Pointer to cdns3 core platform device
> > > @@ -437,6 +431,7 @@ static const struct usb_role_switch_desc cdns3_switch_desc = {
> > >   */
> > >  static int cdns3_probe(struct platform_device *pdev)
> > >  {
> > > +	struct usb_role_switch_desc sw_desc = { };
> > >  	struct device *dev = &pdev->dev;
> > >  	struct resource	*res;
> > >  	struct cdns3 *cdns;
> > > @@ -529,7 +524,12 @@ static int cdns3_probe(struct platform_device *pdev)
> > >  	if (ret)
> > >  		goto err3;
> > >  
> > > -	cdns->role_sw = usb_role_switch_register(dev, &cdns3_switch_desc);
> > > +	sw_desc.set = cdns3_role_set,
> > > +	sw_desc.get = cdns3_role_get,
> > > +	sw_desc.allow_userspace_control = true,
> 
> Heikki, when I try to apply above, and compile, I find above issue,
> the end of code should be ";", but not ",".

OK. I'll fix that. I could have sworn that I tested that, but clearly
I have not done it.

> > > +	sw_desc.driver_data = cdns;
> > > +
> > > +	cdns->role_sw = usb_role_switch_register(dev, &sw_desc);
> > >  	if (IS_ERR(cdns->role_sw)) {
> > >  		ret = PTR_ERR(cdns->role_sw);
> > >  		dev_warn(dev, "Unable to register Role Switch\n");
> > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > > index 52139c2a9924..ae0bdc036464 100644
> > > --- a/drivers/usb/chipidea/core.c
> > > +++ b/drivers/usb/chipidea/core.c
> > > @@ -600,9 +600,9 @@ static int ci_cable_notifier(struct notifier_block *nb, unsigned long event,
> > >  	return NOTIFY_DONE;
> > >  }
> > >  
> > > -static enum usb_role ci_usb_role_switch_get(struct device *dev)
> > > +static enum usb_role ci_usb_role_switch_get(struct usb_role_switch *sw)
> > >  {
> > > -	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > > +	struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> > >  	enum usb_role role;
> > >  	unsigned long flags;
> > >  
> > > @@ -613,9 +613,10 @@ static enum usb_role ci_usb_role_switch_get(struct device *dev)
> > >  	return role;
> > >  }
> > >  
> > > -static int ci_usb_role_switch_set(struct device *dev, enum usb_role role)
> > > +static int ci_usb_role_switch_set(struct usb_role_switch *sw,
> > > +				  enum usb_role role)
> > >  {
> > > -	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > > +	struct ci_hdrc *ci = usb_role_switch_get_drvdata(sw);
> > >  	struct ci_hdrc_cable *cable = NULL;
> > >  	enum usb_role current_role = ci_role_to_usb_role(ci);
> > >  	enum ci_role ci_role = usb_role_to_ci_role(role);
> > > @@ -1118,6 +1119,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > >  	}
> > >  
> > >  	if (ci_role_switch.fwnode) {
> > > +		ci_role_switch.driver_data = ci;
> 
> And chipidea code, better change it like cdns3's, otherwise, the
> switch desc for all controllers have the same driver_data.

OK.

thanks,

-- 
heikki