Re: [PATCH v3 4/6] thermal: qcom: tsens: add hardware-bin trip-point filtering

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] thermal: qcom: tsens: broken error handling in
  tsens_populate_hw_bin_info()
- [Low] thermal: qcom: tsens: inaccurate cell_names kernel-doc in
  tsens_hw_bin_desc
- [Low] thermal: qcom: tsens: inaccurate i-temp description in commit
  message
--

commit 65356a4ab856b03d856c9e9331810962e03057df
Author: Amit Kucheria <amit.kucheria@oss.qualcomm.com>

thermal: qcom: tsens: add hardware-bin trip-point filtering

This commit reads hardware-binning qfprom fuse cells at probe time and passes
the computed hardware version bitfields to the thermal OF parser for
qcm6490, sc7280, and x1e80100 TSENS controllers.

>  - tsens_hw_bin_desc_i_temp reads a single i-temp bit and reports it
>    directly as the hw-bin value; used by data_x1e80100.

[Severity: Low]
Is this description of tsens_hw_bin_desc_i_temp accurate?

tsens_compute_hw_version_i_temp() maps bit 0 of the fuse to the bitmask
values 0x1u (standard bin) or 0x2u (high-temp bin) rather than reporting
the raw bit value directly.

> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index bca3fb1d2bf99..db4e02529c91d 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c

[ ... ]

> +static const char * const tsens_cells_qcm6490[] = { "jtag-id", "feat-id" };
> +static const char * const tsens_cells_i_temp[]  = { "i-temp" };
> +
> +const struct tsens_hw_bin_desc tsens_hw_bin_desc_qcm6490 = {
> +	.cell_names         = tsens_cells_qcm6490,
> +	.ncells             = ARRAY_SIZE(tsens_cells_qcm6490),
> +	.compute_hw_version = tsens_compute_hw_version_qcm6490,
> +};
> +
> +const struct tsens_hw_bin_desc tsens_hw_bin_desc_i_temp = {
> +	.cell_names         = tsens_cells_i_temp,
> +	.ncells             = ARRAY_SIZE(tsens_cells_i_temp),
> +	.compute_hw_version = tsens_compute_hw_version_i_temp,
> +};
> +
> +/**
> + * tsens_populate_hw_bin_info - read fuse cells and populate priv->hw_bin_info
> + * @priv: tsens private data
> + * @data: platform data containing the optional hw_bin_desc pointer
> + *
> + * If data->hw_bin_desc is NULL, this is a no-op and returns 0.  Otherwise,
> + * reads the nvmem cells named in hw_bin_desc->cell_names, calls the
> + * compute_hw_version() callback, and stores the result in priv->hw_bin_info
> + * for later use by tsens_register().
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> +static int tsens_populate_hw_bin_info(struct tsens_priv *priv,
> +				      const struct tsens_plat_data *data)
> +{
> +	const struct tsens_hw_bin_desc *desc;
> +	u32 fuse_vals[TSENS_HW_BIN_MAX_CELLS];
> +	u32 hw_versions[THERMAL_HW_BIN_MAX_LEVELS] = { 0 };
> +	unsigned int hw_count = 0;
> +	int i, ret;
> +
> +	if (!data->hw_bin_desc)
> +		return 0;
> +
> +	desc = data->hw_bin_desc;
> +
> +	if (!desc->ncells || desc->ncells > TSENS_HW_BIN_MAX_CELLS) {
> +		dev_err(priv->dev, "hw_bin_desc ncells %u out of range\n",
> +			desc->ncells);
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < (int)desc->ncells; i++) {
> +		ret = nvmem_cell_read_variable_le_u32(priv->dev,
> +						      desc->cell_names[i],
> +						      &fuse_vals[i]);
> +		if (ret < 0) {
> +			dev_err(priv->dev,
> +				"failed to read nvmem cell '%s': %d\n",
> +				desc->cell_names[i], ret);
> +			return ret;
> +		}
> +	}
> +
> +	ret = desc->compute_hw_version(priv->dev, fuse_vals, desc->ncells,
> +				       hw_versions, &hw_count);
> +	if (ret) {
> +		dev_err(priv->dev, "compute_hw_version failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!hw_count || hw_count > THERMAL_HW_BIN_MAX_LEVELS) {
> +		dev_err(priv->dev,
> +			"compute_hw_version returned invalid hw_count %u\n",
> +			hw_count);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Store a copy of the hw-version array in priv->hw_bin_info so
> +	 * tsens_register() can pass it to the thermal OF parser while trips are
> +	 * parsed.
> +	 *
> +	 * Use devm_kmemdup so the copy is freed when the device is unbound.
> +	 */
> +	priv->hw_bin_info.supported_hw_bin = devm_kmemdup(priv->dev, hw_versions,
> +							  hw_count * sizeof(u32),
> +							  GFP_KERNEL);
> +	if (!priv->hw_bin_info.supported_hw_bin)
> +		return -ENOMEM;
> +
> +	priv->hw_bin_info.supported_hw_bin_count = hw_count;
> +
> +	return 0;
> +}

[ ... ]

> @@ -1443,6 +1619,14 @@ static int tsens_probe_common(struct platform_device *pdev)
>  	priv->feat = data->feat;
>  	priv->fields = data->fields;
>  
> +	ret = tsens_populate_hw_bin_info(priv, data);
> +	if (ret == -EPROBE_DEFER)
> +		return ret;
> +	if (ret)
> +		dev_warn(&pdev->dev,
> +			 "thermal-bin failed to read FUSE data (%d): trips with temperature-bin will use first entry\n",
> +			 ret);
> +
>  	platform_set_drvdata(pdev, priv);
>  
>  	device_init_wakeup(dev, !data->no_irq_wake);

[Severity: Medium]
Does tsens_populate_hw_bin_info() log a dev_err() on -EPROBE_DEFER before
tsens_probe_common() checks for probe deferral?

When TSENS probes before the qfprom nvmem provider,
nvmem_cell_read_variable_le_u32() returns -EPROBE_DEFER and
tsens_populate_hw_bin_info() emits an error message on every deferred
probe attempt.  When booting with existing sc7280 or x1e80100 DTBs that do
not define the optional nvmem-cells for hardware binning,
nvmem_cell_read_variable_le_u32() returns -ENOENT, causing both the
dev_err() in tsens_populate_hw_bin_info() and the dev_warn() in
tsens_probe_common() (which also passes &pdev->dev instead of dev) to be
printed.  In addition, if devm_kmemdup() returns -ENOMEM,
tsens_probe_common() treats it as a non-fatal fuse read warning instead of
failing probe.

> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index a35b15312961f..24489542b8a21 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -40,6 +40,29 @@ enum tsens_ver {
>  	VER_2_X_NO_RPM,
>  };
>  
> +/**
> + * struct tsens_hw_bin_desc - per-SoC descriptor for hardware-bin fuse reading
> + * @cell_names: NULL-terminated array of nvmem cell names to read
> + * @ncells:     number of entries in @cell_names
> + * @compute_hw_version: callback that converts raw fuse values to hw-version
> + *                      bitfields for thermal OF trip filtering
> + *
> + * Embed a pointer to this struct in tsens_plat_data.hw_bin_desc to enable
> + * hardware-bin trip filtering for a given SoC.  Set to NULL to disable.
> + */
> +struct tsens_hw_bin_desc {
> +	const char * const	*cell_names;
> +	unsigned int		 ncells;
> +	int (*compute_hw_version)(struct device *dev,
> +				  const u32 *fuse, unsigned int nfuse,
> +				  u32 *hw, unsigned int *count);
> +};

[Severity: Low]
Should the kernel-doc for @cell_names drop "NULL-terminated"?

tsens_cells_qcm6490 and tsens_cells_i_temp are not NULL-terminated and
size @ncells with ARRAY_SIZE(), while tsens_populate_hw_bin_info()
iterates from 0 to desc->ncells - 1.  If a future descriptor follows the
comment and NULL-terminates cell_names while using ARRAY_SIZE() for
ncells, the NULL sentinel will be passed as cell_id to
nvmem_cell_read_variable_le_u32().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-thermal_bin-v3-0-878df6eef8bc@oss.qualcomm.com?part=4