From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Sakamoto Subject: Re: [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path Date: Wed, 22 Feb 2017 22:37:22 +0900 Message-ID: <5433a393-16ce-d690-f8dc-d0741e759eb9@sakamocchi.jp> References: <20170220200921.824-1-o-takashi@sakamocchi.jp> <20170220200921.824-5-o-takashi@sakamocchi.jp> <0f1bb4cb-daa3-7dd9-aa55-990b67c34a38@sakamocchi.jp> <070a40e4-f90e-7c29-5058-f3a247fd9104@sakamocchi.jp> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-proxy004.phy.lolipop.jp (smtp-proxy004.phy.lolipop.jp [157.7.104.45]) by alsa0.perex.cz (Postfix) with ESMTP id 32486266BFF for ; Wed, 22 Feb 2017 14:37:25 +0100 (CET) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: onkel@paraair.de, alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org On Feb 22 2017 16:46, Takashi Iwai wrote: > -- 8< -- > From: Takashi Iwai > Subject: [PATCH] ALSA: usb-audio: Tidy up mixer_us16x08.c > > A few more cleanups and improvements that have been overlooked: > > - Use ARRAY_SIZE() macro appropriately > - Code shuffling for minor optimization > - Omit superfluous variable initializations > - Get rid of superfluous NULL checks > - Add const to snd_us16x08_control_params definitions > > No functional changes. > > Signed-off-by: Takashi Iwai > --- > sound/usb/mixer_us16x08.c | 132 ++++++++++++++++++---------------------------- > 1 file changed, 50 insertions(+), 82 deletions(-) Looks good to me, except for one item. Reviewed-by: Takashi Sakamoto > diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c > index f7289541fbce..dc48eedea92e 100644 > --- a/sound/usb/mixer_us16x08.c > +++ b/sound/usb/mixer_us16x08.c > @@ -176,15 +176,9 @@ static int snd_us16x08_recv_urb(struct snd_usb_audio *chip, > */ > static int snd_us16x08_send_urb(struct snd_usb_audio *chip, char *buf, int size) > { > - int err = 0; > - > - if (chip) { > - err = snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0), > + return snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0), > SND_US16X08_URB_REQUEST, SND_US16X08_URB_REQUESTTYPE, > 0, 0, buf, size); > - } > - > - return err; > } Inline function is better for this part because the definition includes a few statements. > > static int snd_us16x08_route_info(struct snd_kcontrol *kcontrol, > @@ -212,10 +206,7 @@ static int snd_us16x08_route_put(struct snd_kcontrol *kcontrol, > struct snd_usb_audio *chip = elem->head.mixer->chip; > int index = ucontrol->id.index; > char buf[sizeof(route_msg)]; > - int val, val_org, err = 0; > - > - /* prepare the message buffer from template */ > - memcpy(buf, route_msg, sizeof(route_msg)); > + int val, val_org, err; > > /* get the new value (no bias for routes) */ > val = ucontrol->value.enumerated.item[0]; > @@ -224,6 +215,9 @@ static int snd_us16x08_route_put(struct snd_kcontrol *kcontrol, > if (val < 0 || val > 9) > return -EINVAL; > > + /* prepare the message buffer from template */ > + memcpy(buf, route_msg, sizeof(route_msg)); > + > if (val < 2) { > /* input comes from a master channel */ > val_org = val; > @@ -279,12 +273,9 @@ static int snd_us16x08_master_put(struct snd_kcontrol *kcontrol, > struct usb_mixer_elem_info *elem = kcontrol->private_data; > struct snd_usb_audio *chip = elem->head.mixer->chip; > char buf[sizeof(mix_msg_out)]; > - int val, err = 0; > + int val, err; > int index = ucontrol->id.index; > > - /* prepare the message buffer from template */ > - memcpy(buf, mix_msg_out, sizeof(mix_msg_out)); > - > /* new control value incl. bias*/ > val = ucontrol->value.integer.value[0]; > > @@ -293,6 +284,9 @@ static int snd_us16x08_master_put(struct snd_kcontrol *kcontrol, > || val > SND_US16X08_KCMAX(kcontrol)) > return -EINVAL; > > + /* prepare the message buffer from template */ > + memcpy(buf, mix_msg_out, sizeof(mix_msg_out)); > + > buf[8] = val - SND_US16X08_KCBIAS(kcontrol); > buf[6] = elem->head.id; > > @@ -392,9 +386,6 @@ static int snd_us16x08_channel_put(struct snd_kcontrol *kcontrol, > int val, err; > int index = ucontrol->id.index; > > - /* prepare URB message from template */ > - memcpy(buf, mix_msg_in, sizeof(mix_msg_in)); > - > val = ucontrol->value.integer.value[0]; > > /* sanity check */ > @@ -402,6 +393,9 @@ static int snd_us16x08_channel_put(struct snd_kcontrol *kcontrol, > || val > SND_US16X08_KCMAX(kcontrol)) > return -EINVAL; > > + /* prepare URB message from template */ > + memcpy(buf, mix_msg_in, sizeof(mix_msg_in)); > + > /* add the bias to the new value */ > buf[8] = val - SND_US16X08_KCBIAS(kcontrol); > buf[6] = elem->head.id; > @@ -434,8 +428,7 @@ static int snd_us16x08_comp_get(struct snd_kcontrol *kcontrol, > struct snd_ctl_elem_value *ucontrol) > { > struct usb_mixer_elem_info *elem = kcontrol->private_data; > - struct snd_us16x08_comp_store *store = > - ((struct snd_us16x08_comp_store *) elem->private_data); > + struct snd_us16x08_comp_store *store = elem->private_data; > int index = ucontrol->id.index; > int val_idx = COMP_STORE_IDX(elem->head.id); > > @@ -449,18 +442,11 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol, > { > struct usb_mixer_elem_info *elem = kcontrol->private_data; > struct snd_usb_audio *chip = elem->head.mixer->chip; > - struct snd_us16x08_comp_store *store = > - ((struct snd_us16x08_comp_store *) elem->private_data); > + struct snd_us16x08_comp_store *store = elem->private_data; > int index = ucontrol->id.index; > char buf[sizeof(comp_msg)]; > int val_idx, val; > - int err = 0; > - > - /* prepare compressor URB message from template */ > - memcpy(buf, comp_msg, sizeof(comp_msg)); > - > - /* new control value incl. bias*/ > - val_idx = elem->head.id - SND_US16X08_ID_COMP_BASE; > + int err; > > val = ucontrol->value.integer.value[0]; > > @@ -469,8 +455,14 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol, > || val > SND_US16X08_KCMAX(kcontrol)) > return -EINVAL; > > + /* new control value incl. bias*/ > + val_idx = elem->head.id - SND_US16X08_ID_COMP_BASE; > + > store->val[val_idx][index] = ucontrol->value.integer.value[0]; > > + /* prepare compressor URB message from template */ > + memcpy(buf, comp_msg, sizeof(comp_msg)); > + > /* place comp values in message buffer watch bias! */ > buf[8] = store->val[ > COMP_STORE_IDX(SND_US16X08_ID_COMP_THRESHOLD)][index] > @@ -502,10 +494,9 @@ static int snd_us16x08_comp_put(struct snd_kcontrol *kcontrol, > static int snd_us16x08_eqswitch_get(struct snd_kcontrol *kcontrol, > struct snd_ctl_elem_value *ucontrol) > { > - int val = 0; > + int val; > struct usb_mixer_elem_info *elem = kcontrol->private_data; > - struct snd_us16x08_eq_store *store = > - ((struct snd_us16x08_eq_store *) elem->private_data); > + struct snd_us16x08_eq_store *store = elem->private_data; > int index = ucontrol->id.index; > > /* get low switch from cache is enough, cause all bands are together */ > @@ -521,10 +512,8 @@ static int snd_us16x08_eqswitch_put(struct snd_kcontrol *kcontrol, > { > struct usb_mixer_elem_info *elem = kcontrol->private_data; > struct snd_usb_audio *chip = elem->head.mixer->chip; > - struct snd_us16x08_eq_store *store = > - ((struct snd_us16x08_eq_store *) elem->private_data); > + struct snd_us16x08_eq_store *store = elem->private_data; > int index = ucontrol->id.index; > - > char buf[sizeof(eqs_msq)]; > int val, err = 0; > int b_idx; > @@ -564,10 +553,9 @@ static int snd_us16x08_eqswitch_put(struct snd_kcontrol *kcontrol, > static int snd_us16x08_eq_get(struct snd_kcontrol *kcontrol, > struct snd_ctl_elem_value *ucontrol) > { > - int val = 0; > + int val; > struct usb_mixer_elem_info *elem = kcontrol->private_data; > - struct snd_us16x08_eq_store *store = > - ((struct snd_us16x08_eq_store *) elem->private_data); > + struct snd_us16x08_eq_store *store = elem->private_data; > int index = ucontrol->id.index; > int b_idx = EQ_STORE_BAND_IDX(elem->head.id) - 1; > int p_idx = EQ_STORE_PARAM_IDX(elem->head.id); > @@ -584,17 +572,13 @@ static int snd_us16x08_eq_put(struct snd_kcontrol *kcontrol, > { > struct usb_mixer_elem_info *elem = kcontrol->private_data; > struct snd_usb_audio *chip = elem->head.mixer->chip; > - struct snd_us16x08_eq_store *store = > - ((struct snd_us16x08_eq_store *) elem->private_data); > + struct snd_us16x08_eq_store *store = elem->private_data; > int index = ucontrol->id.index; > char buf[sizeof(eqs_msq)]; > - int val, err = 0; > + int val, err; > int b_idx = EQ_STORE_BAND_IDX(elem->head.id) - 1; > int p_idx = EQ_STORE_PARAM_IDX(elem->head.id); > > - /* copy URB buffer from EQ template */ > - memcpy(buf, eqs_msq, sizeof(eqs_msq)); > - > val = ucontrol->value.integer.value[0]; > > /* sanity check */ > @@ -602,6 +586,9 @@ static int snd_us16x08_eq_put(struct snd_kcontrol *kcontrol, > || val > SND_US16X08_KCMAX(kcontrol)) > return -EINVAL; > > + /* copy URB buffer from EQ template */ > + memcpy(buf, eqs_msq, sizeof(eqs_msq)); > + > store->val[b_idx][p_idx][index] = val; > buf[20] = store->val[b_idx][3][index]; > buf[17] = store->val[b_idx][2][index]; > @@ -713,12 +700,6 @@ static int snd_us16x08_meter_get(struct snd_kcontrol *kcontrol, > u8 meter_urb[64]; > char tmp[sizeof(mix_init_msg2)] = {0}; > > - if (elem) { > - store = (struct snd_us16x08_meter_store *) elem->private_data; > - chip = elem->head.mixer->chip; > - } else > - return 0; > - > switch (kcontrol->private_value) { > case 0: > snd_us16x08_send_urb(chip, (char *)mix_init_msg1, > @@ -983,11 +964,11 @@ static struct snd_kcontrol_new snd_us16x08_meter_ctl = { > /* setup compressor store and assign default value */ > static struct snd_us16x08_comp_store *snd_us16x08_create_comp_store(void) > { > - int i = 0; > - struct snd_us16x08_comp_store *tmp = > - kmalloc(sizeof(struct snd_us16x08_comp_store), GFP_KERNEL); > + int i; > + struct snd_us16x08_comp_store *tmp; > > - if (tmp == NULL) > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL); > + if (!tmp) > return NULL; > > for (i = 0; i < SND_US16X08_MAX_CHANNELS; i++) { > @@ -1006,10 +987,10 @@ static struct snd_us16x08_comp_store *snd_us16x08_create_comp_store(void) > static struct snd_us16x08_eq_store *snd_us16x08_create_eq_store(void) > { > int i, b_idx; > - struct snd_us16x08_eq_store *tmp = > - kmalloc(sizeof(struct snd_us16x08_eq_store), GFP_KERNEL); > + struct snd_us16x08_eq_store *tmp; > > - if (tmp == NULL) > + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL); > + if (!tmp) > return NULL; > > for (i = 0; i < SND_US16X08_MAX_CHANNELS; i++) { > @@ -1042,15 +1023,14 @@ static struct snd_us16x08_eq_store *snd_us16x08_create_eq_store(void) > > static struct snd_us16x08_meter_store *snd_us16x08_create_meter_store(void) > { > - struct snd_us16x08_meter_store *tmp = > - kzalloc(sizeof(struct snd_us16x08_meter_store), GFP_KERNEL); > + struct snd_us16x08_meter_store *tmp; > > + tmp = kzalloc(sizeof(*tmp), GFP_KERNEL); > if (!tmp) > return NULL; > tmp->comp_index = 1; > tmp->comp_active_index = 0; > return tmp; > - > } > > /* release elem->private_free as well; called only once for each *_store */ > @@ -1067,7 +1047,7 @@ static void elem_private_free(struct snd_kcontrol *kctl) > static int add_new_ctl(struct usb_mixer_interface *mixer, > const struct snd_kcontrol_new *ncontrol, > int index, int val_type, int channels, > - const char *name, const void *opt, > + const char *name, void *opt, > bool do_private_free, > struct usb_mixer_elem_info **elem_ret) > { > @@ -1088,7 +1068,7 @@ static int add_new_ctl(struct usb_mixer_interface *mixer, > elem->head.id = index; > elem->val_type = val_type; > elem->channels = channels; > - elem->private_data = (void *) opt; > + elem->private_data = opt; > > kctl = snd_ctl_new1(ncontrol, elem); > if (!kctl) { > @@ -1113,10 +1093,8 @@ static int add_new_ctl(struct usb_mixer_interface *mixer, > return 0; > } > > -static struct snd_us16x08_control_params control_params; > - > /* table of EQ controls */ > -static struct snd_us16x08_control_params eq_controls[] = { > +static const struct snd_us16x08_control_params eq_controls[] = { > { /* EQ switch */ > .kcontrol_new = &snd_us16x08_eq_switch_ctl, > .control_id = SND_US16X08_ID_EQENABLE, > @@ -1197,7 +1175,7 @@ static struct snd_us16x08_control_params eq_controls[] = { > }; > > /* table of compressor controls */ > -static struct snd_us16x08_control_params comp_controls[] = { > +static const struct snd_us16x08_control_params comp_controls[] = { > { /* Comp enable */ > .kcontrol_new = &snd_us16x08_compswitch_ctl, > .control_id = SND_US16X08_ID_COMP_SWITCH, > @@ -1243,7 +1221,7 @@ static struct snd_us16x08_control_params comp_controls[] = { > }; > > /* table of channel controls */ > -static struct snd_us16x08_control_params channel_controls[] = { > +static const struct snd_us16x08_control_params channel_controls[] = { > { /* Phase */ > .kcontrol_new = &snd_us16x08_ch_boolean_ctl, > .control_id = SND_US16X08_ID_PHASE, > @@ -1279,7 +1257,7 @@ static struct snd_us16x08_control_params channel_controls[] = { > }; > > /* table of master controls */ > -static struct snd_us16x08_control_params master_controls[] = { > +static const struct snd_us16x08_control_params master_controls[] = { > { /* Master */ > .kcontrol_new = &snd_us16x08_master_ctl, > .control_id = SND_US16X08_ID_FADER, > @@ -1347,10 +1325,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer) > return -ENOMEM; > > /* add master controls */ > - for (i = 0; > - i < sizeof(master_controls) > - / sizeof(control_params); > - i++) { > + for (i = 0; i < ARRAY_SIZE(master_controls); i++) { > > err = add_new_ctl(mixer, > master_controls[i].kcontrol_new, > @@ -1368,10 +1343,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer) > } > > /* add channel controls */ > - for (i = 0; > - i < sizeof(channel_controls) > - / sizeof(control_params); > - i++) { > + for (i = 0; i < ARRAY_SIZE(channel_controls); i++) { > > err = add_new_ctl(mixer, > channel_controls[i].kcontrol_new, > @@ -1396,8 +1368,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer) > return -ENOMEM; > > /* add EQ controls */ > - for (i = 0; i < sizeof(eq_controls) / > - sizeof(control_params); i++) { > + for (i = 0; i < ARRAY_SIZE(eq_controls); i++) { > > err = add_new_ctl(mixer, > eq_controls[i].kcontrol_new, > @@ -1413,10 +1384,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer) > } > > /* add compressor controls */ > - for (i = 0; > - i < sizeof(comp_controls) > - / sizeof(control_params); > - i++) { > + for (i = 0; i < ARRAY_SIZE(comp_controls); i++) { > > err = add_new_ctl(mixer, > comp_controls[i].kcontrol_new, Regards Takashi Sakamoto