From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754118AbbGWUIP (ORCPT ); Thu, 23 Jul 2015 16:08:15 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:55648 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753677AbbGWUIM (ORCPT ); Thu, 23 Jul 2015 16:08:12 -0400 Date: Thu, 23 Jul 2015 15:08:08 -0500 From: Felipe Balbi To: Greg Kroah-Hartman CC: Felipe Balbi , Heikki Krogerus , ChengYi He , , Subject: Re: [PATCH] usb: ulpi: call put_device if device_register fails Message-ID: <20150723200808.GF3683@saruman.tx.rr.com> Reply-To: References: <20150618171236.GA18007@chengyihe-ThinkCentre-M82> <20150623105738.GD1296@kuha.fi.intel.com> <20150722213934.GA10425@kroah.com> <20150723020440.GB16806@saruman.tx.rr.com> <20150723031446.GA10588@kroah.com> <20150723050240.GA18430@saruman.tx.rr.com> <20150723180029.GA29213@kroah.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="V4b9U9vrdWczvw78" Content-Disposition: inline In-Reply-To: <20150723180029.GA29213@kroah.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --V4b9U9vrdWczvw78 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Thu, Jul 23, 2015 at 11:00:29AM -0700, Greg Kroah-Hartman wrote: > On Thu, Jul 23, 2015 at 12:02:40AM -0500, Felipe Balbi wrote: > > Hi, > >=20 > > On Wed, Jul 22, 2015 at 08:14:46PM -0700, Greg Kroah-Hartman wrote: > > > On Wed, Jul 22, 2015 at 09:04:40PM -0500, Felipe Balbi wrote: > > > > On Wed, Jul 22, 2015 at 02:39:34PM -0700, Greg Kroah-Hartman wrote: > > > > > On Tue, Jun 23, 2015 at 01:57:38PM +0300, Heikki Krogerus wrote: > > > > > > On Fri, Jun 19, 2015 at 01:12:36AM +0800, ChengYi He wrote: > > > > > > > put_device is required to release the last reference to the d= evice. > > > > > > >=20 > > > > > > > Signed-off-by: ChengYi He > > > > > > > --- > > > > > > > drivers/usb/common/ulpi.c | 4 +++- > > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > >=20 > > > > > > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/u= lpi.c > > > > > > > index 0e6f968..bd25bdb 100644 > > > > > > > --- a/drivers/usb/common/ulpi.c > > > > > > > +++ b/drivers/usb/common/ulpi.c > > > > > > > @@ -184,8 +184,10 @@ static int ulpi_register(struct device *= dev, struct ulpi *ulpi) > > > > > > > request_module("ulpi:v%04xp%04x", ulpi->id.vendor, ulpi->id= =2Eproduct); > > > > > > > =20 > > > > > > > ret =3D device_register(&ulpi->dev); > > > > > > > - if (ret) > > > > > > > + if (ret) { > > > > > > > + put_device(&ulpi->dev); > > > > > >=20 > > > > > > If device_register returns failure, put_device has already been > > > > > > called. Check device_add in drivers/base/core.c. > > > > >=20 > > > > > Yes, please read the function, which says: > > > > > * NOTE: _Never_ directly free @dev after calling this function, = even > > > > > * if it returned an error! Always use put_device() to give up yo= ur > > > > > * reference instead. > > > > >=20 > > > > > But, the problem is that the ulpi core doesn't "own" that struct = device. > > > > > It comes from elsewhere. It comes from somewhere deep down in th= e dw3 > > > > > core, which is where I lost the path. Something needs to be fixe= d in > > > > > dwc3_probe() to properly clean up the device if it fails, which i= s not > > > > > happening right now. > > > > >=20 > > > > > So this patch would actually cause much bigger problems than fixi= ng > > > > > anything, so it's wrong, but for a different reason than you are = talking > > > > > about here. > > > > >=20 > > > > > And ugh, the ulpi and dwc code binding together, what a mess, hor= rid... > > > >=20 > > > > any suggestions ? DWC *is* the one implementing the bus. If there's= a > > > > better way, we can certainly shuffle code around. > > >=20 > > > As dwc is the only thing using the bus, why is it drivers/usb/core/ ? > >=20 > > musb also has a SW-accessible ULPI bus. And, IIRC, so does DWC2 ;-) >=20 > But they aren't calling ulpi_register(), so how can they be using this > code? the thing was just added :-) It didn't exist before. > > > And the error path here is broken, the bus should be creating the dev= ice > > > (i.e. no subsystem should ever be registering a device it did not > > > create), so that it can properly clean things up when stuff goes wron= g. > > >=20 > > > The whole subsys_init() is also a bad feeling that it's not architect= ed > > > correctly, that shouldn't be needed, which is why I never took that > > > patch. Just noticed it came in through yours, I wanted it "broken" so > > > it would be fixed "properly" and not papered over like this. > >=20 > > I just felt it would be better to 'fix' it for the -rc until it can be > > fixed *properly*. A follow up fix should incur no visible changes to > > drivers anyway. >=20 > I don't like fixes like this because no one now has any pressure to fix > it "properly". Are you doing that work? If not, who is? Heikki is author, I'd expect him to fix it up. We can also revert the fix if you prefer, I'm totally fine with that. --=20 balbi --V4b9U9vrdWczvw78 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVsUmoAAoJEIaOsuA1yqREevoP/1zWP0WWYscmPwjqCvLrZKHZ gy6wLowABDXgUCXGB2KvbdI4sIpGaGKoZBjCC1DvXJyfmCu8wEOu19Esrjr84mhH /o3+1QkHBfEer/DDbKbiwvXV4Wtm3EH+fN1AjR/KN0Yj+HzRFkYgEDfJK+bp2Uhf OvFV7ZqJG6WAif4x+wP1bYNF/tyGuOM7HYXkDfl0IieyJVpWKdOsXxmlFHLSaCdW yrcuMsqxFEdSKB9qYUA0l5zr4isUeb4kbbKzI30ZdF0CbchKBIMkndEtYeUGo5wE DL2DoIykSOkjyajUtb8g/0QDqDIF2h88Cr+EJ1uWFNeg2bxHZv49HdgY/+ZJgNrL w5T+KOMxx2Xha5Eg+a5v5d/JLrLdLtOGGzwCdXYXJFG+cZgsg07qFi57MAbmu2oV q4jVU2ij4HMaEslrcXBRvV4HeUtY9j3Tc2c6pDIh6VZvVzLzVOLOXj9atLM/U/kB 05Z0g6mo7K6ipI8V+LM1pUjbie227b23YExDcAQAY6Qp042PRvLxDnuyD7lJB8R7 kTi9HTvxeaG9iLAsHOqMwGFR3Xyw+Yhqt1cHsUBpKDO+p5y0EvEKpQbBJekfUmcg U2Vn3fa83df/Rk9wc09eHBgFSp7WNurBGwk7cArcAmJxYdB0/BfVT/aJlHvwBxr9 SmORoAl9Mwx+pVgiulhp =DJMh -----END PGP SIGNATURE----- --V4b9U9vrdWczvw78--