[alsa-devel] [PATCH 0/7] Add devres support to card resources
Hi,
here is the patchset containing the changes to support a device-managed resource management for ALSA card and memory pages. The actual code changes are identical with the previous RFC series, but a few documentation updates are included in this official set.
Takashi
===
Takashi Iwai (7): ALSA: core: Add device-managed page allocator helper ALSA: core: Add managed card creation ALSA: intel8x0: Allocate resources with device-managed APIs ALSA: atiixp: Allocate resources with device-managed APIs ALSA: hda: Allocate resources with device-managed APIs ALSA: doc: Brush up the old writing-an-alsa-driver ALSA: doc: Add device-managed resource section
.../kernel-api/writing-an-alsa-driver.rst | 340 ++++++++++-------- include/sound/core.h | 5 + include/sound/memalloc.h | 4 + sound/core/init.c | 95 ++++- sound/core/memalloc.c | 88 ++++- sound/pci/atiixp.c | 104 ++---- sound/pci/atiixp_modem.c | 104 ++---- sound/pci/hda/hda_controller.h | 1 - sound/pci/hda/hda_intel.c | 43 +-- sound/pci/intel8x0.c | 143 +++----- sound/pci/intel8x0m.c | 143 +++----- 11 files changed, 514 insertions(+), 556 deletions(-)
This is a preparation for managing the whole resource via devres. As a first step, add a new allocator function, snd_devm_alloc_pages() to manage the allocated pages via devres, so that the pages will be automagically released as device unbinding.
Unlike the old snd_dma_alloc_pages(), the new function returns directly the snd_dma_buffer pointer. The caller needs to check the error via IS_ERR().
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/memalloc.h | 4 ++ sound/core/memalloc.c | 88 ++++++++++++++++++++++++++++++++-------- 2 files changed, 74 insertions(+), 18 deletions(-)
diff --git a/include/sound/memalloc.h b/include/sound/memalloc.h index af3fa577fa06..3a1d9fb44fcf 100644 --- a/include/sound/memalloc.h +++ b/include/sound/memalloc.h @@ -156,5 +156,9 @@ void snd_dma_free_pages(struct snd_dma_buffer *dmab); void *snd_malloc_pages(size_t size, gfp_t gfp_flags); void snd_free_pages(void *ptr, size_t size);
+/* device-managed memory allocator */ +struct snd_dma_buffer *snd_devm_alloc_pages(struct device *dev, int type, + size_t size); + #endif /* __SOUND_MEMALLOC_H */
diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c index aa266907ec9b..a54d7aacec43 100644 --- a/sound/core/memalloc.c +++ b/sound/core/memalloc.c @@ -161,22 +161,8 @@ static void snd_free_dev_iram(struct snd_dma_buffer *dmab) * */
- -/** - * snd_dma_alloc_pages - allocate the buffer area according to the given type - * @type: the DMA buffer type - * @device: the device pointer - * @size: the buffer size to allocate - * @dmab: buffer allocation record to store the allocated data - * - * Calls the memory-allocator function for the corresponding - * buffer type. - * - * Return: Zero if the buffer with the given size is allocated successfully, - * otherwise a negative value on error. - */ -int snd_dma_alloc_pages(int type, struct device *device, size_t size, - struct snd_dma_buffer *dmab) +static int __snd_dma_alloc_pages(struct device *device, int type, size_t size, + gfp_t gfp_flags, struct snd_dma_buffer *dmab) { if (WARN_ON(!size)) return -ENXIO; @@ -188,8 +174,7 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size, dmab->bytes = 0; switch (type) { case SNDRV_DMA_TYPE_CONTINUOUS: - dmab->area = snd_malloc_pages(size, - (__force gfp_t)(unsigned long)device); + dmab->area = snd_malloc_pages(size, gfp_flags); dmab->addr = 0; break; #ifdef CONFIG_HAS_DMA @@ -225,6 +210,30 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size, dmab->bytes = size; return 0; } + +/** + * snd_dma_alloc_pages - allocate the buffer area according to the given type + * @type: the DMA buffer type + * @device: the device pointer + * @size: the buffer size to allocate + * @dmab: buffer allocation record to store the allocated data + * + * Calls the memory-allocator function for the corresponding + * buffer type. + * + * Return: Zero if the buffer with the given size is allocated successfully, + * otherwise a negative value on error. + */ +int snd_dma_alloc_pages(int type, struct device *device, size_t size, + struct snd_dma_buffer *dmab) +{ + gfp_t gfp_flags = 0; + + if (type == SNDRV_DMA_TYPE_CONTINUOUS) + gfp_flags = (__force gfp_t)(unsigned long)device; + + return __snd_dma_alloc_pages(device, type, size, gfp_flags, dmab); +} EXPORT_SYMBOL(snd_dma_alloc_pages);
/** @@ -296,3 +305,46 @@ void snd_dma_free_pages(struct snd_dma_buffer *dmab) } } EXPORT_SYMBOL(snd_dma_free_pages); + +/* called by devres */ +static void __snd_release_pages(struct device *dev, void *res) +{ + snd_dma_free_pages(res); +} + +/** + * snd_devm_alloc_pages - allocate the buffer and manage with devres + * @dev: the device pointer + * @type: the DMA buffer type + * @size: the buffer size to allocate + * + * Allocate buffer pages depending on the given type and manage using devres. + * The pages will be released automatically at the device removal. + * + * The function cannot handle GFP flags for SNDRV_DMA_TYPE_CONTINUOUS type, + * only GFP_KERNEL is assumed. + * + * The function returns the snd_dma_buffer object at success or the encoded + * error pointer via ERR_PTR(). The caller needs to check the error via + * IS_ERR() and PTR_ERR(). + */ +struct snd_dma_buffer * +snd_devm_alloc_pages(struct device *dev, int type, size_t size) +{ + struct snd_dma_buffer *dmab; + int err; + + dmab = devres_alloc(__snd_release_pages, sizeof(*dmab), GFP_KERNEL); + if (!dmab) + return ERR_PTR(-ENOMEM); + + err = __snd_dma_alloc_pages(dev, type, size, GFP_KERNEL, dmab); + if (err < 0) { + devres_free(dmab); + return ERR_PTR(err); + } + + devres_add(dev, dmab); + return dmab; +} +EXPORT_SYMBOL_GPL(snd_devm_alloc_pages);
Per popular demands, this patch adds a new ALSA core API function, snd_devm_card_new(), to create a snd_card object in a managed way via devres. When a card object is created by this new function, it's released automatically at the device release. It includes also the call of snd_card_free().
However, the story isn't that simple. A caveat is that We have to call snd_card_new(), more specifically, the disconnection part, at very first of the whole resource release procedure. This assures that the exposed devices are deleted and sync with the all accessing processes getting closed.
For achieving it, snd_card_register() adds a new devres action to trigger snd_card_free() automatically when the given card object is a "managed" one. Since usually snd_card_register() is the last step of the initialization, this should work in most cases.
With all these tricks, some drivers can get rid of the whole the driver remove callback.
About a bit of implementation details: the patch adds two new flags to snd_card object, managed and releasing. The former indicates that the object was created via snd_devm_card_new(), and the latter is used for avoiding the double-free of snd_card_free() calls. Both flags are fairly internal and likely uninteresting to normal users.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/core.h | 5 +++ sound/core/init.c | 95 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 4 deletions(-)
diff --git a/include/sound/core.h b/include/sound/core.h index 36a5934cf4b1..6ff75f92ec7d 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -133,6 +133,8 @@ struct snd_card { struct device card_dev; /* cardX object for sysfs */ const struct attribute_group *dev_groups[4]; /* assigned sysfs attr */ bool registered; /* card_dev is registered? */ + bool managed; /* managed via devres */ + bool releasing; /* during card free process */ wait_queue_head_t remove_sleep;
#ifdef CONFIG_PM @@ -239,6 +241,9 @@ extern int (*snd_mixer_oss_notify_callback)(struct snd_card *card, int cmd); int snd_card_new(struct device *parent, int idx, const char *xid, struct module *module, int extra_size, struct snd_card **card_ret); +int snd_devm_card_new(struct device *parent, int idx, const char *xid, + struct module *module, int extra_size, + struct snd_card **card_ret);
int snd_card_disconnect(struct snd_card *card); void snd_card_disconnect_sync(struct snd_card *card); diff --git a/sound/core/init.c b/sound/core/init.c index 4849c611c0fe..2eade57db4b4 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -173,6 +173,9 @@ void snd_device_initialize(struct device *dev, struct snd_card *card) } EXPORT_SYMBOL_GPL(snd_device_initialize);
+static int snd_card_init(struct snd_card *card, struct device *parent, + int idx, const char *xid, struct module *module, + int extra_size); static int snd_card_do_free(struct snd_card *card); static const struct attribute_group card_dev_attr_group;
@@ -214,6 +217,73 @@ int snd_card_new(struct device *parent, int idx, const char *xid, card = kzalloc(sizeof(*card) + extra_size, GFP_KERNEL); if (!card) return -ENOMEM; + + err = snd_card_init(card, parent, idx, xid, module, extra_size); + if (err < 0) { + kfree(card); + return err; + } + + *card_ret = card; + return 0; +} +EXPORT_SYMBOL(snd_card_new); + +static void __snd_card_release(struct device *dev, void *data) +{ + snd_card_free(data); +} + +/** + * snd_devm_card_new - managed snd_card object creation + * @parent: the parent device object + * @idx: card index (address) [0 ... (SNDRV_CARDS-1)] + * @xid: card identification (ASCII string) + * @module: top level module for locking + * @extra_size: allocate this extra size after the main soundcard structure + * @card_ret: the pointer to store the created card instance + * + * This function works like snd_card_new() but manages the allocated resource + * via devres, i.e. you don't need to free explicitly. + * + * When a snd_card object is created with this function and registered via + * snd_card_register(), the very first devres action to call snd_card_free() + * is added automatically. In that way, the resource disconnection is assured + * at first, then released in the expected order. + */ +int snd_devm_card_new(struct device *parent, int idx, const char *xid, + struct module *module, int extra_size, + struct snd_card **card_ret) +{ + struct snd_card *card; + int err; + + *card_ret = NULL; + if (extra_size < 0) + extra_size = 0; + card = devres_alloc(__snd_card_release, sizeof(*card) + extra_size, + GFP_KERNEL); + if (!card) + return -ENOMEM; + card->managed = true; + err = snd_card_init(card, parent, idx, xid, module, extra_size); + if (err < 0) { + devres_free(card); + return err; + } + + devres_add(parent, card); + *card_ret = card; + return 0; +} +EXPORT_SYMBOL_GPL(snd_devm_card_new); + +static int snd_card_init(struct snd_card *card, struct device *parent, + int idx, const char *xid, struct module *module, + int extra_size) +{ + int err; + if (extra_size > 0) card->private_data = (char *)card + sizeof(struct snd_card); if (xid) @@ -235,7 +305,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid, mutex_unlock(&snd_card_mutex); dev_err(parent, "cannot find the slot for index %d (range 0-%i), error: %d\n", idx, snd_ecards_limit - 1, err); - kfree(card); return err; } set_bit(idx, snd_cards_lock); /* lock it */ @@ -282,7 +351,7 @@ int snd_card_new(struct device *parent, int idx, const char *xid, dev_err(parent, "unable to create card info\n"); goto __error_ctl; } - *card_ret = card; + return 0;
__error_ctl: @@ -291,7 +360,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid, put_device(&card->card_dev); return err; } -EXPORT_SYMBOL(snd_card_new);
/* return non-zero if a card is already locked */ int snd_card_locked(int card) @@ -484,6 +552,7 @@ EXPORT_SYMBOL_GPL(snd_card_disconnect_sync);
static int snd_card_do_free(struct snd_card *card) { + card->releasing = true; #if IS_ENABLED(CONFIG_SND_MIXER_OSS) if (snd_mixer_oss_notify_callback) snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_FREE); @@ -498,7 +567,8 @@ static int snd_card_do_free(struct snd_card *card) } if (card->release_completion) complete(card->release_completion); - kfree(card); + if (!card->managed) + kfree(card); return 0; }
@@ -539,6 +609,9 @@ int snd_card_free(struct snd_card *card) struct completion released; int ret;
+ if (card->releasing) + return 0; + init_completion(&released); card->release_completion = &released; ret = snd_card_free_when_closed(card); @@ -748,6 +821,11 @@ int snd_card_add_dev_attr(struct snd_card *card, } EXPORT_SYMBOL_GPL(snd_card_add_dev_attr);
+static void trigger_card_free(void *data) +{ + snd_card_free(data); +} + /** * snd_card_register - register the soundcard * @card: soundcard structure @@ -771,6 +849,15 @@ int snd_card_register(struct snd_card *card) if (err < 0) return err; card->registered = true; + } else { + if (card->managed) + devm_remove_action(card->dev, trigger_card_free, card); + } + + if (card->managed) { + err = devm_add_action(card->dev, trigger_card_free, card); + if (err < 0) + return err; }
if ((err = snd_device_register_all(card)) < 0)
Hi,
On Sep 21 2018 00:54, Takashi Iwai wrote:
Per popular demands, this patch adds a new ALSA core API function, snd_devm_card_new(), to create a snd_card object in a managed way via devres. When a card object is created by this new function, it's released automatically at the device release. It includes also the call of snd_card_free().
However, the story isn't that simple. A caveat is that We have to call snd_card_new(), more specifically, the disconnection part, at very first of the whole resource release procedure. This assures that the exposed devices are deleted and sync with the all accessing processes getting closed.
For achieving it, snd_card_register() adds a new devres action to trigger snd_card_free() automatically when the given card object is a "managed" one. Since usually snd_card_register() is the last step of the initialization, this should work in most cases.
With all these tricks, some drivers can get rid of the whole the driver remove callback.
About a bit of implementation details: the patch adds two new flags to snd_card object, managed and releasing. The former indicates that the object was created via snd_devm_card_new(), and the latter is used for avoiding the double-free of snd_card_free() calls. Both flags are fairly internal and likely uninteresting to normal users.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/core.h | 5 +++ sound/core/init.c | 95 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 4 deletions(-)
In my opinion, the new 'snd_devm_card_new()' is not good in hot-plug scenario. It brings kernel oops for processes to touch released device data relevant to target devices.
For example, for devices connected to each buses, some helper functions are available to up/down reference count of 'struct device': - PCIe: pci_dev_get()/pci_dev_put() - USB: usb_get_intf()/usb_put_intf() - IEEE 1394: fw_unit_get()/fw_unit_put()
In hot-plug scenario, drivers need to increment the reference counter in .probe() callback. In .remove/.disconnect callback, the reference counter should be kept but just set disconnect state to sound card/device instances. When .private_free callback of sound card device, the reference is decremented. This is required to enable userspace applications to handle disconnect processes and avoid kernel oops by touching released device data related to the connected bus.
As a quick glance, existent drivers for devices on PCIe/USB are not programmed with enough care of this point. It's prior to fix them for your 'caveat'.
...but it's likely for me to get wrong understanding design of whole existent driver in sound subsystem. I'm happy to receive your indications against my misunderstanding.
Regards
Takashi Sakamoto
On Fri, 21 Sep 2018 05:01:31 +0200, Takashi Sakamoto wrote:
Hi,
On Sep 21 2018 00:54, Takashi Iwai wrote:
Per popular demands, this patch adds a new ALSA core API function, snd_devm_card_new(), to create a snd_card object in a managed way via devres. When a card object is created by this new function, it's released automatically at the device release. It includes also the call of snd_card_free().
However, the story isn't that simple. A caveat is that We have to call snd_card_new(), more specifically, the disconnection part, at very first of the whole resource release procedure. This assures that the exposed devices are deleted and sync with the all accessing processes getting closed.
For achieving it, snd_card_register() adds a new devres action to trigger snd_card_free() automatically when the given card object is a "managed" one. Since usually snd_card_register() is the last step of the initialization, this should work in most cases.
With all these tricks, some drivers can get rid of the whole the driver remove callback.
About a bit of implementation details: the patch adds two new flags to snd_card object, managed and releasing. The former indicates that the object was created via snd_devm_card_new(), and the latter is used for avoiding the double-free of snd_card_free() calls. Both flags are fairly internal and likely uninteresting to normal users.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/core.h | 5 +++ sound/core/init.c | 95 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 4 deletions(-)
In my opinion, the new 'snd_devm_card_new()' is not good in hot-plug scenario. It brings kernel oops for processes to touch released device data relevant to target devices.
For example, for devices connected to each buses, some helper functions are available to up/down reference count of 'struct device':
- PCIe: pci_dev_get()/pci_dev_put()
- USB: usb_get_intf()/usb_put_intf()
- IEEE 1394: fw_unit_get()/fw_unit_put()
In hot-plug scenario, drivers need to increment the reference counter in .probe() callback. In .remove/.disconnect callback, the reference counter should be kept but just set disconnect state to sound card/device instances. When .private_free callback of sound card device, the reference is decremented. This is required to enable userspace applications to handle disconnect processes and avoid kernel oops by touching released device data related to the connected bus.
As a quick glance, existent drivers for devices on PCIe/USB are not programmed with enough care of this point. It's prior to fix them for your 'caveat'.
...but it's likely for me to get wrong understanding design of whole existent driver in sound subsystem. I'm happy to receive your indications against my misunderstanding.
It should work as long as the whole remove procedure is performed after snd_card_free(). With the use of devres, typically you can drop the whole remove() callback, and that's it.
Basically a device hot-unplug is nothing but the driver unbinding from the device. Under the normal situation, the driver core calls its remove() callback, then releases the rest via devres in the reverse order. When remove() is empty, it'll just perform the devres release. So, when snd_card_free() is performed at the beginning of devres release, the call order is as if you were calling snd_card_free() at remove() callback.
The snd_card_free() syncs with the release of the all active files, i.e. it waits until all accesses get released, then proceed to the further procedures to free resources, including the call of private_free. Hence this call itself should be safe, as long as it's called at first.
The rest of resource free is left to devres, and devres guarantees the resource free in the reverse order. That's the reason that it'd be best to use devres for the whole resources. Mixing up both devres and normal resources needs a careful handling of the release order.
thanks,
Takashi
Hi,
On Sep 21 2018 15:32, Takashi Iwai wrote:
On Fri, 21 Sep 2018 05:01:31 +0200, Takashi Sakamoto wrote:
Hi,
On Sep 21 2018 00:54, Takashi Iwai wrote:
Per popular demands, this patch adds a new ALSA core API function, snd_devm_card_new(), to create a snd_card object in a managed way via devres. When a card object is created by this new function, it's released automatically at the device release. It includes also the call of snd_card_free().
However, the story isn't that simple. A caveat is that We have to call snd_card_new(), more specifically, the disconnection part, at very first of the whole resource release procedure. This assures that the exposed devices are deleted and sync with the all accessing processes getting closed.
For achieving it, snd_card_register() adds a new devres action to trigger snd_card_free() automatically when the given card object is a "managed" one. Since usually snd_card_register() is the last step of the initialization, this should work in most cases.
With all these tricks, some drivers can get rid of the whole the driver remove callback.
About a bit of implementation details: the patch adds two new flags to snd_card object, managed and releasing. The former indicates that the object was created via snd_devm_card_new(), and the latter is used for avoiding the double-free of snd_card_free() calls. Both flags are fairly internal and likely uninteresting to normal users.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/core.h | 5 +++ sound/core/init.c | 95 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 4 deletions(-)
In my opinion, the new 'snd_devm_card_new()' is not good in hot-plug scenario. It brings kernel oops for processes to touch released device data relevant to target devices.
For example, for devices connected to each buses, some helper functions are available to up/down reference count of 'struct device':
- PCIe: pci_dev_get()/pci_dev_put()
- USB: usb_get_intf()/usb_put_intf()
- IEEE 1394: fw_unit_get()/fw_unit_put()
In hot-plug scenario, drivers need to increment the reference counter in .probe() callback. In .remove/.disconnect callback, the reference counter should be kept but just set disconnect state to sound card/device instances. When .private_free callback of sound card device, the reference is decremented. This is required to enable userspace applications to handle disconnect processes and avoid kernel oops by touching released device data related to the connected bus.
As a quick glance, existent drivers for devices on PCIe/USB are not programmed with enough care of this point. It's prior to fix them for your 'caveat'.
...but it's likely for me to get wrong understanding design of whole existent driver in sound subsystem. I'm happy to receive your indications against my misunderstanding.
It should work as long as the whole remove procedure is performed after snd_card_free(). With the use of devres, typically you can drop the whole remove() callback, and that's it.
Basically a device hot-unplug is nothing but the driver unbinding from the device. Under the normal situation, the driver core calls its remove() callback, then releases the rest via devres in the reverse order. When remove() is empty, it'll just perform the devres release. So, when snd_card_free() is performed at the beginning of devres release, the call order is as if you were calling snd_card_free() at remove() callback.
I'm OK to discuss for a case of unbinding as more-popular cases.
The snd_card_free() syncs with the release of the all active files, i.e. it waits until all accesses get released, then proceed to the further procedures to free resources, including the call of private_free. Hence this call itself should be safe, as long as it's called at first.
I overlooked that snd_card_free() calls 'wait_for_completion()'. Thanks for your indication. As you said, no worries.
Here I have another concern about timing for processes to return from unbinding operation. For example:
echo '0000:0a:00.1' > /sys/bus/pci/drivers/snd_hda_intel/unbind
This process returns when the other processes close all of ALSA character devices related to the sound card because wait for completion is executed in context of the process. Well-programmed userspace applications are expected to release the character devices when received -ENODEV from ioctl(2) or EPOLLERR/EPOLLNVAL from poll(2) to the character devices.
I don't know exactly that it's acceptable to block a process which performs unbinding, depending on behaviours of the other processes. In a point of safe ABI, it's worth for us to consider or decide a policy for the point.
The rest of resource free is left to devres, and devres guarantees the resource free in the reverse order. That's the reason that it'd be best to use devres for the whole resources. Mixing up both devres and normal resources needs a careful handling of the release order.
Thanks
Takashi Sakamoto
On Sun, 23 Sep 2018 10:08:14 +0200, Takashi Sakamoto wrote:
Hi,
On Sep 21 2018 15:32, Takashi Iwai wrote:
On Fri, 21 Sep 2018 05:01:31 +0200, Takashi Sakamoto wrote:
Hi,
On Sep 21 2018 00:54, Takashi Iwai wrote:
Per popular demands, this patch adds a new ALSA core API function, snd_devm_card_new(), to create a snd_card object in a managed way via devres. When a card object is created by this new function, it's released automatically at the device release. It includes also the call of snd_card_free().
However, the story isn't that simple. A caveat is that We have to call snd_card_new(), more specifically, the disconnection part, at very first of the whole resource release procedure. This assures that the exposed devices are deleted and sync with the all accessing processes getting closed.
For achieving it, snd_card_register() adds a new devres action to trigger snd_card_free() automatically when the given card object is a "managed" one. Since usually snd_card_register() is the last step of the initialization, this should work in most cases.
With all these tricks, some drivers can get rid of the whole the driver remove callback.
About a bit of implementation details: the patch adds two new flags to snd_card object, managed and releasing. The former indicates that the object was created via snd_devm_card_new(), and the latter is used for avoiding the double-free of snd_card_free() calls. Both flags are fairly internal and likely uninteresting to normal users.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/core.h | 5 +++ sound/core/init.c | 95 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 4 deletions(-)
In my opinion, the new 'snd_devm_card_new()' is not good in hot-plug scenario. It brings kernel oops for processes to touch released device data relevant to target devices.
For example, for devices connected to each buses, some helper functions are available to up/down reference count of 'struct device':
- PCIe: pci_dev_get()/pci_dev_put()
- USB: usb_get_intf()/usb_put_intf()
- IEEE 1394: fw_unit_get()/fw_unit_put()
In hot-plug scenario, drivers need to increment the reference counter in .probe() callback. In .remove/.disconnect callback, the reference counter should be kept but just set disconnect state to sound card/device instances. When .private_free callback of sound card device, the reference is decremented. This is required to enable userspace applications to handle disconnect processes and avoid kernel oops by touching released device data related to the connected bus.
As a quick glance, existent drivers for devices on PCIe/USB are not programmed with enough care of this point. It's prior to fix them for your 'caveat'.
...but it's likely for me to get wrong understanding design of whole existent driver in sound subsystem. I'm happy to receive your indications against my misunderstanding.
It should work as long as the whole remove procedure is performed after snd_card_free(). With the use of devres, typically you can drop the whole remove() callback, and that's it.
Basically a device hot-unplug is nothing but the driver unbinding from the device. Under the normal situation, the driver core calls its remove() callback, then releases the rest via devres in the reverse order. When remove() is empty, it'll just perform the devres release. So, when snd_card_free() is performed at the beginning of devres release, the call order is as if you were calling snd_card_free() at remove() callback.
I'm OK to discuss for a case of unbinding as more-popular cases.
The snd_card_free() syncs with the release of the all active files, i.e. it waits until all accesses get released, then proceed to the further procedures to free resources, including the call of private_free. Hence this call itself should be safe, as long as it's called at first.
I overlooked that snd_card_free() calls 'wait_for_completion()'. Thanks for your indication. As you said, no worries.
Here I have another concern about timing for processes to return from unbinding operation. For example:
echo '0000:0a:00.1' > /sys/bus/pci/drivers/snd_hda_intel/unbind
This process returns when the other processes close all of ALSA character devices related to the sound card because wait for completion is executed in context of the process. Well-programmed userspace applications are expected to release the character devices when received -ENODEV from ioctl(2) or EPOLLERR/EPOLLNVAL from poll(2) to the character devices.
I don't know exactly that it's acceptable to block a process which performs unbinding, depending on behaviours of the other processes. In a point of safe ABI, it's worth for us to consider or decide a policy for the point.
Right, the behavior of unbind is currently sub-optimal. But basically it's above this devres patch series; the unbind behavior itself doesn't change no matter whether the resource is released via devres or not. So we can keep discussing about this, but maybe in another thread.
When we see unbind as a sort of hot-unplug action, it's understandable that unbind never returns an error. And that's the current situation, and it implies that every driver needs to take care of all pending resources by itself. Hence, you have only two choices: block and sync with the pending resources, or make everything in async.
As of now, the most of drivers take the former approach -- at least for sound stuff -- just because of simplicity. Admittedly, the latter approach would look better from the user-space POV. However, it'll be far complicated in the actual implementation.
Maybe another consideration would be to allow unbind action actually returning -EBUSY error. This option would require the driver base code change, and above all, the consensus from other parties.
thanks,
Takashi
Hi,
Thanks for you to be involved in this issue.
On Oct 3 2018 01:30, Takashi Iwai wrote:
The snd_card_free() syncs with the release of the all active files, i.e. it waits until all accesses get released, then proceed to the further procedures to free resources, including the call of private_free. Hence this call itself should be safe, as long as it's called at first.
I overlooked that snd_card_free() calls 'wait_for_completion()'. Thanks for your indication. As you said, no worries.
Here I have another concern about timing for processes to return from unbinding operation. For example:
echo '0000:0a:00.1' > /sys/bus/pci/drivers/snd_hda_intel/unbind
This process returns when the other processes close all of ALSA character devices related to the sound card because wait for completion is executed in context of the process. Well-programmed userspace applications are expected to release the character devices when received -ENODEV from ioctl(2) or EPOLLERR/EPOLLNVAL from poll(2) to the character devices.
I don't know exactly that it's acceptable to block a process which performs unbinding, depending on behaviours of the other processes. In a point of safe ABI, it's worth for us to consider or decide a policy for the point.
Right, the behavior of unbind is currently sub-optimal. But basically it's above this devres patch series; the unbind behavior itself doesn't change no matter whether the resource is released via devres or not. So we can keep discussing about this, but maybe in another thread.
Indeed.
For this patch, I have another concern and will post it soon (for a helper function to release memory object immediately).
When we see unbind as a sort of hot-unplug action, it's understandable that unbind never returns an error. And that's the current situation, and it implies that every driver needs to take care of all pending resources by itself. Hence, you have only two choices: block and sync with the pending resources, or make everything in async.
As of now, the most of drivers take the former approach -- at least for sound stuff -- just because of simplicity. Admittedly, the latter approach would look better from the user-space POV. However, it'll be far complicated in the actual implementation.
I note that all drivers in ALSA firewire stack are implemented according to the 'async' scenario. Usage of reference counting of struct device for units on IEEE 1394 bus is used to achieve it.
Maybe another consideration would be to allow unbind action actually returning -EBUSY error. This option would require the driver base code change, and above all, the consensus from other parties.
At present, 'unbind_store()' in 'drivers/base/bus.c'[1] can return -ENODEV if error, or the length of given string if success. It's possible for us to change this function to return -EBUSY when the target device is still referred after a call of 'device_release_driver()'. But this is wider issue in Linux DD and should be discussed in a right place.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/driv...
Thanks
Takashi Sakamoto
Hi,
On Sep 21 2018 00:54, Takashi Iwai wrote:
Per popular demands, this patch adds a new ALSA core API function, snd_devm_card_new(), to create a snd_card object in a managed way via devres. When a card object is created by this new function, it's released automatically at the device release. It includes also the call of snd_card_free().
However, the story isn't that simple. A caveat is that We have to call snd_card_new(), more specifically, the disconnection part, at very first of the whole resource release procedure. This assures that the exposed devices are deleted and sync with the all accessing processes getting closed.
For achieving it, snd_card_register() adds a new devres action to trigger snd_card_free() automatically when the given card object is a "managed" one. Since usually snd_card_register() is the last step of the initialization, this should work in most cases.
With all these tricks, some drivers can get rid of the whole the driver remove callback.
About a bit of implementation details: the patch adds two new flags to snd_card object, managed and releasing. The former indicates that the object was created via snd_devm_card_new(), and the latter is used for avoiding the double-free of snd_card_free() calls. Both flags are fairly internal and likely uninteresting to normal users.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/core.h | 5 +++ sound/core/init.c | 95 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 4 deletions(-)
I've investigated to use the new function, 'snd_devm_card_new()', and have a concern about timing to release memory object for sound card instance (and tailing private data) in error path between calls of the function and 'snd_card_register()'.
In the error path, 'snd_card_free()' is called to release instances associated to the sound card instance as expected, however memory object for the sound card instance is not released yet because in usual cases associated device structure does not lost its reference count in this timing. It's released when the associated device is removed. This means that the memory object remains against its practicality during lifetime of the device.
This is not a bug itself, so I have no opposition to this patchset. But it's reasonable for us to have a bit time to consider about it, IMO.
Thanks
Takashi Sakamoto
On Wed, 03 Oct 2018 10:12:34 +0200, Takashi Sakamoto wrote:
Hi,
On Sep 21 2018 00:54, Takashi Iwai wrote:
Per popular demands, this patch adds a new ALSA core API function, snd_devm_card_new(), to create a snd_card object in a managed way via devres. When a card object is created by this new function, it's released automatically at the device release. It includes also the call of snd_card_free().
However, the story isn't that simple. A caveat is that We have to call snd_card_new(), more specifically, the disconnection part, at very first of the whole resource release procedure. This assures that the exposed devices are deleted and sync with the all accessing processes getting closed.
For achieving it, snd_card_register() adds a new devres action to trigger snd_card_free() automatically when the given card object is a "managed" one. Since usually snd_card_register() is the last step of the initialization, this should work in most cases.
With all these tricks, some drivers can get rid of the whole the driver remove callback.
About a bit of implementation details: the patch adds two new flags to snd_card object, managed and releasing. The former indicates that the object was created via snd_devm_card_new(), and the latter is used for avoiding the double-free of snd_card_free() calls. Both flags are fairly internal and likely uninteresting to normal users.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/core.h | 5 +++ sound/core/init.c | 95 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 4 deletions(-)
I've investigated to use the new function, 'snd_devm_card_new()', and have a concern about timing to release memory object for sound card instance (and tailing private data) in error path between calls of the function and 'snd_card_register()'.
In the error path, 'snd_card_free()' is called to release instances associated to the sound card instance as expected, however memory object for the sound card instance is not released yet because in usual cases associated device structure does not lost its reference count in this timing. It's released when the associated device is removed. This means that the memory object remains against its practicality during lifetime of the device.
This is not a bug itself, so I have no opposition to this patchset. But it's reasonable for us to have a bit time to consider about it, IMO.
Well, it's exactly same as the usual devm_kzalloc() & co, which have been already deployed at various places (even you posted a new patch series for using them :) The card memory is tied with the device, and it's natural to align with the lifetime of the device.
Of course, the memory release timing would be slightly differed by this code change, but this shouldn't be a problem, as long as it's about the memory release. That is:
- In the old situation, kfree() is called from snd_card_free() via driver remove callback called from __device_release_driver().
- In the devres mode, kfree() is called via devres_release_all() that is called in __device_release_driver() right after drv->remove() call above.
So the difference of the release timing is only between these call paths.
thanks,
Takashi
Hi,
On Oct 3 2018 19:30, Takashi Iwai wrote:
On Wed, 03 Oct 2018 10:12:34 +0200,
On Sep 21 2018 00:54, Takashi Iwai wrote:
Per popular demands, this patch adds a new ALSA core API function, snd_devm_card_new(), to create a snd_card object in a managed way via devres. When a card object is created by this new function, it's released automatically at the device release. It includes also the call of snd_card_free().
However, the story isn't that simple. A caveat is that We have to call snd_card_new(), more specifically, the disconnection part, at very first of the whole resource release procedure. This assures that the exposed devices are deleted and sync with the all accessing processes getting closed.
For achieving it, snd_card_register() adds a new devres action to trigger snd_card_free() automatically when the given card object is a "managed" one. Since usually snd_card_register() is the last step of the initialization, this should work in most cases.
With all these tricks, some drivers can get rid of the whole the driver remove callback.
About a bit of implementation details: the patch adds two new flags to snd_card object, managed and releasing. The former indicates that the object was created via snd_devm_card_new(), and the latter is used for avoiding the double-free of snd_card_free() calls. Both flags are fairly internal and likely uninteresting to normal users.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/core.h | 5 +++ sound/core/init.c | 95 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 4 deletions(-)
I've investigated to use the new function, 'snd_devm_card_new()', and have a concern about timing to release memory object for sound card instance (and tailing private data) in error path between calls of the function and 'snd_card_register()'.
In the error path, 'snd_card_free()' is called to release instances associated to the sound card instance as expected, however memory object for the sound card instance is not released yet because in usual cases associated device structure does not lost its reference count in this timing. It's released when the associated device is removed. This means that the memory object remains against its practicality during lifetime of the device.
This is not a bug itself, so I have no opposition to this patchset. But it's reasonable for us to have a bit time to consider about it, IMO.
Well, it's exactly same as the usual devm_kzalloc() & co, which have been already deployed at various places (even you posted a new patch series for using them :) The card memory is tied with the device, and it's natural to align with the lifetime of the device.
I have this concern against the usage of devres for a long time.
A device can be referred by several handlers; e.g. character device driver and kernel driver. An example is USB devices. Userspace applications can transfer message to the device via character device. At the same time, iface driver in kernel land can do the same work.
The lifetime of device is apparently different from each of the handler. Even if kernel driver returns negative value in its .probe callback, the other handlers can refer to and use it.
If the kernel driver leave the devm-allocated memory in error path, it remains till all of the handler release its reference to the device. Even it the memory objects are maintained by devres, several small parts of kernel logical space is unavailable. This is similar to memory leak, in my view.
Regards
Takashi Sakamoto
Hi,
On Oct 3 2018 22:14, Takashi Sakamoto wrote:
On Oct 3 2018 19:30, Takashi Iwai wrote:
On Wed, 03 Oct 2018 10:12:34 +0200,
I've investigated to use the new function, 'snd_devm_card_new()', and have a concern about timing to release memory object for sound card instance (and tailing private data) in error path between calls of the function and 'snd_card_register()'.
In the error path, 'snd_card_free()' is called to release instances associated to the sound card instance as expected, however memory object for the sound card instance is not released yet because in usual cases associated device structure does not lost its reference count in this timing. It's released when the associated device is removed. This means that the memory object remains against its practicality during lifetime of the device.
This is not a bug itself, so I have no opposition to this patchset. But it's reasonable for us to have a bit time to consider about it, IMO.
Well, it's exactly same as the usual devm_kzalloc() & co, which have been already deployed at various places (even you posted a new patch series for using them :) The card memory is tied with the device, and it's natural to align with the lifetime of the device.
I have this concern against the usage of devres for a long time.
A device can be referred by several handlers; e.g. character device driver and kernel driver. An example is USB devices. Userspace applications can transfer message to the device via character device. At the same time, iface driver in kernel land can do the same work.
The lifetime of device is apparently different from each of the handler. Even if kernel driver returns negative value in its .probe callback, the other handlers can refer to and use it.
If the kernel driver leave the devm-allocated memory in error path, it remains till all of the handler release its reference to the device. Even it the memory objects are maintained by devres, several small parts of kernel logical space is unavailable. This is similar to memory leak, in my view.
I wrote a patch to e06afb87065c (HEAD of 'topic/devres' branch) for this purpose. I've not tested thoroughly but expect it solves the above issue.
======== 8< --------
From 2ef56174ea3c07eb5d6c2adb60573321c04a02af Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto o-takashi@sakamocchi.jp Date: Wed, 3 Oct 2018 23:44:43 +0900 Subject: [PATCH] ALSA: core: release memory object for sound card immediately in error path for a case of devm usage
Drivers can snd_card_free() in error path between calls of snd_devm_card_new() and snd_card_register(), In this case, memory object for sound card structure is not deallocated immediately because it's associated to target device by devres framework. The memory object remains till the target device is released. This looks like memory leak.
This commit postpones association of the memory object to target device till a call of snd_card_register(). In error path, a call of snd_card_free() releases the memory object in the last part of snd_card_do_free(). --- sound/core/init.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/sound/core/init.c b/sound/core/init.c index 2eade57db4b4..49cb22a735c1 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -272,7 +272,6 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid, return err; }
- devres_add(parent, card); *card_ret = card; return 0; } @@ -569,6 +568,8 @@ static int snd_card_do_free(struct snd_card *card) complete(card->release_completion); if (!card->managed) kfree(card); + else if (!devres_find(card->dev, __snd_card_release, NULL, NULL)) + devres_free(card); return 0; }
@@ -849,15 +850,6 @@ int snd_card_register(struct snd_card *card) if (err < 0) return err; card->registered = true; - } else { - if (card->managed) - devm_remove_action(card->dev, trigger_card_free, card); - } - - if (card->managed) { - err = devm_add_action(card->dev, trigger_card_free, card); - if (err < 0) - return err; }
if ((err = snd_device_register_all(card)) < 0) @@ -887,6 +879,14 @@ int snd_card_register(struct snd_card *card) if (snd_mixer_oss_notify_callback) snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_REGISTER); #endif + if (card->managed && + !devres_find(card->dev, __snd_card_release, NULL, NULL)) { + err = devm_add_action(card->dev, trigger_card_free, card); + if (err < 0) + return err; + devres_add(card->dev, card); + } + return 0; } EXPORT_SYMBOL(snd_card_register);
On Wed, 03 Oct 2018 17:02:59 +0200, Takashi Sakamoto wrote:
Hi,
On Oct 3 2018 22:14, Takashi Sakamoto wrote:
On Oct 3 2018 19:30, Takashi Iwai wrote:
On Wed, 03 Oct 2018 10:12:34 +0200,
I've investigated to use the new function, 'snd_devm_card_new()', and have a concern about timing to release memory object for sound card instance (and tailing private data) in error path between calls of the function and 'snd_card_register()'.
In the error path, 'snd_card_free()' is called to release instances associated to the sound card instance as expected, however memory object for the sound card instance is not released yet because in usual cases associated device structure does not lost its reference count in this timing. It's released when the associated device is removed. This means that the memory object remains against its practicality during lifetime of the device.
This is not a bug itself, so I have no opposition to this patchset. But it's reasonable for us to have a bit time to consider about it, IMO.
Well, it's exactly same as the usual devm_kzalloc() & co, which have been already deployed at various places (even you posted a new patch series for using them :) The card memory is tied with the device, and it's natural to align with the lifetime of the device.
I have this concern against the usage of devres for a long time.
A device can be referred by several handlers; e.g. character device driver and kernel driver. An example is USB devices. Userspace applications can transfer message to the device via character device. At the same time, iface driver in kernel land can do the same work.
The lifetime of device is apparently different from each of the handler. Even if kernel driver returns negative value in its .probe callback, the other handlers can refer to and use it.
If the kernel driver leave the devm-allocated memory in error path, it remains till all of the handler release its reference to the device. Even it the memory objects are maintained by devres, several small parts of kernel logical space is unavailable. This is similar to memory leak, in my view.
I wrote a patch to e06afb87065c (HEAD of 'topic/devres' branch) for this purpose. I've not tested thoroughly but expect it solves the above issue.
======== 8< --------
From 2ef56174ea3c07eb5d6c2adb60573321c04a02af Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto o-takashi@sakamocchi.jp Date: Wed, 3 Oct 2018 23:44:43 +0900 Subject: [PATCH] ALSA: core: release memory object for sound card immediately in error path for a case of devm usage
Drivers can snd_card_free() in error path between calls of snd_devm_card_new() and snd_card_register(), In this case, memory object for sound card structure is not deallocated immediately because it's associated to target device by devres framework. The memory object remains till the target device is released. This looks like memory leak.
Not really, this is *intentional* and designed behavior.
Imagine the case if you have a device-managed irq handler and a device-managed IO memory. They are not freed at snd_card_free() call time, but at a later stage of devres release. Hence the *all* devres-managed resources have to be released there.
thanks,
Takashi
This commit postpones association of the memory object to target device till a call of snd_card_register(). In error path, a call of snd_card_free() releases the memory object in the last part of snd_card_do_free().
sound/core/init.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/sound/core/init.c b/sound/core/init.c index 2eade57db4b4..49cb22a735c1 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -272,7 +272,6 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid, return err; }
- devres_add(parent, card); *card_ret = card; return 0;
} @@ -569,6 +568,8 @@ static int snd_card_do_free(struct snd_card *card) complete(card->release_completion); if (!card->managed) kfree(card);
- else if (!devres_find(card->dev, __snd_card_release, NULL, NULL))
devres_free(card);}
@@ -849,15 +850,6 @@ int snd_card_register(struct snd_card *card) if (err < 0) return err; card->registered = true;
} else {
if (card->managed)devm_remove_action(card->dev, trigger_card_free, card);}
if (card->managed) {
err = devm_add_action(card->dev, trigger_card_free, card);if (err < 0)return err;}
if ((err = snd_device_register_all(card)) < 0)
@@ -887,6 +879,14 @@ int snd_card_register(struct snd_card *card) if (snd_mixer_oss_notify_callback) snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_REGISTER); #endif
- if (card->managed &&
!devres_find(card->dev, __snd_card_release, NULL, NULL)) {err = devm_add_action(card->dev, trigger_card_free, card);if (err < 0)return err;devres_add(card->dev, card);- }
- return 0;
} EXPORT_SYMBOL(snd_card_register); -- 2.17.1
Thanks
Takashi Sakamoto
Hi,
On Oct 4 2018 00:37, Takashi Iwai wrote:
On Wed, 03 Oct 2018 17:02:59 +0200, Takashi Sakamoto wrote:
Hi,
On Oct 3 2018 22:14, Takashi Sakamoto wrote:
On Oct 3 2018 19:30, Takashi Iwai wrote:
On Wed, 03 Oct 2018 10:12:34 +0200,
I've investigated to use the new function, 'snd_devm_card_new()', and have a concern about timing to release memory object for sound card instance (and tailing private data) in error path between calls of the function and 'snd_card_register()'.
In the error path, 'snd_card_free()' is called to release instances associated to the sound card instance as expected, however memory object for the sound card instance is not released yet because in usual cases associated device structure does not lost its reference count in this timing. It's released when the associated device is removed. This means that the memory object remains against its practicality during lifetime of the device.
This is not a bug itself, so I have no opposition to this patchset. But it's reasonable for us to have a bit time to consider about it, IMO.
Well, it's exactly same as the usual devm_kzalloc() & co, which have been already deployed at various places (even you posted a new patch series for using them :) The card memory is tied with the device, and it's natural to align with the lifetime of the device.
I have this concern against the usage of devres for a long time.
A device can be referred by several handlers; e.g. character device driver and kernel driver. An example is USB devices. Userspace applications can transfer message to the device via character device. At the same time, iface driver in kernel land can do the same work.
The lifetime of device is apparently different from each of the handler. Even if kernel driver returns negative value in its .probe callback, the other handlers can refer to and use it.
If the kernel driver leave the devm-allocated memory in error path, it remains till all of the handler release its reference to the device. Even it the memory objects are maintained by devres, several small parts of kernel logical space is unavailable. This is similar to memory leak, in my view.
I wrote a patch to e06afb87065c (HEAD of 'topic/devres' branch) for this purpose. I've not tested thoroughly but expect it solves the above issue.
======== 8< --------
From 2ef56174ea3c07eb5d6c2adb60573321c04a02af Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto o-takashi@sakamocchi.jp Date: Wed, 3 Oct 2018 23:44:43 +0900 Subject: [PATCH] ALSA: core: release memory object for sound card immediately in error path for a case of devm usage
Drivers can snd_card_free() in error path between calls of snd_devm_card_new() and snd_card_register(), In this case, memory object for sound card structure is not deallocated immediately because it's associated to target device by devres framework. The memory object remains till the target device is released. This looks like memory leak.
Not really, this is *intentional* and designed behavior.
Imagine the case if you have a device-managed irq handler and a device-managed IO memory. They are not freed at snd_card_free() call time, but at a later stage of devres release. Hence the *all* devres-managed resources have to be released there.
Indeed. This patch includes no extra care to your patch 01/03/04/05 because I've already proposed a solution to use of reference counting without associating sound card instance to target device.
I decide not to use 'snd_devm_card_new()' for all drivers in ALSA firewire stack because of several issues I've addressed. Thanks for your time to discuss about them.
This commit postpones association of the memory object to target device till a call of snd_card_register(). In error path, a call of snd_card_free() releases the memory object in the last part of snd_card_do_free().
sound/core/init.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/sound/core/init.c b/sound/core/init.c index 2eade57db4b4..49cb22a735c1 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -272,7 +272,6 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid, return err; }
- devres_add(parent, card); *card_ret = card; return 0; }
@@ -569,6 +568,8 @@ static int snd_card_do_free(struct snd_card *card) complete(card->release_completion); if (!card->managed) kfree(card);
- else if (!devres_find(card->dev, __snd_card_release, NULL, NULL))
devres_free(card);@@ -849,15 +850,6 @@ int snd_card_register(struct snd_card *card) if (err < 0) return err; card->registered = true;
} else {
if (card->managed)devm_remove_action(card->dev, trigger_card_free, card);}
if (card->managed) {
err = devm_add_action(card->dev, trigger_card_free, card);if (err < 0)return err;}
if ((err = snd_device_register_all(card)) < 0)
@@ -887,6 +879,14 @@ int snd_card_register(struct snd_card *card) if (snd_mixer_oss_notify_callback) snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_REGISTER); #endif
- if (card->managed &&
!devres_find(card->dev, __snd_card_release, NULL, NULL)) {err = devm_add_action(card->dev, trigger_card_free, card);if (err < 0)return err;devres_add(card->dev, card);- }
- return 0; } EXPORT_SYMBOL(snd_card_register);
-- 2.17.1
Regards
Takashi Sakamoto
On Wed, 03 Oct 2018 15:14:50 +0200, Takashi Sakamoto wrote:
Hi,
On Oct 3 2018 19:30, Takashi Iwai wrote:
On Wed, 03 Oct 2018 10:12:34 +0200,
On Sep 21 2018 00:54, Takashi Iwai wrote:
Per popular demands, this patch adds a new ALSA core API function, snd_devm_card_new(), to create a snd_card object in a managed way via devres. When a card object is created by this new function, it's released automatically at the device release. It includes also the call of snd_card_free().
However, the story isn't that simple. A caveat is that We have to call snd_card_new(), more specifically, the disconnection part, at very first of the whole resource release procedure. This assures that the exposed devices are deleted and sync with the all accessing processes getting closed.
For achieving it, snd_card_register() adds a new devres action to trigger snd_card_free() automatically when the given card object is a "managed" one. Since usually snd_card_register() is the last step of the initialization, this should work in most cases.
With all these tricks, some drivers can get rid of the whole the driver remove callback.
About a bit of implementation details: the patch adds two new flags to snd_card object, managed and releasing. The former indicates that the object was created via snd_devm_card_new(), and the latter is used for avoiding the double-free of snd_card_free() calls. Both flags are fairly internal and likely uninteresting to normal users.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/core.h | 5 +++ sound/core/init.c | 95 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 4 deletions(-)
I've investigated to use the new function, 'snd_devm_card_new()', and have a concern about timing to release memory object for sound card instance (and tailing private data) in error path between calls of the function and 'snd_card_register()'.
In the error path, 'snd_card_free()' is called to release instances associated to the sound card instance as expected, however memory object for the sound card instance is not released yet because in usual cases associated device structure does not lost its reference count in this timing. It's released when the associated device is removed. This means that the memory object remains against its practicality during lifetime of the device.
This is not a bug itself, so I have no opposition to this patchset. But it's reasonable for us to have a bit time to consider about it, IMO.
Well, it's exactly same as the usual devm_kzalloc() & co, which have been already deployed at various places (even you posted a new patch series for using them :) The card memory is tied with the device, and it's natural to align with the lifetime of the device.
I have this concern against the usage of devres for a long time.
A device can be referred by several handlers; e.g. character device driver and kernel driver. An example is USB devices. Userspace applications can transfer message to the device via character device. At the same time, iface driver in kernel land can do the same work.
The lifetime of device is apparently different from each of the handler. Even if kernel driver returns negative value in its .probe callback, the other handlers can refer to and use it.
If the kernel driver leave the devm-allocated memory in error path, it remains till all of the handler release its reference to the device. Even it the memory objects are maintained by devres, several small parts of kernel logical space is unavailable. This is similar to memory leak, in my view.
Well, this thing can be better discussed on LKML or in other appropriate places.
The devres usage is tied with the assigned device, and if the question comes to the multiple devices in hierarchy, its handling becomes more sensitive, of course. It's no silver bullet that works perfectly in every single case.
But I believe most of devres usages are fine. Some usages might be keeping the memory longer than needed, but they are no leaks. If the devices are unbound from the driver code, they are assured to be freed, after all.
If you have a real concern, please raise the issue to the corresponding subsystems.
thanks,
Takashi
This patch refactors the intel8x0 and intel8x0m driver codes using devres and gets rid of the driver remove callback.
The conversion is fairly straightforward: each API call is replaced with the device-managed API function, e.g. pci_enable_device() -> pcim_enable_device(), and so on. The buffer descriptor list is allocated with a new API, snd_devm_alloc_pages().
A slight code structure change is that the intel8x0 object is allocated as a card's private_data instead of the own lowlevel snd_device object. This simplifies the resource management. And, the take-down procedure is triggered via card->private_free, and it's registered at the end of the whole initialization, i.e. after the all resources get properly managed.
The only not-devres-managed resource is the irq handler. Since we need to release at suspend and re-acquire at resume (otherwise something weird happens on some machines), this is still managed manually. But the rest are all freed automatically.
The end result is a good amount of code reduction.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/intel8x0.c | 143 ++++++++++++++---------------------------- sound/pci/intel8x0m.c | 143 ++++++++++++++---------------------------- 2 files changed, 92 insertions(+), 194 deletions(-)
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c index 9517f9b8f1d4..662d5c9109f5 100644 --- a/sound/pci/intel8x0.c +++ b/sound/pci/intel8x0.c @@ -415,7 +415,7 @@ struct intel8x0 { spinlock_t reg_lock; u32 bdbars_count; - struct snd_dma_buffer bdbars; + struct snd_dma_buffer *bdbars; u32 int_sta_reg; /* interrupt status register */ u32 int_sta_mask; /* interrupt status mask */ }; @@ -2567,8 +2567,9 @@ static int snd_intel8x0_chip_init(struct intel8x0 *chip, int probing) return 0; }
-static int snd_intel8x0_free(struct intel8x0 *chip) +static void snd_intel8x0_free(struct snd_card *card) { + struct intel8x0 *chip = card->private_data; unsigned int i;
if (chip->irq < 0) @@ -2591,16 +2592,6 @@ static int snd_intel8x0_free(struct intel8x0 *chip) __hw_end: if (chip->irq >= 0) free_irq(chip->irq, chip); - if (chip->bdbars.area) - snd_dma_free_pages(&chip->bdbars); - if (chip->addr) - pci_iounmap(chip->pci, chip->addr); - if (chip->bmaddr) - pci_iounmap(chip->pci, chip->bmaddr); - pci_release_regions(chip->pci); - pci_disable_device(chip->pci); - kfree(chip); - return 0; }
#ifdef CONFIG_PM_SLEEP @@ -2871,12 +2862,6 @@ static void snd_intel8x0_proc_init(struct intel8x0 *chip) snd_info_set_text_ops(entry, chip, snd_intel8x0_proc_read); }
-static int snd_intel8x0_dev_free(struct snd_device *device) -{ - struct intel8x0 *chip = device->device_data; - return snd_intel8x0_free(chip); -} - struct ich_reg_info { unsigned int int_sta_mask; unsigned int offset; @@ -2920,19 +2905,15 @@ static int snd_intel8x0_inside_vm(struct pci_dev *pci) return result; }
-static int snd_intel8x0_create(struct snd_card *card, - struct pci_dev *pci, - unsigned long device_type, - struct intel8x0 **r_intel8x0) +static int snd_intel8x0_init(struct snd_card *card, + struct pci_dev *pci, + unsigned long device_type) { - struct intel8x0 *chip; + struct intel8x0 *chip = card->private_data; int err; unsigned int i; unsigned int int_sta_masks; struct ichdev *ichdev; - static struct snd_device_ops ops = { - .dev_free = snd_intel8x0_dev_free, - };
static unsigned int bdbars[] = { 3, /* DEVICE_INTEL */ @@ -2965,16 +2946,10 @@ static int snd_intel8x0_create(struct snd_card *card, }; struct ich_reg_info *tbl;
- *r_intel8x0 = NULL; - - if ((err = pci_enable_device(pci)) < 0) + err = pcim_enable_device(pci); + if (err < 0) return err;
- chip = kzalloc(sizeof(*chip), GFP_KERNEL); - if (chip == NULL) { - pci_disable_device(pci); - return -ENOMEM; - } spin_lock_init(&chip->reg_lock); chip->device_type = device_type; chip->card = card; @@ -2999,38 +2974,24 @@ static int snd_intel8x0_create(struct snd_card *card, pci->device == PCI_DEVICE_ID_INTEL_440MX) chip->fix_nocache = 1; /* enable workaround */
- if ((err = pci_request_regions(pci, card->shortname)) < 0) { - kfree(chip); - pci_disable_device(pci); + err = pcim_iomap_regions(pci, -1, card->shortname); + if (err < 0) return err; - }
if (device_type == DEVICE_ALI) { /* ALI5455 has no ac97 region */ - chip->bmaddr = pci_iomap(pci, 0, 0); - goto port_inited; - } - - if (pci_resource_flags(pci, 2) & IORESOURCE_MEM) /* ICH4 and Nforce */ - chip->addr = pci_iomap(pci, 2, 0); - else - chip->addr = pci_iomap(pci, 0, 0); - if (!chip->addr) { - dev_err(card->dev, "AC'97 space ioremap problem\n"); - snd_intel8x0_free(chip); - return -EIO; + chip->bmaddr = pcim_iomap_table(pci)[0]; + } else { + if (pci_resource_flags(pci, 2) & IORESOURCE_MEM) /* ICH4 and Nforce */ + chip->addr = pcim_iomap_table(pci)[2]; + else + chip->addr = pcim_iomap_table(pci)[0]; + if (pci_resource_flags(pci, 3) & IORESOURCE_MEM) /* ICH4 */ + chip->bmaddr = pcim_iomap_table(pci)[3]; + else + chip->bmaddr = pcim_iomap_table(pci)[1]; } - if (pci_resource_flags(pci, 3) & IORESOURCE_MEM) /* ICH4 */ - chip->bmaddr = pci_iomap(pci, 3, 0); - else - chip->bmaddr = pci_iomap(pci, 1, 0);
- port_inited: - if (!chip->bmaddr) { - dev_err(card->dev, "Controller space ioremap problem\n"); - snd_intel8x0_free(chip); - return -EIO; - } chip->bdbars_count = bdbars[device_type];
/* initialize offsets */ @@ -3066,10 +3027,10 @@ static int snd_intel8x0_create(struct snd_card *card,
/* allocate buffer descriptor lists */ /* the start of each lists must be aligned to 8 bytes */ - if (snd_dma_alloc_pages(intel8x0_dma_type(chip), snd_dma_pci_data(pci), - chip->bdbars_count * sizeof(u32) * ICH_MAX_FRAGS * 2, - &chip->bdbars) < 0) { - snd_intel8x0_free(chip); + chip->bdbars = snd_devm_alloc_pages(&pci->dev, intel8x0_dma_type(chip), + chip->bdbars_count * sizeof(u32) * + ICH_MAX_FRAGS * 2); + if (IS_ERR(chip->bdbars)) { dev_err(card->dev, "cannot allocate buffer descriptors\n"); return -ENOMEM; } @@ -3078,9 +3039,9 @@ static int snd_intel8x0_create(struct snd_card *card, int_sta_masks = 0; for (i = 0; i < chip->bdbars_count; i++) { ichdev = &chip->ichd[i]; - ichdev->bdbar = ((__le32 *)chip->bdbars.area) + + ichdev->bdbar = ((__le32 *)chip->bdbars->area) + (i * ICH_MAX_FRAGS * 2); - ichdev->bdbar_addr = chip->bdbars.addr + + ichdev->bdbar_addr = chip->bdbars->addr + (i * sizeof(u32) * ICH_MAX_FRAGS * 2); int_sta_masks |= ichdev->int_sta_mask; } @@ -3113,26 +3074,23 @@ static int snd_intel8x0_create(struct snd_card *card, for (i = 0; i < chip->max_codecs; i++) chip->codec_isr_bits |= chip->codec_bit[i];
- if ((err = snd_intel8x0_chip_init(chip, 1)) < 0) { - snd_intel8x0_free(chip); + err = snd_intel8x0_chip_init(chip, 1); + if (err < 0) return err; - }
/* request irq after initializaing int_sta_mask, etc */ + /* NOTE: we don't use devm version here since it's released / + * re-acquired in PM callbacks. + * It's released explicitly in snd_intel8x0_free(), too. + */ if (request_irq(pci->irq, snd_intel8x0_interrupt, IRQF_SHARED, KBUILD_MODNAME, chip)) { dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); - snd_intel8x0_free(chip); return -EBUSY; } chip->irq = pci->irq; + card->private_free = snd_intel8x0_free;
- if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { - snd_intel8x0_free(chip); - return err; - } - - *r_intel8x0 = chip; return 0; }
@@ -3198,9 +3156,11 @@ static int snd_intel8x0_probe(struct pci_dev *pci, int err; struct shortname_table *name;
- err = snd_card_new(&pci->dev, index, id, THIS_MODULE, 0, &card); + err = snd_devm_card_new(&pci->dev, index, id, THIS_MODULE, + sizeof(*chip), &card); if (err < 0) return err; + chip = card->private_data;
if (spdif_aclink < 0) spdif_aclink = check_default_spdif_aclink(pci); @@ -3234,21 +3194,16 @@ static int snd_intel8x0_probe(struct pci_dev *pci, buggy_irq = 0; }
- if ((err = snd_intel8x0_create(card, pci, pci_id->driver_data, - &chip)) < 0) { - snd_card_free(card); + err = snd_intel8x0_init(card, pci, pci_id->driver_data); + if (err < 0) return err; - } - card->private_data = chip;
- if ((err = snd_intel8x0_mixer(chip, ac97_clock, ac97_quirk)) < 0) { - snd_card_free(card); + err = snd_intel8x0_mixer(chip, ac97_clock, ac97_quirk); + if (err < 0) return err; - } - if ((err = snd_intel8x0_pcm(chip)) < 0) { - snd_card_free(card); + err = snd_intel8x0_pcm(chip); + if (err < 0) return err; - } snd_intel8x0_proc_init(chip);
@@ -3265,24 +3220,18 @@ static int snd_intel8x0_probe(struct pci_dev *pci, } }
- if ((err = snd_card_register(card)) < 0) { - snd_card_free(card); + err = snd_card_register(card); + if (err < 0) return err; - } + pci_set_drvdata(pci, card); return 0; }
-static void snd_intel8x0_remove(struct pci_dev *pci) -{ - snd_card_free(pci_get_drvdata(pci)); -} - static struct pci_driver intel8x0_driver = { .name = KBUILD_MODNAME, .id_table = snd_intel8x0_ids, .probe = snd_intel8x0_probe, - .remove = snd_intel8x0_remove, .driver = { .pm = INTEL8X0_PM_OPS, }, diff --git a/sound/pci/intel8x0m.c b/sound/pci/intel8x0m.c index c84629190cba..22fc834f48c0 100644 --- a/sound/pci/intel8x0m.c +++ b/sound/pci/intel8x0m.c @@ -212,7 +212,7 @@ struct intel8x0m {
spinlock_t reg_lock; - struct snd_dma_buffer bdbars; + struct snd_dma_buffer *bdbars; u32 bdbars_count; u32 int_sta_reg; /* interrupt status register */ u32 int_sta_mask; /* interrupt status mask */ @@ -990,8 +990,9 @@ static int snd_intel8x0m_chip_init(struct intel8x0m *chip, int probing) return 0; }
-static int snd_intel8x0m_free(struct intel8x0m *chip) +static void snd_intel8x0m_free(struct snd_card *card) { + struct intel8x0m *chip = card->private_data; unsigned int i;
if (chip->irq < 0) @@ -1005,16 +1006,6 @@ static int snd_intel8x0m_free(struct intel8x0m *chip) __hw_end: if (chip->irq >= 0) free_irq(chip->irq, chip); - if (chip->bdbars.area) - snd_dma_free_pages(&chip->bdbars); - if (chip->addr) - pci_iounmap(chip->pci, chip->addr); - if (chip->bmaddr) - pci_iounmap(chip->pci, chip->bmaddr); - pci_release_regions(chip->pci); - pci_disable_device(chip->pci); - kfree(chip); - return 0; }
#ifdef CONFIG_PM_SLEEP @@ -1093,84 +1084,54 @@ static void snd_intel8x0m_proc_init(struct intel8x0m *chip) snd_info_set_text_ops(entry, chip, snd_intel8x0m_proc_read); }
-static int snd_intel8x0m_dev_free(struct snd_device *device) -{ - struct intel8x0m *chip = device->device_data; - return snd_intel8x0m_free(chip); -} - struct ich_reg_info { unsigned int int_sta_mask; unsigned int offset; };
-static int snd_intel8x0m_create(struct snd_card *card, - struct pci_dev *pci, - unsigned long device_type, - struct intel8x0m **r_intel8x0m) +static int snd_intel8x0m_init(struct snd_card *card, + struct pci_dev *pci, + unsigned long device_type) { - struct intel8x0m *chip; + struct intel8x0m *chip = card->private_data; int err; unsigned int i; unsigned int int_sta_masks; struct ichdev *ichdev; - static struct snd_device_ops ops = { - .dev_free = snd_intel8x0m_dev_free, - }; static struct ich_reg_info intel_regs[2] = { { ICH_MIINT, 0 }, { ICH_MOINT, 0x10 }, }; struct ich_reg_info *tbl;
- *r_intel8x0m = NULL; - - if ((err = pci_enable_device(pci)) < 0) + err = pcim_enable_device(pci); + if (err < 0) return err;
- chip = kzalloc(sizeof(*chip), GFP_KERNEL); - if (chip == NULL) { - pci_disable_device(pci); - return -ENOMEM; - } spin_lock_init(&chip->reg_lock); chip->device_type = device_type; chip->card = card; chip->pci = pci; chip->irq = -1;
- if ((err = pci_request_regions(pci, card->shortname)) < 0) { - kfree(chip); - pci_disable_device(pci); + err = pcim_iomap_regions(pci, -1, card->shortname); + if (err < 0) return err; - }
if (device_type == DEVICE_ALI) { /* ALI5455 has no ac97 region */ - chip->bmaddr = pci_iomap(pci, 0, 0); - goto port_inited; - } - - if (pci_resource_flags(pci, 2) & IORESOURCE_MEM) /* ICH4 and Nforce */ - chip->addr = pci_iomap(pci, 2, 0); - else - chip->addr = pci_iomap(pci, 0, 0); - if (!chip->addr) { - dev_err(card->dev, "AC'97 space ioremap problem\n"); - snd_intel8x0m_free(chip); - return -EIO; - } - if (pci_resource_flags(pci, 3) & IORESOURCE_MEM) /* ICH4 */ - chip->bmaddr = pci_iomap(pci, 3, 0); - else - chip->bmaddr = pci_iomap(pci, 1, 0); - if (!chip->bmaddr) { - dev_err(card->dev, "Controller space ioremap problem\n"); - snd_intel8x0m_free(chip); - return -EIO; + chip->bmaddr = pcim_iomap_table(pci)[0]; + } else { + if (pci_resource_flags(pci, 2) & IORESOURCE_MEM) /* ICH4 and Nforce */ + chip->addr = pcim_iomap_table(pci)[2]; + else + chip->addr = pcim_iomap_table(pci)[0]; + if (pci_resource_flags(pci, 3) & IORESOURCE_MEM) /* ICH4 */ + chip->bmaddr = pcim_iomap_table(pci)[3]; + else + chip->bmaddr = pcim_iomap_table(pci)[1]; }
- port_inited: /* initialize offsets */ chip->bdbars_count = 2; tbl = intel_regs; @@ -1196,19 +1157,19 @@ static int snd_intel8x0m_create(struct snd_card *card,
/* allocate buffer descriptor lists */ /* the start of each lists must be aligned to 8 bytes */ - if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, snd_dma_pci_data(pci), - chip->bdbars_count * sizeof(u32) * ICH_MAX_FRAGS * 2, - &chip->bdbars) < 0) { - snd_intel8x0m_free(chip); + chip->bdbars = snd_devm_alloc_pages(&pci->dev, SNDRV_DMA_TYPE_DEV, + chip->bdbars_count * sizeof(u32) * + ICH_MAX_FRAGS * 2); + if (IS_ERR(chip->bdbars)) return -ENOMEM; - } + /* tables must be aligned to 8 bytes here, but the kernel pages are much bigger, so we don't care (on i386) */ int_sta_masks = 0; for (i = 0; i < chip->bdbars_count; i++) { ichdev = &chip->ichd[i]; - ichdev->bdbar = ((__le32 *)chip->bdbars.area) + (i * ICH_MAX_FRAGS * 2); - ichdev->bdbar_addr = chip->bdbars.addr + (i * sizeof(u32) * ICH_MAX_FRAGS * 2); + ichdev->bdbar = ((__le32 *)chip->bdbars->area) + (i * ICH_MAX_FRAGS * 2); + ichdev->bdbar_addr = chip->bdbars->addr + (i * sizeof(u32) * ICH_MAX_FRAGS * 2); int_sta_masks |= ichdev->int_sta_mask; } chip->int_sta_reg = ICH_REG_GLOB_STA; @@ -1216,25 +1177,22 @@ static int snd_intel8x0m_create(struct snd_card *card,
pci_set_master(pci);
- if ((err = snd_intel8x0m_chip_init(chip, 1)) < 0) { - snd_intel8x0m_free(chip); + err = snd_intel8x0m_chip_init(chip, 1); + if (err < 0) return err; - }
+ /* NOTE: we don't use devm version here since it's released / + * re-acquired in PM callbacks. + * It's released explicitly in snd_intel8x0m_free(), too. + */ if (request_irq(pci->irq, snd_intel8x0m_interrupt, IRQF_SHARED, KBUILD_MODNAME, chip)) { dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); - snd_intel8x0m_free(chip); return -EBUSY; } chip->irq = pci->irq; + card->private_free = snd_intel8x0m_free;
- if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { - snd_intel8x0m_free(chip); - return err; - } - - *r_intel8x0m = chip; return 0; }
@@ -1272,9 +1230,11 @@ static int snd_intel8x0m_probe(struct pci_dev *pci, int err; struct shortname_table *name;
- err = snd_card_new(&pci->dev, index, id, THIS_MODULE, 0, &card); + err = snd_devm_card_new(&pci->dev, index, id, THIS_MODULE, + sizeof(*chip), &card); if (err < 0) return err; + chip = card->private_data;
strcpy(card->driver, "ICH-MODEM"); strcpy(card->shortname, "Intel ICH"); @@ -1286,44 +1246,33 @@ static int snd_intel8x0m_probe(struct pci_dev *pci, } strcat(card->shortname," Modem");
- if ((err = snd_intel8x0m_create(card, pci, pci_id->driver_data, &chip)) < 0) { - snd_card_free(card); + err = snd_intel8x0m_init(card, pci, pci_id->driver_data); + if (err < 0) return err; - } - card->private_data = chip;
- if ((err = snd_intel8x0m_mixer(chip, ac97_clock)) < 0) { - snd_card_free(card); + err = snd_intel8x0m_mixer(chip, ac97_clock); + if (err < 0) return err; - } - if ((err = snd_intel8x0m_pcm(chip)) < 0) { - snd_card_free(card); + err = snd_intel8x0m_pcm(chip); + if (err < 0) return err; - } snd_intel8x0m_proc_init(chip);
sprintf(card->longname, "%s at irq %i", card->shortname, chip->irq);
- if ((err = snd_card_register(card)) < 0) { - snd_card_free(card); + err = snd_card_register(card); + if (err < 0) return err; - } pci_set_drvdata(pci, card); return 0; }
-static void snd_intel8x0m_remove(struct pci_dev *pci) -{ - snd_card_free(pci_get_drvdata(pci)); -} - static struct pci_driver intel8x0m_driver = { .name = KBUILD_MODNAME, .id_table = snd_intel8x0m_ids, .probe = snd_intel8x0m_probe, - .remove = snd_intel8x0m_remove, .driver = { .pm = INTEL8X0M_PM_OPS, },
Like the previous patch, this patch converts the resource allocations with device-managed API calls, so that we can reduce resource-free calls.
The atiixp drivers are simpler than intel8x0, and even the irq can be allocated with devres.
The end result is a good amount of code reduction.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/atiixp.c | 104 +++++++++++---------------------------- sound/pci/atiixp_modem.c | 104 +++++++++++---------------------------- 2 files changed, 60 insertions(+), 148 deletions(-)
diff --git a/sound/pci/atiixp.c b/sound/pci/atiixp.c index 1a41f8c80243..c18959592738 100644 --- a/sound/pci/atiixp.c +++ b/sound/pci/atiixp.c @@ -1557,84 +1557,44 @@ static void snd_atiixp_proc_init(struct atiixp *chip) * destructor */
-static int snd_atiixp_free(struct atiixp *chip) +static void snd_atiixp_free(struct snd_card *card) { - if (chip->irq < 0) - goto __hw_end; - snd_atiixp_chip_stop(chip); - - __hw_end: - if (chip->irq >= 0) - free_irq(chip->irq, chip); - iounmap(chip->remap_addr); - pci_release_regions(chip->pci); - pci_disable_device(chip->pci); - kfree(chip); - return 0; -} - -static int snd_atiixp_dev_free(struct snd_device *device) -{ - struct atiixp *chip = device->device_data; - return snd_atiixp_free(chip); + snd_atiixp_chip_stop(card->private_data); }
/* * constructor for chip instance */ -static int snd_atiixp_create(struct snd_card *card, - struct pci_dev *pci, - struct atiixp **r_chip) +static int snd_atiixp_init(struct snd_card *card, struct pci_dev *pci) { - static struct snd_device_ops ops = { - .dev_free = snd_atiixp_dev_free, - }; - struct atiixp *chip; + struct atiixp *chip = card->private_data; int err;
- if ((err = pci_enable_device(pci)) < 0) + err = pcim_enable_device(pci); + if (err < 0) return err;
- chip = kzalloc(sizeof(*chip), GFP_KERNEL); - if (chip == NULL) { - pci_disable_device(pci); - return -ENOMEM; - } - spin_lock_init(&chip->reg_lock); mutex_init(&chip->open_mutex); chip->card = card; chip->pci = pci; chip->irq = -1; - if ((err = pci_request_regions(pci, "ATI IXP AC97")) < 0) { - pci_disable_device(pci); - kfree(chip); + err = pcim_iomap_regions(pci, 1 << 0, "ATI IXP AC97"); + if (err < 0) return err; - } chip->addr = pci_resource_start(pci, 0); - chip->remap_addr = pci_ioremap_bar(pci, 0); - if (chip->remap_addr == NULL) { - dev_err(card->dev, "AC'97 space ioremap problem\n"); - snd_atiixp_free(chip); - return -EIO; - } + chip->remap_addr = pcim_iomap_table(pci)[0];
- if (request_irq(pci->irq, snd_atiixp_interrupt, IRQF_SHARED, - KBUILD_MODNAME, chip)) { + if (devm_request_irq(&pci->dev, pci->irq, snd_atiixp_interrupt, + IRQF_SHARED, KBUILD_MODNAME, chip)) { dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); - snd_atiixp_free(chip); return -EBUSY; } chip->irq = pci->irq; + card->private_free = snd_atiixp_free; pci_set_master(pci); synchronize_irq(chip->irq);
- if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { - snd_atiixp_free(chip); - return err; - } - - *r_chip = chip; return 0; }
@@ -1646,26 +1606,31 @@ static int snd_atiixp_probe(struct pci_dev *pci, struct atiixp *chip; int err;
- err = snd_card_new(&pci->dev, index, id, THIS_MODULE, 0, &card); + err = snd_devm_card_new(&pci->dev, index, id, THIS_MODULE, + sizeof(*chip), &card); if (err < 0) return err; + chip = card->private_data;
strcpy(card->driver, spdif_aclink ? "ATIIXP" : "ATIIXP-SPDMA"); strcpy(card->shortname, "ATI IXP"); - if ((err = snd_atiixp_create(card, pci, &chip)) < 0) - goto __error; - card->private_data = chip; + err = snd_atiixp_init(card, pci); + if (err < 0) + return err;
- if ((err = snd_atiixp_aclink_reset(chip)) < 0) - goto __error; + err = snd_atiixp_aclink_reset(chip); + if (err < 0) + return err;
chip->spdif_over_aclink = spdif_aclink;
- if ((err = snd_atiixp_mixer_new(chip, ac97_clock, ac97_quirk)) < 0) - goto __error; + err = snd_atiixp_mixer_new(chip, ac97_clock, ac97_quirk); + if (err < 0) + return err;
- if ((err = snd_atiixp_pcm_new(chip)) < 0) - goto __error; + err = snd_atiixp_pcm_new(chip); + if (err < 0) + return err; snd_atiixp_proc_init(chip);
@@ -1677,27 +1642,18 @@ static int snd_atiixp_probe(struct pci_dev *pci, chip->ac97[0] ? snd_ac97_get_short_name(chip->ac97[0]) : "?", chip->addr, chip->irq);
- if ((err = snd_card_register(card)) < 0) - goto __error; + err = snd_card_register(card); + if (err < 0) + return err;
pci_set_drvdata(pci, card); return 0; - - __error: - snd_card_free(card); - return err; -} - -static void snd_atiixp_remove(struct pci_dev *pci) -{ - snd_card_free(pci_get_drvdata(pci)); }
static struct pci_driver atiixp_driver = { .name = KBUILD_MODNAME, .id_table = snd_atiixp_ids, .probe = snd_atiixp_probe, - .remove = snd_atiixp_remove, .driver = { .pm = SND_ATIIXP_PM_OPS, }, diff --git a/sound/pci/atiixp_modem.c b/sound/pci/atiixp_modem.c index dc1de860cedf..207f2be1d73e 100644 --- a/sound/pci/atiixp_modem.c +++ b/sound/pci/atiixp_modem.c @@ -1183,84 +1183,44 @@ static void snd_atiixp_proc_init(struct atiixp_modem *chip) * destructor */
-static int snd_atiixp_free(struct atiixp_modem *chip) +static void snd_atiixp_free(struct snd_card *card) { - if (chip->irq < 0) - goto __hw_end; - snd_atiixp_chip_stop(chip); - - __hw_end: - if (chip->irq >= 0) - free_irq(chip->irq, chip); - iounmap(chip->remap_addr); - pci_release_regions(chip->pci); - pci_disable_device(chip->pci); - kfree(chip); - return 0; -} - -static int snd_atiixp_dev_free(struct snd_device *device) -{ - struct atiixp_modem *chip = device->device_data; - return snd_atiixp_free(chip); + snd_atiixp_chip_stop(card->private_data); }
/* * constructor for chip instance */ -static int snd_atiixp_create(struct snd_card *card, - struct pci_dev *pci, - struct atiixp_modem **r_chip) +static int snd_atiixp_init(struct snd_card *card, struct pci_dev *pci) { - static struct snd_device_ops ops = { - .dev_free = snd_atiixp_dev_free, - }; - struct atiixp_modem *chip; + struct atiixp_modem *chip = card->private_data; int err;
- if ((err = pci_enable_device(pci)) < 0) + err = pcim_enable_device(pci); + if (err < 0) return err;
- chip = kzalloc(sizeof(*chip), GFP_KERNEL); - if (chip == NULL) { - pci_disable_device(pci); - return -ENOMEM; - } - spin_lock_init(&chip->reg_lock); mutex_init(&chip->open_mutex); chip->card = card; chip->pci = pci; chip->irq = -1; - if ((err = pci_request_regions(pci, "ATI IXP MC97")) < 0) { - kfree(chip); - pci_disable_device(pci); + err = pcim_iomap_regions(pci, 1 << 0, "ATI IXP MC97"); + if (err < 0) return err; - } chip->addr = pci_resource_start(pci, 0); - chip->remap_addr = pci_ioremap_bar(pci, 0); - if (chip->remap_addr == NULL) { - dev_err(card->dev, "AC'97 space ioremap problem\n"); - snd_atiixp_free(chip); - return -EIO; - } + chip->remap_addr = pcim_iomap_table(pci)[0];
- if (request_irq(pci->irq, snd_atiixp_interrupt, IRQF_SHARED, - KBUILD_MODNAME, chip)) { + if (devm_request_irq(&pci->dev, pci->irq, snd_atiixp_interrupt, + IRQF_SHARED, KBUILD_MODNAME, chip)) { dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); - snd_atiixp_free(chip); return -EBUSY; } chip->irq = pci->irq; + card->private_free = snd_atiixp_free; pci_set_master(pci); synchronize_irq(chip->irq);
- if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { - snd_atiixp_free(chip); - return err; - } - - *r_chip = chip; return 0; }
@@ -1272,24 +1232,29 @@ static int snd_atiixp_probe(struct pci_dev *pci, struct atiixp_modem *chip; int err;
- err = snd_card_new(&pci->dev, index, id, THIS_MODULE, 0, &card); + err = snd_devm_card_new(&pci->dev, index, id, THIS_MODULE, + sizeof(*chip), &card); if (err < 0) return err; + chip = card->private_data;
strcpy(card->driver, "ATIIXP-MODEM"); strcpy(card->shortname, "ATI IXP Modem"); - if ((err = snd_atiixp_create(card, pci, &chip)) < 0) - goto __error; - card->private_data = chip; + err = snd_atiixp_init(card, pci); + if (err < 0) + return err;
- if ((err = snd_atiixp_aclink_reset(chip)) < 0) - goto __error; + err = snd_atiixp_aclink_reset(chip); + if (err < 0) + return err;
- if ((err = snd_atiixp_mixer_new(chip, ac97_clock)) < 0) - goto __error; + err = snd_atiixp_mixer_new(chip, ac97_clock); + if (err < 0) + return err;
- if ((err = snd_atiixp_pcm_new(chip)) < 0) - goto __error; + err = snd_atiixp_pcm_new(chip); + if (err < 0) + return err; snd_atiixp_proc_init(chip);
@@ -1298,27 +1263,18 @@ static int snd_atiixp_probe(struct pci_dev *pci, sprintf(card->longname, "%s rev %x at 0x%lx, irq %i", card->shortname, pci->revision, chip->addr, chip->irq);
- if ((err = snd_card_register(card)) < 0) - goto __error; + err = snd_card_register(card); + if (err < 0) + return err;
pci_set_drvdata(pci, card); return 0; - - __error: - snd_card_free(card); - return err; -} - -static void snd_atiixp_remove(struct pci_dev *pci) -{ - snd_card_free(pci_get_drvdata(pci)); }
static struct pci_driver atiixp_modem_driver = { .name = KBUILD_MODNAME, .id_table = snd_atiixp_ids, .probe = snd_atiixp_probe, - .remove = snd_atiixp_remove, .driver = { .pm = SND_ATIIXP_PM_OPS, },
This patch is an attempt to slightly simplify the resource management in HD-audio code, by using some device-managed APIs. A few resources like PCI enablement, PCI resources and the card object are managed via devres now. But most of codes dealing with HD-audio core stuff couldn't be changed so much, hence the changes in this patch are pretty small in the end.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_controller.h | 1 - sound/pci/hda/hda_intel.c | 43 ++++++++-------------------------- 2 files changed, 10 insertions(+), 34 deletions(-)
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h index c95097bb5a0c..16f2285847b3 100644 --- a/sound/pci/hda/hda_controller.h +++ b/sound/pci/hda/hda_controller.h @@ -156,7 +156,6 @@ struct azx { unsigned int snoop:1; unsigned int uc_buffer:1; /* non-cached pages for stream buffers */ unsigned int align_buffer_size:1; - unsigned int region_requested:1; unsigned int disabled:1; /* disabled by vga_switcheroo */
/* GTS present */ diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 4e64595b381b..7a697db7a6a0 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1318,18 +1318,11 @@ static int azx_free(struct azx *chip)
if (bus->irq >= 0) free_irq(bus->irq, (void*)chip); - if (chip->msi) - pci_disable_msi(chip->pci); - iounmap(bus->remap_addr);
azx_free_stream_pages(chip); azx_free_streams(chip); snd_hdac_bus_exit(bus);
- if (chip->region_requested) - pci_release_regions(chip->pci); - - pci_disable_device(chip->pci); #ifdef CONFIG_SND_HDA_PATCH_LOADER release_firmware(chip->fw); #endif @@ -1657,15 +1650,13 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci,
*rchip = NULL;
- err = pci_enable_device(pci); + err = pcim_enable_device(pci); if (err < 0) return err;
hda = kzalloc(sizeof(*hda), GFP_KERNEL); - if (!hda) { - pci_disable_device(pci); + if (!hda) return -ENOMEM; - }
chip = &hda->chip; mutex_init(&chip->open_mutex); @@ -1707,7 +1698,6 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci, err = azx_bus_init(chip, model[dev], &pci_hda_io_ops); if (err < 0) { kfree(hda); - pci_disable_device(pci); return err; }
@@ -1751,17 +1741,12 @@ static int azx_first_init(struct azx *chip) } #endif
- err = pci_request_regions(pci, "ICH HD audio"); + err = pcim_iomap_regions(pci, 1 << 0, "ICH HD audio"); if (err < 0) return err; - chip->region_requested = 1;
bus->addr = pci_resource_start(pci, 0); - bus->remap_addr = pci_ioremap_bar(pci, 0); - if (bus->remap_addr == NULL) { - dev_err(card->dev, "ioremap error\n"); - return -ENXIO; - } + bus->remap_addr = pcim_iomap_table(pci)[0];
if (chip->driver_type == AZX_DRIVER_SKL) snd_hdac_bus_parse_capabilities(bus); @@ -2057,16 +2042,14 @@ static int azx_probe(struct pci_dev *pci, return -ENOENT; }
- err = snd_card_new(&pci->dev, index[dev], id[dev], THIS_MODULE, - 0, &card); - if (err < 0) { - dev_err(&pci->dev, "Error creating card!\n"); + err = snd_devm_card_new(&pci->dev, index[dev], id[dev], THIS_MODULE, 0, + &card); + if (err < 0) return err; - }
err = azx_create(card, pci, dev, pci_id->driver_data, &chip); if (err < 0) - goto out_free; + return err; card->private_data = chip; hda = container_of(chip, struct hda_intel, chip);
@@ -2075,7 +2058,7 @@ static int azx_probe(struct pci_dev *pci, err = register_vga_switcheroo(chip); if (err < 0) { dev_err(card->dev, "Error registering vga_switcheroo client\n"); - goto out_free; + return err; }
if (check_hdmi_disabled(pci)) { @@ -2094,7 +2077,7 @@ static int azx_probe(struct pci_dev *pci, &pci->dev, GFP_KERNEL, card, azx_firmware_cb); if (err < 0) - goto out_free; + return err; schedule_probe = false; /* continued in azx_firmware_cb() */ } #endif /* CONFIG_SND_HDA_PATCH_LOADER */ @@ -2111,10 +2094,6 @@ static int azx_probe(struct pci_dev *pci, if (chip->disabled) complete_all(&hda->probe_wait); return 0; - -out_free: - snd_card_free(card); - return err; }
#ifdef CONFIG_PM @@ -2304,8 +2283,6 @@ static void azx_remove(struct pci_dev *pci) device_unlock(&pci->dev); cancel_work_sync(&hda->probe_work); device_lock(&pci->dev); - - snd_card_free(card); } }
Slightly brushing up and throw the old dust away from my ancient writing-an-alsa-driver document. The contents aren't changed so much but the obsoleted parts are dropped.
Also, remove the date and the version number. It's useless.
Signed-off-by: Takashi Iwai tiwai@suse.de --- .../kernel-api/writing-an-alsa-driver.rst | 307 +++++++++--------- 1 file changed, 149 insertions(+), 158 deletions(-)
diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst index a0b268466cb1..b60a26807420 100644 --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst @@ -3,8 +3,6 @@ Writing an ALSA Driver ======================
:Author: Takashi Iwai tiwai@suse.de -:Date: Oct 15, 2007 -:Edition: 0.3.7
Preface ======= @@ -21,11 +19,6 @@ explain the general topic of linux kernel coding and doesn't cover low-level driver implementation details. It only describes the standard way to write a PCI sound driver on ALSA.
-If you are already familiar with the older ALSA ver.0.5.x API, you can -check the drivers such as ``sound/pci/es1938.c`` or -``sound/pci/maestro3.c`` which have also almost the same code-base in -the ALSA 0.5.x tree, so you can compare the differences. - This document is still a draft version. Any feedback and corrections, please!!
@@ -35,24 +28,7 @@ File Tree Structure General -------
-The ALSA drivers are provided in two ways. - -One is the trees provided as a tarball or via cvs from the ALSA's ftp -site, and another is the 2.6 (or later) Linux kernel tree. To -synchronize both, the ALSA driver tree is split into two different -trees: alsa-kernel and alsa-driver. The former contains purely the -source code for the Linux 2.6 (or later) tree. This tree is designed -only for compilation on 2.6 or later environment. The latter, -alsa-driver, contains many subtle files for compiling ALSA drivers -outside of the Linux kernel tree, wrapper functions for older 2.2 and -2.4 kernels, to adapt the latest kernel API, and additional drivers -which are still in development or in tests. The drivers in alsa-driver -tree will be moved to alsa-kernel (and eventually to the 2.6 kernel -tree) when they are finished and confirmed to work fine. - -The file tree structure of ALSA driver is depicted below. Both -alsa-kernel and alsa-driver have almost the same file structure, except -for “core” directory. It's named as “acore” in alsa-driver tree. +The file tree structure of ALSA driver is depicted below.
::
@@ -61,14 +37,11 @@ for “core” directory. It's named as “acore” in alsa-driver tree. /oss /seq /oss - /instr - /ioctl32 /include /drivers /mpu401 /opl3 /i2c - /l3 /synth /emux /pci @@ -80,6 +53,7 @@ for “core” directory. It's named as “acore” in alsa-driver tree. /sparc /usb /pcmcia /(cards) + /soc /oss
@@ -99,13 +73,6 @@ directory. The rawmidi OSS emulation is included in the ALSA rawmidi code since it's quite small. The sequencer code is stored in ``core/seq/oss`` directory (see `below <#core-seq-oss>`__).
-core/ioctl32 -~~~~~~~~~~~~ - -This directory contains the 32bit-ioctl wrappers for 64bit architectures -such like x86-64, ppc64 and sparc64. For 32bit and alpha architectures, -these are not compiled. - core/seq ~~~~~~~~
@@ -119,11 +86,6 @@ core/seq/oss
This contains the OSS sequencer emulation codes.
-core/seq/instr -~~~~~~~~~~~~~~ - -This directory contains the modules for the sequencer instrument layer. - include directory -----------------
@@ -161,11 +123,6 @@ Although there is a standard i2c layer on Linux, ALSA has its own i2c code for some cards, because the soundcard needs only a simple operation and the standard i2c API is too complicated for such a purpose.
-i2c/l3 -~~~~~~ - -This is a sub-directory for ARM L3 i2c. - synth directory ---------------
@@ -209,11 +166,19 @@ The PCMCIA, especially PCCard drivers will go here. CardBus drivers will be in the pci directory, because their API is identical to that of standard PCI cards.
+soc directory +------------- + +This directory contains the codes for ASoC (ALSA System on Chip) +layer including ASoC core, codec and machine drivers. + oss directory -------------
-The OSS/Lite source files are stored here in Linux 2.6 (or later) tree. -In the ALSA driver tarball, this directory is empty, of course :) +Here contains OSS/Lite codes. +All codes have been deprecated except for dmasound on m68k as of +writing this. +
Basic Flow for PCI Drivers ========================== @@ -352,10 +317,8 @@ to details explained in the following section.
/* (3) */ err = snd_mychip_create(card, pci, &chip); - if (err < 0) { - snd_card_free(card); - return err; - } + if (err < 0) + goto error;
/* (4) */ strcpy(card->driver, "My Chip"); @@ -368,22 +331,23 @@ to details explained in the following section.
/* (6) */ err = snd_card_register(card); - if (err < 0) { - snd_card_free(card); - return err; - } + if (err < 0) + goto error;
/* (7) */ pci_set_drvdata(pci, card); dev++; return 0; + + error: + snd_card_free(card); + return err; }
/* destructor -- see the "Destructor" sub-section */ static void snd_mychip_remove(struct pci_dev *pci) { snd_card_free(pci_get_drvdata(pci)); - pci_set_drvdata(pci, NULL); }
@@ -445,14 +409,26 @@ In this part, the PCI resources are allocated. struct mychip *chip; .... err = snd_mychip_create(card, pci, &chip); - if (err < 0) { - snd_card_free(card); - return err; - } + if (err < 0) + goto error;
The details will be explained in the section `PCI Resource Management`_.
+When something goes wrong, the probe function needs to deal with the +error. In this example, we have a single error handling path placed +at the end of the function. + +:: + + error: + snd_card_free(card); + return err; + +Since each component can be properly freed, the single +:c:func:`snd_card_free()` call should suffice in most cases. + + 4) Set the driver ID and name strings. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -486,10 +462,8 @@ too. ::
err = snd_card_register(card); - if (err < 0) { - snd_card_free(card); - return err; - } + if (err < 0) + goto error;
Will be explained in the section `Management of Cards and Components`_, too. @@ -513,14 +487,13 @@ The destructor, remove callback, simply releases the card instance. Then the ALSA middle layer will release all the attached components automatically.
-It would be typically like the following: +It would be typically just :c:func:`calling snd_card_free()`:
::
static void snd_mychip_remove(struct pci_dev *pci) { snd_card_free(pci_get_drvdata(pci)); - pci_set_drvdata(pci, NULL); }
@@ -546,7 +519,7 @@ in the source file. If the code is split into several files, the files without module options don't need them.
In addition to these headers, you'll need ``<linux/interrupt.h>`` for -interrupt handling, and ``<asm/io.h>`` for I/O access. If you use the +interrupt handling, and ``<linux/io.h>`` for I/O access. If you use the :c:func:`mdelay()` or :c:func:`udelay()` functions, you'll need to include ``<linux/delay.h>`` too.
@@ -720,6 +693,13 @@ function, which will call the real destructor.
where :c:func:`snd_mychip_free()` is the real destructor.
+The demerit of this method is the obviously more amount of codes. +The merit is, however, you can trigger the own callback at registering +and disconnecting the card via setting in snd_device_ops. +About the registering and disconnecting the card, see the subsections +below. + + Registration and Release ------------------------
@@ -905,10 +885,8 @@ Resource Allocation -------------------
The allocation of I/O ports and irqs is done via standard kernel -functions. Unlike ALSA ver.0.5.x., there are no helpers for that. And -these resources must be released in the destructor function (see below). -Also, on ALSA 0.9.x, you don't need to allocate (pseudo-)DMA for PCI -like in ALSA 0.5.x. +functions. These resources must be released in the destructor +function (see below).
Now assume that the PCI device has an I/O port with 8 bytes and an interrupt. Then :c:type:`struct mychip <mychip>` will have the @@ -1064,7 +1042,8 @@ and the allocation would be like below:
::
- if ((err = pci_request_regions(pci, "My Chip")) < 0) { + err = pci_request_regions(pci, "My Chip"); + if (err < 0) { kfree(chip); return err; } @@ -1086,6 +1065,21 @@ and the corresponding destructor would be: .... }
+Of course, a modern way with :c:func:`pci_iomap()` will make things a +bit easier, too. + +:: + + err = pci_request_regions(pci, "My Chip"); + if (err < 0) { + kfree(chip); + return err; + } + chip->iobase_virt = pci_iomap(pci, 0, 0); + +which is paired with :c:func:`pci_iounmap()` at destructor. + + PCI Entries -----------
@@ -1154,13 +1148,6 @@ And at last, the module entries: Note that these module entries are tagged with ``__init`` and ``__exit`` prefixes.
-Oh, one thing was forgotten. If you have no exported symbols, you need -to declare it in 2.2 or 2.4 kernels (it's not necessary in 2.6 kernels). - -:: - - EXPORT_NO_SYMBOLS; - That's all!
PCM Interface @@ -2113,6 +2100,16 @@ non-contiguous buffers. The mmap calls this callback to get the page address. Some examples will be explained in the later section `Buffer and Memory Management`_, too.
+mmap calllback +~~~~~~~~~~~~~~ + +This is another optional callback for controlling mmap behavior. +Once when defined, PCM core calls this callback when a page is +memory-mapped instead of dealing via the standard helper. +If you need special handling (due to some architecture or +device-specific issues), implement everything here as you like. + + PCM Interrupt Handler ---------------------
@@ -2370,6 +2367,27 @@ to define the inverse rule: hw_rule_format_by_channels, NULL, SNDRV_PCM_HW_PARAM_CHANNELS, -1);
+One typical usage of the hw constraints is to align the buffer size +with the period size. As default, ALSA PCM core doesn't enforce the +buffer size to be aligned with the period size. For example, it'd be +possible to have a combination like 256 period bytes with 999 buffer +bytes. + +Many device chips, however, require the buffer to be a multiple of +periods. In such a case, call +:c:func:`snd_pcm_hw_constraint_integer()` for +``SNDRV_PCM_HW_PARAM_PERIODS``. + +:: + + snd_pcm_hw_constraint_integer(substream->runtime, + SNDRV_PCM_HW_PARAM_PERIODS); + +This assures that the number of periods is integer, hence the buffer +size is aligned with the period size. + +The hw constraint is a very much powerful mechanism to define the +preferred PCM configuration, and there are relevant helpers. I won't give more details here, rather I would like to say, “Luke, use the source.”
@@ -3712,7 +3730,14 @@ example, for an intermediate buffer. Since the allocated pages are not contiguous, you need to set the ``page`` callback to obtain the physical address at every offset.
-The implementation of ``page`` callback would be like this: +The easiest way to achieve it would be to use +:c:func:`snd_pcm_lib_alloc_vmalloc_buffer()` for allocating the buffer +via :c:func:`vmalloc()`, and set :c:func:`snd_pcm_sgbuf_ops_page()` to +the ``page`` callback. At release, you need to call +:c:func:`snd_pcm_lib_free_vmalloc_buffer()`. + +If you want to implementation the ``page`` manually, it would be like +this:
::
@@ -3848,7 +3873,9 @@ Power Management
If the chip is supposed to work with suspend/resume functions, you need to add power-management code to the driver. The additional code for -power-management should be ifdef-ed with ``CONFIG_PM``. +power-management should be ifdef-ed with ``CONFIG_PM``, or annotated +with __maybe_unused attribute; otherwise the compiler will complain +you.
If the driver *fully* supports suspend/resume that is, the device can be properly resumed to its state when suspend was called, you can set the @@ -3879,18 +3906,16 @@ the case of PCI drivers, the callbacks look like below:
::
- #ifdef CONFIG_PM - static int snd_my_suspend(struct pci_dev *pci, pm_message_t state) + static int __maybe_unused snd_my_suspend(struct device *dev) { .... /* do things for suspend */ return 0; } - static int snd_my_resume(struct pci_dev *pci) + static int __maybe_unused snd_my_resume(struct device *dev) { .... /* do things for suspend */ return 0; } - #endif
The scheme of the real suspend job is as follows.
@@ -3909,18 +3934,14 @@ The scheme of the real suspend job is as follows.
6. Stop the hardware if necessary.
-7. Disable the PCI device by calling - :c:func:`pci_disable_device()`. Then, call - :c:func:`pci_save_state()` at last. - A typical code would be like:
::
- static int mychip_suspend(struct pci_dev *pci, pm_message_t state) + static int __maybe_unused mychip_suspend(struct device *dev) { /* (1) */ - struct snd_card *card = pci_get_drvdata(pci); + struct snd_card *card = dev_get_drvdata(dev); struct mychip *chip = card->private_data; /* (2) */ snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); @@ -3932,9 +3953,6 @@ A typical code would be like: snd_mychip_save_registers(chip); /* (6) */ snd_mychip_stop_hardware(chip); - /* (7) */ - pci_disable_device(pci); - pci_save_state(pci); return 0; }
@@ -3943,44 +3961,35 @@ The scheme of the real resume job is as follows.
1. Retrieve the card and the chip data.
-2. Set up PCI. First, call :c:func:`pci_restore_state()`. Then - enable the pci device again by calling - :c:func:`pci_enable_device()`. Call - :c:func:`pci_set_master()` if necessary, too. +2. Re-initialize the chip.
-3. Re-initialize the chip. +3. Restore the saved registers if necessary.
-4. Restore the saved registers if necessary. +4. Resume the mixer, e.g. calling :c:func:`snd_ac97_resume()`.
-5. Resume the mixer, e.g. calling :c:func:`snd_ac97_resume()`. +5. Restart the hardware (if any).
-6. Restart the hardware (if any). - -7. Call :c:func:`snd_power_change_state()` with +6. Call :c:func:`snd_power_change_state()` with ``SNDRV_CTL_POWER_D0`` to notify the processes.
A typical code would be like:
::
- static int mychip_resume(struct pci_dev *pci) + static int __maybe_unused mychip_resume(struct pci_dev *pci) { /* (1) */ - struct snd_card *card = pci_get_drvdata(pci); + struct snd_card *card = dev_get_drvdata(dev); struct mychip *chip = card->private_data; /* (2) */ - pci_restore_state(pci); - pci_enable_device(pci); - pci_set_master(pci); - /* (3) */ snd_mychip_reinit_chip(chip); - /* (4) */ + /* (3) */ snd_mychip_restore_registers(chip); - /* (5) */ + /* (4) */ snd_ac97_resume(chip->ac97); - /* (6) */ + /* (5) */ snd_mychip_restart_chip(chip); - /* (7) */ + /* (6) */ snd_power_change_state(card, SNDRV_CTL_POWER_D0); return 0; } @@ -4046,15 +4055,14 @@ And next, set suspend/resume callbacks to the pci_driver.
::
+ static SIMPLE_DEV_PM_OPS(snd_my_pm_ops, mychip_suspend, mychip_resume); + static struct pci_driver driver = { .name = KBUILD_MODNAME, .id_table = snd_my_ids, .probe = snd_my_probe, .remove = snd_my_remove, - #ifdef CONFIG_PM - .suspend = snd_my_suspend, - .resume = snd_my_resume, - #endif + .driver.pm = &snd_my_pm_ops, };
Module Parameters @@ -4078,7 +4086,7 @@ variables, instead. ``enable`` option is not always necessary in this case, but it would be better to have a dummy option for compatibility.
The module parameters must be declared with the standard -``module_param()()``, ``module_param_array()()`` and +``module_param()``, ``module_param_array()`` and :c:func:`MODULE_PARM_DESC()` macros.
The typical coding would be like below: @@ -4094,15 +4102,14 @@ The typical coding would be like below: module_param_array(enable, bool, NULL, 0444); MODULE_PARM_DESC(enable, "Enable " CARD_NAME " soundcard.");
-Also, don't forget to define the module description, classes, license -and devices. Especially, the recent modprobe requires to define the +Also, don't forget to define the module description and the license. +Especially, the recent modprobe requires to define the module license as GPL, etc., otherwise the system is shown as “tainted”.
::
- MODULE_DESCRIPTION("My Chip"); + MODULE_DESCRIPTION("Sound driver for My Chip"); MODULE_LICENSE("GPL"); - MODULE_SUPPORTED_DEVICE("{{Vendor,My Chip Name}}");
How To Put Your Driver Into ALSA Tree @@ -4117,21 +4124,17 @@ a question now: how to put my own driver into the ALSA driver tree? Here
Suppose that you create a new PCI driver for the card “xyz”. The card module name would be snd-xyz. The new driver is usually put into the -alsa-driver tree, ``alsa-driver/pci`` directory in the case of PCI -cards. Then the driver is evaluated, audited and tested by developers -and users. After a certain time, the driver will go to the alsa-kernel -tree (to the corresponding directory, such as ``alsa-kernel/pci``) and -eventually will be integrated into the Linux 2.6 tree (the directory -would be ``linux/sound/pci``). +alsa-driver tree, ``sound/pci`` directory in the case of PCI +cards.
In the following sections, the driver code is supposed to be put into -alsa-driver tree. The two cases are covered: a driver consisting of a +Linux kernel tree. The two cases are covered: a driver consisting of a single source file and one consisting of several source files.
Driver with A Single Source File --------------------------------
-1. Modify alsa-driver/pci/Makefile +1. Modify sound/pci/Makefile
Suppose you have a file xyz.c. Add the following two lines
@@ -4160,52 +4163,43 @@ Driver with A Single Source File
For the details of Kconfig script, refer to the kbuild documentation.
-3. Run cvscompile script to re-generate the configure script and build - the whole stuff again. - Drivers with Several Source Files ---------------------------------
Suppose that the driver snd-xyz have several source files. They are -located in the new subdirectory, pci/xyz. +located in the new subdirectory, sound/pci/xyz.
-1. Add a new directory (``xyz``) in ``alsa-driver/pci/Makefile`` as - below +1. Add a new directory (``sound/pci/xyz``) in ``sound/pci/Makefile`` + as below
::
- obj-$(CONFIG_SND) += xyz/ + obj-$(CONFIG_SND) += sound/pci/xyz/
-2. Under the directory ``xyz``, create a Makefile +2. Under the directory ``sound/pci/xyz``, create a Makefile
::
- ifndef SND_TOPDIR - SND_TOPDIR=../.. - endif - - include $(SND_TOPDIR)/toplevel.config - include $(SND_TOPDIR)/Makefile.conf - snd-xyz-objs := xyz.o abc.o def.o - obj-$(CONFIG_SND_XYZ) += snd-xyz.o
- include $(SND_TOPDIR)/Rules.make - 3. Create the Kconfig entry
This procedure is as same as in the last section.
-4. Run cvscompile script to re-generate the configure script and build - the whole stuff again.
Useful Functions ================
:c:func:`snd_printk()` and friends ---------------------------------------- +---------------------------------- + +.. note:: This subsection describes a few helper functions for +decorating a bit more on the standard :c:func:`printk()` & co. +However, in general, the use of such helpers is no longer recommended. +If possible, try to stick with the standard functions like +:c:func:`dev_err()` or :c:func:`pr_err()`.
ALSA provides a verbose version of the :c:func:`printk()` function. If a kernel config ``CONFIG_SND_VERBOSE_PRINTK`` is set, this function @@ -4221,13 +4215,10 @@ just like :c:func:`snd_printk()`. If the ALSA is compiled without the debugging flag, it's ignored.
:c:func:`snd_printdd()` is compiled in only when -``CONFIG_SND_DEBUG_VERBOSE`` is set. Please note that -``CONFIG_SND_DEBUG_VERBOSE`` is not set as default even if you configure -the alsa-driver with ``--with-debug=full`` option. You need to give -explicitly ``--with-debug=detect`` option instead. +``CONFIG_SND_DEBUG_VERBOSE`` is set.
:c:func:`snd_BUG()` ------------------------- +-------------------
It shows the ``BUG?`` message and stack trace as well as :c:func:`snd_BUG_ON()` at the point. It's useful to show that a @@ -4236,7 +4227,7 @@ fatal error happens there. When no debug flag is set, this macro is ignored.
:c:func:`snd_BUG_ON()` ----------------------------- +----------------------
:c:func:`snd_BUG_ON()` macro is similar with :c:func:`WARN_ON()` macro. For example, snd_BUG_ON(!pointer); or
Give brief explanations about the device-managed resources and the newly introduced snd_devm_card_new() helper.
Signed-off-by: Takashi Iwai tiwai@suse.de --- .../kernel-api/writing-an-alsa-driver.rst | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+)
diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst index b60a26807420..b0ca01f3de81 100644 --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst @@ -4112,6 +4112,39 @@ module license as GPL, etc., otherwise the system is shown as “tainted”. MODULE_LICENSE("GPL");
+Device-Managed Resources +======================== + +In the examples above, all resources are allocated and released +manually. But human beings are lazy in nature, especially developers +are lazier. So there are some ways to automate the release part; it's +the (device-)managed resources aka devres or devm family. For +example, an object allocated via :c:func:`devm_kmalloc()` will be +freed automatically at unbinding the device. + +ALSA core provides also the device-managed helper, namely, +:c:func:`snd_devm_card_new()` for creating a card object. +Call this functions instead of the normal :c:func:`snd_card_new()`, +and you can forget the explicit :c:func:`snd_card_free()` call, as +it's called automagically at error and removal paths. + +One caveat is that the call of :c:func:`snd_card_free()` would be put +at the beginning of the call chain only after you call +:c:func:`snd_card_register()`. + +Also, the ``private_free`` callback is always called at the card free, +so be careful to put the hardware clean-up procedure in +``private_free`` callback. It might be called even before you +actually set up at an earlier error path. For avoiding such an +invalid initialization, you can set ``private_free`` callback after +:c:func:`snd_card_register()` call succeeds. + +Another thing to be remarked is that you should use device-managed +helpers for each component as much as possible once when you manage +the card in that way. Mixing up with the normal and the managed +resources may screw up the release order. + + How To Put Your Driver Into ALSA Tree =====================================
participants (2)
-
Takashi Iwai -
Takashi Sakamoto