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

[Baolin Wang]

On 22 September 2017 at 16:48, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Fri, Sep 22, 2017 at 8:47 AM, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
>> On 21 September 2017 at 20:50, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>>> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@xxxxxxxxxx> 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