| Message ID | 20210713142857.19654-3-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: More devres usages | expand |
On 7/13/2021 4:28 PM, Takashi Iwai wrote: ... > + > +/** > + * 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; Maybe just make extra_size unsigned or even better size_t? ... > > /** > * snd_card_ref - Get the card object from the index > @@ -481,6 +547,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 +565,8 @@ static int snd_card_do_free(struct snd_card *card) > #endif > if (card->release_completion) > complete(card->release_completion); > - kfree(card); > + if (!card->managed) > + kfree(card); > return 0; > } > > @@ -539,6 +607,9 @@ int snd_card_free(struct snd_card *card) > DECLARE_COMPLETION_ONSTACK(released); > int ret; > > + if (card->releasing) > + return 0; > + "card->releasing" use feels bit racy to me... something like below would break it? thread1 thread2 snd_card_free() if(card->releasing) == false thread1 goes sleep snd_card_do_free() card->releasing = true run until the end thread1 resume continues with trying to release > card->release_completion = &released; > ret = snd_card_free_when_closed(card); > if (ret) > @@ -745,6 +816,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 > @@ -768,6 +844,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); Not sure I understand, we are in _register function, so why do we remove action? > + } > + > + if (card->managed) { > + err = devm_add_action(card->dev, trigger_card_free, card); > + if (err < 0) > + return err; > } > > err = snd_device_register_all(card); >
On Tue, 13 Jul 2021 17:23:02 +0200, Amadeusz SX2awiX4ski wrote: > > On 7/13/2021 4:28 PM, Takashi Iwai wrote: > > ... > > > + > > +/** > > + * 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; > Maybe just make extra_size unsigned or even better size_t? OK, that would fit for a new function, indeed. Will modify in v2. > > /** > > * snd_card_ref - Get the card object from the index > > @@ -481,6 +547,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 +565,8 @@ static int snd_card_do_free(struct snd_card *card) > > #endif > > if (card->release_completion) > > complete(card->release_completion); > > - kfree(card); > > + if (!card->managed) > > + kfree(card); > > return 0; > > } > > @@ -539,6 +607,9 @@ int snd_card_free(struct snd_card *card) > > DECLARE_COMPLETION_ONSTACK(released); > > int ret; > > + if (card->releasing) > > + return 0; > > + > > "card->releasing" use feels bit racy to me... something like below > would break it? > > thread1 thread2 > snd_card_free() > if(card->releasing) == false > thread1 goes sleep > snd_card_do_free() > card->releasing = true > run until the end > thread1 resume > continues with trying to release It's a destructor and can't be called in parallel. > > card->release_completion = &released; > > ret = snd_card_free_when_closed(card); > > if (ret) > > @@ -745,6 +816,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 > > @@ -768,6 +844,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); > > Not sure I understand, we are in _register function, so why do we > remove action? snd_card_register() can be called multiple times for re-registering the newly added components (e.g. usb-audio driver does it). In that case, we have to move this trigger_card_free action at the head again. thanks, Takashi
On Tue, 13 Jul 2021 18:05:34 +0200, Takashi Iwai wrote: > > On Tue, 13 Jul 2021 17:23:02 +0200, > Amadeusz SX2awiX4ski wrote: > > > > On 7/13/2021 4:28 PM, Takashi Iwai wrote: > > > > > /** > > > * snd_card_ref - Get the card object from the index > > > @@ -481,6 +547,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 +565,8 @@ static int snd_card_do_free(struct snd_card *card) > > > #endif > > > if (card->release_completion) > > > complete(card->release_completion); > > > - kfree(card); > > > + if (!card->managed) > > > + kfree(card); > > > return 0; > > > } > > > @@ -539,6 +607,9 @@ int snd_card_free(struct snd_card *card) > > > DECLARE_COMPLETION_ONSTACK(released); > > > int ret; > > > + if (card->releasing) > > > + return 0; > > > + > > > > "card->releasing" use feels bit racy to me... something like below > > would break it? > > > > thread1 thread2 > > snd_card_free() > > if(card->releasing) == false > > thread1 goes sleep > > snd_card_do_free() > > card->releasing = true > > run until the end > > thread1 resume > > continues with trying to release > > It's a destructor and can't be called in parallel. That is, what the code above cares is the case where snd_card_free() is called explicitly even if the card is created with devres. So the check of card->releasing could be __snd_card_release() instead in snd_card_free(), too. Takashi
diff --git a/include/sound/core.h b/include/sound/core.h index c4ade121727d..331195b51237 100644 --- a/include/sound/core.h +++ b/include/sound/core.h @@ -117,6 +117,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 */ int sync_irq; /* assigned irq, used for PCM sync */ wait_queue_head_t remove_sleep; @@ -274,6 +276,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 1490568efdb0..7a507e8d24c1 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -134,6 +134,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; @@ -163,9 +166,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid, { struct snd_card *card; int err; -#ifdef CONFIG_SND_DEBUG - char name[8]; -#endif if (snd_BUG_ON(!card_ret)) return -EINVAL; @@ -176,6 +176,76 @@ 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; +#ifdef CONFIG_SND_DEBUG + char name[8]; +#endif + if (extra_size > 0) card->private_data = (char *)card + sizeof(struct snd_card); if (xid) @@ -197,7 +267,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 */ @@ -256,8 +325,6 @@ int snd_card_new(struct device *parent, int idx, const char *xid, sprintf(name, "card%d", idx); card->debugfs_root = debugfs_create_dir(name, sound_debugfs_root); #endif - - *card_ret = card; return 0; __error_ctl: @@ -266,7 +333,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); /** * snd_card_ref - Get the card object from the index @@ -481,6 +547,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 +565,8 @@ static int snd_card_do_free(struct snd_card *card) #endif if (card->release_completion) complete(card->release_completion); - kfree(card); + if (!card->managed) + kfree(card); return 0; } @@ -539,6 +607,9 @@ int snd_card_free(struct snd_card *card) DECLARE_COMPLETION_ONSTACK(released); int ret; + if (card->releasing) + return 0; + card->release_completion = &released; ret = snd_card_free_when_closed(card); if (ret) @@ -745,6 +816,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 @@ -768,6 +844,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; } err = snd_device_register_all(card);
As a second step for preliminary to widen the devres usages among sound drivers, this patch adds a new ALSA core API function, snd_devm_card_new(), to create a snd_card object via devres. When a card object is created by this new function, snd_card_free() is called automatically and the card object resource gets released at the device unbinding time. However, the story isn't that simple. A caveat is that we have to call snd_card_free() at the very first of the whole resource release procedure, in order to assure that the all exposed devices on user-space are deleted and sync with processes accessing those devices before releasing resources. 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 driver remove callback code. 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 | 101 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 98 insertions(+), 8 deletions(-)