From patchwork Sun Nov 5 13:48:38 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arnd Bergmann X-Patchwork-Id: 10042265 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 6059060247 for ; Sun, 5 Nov 2017 14:44:29 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 50E1B289CE for ; Sun, 5 Nov 2017 14:44:29 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 424DB289E1; Sun, 5 Nov 2017 14:44:29 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.3 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,T_DKIM_INVALID autolearn=no version=3.3.1 Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4C0E8289CE for ; Sun, 5 Nov 2017 14:44:27 +0000 (UTC) Received: from alsa0.perex.cz (localhost [127.0.0.1]) by alsa0.perex.cz (Postfix) with ESMTP id C300826704E; Sun, 5 Nov 2017 14:48:48 +0100 (CET) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id 13E4826705A; Sun, 5 Nov 2017 14:48:47 +0100 (CET) Received: from mail-ot0-f196.google.com (mail-ot0-f196.google.com [74.125.82.196]) by alsa0.perex.cz (Postfix) with ESMTP id E4F3E266A0C for ; Sun, 5 Nov 2017 14:48:40 +0100 (CET) Received: by mail-ot0-f196.google.com with SMTP id c47so6271133otj.6 for ; Sun, 05 Nov 2017 05:48:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=l494GcmsNLPn26n0SH5MMtZcI5ACqW5982fP1Qos030=; b=Uw2rdjqoE3Fk8lCDRMQmF0ZWwqAMf5qmc8nKwLh9YJlZmPO2r8Cos3D8oCQe/dzlVf JvYBRy2uYTZnqPPi9pEGm82lY4eP3pvD5nE6BQ1IdZ2K2tS5IRUpiRGu6EqJTHWpfftd +ehuLh/dtcQThKipKtM8C+ZW0PKDSV5/wQaNcbNiTAuV9iNjVdCe8V8OF6Ft3/hRiAG1 5JSQ9Sob75uNUXT3DSCvc76uDzrRh88wNXAodqXwT8H9hIJJVNGhaGHV+DKGXSw9oxai lV63iMc5BOONiZx1TOwxLoMwK73mbxedKyub63qIpBaas4cFGGTkWVWRttrfFYYEg+II COwA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=l494GcmsNLPn26n0SH5MMtZcI5ACqW5982fP1Qos030=; b=EtcrwaKioD/yk4c56nOFWKnHcqYmayQa6jn9nYS6zve37B+82gMvaKXyzgYiq8Zfvt PCpjJzDAUDHxK/om7avxVTmc0g+Qwgx9YC3TcSoD45LSBgT6ONsmwF5C+LLz6FkKAUJW m6YRo2tntt8hgPPDSyss71L2WxW9vvUDGTs2EkJuQX3eNJkXJLqjxYV6X6D9/fL/tjbS I+LchOSiyTw0IoUQpSLfqtWoBu6QiLjXLNxsLFJxAEOkKMY0cJF+Yr2djKdBWq/U7+Sz +P64D7hrdIoma7leomm5hxmyzyyBt7YFdYaNC/V3bq32BPu88N2gwtBQahTJPdD2zCbf TNJA== X-Gm-Message-State: AJaThX55t2/buiY7DfQcq8XkBnmksUcj9g3fKZSnEm39IJE2KBFrmeBD wq4NtdGcIyiCYA+523REhWmglSe3evhh2qJOMwI= X-Google-Smtp-Source: ABhQp+QBh874CDaer/OVPTFp5pj2shlLqYpuYGkQKfZd6L8aV+zCFSMXCEmbdlsbYZ/iv/ZFs8WA17ZXiBpkIVWnarI= X-Received: by 10.157.9.42 with SMTP id 39mr1662527otp.276.1509889719476; Sun, 05 Nov 2017 05:48:39 -0800 (PST) MIME-Version: 1.0 Received: by 10.157.28.152 with HTTP; Sun, 5 Nov 2017 05:48:38 -0800 (PST) In-Reply-To: References: From: Arnd Bergmann Date: Sun, 5 Nov 2017 14:48:38 +0100 X-Google-Sender-Auth: xjPc7v1fdzaJ75NoW-o2MiVfieM Message-ID: To: Takashi Iwai Cc: jeeja.kp@intel.com, gudishax.kranthikumar@intel.com, alsa-devel@alsa-project.org, Liam Girdwood , Baolin Wang , Vinod Koul , hardik.t.shah@intel.com, Ingo Molnar , guneshwor.o.singh@intel.com, Linux Kernel Mailing List , Fabian Frederick , Mark Brown , Dan Carpenter , Naveen M , Arvind Yadav , Takashi Sakamoto , subhransu.s.prusty@intel.com, SF Markus Elfring , Bhumika Goyal Subject: Re: [alsa-devel] [RFC PATCH v2 2/7] sound: core: Avoid using timespec for struct snd_pcm_status X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org X-Virus-Scanned: ClamAV using ClamSMTP On Sun, Nov 5, 2017 at 11:23 AM, Takashi Iwai wrote: > On Thu, 02 Nov 2017 12:06:52 +0100, > Baolin Wang wrote: >> >> The struct snd_pcm_status will use 'timespec' type variables to record >> timestamp, which is not year 2038 safe on 32bits system. >> >> Userspace will use SNDRV_PCM_IOCTL_STATUS and SNDRV_PCM_IOCTL_STATUS_EXT >> as commands to issue ioctl() to fill the 'snd_pcm_status' structure in >> userspace. The command number is always defined through _IOR/_IOW/IORW, >> so when userspace changes the definition of 'struct timespec' to use >> 64-bit types, the command number also changes. >> >> Thus in the kernel, we now need to define two versions of each such ioctl >> and corresponding ioctl commands to handle 32bit time_t and 64bit time_t >> in native mode: >> struct snd_pcm_status32 { >> ...... >> struct { s32 tv_sec; s32 tv_nsec; } trigger_tstamp; >> struct { s32 tv_sec; s32 tv_nsec; } tstamp; >> ...... >> } >> >> struct snd_pcm_status64 { >> ...... >> struct { s64 tv_sec; s64 tv_nsec; } trigger_tstamp; >> struct { s64 tv_sec; s64 tv_nsec; } tstamp; >> ...... >> } >> >> Moreover in compat file, we renamed or introduced new structures to handle >> 32bit/64bit time_t in compatible mode. 'struct compat_snd_pcm_status32' and >> snd_pcm_status_user_compat() are used to handle 32bit time_t in compat mode. >> 'struct compat_snd_pcm_status64' and snd_pcm_status_user_compat64() are used >> to handle 64bit time_t with 64bit alignment. 'struct compat_snd_pcm_status64_x86_32' >> and snd_pcm_status_user_compat64_x86_32() are used to handle 64bit time_t with >> 32bit alignment. > > Hmm... I don't get why you need to redefine these in > include/sound/pcm.h and define even IOCTLs there. These are the > things for ABI, no? If yes, we don't need to have it globally in the > public header but define/convert them locally. The problem is that the ABI is currently defined in terms of a mix of data structures from kernel and glibc. We have to be very careful about changing the public header files to avoid breaking applications that were built against new headers but run on old kernels, so we can't just make the time_t fields 64-bit wide in the header and change the ioctl number for everyone. The new structure definitions in the internal are representations of the possible binary layouts that user space will actually see with the possible combinations of 32-bit and 64-bit toolchains, the x86-32 quirk (64-bit variables being misaligned), the x32 hack (makeing __kernel_long_t 64-bit) and new glibc using 64-bit time_t. > And, renaming snd_pcm_status64 allover the places for internal doesn't look good. Would you prefer adding another distinct type for the kernel-internal structure? That would probably be cleaner overall, but also complicate the one case in which the user-space representation is actually the same as the one in the kernel. Note that we can't use snd_pcm_status here, because that is based on 'time_t', which in the kernel has to remain defined as 'long', so we'd still suffer from the overflow. One more thing: as we discussed in Prague, I think the additional compat_snd_pcm_status64_x86_32 structure can be avoided if we make slight changes to the user-visible definition of snd_pcm_status that only take effect on x86-32 with a modified libc, i.e. snd_pcm_uframes_t appl_ptr; /* appl ptr */ With that change, some of the other changes should get simpler too. Arnd diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 299a822d2c4e..40be757de124 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -447,6 +447,7 @@ enum { struct snd_pcm_status { snd_pcm_state_t state; /* stream state */ + __u8 __pad0[sizeof(time_t) - sizeof(__kernel_long_t)]; struct timespec trigger_tstamp; /* time when stream was started/stopped/paused */ struct timespec tstamp; /* reference timestamp */