mbox series

[v6,0/8] Fix year 2038 issue for sound subsystem

Message ID 20191112151642.680072-1-arnd@arndb.de (mailing list archive)
Headers show
Series Fix year 2038 issue for sound subsystem | expand

Message

Arnd Bergmann Nov. 12, 2019, 3:16 p.m. UTC
This is a series I worked on with Baolin in 2017 and 2018, but we
never quite managed to finish up the last pieces. During the
ALSA developer meetup at ELC-E 2018 in Edinburgh, a decision was
made to go with this approach for keeping best compatibility
with existing source code, and then I failed to follow up by
resending the patches.

Now I have patches for all remaining time_t uses in the kernel,
so it's absolutely time to revisit them. I have done more
review of the patches myself and found a couple of minor issues
that I have fixed up, otherwise the series is still the same as
before.

Conceptually, the idea of these patches is:

- 64-bit applications should see no changes at all, neither
  compile-time nor run-time.

- 32-bit code compiled with a 64-bit time_t currently
  does not work with ALSA, and requires kernel changes and/or
  sound/asound.h changes

- Most 32-bit code using these interfaces will work correctly
  on a modified kernel, with or without the uapi header changes.

- 32-bit code using SNDRV_TIMER_IOCTL_TREAD requires the
  updated header file for 64-bit time_t support

- 32-bit i386 user space with 64-bit time_t is broken for
  SNDRV_PCM_IOCTL_STATUS, SNDRV_RAWMIDI_IOCTL_STATUS and
  SNDRV_PCM_IOCTL_SYNC_PTR because of i386 alignment. This is also
  addressed by the updated uapi header.

- PCM mmap is currently supported on native x86 kernels
  (both 32-bit and 64-bit) but not for compat mode. This series breaks
  the 32-bit native mmap support for 32-bit time_t, but instead allows
  it for 64-bit time_t on both native and compat kernels. This seems to
  be the best trade-off, as mmap support is optional already, and most
  32-bit code runs in compat mode anyway.

- I've tried to avoid breaking compilation of 32-bit code
  as much as possible. Anything that does break however is likely code
  that is already broken on 64-bit time_t and needs source changes to
  fix them.

I would like to propose merging this into the alsa tree after
the v5.5 merge window for inclusion into v5.6, to allow a good
amount of testing, in particular for the header changes that
may cause problems for user space applications.

A git branch with the same contents is available for testing at [1].

Please review and test!

     Arnd

[1] https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git y2038-alsa
[2] https://lore.kernel.org/lkml/CAK8P3a2Os66+iwQYf97qh05W2JP8rmWao8zmKoHiXqVHvyYAJA@mail.gmail.com/T/#m6519cb07cfda08adf1dedea6596bb98892b4d5dc

Changes since v5 (Arnd):
 - Rebased to linux-5.4-rc4
 - Updated to completely remove timespec and time_t references from alsa
 - found and fixed a few bugs

Changes since v4 (Baolin):
 - Add patch 5 to change trigger_tstamp member of struct snd_pcm_runtime.
 - Add patch 8 to change internal timespec.
 - Add more explanation in commit message.
 - Use ktime_get_real_ts64() in patch 6.
 - Split common code out into a separate function in patch 6.
 - Fix tu->tread bug in patch 6 and remove #if __BITS_PER_LONG == 64 macro.

Changes since v3:
 - Move struct snd_pcm_status32 to pcm.h file.
 - Modify comments and commit message.
 - Add new patch2 ~ patch6.

Changes since v2:
 - Renamed all structures to make clear.
 - Remove CONFIG_X86_X32 macro and introduced new compat_snd_pcm_status64_x86_32.

Changes since v1:
 - Add one macro for struct snd_pcm_status_32 which only active in 32bits kernel.
 - Convert pcm_compat.c to use struct snd_pcm_status_64.
 - Convert pcm_native.c to use struct snd_pcm_status_64.

---

Arnd Bergmann (2):
  ALSA: move snd_pcm_ioctl_sync_ptr_compat into pcm_native.c
  ALSA: add new 32-bit layout for snd_pcm_mmap_status/control

Baolin Wang (6):
  ALSA: Replace timespec with timespec64
  ALSA: Avoid using timespec for struct snd_timer_status
  ALSA: Avoid using timespec for struct snd_ctl_elem_value
  ALSA: Avoid using timespec for struct snd_pcm_status
  ALSA: Avoid using timespec for struct snd_rawmidi_status
  ALSA: Avoid using timespec for struct snd_timer_tread

 include/sound/pcm.h               |  74 ++++++--
 include/sound/timer.h             |   4 +-
 include/uapi/sound/asound.h       | 132 ++++++++++++--
 sound/core/pcm.c                  |  12 +-
 sound/core/pcm_compat.c           | 282 ++++++++----------------------
 sound/core/pcm_lib.c              |  38 ++--
 sound/core/pcm_native.c           | 226 +++++++++++++++++++++---
 sound/core/rawmidi.c              | 132 +++++++++++---
 sound/core/rawmidi_compat.c       |  87 +++------
 sound/core/timer.c                | 229 ++++++++++++++++++------
 sound/core/timer_compat.c         |  62 +------
 sound/pci/hda/hda_controller.c    |  10 +-
 sound/soc/intel/skylake/skl-pcm.c |   4 +-
 13 files changed, 804 insertions(+), 488 deletions(-)

Comments

Takashi Iwai Nov. 12, 2019, 8:37 p.m. UTC | #1
On Tue, 12 Nov 2019 16:16:34 +0100,
Arnd Bergmann wrote:
> 
> This is a series I worked on with Baolin in 2017 and 2018, but we
> never quite managed to finish up the last pieces. During the
> ALSA developer meetup at ELC-E 2018 in Edinburgh, a decision was
> made to go with this approach for keeping best compatibility
> with existing source code, and then I failed to follow up by
> resending the patches.
> 
> Now I have patches for all remaining time_t uses in the kernel,
> so it's absolutely time to revisit them. I have done more
> review of the patches myself and found a couple of minor issues
> that I have fixed up, otherwise the series is still the same as
> before.
> 
> Conceptually, the idea of these patches is:
> 
> - 64-bit applications should see no changes at all, neither
>   compile-time nor run-time.
> 
> - 32-bit code compiled with a 64-bit time_t currently
>   does not work with ALSA, and requires kernel changes and/or
>   sound/asound.h changes
> 
> - Most 32-bit code using these interfaces will work correctly
>   on a modified kernel, with or without the uapi header changes.
> 
> - 32-bit code using SNDRV_TIMER_IOCTL_TREAD requires the
>   updated header file for 64-bit time_t support
> 
> - 32-bit i386 user space with 64-bit time_t is broken for
>   SNDRV_PCM_IOCTL_STATUS, SNDRV_RAWMIDI_IOCTL_STATUS and
>   SNDRV_PCM_IOCTL_SYNC_PTR because of i386 alignment. This is also
>   addressed by the updated uapi header.
> 
> - PCM mmap is currently supported on native x86 kernels
>   (both 32-bit and 64-bit) but not for compat mode. This series breaks
>   the 32-bit native mmap support for 32-bit time_t, but instead allows
>   it for 64-bit time_t on both native and compat kernels. This seems to
>   be the best trade-off, as mmap support is optional already, and most
>   32-bit code runs in compat mode anyway.
> 
> - I've tried to avoid breaking compilation of 32-bit code
>   as much as possible. Anything that does break however is likely code
>   that is already broken on 64-bit time_t and needs source changes to
>   fix them.
> 
> I would like to propose merging this into the alsa tree after
> the v5.5 merge window for inclusion into v5.6, to allow a good
> amount of testing, in particular for the header changes that
> may cause problems for user space applications.

Agreed, it's still no urgent problem.

> A git branch with the same contents is available for testing at [1].
> 
> Please review and test!
> 
>      Arnd

So now taking a quick look through the series, I find this approach is
the way to go.  Although one might get a bit more optimization after
squeeze, it's already a good compromise between the readability and
the efficiency.

A slight uncertain implementation is the timer tread stuff, especially
the conditional definition of SNDRV_TIMER_IOCTL_TREAD (IIRC, I already
complained it in the past, too).  But I have no other idea as well, so
unless someone else gives a better option, we can live with that.


Thanks!

Takashi

> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git y2038-alsa
> [2] https://lore.kernel.org/lkml/CAK8P3a2Os66+iwQYf97qh05W2JP8rmWao8zmKoHiXqVHvyYAJA@mail.gmail.com/T/#m6519cb07cfda08adf1dedea6596bb98892b4d5dc
> 
> Changes since v5 (Arnd):
>  - Rebased to linux-5.4-rc4
>  - Updated to completely remove timespec and time_t references from alsa
>  - found and fixed a few bugs
> 
> Changes since v4 (Baolin):
>  - Add patch 5 to change trigger_tstamp member of struct snd_pcm_runtime.
>  - Add patch 8 to change internal timespec.
>  - Add more explanation in commit message.
>  - Use ktime_get_real_ts64() in patch 6.
>  - Split common code out into a separate function in patch 6.
>  - Fix tu->tread bug in patch 6 and remove #if __BITS_PER_LONG == 64 macro.
> 
> Changes since v3:
>  - Move struct snd_pcm_status32 to pcm.h file.
>  - Modify comments and commit message.
>  - Add new patch2 ~ patch6.
> 
> Changes since v2:
>  - Renamed all structures to make clear.
>  - Remove CONFIG_X86_X32 macro and introduced new compat_snd_pcm_status64_x86_32.
> 
> Changes since v1:
>  - Add one macro for struct snd_pcm_status_32 which only active in 32bits kernel.
>  - Convert pcm_compat.c to use struct snd_pcm_status_64.
>  - Convert pcm_native.c to use struct snd_pcm_status_64.
> 
> ---
> 
> Arnd Bergmann (2):
>   ALSA: move snd_pcm_ioctl_sync_ptr_compat into pcm_native.c
>   ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
> 
> Baolin Wang (6):
>   ALSA: Replace timespec with timespec64
>   ALSA: Avoid using timespec for struct snd_timer_status
>   ALSA: Avoid using timespec for struct snd_ctl_elem_value
>   ALSA: Avoid using timespec for struct snd_pcm_status
>   ALSA: Avoid using timespec for struct snd_rawmidi_status
>   ALSA: Avoid using timespec for struct snd_timer_tread
> 
>  include/sound/pcm.h               |  74 ++++++--
>  include/sound/timer.h             |   4 +-
>  include/uapi/sound/asound.h       | 132 ++++++++++++--
>  sound/core/pcm.c                  |  12 +-
>  sound/core/pcm_compat.c           | 282 ++++++++----------------------
>  sound/core/pcm_lib.c              |  38 ++--
>  sound/core/pcm_native.c           | 226 +++++++++++++++++++++---
>  sound/core/rawmidi.c              | 132 +++++++++++---
>  sound/core/rawmidi_compat.c       |  87 +++------
>  sound/core/timer.c                | 229 ++++++++++++++++++------
>  sound/core/timer_compat.c         |  62 +------
>  sound/pci/hda/hda_controller.c    |  10 +-
>  sound/soc/intel/skylake/skl-pcm.c |   4 +-
>  13 files changed, 804 insertions(+), 488 deletions(-)
> 
> -- 
> 2.20.0
>
Arnd Bergmann Nov. 13, 2019, 2:32 p.m. UTC | #2
On Tue, Nov 12, 2019 at 9:37 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Tue, 12 Nov 2019 16:16:34 +0100, Arnd Bergmann wrote:
> > I would like to propose merging this into the alsa tree after
> > the v5.5 merge window for inclusion into v5.6, to allow a good
> > amount of testing, in particular for the header changes that
> > may cause problems for user space applications.
>
> Agreed, it's still no urgent problem.

I actually do think it's getting urgent, anything that touches
the ABI must be done carefully and not rushed.

The urgency at the moment is that developers are starting to
deploy systems with 64-bit time_t with musl-libc making this
the default now, and 32-bit risc-v not offering 32-bit time_t at all.

At the moment, this means that audio support is broken for
them, and that needs to be fixed.

The other reason why lots of people care about moving all user
space to 64-bit time_t is that 32-bit hardware is slowly starting
to become less common. We know there will still be many
32-bit ARM systems operational in 2038, but most of them will
be on (then) 10+ year old hardware, running even older software
that already being worked on today. The longer it takes us to
stop using 32-bit time_t in user space, the more systems will
end up having to be thrown away rather than fixed.

> So now taking a quick look through the series, I find this approach is
> the way to go.  Although one might get a bit more optimization after
> squeeze, it's already a good compromise between the readability and
> the efficiency.

Thanks!

> A slight uncertain implementation is the timer tread stuff, especially
> the conditional definition of SNDRV_TIMER_IOCTL_TREAD (IIRC, I already
> complained it in the past, too).  But I have no other idea as well, so
> unless someone else gives a better option, we can live with that.

We had discussed alternatives for this one last time, and decided
to go with the solution I posted here. The main alternative would
be to change the 'timespec' in snd_timer_tread to a fixed-length
structure based on two 'long' members. This would avoid the
need to match the command with the time_t type, but the cost would
be requiring CLOCK_MONOTONIC timestamps to avoid the
overflow, and changing all application source code that requires
the type to be compatible with 'timespec'.

     Arnd
Takashi Iwai Nov. 13, 2019, 4:40 p.m. UTC | #3
On Wed, 13 Nov 2019 15:32:44 +0100,
Arnd Bergmann wrote:
> 
> On Tue, Nov 12, 2019 at 9:37 PM Takashi Iwai <tiwai@suse.de> wrote:
> > On Tue, 12 Nov 2019 16:16:34 +0100, Arnd Bergmann wrote:
> > > I would like to propose merging this into the alsa tree after
> > > the v5.5 merge window for inclusion into v5.6, to allow a good
> > > amount of testing, in particular for the header changes that
> > > may cause problems for user space applications.
> >
> > Agreed, it's still no urgent problem.
> 
> I actually do think it's getting urgent, anything that touches
> the ABI must be done carefully and not rushed.
> 
> The urgency at the moment is that developers are starting to
> deploy systems with 64-bit time_t with musl-libc making this
> the default now, and 32-bit risc-v not offering 32-bit time_t at all.
> 
> At the moment, this means that audio support is broken for
> them, and that needs to be fixed.
> 
> The other reason why lots of people care about moving all user
> space to 64-bit time_t is that 32-bit hardware is slowly starting
> to become less common. We know there will still be many
> 32-bit ARM systems operational in 2038, but most of them will
> be on (then) 10+ year old hardware, running even older software
> that already being worked on today. The longer it takes us to
> stop using 32-bit time_t in user space, the more systems will
> end up having to be thrown away rather than fixed.

Don't worry, I planned merging the whole changes for 5.6.

> > So now taking a quick look through the series, I find this approach is
> > the way to go.  Although one might get a bit more optimization after
> > squeeze, it's already a good compromise between the readability and
> > the efficiency.
> 
> Thanks!
> 
> > A slight uncertain implementation is the timer tread stuff, especially
> > the conditional definition of SNDRV_TIMER_IOCTL_TREAD (IIRC, I already
> > complained it in the past, too).  But I have no other idea as well, so
> > unless someone else gives a better option, we can live with that.
> 
> We had discussed alternatives for this one last time, and decided
> to go with the solution I posted here. The main alternative would
> be to change the 'timespec' in snd_timer_tread to a fixed-length
> structure based on two 'long' members. This would avoid the
> need to match the command with the time_t type, but the cost would
> be requiring CLOCK_MONOTONIC timestamps to avoid the
> overflow, and changing all application source code that requires
> the type to be compatible with 'timespec'.

Fair enough.

One thing I forgot to mention: when we add/modify the ioctl or ABI, we
need to increment the protocol version, e.g. SNDRV_PCM_VERSION to
indicate user-space the supported ABI.  Please change these in your
next patches, too.


thanks,

Takashi
Arnd Bergmann Nov. 13, 2019, 4:51 p.m. UTC | #4
On Wed, Nov 13, 2019 at 5:40 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 13 Nov 2019 15:32:44 +0100, Arnd Bergmann wrote:

> > We had discussed alternatives for this one last time, and decided
> > to go with the solution I posted here. The main alternative would
> > be to change the 'timespec' in snd_timer_tread to a fixed-length
> > structure based on two 'long' members. This would avoid the
> > need to match the command with the time_t type, but the cost would
> > be requiring CLOCK_MONOTONIC timestamps to avoid the
> > overflow, and changing all application source code that requires
> > the type to be compatible with 'timespec'.
>
> Fair enough.
>
> One thing I forgot to mention: when we add/modify the ioctl or ABI, we
> need to increment the protocol version, e.g. SNDRV_PCM_VERSION to
> indicate user-space the supported ABI.  Please change these in your
> next patches, too.

Just to confirm: this should be a simple one-line patch at the end of the
series like

diff --git a/tools/include/uapi/sound/asound.h
b/tools/include/uapi/sound/asound.h
index df1153cea0b7..72e8380c6dcd 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -154,7 +154,7 @@ struct snd_hwdep_dsp_image {
  *                                                                           *
  *****************************************************************************/

-#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 14)
+#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 15)

 typedef unsigned long snd_pcm_uframes_t;
 typedef signed long snd_pcm_sframes_t;

right? Most other kernel interfaces have no version numbering, so
I don't know what policy you have here.

        Arnd
Takashi Iwai Nov. 13, 2019, 5:04 p.m. UTC | #5
On Wed, 13 Nov 2019 17:51:57 +0100,
Arnd Bergmann wrote:
> 
> On Wed, Nov 13, 2019 at 5:40 PM Takashi Iwai <tiwai@suse.de> wrote:
> > On Wed, 13 Nov 2019 15:32:44 +0100, Arnd Bergmann wrote:
> 
> > > We had discussed alternatives for this one last time, and decided
> > > to go with the solution I posted here. The main alternative would
> > > be to change the 'timespec' in snd_timer_tread to a fixed-length
> > > structure based on two 'long' members. This would avoid the
> > > need to match the command with the time_t type, but the cost would
> > > be requiring CLOCK_MONOTONIC timestamps to avoid the
> > > overflow, and changing all application source code that requires
> > > the type to be compatible with 'timespec'.
> >
> > Fair enough.
> >
> > One thing I forgot to mention: when we add/modify the ioctl or ABI, we
> > need to increment the protocol version, e.g. SNDRV_PCM_VERSION to
> > indicate user-space the supported ABI.  Please change these in your
> > next patches, too.
> 
> Just to confirm: this should be a simple one-line patch at the end of the
> series like
> 
> diff --git a/tools/include/uapi/sound/asound.h
> b/tools/include/uapi/sound/asound.h
> index df1153cea0b7..72e8380c6dcd 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -154,7 +154,7 @@ struct snd_hwdep_dsp_image {
>   *                                                                           *
>   *****************************************************************************/
> 
> -#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 14)
> +#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 15)
> 
>  typedef unsigned long snd_pcm_uframes_t;
>  typedef signed long snd_pcm_sframes_t;
> 
> right? Most other kernel interfaces have no version numbering, so
> I don't know what policy you have here.

I don't mind much about that, so it's up to you -- we can fold this
change into the patch that actually adds or modifies the ioctl, too.


thanks,

Takashi
Arnd Bergmann Nov. 13, 2019, 5:19 p.m. UTC | #6
On Wed, Nov 13, 2019 at 6:04 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Wed, 13 Nov 2019 17:51:57 +0100,
> Arnd Bergmann wrote:
> >
> > On Wed, Nov 13, 2019 at 5:40 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > On Wed, 13 Nov 2019 15:32:44 +0100, Arnd Bergmann wrote:
> >
> > > > We had discussed alternatives for this one last time, and decided
> > > > to go with the solution I posted here. The main alternative would
> > > > be to change the 'timespec' in snd_timer_tread to a fixed-length
> > > > structure based on two 'long' members. This would avoid the
> > > > need to match the command with the time_t type, but the cost would
> > > > be requiring CLOCK_MONOTONIC timestamps to avoid the
> > > > overflow, and changing all application source code that requires
> > > > the type to be compatible with 'timespec'.
> > >
> > > Fair enough.
> > >
> > > One thing I forgot to mention: when we add/modify the ioctl or ABI, we
> > > need to increment the protocol version, e.g. SNDRV_PCM_VERSION to
> > > indicate user-space the supported ABI.  Please change these in your
> > > next patches, too.
> >
> > Just to confirm: this should be a simple one-line patch at the end of the
> > series like
> >
> > diff --git a/tools/include/uapi/sound/asound.h
> > b/tools/include/uapi/sound/asound.h
> > index df1153cea0b7..72e8380c6dcd 100644
> > --- a/include/uapi/sound/asound.h
> > +++ b/include/uapi/sound/asound.h
> > @@ -154,7 +154,7 @@ struct snd_hwdep_dsp_image {
> >   *                                                                           *
> >   *****************************************************************************/
> >
> > -#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 14)
> > +#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 15)
> >
> >  typedef unsigned long snd_pcm_uframes_t;
> >  typedef signed long snd_pcm_sframes_t;
> >
> > right? Most other kernel interfaces have no version numbering, so
> > I don't know what policy you have here.
>
> I don't mind much about that, so it's up to you -- we can fold this
> change into the patch that actually adds or modifies the ioctl, too.

I've added the patch below to the end of the series now, will repost
the series  after no more comments come in for this version.
Having a single patch to update the version seems better than updating
it multiple times with each patch touching the ABI in this series.

      Arnd

commit b83a3eaece9b445f897a6f5ac2a903f2566a0e9d
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Wed Nov 13 17:49:14 2019 +0100

    ALSA: bump uapi version number

    Change SNDRV_PCM_VERSION to indicate the addition of the time64
    version of the mmap interface and these ioctl commands:

    SNDRV_PCM_IOCTL_SYNC
    SNDRV_RAWMIDI_IOCTL_STATUS
    SNDRV_PCM_IOCTL_STATUS
    SNDRV_PCM_IOCTL_STATUS_EXT
    SNDRV_TIMER_IOCTL_TREAD
    SNDRV_TIMER_IOCTL_STATUS

    32-bit applications built with 64-bit time_t require both the headers
    and the running kernel to support at least API version 2.0.15. When
    built with earlier kernel headers, some of these may not work
    correctly, so applications are encouraged to fail compilation like

     #if SNDRV_PCM_VERSION < SNDRV_PROTOCOL_VERSION(2, 0, 15)
     extern int __fail_build_for_time_64[sizeof(long) - sizeof(time_t)];
     #endif

    or provide their own updated copy of the header file.

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

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 25dbf71fa667..5cddfe62c97a 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -156,7 +156,7 @@ struct snd_hwdep_dsp_image {
  *                                                                           *
  *****************************************************************************/

-#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 14)
+#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 15)

 typedef unsigned long snd_pcm_uframes_t;
 typedef signed long snd_pcm_sframes_t;
Takashi Iwai Nov. 13, 2019, 6:13 p.m. UTC | #7
On Wed, 13 Nov 2019 18:19:56 +0100,
Arnd Bergmann wrote:
> 
> On Wed, Nov 13, 2019 at 6:04 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Wed, 13 Nov 2019 17:51:57 +0100,
> > Arnd Bergmann wrote:
> > >
> > > On Wed, Nov 13, 2019 at 5:40 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > > On Wed, 13 Nov 2019 15:32:44 +0100, Arnd Bergmann wrote:
> > >
> > > > > We had discussed alternatives for this one last time, and decided
> > > > > to go with the solution I posted here. The main alternative would
> > > > > be to change the 'timespec' in snd_timer_tread to a fixed-length
> > > > > structure based on two 'long' members. This would avoid the
> > > > > need to match the command with the time_t type, but the cost would
> > > > > be requiring CLOCK_MONOTONIC timestamps to avoid the
> > > > > overflow, and changing all application source code that requires
> > > > > the type to be compatible with 'timespec'.
> > > >
> > > > Fair enough.
> > > >
> > > > One thing I forgot to mention: when we add/modify the ioctl or ABI, we
> > > > need to increment the protocol version, e.g. SNDRV_PCM_VERSION to
> > > > indicate user-space the supported ABI.  Please change these in your
> > > > next patches, too.
> > >
> > > Just to confirm: this should be a simple one-line patch at the end of the
> > > series like
> > >
> > > diff --git a/tools/include/uapi/sound/asound.h
> > > b/tools/include/uapi/sound/asound.h
> > > index df1153cea0b7..72e8380c6dcd 100644
> > > --- a/include/uapi/sound/asound.h
> > > +++ b/include/uapi/sound/asound.h
> > > @@ -154,7 +154,7 @@ struct snd_hwdep_dsp_image {
> > >   *                                                                           *
> > >   *****************************************************************************/
> > >
> > > -#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 14)
> > > +#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 15)
> > >
> > >  typedef unsigned long snd_pcm_uframes_t;
> > >  typedef signed long snd_pcm_sframes_t;
> > >
> > > right? Most other kernel interfaces have no version numbering, so
> > > I don't know what policy you have here.
> >
> > I don't mind much about that, so it's up to you -- we can fold this
> > change into the patch that actually adds or modifies the ioctl, too.
> 
> I've added the patch below to the end of the series now, will repost
> the series  after no more comments come in for this version.
> Having a single patch to update the version seems better than updating
> it multiple times with each patch touching the ABI in this series.
> 
>       Arnd
> 
> commit b83a3eaece9b445f897a6f5ac2a903f2566a0e9d
> Author: Arnd Bergmann <arnd@arndb.de>
> Date:   Wed Nov 13 17:49:14 2019 +0100
> 
>     ALSA: bump uapi version number
> 
>     Change SNDRV_PCM_VERSION to indicate the addition of the time64
>     version of the mmap interface and these ioctl commands:
> 
>     SNDRV_PCM_IOCTL_SYNC
>     SNDRV_RAWMIDI_IOCTL_STATUS
>     SNDRV_PCM_IOCTL_STATUS
>     SNDRV_PCM_IOCTL_STATUS_EXT
>     SNDRV_TIMER_IOCTL_TREAD
>     SNDRV_TIMER_IOCTL_STATUS
> 
>     32-bit applications built with 64-bit time_t require both the headers
>     and the running kernel to support at least API version 2.0.15. When
>     built with earlier kernel headers, some of these may not work
>     correctly, so applications are encouraged to fail compilation like
> 
>      #if SNDRV_PCM_VERSION < SNDRV_PROTOCOL_VERSION(2, 0, 15)
>      extern int __fail_build_for_time_64[sizeof(long) - sizeof(time_t)];
>      #endif
> 
>     or provide their own updated copy of the header file.
> 
>     Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 25dbf71fa667..5cddfe62c97a 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -156,7 +156,7 @@ struct snd_hwdep_dsp_image {
>   *                                                                           *
>   *****************************************************************************/
> 
> -#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 14)
> +#define SNDRV_PCM_VERSION              SNDRV_PROTOCOL_VERSION(2, 0, 15)
> 
>  typedef unsigned long snd_pcm_uframes_t;
>  typedef signed long snd_pcm_sframes_t;

I wasn't clear enough.  Each ALSA device API has a protocol version,
not only PCM, so we need updating SNDRV_RAWMIDI_VERSION and
SNDRV_TIMER_VERSION as well.


thanks,

Takashi
Arnd Bergmann Nov. 13, 2019, 8:48 p.m. UTC | #8
On Wed, Nov 13, 2019 at 7:13 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 13 Nov 2019 18:19:56 +0100, Arnd Bergmann wrote:
> > On Wed, Nov 13, 2019 at 6:04 PM Takashi Iwai <tiwai@suse.de> wrote:
> > > I don't mind much about that, so it's up to you -- we can fold this
> > > change into the patch that actually adds or modifies the ioctl, too.
> >
> > I've added the patch below to the end of the series now, will repost
> > the series  after no more comments come in for this version.
> > Having a single patch to update the version seems better than updating
> > it multiple times with each patch touching the ABI in this series.
>
> I wasn't clear enough.  Each ALSA device API has a protocol version,
> not only PCM, so we need updating SNDRV_RAWMIDI_VERSION and
> SNDRV_TIMER_VERSION as well.

Ok, I see. I've updated the patch to include all three, and adapted the
changelog text. There are still multiple changes for the PCM interface,
so this avoids having the version number change attached to half the
code changes.

     Arnd