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

[Takashi Iwai <tiwai@suse.de>]

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.

> > I'm not sure whether this would be actually OK with hdac_device.  You
> > need to double-check, not only check whether the driver appears to
> > work, but track all object lifetime properly.
> 
> Will testing driver load/unload be helpful here with CONFIG_DEBUG_DEVRES
> enabled?

Maybe.  But at best you should read the code and follow the flow
closely by yourself.


Takashi

> 
> Regards,
> Subhransu
> > 
> > 
> > Takashi
> > 
> > > +	if (!cvt)
> > > +		return -ENOMEM;
> > > +
> > > +	cvt->nid = nid;
> > > +
> > > +	list_add_tail(&cvt->head, &hdmi->cvt_list);
> > > +	hdmi->num_cvt++;
> > > +
> > > +	return hdac_hdmi_query_cvt_params(&edev->hdac, cvt);
> > > +}
> > > +
> > > +static int hdac_hdmi_add_pin(struct hdac_ext_device *edev, hda_nid_t nid)
> > > +{
> > > +	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > > +	struct hdac_hdmi_pin *pin;
> > > +
> > > +	pin = devm_kzalloc(&edev->hdac.dev, sizeof(*pin), GFP_KERNEL);
> > > +	if (!pin)
> > > +		return -ENOMEM;
> > > +
> > > +	pin->nid = nid;
> > > +
> > > +	list_add_tail(&pin->head, &hdmi->pin_list);
> > > +	hdmi->num_pin++;
> > > +
> > > +	return 0;
> > >  }
> > >  
> > >  /*
> > > @@ -414,7 +459,7 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
> > >  	int i;
> > >  	struct hdac_device *hdac = &edev->hdac;
> > >  	struct hdac_hdmi_priv *hdmi = edev->private_data;
> > > -	int cvt_nid = 0, pin_nid = 0;
> > > +	int ret;
> > >  
> > >  	hdac->num_nodes = snd_hdac_get_sub_nodes(hdac, hdac->afg, &nid);
> > >  	if (!nid || hdac->num_nodes <= 0) {
> > > @@ -437,29 +482,25 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_ext_device *edev)
> > >  		switch (type) {
> > >  
> > >  		case AC_WID_AUD_OUT:
> > > -			hdmi->cvt_nid[cvt_nid] = nid;
> > > -			cvt_nid++;
> > > +			ret = hdac_hdmi_add_cvt(edev, nid);
> > > +			if (ret < 0)
> > > +				return ret;
> > >  			break;
> > >  
> > >  		case AC_WID_PIN:
> > > -			hdmi->pin_nid[pin_nid] = nid;
> > > -			pin_nid++;
> > > +			ret = hdac_hdmi_add_pin(edev, nid);
> > > +			if (ret < 0)
> > > +				return ret;
> > >  			break;
> > >  		}
> > >  	}
> > >  
> > >  	hdac->end_nid = nid;
> > >  
> > > -	if (!pin_nid || !cvt_nid)
> > > +	if (!hdmi->num_pin || !hdmi->num_cvt)
> > >  		return -EIO;
> > >  
> > > -	/*
> > > -	 * Currently on board only 1 pin and 1 converter is enabled for
> > > -	 * simplification, more will be added eventually
> > > -	 * So using fixed map for dai_id:pin:cvt
> > > -	 */
> > > -	return hdac_hdmi_init_dai_map(edev, &hdmi->dai_map[0], hdmi->pin_nid[0],
> > > -			hdmi->cvt_nid[0], 0);
> > > +	return hdac_hdmi_init_dai_map(edev);
> > >  }
> > >  
> > >  static int hdmi_codec_probe(struct snd_soc_codec *codec)
> > > @@ -543,6 +584,9 @@ static int hdac_hdmi_dev_probe(struct hdac_ext_device *edev)
> > >  
> > >  	dev_set_drvdata(&codec->dev, edev);
> > >  
> > > +	INIT_LIST_HEAD(&hdmi_priv->pin_list);
> > > +	INIT_LIST_HEAD(&hdmi_priv->cvt_list);
> > > +
> > >  	ret = hdac_hdmi_parse_and_map_nid(edev);
> > >  	if (ret < 0)
> > >  		return ret;
> > > -- 
> > > 1.9.1
> > > 
> 
> -- 
>