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

[Takashi Iwai <tiwai@suse.de>]

On Wed, 25 Dec 2024 22:23:19 +0100,
Geoffrey D. Bennett wrote:
> 
> --- /dev/null
> +++ b/include/uapi/sound/fcp.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Focusrite Control Protocol Driver for ALSA
> + *
> + * Copyright (c) 2024 by Geoffrey D. Bennett <g at b4.vu>
> + */
> +#ifndef __UAPI_SOUND_FCP_H
> +#define __UAPI_SOUND_FCP_H
> +
> +#ifdef __KERNEL__
> +#include <linux/types.h>
> +#else
> +#include <stdint.h>
> +#endif
> +
> +#include <linux/ioctl.h>
> +
> +#define FCP_HWDEP_MAJOR 2
> +#define FCP_HWDEP_MINOR 0
> +#define FCP_HWDEP_SUBMINOR 0
> +
> +#define FCP_HWDEP_VERSION \
> +	((FCP_HWDEP_MAJOR << 16) | \
> +	 (FCP_HWDEP_MINOR << 8) | \
> +	  FCP_HWDEP_SUBMINOR)
> +
> +#define FCP_HWDEP_VERSION_MAJOR(v) (((v) >> 16) & 0xFF)
> +#define FCP_HWDEP_VERSION_MINOR(v) (((v) >> 8) & 0xFF)
> +#define FCP_HWDEP_VERSION_SUBMINOR(v) ((v) & 0xFF)
> +
> +/* Get protocol version */
> +#define FCP_IOCTL_PVERSION _IOR('S', 0x60, int)
> +
> +/* Do FCP step 0 */
> +struct fcp_step0 {
> +	void     *data;
> +	uint16_t  size;
> +};
> +#define FCP_IOCTL_INIT _IOWR('S', 0x64, struct fcp_step0)

An ioctl with a pointer has to be handled carefully because you must
convert it for 32bit compat code.  IOW, it's better to be avoided if
possible.

The same is true for other ioctls.


> --- /dev/null
> +++ b/sound/usb/fcp.c
(snip)
> +struct fcp_data {
> +	struct usb_mixer_interface *mixer;
> +
> +	struct mutex mutex;         /* serialise access to the device */
> +	struct completion cmd_done; /* wait for command completion */
> +	struct file *file;          /* hwdep file */
> +
> +	/* notify waiting to send to *file */
> +	struct {
> +		wait_queue_head_t queue;
> +		u32               event;
> +		spinlock_t        lock;
> +	} notify;

Might be better to define outside with an explicit type name in case
of code simplification (e.g. factoring out the struct init code,
etc).

> +	__u8  bInterfaceNumber;
> +	__u8  bEndpointAddress;
> +	__u16 wMaxPacketSize;
> +	__u8  bInterval;

This is an internal struct and should be types without __ prefix.

> +/* 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, *resp = 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;
> +
> +	req = kmalloc(req_buf_size, GFP_KERNEL);
> +	if (!req) {
> +		err = -ENOMEM;
> +		goto done;

You can use cleanup.h stuff for code simplification.
(Also for other possible functions.)

> +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;
> +	int err;
> +
> +	mutex_lock(&private->mutex);
> +
> +	switch (cmd) {
> +
> +	case FCP_IOCTL_PVERSION:
> +		err = put_user(FCP_HWDEP_VERSION,
> +			       (int __user *)arg) ? -EFAULT : 0;
> +		break;
> +
> +	case FCP_IOCTL_INIT:
> +		err = fcp_ioctl_init(mixer, arg);
> +		break;
> +
> +	case FCP_IOCTL_CMD:
> +		err = fcp_ioctl_cmd(mixer, arg);
> +		break;
> +
> +	case FCP_IOCTL_SET_METER_MAP:
> +		err = fcp_ioctl_set_meter_map(mixer, arg);
> +		break;
> +
> +	default:
> +		err = -ENOIOCTLCMD;
> +	}
> +
> +	mutex_unlock(&private->mutex);

There seems an implicit ordering of the command -- FCP_IOCTL_INIT must
be called before *_CMD?  If so, do we have a sanity check for that?

Also, the behavior of *_SET_METER_MAP -- it dynamically creates and
deletes the meter kcontrol -- can be a bit dangerous.  Can't it be
without recreating the kcontrol itself?

Last but not least, any need for suspend/resume and disconnect
handling?


thanks,

Takashi