diff mbox

Re: [PATCH] Introduce Naming Convention in Input Subsystem

Message ID 25201406.444611389372583656.JavaMail.weblogic@epml19 (mailing list archive)
State New, archived
Headers show

Commit Message

Aniroop Mathur Jan. 10, 2014, 4:49 p.m. UTC
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<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.

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

Comments

Dmitry Torokhov Jan. 10, 2014, 7:11 p.m. UTC | #1
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.
Aniroop Mathur Jan. 10, 2014, 9:25 p.m. UTC | #2
Hello Mr. Torokhov,
Greetings!

First of all, So sorry, unfortunately i used HTML text again.
and Many thanks for all replies.

Sending email again in plain text.


On Sat, Jan 11, 2014 at 12:41 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> 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?
>

Yes, agreed, we can use udev and soft links to achieve this.
But i think there is something more to take care.

So far, as per discussion, i understood that if an end user wants to use
node names instead of numbers, he/she has to do the following things:
1. Create rules for all input devices in udev rule file i.e. set atleast
unique id and unique name.
(end user need to determine unique id too for every input device)
2. Create links for all input device nodes using names.
(in probe function, after input_register_device)

By following above two steps, the file structure will look like:
devfs - /dev/input_proximity
sysfs - my-sysfs-view/input_proximity --> sys/class/input/input2
sysfs - my-sysfs-view/event_proximity --> sys/class/input/event2

But my concern is why to create trouble for end user to perform
and spend time for two extra steps, when an easy way is possible
to achieve the same task ?

With this patch, end user only need to set node_name_unique variable
and right after that, both for devfs and sysfs,same node name is set.
End user does not need to do or take care of any other extra work,
like creating entry in udev rules, creating links, etc

Also, with creating links for all input devices and checking udev rules
before actually creating a device node, will only increase computation
and time in kernel code.

My purpose is to avoid extra work load and directly create node names
within input subsystem. Also backward compatibility is there.
So i think, it is better than the other alternative way.
Isn't this more easy ? Is there any side-effect or drawback of this patch ?

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

In this i am including time to open a directory, close a directory, open file of
that directory, close file of that directory, searching and computation part.
Including all these, every time for each input device.
All this sums upto 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?
>

End user.

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

Sorry, I am not able to understand this point with respect to removing input
device dependency. Please elaborate a bit more.

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

Thanks,
Aniroop Mathur
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Jan. 10, 2014, 9:46 p.m. UTC | #3
On Sat, Jan 11, 2014 at 02:55:33AM +0530, Aniroop Mathur wrote:
> Hello Mr. Torokhov,
> Greetings!
> 
> First of all, So sorry, unfortunately i used HTML text again.
> and Many thanks for all replies.
> 
> Sending email again in plain text.
> 
> 
> On Sat, Jan 11, 2014 at 12:41 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > 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?
> >
> 
> Yes, agreed, we can use udev and soft links to achieve this.
> But i think there is something more to take care.
> 
> So far, as per discussion, i understood that if an end user wants to use
> node names instead of numbers, he/she has to do the following things:

No, not the end user, system integrator, which is quite different
beast.

> 1. Create rules for all input devices in udev rule file i.e. set atleast
> unique id and unique name.
> (end user need to determine unique id too for every input device)
> 2. Create links for all input device nodes using names.
> (in probe function, after input_register_device)
> 
> By following above two steps, the file structure will look like:
> devfs - /dev/input_proximity
> sysfs - my-sysfs-view/input_proximity --> sys/class/input/input2
> sysfs - my-sysfs-view/event_proximity --> sys/class/input/event2
> 
> But my concern is why to create trouble for end user to perform
> and spend time for two extra steps, when an easy way is possible
> to achieve the same task ?
> 
> With this patch, end user only need to set node_name_unique variable
> and right after that, both for devfs and sysfs,same node name is set.
> End user does not need to do or take care of any other extra work,
> like creating entry in udev rules, creating links, etc
> 
> Also, with creating links for all input devices and checking udev rules
> before actually creating a device node, will only increase computation
> and time in kernel code.

So do not create links, use something else to track devices. You are
getting uevents, that is all you need.

> 
> My purpose is to avoid extra work load and directly create node names
> within input subsystem. Also backward compatibility is there.
> So i think, it is better than the other alternative way.
> Isn't this more easy ? Is there any side-effect or drawback of this patch ?

Yes, there is huge side effect - it is maintenance nightmare, where one
driver can now cause failure for others. What if I have 2 proximity
sensors? 2 accelerometers? How will generic drivers select names that
will satisfy all boards that might use the chips out there? Are you
proposing to put this data in device tree for example? Board files?

IOW no, this is not right solution and the patch will not be accepted.

> 
> >>
> >> 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.
> >
> 
> In this i am including time to open a directory, close a directory, open file of
> that directory, close file of that directory, searching and computation part.
> Including all these, every time for each input device.
> All this sums upto 1 second.

Why are you doing it one at at time? It appears that this happens in
build at boot up for you...

> 
> >>
> >> 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?
> >
> 
> End user.

Why would end user care? He wants his touchscreen to work, not fiddle
with its settings, And we aleady discussed what system integrator should
do.

> 
> >>
> >> >
> >> > 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.
> >
> 
> Sorry, I am not able to understand this point with respect to removing input
> device dependency. Please elaborate a bit more.

Look at the data that is passed in uevents that are sent either when new
input device is created, or when you request replay of such evenst,
realize that it is enough to identify the device and stop relying on
inputX names to remain static. That's it.

Thanks.
Aniroop Mathur Jan. 19, 2014, 3:19 p.m. UTC | #4
Hello Mr.Torokhov
Greetings of the day !!

I have sent you a mail few days back but unfortunately did not
received a mail back.
Thereore, sending mail again.
I hope to hear from you soon.


On Sat, Jan 11, 2014 at 3:16 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Sat, Jan 11, 2014 at 02:55:33AM +0530, Aniroop Mathur wrote:
>> Hello Mr. Torokhov,
>> Greetings!
>>
>> First of all, So sorry, unfortunately i used HTML text again.
>> and Many thanks for all replies.
>>
>> Sending email again in plain text.
>>
>>
>> On Sat, Jan 11, 2014 at 12:41 AM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > 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?
>> >
>>
>> Yes, agreed, we can use udev and soft links to achieve this.
>> But i think there is something more to take care.
>>
>> So far, as per discussion, i understood that if an end user wants to use
>> node names instead of numbers, he/she has to do the following things:
>
> No, not the end user, system integrator, which is quite different
> beast.
>

 Umm.. Sorry, i used wrong word "end user".
 I meant to say "system integrator" only all the time.

>> 1. Create rules for all input devices in udev rule file i.e. set atleast
>> unique id and unique name.
>> (end user need to determine unique id too for every input device)
>> 2. Create links for all input device nodes using names.
>> (in probe function, after input_register_device)
>>
>> By following above two steps, the file structure will look like:
>> devfs - /dev/input_proximity
>> sysfs - my-sysfs-view/input_proximity --> sys/class/input/input2
>> sysfs - my-sysfs-view/event_proximity --> sys/class/input/event2
>>
>> But my concern is why to create trouble for end user to perform
>> and spend time for two extra steps, when an easy way is possible
>> to achieve the same task ?
>>
>> With this patch, end user only need to set node_name_unique variable
>> and right after that, both for devfs and sysfs,same node name is set.
>> End user does not need to do or take care of any other extra work,
>> like creating entry in udev rules, creating links, etc
>>
>> Also, with creating links for all input devices and checking udev rules
>> before actually creating a device node, will only increase computation
>> and time in kernel code.
>
> So do not create links, use something else to track devices. You are
> getting uevents, that is all you need.
>

Firstly,
For input device event node, (/input/input1/event1)
we get below 7 uevents only:
Action, Devpath, Subsystem, Major, Minor, Devname, Seqnum
It is impossible to uniquely identify the device using
these 7 uevent variables, because all this is set in
input subsystem and not by driver developer or system integrator.
Devpath, Devname, minor is all set by input subsytem.
So, these uevents are not sufficient to identify the device.

Secondly,
For input device input node (/input/input1),
I know we get more uevents like Name, Phys, etc,
But problem area is not input node because that we can
already do this using dev_set_name(input_dev->dev) or
using init_name variable in driver code.
But evdev (event device) structure is not accessible to driver
so we cannot use the same for this.

Thirdly,
I know if these default uevents are not sufficient,
additionally, we can read sysfs attribute to identify device,
but as i already said, all this(udev rules or links),
will only increase computation time, and my purpose is to save time
and achieving the same task all together.

>>
>> My purpose is to avoid extra work load and directly create node names
>> within input subsystem. Also backward compatibility is there.
>> So i think, it is better than the other alternative way.
>> Isn't this more easy ? Is there any side-effect or drawback of this patch ?
>
> Yes, there is huge side effect - it is maintenance nightmare, where one
> driver can now cause failure for others. What if I have 2 proximity
> sensors? 2 accelerometers? How will generic drivers select names that
> will satisfy all boards that might use the chips out there? Are you
> proposing to put this data in device tree for example? Board files?
>
> IOW no, this is not right solution and the patch will not be accepted.
>

Firstly,
Yes, i will put name of all input devices in one place i.e.
in board file or device tree. With this, it is very easy for system
integrator to assign unique names for all input devices.

Secondly,
As already mentioned, this patch is backward compatible.
It is not compulsory to use node_name_unique variable.
So generic drivers can still use the same numbering system.
Also, if there are two accelerometers, system integrator can
easily give "accelerometer1" and "accelerometer2" as names.
Moreover, this same problem is for existing kernel system also.
As you know, using dev_set_name function or init_name variable,
we can set name of input node(not event node). So, if same name
is used, device_add of that input device will fail here also
in existing kernel code.

Thirdly,
Using the existing init_name variable also, we can add naming convention
in input subystem. I can also submit patch using this variable already present
in kernel. shall i ?

>>
>> >>
>> >> 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.
>> >
>>
>> In this i am including time to open a directory, close a directory, open file of
>> that directory, close file of that directory, searching and computation part.
>> Including all these, every time for each input device.
>> All this sums upto 1 second.
>
> Why are you doing it one at at time? It appears that this happens in
> build at boot up for you...
>

Yes, this happenns at boot up of upper-layer.
Just as in kernel, probe of each device is
called by one by one, in upper-layer too, input device initialization
is done one by one in some order and input device number is searched.
So this is done every time.
Moreover, with naming convention, scanning is totally removed.
No need to scan/search even once.

>>
>> >>
>> >> 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?
>> >
>>
>> End user.
>
> Why would end user care? He wants his touchscreen to work, not fiddle
> with its settings, And we aleady discussed what system integrator should
> do.
>

Yeah, i meant system integrator only.
Sorry, not end user (i used wrong term).
System integrator or code developer cares for this.

>>
>> >>
>> >> >
>> >> > 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.
>> >
>>
>> Sorry, I am not able to understand this point with respect to removing input
>> device dependency. Please elaborate a bit more.
>
> Look at the data that is passed in uevents that are sent either when new
> input device is created, or when you request replay of such evenst,
> realize that it is enough to identify the device and stop relying on
> inputX names to remain static. That's it.
>
> Thanks.
>
> --
> Dmitry

As already mentioned above, the default uevents are not sufficient to
uniquely identify the input device. Can it ?

Thanks and have a nice day,
Aniroop Mathur
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Tissoires Jan. 20, 2014, 6:27 p.m. UTC | #5
OK, sorry for the noise, my previous message contained HTML and was
refused by the server... Re-posting it

Cheers,
Benjamin

On Mon, Jan 20, 2014 at 1:24 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> Hi Aniroop,
>
> [sorry for top posting, but I really don't know where to put this regarding
> your answers].
>
> I _think_ I get one of the reasons you don't understand Dmitry, and why
> Dmitry does not understand you. From the different mails, I would say that
> you are referring to an Android platform.
> If it's not the case, then sorry for the noise.
>
> So, the last time I checked with android, this system does _not_ have an
> udev user-space daemon, which means that what you seem to be doing is
> parsing manually the sysfs tree to retrieve the inputs and their properties.
> This, IMO, is a very bad idea. Parsing the entire sysfs tree is indeed
> costly and you will have to build everything by hand.
>
> On regular distros, we use udev. This daemon builds its device database from
> the events generated by the kernels directly (the uevents). Once the events
> are emitted, we never (except for some user-space drivers) use them in the
> kernel drivers (at least, I never saw that).
>
> So if you want to create symlinks, then indeed, you just add 2 or 3 rules in
> /etc/udev/rules.d, and then the user space (and the system integrator) can
> see the different devices with the "correct" symlink.
> However, the kernel developer will never see them (and especially in the
> ->probe callback). However, the user-space tools which receives the udev
> events (emitted from udev, not the kernel this time) can easily retrieve
> many information from the event. Just run "udevadm info --export-db" on a
> regular Linux, and you will see that the device which presents the event
> node (/dev/input/eventX) has all the requirements to identify itself (VID,
> PID, path, etc...)
>
> Without udev, you are basically screwed, and I think it is definitively not
> the fault of the kernel, but of android. They had their reasons to not
> include it, so I hope they have an equivalent to help system integrators to
> do their job.
>
> Cheers,
> Benjamin
>
>
>
> On Sun, Jan 19, 2014 at 10:19 AM, Aniroop Mathur <aniroop.mathur@gmail.com>
> wrote:
>>
>> Hello Mr.Torokhov
>> Greetings of the day !!
>>
>> I have sent you a mail few days back but unfortunately did not
>> received a mail back.
>> Thereore, sending mail again.
>> I hope to hear from you soon.
>>
>>
>> On Sat, Jan 11, 2014 at 3:16 AM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Sat, Jan 11, 2014 at 02:55:33AM +0530, Aniroop Mathur wrote:
>> >> Hello Mr. Torokhov,
>> >> Greetings!
>> >>
>> >> First of all, So sorry, unfortunately i used HTML text again.
>> >> and Many thanks for all replies.
>> >>
>> >> Sending email again in plain text.
>> >>
>> >>
>> >> On Sat, Jan 11, 2014 at 12:41 AM, Dmitry Torokhov
>> >> <dmitry.torokhov@gmail.com> wrote:
>> >> > 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?
>> >> >
>> >>
>> >> Yes, agreed, we can use udev and soft links to achieve this.
>> >> But i think there is something more to take care.
>> >>
>> >> So far, as per discussion, i understood that if an end user wants to
>> >> use
>> >> node names instead of numbers, he/she has to do the following things:
>> >
>> > No, not the end user, system integrator, which is quite different
>> > beast.
>> >
>>
>>  Umm.. Sorry, i used wrong word "end user".
>>  I meant to say "system integrator" only all the time.
>>
>> >> 1. Create rules for all input devices in udev rule file i.e. set
>> >> atleast
>> >> unique id and unique name.
>> >> (end user need to determine unique id too for every input device)
>> >> 2. Create links for all input device nodes using names.
>> >> (in probe function, after input_register_device)
>> >>
>> >> By following above two steps, the file structure will look like:
>> >> devfs - /dev/input_proximity
>> >> sysfs - my-sysfs-view/input_proximity --> sys/class/input/input2
>> >> sysfs - my-sysfs-view/event_proximity --> sys/class/input/event2
>> >>
>> >> But my concern is why to create trouble for end user to perform
>> >> and spend time for two extra steps, when an easy way is possible
>> >> to achieve the same task ?
>> >>
>> >> With this patch, end user only need to set node_name_unique variable
>> >> and right after that, both for devfs and sysfs,same node name is set.
>> >> End user does not need to do or take care of any other extra work,
>> >> like creating entry in udev rules, creating links, etc
>> >>
>> >> Also, with creating links for all input devices and checking udev rules
>> >> before actually creating a device node, will only increase computation
>> >> and time in kernel code.
>> >
>> > So do not create links, use something else to track devices. You are
>> > getting uevents, that is all you need.
>> >
>>
>> Firstly,
>> For input device event node, (/input/input1/event1)
>> we get below 7 uevents only:
>> Action, Devpath, Subsystem, Major, Minor, Devname, Seqnum
>> It is impossible to uniquely identify the device using
>> these 7 uevent variables, because all this is set in
>> input subsystem and not by driver developer or system integrator.
>> Devpath, Devname, minor is all set by input subsytem.
>> So, these uevents are not sufficient to identify the device.
>>
>> Secondly,
>> For input device input node (/input/input1),
>> I know we get more uevents like Name, Phys, etc,
>> But problem area is not input node because that we can
>> already do this using dev_set_name(input_dev->dev) or
>> using init_name variable in driver code.
>> But evdev (event device) structure is not accessible to driver
>> so we cannot use the same for this.
>>
>> Thirdly,
>> I know if these default uevents are not sufficient,
>> additionally, we can read sysfs attribute to identify device,
>> but as i already said, all this(udev rules or links),
>> will only increase computation time, and my purpose is to save time
>> and achieving the same task all together.
>>
>> >>
>> >> My purpose is to avoid extra work load and directly create node names
>> >> within input subsystem. Also backward compatibility is there.
>> >> So i think, it is better than the other alternative way.
>> >> Isn't this more easy ? Is there any side-effect or drawback of this
>> >> patch ?
>> >
>> > Yes, there is huge side effect - it is maintenance nightmare, where one
>> > driver can now cause failure for others. What if I have 2 proximity
>> > sensors? 2 accelerometers? How will generic drivers select names that
>> > will satisfy all boards that might use the chips out there? Are you
>> > proposing to put this data in device tree for example? Board files?
>> >
>> > IOW no, this is not right solution and the patch will not be accepted.
>> >
>>
>> Firstly,
>> Yes, i will put name of all input devices in one place i.e.
>> in board file or device tree. With this, it is very easy for system
>> integrator to assign unique names for all input devices.
>>
>> Secondly,
>> As already mentioned, this patch is backward compatible.
>> It is not compulsory to use node_name_unique variable.
>> So generic drivers can still use the same numbering system.
>> Also, if there are two accelerometers, system integrator can
>> easily give "accelerometer1" and "accelerometer2" as names.
>> Moreover, this same problem is for existing kernel system also.
>> As you know, using dev_set_name function or init_name variable,
>> we can set name of input node(not event node). So, if same name
>> is used, device_add of that input device will fail here also
>> in existing kernel code.
>>
>> Thirdly,
>> Using the existing init_name variable also, we can add naming convention
>> in input subystem. I can also submit patch using this variable already
>> present
>> in kernel. shall i ?
>>
>> >>
>> >> >>
>> >> >> 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.
>> >> >
>> >>
>> >> In this i am including time to open a directory, close a directory,
>> >> open file of
>> >> that directory, close file of that directory, searching and computation
>> >> part.
>> >> Including all these, every time for each input device.
>> >> All this sums upto 1 second.
>> >
>> > Why are you doing it one at at time? It appears that this happens in
>> > build at boot up for you...
>> >
>>
>> Yes, this happenns at boot up of upper-layer.
>> Just as in kernel, probe of each device is
>> called by one by one, in upper-layer too, input device initialization
>> is done one by one in some order and input device number is searched.
>> So this is done every time.
>> Moreover, with naming convention, scanning is totally removed.
>> No need to scan/search even once.
>>
>> >>
>> >> >>
>> >> >> 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?
>> >> >
>> >>
>> >> End user.
>> >
>> > Why would end user care? He wants his touchscreen to work, not fiddle
>> > with its settings, And we aleady discussed what system integrator should
>> > do.
>> >
>>
>> Yeah, i meant system integrator only.
>> Sorry, not end user (i used wrong term).
>> System integrator or code developer cares for this.
>>
>> >>
>> >> >>
>> >> >> >
>> >> >> > 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.
>> >> >
>> >>
>> >> Sorry, I am not able to understand this point with respect to removing
>> >> input
>> >> device dependency. Please elaborate a bit more.
>> >
>> > Look at the data that is passed in uevents that are sent either when new
>> > input device is created, or when you request replay of such evenst,
>> > realize that it is enough to identify the device and stop relying on
>> > inputX names to remain static. That's it.
>> >
>> > Thanks.
>> >
>> > --
>> > Dmitry
>>
>> As already mentioned above, the default uevents are not sufficient to
>> uniquely identify the input device. Can it ?
>>
>> Thanks and have a nice day,
>> Aniroop Mathur
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Jan. 20, 2014, 9:49 p.m. UTC | #6
Hi Aniroop,

On Tue, Jan 21, 2014 at 01:34:07AM +0530, Aniroop Mathur wrote:
> Hello Mr Tissoires
> Greetings !!
> 
> On Jan 20, 2014 11:54 PM, "Benjamin Tissoires" <benjamin.tissoires@gmail.com>
> wrote:
> >
> > Hi Aniroop,
> >
> > [sorry for top posting, but I really don't know where to put this
> regarding your answers].
> >
> No problem at all.
> In fact I am quite happy to hear from
> Linux community again
> Its my pleasure.
> 
> > I _think_ I get one of the reasons you don't understand Dmitry, and why
> Dmitry does not understand you. From the different mails, I would say that
> you are referring to an Android platform.
> > If it's not the case, then sorry for the noise.
> >
> No I am not specifically referring to
> Android platform
> But yes, I am from android background
> and through android only I came up with
> this idea.
> 
> > So, the last time I checked with android, this system does _not_ have an
> udev user-space daemon, which means that what you seem to be doing is
> parsing manually the sysfs tree to retrieve the inputs and their properties.
> > This, IMO, is a very bad idea. Parsing the entire sysfs tree is indeed
> costly and you will have to build everything by hand.
> >
> No, Android also have a daemon but it is not udev daemon. There is a
> separate daemon in android different from generic
> udev daemon. It is different but basic is
> still same.
> Just like udev daemon, android daemon
> also receives uevents from kernel
> and creates devfs nodes for us,
> that is /Dev/input/event1/2/3... based
> on uevent env variables.
> Android daemon is different from generic
> daemon in a sense it does not see any file
> like udev rules before creating nodes. Instead it directly create nodes as
> per uevent env variables in the structure.
> So without opening any extra udev rule
> file, it create nodes.
> So I understand it has also a limitation that it only has to be dependent
> only on default uevents and cannot read any sysfs attribute to identify the
> device if default uevents are not sufficient.

Why can it not?

> 
> But the problem I am referring  is for both in case of generic kernel and
> android kernel.

So far I really do not see it as generic kernel problem since we do have
existing infrastructure in userspace that is quite efficient.

> 
> > On regular distros, we use udev. This daemon builds its device database
> from the events generated by the kernels directly (the uevents). Once the
> events are emitted, we never (except for some user-space drivers) use them
> in the kernel drivers (at least, I never saw that).
> >
> > So if you want to create symlinks, then indeed, you just add 2 or 3 rules
> in /etc/udev/rules.d, and then the user space (and the system integrator)
> can see the different devices with the "correct" symlink.
> > However, the kernel developer will never see them (and especially in the
> ->probe callback). However, the user-space tools which receives the udev
> events (emitted from udev, not the kernel this time) can easily retrieve
> many information from the event. Just run "udevadm info --export-db" on a
> regular Linux, and you will see that the device which presents the event
> node (/dev/input/eventX) has all the requirements to identify itself (VID,
> PID, path, etc...)
> >
> Yes udev gives so many uevents but as I checked through logs specifically
> in case of /Dev/input/event1/2/3/4....it does not give sufficient uevents
> to identify the device.
> As I found in logs it gives only 7 env variable like action, subsystem, Dev
> name, Dev path, major, minor and seq number.
> It does not give vendor Id or pid.
> Now with these uevent we cannot identify the input device whether its
> accelerometer or touchscreen. Can we ??

While information sent in uevent emitted for creation of event device
might not be enough to identify and classify input device you can:

1. Capture udevent sent a few moments earlier for the input device
   itself, or

2. Read /sys/class/$DEVNAME/device/uevent to retrieve input device's
uevent data:

[dtor@dtor-d630 work]$ cat /sys/class/input/event6/uevent 
MAJOR=13
MINOR=70
DEVNAME=input/event6
[dtor@dtor-d630 work]$ cat /sys/class/input/event6/device/uevent 
PRODUCT=11/2/8/300
NAME="AlpsPS/2 ALPS DualPoint TouchPad"
PHYS="isa0060/serio1/input0"
PROP=8
EV=b
KEY=e420 70000 0 0 0 0
ABS=260800001000003
MODALIAS=input:b0011v0002p0008e0300-e0,1,3,k110,111,112,145,14A,14D,14E,14F,ra0,1,18,2F,35,36,39,mlsfw
[dtor@dtor-d630 work]$ 

That should give you plenty data to work with. Yes, the 2nd option
involves additional filesystem operations, but this is sysfs (so you are
not hitting real media) and you only need a few (open, read, and close)
per device.

Thanks.
Aniroop Mathur Feb. 10, 2014, 3:54 p.m. UTC | #7
Dear Mr. Torokhov, Mr. Benjamin,
Greetings!

I sent a mail on January 25th, but still waiting for the reply.
Therefore, sending it again.
I hope to hear soon ~


On Tue, Jan 21, 2014 at 3:19 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Aniroop,
>
> On Tue, Jan 21, 2014 at 01:34:07AM +0530, Aniroop Mathur wrote:
>> Hello Mr Tissoires
>> Greetings !!
>>
>> On Jan 20, 2014 11:54 PM, "Benjamin Tissoires" <benjamin.tissoires@gmail.com>
>> wrote:
>> >
>> > Hi Aniroop,
>> >
>> > [sorry for top posting, but I really don't know where to put this
>> regarding your answers].
>> >
>> No problem at all.
>> In fact I am quite happy to hear from
>> Linux community again
>> Its my pleasure.
>>
>> > I _think_ I get one of the reasons you don't understand Dmitry, and why
>> Dmitry does not understand you. From the different mails, I would say that
>> you are referring to an Android platform.
>> > If it's not the case, then sorry for the noise.
>> >
>> No I am not specifically referring to
>> Android platform
>> But yes, I am from android background
>> and through android only I came up with
>> this idea.
>>
>> > So, the last time I checked with android, this system does _not_ have an
>> udev user-space daemon, which means that what you seem to be doing is
>> parsing manually the sysfs tree to retrieve the inputs and their properties.
>> > This, IMO, is a very bad idea. Parsing the entire sysfs tree is indeed
>> costly and you will have to build everything by hand.
>> >
>> No, Android also have a daemon but it is not udev daemon. There is a
>> separate daemon in android different from generic
>> udev daemon. It is different but basic is
>> still same.
>> Just like udev daemon, android daemon
>> also receives uevents from kernel
>> and creates devfs nodes for us,
>> that is /Dev/input/event1/2/3... based
>> on uevent env variables.
>> Android daemon is different from generic
>> daemon in a sense it does not see any file
>> like udev rules before creating nodes. Instead it directly create nodes as
>> per uevent env variables in the structure.
>> So without opening any extra udev rule
>> file, it create nodes.
>> So I understand it has also a limitation that it only has to be dependent
>> only on default uevents and cannot read any sysfs attribute to identify the
>> device if default uevents are not sufficient.
>
> Why can it not?
>

It cannot because currently in android code, it does not read any
sysfs attribute.
It only reads default kernel uevents. I think functionality may be added to read
sysfs attribute also but this functionality is currently not added in
android code.
I think it is not added because in embedded system mostly there are only single
and unique devices and this manipulation might add extra time during android
initialization.

>>
>> But the problem I am referring  is for both in case of generic kernel and
>> android kernel.
>
> So far I really do not see it as generic kernel problem since we do have
> existing infrastructure in userspace that is quite efficient.
>

The existing way is also there but there is always a scope of improvement.
Tell me which is more efficient ?
1. existing method in which udev opens and read udev files for setting name
or
2. Directly set name without doing extra calculations

Its correct existing method will also do the same task but why not to use
more efficient way.
I just want to improve efficiency.


>>
>> > On regular distros, we use udev. This daemon builds its device database
>> from the events generated by the kernels directly (the uevents). Once the
>> events are emitted, we never (except for some user-space drivers) use them
>> in the kernel drivers (at least, I never saw that).
>> >
>> > So if you want to create symlinks, then indeed, you just add 2 or 3 rules
>> in /etc/udev/rules.d, and then the user space (and the system integrator)
>> can see the different devices with the "correct" symlink.
>> > However, the kernel developer will never see them (and especially in the
>> ->probe callback). However, the user-space tools which receives the udev
>> events (emitted from udev, not the kernel this time) can easily retrieve
>> many information from the event. Just run "udevadm info --export-db" on a
>> regular Linux, and you will see that the device which presents the event
>> node (/dev/input/eventX) has all the requirements to identify itself (VID,
>> PID, path, etc...)
>> >
>> Yes udev gives so many uevents but as I checked through logs specifically
>> in case of /Dev/input/event1/2/3/4....it does not give sufficient uevents
>> to identify the device.
>> As I found in logs it gives only 7 env variable like action, subsystem, Dev
>> name, Dev path, major, minor and seq number.
>> It does not give vendor Id or pid.
>> Now with these uevent we cannot identify the input device whether its
>> accelerometer or touchscreen. Can we ??
>
> While information sent in uevent emitted for creation of event device
> might not be enough to identify and classify input device you can:
>
> 1. Capture udevent sent a few moments earlier for the input device
>    itself, or
>
> 2. Read /sys/class/$DEVNAME/device/uevent to retrieve input device's
> uevent data:
>
> [dtor@dtor-d630 work]$ cat /sys/class/input/event6/uevent
> MAJOR=13
> MINOR=70
> DEVNAME=input/event6
> [dtor@dtor-d630 work]$ cat /sys/class/input/event6/device/uevent
> PRODUCT=11/2/8/300
> NAME="AlpsPS/2 ALPS DualPoint TouchPad"
> PHYS="isa0060/serio1/input0"
> PROP=8
> EV=b
> KEY=e420 70000 0 0 0 0
> ABS=260800001000003
> MODALIAS=input:b0011v0002p0008e0300-e0,1,3,k110,111,112,145,14A,14D,14E,14F,ra0,1,18,2F,35,36,39,mlsfw
> [dtor@dtor-d630 work]$
>
> That should give you plenty data to work with. Yes, the 2nd option
> involves additional filesystem operations, but this is sysfs (so you are
> not hitting real media) and you only need a few (open, read, and close)
> per device.

Yes this method can surely solve the task , but not the purpose.
My query is why not to use more efficient way when we can ?
( Already discussed this way is backward compatible and is optional to use)



Apart from above, Could you please tell me the role/use of init_name variable,
which is defined in "struct device" ?
(I think, it is also used to set name of device node)

Thanks,
Aniroop Mathur

>
> Thanks.
>
> --
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aniroop Mathur March 1, 2014, 7:17 p.m. UTC | #8
Dear Mr. Torokhov, Mr. Tissoires,

Gentle Reminder:
Please, update your comment/opinion/suggestion on below mail.

On Mon, Feb 10, 2014 at 9:24 PM, Aniroop Mathur
<aniroop.mathur@gmail.com> wrote:
> Dear Mr. Torokhov, Mr. Benjamin,
> Greetings!
>
> I sent a mail on January 25th, but still waiting for the reply.
> Therefore, sending it again.
> I hope to hear soon ~
>
>
> On Tue, Jan 21, 2014 at 3:19 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> Hi Aniroop,
>>
>> On Tue, Jan 21, 2014 at 01:34:07AM +0530, Aniroop Mathur wrote:
>>> Hello Mr Tissoires
>>> Greetings !!
>>>
>>> On Jan 20, 2014 11:54 PM, "Benjamin Tissoires" <benjamin.tissoires@gmail.com>
>>> wrote:
>>> >
>>> > Hi Aniroop,
>>> >
>>> > [sorry for top posting, but I really don't know where to put this
>>> regarding your answers].
>>> >
>>> No problem at all.
>>> In fact I am quite happy to hear from
>>> Linux community again
>>> Its my pleasure.
>>>
>>> > I _think_ I get one of the reasons you don't understand Dmitry, and why
>>> Dmitry does not understand you. From the different mails, I would say that
>>> you are referring to an Android platform.
>>> > If it's not the case, then sorry for the noise.
>>> >
>>> No I am not specifically referring to
>>> Android platform
>>> But yes, I am from android background
>>> and through android only I came up with
>>> this idea.
>>>
>>> > So, the last time I checked with android, this system does _not_ have an
>>> udev user-space daemon, which means that what you seem to be doing is
>>> parsing manually the sysfs tree to retrieve the inputs and their properties.
>>> > This, IMO, is a very bad idea. Parsing the entire sysfs tree is indeed
>>> costly and you will have to build everything by hand.
>>> >
>>> No, Android also have a daemon but it is not udev daemon. There is a
>>> separate daemon in android different from generic
>>> udev daemon. It is different but basic is
>>> still same.
>>> Just like udev daemon, android daemon
>>> also receives uevents from kernel
>>> and creates devfs nodes for us,
>>> that is /Dev/input/event1/2/3... based
>>> on uevent env variables.
>>> Android daemon is different from generic
>>> daemon in a sense it does not see any file
>>> like udev rules before creating nodes. Instead it directly create nodes as
>>> per uevent env variables in the structure.
>>> So without opening any extra udev rule
>>> file, it create nodes.
>>> So I understand it has also a limitation that it only has to be dependent
>>> only on default uevents and cannot read any sysfs attribute to identify the
>>> device if default uevents are not sufficient.
>>
>> Why can it not?
>>
>
> It cannot because currently in android code, it does not read any
> sysfs attribute.
> It only reads default kernel uevents. I think functionality may be added to read
> sysfs attribute also but this functionality is currently not added in
> android code.
> I think it is not added because in embedded system mostly there are only single
> and unique devices and this manipulation might add extra time during android
> initialization.
>
>>>
>>> But the problem I am referring  is for both in case of generic kernel and
>>> android kernel.
>>
>> So far I really do not see it as generic kernel problem since we do have
>> existing infrastructure in userspace that is quite efficient.
>>
>
> The existing way is also there but there is always a scope of improvement.
> Tell me which is more efficient ?
> 1. existing method in which udev opens and read udev files for setting name
> or
> 2. Directly set name without doing extra calculations
>
> Its correct existing method will also do the same task but why not to use
> more efficient way.
> I just want to improve efficiency.
>
>
>>>
>>> > On regular distros, we use udev. This daemon builds its device database
>>> from the events generated by the kernels directly (the uevents). Once the
>>> events are emitted, we never (except for some user-space drivers) use them
>>> in the kernel drivers (at least, I never saw that).
>>> >
>>> > So if you want to create symlinks, then indeed, you just add 2 or 3 rules
>>> in /etc/udev/rules.d, and then the user space (and the system integrator)
>>> can see the different devices with the "correct" symlink.
>>> > However, the kernel developer will never see them (and especially in the
>>> ->probe callback). However, the user-space tools which receives the udev
>>> events (emitted from udev, not the kernel this time) can easily retrieve
>>> many information from the event. Just run "udevadm info --export-db" on a
>>> regular Linux, and you will see that the device which presents the event
>>> node (/dev/input/eventX) has all the requirements to identify itself (VID,
>>> PID, path, etc...)
>>> >
>>> Yes udev gives so many uevents but as I checked through logs specifically
>>> in case of /Dev/input/event1/2/3/4....it does not give sufficient uevents
>>> to identify the device.
>>> As I found in logs it gives only 7 env variable like action, subsystem, Dev
>>> name, Dev path, major, minor and seq number.
>>> It does not give vendor Id or pid.
>>> Now with these uevent we cannot identify the input device whether its
>>> accelerometer or touchscreen. Can we ??
>>
>> While information sent in uevent emitted for creation of event device
>> might not be enough to identify and classify input device you can:
>>
>> 1. Capture udevent sent a few moments earlier for the input device
>>    itself, or
>>
>> 2. Read /sys/class/$DEVNAME/device/uevent to retrieve input device's
>> uevent data:
>>
>> [dtor@dtor-d630 work]$ cat /sys/class/input/event6/uevent
>> MAJOR=13
>> MINOR=70
>> DEVNAME=input/event6
>> [dtor@dtor-d630 work]$ cat /sys/class/input/event6/device/uevent
>> PRODUCT=11/2/8/300
>> NAME="AlpsPS/2 ALPS DualPoint TouchPad"
>> PHYS="isa0060/serio1/input0"
>> PROP=8
>> EV=b
>> KEY=e420 70000 0 0 0 0
>> ABS=260800001000003
>> MODALIAS=input:b0011v0002p0008e0300-e0,1,3,k110,111,112,145,14A,14D,14E,14F,ra0,1,18,2F,35,36,39,mlsfw
>> [dtor@dtor-d630 work]$
>>
>> That should give you plenty data to work with. Yes, the 2nd option
>> involves additional filesystem operations, but this is sysfs (so you are
>> not hitting real media) and you only need a few (open, read, and close)
>> per device.
>
> Yes this method can surely solve the task , but not the purpose.
> My query is why not to use more efficient way when we can ?
> ( Already discussed this way is backward compatible and is optional to use)
>
>
>
> Apart from above, Could you please tell me the role/use of init_name variable,
> which is defined in "struct device" ?
> (I think, it is also used to set name of device node)
>
> Thanks,
> Aniroop Mathur
>
>>
>> Thanks.
>>
>> --
>> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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