[RFC, PATCH] HSI: cmt_speech: fix timestamp interface

[RFC, PATCH] HSI: cmt_speech: fix timestamp interface

[Arnd Bergmann]

The user interface for timestamps in the new cms_speech
driver is broken in multiple ways:

- The layout is incompatible between 32-bit and 64-bit user
  space, because of the size differences in 'struct timespec'.
  This means that the driver can not work when used with 32-bit
  user space on a 64-bit kernel.

- As there are plans to change 32-bit user space to use
  a 64-bit time_t type in the future, it will also be
  incompatible with new 32-bit user space.

As the driver is not yet widely used, now would be a good time
to change it to just use a 64-bit nanosecond value, which also
happens to be more efficient. In order to make the layout
non-ambiguous, we have to also add extra padding so the 64-bit
value is naturally aligned.

The change requires the respective update to user space tools
using it. If that is for some reason not possible any more,
another solution would be to replace the timespec structure
with two __u32 values to avoid incompatibilities later.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/hsi/clients/cmt_speech.c b/drivers/hsi/clients/cmt_speech.c
index 4983529a9c6c..568dbc117f2e 100644
--- a/drivers/hsi/clients/cmt_speech.c
+++ b/drivers/hsi/clients/cmt_speech.c
@@ -451,9 +451,7 @@ static void cs_hsi_read_on_control_complete(struct hsi_msg *msg)
 	dev_dbg(&hi->cl->device, "Read on control: %08X\n", cmd);
 	cs_release_cmd(msg);
 	if (hi->flags & CS_FEAT_TSTAMP_RX_CTRL) {
-		struct timespec *tstamp =
-			&hi->mmap_cfg->tstamp_rx_ctrl;
-		do_posix_clock_monotonic_gettime(tstamp);
+		hi->mmap_cfg->tstamp_rx_ctrl = ktime_get_ns();
 	}
 	spin_unlock(&hi->lock);
 
diff --git a/include/uapi/linux/hsi/cs-protocol.h b/include/uapi/linux/hsi/cs-protocol.h
index 4957bba57cbe..fce633c7c649 100644
--- a/include/uapi/linux/hsi/cs-protocol.h
+++ b/include/uapi/linux/hsi/cs-protocol.h
@@ -90,12 +90,12 @@ struct cs_mmap_config_block {
 	__u32 tx_offsets[CS_MAX_BUFFERS];
 	__u32 rx_ptr;
 	__u32 rx_ptr_boundary;
-	__u32 reserved3[2];
+	__u32 reserved3[3];
 	/*
 	 * if enabled with CS_FEAT_TSTAMP_RX_CTRL, monotonic
 	 * timestamp taken when the last control command was received
 	 */
-	struct timespec tstamp_rx_ctrl;
+	__u64 tstamp_rx_ctrl;
 };
 
 #define CS_IO_MAGIC		'C'

Re: [RFC, PATCH] HSI: cmt_speech: fix timestamp interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2818 bytes --]

Hi Arnd,

[CC'd Kai's new mail address]

On Fri, Apr 10, 2015 at 01:59:19PM +0200, Arnd Bergmann wrote:
> The user interface for timestamps in the new cms_speech
> driver is broken in multiple ways:

ow :( Thanks for the report.

> - The layout is incompatible between 32-bit and 64-bit user
>   space, because of the size differences in 'struct timespec'.
>   This means that the driver can not work when used with 32-bit
>   user space on a 64-bit kernel.
> 
> - As there are plans to change 32-bit user space to use
>   a 64-bit time_t type in the future, it will also be
>   incompatible with new 32-bit user space.
> 
> As the driver is not yet widely used, now would be a good time
> to change it to just use a 64-bit nanosecond value, which also
> happens to be more efficient. In order to make the layout
> non-ambiguous, we have to also add extra padding so the 64-bit
> value is naturally aligned.
> 
> The change requires the respective update to user space tools
> using it. If that is for some reason not possible any more,
> another solution would be to replace the timespec structure
> with two __u32 values to avoid incompatibilities later.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I think updating to two __u32 values is the better solution here.
While the driver is new in the mainline kernel it has been used
for multiple years downstream in the Maemo/MeeGo/Mer communities
and I would prefer to keep userspace compatibility.

-- Sebastian

> diff --git a/drivers/hsi/clients/cmt_speech.c b/drivers/hsi/clients/cmt_speech.c
> index 4983529a9c6c..568dbc117f2e 100644
> --- a/drivers/hsi/clients/cmt_speech.c
> +++ b/drivers/hsi/clients/cmt_speech.c
> @@ -451,9 +451,7 @@ static void cs_hsi_read_on_control_complete(struct hsi_msg *msg)
>  	dev_dbg(&hi->cl->device, "Read on control: %08X\n", cmd);
>  	cs_release_cmd(msg);
>  	if (hi->flags & CS_FEAT_TSTAMP_RX_CTRL) {
> -		struct timespec *tstamp =
> -			&hi->mmap_cfg->tstamp_rx_ctrl;
> -		do_posix_clock_monotonic_gettime(tstamp);
> +		hi->mmap_cfg->tstamp_rx_ctrl = ktime_get_ns();
>  	}
>  	spin_unlock(&hi->lock);
>  
> diff --git a/include/uapi/linux/hsi/cs-protocol.h b/include/uapi/linux/hsi/cs-protocol.h
> index 4957bba57cbe..fce633c7c649 100644
> --- a/include/uapi/linux/hsi/cs-protocol.h
> +++ b/include/uapi/linux/hsi/cs-protocol.h
> @@ -90,12 +90,12 @@ struct cs_mmap_config_block {
>  	__u32 tx_offsets[CS_MAX_BUFFERS];
>  	__u32 rx_ptr;
>  	__u32 rx_ptr_boundary;
> -	__u32 reserved3[2];
> +	__u32 reserved3[3];
>  	/*
>  	 * if enabled with CS_FEAT_TSTAMP_RX_CTRL, monotonic
>  	 * timestamp taken when the last control command was received
>  	 */
> -	struct timespec tstamp_rx_ctrl;
> +	__u64 tstamp_rx_ctrl;
>  };
>  
>  #define CS_IO_MAGIC		'C'

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC, PATCH] HSI: cmt_speech: fix timestamp interface

[Arnd Bergmann]

On Friday 10 April 2015 15:36:10 Sebastian Reichel wrote:
> 
> > - The layout is incompatible between 32-bit and 64-bit user
> >   space, because of the size differences in 'struct timespec'.
> >   This means that the driver can not work when used with 32-bit
> >   user space on a 64-bit kernel.
> > 
> > - As there are plans to change 32-bit user space to use
> >   a 64-bit time_t type in the future, it will also be
> >   incompatible with new 32-bit user space.
> > 
> > As the driver is not yet widely used, now would be a good time
> > to change it to just use a 64-bit nanosecond value, which also
> > happens to be more efficient. In order to make the layout
> > non-ambiguous, we have to also add extra padding so the 64-bit
> > value is naturally aligned.
> > 
> > The change requires the respective update to user space tools
> > using it. If that is for some reason not possible any more,
> > another solution would be to replace the timespec structure
> > with two __u32 values to avoid incompatibilities later.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> I think updating to two __u32 values is the better solution here.
> While the driver is new in the mainline kernel it has been used
> for multiple years downstream in the Maemo/MeeGo/Mer communities
> and I would prefer to keep userspace compatibility.

Ok. You still need to update that userspace to the new ABI in order
to support compiling on 64-bit machines and on future arm32 with
64-bit time_t, but existing binaries will continue to run.

Can you (or someone on Cc) prepare a patch for this?

	Arnd

[PATCH] HSI: cmt_speech: fix timestamp interface

[Sebastian Reichel]

The user interface for timestamps in the new cmt_speech
driver is broken in multiple ways:

- The layout is incompatible between 32-bit and 64-bit user
  space, because of the size differences in 'struct timespec'.
  This means that the driver can not work when used with 32-bit
  user space on a 64-bit kernel.

- As there are plans to change 32-bit user space to use
  a 64-bit time_t type in the future, it will also be
  incompatible with new 32-bit user space.

To keep support for the user space tools written for this driver (which
have lived many years out-of-tree), the interface has been hardened to
unsigned 32-bit values.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/hsi/clients/cmt_speech.c     |  9 +++++++--
 include/uapi/linux/hsi/cs-protocol.h | 16 +++++++++++-----
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/hsi/clients/cmt_speech.c b/drivers/hsi/clients/cmt_speech.c
index 4983529..67aef3f 100644
--- a/drivers/hsi/clients/cmt_speech.c
+++ b/drivers/hsi/clients/cmt_speech.c
@@ -451,9 +451,14 @@ static void cs_hsi_read_on_control_complete(struct hsi_msg *msg)
 	dev_dbg(&hi->cl->device, "Read on control: %08X\n", cmd);
 	cs_release_cmd(msg);
 	if (hi->flags & CS_FEAT_TSTAMP_RX_CTRL) {
-		struct timespec *tstamp =
+		struct timespec tspec;
+		struct cs_timestamp *tstamp =
 			&hi->mmap_cfg->tstamp_rx_ctrl;
-		do_posix_clock_monotonic_gettime(tstamp);
+
+		do_posix_clock_monotonic_gettime(&tspec);
+
+		tstamp->tv_sec = (__u32) tspec.tv_sec;
+		tstamp->tv_nsec = (__u32) tspec.tv_nsec;
 	}
 	spin_unlock(&hi->lock);
 
diff --git a/include/uapi/linux/hsi/cs-protocol.h b/include/uapi/linux/hsi/cs-protocol.h
index 4957bba..f153d6e 100644
--- a/include/uapi/linux/hsi/cs-protocol.h
+++ b/include/uapi/linux/hsi/cs-protocol.h
@@ -76,6 +76,15 @@ struct cs_buffer_config {
 };
 
 /*
+ * struct for monotonic timestamp taken when the
+ * last control command was received
+ */
+struct cs_timestamp {
+	__u32 tv_sec;  /* seconds */
+	__u32 tv_nsec; /* nanoseconds */
+};
+
+/*
  * Struct describing the layout and contents of the driver mmap area.
  * This information is meant as read-only information for the application.
  */
@@ -91,11 +100,8 @@ struct cs_mmap_config_block {
 	__u32 rx_ptr;
 	__u32 rx_ptr_boundary;
 	__u32 reserved3[2];
-	/*
-	 * if enabled with CS_FEAT_TSTAMP_RX_CTRL, monotonic
-	 * timestamp taken when the last control command was received
-	 */
-	struct timespec tstamp_rx_ctrl;
+	/* enabled with CS_FEAT_TSTAMP_RX_CTRL */
+	struct cs_timestamp tstamp_rx_ctrl;
 };
 
 #define CS_IO_MAGIC		'C'
-- 
2.1.4

Re: [PATCH] HSI: cmt_speech: fix timestamp interface

[Thomas Gleixner]

On Thu, 16 Apr 2015, Sebastian Reichel wrote:

> The user interface for timestamps in the new cmt_speech
> driver is broken in multiple ways:
> 
> - The layout is incompatible between 32-bit and 64-bit user
>   space, because of the size differences in 'struct timespec'.
>   This means that the driver can not work when used with 32-bit
>   user space on a 64-bit kernel.
> 
> - As there are plans to change 32-bit user space to use
>   a 64-bit time_t type in the future, it will also be
>   incompatible with new 32-bit user space.
> 
> To keep support for the user space tools written for this driver (which
> have lived many years out-of-tree), the interface has been hardened to
> unsigned 32-bit values.
> 
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>  drivers/hsi/clients/cmt_speech.c     |  9 +++++++--
>  include/uapi/linux/hsi/cs-protocol.h | 16 +++++++++++-----
>  2 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hsi/clients/cmt_speech.c b/drivers/hsi/clients/cmt_speech.c
> index 4983529..67aef3f 100644
> --- a/drivers/hsi/clients/cmt_speech.c
> +++ b/drivers/hsi/clients/cmt_speech.c
> @@ -451,9 +451,14 @@ static void cs_hsi_read_on_control_complete(struct hsi_msg *msg)
>  	dev_dbg(&hi->cl->device, "Read on control: %08X\n", cmd);
>  	cs_release_cmd(msg);
>  	if (hi->flags & CS_FEAT_TSTAMP_RX_CTRL) {
> -		struct timespec *tstamp =
> +		struct timespec tspec;
> +		struct cs_timestamp *tstamp =
>  			&hi->mmap_cfg->tstamp_rx_ctrl;
> -		do_posix_clock_monotonic_gettime(tstamp);
> +
> +		do_posix_clock_monotonic_gettime(&tspec);

Can you please use ktime_get_ts() instead of
do_posix_clock_monotonic_gettime(). I'm desperately trying to get rid
of the latter, but it seems to come back in circles.

Thanks,

	tglx

Re: [PATCH] HSI: cmt_speech: fix timestamp interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

Hi Arnd,

On Thu, Apr 16, 2015 at 07:13:50PM +0200, Sebastian Reichel wrote:
> The user interface for timestamps in the new cmt_speech
> driver is broken in multiple ways:
> 
> - The layout is incompatible between 32-bit and 64-bit user
>   space, because of the size differences in 'struct timespec'.
>   This means that the driver can not work when used with 32-bit
>   user space on a 64-bit kernel.
> 
> - As there are plans to change 32-bit user space to use
>   a 64-bit time_t type in the future, it will also be
>   incompatible with new 32-bit user space.
> 
> To keep support for the user space tools written for this driver (which
> have lived many years out-of-tree), the interface has been hardened to
> unsigned 32-bit values.

Are you fine with this patch? I would like to queue it for 4.1-rc.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] HSI: cmt_speech: fix timestamp interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

Hi Thomas,

On Thu, Apr 23, 2015 at 03:37:57PM +0200, Thomas Gleixner wrote:
> > -		do_posix_clock_monotonic_gettime(tstamp);
> > +
> > +		do_posix_clock_monotonic_gettime(&tspec);
> 
> Can you please use ktime_get_ts() instead of
> do_posix_clock_monotonic_gettime(). I'm desperately trying to get rid
> of the latter, but it seems to come back in circles.

Sure, I modified the patch to use ktime_get_ts() instead of
do_posix_clock_monotonic_gettime(). If Arnd has no other
comments I will not resend it and queue the modified patch
for 4.1-rc.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]