On Thu 03 Oct 22:38 PDT 2019, Stephen Boyd wrote: > Quoting Murali Nalajala (2019-10-03 16:51:50) > > @@ -151,14 +156,16 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr > > > > ret = device_register(&soc_dev->dev); > > if (ret) > > - goto out3; > > + goto out4; > > > > return soc_dev; > > > > -out3: > > +out4: > > ida_simple_remove(&soc_ida, soc_dev->soc_dev_num); > > put_device(&soc_dev->dev); > > soc_dev = NULL; > > +out3: > > + kfree(soc_attr_groups); > > This code is tricky. put_device(&soc_dev->dev) will call soc_release() > so we set soc_dev to NULL before calling kfree() on the error path. This > way we don't doubly free a pointer that the release function will take > care of. I wonder if the release function could free the ida as well, > and then we could just make the device_register() failure path call > put_device() and return ERR_PTR(ret) directly. Then the error path is > simpler because we can avoid changing two pointers to NULL to avoid the > double free twice. Or just inline the ida remove and put_device() call > in the if and then goto out1 to consolidate the error pointer > conversion. > But if we instead allocates the ida before the soc_dev, wouldn't the error path be something like?: foo: put_device(&soc_dev->dev); bar: ida_simple_remove(&soc_ida, soc_num); return err; I think we still need two exit paths from soc_device_register() regardless of moving the ida_simple_remove() into the release, but we could drop it from the unregister(). So not sure if this is cleaner... Regards, Bjorn > > out2: > > kfree(soc_dev); > > out1:
