Re: [PATCH] media: Fix Media Controller API config checks

[Shuah Khan <skhan@linuxfoundation.org>]

On 6/15/21 10:55 AM, Shuah Khan wrote:
> On 6/15/21 7:36 AM, Hans Verkuil wrote:
>> Hi Shuah,
>>
>> On 11/06/2021 03:58, Shuah Khan wrote:
>>> Smatch static checker warns that "mdev" can be null:
>>>
>>> sound/usb/media.c:287 snd_media_device_create()
>>>      warn: 'mdev' can also be NULL
>>>
>>> If CONFIG_MEDIA_CONTROLLER is disabled, this file should not be included
>>> in the build.
>>>
>>> The below conditions in the sound/usb/Makefile are in place to ensure 
>>> that
>>> media.c isn't included in the build.
>>>
>>> sound/usb/Makefile:
>>> snd-usb-audio-$(CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER) += media.o
>>>
>>> select SND_USB_AUDIO_USE_MEDIA_CONTROLLER if MEDIA_CONTROLLER &&
>>>         (MEDIA_SUPPORT=y || MEDIA_SUPPORT=SND_USB_AUDIO)
>>>
>>> The following config check in include/media/media-dev-allocator.h is
>>> in place to enable the API only when CONFIG_MEDIA_CONTROLLER and
>>> CONFIG_USB are enabled.
>>>
>>>   #if defined(CONFIG_MEDIA_CONTROLLER) && defined(CONFIG_USB)
>>>
>>> This check doesn't work as intended when CONFIG_USB=m. When 
>>> CONFIG_USB=m,
>>> CONFIG_USB_MODULE is defined and CONFIG_USB is not. The above config 
>>> check
>>> doesn't catch that CONFIG_USB is defined as a module and disables the 
>>> API.
>>> This results in sound/usb enabling Media Controller specific ALSA driver
>>> code, while Media disables the Media Controller API.
>>>
>>> Fix the problem requires two changes:
>>>
>>> 1. Change the check to use IS_ENABLED to detect when CONFIG_USB is 
>>> enabled
>>>     as a module or static. Since CONFIG_MEDIA_CONTROLLER is a bool, 
>>> leave
>>>     the check unchanged to be consistent with drivers/media/Makefile.
>>>
>>> 2. Change the drivers/media/mc/Makefile to include mc-dev-allocator.o
>>>     in mc-objs when CONFIG_USB is y or m.
>>
>> If I test this patch, then I get:
>>
>> drivers/media/mc/mc-dev-allocator.c:97:22: error: redefinition of 
>> 'media_device_usb_allocate'
>>     97 | struct media_device *media_device_usb_allocate(struct 
>> usb_device *udev,
>>        |                      ^~~~~~~~~~~~~~~~~~~~~~~~~
>> In file included from drivers/media/mc/mc-dev-allocator.c:24:
>> include/media/media-dev-allocator.h:55:36: note: previous definition 
>> of 'media_device_usb_allocate' was here
>>     55 | static inline struct media_device *media_device_usb_allocate(
>>        |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/media/mc/mc-dev-allocator.c:119:6: error: redefinition of 
>> 'media_device_delete'
>>    119 | void media_device_delete(struct media_device *mdev, const 
>> char *module_name,
>>        |      ^~~~~~~~~~~~~~~~~~~
>> In file included from drivers/media/mc/mc-dev-allocator.c:24:
>> include/media/media-dev-allocator.h:59:20: note: previous definition 
>> of 'media_device_delete' was here
>>     59 | static inline void media_device_delete(
>>        |                    ^~~~~~~~~~~~~~~~~~~
>>
>> The .config has:
>>
>> # CONFIG_USB_SUPPORT is not set
>> CONFIG_MEDIA_CONTROLLER=y
>>
> 

Hi Hans,

I don't know why I made the logic fancy and complex. Fixed now and
sending v2 shortly. I made sure all the combinations are tested now.

thanks,
-- Shuah