[alsa-devel] [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path
Takashi Sakamoto
o-takashi at sakamocchi.jp
Wed Feb 22 14:37:22 CET 2017
On Feb 22 2017 16:46, Takashi Iwai wrote:
> -- 8< --
> From: Takashi Iwai <tiwai at 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 at 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 at 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
More information about the Alsa-devel
mailing list