Re: [PATCH 02/51] ALSA: core: Add managed card creation

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

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