Re: Re: [PATCH] Introduce Naming Convention in Input Subsystem

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Aniroop,

On Fri, Jan 10, 2014 at 04:49:43PM +0000, Aniroop Mathur wrote:
> 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)

First of all, it would be great if you could use MUA that can properly
quote and wrap long lines...

> 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.

[dtor@dtor-d630 ~]$ mkdir my-sysfs-view
[dtor@dtor-d630 ~]$ ln -s
/sys/devices/platform/i8042/serio1/input/input6
my-sysfs-view/input_touchpad
[dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
capabilities/ event6/       modalias      name          power/
subsystem/    uniq
device/       id/           mouse1/       phys          properties
uevent
[dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/
capabilities  device  event6  id  modalias  mouse1  name  phys  power
properties  subsystem  uevent  uniq
[dtor@dtor-d630 ~]$ ls my-sysfs-view/input_touchpad/event6/
dev  device  power  subsystem  uevent

Mmmmkay?

> 
> 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)

No, udev also manages those by requesting to replay all events that
happened dyuring boot.

> 
> 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<num>
> 2. Enable device - does through sys/class/input<num>/enable
> 3. Set delay - does through sys/class/input<num>/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.
>

See the above. You can very easily create your own private 'view' of
sysfs, no kernel changes needed.

> 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.

That seems _very_ high, maybe you need to profile your code a bit. To
search though 2 directories with less than a hundred files each should
not take 1 second.

> 
> 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)

OK, so create your own sysfs view and use it to do direct lookups.

> 
> (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)

User as in end user or your HAL layer?

> 
> > 
> > 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.

Do not use numbers. We emit uevents describing the devices and there a
_lot_ of data there that helps identifying device, such as its path,
subsystem, name, etc.

> 
> ----------------------------
> 
> 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.
> 

Not in the slightest, I am sorry.

Thanks.

-- 
Dmitry