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

[Takashi Iwai <tiwai@xxxxxxx>]

On Sun, 12 Jan 2025 18:10:31 +0100,
Geoffrey D. Bennett wrote:
> 
> +/* Send an FCP command and get the response */
> +static int fcp_usb(struct usb_mixer_interface *mixer, u32 opcode,
> +		   const void *req_data, u16 req_size,
> +		   void *resp_data, u16 resp_size)
> +{
> +	struct fcp_data *private = mixer->private_data;
> +	struct usb_device *dev = mixer->chip->dev;
> +	struct fcp_usb_packet *req __free(kfree) = NULL;
> +	struct fcp_usb_packet *resp __free(kfree) = NULL;
> +	size_t req_buf_size = struct_size(req, data, req_size);
> +	size_t resp_buf_size = struct_size(resp, data, resp_size);
> +	int retries = 0;
> +	const int max_retries = 5;
> +	int err;
> +
> +	if (!mixer->urb) {
> +		void *step0_resp __free(kfree) = NULL;
> +		void *step2_resp __free(kfree) = NULL;
> +
> +		step0_resp = kmalloc(private->step0_resp_size, GFP_KERNEL);
> +		if (!step0_resp)
> +			return -ENOMEM;
> +		step2_resp = kmalloc(private->step2_resp_size, GFP_KERNEL);
> +		if (!step2_resp)
> +			return -ENOMEM;
> +		err = fcp_init(mixer, step0_resp, step2_resp);
> +		if (err < 0)
> +			return err;
> +	}

This happens only after the suspend, and this is for automatic
re-initialization, right?

It's a bit scary that such a low-level function itself does
re-initialization, as fcp_init() will call this function again.
That said, it's more straightforward if the re-initialization is
checked and done in the upper level.  (And here check only mixer->urb
and return error (or spew WARN_ON()).

> +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_size == 0)
> +			return 0;
> +
> +		if (size > private->meter_labels_size)
> +			size = private->meter_labels_size;
> +
> +		if (copy_to_user(tlv, private->meter_labels, size))
> +			return -EFAULT;
> +
> +		return size;
> +	}
> +
> +	if (op_flag == SNDRV_CTL_TLV_OP_WRITE) {
> +		kfree(private->meter_labels);
> +		private->meter_labels = NULL;
> +		private->meter_labels_size = 0;
> +
> +		if (size == 0)
> +			return 0;
> +
> +		if (size > 4096)
> +			return -EINVAL;
> +
> +		private->meter_labels = kmalloc(size, GFP_KERNEL);
> +		if (!private->meter_labels)
> +			return -ENOMEM;
> +
> +		if (copy_from_user(private->meter_labels, tlv, size)) {
> +			kfree(private->meter_labels);
> +			private->meter_labels = NULL;
> +			return -EFAULT;
> +		}
> +
> +		private->meter_labels_size = size;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}

Hmm, this special is a special use of TLV in non-standard way, which
needs definitely documentation.  The use is no longer TLV, just some
raw read/write of a bulk data for the kcontrol , after all.

Also, I couldn't figure out what exactly this "meter_labels" stuff
serves for.  It's not referred from anywhere else than TLV read?


> +/* Execute an FCP command specified by the user */
> +static int fcp_ioctl_cmd(struct usb_mixer_interface *mixer, unsigned long arg)
> +{
> +	struct fcp_cmd cmd;
> +	int err, buf_size;
> +	void *data __free(kfree) = NULL;
> +
> +	/* get opcode and request/response size */
> +	if (copy_from_user(&cmd, (void __user *)arg, sizeof(cmd)))
> +		return -EFAULT;

You can avoid unneeded multiple cast.  e.g. make a void * pointer in
the caller side cast from arg, then pass it to each function.
Each function may have an argument with the explicit type pointer,
here would be "struct fcp_cmd *arg".  Then the rest can be just "arg"
here as the address, and...

> +	/* copy request from user */
> +	if (cmd.req_size > 0)
> +		if (copy_from_user(data, ((void __user *)arg) + sizeof(cmd),
> +				   cmd.req_size))

"arg.data" here instead of the error-prone cast + offset.


thanks,

Takashi