All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] media: Fix Media Controller API config checks
@ 2021-06-11  1:58 ` Shuah Khan
  0 siblings, 0 replies; 10+ messages in thread
From: Shuah Khan @ 2021-06-11  1:58 UTC (permalink / raw)
  To: sakari.ailus, laurent.pinchart, dan.carpenter, mchehab
  Cc: Shuah Khan, linux-media, linux-kernel, alsa-devel

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

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
  *
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH] media: Fix Media Controller API config checks
@ 2021-06-11  1:58 ` Shuah Khan
  0 siblings, 0 replies; 10+ messages in thread
From: Shuah Khan @ 2021-06-11  1:58 UTC (permalink / raw)
  To: sakari.ailus, laurent.pinchart, dan.carpenter, mchehab
  Cc: alsa-devel, linux-media, linux-kernel, Shuah Khan

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

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
  *
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] media: Fix Media Controller API config checks
  2021-06-11  1:58 ` Shuah Khan
@ 2021-06-15 13:36   ` Hans Verkuil
  -1 siblings, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2021-06-15 13:36 UTC (permalink / raw)
  To: Shuah Khan, sakari.ailus, laurent.pinchart, dan.carpenter, mchehab
  Cc: linux-media, linux-kernel, alsa-devel

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
>   *
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] media: Fix Media Controller API config checks
@ 2021-06-15 13:36   ` Hans Verkuil
  0 siblings, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2021-06-15 13:36 UTC (permalink / raw)
  To: Shuah Khan, sakari.ailus, laurent.pinchart, dan.carpenter, mchehab
  Cc: alsa-devel, linux-kernel, linux-media

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
>   *
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] media: Fix Media Controller API config checks
  2021-06-15 13:36   ` Hans Verkuil
@ 2021-06-15 14:09     ` Shuah Khan
  -1 siblings, 0 replies; 10+ messages in thread
From: Shuah Khan @ 2021-06-15 14:09 UTC (permalink / raw)
  To: Hans Verkuil, sakari.ailus, laurent.pinchart, dan.carpenter, mchehab
  Cc: linux-media, linux-kernel, alsa-devel, Shuah Khan

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
>
Oops. I tried different combinations of CONFIG_USB and didn't try this
one. Will send v2.

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] media: Fix Media Controller API config checks
@ 2021-06-15 14:09     ` Shuah Khan
  0 siblings, 0 replies; 10+ messages in thread
From: Shuah Khan @ 2021-06-15 14:09 UTC (permalink / raw)
  To: Hans Verkuil, sakari.ailus, laurent.pinchart, dan.carpenter, mchehab
  Cc: alsa-devel, Shuah Khan, linux-kernel, linux-media

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
>
Oops. I tried different combinations of CONFIG_USB and didn't try this
one. Will send v2.

thanks,
-- Shuah

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] media: Fix Media Controller API config checks
  2021-06-15 13:36   ` Hans Verkuil
@ 2021-06-15 16:55     ` Shuah Khan
  -1 siblings, 0 replies; 10+ messages in thread
From: Shuah Khan @ 2021-06-15 16:55 UTC (permalink / raw)
  To: Hans Verkuil, sakari.ailus, laurent.pinchart, dan.carpenter, mchehab
  Cc: linux-media, linux-kernel, alsa-devel, Shuah Khan

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
> 

Hans,

Can you share your full config with me?

thanks,
-- Shuah


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] media: Fix Media Controller API config checks
@ 2021-06-15 16:55     ` Shuah Khan
  0 siblings, 0 replies; 10+ messages in thread
From: Shuah Khan @ 2021-06-15 16:55 UTC (permalink / raw)
  To: Hans Verkuil, sakari.ailus, laurent.pinchart, dan.carpenter, mchehab
  Cc: alsa-devel, Shuah Khan, linux-kernel, linux-media

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
> 

Hans,

Can you share your full config with me?

thanks,
-- Shuah


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] media: Fix Media Controller API config checks
  2021-06-15 16:55     ` Shuah Khan
@ 2021-06-16  3:15       ` Shuah Khan
  -1 siblings, 0 replies; 10+ messages in thread
From: Shuah Khan @ 2021-06-16  3:15 UTC (permalink / raw)
  To: Hans Verkuil, sakari.ailus, laurent.pinchart, dan.carpenter, mchehab
  Cc: linux-media, linux-kernel, alsa-devel, Shuah Khan

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] media: Fix Media Controller API config checks
@ 2021-06-16  3:15       ` Shuah Khan
  0 siblings, 0 replies; 10+ messages in thread
From: Shuah Khan @ 2021-06-16  3:15 UTC (permalink / raw)
  To: Hans Verkuil, sakari.ailus, laurent.pinchart, dan.carpenter, mchehab
  Cc: alsa-devel, Shuah Khan, linux-kernel, linux-media

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-06-16  3:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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.