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
