[PATCH 0/3] RMI4: improve trackpoint detection

[PATCH 0/3] RMI4: improve trackpoint detection

[Dmitry Torokhov]

Hi,

The following patch series is hopefully improves trackpint detection
on systems using RMI4 and PS/2 guest modules. The issue was that we
were registering port too early, before RMI driver was ready to service
interrupts properly, so psmouse would try to get ID from the device and RMI
driver would still be initializing other functions and interrupts would still be
masked, so GetID would simply time out. Then we may or may not get interrupt
from trackpoint and run through the detection again, but probably much later.

Please take a look and hpefully give it a try. Damjan, I hope it might fix your
issue with trackpoint appearing much much later.

Thanks!

-- 
Dmitry

[PATCH 1/3] Input: synaptics_rmi4 - do not delete interrupt memory too early

[Dmitry Torokhov]

We want to free memory reserved for interrupt mask handling only after we
free functions, as function drivers might want to mask interrupts. This is
needed for the followup patch to the F03 that would implement unmasking and
masking interrupts from the serio pass-through port open() and close()
methods.

Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_driver.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 27791d6546c66..ce3ede289ed04 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -41,6 +41,13 @@ void rmi_free_function_list(struct rmi_device *rmi_dev)
 
 	rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev, "Freeing function list\n");
 
+	/* Doing it in the reverse order so F01 will be removed last */
+	list_for_each_entry_safe_reverse(fn, tmp,
+					 &data->function_list, node) {
+		list_del(&fn->node);
+		rmi_unregister_function(fn);
+	}
+
 	devm_kfree(&rmi_dev->dev, data->irq_memory);
 	data->irq_memory = NULL;
 	data->irq_status = NULL;
@@ -50,13 +57,6 @@ void rmi_free_function_list(struct rmi_device *rmi_dev)
 
 	data->f01_container = NULL;
 	data->f34_container = NULL;
-
-	/* Doing it in the reverse order so F01 will be removed last */
-	list_for_each_entry_safe_reverse(fn, tmp,
-					 &data->function_list, node) {
-		list_del(&fn->node);
-		rmi_unregister_function(fn);
-	}
 }
 
 static int reset_one_function(struct rmi_function *fn)
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH 1/3] Input: synaptics_rmi4 - do not delete interrupt memory too early

[thatslyude]

Reviewed-by: Lyude Paul <lyude@redhat.com>
(hope you don't mind that I'm using my gmail address for this ;)

On Thu, 2018-01-18 at 16:49 -0800, Dmitry Torokhov wrote:
> We want to free memory reserved for interrupt mask handling only after we
> free functions, as function drivers might want to mask interrupts. This is
> needed for the followup patch to the F03 that would implement unmasking and
> masking interrupts from the serio pass-through port open() and close()
> methods.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/rmi4/rmi_driver.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_driver.c
> b/drivers/input/rmi4/rmi_driver.c
> index 27791d6546c66..ce3ede289ed04 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -41,6 +41,13 @@ void rmi_free_function_list(struct rmi_device *rmi_dev)
>  
>  	rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev, "Freeing function list\n");
>  
> +	/* Doing it in the reverse order so F01 will be removed last */
> +	list_for_each_entry_safe_reverse(fn, tmp,
> +					 &data->function_list, node) {
> +		list_del(&fn->node);
> +		rmi_unregister_function(fn);
> +	}
> +
>  	devm_kfree(&rmi_dev->dev, data->irq_memory);
>  	data->irq_memory = NULL;
>  	data->irq_status = NULL;
> @@ -50,13 +57,6 @@ void rmi_free_function_list(struct rmi_device *rmi_dev)
>  
>  	data->f01_container = NULL;
>  	data->f34_container = NULL;
> -
> -	/* Doing it in the reverse order so F01 will be removed last */
> -	list_for_each_entry_safe_reverse(fn, tmp,
> -					 &data->function_list, node) {
> -		list_del(&fn->node);
> -		rmi_unregister_function(fn);
> -	}
>  }
>  
>  static int reset_one_function(struct rmi_function *fn)

[PATCH 2/3] Input: synaptics_rmi4 - unmask F03 interrupts when port is opened

[Dmitry Torokhov]

Currently we register the pass-through serio port when we probe the F03 RMI
function, and then, in sensor configure phase, we unmask interrupts.
Unfortunately this is too late, as other drivers are free probe devices
attached to the serio port as soon as it is probed. Because interrupts are
masked, the IO times out, which may result in not being able to detect
trackpoints on the pass-through port.

To fix the issue we implement open() and close() methods for the
pass-through serio port and unmask interrupts from there. We also move
creation of the pass-through port form probe to configure stage, as RMI
driver does not enable transport interrupt until all functions are probed
(we should change this, but this is a separate topic).

We also try to clear the pending data before unmasking interrupts, because
some devices like to spam the system with multiple 0xaa 0x00 announcements,
which may interfere with us trying to query ID of the device.

Fixes: c5e8848fc98e ("Input: synaptics-rmi4 - add support for F03")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f03.c | 64 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 10 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f03.c b/drivers/input/rmi4/rmi_f03.c
index ad71a5e768dc4..7ccbb370a9a81 100644
--- a/drivers/input/rmi4/rmi_f03.c
+++ b/drivers/input/rmi4/rmi_f03.c
@@ -32,6 +32,7 @@ struct f03_data {
 	struct rmi_function *fn;
 
 	struct serio *serio;
+	bool serio_registered;
 
 	unsigned int overwrite_buttons;
 
@@ -138,6 +139,37 @@ static int rmi_f03_initialize(struct f03_data *f03)
 	return 0;
 }
 
+static int rmi_f03_pt_open(struct serio *serio)
+{
+	struct f03_data *f03 = serio->port_data;
+	struct rmi_function *fn = f03->fn;
+	const u8 ob_len = f03->rx_queue_length * RMI_F03_OB_SIZE;
+	const u16 data_addr = fn->fd.data_base_addr + RMI_F03_OB_OFFSET;
+	u8 obs[RMI_F03_QUEUE_LENGTH * RMI_F03_OB_SIZE];
+	int error;
+
+	/*
+	 * Consume any pending data. Some devices like to spam with
+	 * 0xaa 0x00 announcements which may confuse us as we try to
+	 * probe the device.
+	 */
+	error = rmi_read_block(fn->rmi_dev, data_addr, &obs, ob_len);
+	if (!error)
+		rmi_dbg(RMI_DEBUG_FN, &fn->dev,
+			"%s: Consumed %*ph (%d) from PS2 guest\n",
+			__func__, ob_len, obs, ob_len);
+
+	return fn->rmi_dev->driver->set_irq_bits(fn->rmi_dev, fn->irq_mask);
+}
+
+static void rmi_f03_pt_close(struct serio *serio)
+{
+	struct f03_data *f03 = serio->port_data;
+	struct rmi_function *fn = f03->fn;
+
+	fn->rmi_dev->driver->clear_irq_bits(fn->rmi_dev, fn->irq_mask);
+}
+
 static int rmi_f03_register_pt(struct f03_data *f03)
 {
 	struct serio *serio;
@@ -148,6 +180,8 @@ static int rmi_f03_register_pt(struct f03_data *f03)
 
 	serio->id.type = SERIO_PS_PSTHRU;
 	serio->write = rmi_f03_pt_write;
+	serio->open = rmi_f03_pt_open;
+	serio->close = rmi_f03_pt_close;
 	serio->port_data = f03;
 
 	strlcpy(serio->name, "Synaptics RMI4 PS/2 pass-through",
@@ -184,17 +218,27 @@ static int rmi_f03_probe(struct rmi_function *fn)
 			 f03->device_count);
 
 	dev_set_drvdata(dev, f03);
-
-	error = rmi_f03_register_pt(f03);
-	if (error)
-		return error;
-
 	return 0;
 }
 
 static int rmi_f03_config(struct rmi_function *fn)
 {
-	fn->rmi_dev->driver->set_irq_bits(fn->rmi_dev, fn->irq_mask);
+	struct f03_data *f03 = dev_get_drvdata(&fn->dev);
+	int error;
+
+	if (!f03->serio_registered) {
+		error = rmi_f03_register_pt(f03);
+		if (error)
+			return error;
+
+		f03->serio_registered = true;
+	} else {
+		/*
+		 * We must be re-configuring the sensor, just enable
+		 * interrupts for this function.
+		 */
+		fn->rmi_dev->driver->set_irq_bits(fn->rmi_dev, fn->irq_mask);
+	}
 
 	return 0;
 }
@@ -204,7 +248,7 @@ static int rmi_f03_attention(struct rmi_function *fn, unsigned long *irq_bits)
 	struct rmi_device *rmi_dev = fn->rmi_dev;
 	struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
 	struct f03_data *f03 = dev_get_drvdata(&fn->dev);
-	u16 data_addr = fn->fd.data_base_addr;
+	const u16 data_addr = fn->fd.data_base_addr + RMI_F03_OB_OFFSET;
 	const u8 ob_len = f03->rx_queue_length * RMI_F03_OB_SIZE;
 	u8 obs[RMI_F03_QUEUE_LENGTH * RMI_F03_OB_SIZE];
 	u8 ob_status;
@@ -226,8 +270,7 @@ static int rmi_f03_attention(struct rmi_function *fn, unsigned long *irq_bits)
 		drvdata->attn_data.size -= ob_len;
 	} else {
 		/* Grab all of the data registers, and check them for data */
-		error = rmi_read_block(fn->rmi_dev, data_addr + RMI_F03_OB_OFFSET,
-				       &obs, ob_len);
+		error = rmi_read_block(fn->rmi_dev, data_addr, &obs, ob_len);
 		if (error) {
 			dev_err(&fn->dev,
 				"%s: Failed to read F03 output buffers: %d\n",
@@ -266,7 +309,8 @@ static void rmi_f03_remove(struct rmi_function *fn)
 {
 	struct f03_data *f03 = dev_get_drvdata(&fn->dev);
 
-	serio_unregister_port(f03->serio);
+	if (f03->serio_registered)
+		serio_unregister_port(f03->serio);
 }
 
 struct rmi_function_handler rmi_f03_handler = {
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH 2/3] Input: synaptics_rmi4 - unmask F03 interrupts when port is opened

[thatslyude]

Looks good to me.

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Thu, 2018-01-18 at 16:49 -0800, Dmitry Torokhov wrote:
> Currently we register the pass-through serio port when we probe the F03 RMI
> function, and then, in sensor configure phase, we unmask interrupts.
> Unfortunately this is too late, as other drivers are free probe devices
> attached to the serio port as soon as it is probed. Because interrupts are
> masked, the IO times out, which may result in not being able to detect
> trackpoints on the pass-through port.
> 
> To fix the issue we implement open() and close() methods for the
> pass-through serio port and unmask interrupts from there. We also move
> creation of the pass-through port form probe to configure stage, as RMI
> driver does not enable transport interrupt until all functions are probed
> (we should change this, but this is a separate topic).
> 
> We also try to clear the pending data before unmasking interrupts, because
> some devices like to spam the system with multiple 0xaa 0x00 announcements,
> which may interfere with us trying to query ID of the device.
> 
> Fixes: c5e8848fc98e ("Input: synaptics-rmi4 - add support for F03")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/rmi4/rmi_f03.c | 64 +++++++++++++++++++++++++++++++++++++--
> -----
>  1 file changed, 54 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_f03.c b/drivers/input/rmi4/rmi_f03.c
> index ad71a5e768dc4..7ccbb370a9a81 100644
> --- a/drivers/input/rmi4/rmi_f03.c
> +++ b/drivers/input/rmi4/rmi_f03.c
> @@ -32,6 +32,7 @@ struct f03_data {
>  	struct rmi_function *fn;
>  
>  	struct serio *serio;
> +	bool serio_registered;
>  
>  	unsigned int overwrite_buttons;
>  
> @@ -138,6 +139,37 @@ static int rmi_f03_initialize(struct f03_data *f03)
>  	return 0;
>  }
>  
> +static int rmi_f03_pt_open(struct serio *serio)
> +{
> +	struct f03_data *f03 = serio->port_data;
> +	struct rmi_function *fn = f03->fn;
> +	const u8 ob_len = f03->rx_queue_length * RMI_F03_OB_SIZE;
> +	const u16 data_addr = fn->fd.data_base_addr + RMI_F03_OB_OFFSET;
> +	u8 obs[RMI_F03_QUEUE_LENGTH * RMI_F03_OB_SIZE];
> +	int error;
> +
> +	/*
> +	 * Consume any pending data. Some devices like to spam with
> +	 * 0xaa 0x00 announcements which may confuse us as we try to
> +	 * probe the device.
> +	 */
> +	error = rmi_read_block(fn->rmi_dev, data_addr, &obs, ob_len);
> +	if (!error)
> +		rmi_dbg(RMI_DEBUG_FN, &fn->dev,
> +			"%s: Consumed %*ph (%d) from PS2 guest\n",
> +			__func__, ob_len, obs, ob_len);
> +
> +	return fn->rmi_dev->driver->set_irq_bits(fn->rmi_dev, fn-
> >irq_mask);
> +}
> +
> +static void rmi_f03_pt_close(struct serio *serio)
> +{
> +	struct f03_data *f03 = serio->port_data;
> +	struct rmi_function *fn = f03->fn;
> +
> +	fn->rmi_dev->driver->clear_irq_bits(fn->rmi_dev, fn->irq_mask);
> +}
> +
>  static int rmi_f03_register_pt(struct f03_data *f03)
>  {
>  	struct serio *serio;
> @@ -148,6 +180,8 @@ static int rmi_f03_register_pt(struct f03_data *f03)
>  
>  	serio->id.type = SERIO_PS_PSTHRU;
>  	serio->write = rmi_f03_pt_write;
> +	serio->open = rmi_f03_pt_open;
> +	serio->close = rmi_f03_pt_close;
>  	serio->port_data = f03;
>  
>  	strlcpy(serio->name, "Synaptics RMI4 PS/2 pass-through",
> @@ -184,17 +218,27 @@ static int rmi_f03_probe(struct rmi_function *fn)
>  			 f03->device_count);
>  
>  	dev_set_drvdata(dev, f03);
> -
> -	error = rmi_f03_register_pt(f03);
> -	if (error)
> -		return error;
> -
>  	return 0;
>  }
>  
>  static int rmi_f03_config(struct rmi_function *fn)
>  {
> -	fn->rmi_dev->driver->set_irq_bits(fn->rmi_dev, fn->irq_mask);
> +	struct f03_data *f03 = dev_get_drvdata(&fn->dev);
> +	int error;
> +
> +	if (!f03->serio_registered) {
> +		error = rmi_f03_register_pt(f03);
> +		if (error)
> +			return error;
> +
> +		f03->serio_registered = true;
> +	} else {
> +		/*
> +		 * We must be re-configuring the sensor, just enable
> +		 * interrupts for this function.
> +		 */
> +		fn->rmi_dev->driver->set_irq_bits(fn->rmi_dev, fn-
> >irq_mask);
> +	}
>  
>  	return 0;
>  }
> @@ -204,7 +248,7 @@ static int rmi_f03_attention(struct rmi_function *fn,
> unsigned long *irq_bits)
>  	struct rmi_device *rmi_dev = fn->rmi_dev;
>  	struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
>  	struct f03_data *f03 = dev_get_drvdata(&fn->dev);
> -	u16 data_addr = fn->fd.data_base_addr;
> +	const u16 data_addr = fn->fd.data_base_addr + RMI_F03_OB_OFFSET;
>  	const u8 ob_len = f03->rx_queue_length * RMI_F03_OB_SIZE;
>  	u8 obs[RMI_F03_QUEUE_LENGTH * RMI_F03_OB_SIZE];
>  	u8 ob_status;
> @@ -226,8 +270,7 @@ static int rmi_f03_attention(struct rmi_function *fn,
> unsigned long *irq_bits)
>  		drvdata->attn_data.size -= ob_len;
>  	} else {
>  		/* Grab all of the data registers, and check them for data
> */
> -		error = rmi_read_block(fn->rmi_dev, data_addr +
> RMI_F03_OB_OFFSET,
> -				       &obs, ob_len);
> +		error = rmi_read_block(fn->rmi_dev, data_addr, &obs,
> ob_len);
>  		if (error) {
>  			dev_err(&fn->dev,
>  				"%s: Failed to read F03 output buffers:
> %d\n",
> @@ -266,7 +309,8 @@ static void rmi_f03_remove(struct rmi_function *fn)
>  {
>  	struct f03_data *f03 = dev_get_drvdata(&fn->dev);
>  
> -	serio_unregister_port(f03->serio);
> +	if (f03->serio_registered)
> +		serio_unregister_port(f03->serio);
>  }
>  
>  struct rmi_function_handler rmi_f03_handler = {

[PATCH 3/3] Input: synaptics-rmi4 - log when we create a guest serio port

[Dmitry Torokhov]

To ease analyzing boot behavior from logs, let's log when we are about to
register the pass-through serio port.

Also, let's drop "Synaptics" prefix from the port name, as RMI4 is good
enough indicator already, and having the prefix means that the name does
not fit into serio->name field. While at it move from hard-coded seio->phys
to one mentioning the sensor ID (such as "rmi4-00.fn03/serio0").

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f03.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f03.c b/drivers/input/rmi4/rmi_f03.c
index 7ccbb370a9a81..88822196d6b72 100644
--- a/drivers/input/rmi4/rmi_f03.c
+++ b/drivers/input/rmi4/rmi_f03.c
@@ -184,14 +184,15 @@ static int rmi_f03_register_pt(struct f03_data *f03)
 	serio->close = rmi_f03_pt_close;
 	serio->port_data = f03;
 
-	strlcpy(serio->name, "Synaptics RMI4 PS/2 pass-through",
-		sizeof(serio->name));
-	strlcpy(serio->phys, "synaptics-rmi4-pt/serio1",
-		sizeof(serio->phys));
+	strlcpy(serio->name, "RMI4 PS/2 pass-through", sizeof(serio->name));
+	snprintf(serio->phys, sizeof(serio->phys), "%s/serio0",
+		 dev_name(&f03->fn->dev));
 	serio->dev.parent = &f03->fn->dev;
 
 	f03->serio = serio;
 
+	printk(KERN_INFO "serio: %s port at %s\n",
+		serio->name, dev_name(&f03->fn->dev));
 	serio_register_port(serio);
 
 	return 0;
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH 3/3] Input: synaptics-rmi4 - log when we create a guest serio port

[thatslyude]

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Thu, 2018-01-18 at 16:49 -0800, Dmitry Torokhov wrote:
> To ease analyzing boot behavior from logs, let's log when we are about to
> register the pass-through serio port.
> 
> Also, let's drop "Synaptics" prefix from the port name, as RMI4 is good
> enough indicator already, and having the prefix means that the name does
> not fit into serio->name field. While at it move from hard-coded seio->phys
> to one mentioning the sensor ID (such as "rmi4-00.fn03/serio0").
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/rmi4/rmi_f03.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_f03.c b/drivers/input/rmi4/rmi_f03.c
> index 7ccbb370a9a81..88822196d6b72 100644
> --- a/drivers/input/rmi4/rmi_f03.c
> +++ b/drivers/input/rmi4/rmi_f03.c
> @@ -184,14 +184,15 @@ static int rmi_f03_register_pt(struct f03_data *f03)
>  	serio->close = rmi_f03_pt_close;
>  	serio->port_data = f03;
>  
> -	strlcpy(serio->name, "Synaptics RMI4 PS/2 pass-through",
> -		sizeof(serio->name));
> -	strlcpy(serio->phys, "synaptics-rmi4-pt/serio1",
> -		sizeof(serio->phys));
> +	strlcpy(serio->name, "RMI4 PS/2 pass-through", sizeof(serio-
> >name));
> +	snprintf(serio->phys, sizeof(serio->phys), "%s/serio0",
> +		 dev_name(&f03->fn->dev));
>  	serio->dev.parent = &f03->fn->dev;
>  
>  	f03->serio = serio;
>  
> +	printk(KERN_INFO "serio: %s port at %s\n",
> +		serio->name, dev_name(&f03->fn->dev));
>  	serio_register_port(serio);
>  
>  	return 0;