From patchwork Mon Nov 6 16:33:26 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arnd Bergmann X-Patchwork-Id: 10043845 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 9D053603FF for ; Mon, 6 Nov 2017 16:33:37 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9691629E86 for ; Mon, 6 Nov 2017 16:33:37 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8AF0329EBA; Mon, 6 Nov 2017 16:33:37 +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 9B49329E86 for ; Mon, 6 Nov 2017 16:33:36 +0000 (UTC) Received: from alsa0.perex.cz (localhost [127.0.0.1]) by alsa0.perex.cz (Postfix) with ESMTP id 13AD4267258; Mon, 6 Nov 2017 17:33:35 +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 C62D726725C; Mon, 6 Nov 2017 17:33:33 +0100 (CET) Received: from mail-ot0-f195.google.com (mail-ot0-f195.google.com [74.125.82.195]) by alsa0.perex.cz (Postfix) with ESMTP id 03235267230 for ; Mon, 6 Nov 2017 17:33:28 +0100 (CET) Received: by mail-ot0-f195.google.com with SMTP id 15so9490542otj.7 for ; Mon, 06 Nov 2017 08:33:28 -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=Cq+KKTHqVlvMGhTbHyozHs+IyLPMpYJSXEhfTTDEYRA=; b=CKE4nQTVLUkmVtOHyujHclfn9ROtod0xoY1ZVVlHNKyEJ2VWGGHGff4ppNY9FJy0kK BAHDLFJgaUkJr2aZfcV5ouTwrKH2KBixLsBVVsYB2XptWjxeG1lfSY5XqH9qSVID2ZBt sallZe/TGjhsbA4xAsEpd5d/QyeC0WJMbkuqGqkk9cwg5eOkrcNC4YP81G1cdsiGFdjU gw5XGHdO+2PbfXljXZRSYSA9Z6O2hnoGMjVrMDop5Li9IUDDmykok2YeuJpTxe78k4hL iqOPAcZYMv+BEd8BH8s+fkrnUwJAh192+wXmK3C3HAO9Ny5QLr70GDNKJBg2JvAj/mxh lyzw== 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=Cq+KKTHqVlvMGhTbHyozHs+IyLPMpYJSXEhfTTDEYRA=; b=ezBOdckClVHiVMhyWwXNlQfkUHWfgXwkWi4tK90z/aUN0fjZilNLQJQAihg/Lc2K+e RgaqIFJ80aYNUYonqPYTXy+tAsYLT2bomUcGLr20uin+hB3RA25UPyi+YFv9SFLif+H8 NFsqf3wVYi8n/3HAh49fFt1Fhw6Hg8tEU9/VWV2xdJ+Q6vhBJZD8qKwSDLMd7mKM0t7g O8UBIno6rpU33EKVc0Jso5O0+K0v3XE3Nz7lj3kwlybxS0NhGbSc4xBM8u3BlfcP58l2 UwoXxL6DRYxTQOUWza/ue9AA6CO55duPFuFwPBD5eXJc+PrV1oGyL/knuyo78JVTJEDR 6EWw== X-Gm-Message-State: AJaThX7u563qCKArIEPqCWTGwE4x2zRJG8a6XCPGXkVFuRy7TLSqibFy R3L1i5JooYlT7EQPr0jz0U+BqgrHCc16DG/jm/s= X-Google-Smtp-Source: ABhQp+S3YQ9apJSuL8DvDJE3y/Tr/NwuXakBNLt7EyIwL8NNpyI3im6Jehn8Yjxnod5YwoGyNf/6gQKIjVQbTL+q4Zs= X-Received: by 10.157.88.26 with SMTP id r26mr1291667oth.88.1509986007151; Mon, 06 Nov 2017 08:33:27 -0800 (PST) MIME-Version: 1.0 Received: by 10.157.28.152 with HTTP; Mon, 6 Nov 2017 08:33:26 -0800 (PST) In-Reply-To: References: From: Arnd Bergmann Date: Mon, 6 Nov 2017 17:33:26 +0100 X-Google-Sender-Auth: PglgiPWivbTFAmMgyAa5ZLXxEhc 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 7/7] sound: core: Avoid using timespec for struct snd_timer_tread 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 5:59 PM, Takashi Iwai wrote: > On Sun, 05 Nov 2017 14:16:28 +0100, >> On Sun, Nov 5, 2017 at 11:29 AM, Takashi Iwai wrote: >> > On Thu, 02 Nov 2017 12:06:57 +0100, >> > We should introduce SNDRV_TIMER_IOCTL_USER_PVERSION instead, where >> > user-space can tell which protocol version it understands. If the >> > protocol version is higher than some definition, we can assume it's >> > 64-bit ready. The *_USER_PVERSION is issued from alsa-lib side. >> > In that way, we can extend the ABI more flexibly. A similar trick was >> > already used in PCM ABI. (Ditto for control and rawmidi API; we >> > should have the same mechanism for all relevant APIs). >> > >> > Moreover, once when the new protocol is used, we can use the standard >> > 64bit monotonic nsecs as a timestamp, so that we don't need to care >> > about 32/64bit compatibility. >> >> I think that's fine, we can do that too, but I don't see how we get around >> to doing something like Baolin's patch first. Without this, we will get >> existing user space code compiling against our kernel headers using a >> new glibc release that will inadvertently change the structure layout >> on the read file descriptor. > > But it won't work in anyway in multiple ways, e.g. this timer read > stuff and another the structs embedded in the mmappged page. If you > do rebuild things with new glibc, it should tell kernel about the new > ABI in anyway more or less explicitly. And if you need it, it means > that some source-code level API change would be possible. Right, you mentioned the mmap interface at the kernel summit. This is certainly the most tricky part and will probably require source-level changes. Can you clarify a few things about the mmap() interface? Is this specifically about "struct snd_pcm_mmap_status" on the pcm device, or are there others? From what I can see, it's already fairly limited: - on most architectures, it's completely disabled, only x86, ppc and alpha allow it to start with, and user space can work around the mmap not being available by falling back to ioctl if I read the comments correctly - alpha is not affected since time_t is always 64-bit - x86 and ppc disable the mmap() in compat mode already because of the same issue. If it comes to the worst, we can probably do the same for x86-32 and ppc32, disabling the existing status mmap for them as well, and change SNDRV_PCM_MMAP_OFFSET_STATUS to a new value for 32-bit kernels that exposes the same structure as 64-bit kernels. - I think that since we always use an offset that is defined in the header file, we can use the same trick for mmap that we have for the ioctl command number: unsigned short id16[8]; Does that make sense? > Of course, passing which data type is another question. Maybe 64bit > nsecs wouldn't fit all places, and timespec64 style would be still > required. But still, the current patch looks still too unnecessarily > complex to me. (Yeah I know that the problem is complex, but the code > can be simpler, I hope!) I think we can simplify the x86_32 case, but probably not much beyond that. The trick above however can fix 32-bit compat mode for mmap if we want to do that. >> The trick with redefining SNDRV_TIMER_IOCTL_TREAD in that >> configuration lets the kernel know what API the user space expects >> without requiring source-level changes. > > Right, it works for this case, but not always. > If jumping the API would give a cleaner way of implementation, I'd > prefer that over too hackeries, IMO. Generally speaking I try to avoid being incompatible since that causes more problems for users when they either fail to build existing source code, or get silent interface breakage after recompiling against a new glibc. If the kernel can make it work, that should be the first priority. >> If you want to for all users of SNDRV_TIMER_IOCTL_TREAD to move >> to a new interface for y2038-safety, we'd have to redefined the structure >> to avoid the libc-provided 'struct timespec' on 32-bit architectures, like: >> >> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h >> index 299a822d2c4e..f93cace4cd24 100644 >> --- a/include/uapi/sound/asound.h >> +++ b/include/uapi/sound/asound.h >> @@ -801,7 +801,14 @@ enum { >> >> struct snd_timer_tread { >> int event; >> +#if __BITS_PER_LONG == 32 >> + struct { >> + __kernel_long_t tv_sec; >> + __kernel_long_t tv_usec; >> + }; >> +#else >> struct timespec tstamp; >> +#endif >> unsigned int val; >> }; >> >> This has a somewhat higher risk of breaking existing code (since the type >> changes), and it doesn't solve the overflow. > > Hm, how to define the timestamp type is one of the biggest questions > indeed. In general, there can't be any guarantee that just rebuilding > with the 64bit timespec would work for all existing codes. In theory > it shouldn't break, but who knows... Right we can capture most cases then user space gets the kernel headers from /usr/include/linux and that gets created from a new enough kernel, or when the ioctl command number is defined in terms of the variable structure sizes that actually differ. This covers almost all interfaces, but I've seen some exceptions that will be silently broken no matter what we do. For all I can see, ALSA ioctls are fine, it's just a lot of work to get right. The mmap() problem might be fairly easy to solve in the end, or it may be very hard. Arnd diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index c227ccba60ae..bcdbdac097d9 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -306,10 +306,19 @@ typedef int __bitwise snd_pcm_state_t; enum { SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000, - SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000, + SNDRV_PCM_MMAP_OFFSET_STATUS_OLD = 0x80000000, SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000, + SNDRV_PCM_MMAP_OFFSET_STATUS64 = 0x82000000, }; +#if __BITS_PER_LONG == 64 +#define SNDRV_PCM_MMAP_OFFSET_STATUS SNDRV_PCM_MMAP_OFFSET_STATUS_OLD +#else +#define SNDRV_PCM_MMAP_OFFSET_STATUS ((sizeof(time_t) > sizeof(__kernel_long_t)) ? \ + SNDRV_PCM_MMAP_OFFSET_STATUS64 : \ + SNDRV_PCM_MMAP_OFFSET_STATUS_OLD) +#endif + union snd_pcm_sync_id { unsigned char id[16];