diff mbox series

[V2] Input: synaptics-rmi4 - Supports to query DPM value.

Message ID 20240805083636.1381205-1-marge.yang@tw.synaptics.com
State New
Headers show
Series [V2] Input: synaptics-rmi4 - Supports to query DPM value. | expand

Commit Message

Marge Yang Aug. 5, 2024, 8:36 a.m. UTC 
Newer firmware allows to query touchpad resolution
information by reading from resolution register.
Presence of resolution register is signalled via bit
29 of the "register presence" register.
On devices that lack this resolution register
we fall back to using pitch and number of receivers
data to calculate size of the sensor.

RMI4 F12 will support to query DPM value on Touchpad.
When TP firmware doesn't support to report logical and
physical value within the Touchpad's HID report.
We can directly query the DPM value through RMI.

Signed-off-by: Marge Yang <marge.yang@tw.synaptics.com>
Signed-off-by: Vincent Huang <Vincent.Huang@tw.synaptics.com>
---
 drivers/input/rmi4/rmi_f12.c | 41 +++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 10 deletions(-)

Comments

Derek Cheng Aug. 6, 2024, 12:56 a.m. UTC | #1 
Thanks Dmitry.

Regards,
Derek
Synaptics

-----Original Message-----
From: Dmitry Torokhov <dmitry.torokhov@gmail.com> 
Sent: Tuesday, August 6, 2024 1:32 AM
To: Marge Yang <Marge.Yang@tw.synaptics.com>
Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Vincent Huang <Vincent.huang@tw.synaptics.com>; David Chiu <David.Chiu@tw.synaptics.com>; Derek Cheng <derek.cheng@tw.synaptics.com>; Sam Tsai <Sam.Tsai@synaptics.com>
Subject: Re: [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.

CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.


On Mon, Aug 05, 2024 at 08:36:36AM +0000, Marge Yang wrote:
> Newer firmware allows to query touchpad resolution information by 
> reading from resolution register.
> Presence of resolution register is signalled via bit
> 29 of the "register presence" register.
> On devices that lack this resolution register we fall back to using 
> pitch and number of receivers data to calculate size of the sensor.
>
> RMI4 F12 will support to query DPM value on Touchpad.
> When TP firmware doesn't support to report logical and physical value 
> within the Touchpad's HID report.
> We can directly query the DPM value through RMI.
>
> Signed-off-by: Marge Yang <marge.yang@tw.synaptics.com>
> Signed-off-by: Vincent Huang <Vincent.Huang@tw.synaptics.com>

Applied, thank you.

--
Dmitry
Richard Acayan Sept. 3, 2024, 6:07 p.m. UTC | #2 
Hi,

On Mon, Aug 05, 2024 at 08:36:36AM +0000, Marge Yang wrote:
> Newer firmware allows to query touchpad resolution
> information by reading from resolution register.
> Presence of resolution register is signalled via bit
> 29 of the "register presence" register.
> On devices that lack this resolution register
> we fall back to using pitch and number of receivers
> data to calculate size of the sensor.
> 
> RMI4 F12 will support to query DPM value on Touchpad.
> When TP firmware doesn't support to report logical and
> physical value within the Touchpad's HID report.
> We can directly query the DPM value through RMI.
> 
> Signed-off-by: Marge Yang <marge.yang@tw.synaptics.com>
> Signed-off-by: Vincent Huang <Vincent.Huang@tw.synaptics.com>
> ---
>  drivers/input/rmi4/rmi_f12.c | 41 +++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
> index 7e97944f7616..b8dfb9ffdbf5 100644
> --- a/drivers/input/rmi4/rmi_f12.c
> +++ b/drivers/input/rmi4/rmi_f12.c
> @@ -24,6 +24,7 @@ enum rmi_f12_object_type {
>  };
>  
>  #define F12_DATA1_BYTES_PER_OBJ			8
> +#define RMI_F12_QUERY_RESOLUTION		29
>  
>  struct f12_data {
>  	struct rmi_2d_sensor sensor;
> @@ -73,6 +74,8 @@ static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
>  	int pitch_y = 0;
>  	int rx_receivers = 0;
>  	int tx_receivers = 0;
> +	u16 query_dpm_addr = 0;
> +	int dpm_resolution = 0;
>  
>  	item = rmi_get_register_desc_item(&f12->control_reg_desc, 8);
>  	if (!item) {
> @@ -122,18 +125,36 @@ static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
>  		offset += 4;
>  	}
>  
> -	if (rmi_register_desc_has_subpacket(item, 3)) {
> -		rx_receivers = buf[offset];
> -		tx_receivers = buf[offset + 1];
> -		offset += 2;
> -	}
> +	/* Use the Query DPM feature when the query register exists for resolution. */
> +	item = rmi_get_register_desc_item(&f12->query_reg_desc, RMI_F12_QUERY_RESOLUTION);
> +	if (item) {
> +		offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
> +			RMI_F12_QUERY_RESOLUTION);
> +		query_dpm_addr = fn->fd.query_base_addr	+ offset;
> +		ret = rmi_read(fn->rmi_dev, query_dpm_addr, buf);
> +		if (ret < 0) {
> +			dev_err(&fn->dev, "Failed to read DPM value: %d\n", ret);
> +			return -ENODEV;
> +		}
> +		dpm_resolution = buf[0];
> +
> +		sensor->x_mm = sensor->max_x / dpm_resolution;
> +		sensor->y_mm = sensor->max_y / dpm_resolution;
> +	} else {
> +		if (rmi_register_desc_has_subpacket(item, 3)) {

The item variable is NULL in this branch, as it was overwritten just
before the if statement.

This patch causes a NULL pointer dereference:

	[    1.738650] rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: Synaptics, product: S3706B, fw id: 2869618
	[    1.771232] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000018
	[    1.771245] Mem abort info:
	[    1.771248]   ESR = 0x0000000096000004
	[    1.771254]   EC = 0x25: DABT (current EL), IL = 32 bits
	[    1.771261]   SET = 0, FnV = 0
	[    1.771266]   EA = 0, S1PTW = 0
	[    1.771271]   FSC = 0x04: level 0 translation fault
	[    1.771276] Data abort info:
	[    1.771280]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
	[    1.771285]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
	[    1.771291]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
	[    1.771298] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000986a9000
	[    1.771308] [0000000000000018] pgd=0000000000000000, p4d=0000000000000000
	[    1.771326] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
	[    1.771335] Modules linked in: rmi_i2c(+) rmi_core gpi
	[    1.771358] CPU: 1 UID: 0 PID: 165 Comm: udevd Not tainted 6.11.0-rc6-next-20240902-sdm670-00022-g6ab596a153e1-dirty #10
	[    1.771371] Hardware name: Google Inc. MSM sdm670 S4 PVT v1.0 (DT)
	[    1.771378] pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
	[    1.771389] pc : _find_next_bit+0x18/0x78
	[    1.771411] lr : rmi_register_desc_has_subpacket+0x24/0x40 [rmi_core]
	[    1.771444] sp : ffff800080fdb0b0
	[    1.771448] x29: ffff800080fdb0b0 x28: 0000000000000000 x27: 000000000000000c
	[    1.771463] x26: ffff614ec34aa880 x25: ffff800080fdb0f9 x24: ffff614ec34aa958
	[    1.771477] x23: ffff614ec34adc00 x22: ffff614ed8a02880 x21: ffff614ec34aa9c8
	[    1.771492] x20: ffff614ec34adc18 x19: 0000000000000003 x18: 000000005e77a5e3
	[    1.771506] x17: 000000040044ffff x16: ffffb02de6d83ba8 x15: 0000000000000000
	[    1.771520] x14: ffff614ec1a85640 x13: ffff614ec96dc580 x12: 000000003474591d
	[    1.771534] x11: 0000000068d948e4 x10: ffffb02d74a1c000 x9 : 0000000000000000
	[    1.771548] x8 : ffff614ec96dc500 x7 : 0000000000000001 x6 : 0000000000000001
	[    1.771561] x5 : 0000000000000000 x4 : fffffffffffffff8 x3 : 0000000000000000
	[    1.771574] x2 : 0000000000000003 x1 : 0000000000000100 x0 : 0000000000000018
	[    1.771588] Call trace:
	[    1.771593]  _find_next_bit+0x18/0x78
	[    1.771608]  rmi_f12_probe+0x728/0x960 [rmi_core]
	[    1.771632]  rmi_function_probe+0x8c/0x20c [rmi_core]
	[    1.771654]  really_probe+0xbc/0x2c0
	[    1.771671]  __driver_probe_device+0x78/0x120
	[    1.771686]  driver_probe_device+0x3c/0x154
	[    1.771699]  __device_attach_driver+0xb8/0x140
	[    1.771713]  bus_for_each_drv+0x88/0xe8
	[    1.771727]  __device_attach+0xa0/0x190
	[    1.771735]  device_initial_probe+0x14/0x20
	[    1.771744]  bus_probe_device+0xb4/0xc0
	[    1.771757]  device_add+0x578/0x750
	[    1.771769]  rmi_register_function+0x64/0xc8 [rmi_core]
	[    1.771792]  rmi_create_function+0x11c/0x1c4 [rmi_core]
	[    1.771815]  rmi_scan_pdt+0x90/0x1e4 [rmi_core]
	[    1.771837]  rmi_init_functions+0x6c/0x13c [rmi_core]
	[    1.771860]  rmi_driver_probe+0x130/0x3a0 [rmi_core]
	[    1.771882]  really_probe+0xbc/0x2c0
	[    1.771896]  __driver_probe_device+0x78/0x120
	[    1.771911]  driver_probe_device+0x3c/0x154
	[    1.771925]  __device_attach_driver+0xb8/0x140
	[    1.771939]  bus_for_each_drv+0x88/0xe8
	[    1.771952]  __device_attach+0xa0/0x190
	[    1.771960]  device_initial_probe+0x14/0x20
	[    1.771970]  bus_probe_device+0xb4/0xc0
	[    1.771983]  device_add+0x578/0x750
	[    1.771994]  rmi_register_transport_device+0x8c/0x138 [rmi_core]
	[    1.772017]  rmi_i2c_probe+0x1b0/0x304 [rmi_i2c]
	[    1.772040]  i2c_device_probe+0x130/0x290
	[    1.772056]  really_probe+0xbc/0x2c0
	[    1.772070]  __driver_probe_device+0x78/0x120
	[    1.772084]  driver_probe_device+0x3c/0x154
	[    1.772098]  __driver_attach+0x90/0x1a0
	[    1.772111]  bus_for_each_dev+0x7c/0xe0
	[    1.772124]  driver_attach+0x24/0x30
	[    1.772137]  bus_add_driver+0xe4/0x208
	[    1.772150]  driver_register+0x68/0x124
	[    1.772159]  i2c_register_driver+0x48/0xcc
	[    1.772173]  rmi_i2c_driver_init+0x20/0x1000 [rmi_i2c]
	[    1.772185]  do_one_initcall+0x60/0x1d4
	[    1.772198]  do_init_module+0x5c/0x21c
	[    1.772211]  load_module+0x18cc/0x1e60
	[    1.772222]  init_module_from_file+0x88/0xd0
	[    1.772234]  __arm64_sys_finit_module+0x1c0/0x320
	[    1.772246]  invoke_syscall+0x48/0x104
	[    1.772261]  el0_svc_common.constprop.0+0x40/0xe0
	[    1.772274]  do_el0_svc+0x1c/0x28
	[    1.772287]  el0_svc+0x34/0xe0
	[    1.772298]  el0t_64_sync_handler+0x120/0x12c
	[    1.772309]  el0t_64_sync+0x190/0x194
	[    1.772324] Code: d346fc43 92800004 9ac22084 d37df065 (f8656802) 
	[    1.772331] ---[ end trace 0000000000000000 ]---
Dmitry Torokhov Sept. 3, 2024, 6:40 p.m. UTC | #3 
On Tue, Sep 03, 2024 at 02:07:23PM -0400, Richard Acayan wrote:
> > +	/* Use the Query DPM feature when the query register exists for resolution. */
> > +	item = rmi_get_register_desc_item(&f12->query_reg_desc, RMI_F12_QUERY_RESOLUTION);
> > +	if (item) {
> > +		offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
> > +			RMI_F12_QUERY_RESOLUTION);
> > +		query_dpm_addr = fn->fd.query_base_addr	+ offset;
> > +		ret = rmi_read(fn->rmi_dev, query_dpm_addr, buf);
> > +		if (ret < 0) {
> > +			dev_err(&fn->dev, "Failed to read DPM value: %d\n", ret);
> > +			return -ENODEV;
> > +		}
> > +		dpm_resolution = buf[0];
> > +
> > +		sensor->x_mm = sensor->max_x / dpm_resolution;
> > +		sensor->y_mm = sensor->max_y / dpm_resolution;
> > +	} else {
> > +		if (rmi_register_desc_has_subpacket(item, 3)) {
> 
> The item variable is NULL in this branch, as it was overwritten just
> before the if statement.
> 
> This patch causes a NULL pointer dereference:

Ugh, indeed. I guess the simplest way of fixing this would be:

diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
index fc2cc8e2b0ba..8246fe77114b 100644
--- a/drivers/input/rmi4/rmi_f12.c
+++ b/drivers/input/rmi4/rmi_f12.c
@@ -129,9 +129,8 @@ static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
 	 * Use the Query DPM feature when the resolution query register
 	 * exists.
 	 */
-	item = rmi_get_register_desc_item(&f12->query_reg_desc,
-					  RMI_F12_QUERY_RESOLUTION);
-	if (item) {
+	if (rmi_get_register_desc_item(&f12->query_reg_desc,
+				       RMI_F12_QUERY_RESOLUTION)) {
 		offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
 						RMI_F12_QUERY_RESOLUTION);
 		query_dpm_addr = fn->fd.query_base_addr	+ offset;

Could you please tell me if this works for you?

Thanks.
Dmitry Torokhov Sept. 3, 2024, 10:03 p.m. UTC | #4 
On Tue, Sep 03, 2024 at 05:33:23PM -0400, Richard Acayan wrote:
> On Tue, Sep 03, 2024 at 11:40:38AM -0700, Dmitry Torokhov wrote:
> > On Tue, Sep 03, 2024 at 02:07:23PM -0400, Richard Acayan wrote:
> > > > +	/* Use the Query DPM feature when the query register exists for resolution. */
> > > > +	item = rmi_get_register_desc_item(&f12->query_reg_desc, RMI_F12_QUERY_RESOLUTION);
> > > > +	if (item) {
> > > > +		offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
> > > > +			RMI_F12_QUERY_RESOLUTION);
> > > > +		query_dpm_addr = fn->fd.query_base_addr	+ offset;
> > > > +		ret = rmi_read(fn->rmi_dev, query_dpm_addr, buf);
> > > > +		if (ret < 0) {
> > > > +			dev_err(&fn->dev, "Failed to read DPM value: %d\n", ret);
> > > > +			return -ENODEV;
> > > > +		}
> > > > +		dpm_resolution = buf[0];
> > > > +
> > > > +		sensor->x_mm = sensor->max_x / dpm_resolution;
> > > > +		sensor->y_mm = sensor->max_y / dpm_resolution;
> > > > +	} else {
> > > > +		if (rmi_register_desc_has_subpacket(item, 3)) {
> > > 
> > > The item variable is NULL in this branch, as it was overwritten just
> > > before the if statement.
> > > 
> > > This patch causes a NULL pointer dereference:
> > 
> > Ugh, indeed. I guess the simplest way of fixing this would be:
> > 
> > diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
> > index fc2cc8e2b0ba..8246fe77114b 100644
> > --- a/drivers/input/rmi4/rmi_f12.c
> > +++ b/drivers/input/rmi4/rmi_f12.c
> > @@ -129,9 +129,8 @@ static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
> >  	 * Use the Query DPM feature when the resolution query register
> >  	 * exists.
> >  	 */
> > -	item = rmi_get_register_desc_item(&f12->query_reg_desc,
> > -					  RMI_F12_QUERY_RESOLUTION);
> > -	if (item) {
> > +	if (rmi_get_register_desc_item(&f12->query_reg_desc,
> > +				       RMI_F12_QUERY_RESOLUTION)) {
> >  		offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
> >  						RMI_F12_QUERY_RESOLUTION);
> >  		query_dpm_addr = fn->fd.query_base_addr	+ offset;
> > 
> > Could you please tell me if this works for you?
> 
> Yeah, it fixes the bug.

Great, thank you for reporting and testing!
diff mbox series

Patch

diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
index 7e97944f7616..b8dfb9ffdbf5 100644
--- a/drivers/input/rmi4/rmi_f12.c
+++ b/drivers/input/rmi4/rmi_f12.c
@@ -24,6 +24,7 @@  enum rmi_f12_object_type {
 };
 
 #define F12_DATA1_BYTES_PER_OBJ			8
+#define RMI_F12_QUERY_RESOLUTION		29
 
 struct f12_data {
 	struct rmi_2d_sensor sensor;
@@ -73,6 +74,8 @@  static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
 	int pitch_y = 0;
 	int rx_receivers = 0;
 	int tx_receivers = 0;
+	u16 query_dpm_addr = 0;
+	int dpm_resolution = 0;
 
 	item = rmi_get_register_desc_item(&f12->control_reg_desc, 8);
 	if (!item) {
@@ -122,18 +125,36 @@  static int rmi_f12_read_sensor_tuning(struct f12_data *f12)
 		offset += 4;
 	}
 
-	if (rmi_register_desc_has_subpacket(item, 3)) {
-		rx_receivers = buf[offset];
-		tx_receivers = buf[offset + 1];
-		offset += 2;
-	}
+	/* Use the Query DPM feature when the query register exists for resolution. */
+	item = rmi_get_register_desc_item(&f12->query_reg_desc, RMI_F12_QUERY_RESOLUTION);
+	if (item) {
+		offset = rmi_register_desc_calc_reg_offset(&f12->query_reg_desc,
+			RMI_F12_QUERY_RESOLUTION);
+		query_dpm_addr = fn->fd.query_base_addr	+ offset;
+		ret = rmi_read(fn->rmi_dev, query_dpm_addr, buf);
+		if (ret < 0) {
+			dev_err(&fn->dev, "Failed to read DPM value: %d\n", ret);
+			return -ENODEV;
+		}
+		dpm_resolution = buf[0];
+
+		sensor->x_mm = sensor->max_x / dpm_resolution;
+		sensor->y_mm = sensor->max_y / dpm_resolution;
+	} else {
+		if (rmi_register_desc_has_subpacket(item, 3)) {
+			rx_receivers = buf[offset];
+			tx_receivers = buf[offset + 1];
+			offset += 2;
+		}
 
-	/* Skip over sensor flags */
-	if (rmi_register_desc_has_subpacket(item, 4))
-		offset += 1;
+		/* Skip over sensor flags */
+		if (rmi_register_desc_has_subpacket(item, 4))
+			offset += 1;
+
+		sensor->x_mm = (pitch_x * rx_receivers) >> 12;
+		sensor->y_mm = (pitch_y * tx_receivers) >> 12;
+	}
 
-	sensor->x_mm = (pitch_x * rx_receivers) >> 12;
-	sensor->y_mm = (pitch_y * tx_receivers) >> 12;
 
 	rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s: x_mm: %d y_mm: %d\n", __func__,
 		sensor->x_mm, sensor->y_mm);