| Message ID | 20210611015849.42589-1-skhan@linuxfoundation.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: Fix Media Controller API config checks | expand |
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 Regards, Hans > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Link: https://lore.kernel.org/alsa-devel/YLeAvT+R22FQ%2FEyw@mwanda/ > Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> > --- > drivers/media/mc/Makefile | 2 +- > include/media/media-dev-allocator.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/mc/Makefile b/drivers/media/mc/Makefile > index 119037f0e686..140f0a78540e 100644 > --- a/drivers/media/mc/Makefile > +++ b/drivers/media/mc/Makefile > @@ -3,7 +3,7 @@ > mc-objs := mc-device.o mc-devnode.o mc-entity.o \ > mc-request.o > > -ifeq ($(CONFIG_USB),y) > +ifeq ($(CONFIG_USB),$(filter $(CONFIG_USB),y m)) > mc-objs += mc-dev-allocator.o > endif > > diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h > index b35ea6062596..2ab54d426c64 100644 > --- a/include/media/media-dev-allocator.h > +++ b/include/media/media-dev-allocator.h > @@ -19,7 +19,7 @@ > > struct usb_device; > > -#if defined(CONFIG_MEDIA_CONTROLLER) && defined(CONFIG_USB) > +#if defined(CONFIG_MEDIA_CONTROLLER) && IS_ENABLED(CONFIG_USB) > /** > * media_device_usb_allocate() - Allocate and return struct &media device > * >
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
diff --git a/drivers/media/mc/Makefile b/drivers/media/mc/Makefile index 119037f0e686..140f0a78540e 100644 --- a/drivers/media/mc/Makefile +++ b/drivers/media/mc/Makefile @@ -3,7 +3,7 @@ mc-objs := mc-device.o mc-devnode.o mc-entity.o \ mc-request.o -ifeq ($(CONFIG_USB),y) +ifeq ($(CONFIG_USB),$(filter $(CONFIG_USB),y m)) mc-objs += mc-dev-allocator.o endif diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h index b35ea6062596..2ab54d426c64 100644 --- a/include/media/media-dev-allocator.h +++ b/include/media/media-dev-allocator.h @@ -19,7 +19,7 @@ struct usb_device; -#if defined(CONFIG_MEDIA_CONTROLLER) && defined(CONFIG_USB) +#if defined(CONFIG_MEDIA_CONTROLLER) && IS_ENABLED(CONFIG_USB) /** * media_device_usb_allocate() - Allocate and return struct &media device *
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. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Link: https://lore.kernel.org/alsa-devel/YLeAvT+R22FQ%2FEyw@mwanda/ Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> --- drivers/media/mc/Makefile | 2 +- include/media/media-dev-allocator.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)