Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751972AbdIVIsw (ORCPT ); Fri, 22 Sep 2017 04:48:52 -0400 Received: from mail-io0-f195.google.com ([209.85.223.195]:38170 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751833AbdIVIsv (ORCPT ); Fri, 22 Sep 2017 04:48:51 -0400 X-Google-Smtp-Source: AOwi7QBaOL2aUOev4hrHhld8HqjHQtM7ePlSfpnNGRRAjIftbGa2gqD5IQrxWfDtiTLOv/aJlj4ZAXHEmrTBekLnyRQ= MIME-Version: 1.0 In-Reply-To: References: <6781c7b4e5934ad65e3c5b401c0a1bbd7cb44db6.1505973912.git.baolin.wang@linaro.org> From: Arnd Bergmann Date: Fri, 22 Sep 2017 10:48:50 +0200 X-Google-Sender-Auth: OH_D4ds2hf02X0X3tAPSQ4BH8Ko Message-ID: Subject: Re: [RFC PATCH 3/7] sound: core: Avoid using timespec for struct snd_pcm_sync_ptr To: Baolin Wang Cc: Jaroslav Kysela , Takashi Iwai , Liam Girdwood , Ingo Molnar , Takashi Sakamoto , SF Markus Elfring , Dan Carpenter , jeeja.kp@intel.com, Vinod Koul , dharageswari.r@intel.com, guneshwor.o.singh@intel.com, Bhumika Goyal , gudishax.kranthikumar@intel.com, Naveen M , hardik.t.shah@intel.com, Arvind Yadav , Fabian Frederick , Mark Brown , Deepa Dinamani , alsa-devel@alsa-project.org, Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v8M8muD5022324 Content-Length: 2439 Lines: 58 On Fri, Sep 22, 2017 at 8:47 AM, Baolin Wang wrote: > On 21 September 2017 at 20:50, Arnd Bergmann wrote: >> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang 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