From: Hans Verkuil <hverkuil@xs4all.nl> To: Shuah Khan <skhan@linuxfoundation.org>, sakari.ailus@linux.intel.com, laurent.pinchart@ideasonboard.com, dan.carpenter@oracle.com, mchehab@kernel.org Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org Subject: Re: [PATCH] media: Fix Media Controller API config checks Date: Tue, 15 Jun 2021 15:36:05 +0200 [thread overview] Message-ID: <3745852a-a14d-3e66-dd9f-409ec7e43f48@xs4all.nl> (raw) In-Reply-To: <20210611015849.42589-1-skhan@linuxfoundation.org> 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 > * >
WARNING: multiple messages have this Message-ID (diff)
From: Hans Verkuil <hverkuil@xs4all.nl> To: Shuah Khan <skhan@linuxfoundation.org>, sakari.ailus@linux.intel.com, laurent.pinchart@ideasonboard.com, dan.carpenter@oracle.com, mchehab@kernel.org Cc: alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org Subject: Re: [PATCH] media: Fix Media Controller API config checks Date: Tue, 15 Jun 2021 15:36:05 +0200 [thread overview] Message-ID: <3745852a-a14d-3e66-dd9f-409ec7e43f48@xs4all.nl> (raw) In-Reply-To: <20210611015849.42589-1-skhan@linuxfoundation.org> 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 > * >
next prev parent reply other threads:[~2021-06-15 13:36 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-11 1:58 [PATCH] media: Fix Media Controller API config checks Shuah Khan 2021-06-11 1:58 ` Shuah Khan 2021-06-15 13:36 ` Hans Verkuil [this message] 2021-06-15 13:36 ` Hans Verkuil 2021-06-15 14:09 ` Shuah Khan 2021-06-15 14:09 ` Shuah Khan 2021-06-15 16:55 ` Shuah Khan 2021-06-15 16:55 ` Shuah Khan 2021-06-16 3:15 ` Shuah Khan 2021-06-16 3:15 ` Shuah Khan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3745852a-a14d-3e66-dd9f-409ec7e43f48@xs4all.nl \ --to=hverkuil@xs4all.nl \ --cc=alsa-devel@alsa-project.org \ --cc=dan.carpenter@oracle.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=mchehab@kernel.org \ --cc=sakari.ailus@linux.intel.com \ --cc=skhan@linuxfoundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.