Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1032583AbdIZWYc (ORCPT ); Tue, 26 Sep 2017 18:24:32 -0400 Received: from mail-oi0-f51.google.com ([209.85.218.51]:52567 "EHLO mail-oi0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030416AbdIZWYW (ORCPT ); Tue, 26 Sep 2017 18:24:22 -0400 X-Google-Smtp-Source: AOwi7QA/3RgfsZj+kJmKAhabqoL2Hx6a3uXfVRlFes+FwSEYMN4d6NFTOX2QC3TW6mzQ5egkBbbsfOpQ+rrZXk3EDVQ= MIME-Version: 1.0 In-Reply-To: References: <6781c7b4e5934ad65e3c5b401c0a1bbd7cb44db6.1505973912.git.baolin.wang@linaro.org> From: Baolin Wang Date: Wed, 27 Sep 2017 06:24:21 +0800 Message-ID: Subject: Re: [RFC PATCH 3/7] sound: core: Avoid using timespec for struct snd_pcm_sync_ptr To: Arnd Bergmann 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 v8QMOeJL021510 Content-Length: 2642 Lines: 66 On 22 September 2017 at 16:48, Arnd Bergmann wrote: > 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. 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