Re: [PATCH] ALSA: fireface: add field for the number of messages copied to user space

From: Takashi Iwai <tiwai@suse.de>
To: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Cc: alsa-devel@alsa-project.org
Subject: Re: [PATCH] ALSA: fireface: add field for the number of messages copied to user space
Date: Fri, 03 Feb 2023 17:22:40 +0100	[thread overview]
Message-ID: <87fsbmn2gf.wl-tiwai@suse.de> (raw)
In-Reply-To: <20230202133708.163936-1-o-takashi@sakamocchi.jp>

On Thu, 02 Feb 2023 14:37:08 +0100,
Takashi Sakamoto wrote:
> 
> Current structure includes no field to express the number of messages
> copied to user space, thus user space application needs to information
> out of the structure to parse the content of structure.
> 
> This commit adds a field to express the number of messages copied to user
> space since It is more preferable to use self-contained structure.
> 
> Kees Cook proposed an idea of annotation for bound of flexible arrays
> in his future improvement for flexible-length array in kernel. The
> additional field for message count is suitable to the idea as well.
> 
> Reference: https://people.kernel.org/kees/bounded-flexible-arrays-in-c
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Wouldn't changing this break the existing application that talks with
the older ABI?  Just to be sure...

thanks,

Takashi

> ---
>  include/uapi/sound/firewire.h                |  2 ++
>  sound/firewire/fireface/ff-protocol-former.c | 28 +++++++++++---------
>  2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/include/uapi/sound/firewire.h b/include/uapi/sound/firewire.h
> index 50917581dd2b..1e86872c151f 100644
> --- a/include/uapi/sound/firewire.h
> +++ b/include/uapi/sound/firewire.h
> @@ -78,6 +78,7 @@ struct snd_firewire_event_motu_register_dsp_change {
>   *					     operating hardware knob.
>   *
>   * @type: Fixed to SNDRV_FIREWIRE_EVENT_FF400_MESSAGE.
> + * @message_count: The number of messages.
>   * @messages.message: The messages expressing hardware knob operation.
>   * @messages.tstamp: The isochronous cycle at which the request subaction of asynchronous
>   *		     transaction was sent to deliver the message. It has 16 bit unsigned integer
> @@ -89,6 +90,7 @@ struct snd_firewire_event_motu_register_dsp_change {
>   */
>  struct snd_firewire_event_ff400_message {
>  	unsigned int type;
> +	unsigned int message_count;
>  	struct {
>  		__u32 message;
>  		__u32 tstamp;
> diff --git a/sound/firewire/fireface/ff-protocol-former.c b/sound/firewire/fireface/ff-protocol-former.c
> index fa41de978756..efd59e9d9935 100644
> --- a/sound/firewire/fireface/ff-protocol-former.c
> +++ b/sound/firewire/fireface/ff-protocol-former.c
> @@ -677,23 +677,19 @@ static void ff400_handle_msg(struct snd_ff *ff, unsigned int offset, const __le3
>  
>  static long ff400_copy_msg_to_user(struct snd_ff *ff, char __user *buf, long count)
>  {
> +	struct snd_firewire_event_ff400_message ev = {
> +		.type = SNDRV_FIREWIRE_EVENT_FF400_MESSAGE,
> +		.message_count = 0,
> +	};
>  	struct ff400_msg_parser *parser = ff->msg_parser;
> -	u32 type = SNDRV_FIREWIRE_EVENT_FF400_MESSAGE;
>  	long consumed = 0;
> -	int ret = 0;
> +	long ret = 0;
>  
> -	if (count < 8)
> +	if (count < sizeof(ev) || parser->pull_pos == parser->push_pos)
>  		return 0;
>  
> -	spin_unlock_irq(&ff->lock);
> -	if (copy_to_user(buf, &type, sizeof(type)))
> -		ret = -EFAULT;
> -	spin_lock_irq(&ff->lock);
> -	if (ret)
> -		return ret;
> -
> -	count -= sizeof(type);
> -	consumed += sizeof(type);
> +	count -= sizeof(ev);
> +	consumed += sizeof(ev);
>  
>  	while (count >= sizeof(*parser->msgs) && parser->pull_pos != parser->push_pos) {
>  		spin_unlock_irq(&ff->lock);
> @@ -707,10 +703,18 @@ static long ff400_copy_msg_to_user(struct snd_ff *ff, char __user *buf, long cou
>  		++parser->pull_pos;
>  		if (parser->pull_pos >= FF400_QUEUE_SIZE)
>  			parser->pull_pos = 0;
> +		++ev.message_count;
>  		count -= sizeof(*parser->msgs);
>  		consumed += sizeof(*parser->msgs);
>  	}
>  
> +	spin_unlock_irq(&ff->lock);
> +	if (copy_to_user(buf, &ev, sizeof(ev)))
> +		ret = -EFAULT;
> +	spin_lock_irq(&ff->lock);
> +	if (ret)
> +		return ret;
> +
>  	return consumed;
>  }
>  
> -- 
> 2.37.2
> 