[bug report] media: sound/usb: Use Media Controller API to share media resources
Hello Shuah Khan,
The patch 66354f18fe5f: "media: sound/usb: Use Media Controller API to share media resources" from Apr 1, 2019, leads to the following static checker warning:
sound/usb/media.c:287 snd_media_device_create() warn: 'mdev' can also be NULL
sound/usb/media.c 270 271 mdev = media_device_usb_allocate(usbdev, KBUILD_MODNAME, THIS_MODULE); ^^^^
If CONFIG_MEDIA_CONTROLLER is disabled then "mdev" is NULL.
272 if (IS_ERR(mdev)) 273 return -ENOMEM; 274 275 /* save media device - avoid lookups */ 276 chip->media_dev = mdev; 277 278 snd_mixer_init: 279 /* Create media entities for mixer and control dev */ 280 ret = snd_media_mixer_init(chip); 281 /* media_device might be registered, print error and continue */ 282 if (ret) 283 dev_err(&usbdev->dev, 284 "Couldn't create media mixer entities. Error: %d\n", 285 ret); 286 287 if (!media_devnode_is_registered(mdev->devnode)) { ^^^^^^^^^^^^^ dereferenced without checking here
288 /* dont'register if snd_media_mixer_init() failed */ 289 if (ret) 290 goto create_fail; 291 292 /* register media_device */ 293 ret = media_device_register(mdev); 294 create_fail: 295 if (ret) { 296 snd_media_mixer_delete(chip); 297 media_device_delete(mdev, KBUILD_MODNAME, THIS_MODULE); 298 /* clear saved media_dev */ 299 chip->media_dev = NULL; 300 dev_err(&usbdev->dev, 301 "Couldn't register media device. Error: %d\n", 302 ret); 303 return ret; 304 } 305 } 306 307 return ret; 308 }
regards, dan carpenter
On 6/2/21 6:59 AM, Dan Carpenter wrote:
Hello Shuah Khan,
The patch 66354f18fe5f: "media: sound/usb: Use Media Controller API to share media resources" from Apr 1, 2019, leads to the following static checker warning:
sound/usb/media.c:287 snd_media_device_create() warn: 'mdev' can also be NULL
sound/usb/media.c 270 271 mdev = media_device_usb_allocate(usbdev, KBUILD_MODNAME, THIS_MODULE); ^^^^
If CONFIG_MEDIA_CONTROLLER is disabled then "mdev" is NULL.
If CONFIG_MEDIA_CONTROLLER is disabled, this file won't be compiled. Please see below clause in the Makefile.
sound/usb/Makefile: snd-usb-audio-$(CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER) += media.o
Also, this select in sound/usb/Kconfig selects appropriate configs.
select SND_USB_AUDIO_USE_MEDIA_CONTROLLER if MEDIA_CONTROLLER && (MEDIA_SUPPORT=y || MEDIA_SUPPORT=SND_USB_AUDIO)
We are good here with the above in place to make sure media.c code isn't in play when CONFIG_MEDIA_CONTROLLER is disabled.
thanks, -- Shuah
On Tue, Jun 08, 2021 at 11:53:53AM -0600, Shuah Khan wrote:
On 6/2/21 6:59 AM, Dan Carpenter wrote:
Hello Shuah Khan,
The patch 66354f18fe5f: "media: sound/usb: Use Media Controller API to share media resources" from Apr 1, 2019, leads to the following static checker warning:
sound/usb/media.c:287 snd_media_device_create() warn: 'mdev' can also be NULL
sound/usb/media.c 270 271 mdev = media_device_usb_allocate(usbdev, KBUILD_MODNAME, THIS_MODULE); ^^^^
If CONFIG_MEDIA_CONTROLLER is disabled then "mdev" is NULL.
If CONFIG_MEDIA_CONTROLLER is disabled, this file won't be compiled. Please see below clause in the Makefile.
sound/usb/Makefile: snd-usb-audio-$(CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER) += media.o
Also, this select in sound/usb/Kconfig selects appropriate configs.
select SND_USB_AUDIO_USE_MEDIA_CONTROLLER if MEDIA_CONTROLLER && (MEDIA_SUPPORT=y || MEDIA_SUPPORT=SND_USB_AUDIO)
We are good here with the above in place to make sure media.c code isn't in play when CONFIG_MEDIA_CONTROLLER is disabled.
Smatch is working based on a real .config but I misunderstood the problem.
include/media/media-dev-allocator.h 21 22 #if defined(CONFIG_MEDIA_CONTROLLER) && defined(CONFIG_USB) ^^^^^^^^^^^^^^^^^^^ When I have CONFIG_USB=m then this is false, but when I use CONFIG_USB=y then this is true. I'm not an expert on the kernel build system so I can't explain why defined() is not working as expected but I know that if we used IS_ENABLED(CONFIG_USB) that would work.
#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER) && IS_ENABLED)CONFIG_USB)
23 /** 24 * media_device_usb_allocate() - Allocate and return struct &media device 25 * 26 * @udev: struct &usb_device pointer 27 * @module_name: should be filled with %KBUILD_MODNAME 28 * @owner: struct module pointer %THIS_MODULE for the driver. 29 * %THIS_MODULE is null for a built-in driver. 30 * It is safe even when %THIS_MODULE is null. 31 * 32 * This interface should be called to allocate a Media Device when multiple 33 * drivers share usb_device and the media device. This interface allocates 34 * &media_device structure and calls media_device_usb_init() to initialize 35 * it. 36 * 37 */ 38 struct media_device *media_device_usb_allocate(struct usb_device *udev, 39 const char *module_name, 40 struct module *owner);
regards, dan carpenter
On 6/8/21 1:03 PM, Dan Carpenter wrote:
On Tue, Jun 08, 2021 at 11:53:53AM -0600, Shuah Khan wrote:
On 6/2/21 6:59 AM, Dan Carpenter wrote:
Hello Shuah Khan,
The patch 66354f18fe5f: "media: sound/usb: Use Media Controller API to share media resources" from Apr 1, 2019, leads to the following static checker warning:
sound/usb/media.c:287 snd_media_device_create() warn: 'mdev' can also be NULL
sound/usb/media.c 270 271 mdev = media_device_usb_allocate(usbdev, KBUILD_MODNAME, THIS_MODULE); ^^^^
If CONFIG_MEDIA_CONTROLLER is disabled then "mdev" is NULL.
If CONFIG_MEDIA_CONTROLLER is disabled, this file won't be compiled. Please see below clause in the Makefile.
sound/usb/Makefile: snd-usb-audio-$(CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER) += media.o
Also, this select in sound/usb/Kconfig selects appropriate configs.
select SND_USB_AUDIO_USE_MEDIA_CONTROLLER if MEDIA_CONTROLLER && (MEDIA_SUPPORT=y || MEDIA_SUPPORT=SND_USB_AUDIO)
We are good here with the above in place to make sure media.c code isn't in play when CONFIG_MEDIA_CONTROLLER is disabled.
Smatch is working based on a real .config but I misunderstood the problem.
include/media/media-dev-allocator.h 21 22 #if defined(CONFIG_MEDIA_CONTROLLER) && defined(CONFIG_USB) ^^^^^^^^^^^^^^^^^^^ When I have CONFIG_USB=m then this is false, but when I use CONFIG_USB=y then this is true. I'm not an expert on the kernel build system so I can't explain why defined() is not working as expected but I know that if we used IS_ENABLED(CONFIG_USB) that would work.
#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER) && IS_ENABLED)CONFIG_USB)
Good find. IS_ENABLED is the right usage. When CONFIG_USB=m, CONFIG_USB_MODULE is defined and CONFIG_USB isn't.
IS_ENABLED does the following and catches both, where as defined() checks just for CONFIG_USB.
include/linux/kconfig.h 71 #define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))
I will send patch to fix this. Thanks for finding it.
thanks, -- Shuah
participants (2)
-
Dan Carpenter -
Shuah Khan