From patchwork Fri Jan 10 16:49:44 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aniroop Mathur X-Patchwork-Id: 3467221 Return-Path: X-Original-To: patchwork-linux-input@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id C070A9F382 for ; Fri, 10 Jan 2014 16:49:49 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id EE62820121 for ; Fri, 10 Jan 2014 16:49:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7E8422011D for ; Fri, 10 Jan 2014 16:49:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757642AbaAJQtq (ORCPT ); Fri, 10 Jan 2014 11:49:46 -0500 Received: from mailout1.samsung.com ([203.254.224.24]:50547 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756933AbaAJQtp (ORCPT ); Fri, 10 Jan 2014 11:49:45 -0500 Received: from epcpsbgx1.samsung.com (u161.gpu120.samsung.co.kr [203.254.230.161]) by mailout1.samsung.com (Oracle Communications Messaging Server 7u4-24.01 (7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MZ70031M2QWG090@mailout1.samsung.com> for linux-input@vger.kernel.org; Sat, 11 Jan 2014 01:49:44 +0900 (KST) X-AuditID: cbfee6a1-b7f9e6d000002777-4a-52d024a8ae92 Received: from epextmailer03 ( [203.254.219.153]) by epcpsbgx1.samsung.com (EPCPMTA) with SMTP id 9C.A6.10103.8A420D25; Sat, 11 Jan 2014 01:49:44 +0900 (KST) Date: Fri, 10 Jan 2014 16:49:44 +0000 (GMT) From: Aniroop Mathur Subject: Re: Re: [PATCH] Introduce Naming Convention in Input Subsystem To: Dmitry Torokhov Cc: "linux-input@vger.kernel.org" , Aniroop Mathur , "cpgs ." , Anurag Aggarwal , Naveen Kumar , "aniroop.mathur@gmail.com" , VIKAS KALA , Poorva Srivastava Reply-to: a.mathur@samsung.com MIME-version: 1.0 X-MTR: 20140110163815043@a.mathur Msgkey: 20140110163815043@a.mathur X-EPLocale: en_US.windows-1252 X-Priority: 3 X-EPWebmail-Msg-Type: personal X-EPWebmail-Reply-Demand: 0 X-EPApproval-Locale: X-EPHeader: ML X-EPTrCode: X-EPTrName: X-MLAttribute: X-RootMTR: 20140110163815043@a.mathur X-ParentMTR: X-ArchiveUser: X-CPGSPASS: Y Content-transfer-encoding: base64 Content-type: text/plain; charset=windows-1252 MIME-version: 1.0 Message-id: <25201406.444611389372583656.JavaMail.weblogic@epml19> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrFIsWRmVeSWpSXmKPExsVy+t/tmborVC4EGWy7w29x89M3VgdGj8+b 5AIYo7hsUlJzMstSi/TtErgybt7+xlLQlFmxa9dl9gbGJWldjJwcQgLKEruOHmXtYuTgkBAw kZg+iQskLCEgJnHh3nq2LkYuoJL5jBLvju9jAkmwCKhKTFz0kgXEZhPQkTi5/ysbiC0s4C7x 5vxbsBoRAUOJX29fMYI0Mwt8YJJoeHacEWSBkICcxMJJqSA1vAKCEidnPmGBWKYocfHDQrAb eAWUJC6dMIQIy0ksmXqZCcLmlZjR/pQFJj7t6xpmCFta4vysDYwwNy/+/hgqzi9x7PYOJoi3 eCWe3A+GGbN78xc2CFtAYuqZg1CtahKLVvVAjeeTWLPwLQvMmF2nljPD9DZs/M0OYjMDXTyl +yGUbSBxZNEcVnRf8Qo4SbyZ9ZVtAqPcLCSpWUjaZyFpR1azgJFlFaNoakFyQXFSeoWhXnFi bnFpXrpecn7uJkZwjD9buIPxy3nrQ4wCHIxKPLwBIheChFgTy4orcw8xSnAwK4nwCksBhXhT EiurUovy44tKc1KLDzFKc7AoifPevZkUJCSQnliSmp2aWpBaBJNl4uCUamDMfP3/S6rquRjB heaOB968aT8sv3tvoJSygU6kjMjD7P89fY2yoqtvN97YuOyiLs9SLSP2l+dO8XD+m+C669vu pCXVGUfua0/VOvvL+p1yn1ZkyKFDUlbpftU/GPpcf1d6q9g9V5nqLLIv6+ATzUrTPx+YPyzL X3lE4smHBfN4TB6UJbcv+n1diaU4I9FQi7moOBEAUnghtu0CAAA= Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Spam-Status: No, score=-7.0 required=5.0 tests=BAYES_00, MIME_BASE64_BLANKS, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hello Mr. Torokhov, Greetings! On Thu, Jan 09, 2014 at 10:27:56AM +0530, Aniroop Mathur wrote: > This patch allows user(driver) to set sysfs node name of input > devices. To set sysfs node name, user(driver) just needs to set > node_name_unique variable. If node_name_unique is not set, default > name is given(as before). So, this patch is completely > backward-compatible. > > Sysfs Input node name format is: input_ > Sysfs Event node name format is: event_ > > This "name" is given by user and automatically, prefix(input and > event) is added by input core. > > This name must be unique among all input devices and driver(user) has > the responsibility to ensure it. If same name is used again for other > input device, registration of that input device will fail because two > input devices cannot have same name. > > Advantages of this patch are: > > 1. Reduces Booting Time of HAL/Upper-Layer because now HAL or > Upper-Layer do not need to search input/event number corresponding to > each input device in /dev/input/... This searching in /dev/input/ was > taking too much time. (Especially in mobile devices, where there are > many input devices (many sensors, touchscreen, etc), it reduces a lot > of booting time) I am sorry, how much time does it take to scan a directory of what, 20 devices? If it such a factor have udev create nodes that are easier for you to locate, similarly how we already create nodes by-id and by-path. For example you can encode major:minor in device name. Re: (Aniroop Mathur) Its correct that we can set name of a device node using udev. Yes, this will change the name of device node(/dev/...) but not sysfs node.(/sys/class/input/...) So now, the problem area will shift from dev path to sysfs path, because now we dont know which sysfs node to refer for a particular input device and hence HAL/Upper-Layer will need to search in /sys/class/input/... instead of /dev/... directory. Moreover, as i know, udev is mainly for hot-pluggable devices, but my problem is for platform devices, which are already present on the board during boot up. (Like in Embedded devices) To avoid confusion and make the problem more clear, I would like to explain the problem and my suggestion by taking an example: Suppose in a mobile device, there are 10 embedded input devices as below: Proximity --- /dev/input0 --- /sys/class/input/input0 --- /sys/class/input/event0 Magnetometer --- /dev/input1 --- /sys/class/input/input1 --- /sys/class/input/event1 Accelerometer --- /dev/input2 --- /sys/class/input/input2 --- /sys/class/input/event2 Touchscreen --- /dev/input3 --- /sys/class/input/input3 --- /sys/class/input/event3 ... 6 more like this (All these are created during boot up time) Kernel has created all these nodes, so that HAL/UpperLayer can read or write values from it. HAL/Upper-Layer needs to do main tasks like: 1. Read raw data - does through /dev/input 2. Enable device - does through sys/class/input/enable 3. Set delay - does through sys/class/input/delay and many more... Now, Lets suppose we need to do these tasks for Accelerometer. If dev node name is set, HAL can directly read value from it (no search required) But for enabling the accelerometer device or set the delay of a hardware chip, there is no direct way, HAL can know which input node to refer for accelerometer because the input number is created dynamically as per device probe order, so this input number can be anything (0,1,2,3...) So HAL will need to search every input node and read its name attribute and keep on searching until a match is found between the "attribute name" and "name passed as parameter". Like for accelerometer, this searching needs to be done for all other input devices. All of this part is done during booting and this takes a lot for time from booting perspective. As I measured, if there are ten devices, it is taking 1 second to do all this searching. (for all devices) So for 20 devices, i guess, it could take upto 2 seconds. With naming convention, there is no need of search neither for dev path nor for sysfs path because HAL directly know which node to refer for which input device and hence this 1 second is reduced to 10ms or even less, therefore saving 990ms. I believe, this is a very good time saving. (from device booting perspective) (Is there any direct way, without scanning all nodes for every input device ?) > > 2. Improves Readabilty of input and event sysfs node paths because > names are used instead of numbers. I do not see why it is that important. If one wants overview /proc/bus/input/devices gives nice picture. Re: (Aniroop Mathur) Its correct, we can get an overview from /proc/bus/input/devices. And therefore using this, we can know input node number for every input device. But there are many input devices and input numbers are not fixed, so its quite difficult to memorize input number for all input devices. Therefore, if a user needs to open some input node from sysfs path, he needs to check /proc/bus/input/devices before opening because he does not know the input number. Moreover, this applies for all other input devices and hence a user need to check this every time. It improves readabilty as below Before: After patch: /dev/input0 /dev/input_proximity /dev/input1 /dev/input_accelerometer ...many more /sys/class/input/input0 /sys/class/input/input_proximity /sys/class/input/input1 /sys/class/input/input_accelerometer ...many more /sys/class/input/event0 /sys/class/input/event_proximity /sys/class/input/event1 /sys/class/input/event_accelerometer ...many more So, just by looking, user can directly open or refer any input node. (no need to refer any other path) > > 3. Removes Input Devices Dependency. If one input device probe fails, > other input devices still work. Before this patch, if one input > device probe fails before input_register_device, then input number of > other input devices changes and due to this permission settings are > disturbed and hence HAL or upper layer cannot open the required sysfs > node because permission denied error comes. I have only one suggestion here: fix your userspace so that does not depend on device initialization ordering. Re: (Aniroop Mathur) We cannot fix userspace because these input/event/dev number are decided/allocated in kernel as per device initialization ordering during boot up. (userspace has no role in it) So, userspace is not aware, which exact input number corresponds to which input device so it ends up searching/scanning every input node untill a match is found. So, there is input device dependency which needs to be removed. ---------------------------- IOW I am totally unconvinced that this facility is needed. Re: (Aniroop Mathur) I hope my problem and suggestion is more clear and convincing now. For reference, copying Patch again: --- drivers/input/evdev.c | 11 ++++++++++- drivers/input/input.c | 12 +++++++++++- include/linux/input.h | 4 ++++ 3 files changed, 25 insertions(+), 2 deletions(-) Thanks, Aniroop Mathur diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index b6ded17..b6a5848 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -1131,7 +1131,16 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev, /* Normalize device number if it falls into legacy range */ if (dev_no < EVDEV_MINOR_BASE + EVDEV_MINORS) dev_no -= EVDEV_MINOR_BASE; - dev_set_name(&evdev->dev, "event%d", dev_no); + + /* + * As per user choice (driver), + * name of sysfs node is set, as mentioned in node_name_unique variable. + * If node_name_unique is not set, default name is given. + */ + if (dev->node_name_unique) + dev_set_name(&evdev->dev, "event_%s", dev->node_name_unique); + else + dev_set_name(&evdev->dev, "event%d", dev_no); evdev->handle.dev = input_get_device(dev); evdev->handle.name = dev_name(&evdev->dev); diff --git a/drivers/input/input.c b/drivers/input/input.c index c044699..c8126b3 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -2073,7 +2073,17 @@ int input_register_device(struct input_dev *dev) if (!dev->setkeycode) dev->setkeycode = input_default_setkeycode; - dev_set_name(&dev->dev, "input%ld", + /* + * As per user choice (driver), + * name of sysfs node is set, as mentioned in node_name_unique variable. + * If node_name_unique is not set, default name is given. + */ + if (dev->node_name_unique) { + atomic_inc_return(&input_no); + dev_set_name(&dev->dev, "input_%s", + dev->node_name_unique); + } else + dev_set_name(&dev->dev, "input%ld", (unsigned long) atomic_inc_return(&input_no) - 1); error = device_add(&dev->dev); diff --git a/include/linux/input.h b/include/linux/input.h index 82ce323..fe44643 100644 --- a/include/linux/input.h +++ b/include/linux/input.h @@ -43,6 +43,9 @@ struct input_value { * @uniq: unique identification code for the device (if device has it) * @id: id of the device (struct input_id) * @propbit: bitmap of device properties and quirks + * @node_name_unique: name of input and event sysfs device node (char *). + * This name must be unique among all input devices and driver(user) + * has the responsibility to ensure it (if using). * @evbit: bitmap of types of events supported by the device (EV_KEY, * EV_REL, etc.) * @keybit: bitmap of keys/buttons this device has @@ -123,6 +126,7 @@ struct input_dev { const char *phys; const char *uniq; struct input_id id; + char *node_name_unique; unsigned long propbit[BITS_TO_LONGS(INPUT_PROP_CNT)];