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