Messages in this thread | | | Date | Thu, 6 Aug 2015 19:30:29 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 1/2] gadget: Introduce the usb charger framework |
| |
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. [unhandled content-type:application/pgp-signature] | |