Re: [PATCH v7 1/2] ALSA: FCP: Add Focusrite Control Protocol driver

[Takashi Iwai <tiwai@xxxxxxx>]

On Thu, 16 Jan 2025 10:03:35 +0100,
Geoffrey D. Bennett wrote:
> diff --git a/include/uapi/sound/tlv.h b/include/uapi/sound/tlv.h
> index b99a2414b53d..b6e260e35529 100644
> --- a/include/uapi/sound/tlv.h
> +++ b/include/uapi/sound/tlv.h
> @@ -18,6 +18,8 @@
>  #define SNDRV_CTL_TLVT_CHMAP_VAR	0x102	/* channels freely swappable */
>  #define SNDRV_CTL_TLVT_CHMAP_PAIRED	0x103	/* pair-wise swappable */
>  
> +#define SNDRV_CTL_TLVT_LABELS		0x110	/* channel labels */

IMHO, the data definition looks too ambiguous here to be put as a
public API.  It can be also a more specific name such as
*_CHANNEL_LABELS, too.


> +static int fcp_meter_ctl_get(struct snd_kcontrol *kctl,
> +			     struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_info *elem = kctl->private_data;
> +	struct usb_mixer_interface *mixer = elem->head.mixer;
> +	struct fcp_data *private = mixer->private_data;
> +	int num_meter_slots, resp_size;
> +	u32 *resp __free(kfree) = NULL;
> +	int i, err = 0;
> +
> +	struct {
> +		__le16 pad;
> +		__le16 num_meters;
> +		__le32 magic;
> +	} __packed req;
> +
> +	guard(mutex)(&private->mutex);
> +
> +	err = fcp_reinit(mixer);
> +	if (err < 0)
> +		return err;
> +
> +	num_meter_slots = private->num_meter_slots;
> +	resp_size = num_meter_slots * sizeof(u32);
> +
> +	resp = kmalloc(resp_size, GFP_KERNEL);
> +	if (!resp)
> +		return -ENOMEM;

It depends on how often this kcontrol gets read, but if it's
frequently read, it might make sense to allocate this buffer
beforehand and keep using it instead of allocating / freeing at each
call.  The call is mutual, hence a common buffer is fine.

> +static int fcp_meter_tlv_callback(struct snd_kcontrol *kctl,
> +				  int op_flag, unsigned int size,
> +				  unsigned int __user *tlv)
> +{
> +	struct usb_mixer_elem_info *elem = kctl->private_data;
> +	struct usb_mixer_interface *mixer = elem->head.mixer;
> +	struct fcp_data *private = mixer->private_data;
> +
> +	if (op_flag == SNDRV_CTL_TLV_OP_READ) {
> +		if (private->meter_labels_tlv_size == 0)
> +			return 0;
> +
> +		if (size > private->meter_labels_tlv_size)
> +			size = private->meter_labels_tlv_size;
> +
> +		if (copy_to_user(tlv, private->meter_labels_tlv, size))
> +			return -EFAULT;

The private->meter_label* stuff can be changed dynamically via ioctl,
hence this call should have the data protection, too.
I didn't check fully, but private->mutex can be taken in this code
path, I suppose.


> +/* FCP initialisation */
> +static int fcp_ioctl_init(struct usb_mixer_interface *mixer,
> +			  struct fcp_init __user *arg)
> +{
> +	struct fcp_init init;
> +	struct usb_device *dev = mixer->chip->dev;
> +	struct fcp_data *private = mixer->private_data;
> +	void *resp __free(kfree) = NULL;
> +	void *step2_resp;
> +	int err, buf_size;
> +
> +	if (usb_pipe_type_check(dev, usb_sndctrlpipe(dev, 0)))
> +		return -EINVAL;
> +
> +	/* Get initialisation parameters */
> +	if (copy_from_user(&init, arg, sizeof(init)))
> +		return -EFAULT;
> +
> +	/* Validate the response sizes */
> +	if (init.step0_resp_size < 1 ||
> +	    init.step0_resp_size > 255 ||
> +	    init.step2_resp_size < 1 ||
> +	    init.step2_resp_size > 255)
> +		return -EINVAL;
> +
> +	private->step0_resp_size = init.step0_resp_size;
> +	private->step2_resp_size = init.step2_resp_size;
> +	private->init1_opcode = init.init1_opcode;
> +	private->init2_opcode = init.init2_opcode;
> +
> +	/* Allocate response buffer */
> +	buf_size = private->step0_resp_size + private->step2_resp_size;
> +
> +	resp = kmalloc(buf_size, GFP_KERNEL);
> +	if (!resp)
> +		return -ENOMEM;

I'd set private->step* and co after allocation of the buffer.
When they are set beforehand, even if the buffer allocation failed,
those values are still set.

> +/* Set the Level Meter labels */
> +static int fcp_ioctl_set_meter_labels(struct usb_mixer_interface *mixer,
> +				      struct fcp_meter_labels __user *arg)
> +{
(snip)
> +	/* Calculate padded data size */
> +	data_size = ((labels.labels_size + sizeof(unsigned int) - 1) &
> +		     ~(sizeof(unsigned int) - 1));

ALIGN() macro can be used nicely here.


thanks,

Takashi