Re: [alsa-devel] [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path

On Feb 22 2017 16:46, Takashi Iwai wrote:

-- 8< -- From: Takashi Iwai tiwai@suse.de 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 tiwai@suse.de

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 o-takashi@sakamocchi.jp

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