Message-ID: <s5h4lrvdixp.wl-tiwai@suse.de>
Date:   Fri, 22 Sep 2017 14:19:46 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Baolin Wang <baolin.wang@...aro.org>,
        Jaroslav Kysela <perex@...ex.cz>, alsa-devel@...a-project.org,
        Arvind Yadav <arvind.yadav.cs@...il.com>,
        Bhumika Goyal <bhumirks@...il.com>,
        Deepa Dinamani <deepa.kernel@...il.com>,
        Liam Girdwood <lgirdwood@...il.com>, dharageswari.r@...el.com,
        gudishax.kranthikumar@...el.com, guneshwor.o.singh@...el.com,
        hardik.t.shah@...el.com, jeeja.kp@...el.com,
        Naveen M <naveen.m@...el.com>,
        Vinod Koul <vinod.koul@...el.com>,
        Mark Brown <broonie@...nel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Takashi Sakamoto <o-takashi@...amocchi.jp>,
        Fabian Frederick <fabf@...net.be>,
        SF Markus Elfring <elfring@...rs.sourceforge.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/7] sound: core: Avoid using timespec for struct snd_pcm_status

On Fri, 22 Sep 2017 13:43:16 +0200,
Arnd Bergmann wrote:
> 
> On Fri, Sep 22, 2017 at 12:49 PM, Takashi Iwai <tiwai@...e.de> wrote:
> > On Fri, 22 Sep 2017 12:14:55 +0200,
> > Arnd Bergmann wrote:
> >> The kernel uses timespec64 internally, which is defined as
> >> "{ s64 tv_sec; long tv_nsec };", so this has the padding
> >> in a different place on big-endian architectures, and has a
> >> different alignment and size on i386. We plan to introduce
> >> a 'struct __kernel_timespec' that is compatible with the
> >> __64_BIT_TIME_T version of the user timespec, but that
> >> doesn't exist yet.
> >>
> >> If you prefer, we can probably introduce it now with Baolin's
> >> series, I think Deepa was planning to post a patch to add
> >> it soon anyway.
> >
> > Yes, this sounds like a saner solution than defining the own timespec
> > at each place individually.  Then we can have better conversion
> > macros, too, I suppose.
> 
> Thinking about it again, we unfortunately can't use
> __kernel_timespec until after all 32-bit architectures have
> been converted to use the new syscalls that we still need
> to introduce: In the meantime the plan is that '__kernel_timespec'
> is an alias for the usual 'timespec' in user space and may still
> be 32-bit wide.

OK.

> I definitely agree that open-coding 'struct { s64 tv_sec;
> s64 tv_nsec}' in a dozen locations is not overly helpful.
> 
> I suggested a different alternative in my reply to patch 3/7.
> Can you have a look at that? The idea would be that we just
> flatten all the structures in the ioctl implementation and make
> the structure definition very explicit using u32/s32/u64/s64
> members with no implied padding or architecture-specific
> types.

Thanks, I took a look at it, and I agree with you about the flatten
struct being easier to handle.


Though, one thing is still considered: snd_pcm_mmap_status struct is
not only passed via ioctl but it's embedded in a mmapped context.
So, differentiation by the struct size can't work with it (as already
you pointed about union, too).

It might sound like a contradiction what I wrote, but another possible
way would be to define 64bit timespec for the sound (both 64bit sec
and nsec), and use it consistently in all places no matter which
architecture is used.  It'd need the changes in alsa-lib or
tiny-alsa, of course.  But that's one of the reasons of the presence
of alsa-lib layer after all.

Recently we introduced the ioctl for user-space to inform the
supported ABI version (SNDRV_PCM_IOCTL_USER_PVERSION).  With this, we
can limit the 64bit timespec support only for the programs that issue
this ioctl with the new protocol version.  If not set or older ABI
version is given, it means still old 32bit timespec.  This can be used
for distinguish which mmapped struct is referred, too.

> > And, if we have kernel_timespec (or kernel_timespec64 or such), can
> > this deprecate the existing timespec64 usages, too?  I see that
> > timespec64 is internal only, so consolidation would be beneficial for
> > code simplification.
> 
> Our current longterm plan is to only use __kernel_timespec on the
> ABI side, where we have to watch out for the tricky conversion of
> tv_nsec: Any timespec copied from a 32-bit process into the kernel
> must ignore the upper half of the nanoseconds, while copying the
> same structure from a 64-bit process must return an error if the
> 64-bit nanoseconds are larger than 999999999. When copying a
> timespec into user space, we have to be careful to zero the upper
> half of tv_nsec to avoid leaking uninitialized kernel data.

Right.

> Inside of the kernel, we can ignore those constraints, so I'd keep
> using the timespec64. We certainly don't want to use the 64-bit
> nanoseconds field for internal uses on 32-bit kernels, as that
> would introduce expensive 64-bit arithmetic in a lot of places
> that don't need it.

OK, that's the reason of timespec64.  But this can be eventually
identical with __kernel_timespec?  It holds 64bit sec and 32bit nsec,
right?  The difference is the presence of padding, but having a pad
makes things arch-agnostic, so its cost looks paying well, I guess.

> My hope is also that we can eventually deprecate any use of the
> plain 'timespec' in the kernel: all internal users should migrate
> to timespec64 (one at a time, so we can properly review the
> changes), and the uapi uses should either have the 64-bit
> version of __kernel_timespec, or use compat_timespec once
> that becomes usable on 32-bit architectures.

Heh, let's keep our hope :)


thanks,

Takashi

Powered by blists - more mailing lists