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
--