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

[Takashi Iwai <tiwai@xxxxxxx>]

On Mon, 13 Jan 2025 20:42:01 +0100,
Geoffrey D. Bennett wrote:
> +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);
> +
> +	num_meter_slots = private->num_meter_slots;
> +	resp_size = num_meter_slots * sizeof(u32);
> +
> +	resp = kmalloc(resp_size, GFP_KERNEL);
> +	if (!resp)
> +		return -ENOMEM;
> +
> +	req.pad = 0;
> +	req.num_meters = cpu_to_le16(num_meter_slots);
> +	req.magic = cpu_to_le32(FCP_USB_METER_LEVELS_GET_MAGIC);
> +	err = fcp_usb(mixer, FCP_USB_GET_METER,
> +		      &req, sizeof(req), resp, resp_size);
> +
> +	/* reinit and retry if needed */
> +	if (err == -ENODEV) {
> +		err = fcp_reinit(mixer);
> +		if (err < 0)
> +			return err;
> +		err = fcp_usb(mixer, FCP_USB_GET_METER,
> +			      &req, sizeof(req), resp, resp_size);
> +	} else if (err < 0) {
> +		return err;
> +	}

You seem to have forgotten to check the error after the second
fcp_usb()?

I think fcp_reinit() can be better checked before the first fcb_usb()
call, just by checking mixer->urb.  i.e.

	if (!mixer->urb) {
		err = fcp_reinit(mixer);
		if (err < 0)
			return err;
	}

	err = fcp_usb(...);
	if (err < 0)
		return err;

Or change fcp_reinit() to return 0 if mixer->urbs is already set, then
you can call fcp_reinit() unconditionally, too.


> +/* Execute an FCP command specified by the user */
> +static int fcp_ioctl_cmd(struct usb_mixer_interface *mixer,
> +			 struct fcp_cmd __user *arg)
> +{
(snip)
> +	/* copy response to user */
> +	if (cmd.resp_size > 0)
> +		if (copy_to_user(((void __user *)arg) + sizeof(cmd), data,
> +				 cmd.resp_size))

This should be
		if (copy_to_user(arg->data, data, cmd.resp_size))

(snip)
> +/* Set the Level Meter map and add the control */
> +static int fcp_ioctl_set_meter_map(struct usb_mixer_interface *mixer,
> +				   unsigned long arg)
> +{
> +	struct fcp_meter_map map;
> +	struct fcp_data *private = mixer->private_data;
> +	s16 *tmp_map __free(kfree) = NULL;
> +	int err;
> +
> +	if (copy_from_user(&map, (void __user *)arg, sizeof(map)))

This function can also take struct fcp_meter_map __user * argument,
and you can avoid the cast.

Ditto for fcp_ioctl_init().

> +static int fcp_hwdep_ioctl(struct snd_hwdep *hw, struct file *file,
> +			   unsigned int cmd, unsigned long arg)
> +{
> +	struct usb_mixer_interface *mixer = hw->private_data;
> +	struct fcp_data *private = mixer->private_data;

You can assign a void __user pointer, e.g.

	void __user *argp = (void __user *)arg;

and pass it to each function, e.g.

> +	guard(mutex)(&private->mutex);
> +
> +	switch (cmd) {
> +
> +	case FCP_IOCTL_PVERSION:
> +		return put_user(FCP_HWDEP_VERSION,
> +				(int __user *)arg) ? -EFAULT : 0;
> +		break;
> +
> +	case FCP_IOCTL_INIT:
> +		return fcp_ioctl_init(mixer, arg);

This will be:
		return fcp_ioctl_init(mixer, argp);

and make fcp_ioctl_init() receiving it like:

static int fcp_ioctl_init(struct usb_mixer_interface *mixer,
			  struct fcp_init __user *arg)
{
	struct fcp_init init;
	....

	if (copy_from_user(&init, arg, sizeof(init)))
		return -EFAULT;	
	....
	if (copy_to_user(arg->resp, resp, buf_size))
		return -EFAULT;


Takashi