Re: [PATCH 03/15] ASoC: hdac_hdmi - Use list to add pins and converters

["Subhransu S. Prusty" <subhransu.s.prusty@intel.com>]

On Wed, Dec 02, 2015 at 10:31:07AM +0100, Takashi Iwai wrote:
> On Tue, 01 Dec 2015 19:52:14 +0100,
> Takashi Iwai wrote:
> > 
> > On Tue, 01 Dec 2015 23:50:50 +0100,
> > Subhransu S. Prusty wrote:
> > > 
> > > On Tue, Dec 01, 2015 at 01:47:04PM +0100, Takashi Iwai wrote:
> > > > On Tue, 01 Dec 2015 18:46:59 +0100,
> > > > Subhransu S. Prusty wrote:
> > > > > 
> > > > > +static int hdac_hdmi_add_cvt(struct hdac_ext_device *edev, hda_nid_t nid)
> > > > > +{
> > > > > +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > > > > +	struct hdac_hdmi_cvt *cvt;
> > > > > +
> > > > > +	cvt = devm_kzalloc(&edev->hdac.dev, sizeof(*cvt), GFP_KERNEL);
> > > > 
> > > > Be careful about devm_*() usage here.  The devres resources may be
> > > > freed before the release callback is called for the assigned device
> > > > object.  See drivers/base/core.c:device_release().
> > > > 
> > > > For the top-level object, usually it's OK, because the top-level
> > > > object itself won't be released but only the driver is detached.
> > > > Then devres_release_all() is called in device_release_driver() while
> > > > the device itself is still present.
> > > 
> > > Not sure if I understood it correctly. But as the devres_release_all is
> > > called after the driver remove and the above resource is not expected to
> > > be used outside the scope of driver.
> > 
> > devres_release_all() is called *before* calling the device's release
> > callback, which is usually triggered by put_device().  So it depends
> > on how you call the device removal whether it's safe or not.
> > 
> > > So should't deleting the list entries
> > > be good enough during driver remove?
> > 
> > It depends on how the allocated data is accessed.  I guess it won't be
> > a problem in this case, but in general, you have to be careful about
> > devm_*() usage.  It can't be used always blindly.
> 
> To make clear my intention: the code used in this patch appears OK, as
> far as I see.  But, for example, if this code would be moved to HDA
> core layer, then it might not work -- there you'll have to release the
> resource in another way.  A driver-bound resource shall be released
> with the driver's unbinding, but a resource bound with the device
> isn't necessarily released there but first at device_release().

It's better then I will remove the devm_xxx and will manage the resources in
the driver itself.

Regards,
Subhransu
> 
> 
> Takashi

--