linux-kernel - Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

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]

Message-ID: <20160420093956.GD27526@shlinux2.ap.freescale.net>
Date:	Wed, 20 Apr 2016 17:39:56 +0800
From:	Peter Chen <hzpeterchen@...il.com>
To:	Roger Quadros <rogerq@...com>
Cc:	stern@...land.harvard.edu, balbi@...nel.org,
	gregkh@...uxfoundation.org, peter.chen@...escale.com,
	dan.j.williams@...el.com, jun.li@...escale.com,
	mathias.nyman@...ux.intel.com, tony@...mide.com,
	Joao.Pinto@...opsys.com, abrestic@...omium.org,
	r.baldyga@...sung.com, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org
Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

On Wed, Apr 20, 2016 at 10:02:33AM +0300, Roger Quadros wrote:
> On 19/04/16 11:06, Peter Chen wrote:
> > On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
> >> +/**
> >> + * usb_otg_start_host - start/stop the host controller
> >> + * @otg:	usb_otg instance
> >> + * @on:		true to start, false to stop
> >> + *
> >> + * Start/stop the USB host controller. This function is meant
> >> + * for use by the OTG controller driver.
> >> + */
> >> +int usb_otg_start_host(struct usb_otg *otg, int on)
> >> +{
> >> +	struct otg_hcd_ops *hcd_ops = otg->hcd_ops;
> >> +
> >> +	dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
> >> +	if (!otg->host) {
> >> +		WARN_ONCE(1, "otg: fsm running without host\n");
> >> +		return 0;
> >> +	}
> >> +
> >> +	if (on) {
> >> +		if (otg->flags & OTG_FLAG_HOST_RUNNING)
> >> +			return 0;
> >> +
> >> +		otg->flags |= OTG_FLAG_HOST_RUNNING;
> >> +
> >> +		/* start host */
> >> +		hcd_ops->add(otg->primary_hcd.hcd, otg->primary_hcd.irqnum,
> >> +			     otg->primary_hcd.irqflags);
> >> +		if (otg->shared_hcd.hcd) {
> >> +			hcd_ops->add(otg->shared_hcd.hcd,
> >> +				     otg->shared_hcd.irqnum,
> >> +				     otg->shared_hcd.irqflags);
> >> +		}
> > 
> > Check the return value please.
> 
> And what should we do on failure?
> Even if things fail, they could potentially start working on next
> remove/add iteration so I didn't bother checking return values.
> 

If usb_add_hcd has failed, the hcd may be released (usb_put_hcd is
called), in that case, we can't call usb_remove_hcd, maybe we may
need to add hcd valid check for primary hcd too. Even we can't
stop fsm, we need to show an error message for user.

Chipidea idea have a bug before:

commit 41314fea2ffb6dc716b7e698a47c68b329602fc0
Author: Russell King - ARM Linux <linux@....linux.org.uk>
Date:   Wed Oct 16 13:45:15 2013 +0100

    usb/chipidea: fix oops on memory allocation failure

-- 

Best Regards,
Peter Chen