by Arnd Bergmann

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] sound: core: Avoid using timespec for struct snd_pcm_sync_ptr

On Fri, Sep 22, 2017 at 8:47 AM, Baolin Wang <[email protected]> wrote:
> On 21 September 2017 at 20:50, Arnd Bergmann <[email protected]> wrote:
>> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <[email protected]> wrote:
>>> The struct snd_pcm_sync_ptr will use 'timespec' type variables to record
>>
>> This looks correct, but there is a subtlety here to note about x86-32
>> that we discussed in a previous (private) review. To recall my earlier
>> thoughts:
>>
>> Normal architectures insert 32 bit padding after 'suspended_state',
>> and 32-bit architectures (including x32) also after hw_ptr,
>> but x86-32 does not. You make that explicit in the compat code,
>> this version just relies on the compiler using identical padding
>> in user and kernel space. We could make that explicit using
>>
>> struct snd_pcm_mmap_status64 {
>> snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */
>> int pad1; /* Needed for 64 bit alignment */
>> snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */
>> #if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32)
>> int pad2;
>> #endif
>> struct { s64 tv_sec; s64 tv_nsec; } tstamp; /* Timestamp */
>> snd_pcm_state_t suspended_state; /* RO: suspended stream state */
>> #if !defined(CONFIG_X86_32)
>> int pad3;
>> #endif
>> struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp; /* from
>> sample counter or wall clock */
>> };
>
> I am sorry I did not get you here, why we do not need pad2 and pad3
> for x86_32?

This is again the x86-32 alignment quirk: the structure as defined
in the uapi header does not have padding, and the new s64 fields
have 32-bit alignment on x86, so the compiler does not add implicit
padding in user space.

On all other architectures, the fields do get padded implicitly
in user space, I'm just listing the padding explicitly.

> You missed ‘#if !defined(CONFIG_64BIT)“ at the second #if
> condition?

No, that was intentional:

snd_pcm_uframes_t is 'unsigned long', so on 64-bit architectures
we have no padding between two 64-bit values (hw_ptr and tstamp),
and on x86-32 we have no padding because both have 32-bit
alignment.

However, snd_pcm_state_t is 'int', which is always 32-bit wide,
so we do have padding on both 32-bit and 64-bit architectures
between syspended_state and audio_tstamp, with the exception
of x86-32.

Arnd

2017-09-22 09:15:45

On 22 September 2017 at 16:48, Arnd Bergmann <[email protected]> wrote:
> On Fri, Sep 22, 2017 at 8:47 AM, Baolin Wang <[email protected]> wrote:
>> On 21 September 2017 at 20:50, Arnd Bergmann <[email protected]> wrote:
>>> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <[email protected]> wrote:
>>>> The struct snd_pcm_sync_ptr will use 'timespec' type variables to record
>>>
>>> This looks correct, but there is a subtlety here to note about x86-32
>>> that we discussed in a previous (private) review. To recall my earlier
>>> thoughts:
>>>
>>> Normal architectures insert 32 bit padding after 'suspended_state',
>>> and 32-bit architectures (including x32) also after hw_ptr,
>>> but x86-32 does not. You make that explicit in the compat code,
>>> this version just relies on the compiler using identical padding
>>> in user and kernel space. We could make that explicit using
>>>
>>> struct snd_pcm_mmap_status64 {
>>> snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */
>>> int pad1; /* Needed for 64 bit alignment */
>>> snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */
>>> #if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32)
>>> int pad2;
>>> #endif
>>> struct { s64 tv_sec; s64 tv_nsec; } tstamp; /* Timestamp */
>>> snd_pcm_state_t suspended_state; /* RO: suspended stream state */
>>> #if !defined(CONFIG_X86_32)
>>> int pad3;
>>> #endif
>>> struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp; /* from
>>> sample counter or wall clock */
>>> };
>>
>> I am sorry I did not get you here, why we do not need pad2 and pad3
>> for x86_32?
>
> This is again the x86-32 alignment quirk: the structure as defined
> in the uapi header does not have padding, and the new s64 fields
> have 32-bit alignment on x86, so the compiler does not add implicit
> padding in user space.
>
> On all other architectures, the fields do get padded implicitly
> in user space, I'm just listing the padding explicitly.

Make sense.

>
>> You missed ‘#if !defined(CONFIG_64BIT)“ at the second #if
>> condition?
>
> No, that was intentional:
>
> snd_pcm_uframes_t is 'unsigned long', so on 64-bit architectures
> we have no padding between two 64-bit values (hw_ptr and tstamp),
> and on x86-32 we have no padding because both have 32-bit
> alignment.
>
> However, snd_pcm_state_t is 'int', which is always 32-bit wide,
> so we do have padding on both 32-bit and 64-bit architectures
> between syspended_state and audio_tstamp, with the exception
> of x86-32.

Thanks, I can understand now.

--
Baolin.wang
Best Regards