[PATCH 1/2] i2c: core: Allow the driver to override the default i2c_bus match behavior

[PATCH 1/2] i2c: core: Allow the driver to override the default i2c_bus match behavior

[Hans de Goede]

Some ACPI devices report multiple ids for a single i2c_client, while not
really implementing the hw-interface asociated with some of these ids.

For some of these devices calling probe and having probe fail with
-ENODEV is a problem in itself as this causes the device to be
powered-up and down again (causes its PS0 and PS3 ACPI methods to be
executed) which puts some devices in an unusable state.

This commit adds a match callback to i2c_driver, allowing drivers to
override the default i2c_bus match behavior and tell the core they
are not the right driver for the device, avoiding i2c_bus_type.probe
getting called, avoiding the undesirable power up / down cycle.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-base.c | 12 +++++++++---
 include/linux/i2c.h         |  6 ++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index c89dac7fd2e7..2c6702f2347b 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -98,8 +98,16 @@ EXPORT_SYMBOL_GPL(i2c_match_id);
 static int i2c_device_match(struct device *dev, struct device_driver *drv)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
-	struct i2c_driver	*driver;
+	struct i2c_driver	*driver = to_i2c_driver(drv);
+	int ret;
 
+	if (driver->match && client) {
+		ret = driver->match(client);
+		if (ret < 0)
+			return 0;
+		if (ret > 0)
+			return 1;
+	}
 
 	/* Attempt an OF style match */
 	if (i2c_of_match_device(drv->of_match_table, client))
@@ -109,8 +117,6 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
 	if (acpi_driver_match_device(dev, drv))
 		return 1;
 
-	driver = to_i2c_driver(drv);
-
 	/* Finally an I2C match */
 	if (i2c_match_id(driver->id_table, client))
 		return 1;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 00ca5b86a753..670577c82bc3 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -139,6 +139,9 @@ enum i2c_alert_protocol {
  * struct i2c_driver - represent an I2C device driver
  * @class: What kind of i2c device we instantiate (for detect)
  * @attach_adapter: Callback for bus addition (deprecated)
+ * @match: Allows the driver to override the default i2c_bus match behavior
+ *         return < 0 to fail the match, > 0 to force a match, 0 to fallback
+ *         to default id matching
  * @probe: Callback for device binding - soon to be deprecated
  * @probe_new: New callback for device binding
  * @remove: Callback for device unbinding
@@ -180,6 +183,9 @@ struct i2c_driver {
 	 */
 	int (*attach_adapter)(struct i2c_adapter *) __deprecated;
 
+	/* Set this to override standard i2c_bus match behavior */
+	int (*match)(struct i2c_client *);
+
 	/* Standard driver model interfaces */
 	int (*probe)(struct i2c_client *, const struct i2c_device_id *);
 	int (*remove)(struct i2c_client *);
-- 
2.13.0

[PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Hans de Goede]

The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
it uses its own protocol which is handled by the chipone_icn8318 driver.

If the i2c_hid_driver's probe functon gets called it will fail with a
"hid_descr_cmd failed" error.

Worse, after the probe failure the i2c / ACPI core code will put the ACPI
device in D3 state and when the chipone_icn8318 driver then loads the
device is put back in D0 state, executing its PS0 ACPI method, which
resets the controller, causing the controller to loose its firmware
which was loaded by the BIOS. The chipone_icn8318 driver has a workaround
for this, but that requires it to be the only (or the first) driver to
probe the device.

This commit adds a match callback and returns -ENODEV for i2c_client-s
with a CHPN0001 ACPI device id, so that the probe function never gets
called, fixing the controller losing its firmware.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Use the new i2c_driver match callback to only filter out the CHPN0001
 ACPI device, rather then use acpi_dev_present in probe and not
 registering the driver at all when the system has a CHPN0001 device
---
 drivers/hid/i2c-hid/i2c-hid.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 046f692fd0a2..79bed9afc388 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -891,6 +891,26 @@ static void i2c_hid_acpi_fix_up_power(struct device *dev)
 		acpi_device_fix_up_power(adev);
 }
 
+static const struct acpi_device_id i2c_hid_acpi_blacklist[] = {
+	/*
+	 * The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID
+	 * compatible, just probing it puts the device in an unusable state due
+	 * to it also have ACPI power management issues.
+	 */
+	{"CHPN0001", 0 },
+	{ },
+};
+
+static int i2c_hid_match(struct i2c_client *client)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+
+	if (adev && acpi_match_device_ids(adev, i2c_hid_acpi_blacklist) == 0)
+		return -ENODEV;
+
+	return 0;
+}
+
 static const struct acpi_device_id i2c_hid_acpi_match[] = {
 	{"ACPI0C50", 0 },
 	{"PNP0C50", 0 },
@@ -905,6 +925,7 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client,
 }
 
 static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {}
+static int i2c_hid_match(struct i2c_client *client) { return 0; }
 #endif
 
 #ifdef CONFIG_OF
@@ -1255,6 +1276,7 @@ static struct i2c_driver i2c_hid_driver = {
 		.of_match_table = of_match_ptr(i2c_hid_of_match),
 	},
 
+	.match		= i2c_hid_match,
 	.probe		= i2c_hid_probe,
 	.remove		= i2c_hid_remove,
 	.shutdown	= i2c_hid_shutdown,
-- 
2.13.0

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Benjamin Tissoires]

On Jul 22 2017 or thereabouts, Hans de Goede wrote:
> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
> it uses its own protocol which is handled by the chipone_icn8318 driver.
> 
> If the i2c_hid_driver's probe functon gets called it will fail with a
> "hid_descr_cmd failed" error.
> 
> Worse, after the probe failure the i2c / ACPI core code will put the ACPI
> device in D3 state and when the chipone_icn8318 driver then loads the
> device is put back in D0 state, executing its PS0 ACPI method, which
> resets the controller, causing the controller to loose its firmware
> which was loaded by the BIOS. The chipone_icn8318 driver has a workaround
> for this, but that requires it to be the only (or the first) driver to
> probe the device.
> 
> This commit adds a match callback and returns -ENODEV for i2c_client-s
> with a CHPN0001 ACPI device id, so that the probe function never gets
> called, fixing the controller losing its firmware.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Use the new i2c_driver match callback to only filter out the CHPN0001
>  ACPI device, rather then use acpi_dev_present in probe and not
>  registering the driver at all when the system has a CHPN0001 device

I like the v2 much more than the v1.

Reviewed-By: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 046f692fd0a2..79bed9afc388 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -891,6 +891,26 @@ static void i2c_hid_acpi_fix_up_power(struct device *dev)
>  		acpi_device_fix_up_power(adev);
>  }
>  
> +static const struct acpi_device_id i2c_hid_acpi_blacklist[] = {
> +	/*
> +	 * The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID
> +	 * compatible, just probing it puts the device in an unusable state due
> +	 * to it also have ACPI power management issues.
> +	 */
> +	{"CHPN0001", 0 },
> +	{ },
> +};
> +
> +static int i2c_hid_match(struct i2c_client *client)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> +
> +	if (adev && acpi_match_device_ids(adev, i2c_hid_acpi_blacklist) == 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
>  static const struct acpi_device_id i2c_hid_acpi_match[] = {
>  	{"ACPI0C50", 0 },
>  	{"PNP0C50", 0 },
> @@ -905,6 +925,7 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client,
>  }
>  
>  static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {}
> +static int i2c_hid_match(struct i2c_client *client) { return 0; }
>  #endif
>  
>  #ifdef CONFIG_OF
> @@ -1255,6 +1276,7 @@ static struct i2c_driver i2c_hid_driver = {
>  		.of_match_table = of_match_ptr(i2c_hid_of_match),
>  	},
>  
> +	.match		= i2c_hid_match,
>  	.probe		= i2c_hid_probe,
>  	.remove		= i2c_hid_remove,
>  	.shutdown	= i2c_hid_shutdown,
> -- 
> 2.13.0
> 

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Jiri Kosina]

On Mon, 24 Jul 2017, Benjamin Tissoires wrote:

> > The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
> > it uses its own protocol which is handled by the chipone_icn8318 driver.
> > 
> > If the i2c_hid_driver's probe functon gets called it will fail with a
> > "hid_descr_cmd failed" error.
> > 
> > Worse, after the probe failure the i2c / ACPI core code will put the ACPI
> > device in D3 state and when the chipone_icn8318 driver then loads the
> > device is put back in D0 state, executing its PS0 ACPI method, which
> > resets the controller, causing the controller to loose its firmware
> > which was loaded by the BIOS. The chipone_icn8318 driver has a workaround
> > for this, but that requires it to be the only (or the first) driver to
> > probe the device.
> > 
> > This commit adds a match callback and returns -ENODEV for i2c_client-s
> > with a CHPN0001 ACPI device id, so that the probe function never gets
> > called, fixing the controller losing its firmware.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > Changes in v2:
> > -Use the new i2c_driver match callback to only filter out the CHPN0001
> >  ACPI device, rather then use acpi_dev_present in probe and not
> >  registering the driver at all when the system has a CHPN0001 device
> 
> I like the v2 much more than the v1.
> 
> Reviewed-By: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Wolfram,

what do we do with this patchset? I think it makes sense for both patches 
to go in via one tree.

Either I can take it through hid.git with your Ack for the first patch, or 
vice versa. Please let me know what you'd prefer.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]


> what do we do with this patchset? I think it makes sense for both patches 
> to go in via one tree.

Yes.

> Either I can take it through hid.git with your Ack for the first patch, or 
> vice versa. Please let me know what you'd prefer.

I'd like use the i2c tree for it. The changes to i2c core are an API
addition while the changes to the HID driver are basically boilerplate.

Makes sense?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Jiri Kosina]

On Tue, 25 Jul 2017, Wolfram Sang wrote:

> > Either I can take it through hid.git with your Ack for the first patch, or 
> > vice versa. Please let me know what you'd prefer.
> 
> I'd like use the i2c tree for it. The changes to i2c core are an API
> addition while the changes to the HID driver are basically boilerplate.
> 
> Makes sense?

Absolutely.

Please feel free to add

	Reviewed-by: Jiri Kosina <jkosina@suse.cz>

to patch 2/2.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] i2c: core: Allow the driver to override the default i2c_bus match behavior

[Hans de Goede]

Hi,

On 22-07-17 20:55, Hans de Goede wrote:
> Some ACPI devices report multiple ids for a single i2c_client, while not
> really implementing the hw-interface asociated with some of these ids.
> 
> For some of these devices calling probe and having probe fail with
> -ENODEV is a problem in itself as this causes the device to be
> powered-up and down again (causes its PS0 and PS3 ACPI methods to be
> executed) which puts some devices in an unusable state.
> 
> This commit adds a match callback to i2c_driver, allowing drivers to
> override the default i2c_bus match behavior and tell the core they
> are not the right driver for the device, avoiding i2c_bus_type.probe
> getting called, avoiding the undesirable power up / down cycle.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

What is the status of this series ? I thought there was agreement
on merging this through the i2c (wsa/linux.git) tree ?

Regards,

Hans


> ---
>   drivers/i2c/i2c-core-base.c | 12 +++++++++---
>   include/linux/i2c.h         |  6 ++++++
>   2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index c89dac7fd2e7..2c6702f2347b 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -98,8 +98,16 @@ EXPORT_SYMBOL_GPL(i2c_match_id);
>   static int i2c_device_match(struct device *dev, struct device_driver *drv)
>   {
>   	struct i2c_client	*client = i2c_verify_client(dev);
> -	struct i2c_driver	*driver;
> +	struct i2c_driver	*driver = to_i2c_driver(drv);
> +	int ret;
>   
> +	if (driver->match && client) {
> +		ret = driver->match(client);
> +		if (ret < 0)
> +			return 0;
> +		if (ret > 0)
> +			return 1;
> +	}
>   
>   	/* Attempt an OF style match */
>   	if (i2c_of_match_device(drv->of_match_table, client))
> @@ -109,8 +117,6 @@ static int i2c_device_match(struct device *dev, struct device_driver *drv)
>   	if (acpi_driver_match_device(dev, drv))
>   		return 1;
>   
> -	driver = to_i2c_driver(drv);
> -
>   	/* Finally an I2C match */
>   	if (i2c_match_id(driver->id_table, client))
>   		return 1;
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 00ca5b86a753..670577c82bc3 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -139,6 +139,9 @@ enum i2c_alert_protocol {
>    * struct i2c_driver - represent an I2C device driver
>    * @class: What kind of i2c device we instantiate (for detect)
>    * @attach_adapter: Callback for bus addition (deprecated)
> + * @match: Allows the driver to override the default i2c_bus match behavior
> + *         return < 0 to fail the match, > 0 to force a match, 0 to fallback
> + *         to default id matching
>    * @probe: Callback for device binding - soon to be deprecated
>    * @probe_new: New callback for device binding
>    * @remove: Callback for device unbinding
> @@ -180,6 +183,9 @@ struct i2c_driver {
>   	 */
>   	int (*attach_adapter)(struct i2c_adapter *) __deprecated;
>   
> +	/* Set this to override standard i2c_bus match behavior */
> +	int (*match)(struct i2c_client *);
> +
>   	/* Standard driver model interfaces */
>   	int (*probe)(struct i2c_client *, const struct i2c_device_id *);
>   	int (*remove)(struct i2c_client *);
> 

Re: [PATCH 1/2] i2c: core: Allow the driver to override the default i2c_bus match behavior

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On Mon, Aug 14, 2017 at 10:13:23PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 22-07-17 20:55, Hans de Goede wrote:
> >Some ACPI devices report multiple ids for a single i2c_client, while not
> >really implementing the hw-interface asociated with some of these ids.
> >
> >For some of these devices calling probe and having probe fail with
> >-ENODEV is a problem in itself as this causes the device to be
> >powered-up and down again (causes its PS0 and PS3 ACPI methods to be
> >executed) which puts some devices in an unusable state.
> >
> >This commit adds a match callback to i2c_driver, allowing drivers to
> >override the default i2c_bus match behavior and tell the core they
> >are not the right driver for the device, avoiding i2c_bus_type.probe
> >getting called, avoiding the undesirable power up / down cycle.
> >
> >Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> What is the status of this series ? I thought there was agreement
> on merging this through the i2c (wsa/linux.git) tree ?

Yes, but I need to review the I2C core changes first.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]

Hey guys,

Sorry, I don't understand some of the stuff here. But I'd like to
understand it before I add something to the I2C core. Especially as it
feels a bit a the edge of the driver model to me.

On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote:
> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
> it uses its own protocol which is handled by the chipone_icn8318 driver.
> 
> If the i2c_hid_driver's probe functon gets called it will fail with a
> "hid_descr_cmd failed" error.

That sounds like it fails pretty late. I'd assume we could check the
blacklist right at the beginning of probe and bail out immediately?

> Worse, after the probe failure the i2c / ACPI core code will put the ACPI
> device in D3 state

Where does that happen? Sorry, I can't find it. Would it be an idea to
add a flag somewhere telling the device should not be put into D3? That
would be way more generic in case this happens outside I2C world, or?
Disclaimer: I am brainstorming here, don't know super much about ACPI.

> This commit adds a match callback and returns -ENODEV for i2c_client-s
> with a CHPN0001 ACPI device id, so that the probe function never gets
> called, fixing the controller losing its firmware.

Do you know if something like a match-callback exists somewhere else in
the kernel?

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Dmitry Torokhov]

On Thu, Aug 17, 2017 at 12:39 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> Hey guys,
>
> Sorry, I don't understand some of the stuff here. But I'd like to
> understand it before I add something to the I2C core. Especially as it
> feels a bit a the edge of the driver model to me.
>
> On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote:
>> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
>> it uses its own protocol which is handled by the chipone_icn8318 driver.
>>
>> If the i2c_hid_driver's probe functon gets called it will fail with a
>> "hid_descr_cmd failed" error.
>
> That sounds like it fails pretty late. I'd assume we could check the
> blacklist right at the beginning of probe and bail out immediately?
>
>> Worse, after the probe failure the i2c / ACPI core code will put the ACPI
>> device in D3 state

So I guess this will also happen if I unbind/rebind chipone_icn8318 driver?

>
> Where does that happen? Sorry, I can't find it. Would it be an idea to
> add a flag somewhere telling the device should not be put into D3? That
> would be way more generic in case this happens outside I2C world, or?
> Disclaimer: I am brainstorming here, don't know super much about ACPI.
>
>> This commit adds a match callback and returns -ENODEV for i2c_client-s
>> with a CHPN0001 ACPI device id, so that the probe function never gets
>> called, fixing the controller losing its firmware.
>
> Do you know if something like a match-callback exists somewhere else in
> the kernel?

Having blacklist means that we are failing at design somewhere...

In this case what we have is wrong ACPI table. Instead of adding hooks
to i2c core can we fix it (DSDT) up? I know people do not like it, but
I'd say this is task for yet another platform driver to adjust ACPI
properties (kill the wrong _CID, disable PS0) before instantiating the
device.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Hans de Goede]

Hi,

On 17-08-17 21:39, Wolfram Sang wrote:
> Hey guys,
> 
> Sorry, I don't understand some of the stuff here. But I'd like to
> understand it before I add something to the I2C core. Especially as it
> feels a bit a the edge of the driver model to me.
> 
> On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote:
>> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
>> it uses its own protocol which is handled by the chipone_icn8318 driver.
>>
>> If the i2c_hid_driver's probe functon gets called it will fail with a
>> "hid_descr_cmd failed" error.
> 
> That sounds like it fails pretty late. I'd assume we could check the
> blacklist right at the beginning of probe and bail out immediately?

The problem is that calling probe (and then failing) leads to the device-core
powering up and then powering down (put in D0 / D3 state) the device, after
which the touchscreen controller has lost its firmware.

So the trick is to get the device-core to never call i2c_bus_type.probe
for i2c-hid at all, which means that i2c_bus_type.match must return false
in this case.

>> Worse, after the probe failure the i2c / ACPI core code will put the ACPI
>> device in D3 state
> 
> Where does that happen? Sorry, I can't find it. Would it be an idea to
> add a flag somewhere telling the device should not be put into D3? That
> would be way more generic in case this happens outside I2C world, or?
> Disclaimer: I am brainstorming here, don't know super much about ACPI.

i2c_device_probe() calls dev_pm_domain_attach(&client->dev, true)
which calls acpi_dev_pm_attach(dev, true) which then does a
acpi_dev_pm_full_power(adev) moving the device to D0 (if it was not
in D0 already).

When the probe fails i2c_device_probe() then does
dev_pm_domain_detach(&client->dev, true); which calls
acpi_dev_pm_detach(dev, true) which does acpi_dev_pm_low_power(...)
and now the device is in D3.

>> This commit adds a match callback and returns -ENODEV for i2c_client-s
>> with a CHPN0001 ACPI device id, so that the probe function never gets
>> called, fixing the controller losing its firmware.
> 
> Do you know if something like a match-callback exists somewhere else in
> the kernel?

No AFAIK there is no driver level match callback (only bus level) at
other places in the kernel.

Regards,

Hans

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Hans de Goede]

Hi again,

I realized I did not answer 1 of your questions:

On 17-08-17 21:39, Wolfram Sang wrote:
> Hey guys,
> 
> Sorry, I don't understand some of the stuff here. But I'd like to
> understand it before I add something to the I2C core. Especially as it
> feels a bit a the edge of the driver model to me.
> 
> On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote:
>> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
>> it uses its own protocol which is handled by the chipone_icn8318 driver.
>>
>> If the i2c_hid_driver's probe functon gets called it will fail with a
>> "hid_descr_cmd failed" error.
> 
> That sounds like it fails pretty late. I'd assume we could check the
> blacklist right at the beginning of probe and bail out immediately?
> 
>> Worse, after the probe failure the i2c / ACPI core code will put the ACPI
>> device in D3 state
> 
> Where does that happen? Sorry, I can't find it. Would it be an idea to
> add a flag somewhere telling the device should not be put into D3?

It is already possible to do this and my patches for the icn8318 driver
do this:

	struct acpi_device *adev;

	adev = ACPI_COMPANION(dev);

	/*
	 * Disable ACPI power management the _PS3 method is empty, so
	 * there is no powersaving when using ACPI power management.
	 * The _PS0 method resets the controller causing it to loose its
	 * firmware, which has been loaded by the BIOS and we do not
	 * know how to restore the firmware.
	 */
	adev->flags.power_manageable = 0;

The problem is that this happens in the probe() from the icn8318 driver
and if the i2c-hid drivers probe() executes first we end up in the
dev_pm_domain_detach() path of i2c_device_probe() and after that the
touchscreen-controller no longer works (*), iow after that it is too late
to disable acpi pm for the device.

*) Unless we find a way to reload the firmware, which technically is
doable, but then we get into the problem of now having to distribute the
firmware in linux-firmware

Regards,

Hans

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Hans de Goede]

Hi,

On 18-08-17 00:15, Dmitry Torokhov wrote:
> On Thu, Aug 17, 2017 at 12:39 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> Hey guys,
>>
>> Sorry, I don't understand some of the stuff here. But I'd like to
>> understand it before I add something to the I2C core. Especially as it
>> feels a bit a the edge of the driver model to me.
>>
>> On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote:
>>> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
>>> it uses its own protocol which is handled by the chipone_icn8318 driver.
>>>
>>> If the i2c_hid_driver's probe functon gets called it will fail with a
>>> "hid_descr_cmd failed" error.
>>
>> That sounds like it fails pretty late. I'd assume we could check the
>> blacklist right at the beginning of probe and bail out immediately?
>>
>>> Worse, after the probe failure the i2c / ACPI core code will put the ACPI
>>> device in D3 state
> 
> So I guess this will also happen if I unbind/rebind chipone_icn8318 driver?

No because on probe the chipone_icn8318 driver does (with the patches
I've pending) :

     struct acpi_device *adev;

     adev = ACPI_COMPANION(dev);

     /*
      * Disable ACPI power management the _PS3 method is empty, so
      * there is no powersaving when using ACPI power management.
      * The _PS0 method resets the controller causing it to loose its
      * firmware, which has been loaded by the BIOS and we do not
      * know how to restore the firmware.
      */
     adev->flags.power_manageable = 0;

So it disables ACPI pm for the device, keeping the controller in D0 so that
it will never loose its firmware.


>> Where does that happen? Sorry, I can't find it. Would it be an idea to
>> add a flag somewhere telling the device should not be put into D3? That
>> would be way more generic in case this happens outside I2C world, or?
>> Disclaimer: I am brainstorming here, don't know super much about ACPI.
>>
>>> This commit adds a match callback and returns -ENODEV for i2c_client-s
>>> with a CHPN0001 ACPI device id, so that the probe function never gets
>>> called, fixing the controller losing its firmware.
>>
>> Do you know if something like a match-callback exists somewhere else in
>> the kernel?
> 
> Having blacklist means that we are failing at design somewhere...
> 
> In this case what we have is wrong ACPI table. Instead of adding hooks
> to i2c core can we fix it (DSDT) up? I know people do not like it, but
> I'd say this is task for yet another platform driver to adjust ACPI
> properties (kill the wrong _CID, disable PS0) before instantiating the
> device.

I don't think that is going to fly. Certainly DSDT patching is out of the
question, we could modify the acpi_dev after enumeration, but that is not
going to be pretty. This is going to be drivers/platform/x86/silead_dmi.c
all over again, but where that made sense as to decouple the board-info
from the driver this case is different as we are not talking board
specific behavior here, but device specific so this really belongs in the
drivers IMHO, also the reception of silead_dmi.c has not been 100% positive.

I don't think the blacklist entry in i2c-hid is a problem per se, we've
similar issues with USB devices where they claim a generic class but need
a specific driver and we typically solve that with a blacklist for the
specific vid:pid in the class driver.

The nastiness here is not the blacklist, but the pm issues when the wrong
driver gets its probe() method called first.

Regards,

Hans

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Dmitry Torokhov]

Hi Hans,

On Mon, Aug 28, 2017 at 03:04:04PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 18-08-17 00:15, Dmitry Torokhov wrote:
> >On Thu, Aug 17, 2017 at 12:39 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> >>Hey guys,
> >>
> >>Sorry, I don't understand some of the stuff here. But I'd like to
> >>understand it before I add something to the I2C core. Especially as it
> >>feels a bit a the edge of the driver model to me.
> >>
> >>On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote:
> >>>The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
> >>>it uses its own protocol which is handled by the chipone_icn8318 driver.
> >>>
> >>>If the i2c_hid_driver's probe functon gets called it will fail with a
> >>>"hid_descr_cmd failed" error.
> >>
> >>That sounds like it fails pretty late. I'd assume we could check the
> >>blacklist right at the beginning of probe and bail out immediately?
> >>
> >>>Worse, after the probe failure the i2c / ACPI core code will put the ACPI
> >>>device in D3 state
> >
> >So I guess this will also happen if I unbind/rebind chipone_icn8318 driver?
> 
> No because on probe the chipone_icn8318 driver does (with the patches
> I've pending) :
> 
>     struct acpi_device *adev;
> 
>     adev = ACPI_COMPANION(dev);
> 
>     /*
>      * Disable ACPI power management the _PS3 method is empty, so
>      * there is no powersaving when using ACPI power management.
>      * The _PS0 method resets the controller causing it to loose its
>      * firmware, which has been loaded by the BIOS and we do not
>      * know how to restore the firmware.
>      */
>     adev->flags.power_manageable = 0;
> 
> So it disables ACPI pm for the device, keeping the controller in D0 so that
> it will never loose its firmware.
> 
> 
> >>Where does that happen? Sorry, I can't find it. Would it be an idea to
> >>add a flag somewhere telling the device should not be put into D3? That
> >>would be way more generic in case this happens outside I2C world, or?
> >>Disclaimer: I am brainstorming here, don't know super much about ACPI.
> >>
> >>>This commit adds a match callback and returns -ENODEV for i2c_client-s
> >>>with a CHPN0001 ACPI device id, so that the probe function never gets
> >>>called, fixing the controller losing its firmware.
> >>
> >>Do you know if something like a match-callback exists somewhere else in
> >>the kernel?
> >
> >Having blacklist means that we are failing at design somewhere...
> >
> >In this case what we have is wrong ACPI table. Instead of adding hooks
> >to i2c core can we fix it (DSDT) up? I know people do not like it, but
> >I'd say this is task for yet another platform driver to adjust ACPI
> >properties (kill the wrong _CID, disable PS0) before instantiating the
> >device.
> 
> I don't think that is going to fly. Certainly DSDT patching is out of the
> question, we could modify the acpi_dev after enumeration, but that is not
> going to be pretty. This is going to be drivers/platform/x86/silead_dmi.c
> all over again, but where that made sense as to decouple the board-info
> from the driver this case is different as we are not talking board
> specific behavior here, but device specific so this really belongs in the
> drivers IMHO, also the reception of silead_dmi.c has not been 100% positive.

No, this is definitely board-specific as well. This is a brain dead
firmware on a particular board that declares PS0 and wrong CID. The
device itself can be made working perfectly well in another box.

I think I also mentioned before that encoding particular platform
behavior in the device driver is not the best option.

I understand that nobody likes silead_dmi as it is nasty (as it shoudl
be, it fixes missing/wrong data suplied by the platform), but I think
the main objection is that we had to make it built-in. I wonder if we
could work on making it usable as a module, by having driver core try
loading "<device-alias>-quirk" module before doing probes so that quirk
is guaranteed to be available?

> 
> I don't think the blacklist entry in i2c-hid is a problem per se, we've
> similar issues with USB devices where they claim a generic class but need
> a specific driver and we typically solve that with a blacklist for the
> specific vid:pid in the class driver.

Yes, you do that for a particular _device_. You move this device to a
brand new box and you still need the same quirk. Here it is the box that
is deficient.

> 
> The nastiness here is not the blacklist, but the pm issues when the wrong
> driver gets its probe() method called first.

BTW, how do we order probing? Should we try matching on _HID before
trying _CID?

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Hans de Goede]

Hi Dmitry,

On 28-08-17 18:31, Dmitry Torokhov wrote:
> Hi Hans,
> 
> On Mon, Aug 28, 2017 at 03:04:04PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 18-08-17 00:15, Dmitry Torokhov wrote:
>>> On Thu, Aug 17, 2017 at 12:39 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>>>> Hey guys,
>>>>
>>>> Sorry, I don't understand some of the stuff here. But I'd like to
>>>> understand it before I add something to the I2C core. Especially as it
>>>> feels a bit a the edge of the driver model to me.
>>>>
>>>> On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote:
>>>>> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
>>>>> it uses its own protocol which is handled by the chipone_icn8318 driver.
>>>>>
>>>>> If the i2c_hid_driver's probe functon gets called it will fail with a
>>>>> "hid_descr_cmd failed" error.
>>>>
>>>> That sounds like it fails pretty late. I'd assume we could check the
>>>> blacklist right at the beginning of probe and bail out immediately?
>>>>
>>>>> Worse, after the probe failure the i2c / ACPI core code will put the ACPI
>>>>> device in D3 state
>>>
>>> So I guess this will also happen if I unbind/rebind chipone_icn8318 driver?
>>
>> No because on probe the chipone_icn8318 driver does (with the patches
>> I've pending) :
>>
>>      struct acpi_device *adev;
>>
>>      adev = ACPI_COMPANION(dev);
>>
>>      /*
>>       * Disable ACPI power management the _PS3 method is empty, so
>>       * there is no powersaving when using ACPI power management.
>>       * The _PS0 method resets the controller causing it to loose its
>>       * firmware, which has been loaded by the BIOS and we do not
>>       * know how to restore the firmware.
>>       */
>>      adev->flags.power_manageable = 0;
>>
>> So it disables ACPI pm for the device, keeping the controller in D0 so that
>> it will never loose its firmware.
>>
>>
>>>> Where does that happen? Sorry, I can't find it. Would it be an idea to
>>>> add a flag somewhere telling the device should not be put into D3? That
>>>> would be way more generic in case this happens outside I2C world, or?
>>>> Disclaimer: I am brainstorming here, don't know super much about ACPI.
>>>>
>>>>> This commit adds a match callback and returns -ENODEV for i2c_client-s
>>>>> with a CHPN0001 ACPI device id, so that the probe function never gets
>>>>> called, fixing the controller losing its firmware.
>>>>
>>>> Do you know if something like a match-callback exists somewhere else in
>>>> the kernel?
>>>
>>> Having blacklist means that we are failing at design somewhere...
>>>
>>> In this case what we have is wrong ACPI table. Instead of adding hooks
>>> to i2c core can we fix it (DSDT) up? I know people do not like it, but
>>> I'd say this is task for yet another platform driver to adjust ACPI
>>> properties (kill the wrong _CID, disable PS0) before instantiating the
>>> device.
>>
>> I don't think that is going to fly. Certainly DSDT patching is out of the
>> question, we could modify the acpi_dev after enumeration, but that is not
>> going to be pretty. This is going to be drivers/platform/x86/silead_dmi.c
>> all over again, but where that made sense as to decouple the board-info
>> from the driver this case is different as we are not talking board
>> specific behavior here, but device specific so this really belongs in the
>> drivers IMHO, also the reception of silead_dmi.c has not been 100% positive.
> 
> No, this is definitely board-specific as well. This is a brain dead
> firmware on a particular board that declares PS0 and wrong CID. The
> device itself can be made working perfectly well in another box.
I believe all x86 tablet models with this touchscreen controller have
the same issues, so although technically this is a board problem we
might just as well treat it as a device problem.

> I think I also mentioned before that encoding particular platform
> behavior in the device driver is not the best option.
> 
> I understand that nobody likes silead_dmi as it is nasty (as it shoudl
> be, it fixes missing/wrong data suplied by the platform), but I think
> the main objection is that we had to make it built-in. I wonder if we
> could work on making it usable as a module, by having driver core try
> loading "<device-alias>-quirk" module before doing probes so that quirk
> is guaranteed to be available?

That is tricky when using an initrd, what if the initial attempt to
load "<device-alias>-quirk" fails because we are running from the initrd
should we retry ? When ? Etc.

> 
>>
>> I don't think the blacklist entry in i2c-hid is a problem per se, we've
>> similar issues with USB devices where they claim a generic class but need
>> a specific driver and we typically solve that with a blacklist for the
>> specific vid:pid in the class driver.
> 
> Yes, you do that for a particular _device_. You move this device to a
> brand new box and you still need the same quirk. Here it is the box that
> is deficient.

See above to the best of my knowledge all models using this touchscreen
controller have the same issue.

>> The nastiness here is not the blacklist, but the pm issues when the wrong
>> driver gets its probe() method called first.
> 
> BTW, how do we order probing? Should we try matching on _HID before
> trying _CID?

That won't help much, udev ends up loading the modules based on a modalias
so playing tricks with ordering is hard (and again we may have an initrd
making things interesting, or have the wrong driver built in).

Regards,

Hans

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Hans de Goede]

Hi,

On 28-08-17 14:50, Hans de Goede wrote:
> Hi again,
> 
> I realized I did not answer 1 of your questions:
> 
> On 17-08-17 21:39, Wolfram Sang wrote:
>> Hey guys,
>>
>> Sorry, I don't understand some of the stuff here. But I'd like to
>> understand it before I add something to the I2C core. Especially as it
>> feels a bit a the edge of the driver model to me.
>>
>> On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote:
>>> The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
>>> it uses its own protocol which is handled by the chipone_icn8318 driver.
>>>
>>> If the i2c_hid_driver's probe functon gets called it will fail with a
>>> "hid_descr_cmd failed" error.
>>
>> That sounds like it fails pretty late. I'd assume we could check the
>> blacklist right at the beginning of probe and bail out immediately?
>>
>>> Worse, after the probe failure the i2c / ACPI core code will put the ACPI
>>> device in D3 state
>>
>> Where does that happen? Sorry, I can't find it. Would it be an idea to
>> add a flag somewhere telling the device should not be put into D3?
> 
> It is already possible to do this and my patches for the icn8318 driver
> do this:
> 
>      struct acpi_device *adev;
> 
>      adev = ACPI_COMPANION(dev);
> 
>      /*
>       * Disable ACPI power management the _PS3 method is empty, so
>       * there is no powersaving when using ACPI power management.
>       * The _PS0 method resets the controller causing it to loose its
>       * firmware, which has been loaded by the BIOS and we do not
>       * know how to restore the firmware.
>       */
>      adev->flags.power_manageable = 0;
> 
> The problem is that this happens in the probe() from the icn8318 driver
> and if the i2c-hid drivers probe() executes first we end up in the
> dev_pm_domain_detach() path of i2c_device_probe() and after that the
> touchscreen-controller no longer works (*), iow after that it is too late
> to disable acpi pm for the device.

So thinking more about this it might be cleaner to add a blacklist
of _CID / _HID ACPI-ids for which power-management should be disabled
to drivers/acpi/device_pm.c : acpi_bus_init_power().

When I've some time to look into this I will write a patch following
that approach.

So lets forget about the approach to add an i2c_driver match callback
for now.

Regards,

Hans


> *) Unless we find a way to reload the firmware, which technically is
> doable, but then we get into the problem of now having to distribute the
> firmware in linux-firmware

Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]


> So thinking more about this it might be cleaner to add a blacklist
> of _CID / _HID ACPI-ids for which power-management should be disabled
> to drivers/acpi/device_pm.c : acpi_bus_init_power().
> 
> When I've some time to look into this I will write a patch following
> that approach.
> 
> So lets forget about the approach to add an i2c_driver match callback
> for now.

Thanks for the update! That sounds better to me from the high level view
that I have. Thanks to you and Dmitry for discussing the details
throughly. Very much appreciated!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]