linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.
@ 2024-08-05  8:36 Marge Yang
  2024-08-05 17:31 ` Dmitry Torokhov
  2024-09-03 18:07 ` Richard Acayan
  0 siblings, 2 replies; 7+ messages in thread
From: Marge Yang @ 2024-08-05  8:36 UTC (permalink / raw)
  To: dmitry.torokhov, linux-input, linux-kernel, vincent.huang,
	marge.yang
  Cc: david.chiu, derek.cheng, sam.tsai, Vincent Huang

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)) {
+			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);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.
  2024-08-05  8:36 [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value Marge Yang
@ 2024-08-05 17:31 ` Dmitry Torokhov
  2024-08-06  0:56   ` Derek Cheng
  2024-09-03 18:07 ` Richard Acayan
  1 sibling, 1 reply; 7+ messages in thread
From: Dmitry Torokhov @ 2024-08-05 17:31 UTC (permalink / raw)
  To: Marge Yang
  Cc: linux-input, linux-kernel, vincent.huang, david.chiu, derek.cheng,
	sam.tsai

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.
  2024-08-05 17:31 ` Dmitry Torokhov
@ 2024-08-06  0:56   ` Derek Cheng
  0 siblings, 0 replies; 7+ messages in thread
From: Derek Cheng @ 2024-08-06  0:56 UTC (permalink / raw)
  To: Dmitry Torokhov, Marge Yang
  Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	Vincent Huang, David Chiu, Sam Tsai

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.
  2024-08-05  8:36 [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value Marge Yang
  2024-08-05 17:31 ` Dmitry Torokhov
@ 2024-09-03 18:07 ` Richard Acayan
  2024-09-03 18:40   ` Dmitry Torokhov
  1 sibling, 1 reply; 7+ messages in thread
From: Richard Acayan @ 2024-09-03 18:07 UTC (permalink / raw)
  To: Marge Yang, Dmitry Torokhov, linux-input, linux-kernel,
	Vincent Huang
  Cc: david.chiu, derek.cheng, sam.tsai

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.
  2024-09-03 18:07 ` Richard Acayan
@ 2024-09-03 18:40   ` Dmitry Torokhov
  2024-09-03 21:33     ` Richard Acayan
  0 siblings, 1 reply; 7+ messages in thread
From: Dmitry Torokhov @ 2024-09-03 18:40 UTC (permalink / raw)
  To: Richard Acayan
  Cc: Marge Yang, linux-input, linux-kernel, Vincent Huang, david.chiu,
	derek.cheng, sam.tsai

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.
  2024-09-03 18:40   ` Dmitry Torokhov
@ 2024-09-03 21:33     ` Richard Acayan
  2024-09-03 22:03       ` Dmitry Torokhov
  0 siblings, 1 reply; 7+ messages in thread
From: Richard Acayan @ 2024-09-03 21:33 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Marge Yang, linux-input, linux-kernel, Vincent Huang, david.chiu,
	derek.cheng, sam.tsai

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value.
  2024-09-03 21:33     ` Richard Acayan
@ 2024-09-03 22:03       ` Dmitry Torokhov
  0 siblings, 0 replies; 7+ messages in thread
From: Dmitry Torokhov @ 2024-09-03 22:03 UTC (permalink / raw)
  To: Richard Acayan
  Cc: Marge Yang, linux-input, linux-kernel, Vincent Huang, david.chiu,
	derek.cheng, sam.tsai

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!

-- 
Dmitry

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-09-03 22:03 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-05  8:36 [PATCH V2] Input: synaptics-rmi4 - Supports to query DPM value Marge Yang
2024-08-05 17:31 ` Dmitry Torokhov
2024-08-06  0:56   ` Derek Cheng
2024-09-03 18:07 ` Richard Acayan
2024-09-03 18:40   ` Dmitry Torokhov
2024-09-03 21:33     ` Richard Acayan
2024-09-03 22:03       ` Dmitry Torokhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).