[alsa-devel] [PATCH] sound: oss: ad1848: Fix returned errno code in ad1848_init()
The driver is using -1 instead of the -ENOMEM defined macro to specify that a buffer allocation failed. Since the error number is propagated, the caller will get a -EPERM which is the wrong error condition.
Smatch tool warning: ad1848_init() warn: returning -1 instead of -ENOMEM is sloppy
Signed-off-by: Luis de Bethencourt luisbg@osg.samsung.com --- sound/oss/ad1848.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/oss/ad1848.c b/sound/oss/ad1848.c index 10c8de1..6b35656 100644 --- a/sound/oss/ad1848.c +++ b/sound/oss/ad1848.c @@ -1992,7 +1992,7 @@ int ad1848_init (char *name, struct resource *ports, int irq, int dma_playback, portc = kmalloc(sizeof(ad1848_port_info), GFP_KERNEL); if(portc==NULL) { release_region(devc->base, 4); - return -1; + return -ENOMEM; }
if ((my_dev = sound_install_audiodrv(AUDIO_DRIVER_VERSION,
On Tue, Sep 22, 2015 at 04:37:37PM +0100, Luis de Bethencourt wrote:
The driver is using -1 instead of the -ENOMEM defined macro to specify that a buffer allocation failed. Since the error number is propagated, the caller will get a -EPERM which is the wrong error condition.
Smatch tool warning: ad1848_init() warn: returning -1 instead of -ENOMEM is sloppy
Signed-off-by: Luis de Bethencourt luisbg@osg.samsung.com
sound/oss/ad1848.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/oss/ad1848.c b/sound/oss/ad1848.c index 10c8de1..6b35656 100644 --- a/sound/oss/ad1848.c +++ b/sound/oss/ad1848.c @@ -1992,7 +1992,7 @@ int ad1848_init (char *name, struct resource *ports, int irq, int dma_playback, portc = kmalloc(sizeof(ad1848_port_info), GFP_KERNEL); if(portc==NULL) { release_region(devc->base, 4);
return -1;
return -ENOMEM;
The return value of ad1848_init is stored in hw_config->slots[0]. And in sound/oss/pss.c hw_config->slots[0] is checked like: if (hw_config->slots[0] != -1)
So, you just changed the functionality of the driver.
regards sudip
On 22/09/15 17:46, Sudip Mukherjee wrote:
On Tue, Sep 22, 2015 at 04:37:37PM +0100, Luis de Bethencourt wrote:
The driver is using -1 instead of the -ENOMEM defined macro to specify that a buffer allocation failed. Since the error number is propagated, the caller will get a -EPERM which is the wrong error condition.
Smatch tool warning: ad1848_init() warn: returning -1 instead of -ENOMEM is sloppy
Signed-off-by: Luis de Bethencourt luisbg@osg.samsung.com
sound/oss/ad1848.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/oss/ad1848.c b/sound/oss/ad1848.c index 10c8de1..6b35656 100644 --- a/sound/oss/ad1848.c +++ b/sound/oss/ad1848.c @@ -1992,7 +1992,7 @@ int ad1848_init (char *name, struct resource *ports, int irq, int dma_playback, portc = kmalloc(sizeof(ad1848_port_info), GFP_KERNEL); if(portc==NULL) { release_region(devc->base, 4);
return -1;
return -ENOMEM;The return value of ad1848_init is stored in hw_config->slots[0]. And in sound/oss/pss.c hw_config->slots[0] is checked like: if (hw_config->slots[0] != -1)
So, you just changed the functionality of the driver.
regards sudip
Hi Sudip,
True! I missed that.
This change will mean the block for 'if (hw_config->slots[0] != -1)' will run and it shouldn't.
In ad1848.c:1998 sound_install_audiodrv() can return -ENOMEM as well, but this is turned into -1 also. All errno codes are ignored in sound/oss/pss.c, not worth it.
I'm withdrawing my patch. Sorry for this.
Is there any other way to silence the smatch warning?
Thanks for the review, Luis
On Tue, Sep 22, 2015 at 08:11:13PM +0100, Luis de Bethencourt wrote:
On 22/09/15 17:46, Sudip Mukherjee wrote:
On Tue, Sep 22, 2015 at 04:37:37PM +0100, Luis de Bethencourt wrote:
<snip>
Hi Sudip,
True! I missed that.
This change will mean the block for 'if (hw_config->slots[0] != -1)' will run and it shouldn't.
In ad1848.c:1998 sound_install_audiodrv() can return -ENOMEM as well, but this is turned into -1 also. All errno codes are ignored in sound/oss/pss.c, not worth it.
I'm withdrawing my patch. Sorry for this.
Is there any other way to silence the smatch warning?
Yes, ad1848_init() is only called from 3 places and everytime the return is stored in hw_config->slots[0]. So change in all these 3 places and then slots[0] is being checked in sound_unload_audiodev() and also in sound/oss/pss.c, so you need to modify those checks also. But these are all just from a quick grep, if you really want to change then you need to see the driver carefully for other places that i might have missed.
regards sudip
participants (2)
-
Luis de Bethencourt -
Sudip Mukherjee