| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGG-pUQAOZ852CamCE+T4zM2Wwc0xKr+WRbGQLEqK=EvC3JCvw@mail.gmail.com>
Date: Thu, 17 Oct 2013 09:55:11 -0300
From: Geyslan Gregório Bem <geyslan@...il.com>
To: Takashi Iwai <tiwai@...e.de>
Cc: kernel-br <kernel-br@...glegroups.com>,
Jaroslav Kysela <perex@...ex.cz>,
Bill Pemberton <wfp5p@...ginia.edu>,
"moderated list:SOUND" <alsa-devel@...a-project.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sound: pci: emu10k1: code refactoring and casting removal
2013/10/17 Takashi Iwai <tiwai@...e.de>:
> At Thu, 17 Oct 2013 08:13:32 -0300,
> Geyslan Gregório Bem wrote:
>>
>> 2013/10/17 Takashi Iwai <tiwai@...e.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@...il.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists