* [patch] ALSA: hda ca0132 - remove unused error handling code @ 2012-02-07 7:19 Dan Carpenter 2012-02-07 9:59 ` Takashi Iwai 0 siblings, 1 reply; 3+ messages in thread From: Dan Carpenter @ 2012-02-07 7:19 UTC (permalink / raw) To: Jaroslav Kysela Cc: Takashi Iwai, Paul Gortmaker, alsa-devel, kernel-janitors, Ian Minett We never save the return value from chipio_write() so "err" is always zero here, not less than zero. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- The other option would be to save the return value. I don't have the hardware and I worried that there was a reason we don't save it. The current code is misleading because it looks like it's checked until you notice that the "err = " bit is missing. diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c index 35abe3c..92f1f0c 100644 --- a/sound/pci/hda/patch_ca0132.c +++ b/sound/pci/hda/patch_ca0132.c @@ -733,8 +733,6 @@ static int ca0132_hp_switch_put(struct snd_kcontrol *kcontrol, /* *valp 0 is mute, 1 is unmute */ data = (data & 0x7f) | (*valp ? 0 : 0x80); chipio_write(codec, REG_CODEC_MUTE, data); - if (err < 0) - return err; spec->curr_hp_switch = *valp; @@ -775,8 +773,6 @@ static int ca0132_speaker_switch_put(struct snd_kcontrol *kcontrol, /* *valp 0 is mute, 1 is unmute */ data = (data & 0xef) | (*valp ? 0 : 0x10); chipio_write(codec, REG_CODEC_MUTE, data); - if (err < 0) - return err; spec->curr_speaker_switch = *valp; @@ -824,14 +820,10 @@ static int ca0132_hp_volume_put(struct snd_kcontrol *kcontrol, val = 31 - left_vol; data = (data & 0xe0) | val; chipio_write(codec, REG_CODEC_HP_VOL_L, data); - if (err < 0) - return err; val = 31 - right_vol; data = (data & 0xe0) | val; chipio_write(codec, REG_CODEC_HP_VOL_R, data); - if (err < 0) - return err; spec->curr_hp_volume[0] = left_vol; spec->curr_hp_volume[1] = right_vol; ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [patch] ALSA: hda ca0132 - remove unused error handling code 2012-02-07 7:19 [patch] ALSA: hda ca0132 - remove unused error handling code Dan Carpenter @ 2012-02-07 9:59 ` Takashi Iwai 2012-02-07 10:15 ` Takashi Iwai 0 siblings, 1 reply; 3+ messages in thread From: Takashi Iwai @ 2012-02-07 9:59 UTC (permalink / raw) To: Dan Carpenter; +Cc: Paul Gortmaker, alsa-devel, Ian Minett, kernel-janitors At Tue, 7 Feb 2012 10:19:32 +0300, Dan Carpenter wrote: > > We never save the return value from chipio_write() so "err" is always > zero here, not less than zero. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > The other option would be to save the return value. Yes, this should be a better option. chipio_write() may fail indeed. But then your report spotted another bug. It doesn't handle snd_hda_power_up|down() correctly in the error path. It must be fixed as well. I'll fix these all up. > I don't have the > hardware and I worried that there was a reason we don't save it. The > current code is misleading because it looks like it's checked until > you notice that the "err = " bit is missing. thanks, Takashi > > diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c > index 35abe3c..92f1f0c 100644 > --- a/sound/pci/hda/patch_ca0132.c > +++ b/sound/pci/hda/patch_ca0132.c > @@ -733,8 +733,6 @@ static int ca0132_hp_switch_put(struct snd_kcontrol *kcontrol, > /* *valp 0 is mute, 1 is unmute */ > data = (data & 0x7f) | (*valp ? 0 : 0x80); > chipio_write(codec, REG_CODEC_MUTE, data); > - if (err < 0) > - return err; > > spec->curr_hp_switch = *valp; > > @@ -775,8 +773,6 @@ static int ca0132_speaker_switch_put(struct snd_kcontrol *kcontrol, > /* *valp 0 is mute, 1 is unmute */ > data = (data & 0xef) | (*valp ? 0 : 0x10); > chipio_write(codec, REG_CODEC_MUTE, data); > - if (err < 0) > - return err; > > spec->curr_speaker_switch = *valp; > > @@ -824,14 +820,10 @@ static int ca0132_hp_volume_put(struct snd_kcontrol *kcontrol, > val = 31 - left_vol; > data = (data & 0xe0) | val; > chipio_write(codec, REG_CODEC_HP_VOL_L, data); > - if (err < 0) > - return err; > > val = 31 - right_vol; > data = (data & 0xe0) | val; > chipio_write(codec, REG_CODEC_HP_VOL_R, data); > - if (err < 0) > - return err; > > spec->curr_hp_volume[0] = left_vol; > spec->curr_hp_volume[1] = right_vol; > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [patch] ALSA: hda ca0132 - remove unused error handling code 2012-02-07 9:59 ` Takashi Iwai @ 2012-02-07 10:15 ` Takashi Iwai 0 siblings, 0 replies; 3+ messages in thread From: Takashi Iwai @ 2012-02-07 10:15 UTC (permalink / raw) To: Dan Carpenter; +Cc: Paul Gortmaker, alsa-devel, Ian Minett, kernel-janitors At Tue, 07 Feb 2012 10:59:48 +0100, Takashi Iwai wrote: > > At Tue, 7 Feb 2012 10:19:32 +0300, > Dan Carpenter wrote: > > > > We never save the return value from chipio_write() so "err" is always > > zero here, not less than zero. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > The other option would be to save the return value. > > Yes, this should be a better option. chipio_write() may fail indeed. > > But then your report spotted another bug. It doesn't handle > snd_hda_power_up|down() correctly in the error path. It must be fixed > as well. > > I'll fix these all up. FWIW, below is the fix patch. Takashi --- From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH] ALSA: hda - Fix error handling in patch_ca0132.c In patch_ca0132.c, the error returned from chipio_write() isn't checked always. Also, the power-up/down sequence isn't tracked properly in some error paths. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/pci/hda/patch_ca0132.c | 33 +++++++++++++++++++-------------- 1 files changed, 19 insertions(+), 14 deletions(-) diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c index 35abe3c..21d91d5 100644 --- a/sound/pci/hda/patch_ca0132.c +++ b/sound/pci/hda/patch_ca0132.c @@ -728,18 +728,19 @@ static int ca0132_hp_switch_put(struct snd_kcontrol *kcontrol, err = chipio_read(codec, REG_CODEC_MUTE, &data); if (err < 0) - return err; + goto exit; /* *valp 0 is mute, 1 is unmute */ data = (data & 0x7f) | (*valp ? 0 : 0x80); - chipio_write(codec, REG_CODEC_MUTE, data); + err = chipio_write(codec, REG_CODEC_MUTE, data); if (err < 0) - return err; + goto exit; spec->curr_hp_switch = *valp; + exit: snd_hda_power_down(codec); - return 1; + return err < 0 ? err : 1; } static int ca0132_speaker_switch_get(struct snd_kcontrol *kcontrol, @@ -770,18 +771,19 @@ static int ca0132_speaker_switch_put(struct snd_kcontrol *kcontrol, err = chipio_read(codec, REG_CODEC_MUTE, &data); if (err < 0) - return err; + goto exit; /* *valp 0 is mute, 1 is unmute */ data = (data & 0xef) | (*valp ? 0 : 0x10); - chipio_write(codec, REG_CODEC_MUTE, data); + err = chipio_write(codec, REG_CODEC_MUTE, data); if (err < 0) - return err; + goto exit; spec->curr_speaker_switch = *valp; + exit: snd_hda_power_down(codec); - return 1; + return err < 0 ? err : 1; } static int ca0132_hp_volume_get(struct snd_kcontrol *kcontrol, @@ -819,25 +821,26 @@ static int ca0132_hp_volume_put(struct snd_kcontrol *kcontrol, err = chipio_read(codec, REG_CODEC_HP_VOL_L, &data); if (err < 0) - return err; + goto exit; val = 31 - left_vol; data = (data & 0xe0) | val; - chipio_write(codec, REG_CODEC_HP_VOL_L, data); + err = chipio_write(codec, REG_CODEC_HP_VOL_L, data); if (err < 0) - return err; + goto exit; val = 31 - right_vol; data = (data & 0xe0) | val; - chipio_write(codec, REG_CODEC_HP_VOL_R, data); + err = chipio_write(codec, REG_CODEC_HP_VOL_R, data); if (err < 0) - return err; + goto exit; spec->curr_hp_volume[0] = left_vol; spec->curr_hp_volume[1] = right_vol; + exit: snd_hda_power_down(codec); - return 1; + return err < 0 ? err : 1; } static int add_hp_switch(struct hda_codec *codec, hda_nid_t nid) @@ -936,6 +939,8 @@ static int ca0132_build_controls(struct hda_codec *codec) if (err < 0) return err; err = add_in_volume(codec, spec->dig_in, "IEC958"); + if (err < 0) + return err; } return 0; } -- 1.7.9.rc2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-02-07 10:15 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-07 7:19 [patch] ALSA: hda ca0132 - remove unused error handling code Dan Carpenter 2012-02-07 9:59 ` Takashi Iwai 2012-02-07 10:15 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox