Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753176AbdLHMOR (ORCPT ); Fri, 8 Dec 2017 07:14:17 -0500 Received: from foss.arm.com ([217.140.101.70]:37854 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752954AbdLHMOM (ORCPT ); Fri, 8 Dec 2017 07:14:12 -0500 Date: Fri, 8 Dec 2017 12:14:06 +0000 From: Mark Rutland To: Greentime Hu Cc: Greentime , Linux Kernel Mailing List , Arnd Bergmann , linux-arch , Thomas Gleixner , Jason Cooper , Marc Zyngier , Rob Herring , netdev , Vincent Chen , DTML , Al Viro , David Howells , Will Deacon , Daniel Lezcano , linux-serial@vger.kernel.org, Geert Uytterhoeven , Linus Walleij , Greg KH , Vincent Chen Subject: Re: [PATCH v3 17/33] nds32: VDSO support Message-ID: <20171208121405.dy6raczdlbxtbnfo@lakrids.cambridge.arm.com> References: <921ccfa97c4c13f12c7b22b9554f52dcce51f87e.1512723245.git.green.hu@gmail.com> <20171208102149.iqiieszktwzorkuw@lakrids.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3127 Lines: 81 On Fri, Dec 08, 2017 at 07:54:42PM +0800, Greentime Hu wrote: > 2017-12-08 18:21 GMT+08:00 Mark Rutland : > > On Fri, Dec 08, 2017 at 05:12:00PM +0800, Greentime Hu wrote: > >> +static int grab_timer_node_info(void) > >> +{ > >> + struct device_node *timer_node; > >> + > >> + timer_node = of_find_node_by_name(NULL, "timer"); > > > > Please use a compatible string, rather than matching the timer by name. > > > > It's plausible that you have multiple nodes called "timer" in the DT, > > under different parent nodes, and this might not be the device you > > think it is. I see your dt in patch 24 has two timer nodes. > > > > It would be best if your clocksource driver exposed some stuct that you > > looked at here, so that you're guaranteed to user the same device. > > We'd like to use "timer" here because there are 2 different timer IPs > and we are sure that they won't be in the same SoC. > We think this implementation in VDSO should be platform independent to > get cycle-count register. > Our customer or other SoC provider who can use "timer" and define > cycle-count-offset or cycle-count-down then we can get the correct > cycle-count. This is not the right way to do things. So from a DT perspective, NAK. You should not add properties to arbitrary DT bindings to handle a Linux implementation detail. Please remove this DT code, and have the drivers for those timer blocks export this information to your vdso code somehow. > We sent atcpit100 patch last time along with our arch, however we'd > like to send it to its sub system this time and my colleague is still > working on it. > He may send the timer patch next week. I think that it would make sense for that patch to be part of the arch port, especially given that (AFAICT) there is no dirver for the other timer IP that you mention. [...] > >> +int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp) > >> +{ > > > >> + /*Map timer to user space */ > >> + vdso_base += PAGE_SIZE; > >> + prot = __pgprot(_PAGE_V | _PAGE_M_UR_KR | _PAGE_D | > >> + _PAGE_G | _PAGE_C_DEV); > >> + ret = io_remap_pfn_range(vma, vdso_base, timer_res.start >> PAGE_SHIFT, > >> + PAGE_SIZE, prot); > >> + if (ret) > >> + goto up_fail; > > > > Maybe this is fine, but it looks a bit suspicious. > > > > Is it safe to map IO memory to a userspace process like this? > > > > In general that isn't safe, since userspace could access other registers > > (if those exist), perform accesses that change the state of hardware, or > > make unsupported access types (e.g. unaligned, atomic) that result in > > errors the kernel can't handle. > > > > Does none of that apply here? > > We only provide read permission to this page so hareware state won't > be chagned. It will trigger exception if we try to write. > We will check about the alignment/atomic issue of this region. Ok, thanks. This is another reason to only do this for devices/drivers that we have drivers for, since we can't know that this is safe in general. Thanks, Mark.