[PATCH] HID: intel-ish-hid: loader: Add PRODUCT_FAMILY-based firmware matching

Vishnu Sankar posted 1 patch 1 month, 3 weeks ago
drivers/hid/intel-ish-hid/ishtp/loader.c | 50 +++++++++++++++++++++++-
1 file changed, 48 insertions(+), 2 deletions(-)
[PATCH] HID: intel-ish-hid: loader: Add PRODUCT_FAMILY-based firmware matching
Posted by Vishnu Sankar 1 month, 3 weeks ago
Add support for firmware filenames that include the CRC32 checksum of the
DMI product_family field. Several OEMs ship ISH firmware variants shared
across a product family while product_name or product_sku may differ. This
intermediate matching granularity reduces duplication and improves firmware
selection for vendor-customized platforms.

The newly supported filename forms are checked before existing patterns:

  ish_${gen}_${vendor}_${family}_${name}_${sku}.bin
  ish_${gen}_${vendor}_${family}_${sku}.bin
  ish_${gen}_${vendor}_${family}_${name}.bin
  ish_${gen}_${vendor}_${family}.bin

The legacy product_name/product_sku rules remain unchanged and continue
to provide fallback matching.

ISH_FW_FILENAME_LEN_MAX is changed to 72 to accommodate the product_family.

Tested with X9 series and X1 series.

Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Tested-by: Richie Roy Jayme <rjayme.jp@gmail.com>
Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
---
 drivers/hid/intel-ish-hid/ishtp/loader.c | 50 +++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp/loader.c b/drivers/hid/intel-ish-hid/ishtp/loader.c
index f34086b29cf0..4559d460fd83 100644
--- a/drivers/hid/intel-ish-hid/ishtp/loader.c
+++ b/drivers/hid/intel-ish-hid/ishtp/loader.c
@@ -194,6 +194,11 @@ static int prepare_dma_bufs(struct ishtp_device *dev,
 
 	return 0;
 }
+/* Patterns with PRODUCT_FAMILY */
+#define ISH_FW_FILE_VENDOR_FAMILY_NAME_SKU_FMT "intel/ish/ish_%s_%08x_%08x_%08x_%08x.bin"
+#define ISH_FW_FILE_VENDOR_FAMILY_SKU_FMT "intel/ish/ish_%s_%08x_%08x_%08x.bin"
+#define ISH_FW_FILE_VENDOR_FAMILY_NAME_FMT "intel/ish/ish_%s_%08x_%08x_%08x.bin"
+#define ISH_FW_FILE_VENDOR_FAMILY_FMT "intel/ish/ish_%s_%08x_%08x.bin"
 
 #define ISH_FW_FILE_VENDOR_NAME_SKU_FMT "intel/ish/ish_%s_%08x_%08x_%08x.bin"
 #define ISH_FW_FILE_VENDOR_SKU_FMT "intel/ish/ish_%s_%08x_%08x.bin"
@@ -201,7 +206,7 @@ static int prepare_dma_bufs(struct ishtp_device *dev,
 #define ISH_FW_FILE_VENDOR_FMT "intel/ish/ish_%s_%08x.bin"
 #define ISH_FW_FILE_DEFAULT_FMT "intel/ish/ish_%s.bin"
 
-#define ISH_FW_FILENAME_LEN_MAX 56
+#define ISH_FW_FILENAME_LEN_MAX 72
 
 #define ISH_CRC_INIT (~0u)
 #define ISH_CRC_XOROUT (~0u)
@@ -256,8 +261,9 @@ static int request_ish_firmware(const struct firmware **firmware_p,
 				struct device *dev)
 {
 	const char *gen, *sys_vendor, *product_name, *product_sku;
+	const char *product_family;
 	struct ishtp_device *ishtp = dev_get_drvdata(dev);
-	u32 vendor_crc, name_crc, sku_crc;
+	u32 vendor_crc, name_crc, sku_crc, family_crc;
 	char filename[ISH_FW_FILENAME_LEN_MAX];
 	int ret;
 
@@ -265,6 +271,7 @@ static int request_ish_firmware(const struct firmware **firmware_p,
 	sys_vendor = dmi_get_system_info(DMI_SYS_VENDOR);
 	product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
 	product_sku = dmi_get_system_info(DMI_PRODUCT_SKU);
+	product_family = dmi_get_system_info(DMI_PRODUCT_FAMILY);
 
 	if (sys_vendor)
 		vendor_crc = crc32(ISH_CRC_INIT, sys_vendor, strlen(sys_vendor)) ^ ISH_CRC_XOROUT;
@@ -272,6 +279,45 @@ static int request_ish_firmware(const struct firmware **firmware_p,
 		name_crc = crc32(ISH_CRC_INIT, product_name, strlen(product_name)) ^ ISH_CRC_XOROUT;
 	if (product_sku)
 		sku_crc = crc32(ISH_CRC_INIT, product_sku, strlen(product_sku)) ^ ISH_CRC_XOROUT;
+	if (product_family)
+		family_crc = crc32(ISH_CRC_INIT, product_family, strlen(product_family)) ^ ISH_CRC_XOROUT;
+
+	/* PRODUCT_FAMILY-extended matching */
+	if (sys_vendor && product_family && product_name && product_sku) {
+		snprintf(filename, sizeof(filename),
+			 ISH_FW_FILE_VENDOR_FAMILY_NAME_SKU_FMT,
+			 gen, vendor_crc, family_crc, name_crc, sku_crc);
+		ret = _request_ish_firmware(firmware_p, filename, dev);
+		if (!ret)
+			return 0;
+	}
+
+	if (sys_vendor && product_family && product_sku) {
+		snprintf(filename, sizeof(filename),
+			 ISH_FW_FILE_VENDOR_FAMILY_SKU_FMT,
+			 gen, vendor_crc, family_crc, sku_crc);
+		ret = _request_ish_firmware(firmware_p, filename, dev);
+		if (!ret)
+			return 0;
+	}
+
+	if (sys_vendor && product_family && product_name) {
+		snprintf(filename, sizeof(filename),
+			 ISH_FW_FILE_VENDOR_FAMILY_NAME_FMT,
+			 gen, vendor_crc, family_crc, name_crc);
+		ret = _request_ish_firmware(firmware_p, filename, dev);
+		if (!ret)
+			return 0;
+	}
+
+	if (sys_vendor && product_family) {
+		snprintf(filename, sizeof(filename),
+			 ISH_FW_FILE_VENDOR_FAMILY_FMT,
+			 gen, vendor_crc, family_crc);
+		ret = _request_ish_firmware(firmware_p, filename, dev);
+		if (!ret)
+			return 0;
+}
 
 	if (sys_vendor && product_name && product_sku) {
 		snprintf(filename, sizeof(filename), ISH_FW_FILE_VENDOR_NAME_SKU_FMT, gen,
-- 
2.51.0
Re: [PATCH] HID: intel-ish-hid: loader: Add PRODUCT_FAMILY-based firmware matching
Posted by srinivas pandruvada 1 month, 2 weeks ago
+ Lixu
+ Even


On Thu, 2025-12-18 at 23:00 +0900, Vishnu Sankar wrote:
> Add support for firmware filenames that include the CRC32 checksum of
> the
> DMI product_family field. Several OEMs ship ISH firmware variants
> shared
> across a product family while product_name or product_sku may differ.
> This
> intermediate matching granularity reduces duplication and improves
> firmware
> selection for vendor-customized platforms.
> 
> The newly supported filename forms are checked before existing
> patterns:
> 
>   ish_${gen}_${vendor}_${family}_${name}_${sku}.bin
>   ish_${gen}_${vendor}_${family}_${sku}.bin
>   ish_${gen}_${vendor}_${family}_${name}.bin
>   ish_${gen}_${vendor}_${family}.bin
> 
> The legacy product_name/product_sku rules remain unchanged and
> continue
> to provide fallback matching.
> 
> ISH_FW_FILENAME_LEN_MAX is changed to 72 to accommodate the
> product_family.
> 
> Tested with X9 series and X1 series.
> 
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> Tested-by: Richie Roy Jayme <rjayme.jp@gmail.com>
> Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>
> ---
>  drivers/hid/intel-ish-hid/ishtp/loader.c | 50
> +++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ishtp/loader.c
> b/drivers/hid/intel-ish-hid/ishtp/loader.c
> index f34086b29cf0..4559d460fd83 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/loader.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/loader.c
> @@ -194,6 +194,11 @@ static int prepare_dma_bufs(struct ishtp_device
> *dev,
>  
>  	return 0;
>  }
> +/* Patterns with PRODUCT_FAMILY */
> +#define ISH_FW_FILE_VENDOR_FAMILY_NAME_SKU_FMT
> "intel/ish/ish_%s_%08x_%08x_%08x_%08x.bin"
> +#define ISH_FW_FILE_VENDOR_FAMILY_SKU_FMT
> "intel/ish/ish_%s_%08x_%08x_%08x.bin"
> +#define ISH_FW_FILE_VENDOR_FAMILY_NAME_FMT
> "intel/ish/ish_%s_%08x_%08x_%08x.bin"
> +#define ISH_FW_FILE_VENDOR_FAMILY_FMT
> "intel/ish/ish_%s_%08x_%08x.bin"
>  
>  #define ISH_FW_FILE_VENDOR_NAME_SKU_FMT
> "intel/ish/ish_%s_%08x_%08x_%08x.bin"
>  #define ISH_FW_FILE_VENDOR_SKU_FMT "intel/ish/ish_%s_%08x_%08x.bin"
> @@ -201,7 +206,7 @@ static int prepare_dma_bufs(struct ishtp_device
> *dev,
>  #define ISH_FW_FILE_VENDOR_FMT "intel/ish/ish_%s_%08x.bin"
>  #define ISH_FW_FILE_DEFAULT_FMT "intel/ish/ish_%s.bin"
>  
> -#define ISH_FW_FILENAME_LEN_MAX 56
> +#define ISH_FW_FILENAME_LEN_MAX 72
>  
>  #define ISH_CRC_INIT (~0u)
>  #define ISH_CRC_XOROUT (~0u)
> @@ -256,8 +261,9 @@ static int request_ish_firmware(const struct
> firmware **firmware_p,
>  				struct device *dev)
>  {
>  	const char *gen, *sys_vendor, *product_name, *product_sku;
> +	const char *product_family;
>  	struct ishtp_device *ishtp = dev_get_drvdata(dev);
> -	u32 vendor_crc, name_crc, sku_crc;
> +	u32 vendor_crc, name_crc, sku_crc, family_crc;
>  	char filename[ISH_FW_FILENAME_LEN_MAX];
>  	int ret;
>  
> @@ -265,6 +271,7 @@ static int request_ish_firmware(const struct
> firmware **firmware_p,
>  	sys_vendor = dmi_get_system_info(DMI_SYS_VENDOR);
>  	product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
>  	product_sku = dmi_get_system_info(DMI_PRODUCT_SKU);
> +	product_family = dmi_get_system_info(DMI_PRODUCT_FAMILY);
>  
>  	if (sys_vendor)
>  		vendor_crc = crc32(ISH_CRC_INIT, sys_vendor,
> strlen(sys_vendor)) ^ ISH_CRC_XOROUT;
> @@ -272,6 +279,45 @@ static int request_ish_firmware(const struct
> firmware **firmware_p,
>  		name_crc = crc32(ISH_CRC_INIT, product_name,
> strlen(product_name)) ^ ISH_CRC_XOROUT;
>  	if (product_sku)
>  		sku_crc = crc32(ISH_CRC_INIT, product_sku,
> strlen(product_sku)) ^ ISH_CRC_XOROUT;
> +	if (product_family)
> +		family_crc = crc32(ISH_CRC_INIT, product_family,
> strlen(product_family)) ^ ISH_CRC_XOROUT;
> +
> +	/* PRODUCT_FAMILY-extended matching */
> +	if (sys_vendor && product_family && product_name &&
> product_sku) {
> +		snprintf(filename, sizeof(filename),
> +			 ISH_FW_FILE_VENDOR_FAMILY_NAME_SKU_FMT,
> +			 gen, vendor_crc, family_crc, name_crc,
> sku_crc);
> +		ret = _request_ish_firmware(firmware_p, filename,
> dev);
> +		if (!ret)
> +			return 0;
> +	}
> +
> +	if (sys_vendor && product_family && product_sku) {
> +		snprintf(filename, sizeof(filename),
> +			 ISH_FW_FILE_VENDOR_FAMILY_SKU_FMT,
> +			 gen, vendor_crc, family_crc, sku_crc);
> +		ret = _request_ish_firmware(firmware_p, filename,
> dev);
> +		if (!ret)
> +			return 0;
> +	}
> +
> +	if (sys_vendor && product_family && product_name) {
> +		snprintf(filename, sizeof(filename),
> +			 ISH_FW_FILE_VENDOR_FAMILY_NAME_FMT,
> +			 gen, vendor_crc, family_crc, name_crc);
> +		ret = _request_ish_firmware(firmware_p, filename,
> dev);
> +		if (!ret)
> +			return 0;
> +	}
> +
> +	if (sys_vendor && product_family) {
> +		snprintf(filename, sizeof(filename),
> +			 ISH_FW_FILE_VENDOR_FAMILY_FMT,
> +			 gen, vendor_crc, family_crc);
> +		ret = _request_ish_firmware(firmware_p, filename,
> dev);
> +		if (!ret)
> +			return 0;
> +}
>  
>  	if (sys_vendor && product_name && product_sku) {
>  		snprintf(filename, sizeof(filename),
> ISH_FW_FILE_VENDOR_NAME_SKU_FMT, gen,
RE: [PATCH] HID: intel-ish-hid: loader: Add PRODUCT_FAMILY-based firmware matching
Posted by Zhang, Lixu 1 month, 2 weeks ago
>-----Original Message-----
>From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
>Sent: Thursday, December 18, 2025 11:55 PM
>To: Vishnu Sankar <vishnuocv@gmail.com>; jikos@kernel.org;
>bentiss@kernel.org; Sankar, Vishnu <vsankar@lenovo.com>; Zhang, Lixu
><lixu.zhang@intel.com>; Xu, Even <even.xu@intel.com>
>Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Mark Pearson
><mpearson-lenovo@squebb.ca>; Richie Roy Jayme <rjayme.jp@gmail.com>
>Subject: Re: [PATCH] HID: intel-ish-hid: loader: Add PRODUCT_FAMILY-based
>firmware matching
>
>+ Lixu
>+ Even
>
>
>On Thu, 2025-12-18 at 23:00 +0900, Vishnu Sankar wrote:
>> Add support for firmware filenames that include the CRC32 checksum of
>> the DMI product_family field. Several OEMs ship ISH firmware variants
>> shared across a product family while product_name or product_sku may
>> differ.
>> This
>> intermediate matching granularity reduces duplication and improves
>> firmware selection for vendor-customized platforms.

...

>> +
>> +	if (sys_vendor && product_family) {
>> +		snprintf(filename, sizeof(filename),
>> +			 ISH_FW_FILE_VENDOR_FAMILY_FMT,
>> +			 gen, vendor_crc, family_crc);
>> +		ret = _request_ish_firmware(firmware_p, filename,
>> dev);
>> +		if (!ret)
>> +			return 0;
>> +}
Indent issue.

Hi Vishnu,
Thanks for your patch.
1. Please run the ` ./scripts/checkpatch.pl --strict --codespell [PATCH FILE]`, and fix the issue.
2. Could you please also update the document `Documentation/hid/intel-ish-hid.rst`?

Thanks,
Lixu

>>
>>  	if (sys_vendor && product_name && product_sku) {
>>  		snprintf(filename, sizeof(filename),
>> ISH_FW_FILE_VENDOR_NAME_SKU_FMT, gen,
Re: [PATCH] HID: intel-ish-hid: loader: Add PRODUCT_FAMILY-based firmware matching
Posted by Vishnu Sankar 1 month, 2 weeks ago
Hi Lixu,

Thank you for the comments.

On Fri, Dec 19, 2025 at 10:26 AM Zhang, Lixu <lixu.zhang@intel.com> wrote:
>
> >-----Original Message-----
> >From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
> >Sent: Thursday, December 18, 2025 11:55 PM
> >To: Vishnu Sankar <vishnuocv@gmail.com>; jikos@kernel.org;
> >bentiss@kernel.org; Sankar, Vishnu <vsankar@lenovo.com>; Zhang, Lixu
> ><lixu.zhang@intel.com>; Xu, Even <even.xu@intel.com>
> >Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Mark Pearson
> ><mpearson-lenovo@squebb.ca>; Richie Roy Jayme <rjayme.jp@gmail.com>
> >Subject: Re: [PATCH] HID: intel-ish-hid: loader: Add PRODUCT_FAMILY-based
> >firmware matching
> >
> >+ Lixu
> >+ Even
> >
> >
> >On Thu, 2025-12-18 at 23:00 +0900, Vishnu Sankar wrote:
> >> Add support for firmware filenames that include the CRC32 checksum of
> >> the DMI product_family field. Several OEMs ship ISH firmware variants
> >> shared across a product family while product_name or product_sku may
> >> differ.
> >> This
> >> intermediate matching granularity reduces duplication and improves
> >> firmware selection for vendor-customized platforms.
>
> ...
>
> >> +
> >> +    if (sys_vendor && product_family) {
> >> +            snprintf(filename, sizeof(filename),
> >> +                     ISH_FW_FILE_VENDOR_FAMILY_FMT,
> >> +                     gen, vendor_crc, family_crc);
> >> +            ret = _request_ish_firmware(firmware_p, filename,
> >> dev);
> >> +            if (!ret)
> >> +                    return 0;
> >> +}
> Indent issue.
>
> Hi Vishnu,
> Thanks for your patch.
> 1. Please run the ` ./scripts/checkpatch.pl --strict --codespell [PATCH FILE]`, and fix the issue.
Acked
> 2. Could you please also update the document `Documentation/hid/intel-ish-hid.rst`?
Will update the document in v2.
>
> Thanks,
> Lixu
>
> >>
> >>      if (sys_vendor && product_name && product_sku) {
> >>              snprintf(filename, sizeof(filename),
> >> ISH_FW_FILE_VENDOR_NAME_SKU_FMT, gen,



-- 

Regards,

      Vishnu Sankar
     +817015150407 (Japan)