Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1232314 > unrolled thread
| Started by | Luis de Bethencourt <luisbg@osg.samsung.com> |
|---|---|
| First post | 2015-09-24 20:00 +0200 |
| Last post | 2015-09-25 11:40 +0200 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v2] sound: oss: ad1848: Fix returned errno code in ad1848_init() Luis de Bethencourt <luisbg@osg.samsung.com> - 2015-09-24 20:00 +0200
Re: [PATCH v2] sound: oss: ad1848: Fix returned errno code in ad1848_init() Takashi Iwai <tiwai@suse.de> - 2015-09-24 21:00 +0200
Re: [PATCH v2] sound: oss: ad1848: Fix returned errno code in ad1848_init() Luis de Bethencourt <luisbg@osg.samsung.com> - 2015-09-24 21:10 +0200
Re: [PATCH v2] sound: oss: ad1848: Fix returned errno code in ad1848_init() Takashi Iwai <tiwai@suse.de> - 2015-09-25 08:40 +0200
Re: [PATCH v2] sound: oss: ad1848: Fix returned errno code in ad1848_init() Luis de Bethencourt <luisbg@osg.samsung.com> - 2015-09-25 11:40 +0200
| From | Luis de Bethencourt <luisbg@osg.samsung.com> |
|---|---|
| Date | 2015-09-24 20:00 +0200 |
| Subject | [PATCH v2] sound: oss: ad1848: Fix returned errno code in ad1848_init() |
| Message-ID | <qciYi-1MZ-21@gated-at.bofh.it> |
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.
Changed the checks of the return of ad1848_init() to be for >= 0,
instead of != -1.
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 | 4 ++--
sound/oss/dev_table.c | 2 +-
sound/oss/pss.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/oss/ad1848.c b/sound/oss/ad1848.c
index 10c8de1..58c6d31 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,
@@ -2007,7 +2007,7 @@ int ad1848_init (char *name, struct resource *ports, int irq, int dma_playback,
{
release_region(devc->base, 4);
kfree(portc);
- return -1;
+ return my_dev;
}
audio_devs[my_dev]->portc = portc;
diff --git a/sound/oss/dev_table.c b/sound/oss/dev_table.c
index 6dad515..abb3db4 100644
--- a/sound/oss/dev_table.c
+++ b/sound/oss/dev_table.c
@@ -148,7 +148,7 @@ EXPORT_SYMBOL(sound_install_mixer);
void sound_unload_audiodev(int dev)
{
- if (dev != -1) {
+ if (dev >= 0) {
DMAbuf_deinit(dev);
audio_devs[dev] = NULL;
unregister_sound_dsp((dev<<4)+3);
diff --git a/sound/oss/pss.c b/sound/oss/pss.c
index 81314f9..e84aafe 100644
--- a/sound/oss/pss.c
+++ b/sound/oss/pss.c
@@ -1091,7 +1091,7 @@ static int __init probe_pss_mss(struct address_info *hw_config)
pss_mixer_reset(devc);
attach_ms_sound(hw_config, ports, THIS_MODULE); /* Slot 0 */
- if (hw_config->slots[0] != -1)
+ if (hw_config->slots[0] >= 0)
{
/* The MSS driver installed itself */
audio_devs[hw_config->slots[0]]->coproc = &pss_coproc_operations;
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Takashi Iwai <tiwai@suse.de> |
|---|---|
| Date | 2015-09-24 21:00 +0200 |
| Message-ID | <qcjUm-389-11@gated-at.bofh.it> |
| In reply to | #1232314 |
On Thu, 24 Sep 2015 19:58:30 +0200,
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.
Where is the propagated error number referred? In other words, do we
have a clear merit of changing this old stuff with rather a risk of
regression?
thanks,
Takashi
> Changed the checks of the return of ad1848_init() to be for >= 0,
> instead of != -1.
>
> 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 | 4 ++--
> sound/oss/dev_table.c | 2 +-
> sound/oss/pss.c | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/sound/oss/ad1848.c b/sound/oss/ad1848.c
> index 10c8de1..58c6d31 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,
> @@ -2007,7 +2007,7 @@ int ad1848_init (char *name, struct resource *ports, int irq, int dma_playback,
> {
> release_region(devc->base, 4);
> kfree(portc);
> - return -1;
> + return my_dev;
> }
>
> audio_devs[my_dev]->portc = portc;
> diff --git a/sound/oss/dev_table.c b/sound/oss/dev_table.c
> index 6dad515..abb3db4 100644
> --- a/sound/oss/dev_table.c
> +++ b/sound/oss/dev_table.c
> @@ -148,7 +148,7 @@ EXPORT_SYMBOL(sound_install_mixer);
>
> void sound_unload_audiodev(int dev)
> {
> - if (dev != -1) {
> + if (dev >= 0) {
> DMAbuf_deinit(dev);
> audio_devs[dev] = NULL;
> unregister_sound_dsp((dev<<4)+3);
> diff --git a/sound/oss/pss.c b/sound/oss/pss.c
> index 81314f9..e84aafe 100644
> --- a/sound/oss/pss.c
> +++ b/sound/oss/pss.c
> @@ -1091,7 +1091,7 @@ static int __init probe_pss_mss(struct address_info *hw_config)
> pss_mixer_reset(devc);
> attach_ms_sound(hw_config, ports, THIS_MODULE); /* Slot 0 */
>
> - if (hw_config->slots[0] != -1)
> + if (hw_config->slots[0] >= 0)
> {
> /* The MSS driver installed itself */
> audio_devs[hw_config->slots[0]]->coproc = &pss_coproc_operations;
> --
> 2.5.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Luis de Bethencourt <luisbg@osg.samsung.com> |
|---|---|
| Date | 2015-09-24 21:10 +0200 |
| Message-ID | <qck42-3yV-23@gated-at.bofh.it> |
| In reply to | #1232337 |
On 24/09/15 19:56, Takashi Iwai wrote:
> On Thu, 24 Sep 2015 19:58:30 +0200,
> 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.
>
> Where is the propagated error number referred? In other words, do we
> have a clear merit of changing this old stuff with rather a risk of
> regression?
>
>
> thanks,
>
> Takashi
Hi Takashi,
The propagated error number is not referred. The only merit here is to
use the appropriate error number and silence the smatch tool.
Thanks for the review,
Luis
>
>> Changed the checks of the return of ad1848_init() to be for >= 0,
>> instead of != -1.
>>
>> 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 | 4 ++--
>> sound/oss/dev_table.c | 2 +-
>> sound/oss/pss.c | 2 +-
>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/sound/oss/ad1848.c b/sound/oss/ad1848.c
>> index 10c8de1..58c6d31 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,
>> @@ -2007,7 +2007,7 @@ int ad1848_init (char *name, struct resource *ports, int irq, int dma_playback,
>> {
>> release_region(devc->base, 4);
>> kfree(portc);
>> - return -1;
>> + return my_dev;
>> }
>>
>> audio_devs[my_dev]->portc = portc;
>> diff --git a/sound/oss/dev_table.c b/sound/oss/dev_table.c
>> index 6dad515..abb3db4 100644
>> --- a/sound/oss/dev_table.c
>> +++ b/sound/oss/dev_table.c
>> @@ -148,7 +148,7 @@ EXPORT_SYMBOL(sound_install_mixer);
>>
>> void sound_unload_audiodev(int dev)
>> {
>> - if (dev != -1) {
>> + if (dev >= 0) {
>> DMAbuf_deinit(dev);
>> audio_devs[dev] = NULL;
>> unregister_sound_dsp((dev<<4)+3);
>> diff --git a/sound/oss/pss.c b/sound/oss/pss.c
>> index 81314f9..e84aafe 100644
>> --- a/sound/oss/pss.c
>> +++ b/sound/oss/pss.c
>> @@ -1091,7 +1091,7 @@ static int __init probe_pss_mss(struct address_info *hw_config)
>> pss_mixer_reset(devc);
>> attach_ms_sound(hw_config, ports, THIS_MODULE); /* Slot 0 */
>>
>> - if (hw_config->slots[0] != -1)
>> + if (hw_config->slots[0] >= 0)
>> {
>> /* The MSS driver installed itself */
>> audio_devs[hw_config->slots[0]]->coproc = &pss_coproc_operations;
>> --
>> 2.5.1
>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Takashi Iwai <tiwai@suse.de> |
|---|---|
| Date | 2015-09-25 08:40 +0200 |
| Message-ID | <qcuPM-1Sp-19@gated-at.bofh.it> |
| In reply to | #1232342 |
On Thu, 24 Sep 2015 21:08:39 +0200,
Luis de Bethencourt wrote:
>
> On 24/09/15 19:56, Takashi Iwai wrote:
> > On Thu, 24 Sep 2015 19:58:30 +0200,
> > 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.
> >
> > Where is the propagated error number referred? In other words, do we
> > have a clear merit of changing this old stuff with rather a risk of
> > regression?
> >
> >
> > thanks,
> >
> > Takashi
>
> Hi Takashi,
>
> The propagated error number is not referred. The only merit here is to
> use the appropriate error number and silence the smatch tool.
In that case, I prefer leaving as is, sorry. The code is using -1 for
its purpose, and the error number isn't important there in anyway
since the driver doesn't abort at all.
thanks,
Takashi
>
> Thanks for the review,
> Luis
>
> >
> >> Changed the checks of the return of ad1848_init() to be for >= 0,
> >> instead of != -1.
> >>
> >> 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 | 4 ++--
> >> sound/oss/dev_table.c | 2 +-
> >> sound/oss/pss.c | 2 +-
> >> 3 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/sound/oss/ad1848.c b/sound/oss/ad1848.c
> >> index 10c8de1..58c6d31 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,
> >> @@ -2007,7 +2007,7 @@ int ad1848_init (char *name, struct resource *ports, int irq, int dma_playback,
> >> {
> >> release_region(devc->base, 4);
> >> kfree(portc);
> >> - return -1;
> >> + return my_dev;
> >> }
> >>
> >> audio_devs[my_dev]->portc = portc;
> >> diff --git a/sound/oss/dev_table.c b/sound/oss/dev_table.c
> >> index 6dad515..abb3db4 100644
> >> --- a/sound/oss/dev_table.c
> >> +++ b/sound/oss/dev_table.c
> >> @@ -148,7 +148,7 @@ EXPORT_SYMBOL(sound_install_mixer);
> >>
> >> void sound_unload_audiodev(int dev)
> >> {
> >> - if (dev != -1) {
> >> + if (dev >= 0) {
> >> DMAbuf_deinit(dev);
> >> audio_devs[dev] = NULL;
> >> unregister_sound_dsp((dev<<4)+3);
> >> diff --git a/sound/oss/pss.c b/sound/oss/pss.c
> >> index 81314f9..e84aafe 100644
> >> --- a/sound/oss/pss.c
> >> +++ b/sound/oss/pss.c
> >> @@ -1091,7 +1091,7 @@ static int __init probe_pss_mss(struct address_info *hw_config)
> >> pss_mixer_reset(devc);
> >> attach_ms_sound(hw_config, ports, THIS_MODULE); /* Slot 0 */
> >>
> >> - if (hw_config->slots[0] != -1)
> >> + if (hw_config->slots[0] >= 0)
> >> {
> >> /* The MSS driver installed itself */
> >> audio_devs[hw_config->slots[0]]->coproc = &pss_coproc_operations;
> >> --
> >> 2.5.1
> >>
> >>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Luis de Bethencourt <luisbg@osg.samsung.com> |
|---|---|
| Date | 2015-09-25 11:40 +0200 |
| Message-ID | <qcxDX-5PG-1@gated-at.bofh.it> |
| In reply to | #1232595 |
On 25/09/15 07:32, Takashi Iwai wrote:
> On Thu, 24 Sep 2015 21:08:39 +0200,
> Luis de Bethencourt wrote:
>>
>> On 24/09/15 19:56, Takashi Iwai wrote:
>>> On Thu, 24 Sep 2015 19:58:30 +0200,
>>> 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.
>>>
>>> Where is the propagated error number referred? In other words, do we
>>> have a clear merit of changing this old stuff with rather a risk of
>>> regression?
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>
>> Hi Takashi,
>>
>> The propagated error number is not referred. The only merit here is to
>> use the appropriate error number and silence the smatch tool.
>
> In that case, I prefer leaving as is, sorry. The code is using -1 for
> its purpose, and the error number isn't important there in anyway
> since the driver doesn't abort at all.
>
>
> thanks,
>
> Takashi
Hi Takashi,
I understand. I was on the fence myself about it being worth it or not,
as I mentioned in the review of the first version of the patch.
Thank you for the review,
Luis
>
>>
>> Thanks for the review,
>> Luis
>>
>>>
>>>> Changed the checks of the return of ad1848_init() to be for >= 0,
>>>> instead of != -1.
>>>>
>>>> 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 | 4 ++--
>>>> sound/oss/dev_table.c | 2 +-
>>>> sound/oss/pss.c | 2 +-
>>>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/sound/oss/ad1848.c b/sound/oss/ad1848.c
>>>> index 10c8de1..58c6d31 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,
>>>> @@ -2007,7 +2007,7 @@ int ad1848_init (char *name, struct resource *ports, int irq, int dma_playback,
>>>> {
>>>> release_region(devc->base, 4);
>>>> kfree(portc);
>>>> - return -1;
>>>> + return my_dev;
>>>> }
>>>>
>>>> audio_devs[my_dev]->portc = portc;
>>>> diff --git a/sound/oss/dev_table.c b/sound/oss/dev_table.c
>>>> index 6dad515..abb3db4 100644
>>>> --- a/sound/oss/dev_table.c
>>>> +++ b/sound/oss/dev_table.c
>>>> @@ -148,7 +148,7 @@ EXPORT_SYMBOL(sound_install_mixer);
>>>>
>>>> void sound_unload_audiodev(int dev)
>>>> {
>>>> - if (dev != -1) {
>>>> + if (dev >= 0) {
>>>> DMAbuf_deinit(dev);
>>>> audio_devs[dev] = NULL;
>>>> unregister_sound_dsp((dev<<4)+3);
>>>> diff --git a/sound/oss/pss.c b/sound/oss/pss.c
>>>> index 81314f9..e84aafe 100644
>>>> --- a/sound/oss/pss.c
>>>> +++ b/sound/oss/pss.c
>>>> @@ -1091,7 +1091,7 @@ static int __init probe_pss_mss(struct address_info *hw_config)
>>>> pss_mixer_reset(devc);
>>>> attach_ms_sound(hw_config, ports, THIS_MODULE); /* Slot 0 */
>>>>
>>>> - if (hw_config->slots[0] != -1)
>>>> + if (hw_config->slots[0] >= 0)
>>>> {
>>>> /* The MSS driver installed itself */
>>>> audio_devs[hw_config->slots[0]]->coproc = &pss_coproc_operations;
>>>> --
>>>> 2.5.1
>>>>
>>>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web