Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756019AbbHFSbI (ORCPT ); Thu, 6 Aug 2015 14:31:08 -0400 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:59722 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753846AbbHFSbG (ORCPT ); Thu, 6 Aug 2015 14:31:06 -0400 Date: Thu, 6 Aug 2015 19:30:29 +0100 From: Mark Brown To: Greg KH Cc: Baolin Wang , balbi@ti.com, linux-kernel@vger.kernel.org, peter.chen@freescale.com, sojka@merica.cz, stern@rowland.harvard.edu, andreas@gaisler.com, linux-usb@vger.kernel.org, device-mainlining@lists.linuxfoundation.org Message-ID: <20150806183029.GE20873@sirena.org.uk> References: <2ba023cbaa1dd96fd5e31e2f01cc0a599a3a5e55.1438844454.git.baolin.wang@linaro.org> <20150806163905.GA6907@kroah.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="fTdpWH5/fVS2oCf7" Content-Disposition: inline In-Reply-To: <20150806163905.GA6907@kroah.com> X-Cookie: Please take note: User-Agent: Mutt/1.5.23 (2014-03-12) X-SA-Exim-Connect-IP: 94.175.94.161 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [PATCH 1/2] gadget: Introduce the usb charger framework X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on mezzanine.sirena.org.uk) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2465 Lines: 68 --fTdpWH5/fVS2oCf7 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Aug 06, 2015 at 09:39:05AM -0700, Greg KH wrote: > On Thu, Aug 06, 2015 at 03:03:48PM +0800, Baolin Wang wrote: > > +static void usb_charger_release(struct device *dev) > > +{ > > + struct usb_charger *uchger = dev_get_drvdata(dev); > > + if (!atomic_dec_and_test(&uchger->count)) { > > + dev_err(dev, "The usb charger is still in use\n"); > Why is the "count" different from the reference count? You shouldn't be > in this function if the reference count is not 0, so tie your "user" > count to this one. Having two different reference counts is a nightmare > and almost impossible to get right. And a huge red flag that the design > is incorrect. > > + return; > You can't "fail" a release call, so you just leaked memory all over the > floor here :( Indeed. I did discuss this with Baolin off list but I'd missed the dynamic allocation of devices for some reason. > > + mutex_lock(&usb_charger_list_lock); > > + list_for_each_entry(tmp, &usb_charger_list, entry) { > > + if (!(strcmp(tmp->name, uchger->name))) { > > + mutex_unlock(&usb_charger_list_lock); > > + ret = -EEXIST; > > + goto out; > > + } > > + } > > + list_add_tail(&uchger->entry, &usb_charger_list); > Why do you need a separate list? This subsystem's bus structure should > own that list of devices, no need for a separate one (again, a huge red > flag that the design is not correct.) Right, if we dynamically allocate a device per charger then the lifetime issues should go away and we get a list for free. --fTdpWH5/fVS2oCf7 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJVw6fEAAoJECTWi3JdVIfQV78H/j/4ll9fEijhHDsBHQk5Fwxm M4YO8mgRaWcqU7AK5VmOi7M65R5BM+UdOIRRPzUdJlJgkH9PLYo6vlSgcrfBk844 ZysG5qUPjeVh4Bb/ub6Mv1BAFaf3BT8dV7BPKQRTz2HYGVq2yjHOkFEhiwrJuE+C REhaZ3dK4IECOWJuR5b0JlqtCTbWAdjMto3Si7qt5K8M+Cf5q3O5sZcZS+Wqr8uh bCA8Y+WrSKKse581w9uX4qIgUzULSmfSD+seH5eGXhhr/S3TfPuJQzZ2X2/FbvO7 /Pf1AVwGdG6VLiMPCL2ANAdLhG2wDHf9GpgPrvaV/EzKVAjn8whQhmmWjXTAeO8= =MvFx -----END PGP SIGNATURE----- --fTdpWH5/fVS2oCf7-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/