* [PATCH] sound: pci: emu10k1: code refactoring and casting removal
@ 2013-10-16 22:11 Geyslan G. Bem
2013-10-17 6:15 ` Takashi Iwai
0 siblings, 1 reply; 7+ messages in thread
From: Geyslan G. Bem @ 2013-10-16 22:11 UTC (permalink / raw)
Cc: kernel-br, Geyslan G. Bem, Jaroslav Kysela, Takashi Iwai,
Bill Pemberton, moderated list:SOUND, open list
Partially restructures _snd_emu10k1_audigy_init_efx() and
_snd_emu10k1_init_efx() functions.
Removes useless casting (void *) from value returned by kcalloc;
see Documentation/CodingStyle, Chap 14.
Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
---
sound/pci/emu10k1/emufx.c | 84 ++++++++++++++++++++++++++---------------------
1 file changed, 46 insertions(+), 38 deletions(-)
diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
index 0275209..afd4691 100644
--- a/sound/pci/emu10k1/emufx.c
+++ b/sound/pci/emu10k1/emufx.c
@@ -1182,15 +1182,20 @@ static int _snd_emu10k1_audigy_init_efx(struct snd_emu10k1 *emu)
u32 *gpr_map;
mm_segment_t seg;
- if ((icode = kzalloc(sizeof(*icode), GFP_KERNEL)) == NULL ||
- (icode->gpr_map = (u_int32_t __user *)
- kcalloc(512 + 256 + 256 + 2 * 1024, sizeof(u_int32_t),
- GFP_KERNEL)) == NULL ||
- (controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
- sizeof(*controls), GFP_KERNEL)) == NULL) {
- err = -ENOMEM;
- goto __err;
- }
+ err = -ENOMEM;
+ icode = kzalloc(sizeof(*icode), GFP_KERNEL);
+ if (!icode)
+ return err;
+
+ icode->gpr_map = (__user) kcalloc(512 + 256 + 256 + 2 * 1024,
+ sizeof(u_int32_t), GFP_KERNEL);
+ if (!icode->gpr_map)
+ goto __err_gpr;
+ controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
+ sizeof(*controls), GFP_KERNEL);
+ if (!controls)
+ goto __err_ctrls;
+
gpr_map = (u32 __force *)icode->gpr_map;
icode->tram_data_map = icode->gpr_map + 512;
@@ -1741,12 +1746,12 @@ A_OP(icode, &ptr, iMAC0, A_GPR(var), A_GPR(var), A_GPR(vol), A_EXTIN(input))
emu->support_tlv = 0; /* clear again */
snd_leave_user(seg);
- __err:
+__err:
kfree(controls);
- if (icode != NULL) {
- kfree((void __force *)icode->gpr_map);
- kfree(icode);
- }
+__err_ctrls:
+ kfree((void __force *)icode->gpr_map);
+__err_gpr:
+ kfree(icode);
return err;
}
@@ -1813,18 +1818,26 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
u32 *gpr_map;
mm_segment_t seg;
- if ((icode = kzalloc(sizeof(*icode), GFP_KERNEL)) == NULL)
- return -ENOMEM;
- if ((icode->gpr_map = (u_int32_t __user *)
- kcalloc(256 + 160 + 160 + 2 * 512, sizeof(u_int32_t),
- GFP_KERNEL)) == NULL ||
- (controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
- sizeof(struct snd_emu10k1_fx8010_control_gpr),
- GFP_KERNEL)) == NULL ||
- (ipcm = kzalloc(sizeof(*ipcm), GFP_KERNEL)) == NULL) {
- err = -ENOMEM;
- goto __err;
- }
+ err = -ENOMEM;
+ icode = kzalloc(sizeof(*icode), GFP_KERNEL);
+ if (!icode)
+ return err;
+
+ icode->gpr_map = (__user) kcalloc(256 + 160 + 160 + 2 * 512,
+ sizeof(u_int32_t), GFP_KERNEL);
+ if (!icode->gpr_map)
+ goto __err_gpr;
+
+ controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
+ sizeof(struct snd_emu10k1_fx8010_control_gpr),
+ GFP_KERNEL);
+ if (!controls)
+ goto __err_ctrls;
+
+ ipcm = kzalloc(sizeof(*ipcm), GFP_KERNEL);
+ if (!ipcm)
+ goto __err_ipcm;
+
gpr_map = (u32 __force *)icode->gpr_map;
icode->tram_data_map = icode->gpr_map + 256;
@@ -2335,14 +2348,8 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
for (z = 0; z < 16; z++)
OP(icode, &ptr, iACC3, FXBUS2(z), C_00000000, C_00000000, EXTIN(z));
}
-
- if (gpr > tmp) {
- snd_BUG();
- err = -EIO;
- goto __err;
- }
- if (i > SND_EMU10K1_GPR_CONTROLS) {
+ if (gpr > tmp || i > SND_EMU10K1_GPR_CONTROLS) {
snd_BUG();
err = -EIO;
goto __err;
@@ -2363,13 +2370,14 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
snd_leave_user(seg);
if (err >= 0)
err = snd_emu10k1_ipcm_poke(emu, ipcm);
- __err:
+__err:
kfree(ipcm);
+__err_ipcm:
kfree(controls);
- if (icode != NULL) {
- kfree((void __force *)icode->gpr_map);
- kfree(icode);
- }
+__err_ctrls:
+ kfree((void __force *)icode->gpr_map);
+__err_gpr:
+ kfree(icode);
return err;
}
--
1.8.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] sound: pci: emu10k1: code refactoring and casting removal
2013-10-16 22:11 [PATCH] sound: pci: emu10k1: code refactoring and casting removal Geyslan G. Bem
@ 2013-10-17 6:15 ` Takashi Iwai
2013-10-17 11:13 ` Geyslan Gregório Bem
0 siblings, 1 reply; 7+ messages in thread
From: Takashi Iwai @ 2013-10-17 6:15 UTC (permalink / raw)
To: Geyslan G. Bem; +Cc: moderated list:SOUND, Bill Pemberton, open list, kernel-br
At Wed, 16 Oct 2013 19:11:21 -0300,
Geyslan G. Bem wrote:
>
> Partially restructures _snd_emu10k1_audigy_init_efx() and
> _snd_emu10k1_init_efx() functions.
>
> Removes useless casting (void *) from value returned by kcalloc;
> see Documentation/CodingStyle, Chap 14.
>
> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
> ---
> sound/pci/emu10k1/emufx.c | 84 ++++++++++++++++++++++++++---------------------
> 1 file changed, 46 insertions(+), 38 deletions(-)
>
> diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
> index 0275209..afd4691 100644
> --- a/sound/pci/emu10k1/emufx.c
> +++ b/sound/pci/emu10k1/emufx.c
> @@ -1182,15 +1182,20 @@ static int _snd_emu10k1_audigy_init_efx(struct snd_emu10k1 *emu)
> u32 *gpr_map;
> mm_segment_t seg;
>
> - if ((icode = kzalloc(sizeof(*icode), GFP_KERNEL)) == NULL ||
> - (icode->gpr_map = (u_int32_t __user *)
> - kcalloc(512 + 256 + 256 + 2 * 1024, sizeof(u_int32_t),
> - GFP_KERNEL)) == NULL ||
> - (controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
> - sizeof(*controls), GFP_KERNEL)) == NULL) {
> - err = -ENOMEM;
> - goto __err;
> - }
> + err = -ENOMEM;
> + icode = kzalloc(sizeof(*icode), GFP_KERNEL);
> + if (!icode)
> + return err;
> +
> + icode->gpr_map = (__user) kcalloc(512 + 256 + 256 + 2 * 1024,
> + sizeof(u_int32_t), GFP_KERNEL);
> + if (!icode->gpr_map)
> + goto __err_gpr;
> + controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
> + sizeof(*controls), GFP_KERNEL);
> + if (!controls)
> + goto __err_ctrls;
> +
> gpr_map = (u32 __force *)icode->gpr_map;
>
> icode->tram_data_map = icode->gpr_map + 512;
> @@ -1741,12 +1746,12 @@ A_OP(icode, &ptr, iMAC0, A_GPR(var), A_GPR(var), A_GPR(vol), A_EXTIN(input))
> emu->support_tlv = 0; /* clear again */
> snd_leave_user(seg);
>
> - __err:
> +__err:
> kfree(controls);
> - if (icode != NULL) {
> - kfree((void __force *)icode->gpr_map);
> - kfree(icode);
> - }
> +__err_ctrls:
> + kfree((void __force *)icode->gpr_map);
> +__err_gpr:
> + kfree(icode);
> return err;
> }
>
> @@ -1813,18 +1818,26 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
> u32 *gpr_map;
> mm_segment_t seg;
>
> - if ((icode = kzalloc(sizeof(*icode), GFP_KERNEL)) == NULL)
> - return -ENOMEM;
> - if ((icode->gpr_map = (u_int32_t __user *)
> - kcalloc(256 + 160 + 160 + 2 * 512, sizeof(u_int32_t),
> - GFP_KERNEL)) == NULL ||
> - (controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
> - sizeof(struct snd_emu10k1_fx8010_control_gpr),
> - GFP_KERNEL)) == NULL ||
> - (ipcm = kzalloc(sizeof(*ipcm), GFP_KERNEL)) == NULL) {
> - err = -ENOMEM;
> - goto __err;
> - }
> + err = -ENOMEM;
> + icode = kzalloc(sizeof(*icode), GFP_KERNEL);
> + if (!icode)
> + return err;
> +
> + icode->gpr_map = (__user) kcalloc(256 + 160 + 160 + 2 * 512,
> + sizeof(u_int32_t), GFP_KERNEL);
> + if (!icode->gpr_map)
> + goto __err_gpr;
> +
> + controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
> + sizeof(struct snd_emu10k1_fx8010_control_gpr),
> + GFP_KERNEL);
> + if (!controls)
> + goto __err_ctrls;
> +
> + ipcm = kzalloc(sizeof(*ipcm), GFP_KERNEL);
> + if (!ipcm)
> + goto __err_ipcm;
> +
> gpr_map = (u32 __force *)icode->gpr_map;
>
> icode->tram_data_map = icode->gpr_map + 256;
> @@ -2335,14 +2348,8 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
> for (z = 0; z < 16; z++)
> OP(icode, &ptr, iACC3, FXBUS2(z), C_00000000, C_00000000, EXTIN(z));
> }
> -
>
> - if (gpr > tmp) {
> - snd_BUG();
> - err = -EIO;
> - goto __err;
> - }
> - if (i > SND_EMU10K1_GPR_CONTROLS) {
> + if (gpr > tmp || i > SND_EMU10K1_GPR_CONTROLS) {
> snd_BUG();
In that way, you can't distinguish which condition triggered the bug
(it could be shown in WARN() called in snd_BUG() in the original
version), so this is a functional change. Avoid it in a clean up
patch.
thanks,
Takashi
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sound: pci: emu10k1: code refactoring and casting removal
2013-10-17 6:15 ` Takashi Iwai
@ 2013-10-17 11:13 ` Geyslan Gregório Bem
2013-10-17 12:36 ` Takashi Iwai
0 siblings, 1 reply; 7+ messages in thread
From: Geyslan Gregório Bem @ 2013-10-17 11:13 UTC (permalink / raw)
To: Takashi Iwai
Cc: kernel-br, Jaroslav Kysela, Bill Pemberton, moderated list:SOUND,
open list
2013/10/17 Takashi Iwai <tiwai@suse.de>:
> At Wed, 16 Oct 2013 19:11:21 -0300,
> Geyslan G. Bem wrote:
>>
>> Partially restructures _snd_emu10k1_audigy_init_efx() and
>> _snd_emu10k1_init_efx() functions.
>>
>> Removes useless casting (void *) from value returned by kcalloc;
>> see Documentation/CodingStyle, Chap 14.
>>
>> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
>> ---
>> sound/pci/emu10k1/emufx.c | 84 ++++++++++++++++++++++++++---------------------
>> 1 file changed, 46 insertions(+), 38 deletions(-)
>>
>> diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
>> index 0275209..afd4691 100644
>> --- a/sound/pci/emu10k1/emufx.c
>> +++ b/sound/pci/emu10k1/emufx.c
>> @@ -1182,15 +1182,20 @@ static int _snd_emu10k1_audigy_init_efx(struct snd_emu10k1 *emu)
>> u32 *gpr_map;
>> mm_segment_t seg;
>>
>> - if ((icode = kzalloc(sizeof(*icode), GFP_KERNEL)) == NULL ||
>> - (icode->gpr_map = (u_int32_t __user *)
>> - kcalloc(512 + 256 + 256 + 2 * 1024, sizeof(u_int32_t),
>> - GFP_KERNEL)) == NULL ||
>> - (controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
>> - sizeof(*controls), GFP_KERNEL)) == NULL) {
>> - err = -ENOMEM;
>> - goto __err;
>> - }
>> + err = -ENOMEM;
>> + icode = kzalloc(sizeof(*icode), GFP_KERNEL);
>> + if (!icode)
>> + return err;
>> +
>> + icode->gpr_map = (__user) kcalloc(512 + 256 + 256 + 2 * 1024,
>> + sizeof(u_int32_t), GFP_KERNEL);
>> + if (!icode->gpr_map)
>> + goto __err_gpr;
>> + controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
>> + sizeof(*controls), GFP_KERNEL);
>> + if (!controls)
>> + goto __err_ctrls;
>> +
>> gpr_map = (u32 __force *)icode->gpr_map;
>>
>> icode->tram_data_map = icode->gpr_map + 512;
>> @@ -1741,12 +1746,12 @@ A_OP(icode, &ptr, iMAC0, A_GPR(var), A_GPR(var), A_GPR(vol), A_EXTIN(input))
>> emu->support_tlv = 0; /* clear again */
>> snd_leave_user(seg);
>>
>> - __err:
>> +__err:
>> kfree(controls);
>> - if (icode != NULL) {
>> - kfree((void __force *)icode->gpr_map);
>> - kfree(icode);
>> - }
>> +__err_ctrls:
>> + kfree((void __force *)icode->gpr_map);
>> +__err_gpr:
>> + kfree(icode);
>> return err;
>> }
>>
>> @@ -1813,18 +1818,26 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
>> u32 *gpr_map;
>> mm_segment_t seg;
>>
>> - if ((icode = kzalloc(sizeof(*icode), GFP_KERNEL)) == NULL)
>> - return -ENOMEM;
>> - if ((icode->gpr_map = (u_int32_t __user *)
>> - kcalloc(256 + 160 + 160 + 2 * 512, sizeof(u_int32_t),
>> - GFP_KERNEL)) == NULL ||
>> - (controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
>> - sizeof(struct snd_emu10k1_fx8010_control_gpr),
>> - GFP_KERNEL)) == NULL ||
>> - (ipcm = kzalloc(sizeof(*ipcm), GFP_KERNEL)) == NULL) {
>> - err = -ENOMEM;
>> - goto __err;
>> - }
>> + err = -ENOMEM;
>> + icode = kzalloc(sizeof(*icode), GFP_KERNEL);
>> + if (!icode)
>> + return err;
>> +
>> + icode->gpr_map = (__user) kcalloc(256 + 160 + 160 + 2 * 512,
>> + sizeof(u_int32_t), GFP_KERNEL);
>> + if (!icode->gpr_map)
>> + goto __err_gpr;
>> +
>> + controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
>> + sizeof(struct snd_emu10k1_fx8010_control_gpr),
>> + GFP_KERNEL);
>> + if (!controls)
>> + goto __err_ctrls;
>> +
>> + ipcm = kzalloc(sizeof(*ipcm), GFP_KERNEL);
>> + if (!ipcm)
>> + goto __err_ipcm;
>> +
>> gpr_map = (u32 __force *)icode->gpr_map;
>>
>> icode->tram_data_map = icode->gpr_map + 256;
>> @@ -2335,14 +2348,8 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
>> for (z = 0; z < 16; z++)
>> OP(icode, &ptr, iACC3, FXBUS2(z), C_00000000, C_00000000, EXTIN(z));
>> }
>> -
>>
>> - if (gpr > tmp) {
>> - snd_BUG();
>> - err = -EIO;
>> - goto __err;
>> - }
>> - if (i > SND_EMU10K1_GPR_CONTROLS) {
>> + if (gpr > tmp || i > SND_EMU10K1_GPR_CONTROLS) {
>> snd_BUG();
>
> In that way, you can't distinguish which condition triggered the bug
> (it could be shown in WARN() called in snd_BUG() in the original
> version), so this is a functional change. Avoid it in a clean up
> patch.
>
>
> thanks,
>
> Takashi
Takashi, I actually thought that that change was a cleanup. :) But
ok, now I see the reason (tracing)
What about using this instead snd_BUG?
snd_printd(KERN_WARN "BUG?\ngpr: %d, i: %d", gpr, i);
It prints file, line and we can put the data. I really want to reduce
repeated code.
Regards,
Geyslan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sound: pci: emu10k1: code refactoring and casting removal
2013-10-17 11:13 ` Geyslan Gregório Bem
@ 2013-10-17 12:36 ` Takashi Iwai
2013-10-17 12:55 ` Geyslan Gregório Bem
0 siblings, 1 reply; 7+ messages in thread
From: Takashi Iwai @ 2013-10-17 12:36 UTC (permalink / raw)
To: Geyslan Gregório Bem
Cc: kernel-br, Jaroslav Kysela, Bill Pemberton, moderated list:SOUND,
open list
At Thu, 17 Oct 2013 08:13:32 -0300,
Geyslan Gregório Bem wrote:
>
> 2013/10/17 Takashi Iwai <tiwai@suse.de>:
> > At Wed, 16 Oct 2013 19:11:21 -0300,
> > Geyslan G. Bem wrote:
> >>
> >> Partially restructures _snd_emu10k1_audigy_init_efx() and
> >> _snd_emu10k1_init_efx() functions.
> >>
> >> Removes useless casting (void *) from value returned by kcalloc;
> >> see Documentation/CodingStyle, Chap 14.
> >>
> >> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
> >> ---
> >> sound/pci/emu10k1/emufx.c | 84 ++++++++++++++++++++++++++---------------------
> >> 1 file changed, 46 insertions(+), 38 deletions(-)
> >>
> >> diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
> >> index 0275209..afd4691 100644
> >> --- a/sound/pci/emu10k1/emufx.c
> >> +++ b/sound/pci/emu10k1/emufx.c
> >> @@ -1182,15 +1182,20 @@ static int _snd_emu10k1_audigy_init_efx(struct snd_emu10k1 *emu)
> >> u32 *gpr_map;
> >> mm_segment_t seg;
> >>
> >> - if ((icode = kzalloc(sizeof(*icode), GFP_KERNEL)) == NULL ||
> >> - (icode->gpr_map = (u_int32_t __user *)
> >> - kcalloc(512 + 256 + 256 + 2 * 1024, sizeof(u_int32_t),
> >> - GFP_KERNEL)) == NULL ||
> >> - (controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
> >> - sizeof(*controls), GFP_KERNEL)) == NULL) {
> >> - err = -ENOMEM;
> >> - goto __err;
> >> - }
> >> + err = -ENOMEM;
> >> + icode = kzalloc(sizeof(*icode), GFP_KERNEL);
> >> + if (!icode)
> >> + return err;
> >> +
> >> + icode->gpr_map = (__user) kcalloc(512 + 256 + 256 + 2 * 1024,
> >> + sizeof(u_int32_t), GFP_KERNEL);
> >> + if (!icode->gpr_map)
> >> + goto __err_gpr;
> >> + controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
> >> + sizeof(*controls), GFP_KERNEL);
> >> + if (!controls)
> >> + goto __err_ctrls;
> >> +
> >> gpr_map = (u32 __force *)icode->gpr_map;
> >>
> >> icode->tram_data_map = icode->gpr_map + 512;
> >> @@ -1741,12 +1746,12 @@ A_OP(icode, &ptr, iMAC0, A_GPR(var), A_GPR(var), A_GPR(vol), A_EXTIN(input))
> >> emu->support_tlv = 0; /* clear again */
> >> snd_leave_user(seg);
> >>
> >> - __err:
> >> +__err:
> >> kfree(controls);
> >> - if (icode != NULL) {
> >> - kfree((void __force *)icode->gpr_map);
> >> - kfree(icode);
> >> - }
> >> +__err_ctrls:
> >> + kfree((void __force *)icode->gpr_map);
> >> +__err_gpr:
> >> + kfree(icode);
> >> return err;
> >> }
> >>
> >> @@ -1813,18 +1818,26 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
> >> u32 *gpr_map;
> >> mm_segment_t seg;
> >>
> >> - if ((icode = kzalloc(sizeof(*icode), GFP_KERNEL)) == NULL)
> >> - return -ENOMEM;
> >> - if ((icode->gpr_map = (u_int32_t __user *)
> >> - kcalloc(256 + 160 + 160 + 2 * 512, sizeof(u_int32_t),
> >> - GFP_KERNEL)) == NULL ||
> >> - (controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
> >> - sizeof(struct snd_emu10k1_fx8010_control_gpr),
> >> - GFP_KERNEL)) == NULL ||
> >> - (ipcm = kzalloc(sizeof(*ipcm), GFP_KERNEL)) == NULL) {
> >> - err = -ENOMEM;
> >> - goto __err;
> >> - }
> >> + err = -ENOMEM;
> >> + icode = kzalloc(sizeof(*icode), GFP_KERNEL);
> >> + if (!icode)
> >> + return err;
> >> +
> >> + icode->gpr_map = (__user) kcalloc(256 + 160 + 160 + 2 * 512,
> >> + sizeof(u_int32_t), GFP_KERNEL);
> >> + if (!icode->gpr_map)
> >> + goto __err_gpr;
> >> +
> >> + controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
> >> + sizeof(struct snd_emu10k1_fx8010_control_gpr),
> >> + GFP_KERNEL);
> >> + if (!controls)
> >> + goto __err_ctrls;
> >> +
> >> + ipcm = kzalloc(sizeof(*ipcm), GFP_KERNEL);
> >> + if (!ipcm)
> >> + goto __err_ipcm;
> >> +
> >> gpr_map = (u32 __force *)icode->gpr_map;
> >>
> >> icode->tram_data_map = icode->gpr_map + 256;
> >> @@ -2335,14 +2348,8 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
> >> for (z = 0; z < 16; z++)
> >> OP(icode, &ptr, iACC3, FXBUS2(z), C_00000000, C_00000000, EXTIN(z));
> >> }
> >> -
> >>
> >> - if (gpr > tmp) {
> >> - snd_BUG();
> >> - err = -EIO;
> >> - goto __err;
> >> - }
> >> - if (i > SND_EMU10K1_GPR_CONTROLS) {
> >> + if (gpr > tmp || i > SND_EMU10K1_GPR_CONTROLS) {
> >> snd_BUG();
> >
> > In that way, you can't distinguish which condition triggered the bug
> > (it could be shown in WARN() called in snd_BUG() in the original
> > version), so this is a functional change. Avoid it in a clean up
> > patch.
> >
> >
> > thanks,
> >
> > Takashi
>
> Takashi, I actually thought that that change was a cleanup. :) But
> ok, now I see the reason (tracing)
> What about using this instead snd_BUG?
>
> snd_printd(KERN_WARN "BUG?\ngpr: %d, i: %d", gpr, i);
>
> It prints file, line and we can put the data. I really want to reduce
> repeated code.
WARN() gives more precise information.
Geyslan, you don't have to waste too much of your time (and my time
for review) for this kind of so old driver code unless it really fixes
the bugs. A clean up is good in general, but it can be sometimes
worse than nothing since it also breaks the history, thus makes hard
to follow via "git blame", for example, and of course, there is always
a potential danger of regression.
So, if it's about clean up, do it only in a systematic way that others
can follow easily, and don't do it too intensively.
thanks,
Takashi
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sound: pci: emu10k1: code refactoring and casting removal
2013-10-17 12:36 ` Takashi Iwai
@ 2013-10-17 12:55 ` Geyslan Gregório Bem
2013-10-17 18:35 ` [Kernel-BR] " Raphael S Carvalho
0 siblings, 1 reply; 7+ messages in thread
From: Geyslan Gregório Bem @ 2013-10-17 12:55 UTC (permalink / raw)
To: Takashi Iwai
Cc: kernel-br, Jaroslav Kysela, Bill Pemberton, moderated list:SOUND,
open list
2013/10/17 Takashi Iwai <tiwai@suse.de>:
> At Thu, 17 Oct 2013 08:13:32 -0300,
> Geyslan Gregório Bem wrote:
>>
>> 2013/10/17 Takashi Iwai <tiwai@suse.de>:
>> > At Wed, 16 Oct 2013 19:11:21 -0300,
>> > Geyslan G. Bem wrote:
>> >>
>> >> Partially restructures _snd_emu10k1_audigy_init_efx() and
>> >> _snd_emu10k1_init_efx() functions.
>> >>
>> >> Removes useless casting (void *) from value returned by kcalloc;
>> >> see Documentation/CodingStyle, Chap 14.
>> >>
>> >> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
>> >> ---
>> >> sound/pci/emu10k1/emufx.c | 84 ++++++++++++++++++++++++++---------------------
>> >> 1 file changed, 46 insertions(+), 38 deletions(-)
>> >>
>> >> diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
>> >> index 0275209..afd4691 100644
>> >> --- a/sound/pci/emu10k1/emufx.c
>> >> +++ b/sound/pci/emu10k1/emufx.c
>> >> @@ -1182,15 +1182,20 @@ static int _snd_emu10k1_audigy_init_efx(struct snd_emu10k1 *emu)
>> >> u32 *gpr_map;
>> >> mm_segment_t seg;
>> >>
>> >> - if ((icode = kzalloc(sizeof(*icode), GFP_KERNEL)) == NULL ||
>> >> - (icode->gpr_map = (u_int32_t __user *)
>> >> - kcalloc(512 + 256 + 256 + 2 * 1024, sizeof(u_int32_t),
>> >> - GFP_KERNEL)) == NULL ||
>> >> - (controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
>> >> - sizeof(*controls), GFP_KERNEL)) == NULL) {
>> >> - err = -ENOMEM;
>> >> - goto __err;
>> >> - }
>> >> + err = -ENOMEM;
>> >> + icode = kzalloc(sizeof(*icode), GFP_KERNEL);
>> >> + if (!icode)
>> >> + return err;
>> >> +
>> >> + icode->gpr_map = (__user) kcalloc(512 + 256 + 256 + 2 * 1024,
>> >> + sizeof(u_int32_t), GFP_KERNEL);
>> >> + if (!icode->gpr_map)
>> >> + goto __err_gpr;
>> >> + controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
>> >> + sizeof(*controls), GFP_KERNEL);
>> >> + if (!controls)
>> >> + goto __err_ctrls;
>> >> +
>> >> gpr_map = (u32 __force *)icode->gpr_map;
>> >>
>> >> icode->tram_data_map = icode->gpr_map + 512;
>> >> @@ -1741,12 +1746,12 @@ A_OP(icode, &ptr, iMAC0, A_GPR(var), A_GPR(var), A_GPR(vol), A_EXTIN(input))
>> >> emu->support_tlv = 0; /* clear again */
>> >> snd_leave_user(seg);
>> >>
>> >> - __err:
>> >> +__err:
>> >> kfree(controls);
>> >> - if (icode != NULL) {
>> >> - kfree((void __force *)icode->gpr_map);
>> >> - kfree(icode);
>> >> - }
>> >> +__err_ctrls:
>> >> + kfree((void __force *)icode->gpr_map);
>> >> +__err_gpr:
>> >> + kfree(icode);
>> >> return err;
>> >> }
>> >>
>> >> @@ -1813,18 +1818,26 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
>> >> u32 *gpr_map;
>> >> mm_segment_t seg;
>> >>
>> >> - if ((icode = kzalloc(sizeof(*icode), GFP_KERNEL)) == NULL)
>> >> - return -ENOMEM;
>> >> - if ((icode->gpr_map = (u_int32_t __user *)
>> >> - kcalloc(256 + 160 + 160 + 2 * 512, sizeof(u_int32_t),
>> >> - GFP_KERNEL)) == NULL ||
>> >> - (controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
>> >> - sizeof(struct snd_emu10k1_fx8010_control_gpr),
>> >> - GFP_KERNEL)) == NULL ||
>> >> - (ipcm = kzalloc(sizeof(*ipcm), GFP_KERNEL)) == NULL) {
>> >> - err = -ENOMEM;
>> >> - goto __err;
>> >> - }
>> >> + err = -ENOMEM;
>> >> + icode = kzalloc(sizeof(*icode), GFP_KERNEL);
>> >> + if (!icode)
>> >> + return err;
>> >> +
>> >> + icode->gpr_map = (__user) kcalloc(256 + 160 + 160 + 2 * 512,
>> >> + sizeof(u_int32_t), GFP_KERNEL);
>> >> + if (!icode->gpr_map)
>> >> + goto __err_gpr;
>> >> +
>> >> + controls = kcalloc(SND_EMU10K1_GPR_CONTROLS,
>> >> + sizeof(struct snd_emu10k1_fx8010_control_gpr),
>> >> + GFP_KERNEL);
>> >> + if (!controls)
>> >> + goto __err_ctrls;
>> >> +
>> >> + ipcm = kzalloc(sizeof(*ipcm), GFP_KERNEL);
>> >> + if (!ipcm)
>> >> + goto __err_ipcm;
>> >> +
>> >> gpr_map = (u32 __force *)icode->gpr_map;
>> >>
>> >> icode->tram_data_map = icode->gpr_map + 256;
>> >> @@ -2335,14 +2348,8 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu)
>> >> for (z = 0; z < 16; z++)
>> >> OP(icode, &ptr, iACC3, FXBUS2(z), C_00000000, C_00000000, EXTIN(z));
>> >> }
>> >> -
>> >>
>> >> - if (gpr > tmp) {
>> >> - snd_BUG();
>> >> - err = -EIO;
>> >> - goto __err;
>> >> - }
>> >> - if (i > SND_EMU10K1_GPR_CONTROLS) {
>> >> + if (gpr > tmp || i > SND_EMU10K1_GPR_CONTROLS) {
>> >> snd_BUG();
>> >
>> > In that way, you can't distinguish which condition triggered the bug
>> > (it could be shown in WARN() called in snd_BUG() in the original
>> > version), so this is a functional change. Avoid it in a clean up
>> > patch.
>> >
>> >
>> > thanks,
>> >
>> > Takashi
>>
>> Takashi, I actually thought that that change was a cleanup. :) But
>> ok, now I see the reason (tracing)
>> What about using this instead snd_BUG?
>>
>> snd_printd(KERN_WARN "BUG?\ngpr: %d, i: %d", gpr, i);
>>
>> It prints file, line and we can put the data. I really want to reduce
>> repeated code.
>
> WARN() gives more precise information.
>
> Geyslan, you don't have to waste too much of your time (and my time
> for review) for this kind of so old driver code unless it really fixes
> the bugs. A clean up is good in general, but it can be sometimes
> worse than nothing since it also breaks the history, thus makes hard
> to follow via "git blame", for example, and of course, there is always
> a potential danger of regression.
>
> So, if it's about clean up, do it only in a systematic way that others
> can follow easily, and don't do it too intensively.
>
>
> thanks,
>
> Takashi
Takashi, ok, I'll undo that patch part and send the new version later.
Thank you again for reply.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Kernel-BR] Re: [PATCH] sound: pci: emu10k1: code refactoring and casting removal
2013-10-17 12:55 ` Geyslan Gregório Bem
@ 2013-10-17 18:35 ` Raphael S Carvalho
2013-10-17 23:04 ` Geyslan Gregório Bem
0 siblings, 1 reply; 7+ messages in thread
From: Raphael S Carvalho @ 2013-10-17 18:35 UTC (permalink / raw)
To: kernel-br, Geyslan Gregório Bem
Cc: Takashi Iwai, Jaroslav Kysela, Bill Pemberton,
moderated list:SOUND, open list
> 2013/10/17 Takashi Iwai <tiwai@suse.de>:
>>
>> Geyslan, you don't have to waste too much of your time (and my time
>> for review) for this kind of so old driver code unless it really fixes
>> the bugs. A clean up is good in general, but it can be sometimes
>> worse than nothing since it also breaks the history, thus makes hard
>> to follow via "git blame", for example, and of course, there is always
>> a potential danger of regression.
>>
>> So, if it's about clean up, do it only in a systematic way that others
>> can follow easily, and don't do it too intensively.
>>
>>
>> thanks,
>>
>> Takashi
His code refactoring proposal is welcome since the old code sucks from
several points of view, e.g. maintainability, legibility, etc.
Yes, of course, it's important to be careful in order not to introduce
regressions. And yes, he could improve something here and there.
As long as changes improve the maintainability and legibility of the
code, there is no reason to refuse them.
--
Raphael S. Carvalho
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Kernel-BR] Re: [PATCH] sound: pci: emu10k1: code refactoring and casting removal
2013-10-17 18:35 ` [Kernel-BR] " Raphael S Carvalho
@ 2013-10-17 23:04 ` Geyslan Gregório Bem
0 siblings, 0 replies; 7+ messages in thread
From: Geyslan Gregório Bem @ 2013-10-17 23:04 UTC (permalink / raw)
To: Raphael S Carvalho
Cc: kernel-br, Takashi Iwai, Jaroslav Kysela, Bill Pemberton,
moderated list:SOUND, open list
2013/10/17 Raphael S Carvalho <raphael.scarv@gmail.com>:
>> 2013/10/17 Takashi Iwai <tiwai@suse.de>:
>>>
>>> Geyslan, you don't have to waste too much of your time (and my time
>>> for review) for this kind of so old driver code unless it really fixes
>>> the bugs. A clean up is good in general, but it can be sometimes
>>> worse than nothing since it also breaks the history, thus makes hard
>>> to follow via "git blame", for example, and of course, there is always
>>> a potential danger of regression.
>>>
>>> So, if it's about clean up, do it only in a systematic way that others
>>> can follow easily, and don't do it too intensively.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
Takashi,
I sent the last version:
[PATCH v3] sound: pci: emu10k1: code refactoring
Regards.
>
> His code refactoring proposal is welcome since the old code sucks from
> several points of view, e.g. maintainability, legibility, etc.
> Yes, of course, it's important to be careful in order not to introduce
> regressions. And yes, he could improve something here and there.
> As long as changes improve the maintainability and legibility of the
> code, there is no reason to refuse them.
>
> --
> Raphael S. Carvalho
Raphael, thank you for reply.
Regards.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-10-17 23:04 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-16 22:11 [PATCH] sound: pci: emu10k1: code refactoring and casting removal Geyslan G. Bem
2013-10-17 6:15 ` Takashi Iwai
2013-10-17 11:13 ` Geyslan Gregório Bem
2013-10-17 12:36 ` Takashi Iwai
2013-10-17 12:55 ` Geyslan Gregório Bem
2013-10-17 18:35 ` [Kernel-BR] " Raphael S Carvalho
2013-10-17 23:04 ` Geyslan Gregório Bem
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).