[PATCH 0/2] HID: Intel-thc-hid: Support complete I2C bus configuration

[PATCH 0/2] HID: Intel-thc-hid: Support complete I2C bus configuration

[Even Xu]

The Intel THC QuickI2C driver currently configures only the target
address, bus speed, and SCL high and low periods. Additional timing
parameters supplied by firmware are not applied to the I2C sub-IP.

Introduce a common I2C configuration structure in the THC core and use
it to configure the target address, addressing mode, bus speed, SCL
timings, and SDA transmit and receive hold periods. Populate this
structure from the QuickI2C ACPI resources so that platform-specific
timings are restored during both initialization and resume.

Even Xu (2):
  HID: Intel-thc-hid: Intel-thc: Refactor I2C bus configuration with
    unified config structure
  HID: Intel-thc-hid: Intel-quicki2c: Support full I2C BUS config
    parameters

 .../intel-quicki2c/pci-quicki2c.c             | 46 +++++++------
 .../intel-quicki2c/quicki2c-dev.h             | 14 ++--
 .../intel-thc-hid/intel-thc/intel-thc-dev.c   | 69 ++++++++++++-------
 .../intel-thc-hid/intel-thc/intel-thc-dev.h   | 23 ++++++-
 .../intel-thc-hid/intel-thc/intel-thc-hw.h    |  3 +
 5 files changed, 97 insertions(+), 58 deletions(-)

-- 
2.43.0

[PATCH 1/2] HID: Intel-thc-hid: Intel-thc: Refactor I2C bus configuration with unified config structure

[Even Xu]

Introduce a new struct thc_i2c_config to consolidate all configurable
I2C bus parameters into a single structure for better maintainability
and extensibility.

Changes include:
- Add struct thc_i2c_config to encapsulate I2C bus parameters
- Rename thc_i2c_subip_set_speed() to thc_i2c_subip_bus_config() to
  better reflect its expanded functionality
- Update thc_i2c_subip_bus_config() to accept struct thc_i2c_config
  parameter for comprehensive I2C parameter configuration
- Modify thc_i2c_subip_init() to use struct thc_i2c_config and call
  thc_i2c_subip_bus_config() for complete bus initialization

This refactoring improves code organization and unifies I2C configuration
parameters.

Signed-off-by: Even Xu <even.xu@intel.com>
---
 .../intel-thc-hid/intel-thc/intel-thc-dev.c   | 69 ++++++++++++-------
 .../intel-thc-hid/intel-thc/intel-thc-dev.h   | 23 ++++++-
 .../intel-thc-hid/intel-thc/intel-thc-hw.h    |  3 +
 3 files changed, 68 insertions(+), 27 deletions(-)

diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c
index 9a8449428170..7b4a58e1416d 100644
--- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c
+++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c
@@ -1422,14 +1422,25 @@ static int thc_i2c_subip_pio_write(struct thc_device *dev, const u32 address,
 #define I2C_SUBIP_DMA_TDLR_DEFAULT	7
 #define I2C_SUBIP_DMA_RDLR_DEFAULT	7
 
-static int thc_i2c_subip_set_speed(struct thc_device *dev, const u32 speed,
-				   const u32 hcnt, const u32 lcnt)
+static int thc_i2c_subip_bus_config(struct thc_device *dev, const struct thc_i2c_config *i2c_config)
 {
 	u32 hcnt_offset, lcnt_offset;
-	u32 val;
+	u32 read_size = sizeof(u32);
+	u32 val = 0;
 	int ret;
 
-	switch (speed) {
+	ret = thc_i2c_subip_pio_read(dev, THC_I2C_IC_TAR_OFFSET, &read_size, &val);
+	if (ret < 0)
+		return ret;
+
+	val &= ~(THC_I2C_IC_TAR_IC_TAR | THC_I2C_IC_TAR_IC_10BITADDR_MASTER);
+	val |= FIELD_PREP(THC_I2C_IC_TAR_IC_10BITADDR_MASTER, i2c_config->addr_mode);
+	val |= FIELD_PREP(THC_I2C_IC_TAR_IC_TAR, i2c_config->target_addr);
+	ret = thc_i2c_subip_pio_write(dev, THC_I2C_IC_TAR_OFFSET, sizeof(u32), &val);
+	if (ret < 0)
+		return ret;
+
+	switch (i2c_config->speed) {
 	case THC_I2C_STANDARD:
 		hcnt_offset = THC_I2C_IC_SS_SCL_HCNT_OFFSET;
 		lcnt_offset = THC_I2C_IC_SS_SCL_LCNT_OFFSET;
@@ -1446,25 +1457,43 @@ static int thc_i2c_subip_set_speed(struct thc_device *dev, const u32 speed,
 		break;
 
 	default:
-		dev_err_once(dev->dev, "Unsupported i2c speed %d\n", speed);
+		dev_err_once(dev->dev, "Unsupported i2c speed %d\n", i2c_config->speed);
 		ret = -EINVAL;
 		return ret;
 	}
 
-	ret = thc_i2c_subip_pio_write(dev, hcnt_offset, sizeof(u32), &hcnt);
+	ret = thc_i2c_subip_pio_write(dev, hcnt_offset, sizeof(u32), &i2c_config->scl_hcnt);
 	if (ret < 0)
 		return ret;
 
-	ret = thc_i2c_subip_pio_write(dev, lcnt_offset, sizeof(u32), &lcnt);
+	ret = thc_i2c_subip_pio_write(dev, lcnt_offset, sizeof(u32), &i2c_config->scl_lcnt);
 	if (ret < 0)
 		return ret;
 
 	val = I2C_SUBIP_CON_DEFAULT & ~THC_I2C_IC_CON_SPEED;
-	val |= FIELD_PREP(THC_I2C_IC_CON_SPEED, speed);
+	val |= FIELD_PREP(THC_I2C_IC_CON_SPEED, i2c_config->speed);
 	ret = thc_i2c_subip_pio_write(dev, THC_I2C_IC_CON_OFFSET, sizeof(u32), &val);
 	if (ret < 0)
 		return ret;
 
+	ret = thc_i2c_subip_pio_read(dev, THC_I2C_IC_SDA_HOLD_OFFSET, &read_size, &val);
+	if (ret < 0)
+		return ret;
+
+	if (i2c_config->sda_tx_hold) {
+		val &= ~THC_I2C_IC_SDA_HOLD_IC_SDA_TX_HOLD;
+		val |= FIELD_PREP(THC_I2C_IC_SDA_HOLD_IC_SDA_TX_HOLD, i2c_config->sda_tx_hold);
+	}
+
+	if (i2c_config->sda_rx_hold) {
+		val &= ~THC_I2C_IC_SDA_HOLD_IC_SDA_RX_HOLD;
+		val |= FIELD_PREP(THC_I2C_IC_SDA_HOLD_IC_SDA_RX_HOLD, i2c_config->sda_rx_hold);
+	}
+
+	ret = thc_i2c_subip_pio_write(dev, THC_I2C_IC_SDA_HOLD_OFFSET, sizeof(u32), &val);
+	if (ret < 0)
+		return ret;
+
 	return 0;
 }
 
@@ -1474,6 +1503,7 @@ static u32 i2c_subip_regs[] = {
 	THC_I2C_IC_INTR_MASK_OFFSET,
 	THC_I2C_IC_RX_TL_OFFSET,
 	THC_I2C_IC_TX_TL_OFFSET,
+	THC_I2C_IC_SDA_HOLD_OFFSET,
 	THC_I2C_IC_DMA_CR_OFFSET,
 	THC_I2C_IC_DMA_TDLR_OFFSET,
 	THC_I2C_IC_DMA_RDLR_OFFSET,
@@ -1490,20 +1520,19 @@ static u32 i2c_subip_regs[] = {
  * thc_i2c_subip_init - Initialize and configure THC I2C subsystem
  *
  * @dev: The pointer of THC private device context
- * @target_address: Slave address of touch device (TIC)
- * @speed: I2C bus frequency speed mode
- * @hcnt: I2C clock SCL high count
- * @lcnt: I2C clock SCL low count
+ * @i2c_config: The pointer of THC I2C bus configure structure
  *
  * Return: 0 on success, other error codes on failed.
  */
-int thc_i2c_subip_init(struct thc_device *dev, const u32 target_address,
-		       const u32 speed, const u32 hcnt, const u32 lcnt)
+int thc_i2c_subip_init(struct thc_device *dev, const struct thc_i2c_config *i2c_config)
 {
 	u32 read_size = sizeof(u32);
 	u32 val;
 	int ret;
 
+	if (!dev || !i2c_config)
+		return -EINVAL;
+
 	ret = thc_i2c_subip_pio_read(dev, THC_I2C_IC_ENABLE_OFFSET, &read_size, &val);
 	if (ret < 0)
 		return ret;
@@ -1513,17 +1542,7 @@ int thc_i2c_subip_init(struct thc_device *dev, const u32 target_address,
 	if (ret < 0)
 		return ret;
 
-	ret = thc_i2c_subip_pio_read(dev, THC_I2C_IC_TAR_OFFSET, &read_size, &val);
-	if (ret < 0)
-		return ret;
-
-	val &= ~THC_I2C_IC_TAR_IC_TAR;
-	val |= FIELD_PREP(THC_I2C_IC_TAR_IC_TAR, target_address);
-	ret = thc_i2c_subip_pio_write(dev, THC_I2C_IC_TAR_OFFSET, sizeof(u32), &val);
-	if (ret < 0)
-		return ret;
-
-	ret = thc_i2c_subip_set_speed(dev, speed, hcnt, lcnt);
+	ret = thc_i2c_subip_bus_config(dev, i2c_config);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.h b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.h
index 0db435335e24..be8a9605d02f 100644
--- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.h
+++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.h
@@ -49,6 +49,26 @@ enum thc_int_type {
 	THC_UNKNOWN_INT
 };
 
+/**
+ * struct thc_i2c_config - THC I2C bus configuration
+ * @target_addr: Slave address of touch device (TIC)
+ * @addr_mode: Slave address mode of touch device (TIC), 7bit or 10bit
+ * @speed: I2C bus frequency speed mode
+ * @scl_hcnt: I2C clock SCL high count
+ * @scl_lcnt: I2C clock SCL low count
+ * @sda_tx_hold: I2C Data SDA transmit hold period
+ * @sda_rx_hold: I2C Data SDA receive hold period
+ */
+struct thc_i2c_config {
+	u16 target_addr;
+	u8  addr_mode;
+	u32 speed;
+	u32 scl_hcnt;
+	u32 scl_lcnt;
+	u32 sda_tx_hold;
+	u32 sda_rx_hold;
+};
+
 /**
  * struct thc_device - THC private device struct
  * @thc_regmap: MMIO regmap structure for accessing THC registers
@@ -121,8 +141,7 @@ int thc_spi_write_config(struct thc_device *dev, u32 spi_freq_val,
 			 u32 io_mode, u32 opcode, u32 spi_wr_mps, u32 perf_limit);
 void thc_spi_input_output_address_config(struct thc_device *dev, u32 input_hdr_addr,
 					 u32 input_bdy_addr, u32 output_addr);
-int thc_i2c_subip_init(struct thc_device *dev, const u32 target_address,
-		       const u32 speed, const u32 hcnt, const u32 lcnt);
+int thc_i2c_subip_init(struct thc_device *dev, const struct thc_i2c_config *i2c_config);
 int thc_i2c_subip_regs_save(struct thc_device *dev);
 int thc_i2c_subip_regs_restore(struct thc_device *dev);
 int thc_i2c_set_rx_max_size(struct thc_device *dev, u32 max_rx_size);
diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-hw.h b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-hw.h
index c6d026686b7a..a21222543ce4 100644
--- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-hw.h
+++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-hw.h
@@ -887,4 +887,7 @@ enum THC_I2C_SPEED_MODE {
 #define THC_I2C_IC_DMA_CR_RDMAE				BIT(0)
 #define THC_I2C_IC_DMA_CR_TDMAE				BIT(1)
 
+#define THC_I2C_IC_SDA_HOLD_IC_SDA_TX_HOLD		GENMASK(15, 0)
+#define THC_I2C_IC_SDA_HOLD_IC_SDA_RX_HOLD		GENMASK(23, 16)
+
 #endif /* _INTEL_THC_HW_H_  */
-- 
2.43.0

Re: [PATCH 1/2] HID: Intel-thc-hid: Intel-thc: Refactor I2C bus configuration with unified config structure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Erroneous bitwise operation when configuring 10-bit address mode using FIELD_PREP reverses the 7-bit and 10-bit modes.
- [Low] The function signature of thc_i2c_subip_init() was modified to take a configuration struct, but callers in pci-quicki2c.c were not updated, leading to compilation failure.

Pre-existing issues:
- [Critical] Hardware-controlled stack and heap buffer overflows via unchecked PIO read size in pio_complete().
--

--- Patch [1]: [PATCH 1/2] HID: Intel-thc-hid: Intel-thc: Refactor I2C bus configuration with unified config structure ---
commit e5a64a16b521af522ba816d8c4f239b2bd596a69
Author: Even Xu <even.xu@intel.com>
Subject: HID: Intel-thc-hid: Intel-thc: Refactor I2C bus configuration with unified config structure

This commit introduces a new struct thc_i2c_config to consolidate configurable
I2C bus parameters. It renames and updates thc_i2c_subip_set_speed to
thc_i2c_subip_bus_config and updates thc_i2c_subip_init to use the new
structure.

> diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c
> index 9a8449428170b..7b4a58e1416d6 100644
> --- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c
> +++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c
> @@ -1422,14 +1422,25 @@ static int thc_i2c_subip_pio_write(struct thc_device *dev, const u32 address,
>  #define I2C_SUBIP_DMA_TDLR_DEFAULT	7
>  #define I2C_SUBIP_DMA_RDLR_DEFAULT	7
>  
> -static int thc_i2c_subip_set_speed(struct thc_device *dev, const u32 speed,
> -				   const u32 hcnt, const u32 lcnt)
> +static int thc_i2c_subip_bus_config(struct thc_device *dev, const struct thc_i2c_config *i2c_config)
>  {
>  	u32 hcnt_offset, lcnt_offset;
> -	u32 val;
> +	u32 read_size = sizeof(u32);
> +	u32 val = 0;
>  	int ret;
>  
> -	switch (speed) {
> +	ret = thc_i2c_subip_pio_read(dev, THC_I2C_IC_TAR_OFFSET, &read_size, &val);

[Severity: Critical]
This is a pre-existing issue, but could this cause a stack buffer overflow?

If a compromised or malicious peripheral returns a large size via the
hardware register in pio_complete(), it will be extracted and used directly
as the byte count for regmap_bulk_read():

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c:pio_complete() {
    ...
    regmap_bulk_read(dev->thc_regmap, THC_M_PRT_SW_SEQ_DATA1_OFFSET,
                     buffer, *size / sizeof(u32));
    ...
}

Since the destination buffer here is a 4-byte local stack variable (val),
could an untrusted peripheral trigger a large out-of-bounds write to the
kernel stack?

> +	if (ret < 0)
> +		return ret;
> +
> +	val &= ~(THC_I2C_IC_TAR_IC_TAR | THC_I2C_IC_TAR_IC_10BITADDR_MASTER);
> +	val |= FIELD_PREP(THC_I2C_IC_TAR_IC_10BITADDR_MASTER, i2c_config->addr_mode);

[Severity: High]
Does this logic correctly enable 10-bit mode?

The struct documentation for thc_i2c_config explicitly instructs callers
to pass the literal value 7 or 10 to addr_mode:

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.h:
 * @addr_mode: Slave address mode of touch device (TIC), 7bit or 10bit

When these values are passed to FIELD_PREP with a 1-bit mask:
- For 7 (binary 0111), the mask yields 1, incorrectly enabling 10-bit mode.
- For 10 (binary 1010), the mask yields 0, incorrectly disabling 10-bit mode.

Could this inadvertently reverse the 7-bit and 10-bit mode configuration,
causing I2C device communication to fail?

[ ... ]

> diff --git a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.h b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.h
> index 0db435335e249..be8a9605d02fe 100644
> --- a/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.h
> +++ b/drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.h
> @@ -121,8 +141,7 @@ int thc_spi_write_config(struct thc_device *dev, u32 spi_freq_val,
>  			 u32 io_mode, u32 opcode, u32 spi_wr_mps, u32 perf_limit);
>  void thc_spi_input_output_address_config(struct thc_device *dev, u32 input_hdr_addr,
>  					 u32 input_bdy_addr, u32 output_addr);
> -int thc_i2c_subip_init(struct thc_device *dev, const u32 target_address,
> -		       const u32 speed, const u32 hcnt, const u32 lcnt);
> +int thc_i2c_subip_init(struct thc_device *dev, const struct thc_i2c_config *i2c_config);

[Severity: Low]
Will this compile correctly given the other callers in the driver?

It looks like the function signature of thc_i2c_subip_init() was changed
from 5 arguments to 2 arguments, but callers in pci-quicki2c.c were not
updated:

drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c:quicki2c_probe() {
    ...
    ret = thc_i2c_subip_init(qcdev->thc_hw, qcdev->i2c_slave_addr,
                             qcdev->i2c_speed_mode,
                             qcdev->i2c_clock_hcnt,
                             qcdev->i2c_clock_lcnt);
    ...
}

Would this regression trigger a build failure at quicki2c_probe() and
quicki2c_restore() since they still attempt to pass the old arguments?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727030310.3850984-1-even.xu@intel.com?part=1

[PATCH 2/2] HID: Intel-thc-hid: Intel-quicki2c: Support full I2C BUS config parameters

[Even Xu]

Read complete I2C bus configuration parameters from ACPI and passes them
to thc_i2c_subip_init() to properly initialize the THC I2C subip with
platform-specific settings.

This change enhances hardware compatibility by allowing full
platform-specific I2C bus configurations.

Signed-off-by: Even Xu <even.xu@intel.com>
---
 .../intel-quicki2c/pci-quicki2c.c             | 46 ++++++++++---------
 .../intel-quicki2c/quicki2c-dev.h             | 14 ++----
 2 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
index 3ccbddfe8e5e..c3c7cf8e4b63 100644
--- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
+++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
@@ -175,7 +175,9 @@ static int quicki2c_get_acpi_resources(struct quicki2c_device *qcdev)
 	if (i2c_param.addressing_mode != HIDI2C_ADDRESSING_MODE_7BIT)
 		return -EOPNOTSUPP;
 
-	qcdev->i2c_slave_addr = i2c_param.device_address;
+	qcdev->i2c_config.addr_mode = HIDI2C_ADDRESSING_MODE_7BIT;
+
+	qcdev->i2c_config.target_addr = i2c_param.device_address;
 
 	ret = quicki2c_acpi_get_dsd_property(adev, QUICKI2C_ACPI_METHOD_NAME_ISUB,
 					     ACPI_TYPE_BUFFER, &i2c_config);
@@ -184,24 +186,32 @@ static int quicki2c_get_acpi_resources(struct quicki2c_device *qcdev)
 
 	if (i2c_param.connection_speed > 0 &&
 	    i2c_param.connection_speed <= QUICKI2C_SUBIP_STANDARD_MODE_MAX_SPEED) {
-		qcdev->i2c_speed_mode = THC_I2C_STANDARD;
-		qcdev->i2c_clock_hcnt = i2c_config.SMHX;
-		qcdev->i2c_clock_lcnt = i2c_config.SMLX;
+		qcdev->i2c_config.speed = THC_I2C_STANDARD;
+		qcdev->i2c_config.scl_hcnt = (u32)i2c_config.SMHX;
+		qcdev->i2c_config.scl_lcnt = (u32)i2c_config.SMLX;
+		qcdev->i2c_config.sda_tx_hold = (u32)i2c_config.SMTD;
+		qcdev->i2c_config.sda_rx_hold = (u32)i2c_config.SMRD;
 	} else if (i2c_param.connection_speed > QUICKI2C_SUBIP_STANDARD_MODE_MAX_SPEED &&
 		   i2c_param.connection_speed <= QUICKI2C_SUBIP_FAST_MODE_MAX_SPEED) {
-		qcdev->i2c_speed_mode = THC_I2C_FAST_AND_PLUS;
-		qcdev->i2c_clock_hcnt = i2c_config.FMHX;
-		qcdev->i2c_clock_lcnt = i2c_config.FMLX;
+		qcdev->i2c_config.speed = THC_I2C_FAST_AND_PLUS;
+		qcdev->i2c_config.scl_hcnt = (u32)i2c_config.FMHX;
+		qcdev->i2c_config.scl_lcnt = (u32)i2c_config.FMLX;
+		qcdev->i2c_config.sda_tx_hold = (u32)i2c_config.FMTD;
+		qcdev->i2c_config.sda_rx_hold = (u32)i2c_config.FMRD;
 	} else if (i2c_param.connection_speed > QUICKI2C_SUBIP_FAST_MODE_MAX_SPEED &&
 		   i2c_param.connection_speed <= QUICKI2C_SUBIP_FASTPLUS_MODE_MAX_SPEED) {
-		qcdev->i2c_speed_mode = THC_I2C_FAST_AND_PLUS;
-		qcdev->i2c_clock_hcnt = i2c_config.FPHX;
-		qcdev->i2c_clock_lcnt = i2c_config.FPLX;
+		qcdev->i2c_config.speed = THC_I2C_FAST_AND_PLUS;
+		qcdev->i2c_config.scl_hcnt = (u32)i2c_config.FPHX;
+		qcdev->i2c_config.scl_lcnt = (u32)i2c_config.FPLX;
+		qcdev->i2c_config.sda_tx_hold = (u32)i2c_config.FPTD;
+		qcdev->i2c_config.sda_rx_hold = (u32)i2c_config.FPRD;
 	} else if (i2c_param.connection_speed > QUICKI2C_SUBIP_FASTPLUS_MODE_MAX_SPEED &&
 		   i2c_param.connection_speed <= QUICKI2C_SUBIP_HIGH_SPEED_MODE_MAX_SPEED) {
-		qcdev->i2c_speed_mode = THC_I2C_HIGH_SPEED;
-		qcdev->i2c_clock_hcnt = i2c_config.HMHX;
-		qcdev->i2c_clock_lcnt = i2c_config.HMLX;
+		qcdev->i2c_config.speed = THC_I2C_HIGH_SPEED;
+		qcdev->i2c_config.scl_hcnt = (u32)i2c_config.HMHX;
+		qcdev->i2c_config.scl_lcnt = (u32)i2c_config.HMLX;
+		qcdev->i2c_config.sda_tx_hold = (u32)i2c_config.HMTD;
+		qcdev->i2c_config.sda_rx_hold = (u32)i2c_config.HMRD;
 	} else {
 		return -EOPNOTSUPP;
 	}
@@ -415,10 +425,7 @@ static struct quicki2c_device *quicki2c_dev_init(struct pci_dev *pdev, void __io
 		return ERR_PTR(ret);
 	}
 
-	ret = thc_i2c_subip_init(qcdev->thc_hw, qcdev->i2c_slave_addr,
-				 qcdev->i2c_speed_mode,
-				 qcdev->i2c_clock_hcnt,
-				 qcdev->i2c_clock_lcnt);
+	ret = thc_i2c_subip_init(qcdev->thc_hw, &qcdev->i2c_config);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -978,10 +985,7 @@ static int quicki2c_restore(struct device *device)
 	if (ret)
 		return ret;
 
-	ret = thc_i2c_subip_init(qcdev->thc_hw, qcdev->i2c_slave_addr,
-				 qcdev->i2c_speed_mode,
-				 qcdev->i2c_clock_hcnt,
-				 qcdev->i2c_clock_lcnt);
+	ret = thc_i2c_subip_init(qcdev->thc_hw, &qcdev->i2c_config);
 	if (ret)
 		return ret;
 
diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h b/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h
index d72565de67b9..6d25a846153e 100644
--- a/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h
+++ b/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h
@@ -7,6 +7,8 @@
 #include <linux/hid-over-i2c.h>
 #include <linux/workqueue.h>
 
+#include "intel-thc-dev.h"
+
 #define PCI_DEVICE_ID_INTEL_THC_LNL_DEVICE_ID_I2C_PORT1		0xA848
 #define PCI_DEVICE_ID_INTEL_THC_LNL_DEVICE_ID_I2C_PORT2		0xA84A
 #define PCI_DEVICE_ID_INTEL_THC_PTL_H_DEVICE_ID_I2C_PORT1	0xE348
@@ -159,7 +161,6 @@ struct quicki2c_ddata {
 
 struct device;
 struct pci_dev;
-struct thc_device;
 struct hid_device;
 struct acpi_device;
 
@@ -174,13 +175,10 @@ struct acpi_device;
  * @state: THC I2C device state
  * @mem_addr: MMIO memory address
  * @dev_desc: Device descriptor for HIDI2C protocol
- * @i2c_slave_addr: HIDI2C device slave address
+ * @i2c_config: I2C bus configuration
  * @hid_desc_addr: Register address for retrieve HID device descriptor
  * @active_ltr_val: THC active LTR value
  * @low_power_ltr_val: THC low power LTR value
- * @i2c_speed_mode: 0 - standard mode, 1 - fast mode, 2 - fast mode plus
- * @i2c_clock_hcnt: I2C CLK high period time (unit in cycle count)
- * @i2c_clock_lcnt: I2C CLK low period time (unit in cycle count)
  * @report_descriptor: Store a copy of device report descriptor
  * @input_buf: Store a copy of latest input report data
  * @report_buf: Store a copy of latest input/output report packet from set/get feature
@@ -206,16 +204,12 @@ struct quicki2c_device {
 	void __iomem *mem_addr;
 
 	struct hidi2c_dev_descriptor dev_desc;
-	u8 i2c_slave_addr;
+	struct thc_i2c_config i2c_config;
 	u16 hid_desc_addr;
 
 	u32 active_ltr_val;
 	u32 low_power_ltr_val;
 
-	u32 i2c_speed_mode;
-	u32 i2c_clock_hcnt;
-	u32 i2c_clock_lcnt;
-
 	u8 *report_descriptor;
 	u8 *input_buf;
 	u8 *report_buf;
-- 
2.43.0

Re: [PATCH 2/2] HID: Intel-thc-hid: Intel-quicki2c: Support full I2C BUS config parameters

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Multiple critical flaws in ACPI evaluation in `quicki2c_acpi_get_dsd_property` lead to memory leaks, potential stack buffer overflows, and type confusion.
- [High] Structs `i2c_config` and `i2c_param` are declared on the stack without zero-initialization, leading to uninitialized kernel memory being used for hardware configuration if the ACPI buffer is smaller than expected.
--

--- Patch [2]: [PATCH 2/2] HID: Intel-thc-hid: Intel-quicki2c: Support full I2C BUS config parameters ---
commit 1910fa4a54d63973a7e0cc155b418558fa7c3450
Author: Even Xu <even.xu@intel.com>

HID: Intel-thc-hid: Intel-quicki2c: Support full I2C BUS config parameters

This patch reads complete I2C bus configuration parameters from ACPI and
passes them to thc_i2c_subip_init() to properly initialize the THC I2C
subip. This enhances hardware compatibility by allowing full
platform-specific I2C bus configurations.

> diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> index 46d3e9a019995..2ec52cb35a132 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -175,7 +175,9 @@ static int quicki2c_get_acpi_resources(struct quicki2c_device *qcdev)
>  	if (i2c_param.addressing_mode != HIDI2C_ADDRESSING_MODE_7BIT)
>  		return -EOPNOTSUPP;
>  
> -	qcdev->i2c_slave_addr = i2c_param.device_address;
> +	qcdev->i2c_config.addr_mode = HIDI2C_ADDRESSING_MODE_7BIT;
> +
> +	qcdev->i2c_config.target_addr = i2c_param.device_address;
>  
>  	ret = quicki2c_acpi_get_dsd_property(adev, QUICKI2C_ACPI_METHOD_NAME_ISUB,
>  					     ACPI_TYPE_BUFFER, &i2c_config);

[Severity: Critical]
This is a pre-existing issue, but does quicki2c_acpi_get_dsd_property() have
multiple flaws when evaluating ACPI methods?

When called here, quicki2c_acpi_get_dsd_property() receives an acpi_buffer, but
it ignores the passed type parameter and unconditionally assumes
ret_obj->type == ACPI_TYPE_BUFFER. If the ACPI method returns a different type,
does type confusion occur, causing ret_obj->buffer.pointer to resolve to
invalid memory and crash during memcpy()?

Additionally, there appears to be no bounds checking between the returned
ret_obj->buffer.length and the size of the destination stack buffer. Can an
ACPI table overflow the kernel stack here? Finally, is buffer.pointer leaked
before the function returns?

>  	if (ret)
>  		return ret;
>  
>  	if (i2c_param.connection_speed > 0 &&
>  	    i2c_param.connection_speed <= QUICKI2C_SUBIP_STANDARD_MODE_MAX_SPEED) {
> -		qcdev->i2c_speed_mode = THC_I2C_STANDARD;
> -		qcdev->i2c_clock_hcnt = i2c_config.SMHX;
> -		qcdev->i2c_clock_lcnt = i2c_config.SMLX;
> +		qcdev->i2c_config.speed = THC_I2C_STANDARD;
> +		qcdev->i2c_config.scl_hcnt = (u32)i2c_config.SMHX;
> +		qcdev->i2c_config.scl_lcnt = (u32)i2c_config.SMLX;
> +		qcdev->i2c_config.sda_tx_hold = (u32)i2c_config.SMTD;
> +		qcdev->i2c_config.sda_rx_hold = (u32)i2c_config.SMRD;

[Severity: High]
This isn't a bug introduced by this patch, but are the i2c_config and
i2c_param structures declared on the stack in quicki2c_get_acpi_resources()
without zero-initialization?

If the ACPI firmware returns a shorter buffer, quicki2c_acpi_get_dsd_property()
will only populate part of the structures. This means the newly added code
above will read uninitialized stack memory for fields like SMTD and SMRD.
Can this lead to invalid bus timings and system instability?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727030310.3850984-1-even.xu@intel.com?part=2