* Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock
From: Mukesh Ojha @ 2019-04-24 14:09 UTC (permalink / raw)
To: Al Viro
Cc: dmitry.torokhov@gmail.com, linux-input, linux-kernel,
Gaurav Kohli, Peter Hutterer, Martin Kepplinger, Paul E. McKenney
In-Reply-To: <20190424130711.GP2217@ZenIV.linux.org.uk>
On 4/24/2019 6:37 PM, Al Viro wrote:
> On Wed, Apr 24, 2019 at 05:40:40PM +0530, Mukesh Ojha wrote:
>> Al,
>>
>> i tried to put traceprintk inside ioctl after fdget and fdput on a simple
>> call of open => ioctl => close
> in a loop, and multithreaded, presumably?
>
>> on /dev/uinput.
>>
>> uinput-532 [002] .... 45.312044: SYSC_ioctl: 2 <= f_count
>>> <After fdget()
>> uinput-532 [002] .... 45.312055: SYSC_ioctl: 2
>> <After fdput()
>> uinput-532 [004] .... 45.313766: uinput_open: uinput: 1
>> uinput-532 [004] .... 45.313783: SYSC_ioctl: 1
>> uinput-532 [004] .... 45.313788: uinput_ioctl_handler:
>> uinput: uinput_ioctl_handler, 1
>> uinput-532 [004] .... 45.313835: SYSC_ioctl: 1
>> uinput-532 [004] .... 45.313843: uinput_release: uinput: 0
>>
>>
>> So while a ioctl is running the f_count is 1, so a fput could be run and do
>> atomic_long_dec_and_test
>> this could call release right ?
> Look at ksys_ioctl():
> int ksys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
> {
> int error;
> struct fd f = fdget(fd);
> an error or refcount bumped
> if (!f.file)
> return -EBADF;
> not an error, then. We know that ->release() won't be called
> until we drop the reference we've just acquired.
> error = security_file_ioctl(f.file, cmd, arg);
> if (!error)
> error = do_vfs_ioctl(f.file, fd, cmd, arg);
> ... and we are done with calling ->ioctl(), so
> fdput(f);
> ... we drop the reference we'd acquired.
>
> Seeing refcount 1 inside ->ioctl() is possible, all right:
>
> CPU1: ioctl(2) resolves fd to struct file *, refcount 2
> CPU2: close(2) rips struct file * from descriptor table and does fput() to drop it;
> refcount reaches 1 and fput() is done; no call of ->release() yet.
> CPU1: we get arouund to ->ioctl(), where your trace sees refcount 1
> CPU1: done with ->ioctl(), drop our reference. *NOW* refcount gets to 0, and
> ->release() is called.
Thanks for the detail reply, Al
This was my simple program no multithreading just to understand f_counting
int main()
{
int fd = open("/dev/uinput", O_WRONLY | O_NONBLOCK);
ioctl(fd, UI_SET_EVBIT, EV_KEY);
close(fd);
return 0;
}
uinput-532 [002] .... 45.312044: SYSC_ioctl: 2 <=
f_count > <After fdget()
uinput-532 [002] .... 45.312055: SYSC_ioctl:
2 <After fdput()
uinput-532 [004] .... 45.313766: uinput_open: uinput:
1 /* This is from the uinput driver uinput_open()*/
=>>>> /* All the above calls happened for the
open() in userspace*/
uinput-532 [004] .... 45.313783: SYSC_ioctl: 1 /* This
print is for the trace, i put after fdget */
uinput-532 [004] .... 45.313788: uinput_ioctl_handler:
uinput: uinput_ioctl_handler, 1 /* This print is from the uinput_ioctl
driver */
uinput-532 [004] .... 45.313835: SYSC_ioctl: 1 /* This
print is for the trace, i put after fdput*/
uinput-532 [004] .... 45.313843: uinput_release:
uinput: 0 /* And this is from the close() */
Should fdget not suppose to increment the f_count here, as it is coming 1 ?
This f_count to one is done at the open, but i have no idea how this
below f_count 2 came before open() for
this simple program.
uinput-532 [002] .... 45.312044: SYSC_ioctl: 2 <= f_count
> <After fdget()
uinput-532 [002] .... 45.312055: SYSC_ioctl:
2 <After fdput()
-Mukesh
> IOW, in your trace fput() has already been run by close(2); having somebody else
> do that again while we are in ->ioctl() would be a bug (to start with, where
> did they get that struct file * and why wasn't that reference contributing to
> struct file refcount?)
>
> In all cases we only call ->release() once all references gone - both
> the one(s) in descriptor tables and any transient ones acquired by
> fdget(), etc.
>
> I would really like to see a reproducer for the original use-after-free report...
^ permalink raw reply
* [PATCH AUTOSEL 4.9 01/28] HID: logitech: check the return value of create_singlethread_workqueue
From: Sasha Levin @ 2019-04-24 13:57 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Kangjie Lu, Jiri Kosina, Sasha Levin, linux-input
From: Kangjie Lu <kjlu@umn.edu>
[ Upstream commit 6c44b15e1c9076d925d5236ddadf1318b0a25ce2 ]
create_singlethread_workqueue may fail and return NULL. The fix checks if it is
NULL to avoid NULL pointer dereference. Also, the fix moves the call of
create_singlethread_workqueue earlier to avoid resource-release issues.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-logitech-hidpp.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2e2515a4c070..3198faf5cff4 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1282,6 +1282,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
kfree(data);
return -ENOMEM;
}
+ data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+ if (!data->wq) {
+ kfree(data->effect_ids);
+ kfree(data);
+ return -ENOMEM;
+ }
+
data->hidpp = hidpp;
data->feature_index = feature_index;
data->version = version;
@@ -1326,7 +1333,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
/* ignore boost value at response.fap.params[2] */
/* init the hardware command queue */
- data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
atomic_set(&data->workqueue_size, 0);
/* initialize with zero autocenter to get wheel in usable state */
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 01/35] HID: logitech: check the return value of create_singlethread_workqueue
From: Sasha Levin @ 2019-04-24 13:57 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Kangjie Lu, Jiri Kosina, Sasha Levin, linux-input
From: Kangjie Lu <kjlu@umn.edu>
[ Upstream commit 6c44b15e1c9076d925d5236ddadf1318b0a25ce2 ]
create_singlethread_workqueue may fail and return NULL. The fix checks if it is
NULL to avoid NULL pointer dereference. Also, the fix moves the call of
create_singlethread_workqueue earlier to avoid resource-release issues.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-logitech-hidpp.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 614054af904a..b83d4173fc7f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1907,6 +1907,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
kfree(data);
return -ENOMEM;
}
+ data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+ if (!data->wq) {
+ kfree(data->effect_ids);
+ kfree(data);
+ return -ENOMEM;
+ }
+
data->hidpp = hidpp;
data->feature_index = feature_index;
data->version = version;
@@ -1951,7 +1958,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
/* ignore boost value at response.fap.params[2] */
/* init the hardware command queue */
- data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
atomic_set(&data->workqueue_size, 0);
/* initialize with zero autocenter to get wheel in usable state */
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 02/52] HID: logitech: check the return value of create_singlethread_workqueue
From: Sasha Levin @ 2019-04-24 13:57 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Kangjie Lu, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424135758.22850-1-sashal@kernel.org>
From: Kangjie Lu <kjlu@umn.edu>
[ Upstream commit 6c44b15e1c9076d925d5236ddadf1318b0a25ce2 ]
create_singlethread_workqueue may fail and return NULL. The fix checks if it is
NULL to avoid NULL pointer dereference. Also, the fix moves the call of
create_singlethread_workqueue earlier to avoid resource-release issues.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-logitech-hidpp.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 19cc980eebce..8425d3548a41 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1907,6 +1907,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
kfree(data);
return -ENOMEM;
}
+ data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+ if (!data->wq) {
+ kfree(data->effect_ids);
+ kfree(data);
+ return -ENOMEM;
+ }
+
data->hidpp = hidpp;
data->feature_index = feature_index;
data->version = version;
@@ -1951,7 +1958,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
/* ignore boost value at response.fap.params[2] */
/* init the hardware command queue */
- data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
atomic_set(&data->workqueue_size, 0);
/* initialize with zero autocenter to get wheel in usable state */
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.0 03/66] HID: logitech: check the return value of create_singlethread_workqueue
From: Sasha Levin @ 2019-04-24 13:56 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Kangjie Lu, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424135746.22655-1-sashal@kernel.org>
From: Kangjie Lu <kjlu@umn.edu>
[ Upstream commit 6c44b15e1c9076d925d5236ddadf1318b0a25ce2 ]
create_singlethread_workqueue may fail and return NULL. The fix checks if it is
NULL to avoid NULL pointer dereference. Also, the fix moves the call of
create_singlethread_workqueue earlier to avoid resource-release issues.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin (Microsoft) <sashal@kernel.org>
---
drivers/hid/hid-logitech-hidpp.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index f040c8a7f9a9..199cc256e9d9 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -2111,6 +2111,13 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
kfree(data);
return -ENOMEM;
}
+ data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
+ if (!data->wq) {
+ kfree(data->effect_ids);
+ kfree(data);
+ return -ENOMEM;
+ }
+
data->hidpp = hidpp;
data->feature_index = feature_index;
data->version = version;
@@ -2155,7 +2162,6 @@ static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
/* ignore boost value at response.fap.params[2] */
/* init the hardware command queue */
- data->wq = create_singlethread_workqueue("hidpp-ff-sendqueue");
atomic_set(&data->workqueue_size, 0);
/* initialize with zero autocenter to get wheel in usable state */
--
2.19.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.0 02/66] HID: Increase maximum report size allowed by hid_field_extract()
From: Sasha Levin @ 2019-04-24 13:56 UTC (permalink / raw)
To: linux-kernel, stable; +Cc: Kai-Heng Feng, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190424135746.22655-1-sashal@kernel.org>
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
[ Upstream commit 94a9992f7dbdfb28976b565af220e0c4a117144a ]
Commit 71f6fa90a353 ("HID: increase maximum global item tag report size
to 256") increases the max report size from 128 to 256.
We also need to update the report size in hid_field_extract() otherwise
it complains and truncates now valid report size:
[ 406.165461] hid-sensor-hub 001F:8086:22D8.0002: hid_field_extract() called with n (192) > 32! (kworker/5:1)
BugLink: https://bugs.launchpad.net/bugs/1818547
Fixes: 71f6fa90a353 ("HID: increase maximum global item tag report size to 256")
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin (Microsoft) <sashal@kernel.org>
---
drivers/hid/hid-core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9993b692598f..860e21ec6a49 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1301,10 +1301,10 @@ static u32 __extract(u8 *report, unsigned offset, int n)
u32 hid_field_extract(const struct hid_device *hid, u8 *report,
unsigned offset, unsigned n)
{
- if (n > 32) {
- hid_warn(hid, "hid_field_extract() called with n (%d) > 32! (%s)\n",
+ if (n > 256) {
+ hid_warn(hid, "hid_field_extract() called with n (%d) > 256! (%s)\n",
n, current->comm);
- n = 32;
+ n = 256;
}
return __extract(report, offset, n);
--
2.19.1
^ permalink raw reply related
* Re: [PATCH 1/5] of/device: Add of_ functions for device_link_{add,remove}
From: Benjamin GAIGNARD @ 2019-04-24 13:39 UTC (permalink / raw)
To: Rafael J. Wysocki, agx@sigxcpu.org, Yannick FERTRE
Cc: dmitry.torokhov@gmail.com, robh+dt@kernel.org,
mark.rutland@arm.com, hadess@hadess.net, frowand.list@gmail.com,
m.felsch@pengutronix.de, arnd@arndb.de,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com, broonie@kernel.org,
Linux PM
In-Reply-To: <d038f078-08dc-41ff-edc2-12f37d88a8a3@intel.com>
On 4/24/19 3:06 PM, Rafael J. Wysocki wrote:
> On 4/24/2019 12:19 PM, Benjamin Gaignard wrote:
>> Allows to create and remove links between consumer and suppliers from
>> device-tree data. Use 'links-add' property from consumer node to setup
>> a link with a list of suppliers.
>
> One immediate question about this one is why stateless links are
> better here?
They aren't better, it is just because I wanted to keep one of_ function
for each related device_link_* functions.
>
>> Consumers will be suspend before their suppliers and resume after them.
>>
>> Add devm_of_device_links_add() to automatically remove the links
>> when the device is unbound from the bus.
>
> And this might not be necessary even with managed links.
I guess I could use DL_FLAG_PM_RUNTIME and DL_FLAG_AUTOREMOVE_CONSUMER
flags and just keep
of_device_links_add() but the naming will not be explicit.
>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>> drivers/of/device.c | 103
>> ++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/of_device.h | 20 +++++++++
>> 2 files changed, 123 insertions(+)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 3717f2a20d0d..011ba9bf7642 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -336,3 +336,106 @@ int of_device_uevent_modalias(struct device
>> *dev, struct kobj_uevent_env *env)
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
>> +
>> +/**
>> + * of_device_links_add - Create links between consumer and suppliers
>> from
>> + * device tree data
>> + *
>> + * @consumer: consumer device
>> + *
>> + * Returns 0 on success, < 0 on failure.
>> + */
>> +int of_device_links_add(struct device *consumer)
>> +{
>> + struct device_node *np;
>> + struct platform_device *pdev;
>> + int i = 0;
>> +
>> + np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> + while (np) {
>> + pdev = of_find_device_by_node(np);
>> + of_node_put(np);
>> + if (!pdev)
>> + return -EINVAL;
>> +
>> + device_link_add(consumer, &pdev->dev, DL_FLAG_STATELESS);
>> + platform_device_put(pdev);
>> +
>> + np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_device_links_add);
>> +
>> +/**
>> + * of_device_links_remove - Remove links between consumer and
>> suppliers from
>> + * device tree data
>> + *
>> + * @consumer: consumer device
>> + *
>> + * Returns 0 on success, < 0 on failure.
>> + */
>> +int of_device_links_remove(struct device *consumer)
>> +{
>> + struct device_node *np;
>> + struct platform_device *pdev;
>> + int i = 0;
>> +
>> + np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> + while (np) {
>> + pdev = of_find_device_by_node(np);
>> + of_node_put(np);
>> + if (!pdev)
>> + return -EINVAL;
>> +
>> + device_link_remove(consumer, &pdev->dev);
>> + platform_device_put(pdev);
>> +
>> + np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_device_links_remove);
>> +
>> +static void devm_of_device_links_remove(struct device *dev, void *res)
>> +{
>> + of_device_links_remove(*(struct device **)res);
>> +}
>> +
>> +/**
>> + * devm_of_device_links_add - Create links between consumer and
>> suppliers
>> + * from device tree data
>> + *
>> + * @consumer: consumer device
>> + *
>> + * Returns 0 on success, < 0 on failure.
>> + *
>> + * Similar to of_device_links_add(), but will automatically call
>> + * of_device_links_remove() when the device is unbound from the bus.
>> + */
>> +int devm_of_device_links_add(struct device *consumer)
>> +{
>> + struct device **ptr;
>> + int ret;
>> +
>> + if (!consumer)
>> + return -EINVAL;
>> +
>> + ptr = devres_alloc(devm_of_device_links_remove,
>> + sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return -ENOMEM;
>> +
>> + ret = of_device_links_add(consumer);
>> + if (ret < 0) {
>> + devres_free(ptr);
>> + } else {
>> + *ptr = consumer;
>> + devres_add(consumer, ptr);
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_of_device_links_add);
>> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
>> index 8d31e39dd564..ad01db6828e8 100644
>> --- a/include/linux/of_device.h
>> +++ b/include/linux/of_device.h
>> @@ -41,6 +41,11 @@ extern int of_device_request_module(struct device
>> *dev);
>> extern void of_device_uevent(struct device *dev, struct
>> kobj_uevent_env *env);
>> extern int of_device_uevent_modalias(struct device *dev, struct
>> kobj_uevent_env *env);
>> +
>> +extern int of_device_links_add(struct device *consumer);
>> +extern int of_device_links_remove(struct device *consumer);
>> +extern int devm_of_device_links_add(struct device *consumer);
>> +
>> static inline void of_device_node_put(struct device *dev)
>> {
>> of_node_put(dev->of_node);
>> @@ -91,6 +96,21 @@ static inline int of_device_uevent_modalias(struct
>> device *dev,
>> return -ENODEV;
>> }
>> +static int of_device_links_add(struct device *consumer)
>> +{
>> + return 0;
>> +}
>> +
>> +static int of_device_links_remove(struct device *consumer)
>> +{
>> + return 0;
>> +}
>> +
>> +static int devm_of_device_links_add(struct device *consumer)
>> +{
>> + return 0;
>> +}
>> +
>> static inline void of_device_node_put(struct device *dev) { }
>> static inline const struct of_device_id *__of_match_device(
>
>
^ permalink raw reply
* Re: [PATCH 1/2] HID: input: make sure the wheel high resolution multiplier is set
From: Benjamin Tissoires @ 2019-04-24 13:30 UTC (permalink / raw)
To: Jiri Kosina, James Feeney, Peter Hutterer
Cc: open list:HID CORE LAYER, lkml, 3.8+
In-Reply-To: <20190423154615.18257-1-benjamin.tissoires@redhat.com>
On Tue, Apr 23, 2019 at 5:46 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Some old mice have a tendency to not accept the high resolution multiplier.
> They reply with a -EPIPE which was previously ignored.
>
> Force the call to resolution multiplier to be synchronous and actually
> check for the answer. If this fails, consider the mouse like a normal one.
>
> Fixes: 2dc702c991e377 ("HID: input: use the Resolution Multiplier for
> high-resolution scrolling")
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1700071
> Reported-and-tested-by: James Feeney <james@nurealm.net>
> Cc: stable@vger.kernel.org # v5.0+
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
Given that this basically breaks a basic functionality of many
Microsoft mice, I went ahead and applied this series to
for-5.1/upstream-fixes
Cheers,
Benjamin
> drivers/hid/hid-core.c | 7 +++-
> drivers/hid/hid-input.c | 81 +++++++++++++++++++++++++----------------
> include/linux/hid.h | 2 +-
> 3 files changed, 56 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 76464aabd20c..92387fc0bf18 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1637,7 +1637,7 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
> * Implement a generic .request() callback, using .raw_request()
> * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
> */
> -void __hid_request(struct hid_device *hid, struct hid_report *report,
> +int __hid_request(struct hid_device *hid, struct hid_report *report,
> int reqtype)
> {
> char *buf;
> @@ -1646,7 +1646,7 @@ void __hid_request(struct hid_device *hid, struct hid_report *report,
>
> buf = hid_alloc_report_buf(report, GFP_KERNEL);
> if (!buf)
> - return;
> + return -ENOMEM;
>
> len = hid_report_len(report);
>
> @@ -1663,8 +1663,11 @@ void __hid_request(struct hid_device *hid, struct hid_report *report,
> if (reqtype == HID_REQ_GET_REPORT)
> hid_input_report(hid, report->type, buf, ret, 0);
>
> + ret = 0;
> +
> out:
> kfree(buf);
> + return ret;
> }
> EXPORT_SYMBOL_GPL(__hid_request);
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 1fce0076e7dc..fadf76f0a386 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1542,52 +1542,71 @@ static void hidinput_close(struct input_dev *dev)
> hid_hw_close(hid);
> }
>
> -static void hidinput_change_resolution_multipliers(struct hid_device *hid)
> +static bool __hidinput_change_resolution_multipliers(struct hid_device *hid,
> + struct hid_report *report, bool use_logical_max)
> {
> - struct hid_report_enum *rep_enum;
> - struct hid_report *rep;
> struct hid_usage *usage;
> + bool update_needed = false;
> int i, j;
>
> - rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
> - list_for_each_entry(rep, &rep_enum->report_list, list) {
> - bool update_needed = false;
> + if (report->maxfield == 0)
> + return false;
>
> - if (rep->maxfield == 0)
> - continue;
> + /*
> + * If we have more than one feature within this report we
> + * need to fill in the bits from the others before we can
> + * overwrite the ones for the Resolution Multiplier.
> + */
> + if (report->maxfield > 1) {
> + hid_hw_request(hid, report, HID_REQ_GET_REPORT);
> + hid_hw_wait(hid);
> + }
>
> - /*
> - * If we have more than one feature within this report we
> - * need to fill in the bits from the others before we can
> - * overwrite the ones for the Resolution Multiplier.
> + for (i = 0; i < report->maxfield; i++) {
> + __s32 value = use_logical_max ?
> + report->field[i]->logical_maximum :
> + report->field[i]->logical_minimum;
> +
> + /* There is no good reason for a Resolution
> + * Multiplier to have a count other than 1.
> + * Ignore that case.
> */
> - if (rep->maxfield > 1) {
> - hid_hw_request(hid, rep, HID_REQ_GET_REPORT);
> - hid_hw_wait(hid);
> - }
> + if (report->field[i]->report_count != 1)
> + continue;
>
> - for (i = 0; i < rep->maxfield; i++) {
> - __s32 logical_max = rep->field[i]->logical_maximum;
> + for (j = 0; j < report->field[i]->maxusage; j++) {
> + usage = &report->field[i]->usage[j];
>
> - /* There is no good reason for a Resolution
> - * Multiplier to have a count other than 1.
> - * Ignore that case.
> - */
> - if (rep->field[i]->report_count != 1)
> + if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
> continue;
>
> - for (j = 0; j < rep->field[i]->maxusage; j++) {
> - usage = &rep->field[i]->usage[j];
> + *report->field[i]->value = value;
> + update_needed = true;
> + }
> + }
> +
> + return update_needed;
> +}
>
> - if (usage->hid != HID_GD_RESOLUTION_MULTIPLIER)
> - continue;
> +static void hidinput_change_resolution_multipliers(struct hid_device *hid)
> +{
> + struct hid_report_enum *rep_enum;
> + struct hid_report *rep;
> + int ret;
>
> - *rep->field[i]->value = logical_max;
> - update_needed = true;
> + rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
> + list_for_each_entry(rep, &rep_enum->report_list, list) {
> + bool update_needed = __hidinput_change_resolution_multipliers(hid,
> + rep, true);
> +
> + if (update_needed) {
> + ret = __hid_request(hid, rep, HID_REQ_SET_REPORT);
> + if (ret) {
> + __hidinput_change_resolution_multipliers(hid,
> + rep, false);
> + return;
> }
> }
> - if (update_needed)
> - hid_hw_request(hid, rep, HID_REQ_SET_REPORT);
> }
>
> /* refresh our structs */
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index ac0c70b4ce10..5a24ebfb5b47 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -894,7 +894,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
> unsigned int hidinput_count_leds(struct hid_device *hid);
> __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
> void hid_output_report(struct hid_report *report, __u8 *data);
> -void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
> +int __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
> u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
> struct hid_device *hid_allocate_device(void);
> struct hid_report *hid_register_report(struct hid_device *device,
> --
> 2.19.2
>
^ permalink raw reply
* Re: [PATCH] HID: logitech-dj: add usbhid dependency in Kconfig
From: Benjamin Tissoires @ 2019-04-24 13:18 UTC (permalink / raw)
To: Jiri Kosina, Hans de Goede
Cc: open list:HID CORE LAYER, lkml, kbuild test robot
In-Reply-To: <20190424131636.8320-1-benjamin.tissoires@redhat.com>
On Wed, Apr 24, 2019 at 3:16 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> An oversight from me.
>
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
I went ahead and pushed it already to for-5.2/logitech
Cheers,
Benjamin
> ---
> drivers/hid/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 76d82063d8d2..c3c390ca3690 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -521,6 +521,7 @@ config HID_LOGITECH
>
> config HID_LOGITECH_DJ
> tristate "Logitech Unifying receivers full support"
> + depends on USB_HID
> depends on HIDRAW
> depends on HID_LOGITECH
> select HID_LOGITECH_HIDPP
> --
> 2.19.2
>
^ permalink raw reply
* [PATCH] HID: logitech-dj: add usbhid dependency in Kconfig
From: Benjamin Tissoires @ 2019-04-24 13:16 UTC (permalink / raw)
To: Jiri Kosina, Hans de Goede
Cc: linux-input, linux-kernel, Benjamin Tissoires, kbuild test robot
An oversight from me.
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 76d82063d8d2..c3c390ca3690 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -521,6 +521,7 @@ config HID_LOGITECH
config HID_LOGITECH_DJ
tristate "Logitech Unifying receivers full support"
+ depends on USB_HID
depends on HIDRAW
depends on HID_LOGITECH
select HID_LOGITECH_HIDPP
--
2.19.2
^ permalink raw reply related
* Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock
From: Al Viro @ 2019-04-24 13:07 UTC (permalink / raw)
To: Mukesh Ojha
Cc: dmitry.torokhov@gmail.com, linux-input, linux-kernel,
Gaurav Kohli, Peter Hutterer, Martin Kepplinger, Paul E. McKenney
In-Reply-To: <5614f04f-827d-1668-9ed0-60d93e110b8e@codeaurora.org>
On Wed, Apr 24, 2019 at 05:40:40PM +0530, Mukesh Ojha wrote:
>
> Al,
>
> i tried to put traceprintk inside ioctl after fdget and fdput on a simple
> call of open => ioctl => close
in a loop, and multithreaded, presumably?
> on /dev/uinput.
>
> uinput-532 [002] .... 45.312044: SYSC_ioctl: 2 <= f_count
> > <After fdget()
> uinput-532 [002] .... 45.312055: SYSC_ioctl: 2
> <After fdput()
> uinput-532 [004] .... 45.313766: uinput_open: uinput: 1
> uinput-532 [004] .... 45.313783: SYSC_ioctl: 1
> uinput-532 [004] .... 45.313788: uinput_ioctl_handler:
> uinput: uinput_ioctl_handler, 1
> uinput-532 [004] .... 45.313835: SYSC_ioctl: 1
> uinput-532 [004] .... 45.313843: uinput_release: uinput: 0
>
>
> So while a ioctl is running the f_count is 1, so a fput could be run and do
> atomic_long_dec_and_test
> this could call release right ?
Look at ksys_ioctl():
int ksys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
{
int error;
struct fd f = fdget(fd);
an error or refcount bumped
if (!f.file)
return -EBADF;
not an error, then. We know that ->release() won't be called
until we drop the reference we've just acquired.
error = security_file_ioctl(f.file, cmd, arg);
if (!error)
error = do_vfs_ioctl(f.file, fd, cmd, arg);
... and we are done with calling ->ioctl(), so
fdput(f);
... we drop the reference we'd acquired.
Seeing refcount 1 inside ->ioctl() is possible, all right:
CPU1: ioctl(2) resolves fd to struct file *, refcount 2
CPU2: close(2) rips struct file * from descriptor table and does fput() to drop it;
refcount reaches 1 and fput() is done; no call of ->release() yet.
CPU1: we get arouund to ->ioctl(), where your trace sees refcount 1
CPU1: done with ->ioctl(), drop our reference. *NOW* refcount gets to 0, and
->release() is called.
IOW, in your trace fput() has already been run by close(2); having somebody else
do that again while we are in ->ioctl() would be a bug (to start with, where
did they get that struct file * and why wasn't that reference contributing to
struct file refcount?)
In all cases we only call ->release() once all references gone - both
the one(s) in descriptor tables and any transient ones acquired by
fdget(), etc.
I would really like to see a reproducer for the original use-after-free report...
^ permalink raw reply
* Re: [PATCH 3/3] iio: light: apple-ib-als: Add driver for ALS on iBridge chip.
From: Jonathan Cameron @ 2019-04-24 12:27 UTC (permalink / raw)
To: Life is hard, and then you die
Cc: Jonathan Cameron, Peter Meerwald-Stadler, Jiri Kosina,
Benjamin Tissoires, Hartmut Knaack, Lars-Peter Clausen, Lee Jones,
linux-input, linux-iio, linux-kernel
In-Reply-To: <20190423103855.GB12528@innovation.ch>
On Tue, 23 Apr 2019 03:38:55 -0700
"Life is hard, and then you die" <ronald@innovation.ch> wrote:
> Hi Jonathan, Peter,
>
> On Mon, Apr 22, 2019 at 01:01:38PM +0100, Jonathan Cameron wrote:
> > On Mon, 22 Apr 2019 11:17:27 +0200 (CEST)
> > Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
> >
> > > On Sun, 21 Apr 2019, Ronald Tschalär wrote:
> > >
> > > > On 2016/2017 MacBook Pro's with a Touch Bar the ALS is attached to,
> > > > and exposed via the iBridge device. This provides the driver for that
> > > > sensor.
> > >
> > > some comments below inline
> > I'll 'nest' on Peter's review to avoid repetition.
> >
> > A few additional comments inline.
>
> Thank you both for your reviews. I've applied most of your
> suggestions, so I'm limiting my responses below to just those places
> where I need further clarification or can provide some answers.
>
> [snip]
> > > > +static int appleals_read_raw(struct iio_dev *iio_dev,
> > > > + struct iio_chan_spec const *chan,
> > > > + int *val, int *val2, long mask)
> > > > +{
> > > > + struct appleals_device *als_dev =
> > > > + *(struct appleals_device **)iio_priv(iio_dev);
> > > > + __s32 value;
> > > > + int rc;
> > > > +
> > > > + *val = 0;
> > > > + *val2 = 0;
> > >
> > > no need to set these here
> > >
> > > > +
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_RAW:
> > > > + case IIO_CHAN_INFO_PROCESSED:
> > > > + *val = appleals_get_field_value(als_dev, als_dev->illum_field);
> >
> > How can one read by both processed and raw?
>
> From my understanding, processed is a converted and normalized value
> of raw? But since the raw value is already in Lux the processed value
> is then the same. Furthermore, looking at userspace apps that read
> these values (e.g. iio-sensor-proxy, lightsd) they seem to be all over
> the map in terms of what sysfs entries they read:
> in_illuminance_input, in_intensity_both_raw, etc. So I figured
> providing both would provide maximal compatibility.
>
> What then is currently the preferred approach here?
Just provide the processed value. We may have to fix any userspace apps
that aren't checking both. For the raw version they should be applying the
scale in userspace.
>
> > > > + case IIO_CHAN_INFO_HYSTERESIS:
> > > > + if (val == APPLEALS_DYN_SENS) {
> > > > + if (als_dev->cur_hysteresis != APPLEALS_DYN_SENS) {
> > > > + als_dev->cur_hysteresis = val;
> > > > + illum = appleals_get_field_value(als_dev,
> > > > + als_dev->illum_field);
> > > > + appleals_update_dyn_sensitivity(als_dev, illum);
> >
> > There is some debate in another thread on whether dynamic sensitivity can be
> > mapped to hysteresis or not...
>
> Is that the "drivers: iio: Add more data field for iio driver
> hysteresis parsing" thread?
That's the one.
>
> In any case, I'm using the value 0 here to indicate that the
> pseudo-percent-relative change sensitivity should be used. Better
> suggestions certainly welcome.
It seems we are converging on a new IIO_CHAN_INFO type for sensitivity.
>
> [snip]
> > > > +static const struct iio_chan_spec appleals_channels[] = {
> > > > + {
> > > > + .type = IIO_INTENSITY,
> > > > + .modified = 1,
> > > > + .channel2 = IIO_MOD_LIGHT_BOTH,
> > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > > > + BIT(IIO_CHAN_INFO_RAW),
> > Why return both processed and raw? We don't generally allow that in IIO
> > (there are a few historical exceptions due to us getting it wrong the
> > first time).
>
> Ok. But which should be used then, especially in view of the fact that
> different user-space apps/daemons appear to use different values?
forwards.
>
> [snip]
> > > > +static int appleals_config_iio(struct appleals_device *als_dev)
> [snip]
> > > > + iio_dev = iio_device_alloc(sizeof(als_dev));
> > >
> > > how about using the devm_ variants?
>
> The problem is that there is no device with the proper lifetime here.
> In particular we can't use hid_device (the only struct device
> available here) because the instances of those can (and do) outlive
> the module. This is a consequence of the hid-driver demuxing: e.g.
> when this als module is unloaded, the hid_device is still in use by
> the touchbar driver, and if this als module is then loaded again it
> will get associated with that same hid_device again.
Fair enough.
>
> [snip]
> > > > + rc = iio_device_register(iio_dev);
> > > > + if (rc) {
> > > > + dev_err(als_dev->log_dev, "failed to register iio device: %d\n",
> > > > + rc);
> > > > + goto unreg_iio_trig;
> > > > + }
> > > > +
> > > > + als_dev->iio_dev = iio_dev;
> >
> > I really don't like nest of pointers going on in here. I haven't dug
> > down to check if any of them can be remove, but they are definitely
> > ugly to deal with.
>
> I'm not sure how this can be avoided: *iio_dev is allocated by
> iio_device_alloc(), and this is just storing that pointer in our data
> structure for this als device.
You are probably correct. Ugly will have to stay :(
This tends to happen when we end up merging various different
driver subsystems together like this.
>
> [snip]
> > > > +static int appleals_probe(struct hid_device *hdev,
> > > > + const struct hid_device_id *id)
> > > > +{
> > > > + struct appleals_device *als_dev =
> > > > + appleib_get_drvdata(hid_get_drvdata(hdev),
> > > > + &appleals_hid_driver);
> > > > + struct hid_field *state_field;
> > > > + struct hid_field *illum_field;
> > > > + int rc;
> > > > +
> > > > + /* find als fields and reports */
> > > > + state_field = appleib_find_hid_field(hdev, HID_USAGE_SENSOR_ALS,
> > > > + HID_USAGE_SENSOR_PROP_REPORT_STATE);
> > > > + illum_field = appleib_find_hid_field(hdev, HID_USAGE_SENSOR_ALS,
> > > > + HID_USAGE_SENSOR_LIGHT_ILLUM);
> > > > + if (!state_field || !illum_field)
> > > > + return -ENODEV;
> > > > +
> > > > + if (als_dev->hid_dev) {
> > > > + dev_warn(als_dev->log_dev,
> > > > + "Found duplicate ambient light sensor - ignoring\n");
> >
> > I'll bite. So how can this actually happen? What fundamentally breaks in
> > your model if there really are two? Can you fix whatever that is so
> > as to drop this handling?
>
> This should indeed never happen - the check is just defensive
> programming, in case either some new device in some new model appears,
> or due to some bug somewhere.
>
> To actually handle this I'd need to split up appleals_device into a
> per iBridge-device part and a per ALS-device part, and allow multiple
> ALS-device parts. This is certainly doable, but seemed a bit overkill
> for something that is unlikely to ever be needed.
Fair enough I guess.
>
> [snip]
> > > > +static void appleals_remove(struct hid_device *hdev)
> > > > +{
> > > > + struct appleals_device *als_dev =
> > > > + appleib_get_drvdata(hid_get_drvdata(hdev),
> > > > + &appleals_hid_driver);
> > > > +
> > >
> > > could be a lot less if devm_ were used?
>
> Agreed, but see explanation above of why this can't be used.
>
> > > > + if (als_dev->iio_dev) {
> > > > + iio_device_unregister(als_dev->iio_dev);
> > > > +
> > > > + iio_trigger_unregister(als_dev->iio_trig);
> > > > + iio_trigger_free(als_dev->iio_trig);
> > > > + als_dev->iio_trig = NULL;
> >
> > I would be decidedly worried if you actually have any paths
> > where setting these to NULL do anything useful. By the time
> > we get here we should absolutely 'know' nothing will touch
> > these pointers.
>
> I guess I was being overly paranoid here. I've removed these now.
>
> > It is these that are blocking Peter's suggestion of using
> > devm to clean all of this up automatically for you.
>
> No, that is for a different reason - see above.
>
> [snip]
> > > > +static int appleals_platform_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct appleib_platform_data *pdata = pdev->dev.platform_data;
> > > > + struct appleib_device *ib_dev = pdata->ib_dev;
> > > > + struct appleals_device *als_dev;
> > > > + int rc;
> > > > +
> > > > + als_dev = kzalloc(sizeof(*als_dev), GFP_KERNEL);
> > > > + if (!als_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + als_dev->ib_dev = ib_dev;
> > > > + als_dev->log_dev = pdata->log_dev;
> > > > +
> > > > + rc = appleib_register_hid_driver(ib_dev, &appleals_hid_driver, als_dev);
> >
> > Hmm. This is all a bit odd. We register a platform device, to call it's driver
> > so that it can then register another driver? I'll let Lee comment on that but
> > it does seem overly complex.
>
> "Normally" this call would be to hid_register_driver().
> However, as I
> tried to explain in the cover letter and module comments, at least one
> of the hid_device's needs to be shared by both this als driver and the
> touchbar driver. But the linux device/driver architecture does not
> allow multiple drivers to be attached to a single device. Hence the
> mfd driver implements demuxing hid_driver that allows multiple
> hid_drivers to be registered for a single hid_device. And this is why
> we register our hid_driver with this demuxing driver here instead of
> directly via hid_register_driver().
I'd expect a probe to create a device managed by the driver. The drive would normally
be registered on module load, hence _init, rather than probe.
I can't find any instances of hid_register_driver being called from a probe function.
Jonathan
>
> Does this make sense?
>
>
> Cheers,
>
> Ronald
>
^ permalink raw reply
* Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock
From: Mukesh Ojha @ 2019-04-24 12:10 UTC (permalink / raw)
To: Al Viro, dmitry.torokhov@gmail.com
Cc: linux-input, linux-kernel, Gaurav Kohli, Peter Hutterer,
Martin Kepplinger, Paul E. McKenney
In-Reply-To: <20190423110611.GL2217@ZenIV.linux.org.uk>
On 4/23/2019 4:36 PM, Al Viro wrote:
> On Tue, Apr 23, 2019 at 08:49:44AM +0000, dmitry.torokhov@gmail.com wrote:
>> On Tue, Apr 23, 2019 at 12:51:13PM +0530, Mukesh Ojha wrote:
>>> I have taken care this case from ioctl and release point of view.
>>>
>>> Even if the release gets called first it will make the
>>> file->private_data=NULL.
>>> and further call to ioctl will not be a problem as the check is already
>>> there.
>> Al, do we have any protections in VFS layer from userspace hanging onto
>> a file descriptor and calling ioctl() on it even as another thread
>> calls close() on the same fd?
>>
>> Should the issue be solved by individual drivers, or more careful
>> accounting for currently running operations is needed at VFS layer?
> Neither. An overlap of ->release() and ->ioctl() is possible only
> if you've got memory corruption somewhere.
>
> close() overlapping ioctl() is certainly possible, and won't trigger
> that at all - sys_ioctl() holds onto reference to struct file, so
> its refcount won't reach zero until we are done with it.
Al,
i tried to put traceprintk inside ioctl after fdget and fdput on a
simple call of open => ioctl => close
on /dev/uinput.
uinput-532 [002] .... 45.312044: SYSC_ioctl: 2 <=
f_count > <After fdget()
uinput-532 [002] .... 45.312055: SYSC_ioctl:
2 <After fdput()
uinput-532 [004] .... 45.313766: uinput_open: uinput: 1
uinput-532 [004] .... 45.313783: SYSC_ioctl: 1
uinput-532 [004] .... 45.313788: uinput_ioctl_handler:
uinput: uinput_ioctl_handler, 1
uinput-532 [004] .... 45.313835: SYSC_ioctl: 1
uinput-532 [004] .... 45.313843: uinput_release: uinput: 0
So while a ioctl is running the f_count is 1, so a fput could be run and
do atomic_long_dec_and_test
this could call release right ?
-Mukesh
^ permalink raw reply
* Re: [PATCH 3/3] iio: Add PAT9125 optical tracker sensor
From: Jonathan Cameron @ 2019-04-24 12:10 UTC (permalink / raw)
To: Alexandre
Cc: Jonathan Cameron, robh+dt, mark.rutland, knaack.h, lars, pmeerw,
linux-kernel, linux-iio, baylibre-upstreaming, Dmitry Torokhov,
linux-input
In-Reply-To: <327a56e4-66d8-859c-7f35-458b533ac72f@baylibre.com>
On Tue, 23 Apr 2019 10:57:40 +0200
Alexandre <amergnat@baylibre.com> wrote:
Hi Alexandre,
> Hi Jonathan,
>
> On 4/22/19 10:42, Jonathan Cameron wrote:
> > On Tue, 16 Apr 2019 14:49:19 +0200
> > Alexandre <amergnat@baylibre.com> wrote:
> >
> >> Hello Jonathan,
> >>
> >> On 4/7/19 12:20, Jonathan Cameron wrote:
> >>> Hi Alexandre,
> >>>
> >>> So I have no problem with this as an IIO driver, but for devices that
> >>> are somewhat 'on the edge' I always like to get a clear answer to the
> >>> question: Why not input?
> >>>
> >>> I would also argue that, to actually be 'useful' we would typically need
> >>> some representation of the 'mechanicals' that are providing the motion
> >>> being measured. Looking at the datasheet this includes, rotating shafts
> >>> (side or end on), disk edges and flat surface tracking (mouse like).
> >>>
> >>> That's easy enough to do with the iio in kernel consumer interface. These
> >>> are similar to when we handle analog electronic front ends.
> >>>
> >>> I you can, please describe what it is being used for in your application
> >>> as that may give us somewhere to start!
> >>>
> >>> + CC Dmitry and linux-input.
> >> I developed this driver to detect the board movement which can't be
> >> detected by accelerometer (very slow motion). I admit this use case can
> >> be handled by an input, and I'm agree with you, PAT9125 driver could be
> >> an input. But, like you said, this chip is able to track different kind
> >> of motion, and additionally have an interrupt GPIO, so using it like
> >> input limit the driver potential. This chip is designed to work in
> >> industrial measurement or embedded systems, and the IIO API match with
> >> these environments, so it's the best way to exploit the entire potential
> >> of this chip.
> >>
> >> As I understand (from
> >> https://www.kernel.org/doc/html/v4.12/input/event-codes.html#mice ),
> >> mouse driver must report values when the device move. This feature
> >> souldn't be mandatory for an optical tracker driver, specially for cases
> >> where user prefers to use buffer or poll only when he need data.
> >>
> >>> If 1 or 2, I would suggest that you provide absolute position to
> >>> Linux. So add the value to a software counter and provide that.
> >>> 32 bits should be plenty of resolution for that.
> >> I can't provide absolute position, only relative. Do you mean using
> >> input driver to do that ? If not, how is built the position data?
> > Sorry, I should have been clearer on this.
> > I mean absolute relative to the start point. So on startup you assume
> > absolute position is 0 and go from there. What I can't work out is
> > if the device does internal tracking, or whether each time you read
> > it effectively resets it's internal counters to 0 so the next measurement
> > is relative to the previous one.
> Each time you read that reset internal counters to 0.
> >>> Silly question for you. What happens if you set the delta values to 0?
> >>> Do we get an interrupt which is effectively data ready?
> >>> If we do, you might want to think about a scheme where that is an option.
> >>> As things currently stand we have a confusing interface where changing this
> >>> threshold effects the buffered data output. That should only be the
> >>> case if this interface is for a trigger, not an event.
> >> I'm not sure to understand your question. Is it possible to read delta_x
> >> and delta_y = 0 in special/corner case because internal value continue
> >> to be updated after toggled motion_detect pin (used for IRQ) until
> >> values registers are read and then motion_detect pin is released:
> >>
> >> * Chip move (i.e. +2 on X axis and 0 on Y axis)
> >> * Motion_detect IRQ trigger and internal reg value is updated (i.e.
> >> delta_x = 2 and delta_y = 0.
> >> * GPIO IRQ handled but read_value isn't executed yet (timing reason)
> >> * Chip move back to it origin point (i.e. -2 on X axis and 0 on Y axis)
> >> * Motion_detect IRQ still low because it hasn't been reset by read
> >> value and internal reg value is updated (i.e. delta_x = 0 and
> >> delta_y = 0)
> >> * Read_value is executed, we get delta values = 0.
> > Again, I was unclear. Is it possible to set the device to interrupt
> > every time it evaluates whether motion has occured? Not only when it
> > concludes that there has been some motion. That would allow the interrupt
> > to be used as a signal that the device has taken a measurement (data
> > ready signal in other sensors).
> >
> I don't know, the datasheet don't describe the role of each bit in
> registers and I don't found documentation which provide that. I had to
> do research on example code to retrieve some bits, but got nothing on
> motion detection pin configuration.
That kind of rules out reporting this as a speed as we can't guarantee
when the last read occurred. Oh well, was a bit optimistic!
>
> >>> If it is actually not possible to report the two channels separately
> >>> then don't report them at all except via the buffered interface and
> >>> set the available scan masks so that both are on.
> >> I found a way to keep the consistency between delta x and delta y
> >> (without losing data). The first part is to reset a value only when user
> >> read it (also when it's buffered). The second part is to add the new
> >> value to the old value. With these two mechanism, X and Y will always be
> >> consistent:
> >>
> >> * as possible during a move.
> >> * perfectly when move is finished.
> > Ah. This adding old value to a new value point is what I was getting
> > at (I think) with 'absolute' position above.
> >
> > In industrial control for example you have absolute position by using
> > limit switches to set your baseline. Measurement devices are then
> > capable of either reporting relative position, which is the movement
> > since the last reading was taken, or 'absolute' position which is
> > referenced to some known point. It was this form of absolute position
> > that I was suggesting you use. If you use such a system without a
> > limit switch it is normally called unreference motion. You can do
> > it but then the 0 is where ever your device was at power on.
> > For some systems it doesn't actually matter (conveyor belts for
> > instance where the positions you care about are between things
> > on the belt, not the position of the belt itself).
>
> Ok, I decided to return delta between last read/buffering to stay closer
> to the hardware mechanism and still coherent with "IIO_CHAN_INFO_RAW".
> If user want absolute position, he can make an addition of all received
> value in user space, and that allow him to reset/replace the initial
> position when he want it.
Hmm. Unfortunately this doesn't really map to the expectations of
userspace in IIO. This particular option is neither position nor speed,
but rather a delta position. For that we don't really have a current
representation. There is also not guarantee that you don't have multiple
concurrent readers. If that happens you will get a delta against something
unknown.
I think you need to do the addition in kernel to be able to provide
userspace with something consistent.
Jonathan
>
> > Thanks,
> >
> > Jonathan
> >
> >>
> >> Regards,
> >>
> >> Alexandre
> >>
>
> Thanks for your comments,
>
> Alexandre
>
^ permalink raw reply
* Re: [PATCH v10 02/11] dt-bindings: power: supply: add DT bindings for max77650
From: Sebastian Reichel @ 2019-04-24 11:31 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski
In-Reply-To: <20190423090451.23711-3-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 1903 bytes --]
Hi,
On Tue, Apr 23, 2019 at 11:04:42AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add the DT binding document for the battery charger module of max77650.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
-- Sebastian
> .../power/supply/max77650-charger.txt | 28 +++++++++++++++++++
> 1 file changed, 28 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/max77650-charger.txt
>
> diff --git a/Documentation/devicetree/bindings/power/supply/max77650-charger.txt b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> new file mode 100644
> index 000000000000..e6d0fb6ff94e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> @@ -0,0 +1,28 @@
> +Battery charger driver for MAX77650 PMIC from Maxim Integrated.
> +
> +This module is part of the MAX77650 MFD device. For more details
> +see Documentation/devicetree/bindings/mfd/max77650.txt.
> +
> +The charger is represented as a sub-node of the PMIC node on the device tree.
> +
> +Required properties:
> +--------------------
> +- compatible: Must be "maxim,max77650-charger"
> +
> +Optional properties:
> +--------------------
> +- input-voltage-min-microvolt: Minimum CHGIN regulation voltage. Must be one
> + of: 4000000, 4100000, 4200000, 4300000,
> + 4400000, 4500000, 4600000, 4700000.
> +- input-current-limit-microamp: CHGIN input current limit (in microamps). Must
> + be one of: 95000, 190000, 285000, 380000,
> + 475000.
> +
> +Example:
> +--------
> +
> + charger {
> + compatible = "maxim,max77650-charger";
> + input-voltage-min-microvolt = <4200000>;
> + input-current-limit-microamp = <285000>;
> + };
> --
> 2.21.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 3/4] input: keyboard: gpio_keys_polled: use gpio lookup table
From: Enrico Weigelt, metux IT consult @ 2019-04-24 10:59 UTC (permalink / raw)
To: Dmitry Torokhov, Enrico Weigelt, metux IT consult
Cc: linux-kernel, linux-input
In-Reply-To: <20190419074811.7rkq3p6t7s56g4fo@penguin>
On 19.04.19 09:48, Dmitry Torokhov wrote:
> I would prefer if gpiod API could parse static board data/gpio lookup
> tables for child nodes, instead of adding this to gpio-keys. Now that
> Heikki Krogerus work on software nodes has landed I need to resurrect
> my patch to gpiolib.
Of course, a more generic approach would be better.
I did it that way I wasn't that aware that a more generic approach is
in the pipeline, and I needed something usable now.
The patch is already used in the field and seems to work quite well,
but if I'm alway open for better solutions.
I figure that the generic solution will still take some time, so can we
take this one for the next merge window and rework it in the next one ?
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: [PATCH 1/4] mod_devicetable: helper macro for declaring oftree module device table
From: Enrico Weigelt, metux IT consult @ 2019-04-24 10:48 UTC (permalink / raw)
To: Dmitry Torokhov, Enrico Weigelt, metux IT consult
Cc: linux-kernel, linux-input
In-Reply-To: <20190419074021.tigbyfezmt4erjms@penguin>
On 19.04.19 09:40, Dmitry Torokhov wrote:
> Hi Enrico,
>
> On Tue, Apr 16, 2019 at 09:57:22PM +0200, Enrico Weigelt, metux IT consult wrote:
>> Little helper macro that declares an oftree module device table,
>> if CONFIG_OF is enabled. Otherwise it's just noop.
>>
>> This is also helpful if some drivers can be built w/ or w/o
>> oftree support.
>
> This should go to OF folks, please.
hmm, they should be CCed, if my script works right.
This one is only needed for the 4th patch (skip oftree...).
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: [PATCH 1/3] mfd: apple-ibridge: Add Apple iBridge MFD driver.
From: Life is hard, and then you die @ 2019-04-24 10:47 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jiri Kosina, Benjamin Tissoires, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Lee Jones,
linux-input, linux-iio, linux-kernel
In-Reply-To: <20190422123426.2d0b4bdf@archlinux>
Hi Jonathan,
On Mon, Apr 22, 2019 at 12:34:26PM +0100, Jonathan Cameron wrote:
> On Sun, 21 Apr 2019 20:12:49 -0700
> Ronald Tschalär <ronald@innovation.ch> wrote:
>
> > The iBridge device provides access to several devices, including:
> > - the Touch Bar
> > - the iSight webcam
> > - the light sensor
> > - the fingerprint sensor
> >
> > This driver provides the core support for managing the iBridge device
> > and the access to the underlying devices. In particular, since the
> > functionality for the touch bar and light sensor is exposed via USB HID
> > interfaces, and the same HID device is used for multiple functions, this
> > driver provides a multiplexing layer that allows multiple HID drivers to
> > be registered for a given HID device. This allows the touch bar and ALS
> > driver to be separated out into their own modules.
> >
> > Signed-off-by: Ronald Tschalär <ronald@innovation.ch
> Hi Ronald,
>
> I've only taken a fairly superficial look at this. A few global
> things to note though.
Thanks for this review.
> 1. Please either use kernel-doc style for function descriptions, or
> do not. Right now you are sort of half way there.
Apologies, on re-reading the docs I realize what you mean here. Should
be fixed now (next rev).
> 2. There is quite a complex nest of separate structures being allocated,
> so think about whether they can be simplified. In particular
> use of container_of macros can allow a lot of forwards and backwards
> pointers to be dropped if you embed the various structures directly.
Done (see also below).
[snip]
> > +#define call_void_driver_func(drv_info, fn, ...) \
>
> This sort of macro may seem like a good idea because it saves a few lines
> of code. However, that comes at the cost of readability, so just
> put the code inline.
>
> > + do { \
> > + if ((drv_info)->driver->fn) \
> > + (drv_info)->driver->fn(__VA_ARGS__); \
> > + } while (0)
> > +
> > +#define call_driver_func(drv_info, fn, ret_type, ...) \
> > + ({ \
> > + ret_type rc = 0; \
> > + \
> > + if ((drv_info)->driver->fn) \
> > + rc = (drv_info)->driver->fn(__VA_ARGS__); \
> > + \
> > + rc; \
> > + })
Just to clarify, you're only talking about removing/inlining the
call_void_driver_func() macro, not the call_driver_func() macro,
right?
[snip]
> > +static struct appleib_hid_dev_info *
> > +appleib_add_device(struct appleib_device *ib_dev, struct hid_device *hdev,
> > + const struct hid_device_id *id)
> > +{
> > + struct appleib_hid_dev_info *dev_info;
> > + struct appleib_hid_drv_info *drv_info;
> > +
> > + /* allocate device-info for this device */
> > + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL);
> > + if (!dev_info)
> > + return NULL;
> > +
> > + INIT_LIST_HEAD(&dev_info->drivers);
> > + dev_info->device = hdev;
> > + dev_info->device_id = id;
> > +
> > + /* notify all our sub drivers */
> > + mutex_lock(&ib_dev->update_lock);
> > +
> This is interesting. I'd like to see a comment here on what
> this flag is going to do.
I'm not sure I follow: update_lock is simply a mutex protecting all
driver and device update (i.e. add/remove) functions. Are you
therefore looking for something like:
/* protect driver and device lists against concurrent updates */
mutex_lock(&ib_dev->update_lock);
[snip]
> > +static int appleib_probe(struct acpi_device *acpi)
> > +{
> > + struct appleib_device *ib_dev;
> > + struct appleib_platform_data *pdata;
> Platform_data has a lot of historical meaning in Linux.
> Also you have things in here that are not platform related
> at all, such as the dev pointer. Hence I would rename it
> as device_data or private or something like that.
Ok. I guess I called in platform_data because it's stored in the mfd
cell's "platform_data" field. Anyway, changed it per your suggestion.
> > + int i;
> > + int ret;
> > +
> > + if (appleib_dev)
> This singleton bothers me a bit. I'm really not sure why it
> is necessary. You can just put a pointer to this in
> the pdata for the subdevs and I think that covers most of your
> usecases. It's generally a bad idea to limit things to one instance
> of a device unless that actually major simplifications.
> I'm not seeing them here.
Yes, this one is quite ugly. appleib_dev is static so that
appleib_hid_probe() can find it. I could not find any other way to
pass the appleib_dev instance to that probe function.
However, on looking at this again, I realized that hid_device_id has
a driver_data field which can be used for this; so if I added the
hid_driver and hid_device_id structs to the appleib_device (instead of
making them static like now) I could fill in the driver_data and avoid
this hack. This looks much cleaner.
Thanks for pointing this uglyness out again.
[snip]
> > + if (!ib_dev->subdevs) {
> > + ret = -ENOMEM;
> > + goto free_dev;
> > + }
> > +
> > + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
>
> Might as well embed this in ib_dev as well.
Agreed.
> That would let
> you used container_of to avoid having to carry the ib_dev pointer
> around in side pdata.
I see. I guess my main reservation is that the functions exported to
the sub-drivers would now take a 'struct appleib_device_data *'
argument instead of a 'struct appleib_device *', which just seems a
bit unnatural. E.g.
int appleib_register_hid_driver(struct appleib_device_data *ib_ddata,
struct hid_driver *driver, void *data);
instead of (the current)
int appleib_register_hid_driver(struct appleib_device *ib_dev,
struct hid_driver *driver, void *data);
[snip]
> > + ret = mfd_add_devices(&acpi->dev, PLATFORM_DEVID_NONE,
> > + ib_dev->subdevs, ARRAY_SIZE(appleib_subdevs),
> > + NULL, 0, NULL);
> > + if (ret) {
> > + dev_err(LOG_DEV(ib_dev), "Error adding MFD devices: %d\n", ret);
> > + goto free_pdata;
> > + }
> > +
> > + acpi->driver_data = ib_dev;
> > + appleib_dev = ib_dev;
> > +
> > + ret = hid_register_driver(&appleib_hid_driver);
> > + if (ret) {
> > + dev_err(LOG_DEV(ib_dev), "Error registering hid driver: %d\n",
> > + ret);
> > + goto rem_mfd_devs;
> > + }
> > +
> > + return 0;
> > +
> > +rem_mfd_devs:
> > + mfd_remove_devices(&acpi->dev);
> > +free_pdata:
> > + kfree(pdata);
> > +free_subdevs:
> > + kfree(ib_dev->subdevs);
> > +free_dev:
> > + appleib_dev = NULL;
> > + acpi->driver_data = NULL;
> Why at this point? It's not set to anything until much later in the
> probe flow.
If the hid_register_driver() call fails, we get here after driver_data
has been assigned. However, looking at this again, acpi->driver_data
is only used by the remove, suspend, and resume callbacks, and those
will not be called until a successful return from probe; therefore I
can safely move the setting of driver_data to after the
hid_register_driver() call and avoid having to set it to NULL in the
error cleanup.
> May be worth thinking about devm_ managed allocations
> to cleanup some of these allocations automatically and simplify
> the error handling.
Good point, thanks.
[snip]
> > +
> > + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> > + if (ACPI_FAILURE(rc))
> > + dev_warn(LOG_DEV(ib_dev), "SOCW(0) failed: %s\n",
>
> I can sort of see you might want to do the LOG_DEV for consistency
> but here I'm fairly sure it's just dev which might be clearer.
Sorry, you mean rename the macro LOG_DEV() to just DEV()?
Cheers,
Ronald
^ permalink raw reply
* [PATCH 5/5] input: goodix - Call devm_of_device_links_add() to create links
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
In-Reply-To: <20190424101913.1534-1-benjamin.gaignard@st.com>
From: Yannick Fertré <yannick.fertre@st.com>
Add a call to devm_of_device_links_add() to create links with suppliers
at probe time.
Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
drivers/input/touchscreen/goodix.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f57d82220a88..9aefbfa39319 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -30,6 +30,7 @@
#include <linux/slab.h>
#include <linux/acpi.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <asm/unaligned.h>
struct goodix_ts_data;
@@ -812,6 +813,8 @@ static int goodix_ts_probe(struct i2c_client *client,
ts->chip = goodix_get_chip_data(ts->id);
+ devm_of_device_links_add(&client->dev);
+
if (ts->gpiod_int && ts->gpiod_rst) {
/* update device config */
ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL,
--
2.15.0
^ permalink raw reply related
* [PATCH 4/5] Input: goodix: Document links-add property
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
In-Reply-To: <20190424101913.1534-1-benjamin.gaignard@st.com>
Explain the purpose of links_add property.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 8cf0b4d38a7e..0447151608c6 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -24,6 +24,8 @@ Optional properties:
- touchscreen-size-x
- touchscreen-size-y
- touchscreen-swapped-x-y
+ - links-add : List of suppliers handles, suppliers will be
+ suspended after goodix device and resume before it.
The touchscreen-* properties are documented in touchscreen.txt in this
directory.
--
2.15.0
^ permalink raw reply related
* [PATCH 3/5] input: edt-ft5x06 - Call devm_of_device_links_add() to create links
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
In-Reply-To: <20190424101913.1534-1-benjamin.gaignard@st.com>
From: Yannick Fertré <yannick.fertre@st.com>
Add a call to devm_of_device_links_add() to create links with suppliers
at probe time.
Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
drivers/input/touchscreen/edt-ft5x06.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 702bfda7ee77..ac9f7e85efb0 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1167,6 +1167,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
i2c_set_clientdata(client, tsdata);
+ devm_of_device_links_add(&client->dev);
+
irq_flags = irq_get_trigger_type(client->irq);
if (irq_flags == IRQF_TRIGGER_NONE)
irq_flags = IRQF_TRIGGER_FALLING;
--
2.15.0
^ permalink raw reply related
* [PATCH 2/5] Input: edt-ft5x06: Document links-add property
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
In-Reply-To: <20190424101913.1534-1-benjamin.gaignard@st.com>
Explain the purpose of links_add property.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
index 870b8c5cce9b..38b84d2b32e6 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -60,6 +60,8 @@ Optional properties:
- touchscreen-inverted-x : See touchscreen.txt
- touchscreen-inverted-y : See touchscreen.txt
- touchscreen-swapped-x-y : See touchscreen.txt
+ - links-add : List of suppliers handles, suppliers will be
+ suspended after touchscreen device and resume before it.
Example:
polytouch: edt-ft5x06@38 {
--
2.15.0
^ permalink raw reply related
* [PATCH 1/5] of/device: Add of_ functions for device_link_{add,remove}
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
In-Reply-To: <20190424101913.1534-1-benjamin.gaignard@st.com>
Allows to create and remove links between consumer and suppliers from
device-tree data. Use 'links-add' property from consumer node to setup
a link with a list of suppliers.
Consumers will be suspend before their suppliers and resume after them.
Add devm_of_device_links_add() to automatically remove the links
when the device is unbound from the bus.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
drivers/of/device.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/of_device.h | 20 +++++++++
2 files changed, 123 insertions(+)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 3717f2a20d0d..011ba9bf7642 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -336,3 +336,106 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
return 0;
}
EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
+
+/**
+ * of_device_links_add - Create links between consumer and suppliers from
+ * device tree data
+ *
+ * @consumer: consumer device
+ *
+ * Returns 0 on success, < 0 on failure.
+ */
+int of_device_links_add(struct device *consumer)
+{
+ struct device_node *np;
+ struct platform_device *pdev;
+ int i = 0;
+
+ np = of_parse_phandle(consumer->of_node, "links-add", i++);
+ while (np) {
+ pdev = of_find_device_by_node(np);
+ of_node_put(np);
+ if (!pdev)
+ return -EINVAL;
+
+ device_link_add(consumer, &pdev->dev, DL_FLAG_STATELESS);
+ platform_device_put(pdev);
+
+ np = of_parse_phandle(consumer->of_node, "links-add", i++);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_device_links_add);
+
+/**
+ * of_device_links_remove - Remove links between consumer and suppliers from
+ * device tree data
+ *
+ * @consumer: consumer device
+ *
+ * Returns 0 on success, < 0 on failure.
+ */
+int of_device_links_remove(struct device *consumer)
+{
+ struct device_node *np;
+ struct platform_device *pdev;
+ int i = 0;
+
+ np = of_parse_phandle(consumer->of_node, "links-add", i++);
+ while (np) {
+ pdev = of_find_device_by_node(np);
+ of_node_put(np);
+ if (!pdev)
+ return -EINVAL;
+
+ device_link_remove(consumer, &pdev->dev);
+ platform_device_put(pdev);
+
+ np = of_parse_phandle(consumer->of_node, "links-add", i++);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_device_links_remove);
+
+static void devm_of_device_links_remove(struct device *dev, void *res)
+{
+ of_device_links_remove(*(struct device **)res);
+}
+
+/**
+ * devm_of_device_links_add - Create links between consumer and suppliers
+ * from device tree data
+ *
+ * @consumer: consumer device
+ *
+ * Returns 0 on success, < 0 on failure.
+ *
+ * Similar to of_device_links_add(), but will automatically call
+ * of_device_links_remove() when the device is unbound from the bus.
+ */
+int devm_of_device_links_add(struct device *consumer)
+{
+ struct device **ptr;
+ int ret;
+
+ if (!consumer)
+ return -EINVAL;
+
+ ptr = devres_alloc(devm_of_device_links_remove,
+ sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return -ENOMEM;
+
+ ret = of_device_links_add(consumer);
+ if (ret < 0) {
+ devres_free(ptr);
+ } else {
+ *ptr = consumer;
+ devres_add(consumer, ptr);
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_of_device_links_add);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 8d31e39dd564..ad01db6828e8 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -41,6 +41,11 @@ extern int of_device_request_module(struct device *dev);
extern void of_device_uevent(struct device *dev, struct kobj_uevent_env *env);
extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env);
+
+extern int of_device_links_add(struct device *consumer);
+extern int of_device_links_remove(struct device *consumer);
+extern int devm_of_device_links_add(struct device *consumer);
+
static inline void of_device_node_put(struct device *dev)
{
of_node_put(dev->of_node);
@@ -91,6 +96,21 @@ static inline int of_device_uevent_modalias(struct device *dev,
return -ENODEV;
}
+static int of_device_links_add(struct device *consumer)
+{
+ return 0;
+}
+
+static int of_device_links_remove(struct device *consumer)
+{
+ return 0;
+}
+
+static int devm_of_device_links_add(struct device *consumer)
+{
+ return 0;
+}
+
static inline void of_device_node_put(struct device *dev) { }
static inline const struct of_device_id *__of_match_device(
--
2.15.0
^ permalink raw reply related
* [PATCH 0/5] Add of_ functions for device_link_add()
From: Benjamin Gaignard @ 2019-04-24 10:19 UTC (permalink / raw)
To: rafael.j.wysocki, dmitry.torokhov, robh+dt, mark.rutland, hadess,
frowand.list, m.felsch, agx, yannick.fertre, arnd
Cc: linux-input, devicetree, linux-kernel, linux-stm32, broonie,
Benjamin Gaignard
It could happen that we need to control suspend/resume ordering between
devices without obvious consumer/supplier link. For example when touchscreens
and DSI panels share the same reset line, in this case we need to be sure
of pm_runtime operations ordering between those two devices to correctly
perform reset.
DSI panel and touchscreen aren't sharing any heriachical relationship (unlike
I2C client and I2C bus or regulator client and regulator provider) so we need
to describe this in device-tree.
This series introduce of_device_links_{add,remove} and devm_of_device_links_add()
helpers to find and parse 'links-add' property in a device-tree node.
It allows to create and remove links between consumer and suppliers from
device-tree data so consumers will be suspend before their suppliers and resume
after them.
Benjamin Gaignard (3):
of/device: Add of_ functions for device_link_{add,remove}
Input: edt-ft5x06: Document links-add property
Input: goodix: Document links-add property
Yannick Fertré (2):
input: edt-ft5x06 - Call devm_of_device_links_add() to create links
input: goodix - Call devm_of_device_links_add() to create links
.../bindings/input/touchscreen/edt-ft5x06.txt | 2 +
.../bindings/input/touchscreen/goodix.txt | 2 +
drivers/input/touchscreen/edt-ft5x06.c | 2 +
drivers/input/touchscreen/goodix.c | 3 +
drivers/of/device.c | 103 +++++++++++++++++++++
include/linux/of_device.h | 20 ++++
6 files changed, 132 insertions(+)
--
2.15.0
^ permalink raw reply
* Re: [PATCH v10 00/11] mfd: add support for max77650 PMIC
From: Bartosz Golaszewski @ 2019-04-24 8:45 UTC (permalink / raw)
To: Pavel Machek, Lee Jones
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <20190424083127.GB6699@amd>
śr., 24 kwi 2019 o 10:31 Pavel Machek <pavel@denx.de> napisał(a):
>
> On Tue 2019-04-23 11:04:40, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > This series adds support for max77650 ultra low-power PMIC. It provides
> > the core mfd driver and a set of five sub-drivers for the regulator,
> > power supply, gpio, leds and input subsystems.
> >
> > Patches 1-4 add the DT binding documents. Patch 5 documents mfd_add_devices().
> > Patches 6-10 add all drivers. Last patch adds a MAINTAINERS entry for this
> > device.
> >
> > The regulator part is already upstream.
>
> I see v10 in my inbox... and acks from all over the place, but patches
> not going in. Who takes these? I don't think I need another 10
> versions in my inbox...
> Pavel
I think Lee Jones should take them through the MFD tree. With the ack
from Sebastian Reichel on the driver part and the example in the
charger binding fixed I think they are ready to be picked up.
Bart
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox