Re: [PATCH v5 22/22] sound/usb: Use Media Controller API to share media resources

[Shuah Khan]

On 03/02/2016 01:41 PM, Dan Carpenter wrote:
> On Wed, Mar 02, 2016 at 09:50:31AM -0700, Shuah Khan wrote:
>> +	mctl = kzalloc(sizeof(*mctl), GFP_KERNEL);
>> +	if (!mctl)
>> +		return -ENOMEM;
>> +
>> +	mctl->media_dev = mdev;
>> +	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> +		intf_type = MEDIA_INTF_T_ALSA_PCM_PLAYBACK;
>> +		mctl->media_entity.function = MEDIA_ENT_F_AUDIO_PLAYBACK;
>> +		mctl->media_pad.flags = MEDIA_PAD_FL_SOURCE;
>> +		mixer_pad = 1;
>> +	} else {
>> +		intf_type = MEDIA_INTF_T_ALSA_PCM_CAPTURE;
>> +		mctl->media_entity.function = MEDIA_ENT_F_AUDIO_CAPTURE;
>> +		mctl->media_pad.flags = MEDIA_PAD_FL_SINK;
>> +		mixer_pad = 2;
>> +	}
>> +	mctl->media_entity.name = pcm->name;
>> +	media_entity_pads_init(&mctl->media_entity, 1, &mctl->media_pad);
>> +	ret =  media_device_register_entity(mctl->media_dev,
>> +					    &mctl->media_entity);
>> +	if (ret)
>> +		goto err1;
> 
> Could we give this label a meaningful name instead of a number?
> goto free_mctl;

I do see other places where numbered labels are used.
Names might help with code readability.

register_entity_fail probably makes more sense as a
label than free_mctl. In any case, I can address the
labels in a follow-on patch.

> 
>> +
>> +	mctl->intf_devnode = media_devnode_create(mdev, intf_type, 0,
>> +						  MAJOR(pcm_dev->devt),
>> +						  MINOR(pcm_dev->devt));
>> +	if (!mctl->intf_devnode) {
>> +		ret = -ENOMEM;
>> +		goto err2;
> 
> goto unregister_device;
> 
>> +	}
>> +	mctl->intf_link = media_create_intf_link(&mctl->media_entity,
>> +						 &mctl->intf_devnode->intf,
>> +						 MEDIA_LNK_FL_ENABLED);
>> +	if (!mctl->intf_link) {
>> +		ret = -ENOMEM;
>> +		goto err3;
> 
> goto delete_devnode;
> 
>> +	}
>> +
>> +	/* create link between mixer and audio */
>> +	media_device_for_each_entity(entity, mdev) {
>> +		switch (entity->function) {
>> +		case MEDIA_ENT_F_AUDIO_MIXER:
>> +			ret = media_create_pad_link(entity, mixer_pad,
>> +						    &mctl->media_entity, 0,
>> +						    MEDIA_LNK_FL_ENABLED);
>> +			if (ret)
>> +				goto err4;
> 
> This is a bit weird because we're inside a loop.  Shouldn't we call
> media_entity_remove_links() or something if this is the second time
> through the loop?

Links are removed from media_device_unregister_entity()
which is called in the error path.

> 
> I don't understand this.  The kernel has the media_entity_cleanup() stub
> function which is supposed to do this but it hasn't been implemented
> yet?
> 

Please see above. Links are removed when entity is
unregistered. media_entity_cleanup() is a stub. It
isn't intended for removing links.

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@xxxxxxxxxxxxxxx | (970) 217-8978