All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ALSA: fireface: add field for the number of messages copied to user space
@ 2023-02-02 13:37 Takashi Sakamoto
  2023-02-03 16:22 ` Takashi Iwai
  0 siblings, 1 reply; 4+ messages in thread
From: Takashi Sakamoto @ 2023-02-02 13:37 UTC (permalink / raw)
  To: tiwai; +Cc: alsa-devel

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>
---
 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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] ALSA: fireface: add field for the number of messages copied to user space
  2023-02-02 13:37 [PATCH] ALSA: fireface: add field for the number of messages copied to user space Takashi Sakamoto
@ 2023-02-03 16:22 ` Takashi Iwai
  2023-02-04  0:55   ` Takashi Sakamoto
  0 siblings, 1 reply; 4+ messages in thread
From: Takashi Iwai @ 2023-02-03 16:22 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel

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
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ALSA: fireface: add field for the number of messages copied to user space
  2023-02-03 16:22 ` Takashi Iwai
@ 2023-02-04  0:55   ` Takashi Sakamoto
  2023-02-04  8:36     ` Takashi Iwai
  0 siblings, 1 reply; 4+ messages in thread
From: Takashi Sakamoto @ 2023-02-04  0:55 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel

Hi,

On Fri, Feb 03, 2023 at 05:22:40PM +0100, Takashi Iwai wrote:
> 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...

You may well have the concern, indeed.

The structure is not exposed to user space yet, since it was added by a
commit ab811cfffa9 ("ALSA: fireface: update UAPI for data of knob
control"). It just exists in your (and mine) tree at present.


Thanks

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ALSA: fireface: add field for the number of messages copied to user space
  2023-02-04  0:55   ` Takashi Sakamoto
@ 2023-02-04  8:36     ` Takashi Iwai
  0 siblings, 0 replies; 4+ messages in thread
From: Takashi Iwai @ 2023-02-04  8:36 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel

On Sat, 04 Feb 2023 01:55:27 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Fri, Feb 03, 2023 at 05:22:40PM +0100, Takashi Iwai wrote:
> > 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...
> 
> You may well have the concern, indeed.
> 
> The structure is not exposed to user space yet, since it was added by a
> commit ab811cfffa9 ("ALSA: fireface: update UAPI for data of knob
> control"). It just exists in your (and mine) tree at present.

Ah I see, then that's OK, better to change now.
Applied now to for-next branch.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-02-04  8:37 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-02 13:37 [PATCH] ALSA: fireface: add field for the number of messages copied to user space Takashi Sakamoto
2023-02-03 16:22 ` Takashi Iwai
2023-02-04  0:55   ` Takashi Sakamoto
2023-02-04  8:36     ` Takashi Iwai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.