[alsa-devel] [PATCH V4] ALSA: usb-audio: Scarlett Gen 2 mixer interface

[Takashi Iwai]

On Tue, 09 Jul 2019 15:26:05 +0200,
Geoffrey D. Bennett wrote:
> 
> Only outstanding question I believe:
> 
> - Takashi, you wrote last time "Also, the delayed work needs to be
>   canceled or flushed at PM suspend callback as well as
>   disconnecting." I'm not sure why it should be cancelled; is it not
>   okay to do on resume?

Not really.  The standard idiom is to cancel the pending task
immediately at suspend, so that the whole PM procedure won't be
interfered, then reschedule the work at resume again.

> But I tested suspending while the delayed work
>   was waiting and it seemed like the delayed work was cancelled
>   anyway. Probably better to do a flush on suspend though;

The flush would block, hence it wastes lots of time at suspend.
Ideally the suspend should be performed immediately.

> should this
>   be done through a hook like:
> 
>   diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>   index 2444737a14b2..1572a89267c6 100644
>   --- a/sound/usb/mixer.c
>   +++ b/sound/usb/mixer.c
>   @@ -3547,6 +3547,8 @@ static int snd_usb_mixer_activate(struct usb_mixer_interface *mixer)
>    int snd_usb_mixer_suspend(struct usb_mixer_interface *mixer)
>    {
>           snd_usb_mixer_inactivate(mixer);
>   +       if (mixer->private_suspend)
>   +               mixer->private_suspend(mixer);
>           return 0;
>    }
>    
>   diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
>   index fa6e216a06f0..d94c688c65f7 100644
>   --- a/sound/usb/mixer.h
>   +++ b/sound/usb/mixer.h
>   @@ -28,6 +28,7 @@ struct usb_mixer_interface {
>    
>           void *private_data;
>           void (*private_free)(struct usb_mixer_interface *private_data);
>   +       void (*private_suspend)(struct usb_mixer_interface *);
>    };
>    
>    #define MAX_CHANNELS   16      /* max logical channels */
> 
>   ...or just keep the existing behaviour where it seems to get
>   cancelled?

I'm fine to add the new suspend/resume callbacks, but maybe the
corresponding resume would be needed.

Other than that, the patch looks almost good.

> --- /dev/null
> +++ b/sound/usb/mixer_scarlett_gen2.c
> @@ -0,0 +1,2078 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *   Focusrite Scarlett 6i6/18i8/18i20 Gen 2 Driver for ALSA
> + *
> + *   Copyright (c) 2018-2019 by Geoffrey D. Bennett <g at b4.vu>
> + *
> + *   Based on the Scarlett (Gen 1) Driver for ALSA:
> + *
> + *   Copyright (c) 2013 by Tobias Hoffmann
> + *   Copyright (c) 2013 by Robin Gareus <robin at gareus.org>
> + *   Copyright (c) 2002 by Takashi Iwai <tiwai at suse.de>
> + *   Copyright (c) 2014 by Chris J Arges <chris.j.arges at canonical.com>
> + *
> + *   Many codes borrowed from audio.c by
> + *     Alan Cox (alan at lxorguk.ukuu.org.uk)
> + *     Thomas Sailer (sailer at ife.ee.ethz.ch)
> + *
> + *   Code cleanup:
> + *   David Henningsson <david.henningsson at canonical.com>
> + *
> + *   This program is free software; you can redistribute it and/or modify
> + *   it under the terms of the GNU General Public License as published by
> + *   the Free Software Foundation; either version 2 of the License, or
> + *   (at your option) any later version.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.

Drop these texts.  The SPDX identifier should suffice.

> +static int scarlett_gen2_mixer_enable;
> +module_param(scarlett_gen2_mixer_enable, int, 0444);
> +MODULE_PARM_DESC(scarlett_gen2_mixer_enable,
> +		 "Focusrite Scarlett Gen 2 Mixer Driver Enable");

Do we need this?  If disabling the quirk is really required, it should
be implemented rather in a generic option, instead.


> +/* Send a proprietary format request to the Scarlett interface */
> +static int scarlett2_usb(
> +	struct usb_mixer_interface *mixer, u32 cmd,
> +	void *req_data, u16 req_size, void *resp_data, u16 resp_size)
> +{
> +	struct scarlett2_mixer_data *private = mixer->private_data;
> +
> +	u16 req_buf_size = sizeof(struct scarlett2_usb_packet) + req_size;
> +	u16 resp_buf_size = sizeof(struct scarlett2_usb_packet) + resp_size;
> +
> +	struct scarlett2_usb_packet *req = 0, *resp = 0;
> +
> +	int err = 0;

Avoid unnecessary blank lines.

> +
> +	mutex_lock(&private->usb_mutex);
> +
> +	/* build request message and send it */
> +
> +	req = kmalloc(req_buf_size, GFP_KERNEL);
> +	if (!req) {
> +		err = -ENOMEM;
> +		goto unlock;
> +	}

Better to run kmalloc outside the mutex.  It's not necessarily
exclusive.

> +	/* send a second message to get the response */
> +
> +	resp = kmalloc(resp_buf_size, GFP_KERNEL);
> +	if (!resp) {
> +		err = -ENOMEM;
> +		goto unlock;
> +	}

Ditto, this could be better allocated before the critical section.


thanks,

Takashi