Re: [PATCH v2 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 issue(s) 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 747c218e81cea1873e3ba13a84fe505aad9fc9f2
Author: Amit Kucheria <amit.kucheria@oss.qualcomm.com>

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

This commit adds per-SoC qfprom fuse support to read hardware binning
information at probe time and pass it to the thermal OF parser for
trip-point filtering on x1e80100, sc7280, and qcm6490.

>     thermal: qcom: tsens: add hardware-bin trip-point filtering
>     
>     Some Qualcomm SoCs are binned at manufacturing time and expose the bin
>     in qfprom fuses. The thermal OF parser can filter trip points by
>     hardware version; wire tsens up to feed it.
>     
>     Add the per-SoC plumbing:
>     
>      - struct tsens_hw_bin_desc names the nvmem cells to read and supplies a
>        compute_hw_version() callback that turns the raw fuse values into the
>        hw-version bitfields consumed by the thermal OF parser. A SoC opts in
>        by pointing tsens_plat_data.hw_bin_desc at one of these descriptors;
>        leaving it NULL disables the feature for that SoC.
>     
>      - tsens_populate_hw_bin_info() reads the named cells at probe time,
>        invokes the callback, and stashes the result in priv->hw_bin_info.
>        When hw_bin_desc is NULL it is a no-op, so SoCs that do not bin are
>        unaffected.
>     
>      - tsens_register() now uses devm_thermal_of_zone_register_with_bin(),
>        passing &priv->hw_bin_info only when a bin was actually read.
>     
>     Two descriptors and their SoC wiring are provided:
>     
>      - 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.
>     
>      - tsens_hw_bin_desc_qcm6490 reads the jtag-id and feat-id fuses and maps
>        the known qcm6490/sc7280 ID pairs to a synthesized hw-bin value (0x1
>        for the standard profile, 0x2 for the elevated profile); used by
>        data_sc7280, which serves both the sc7280 and qcm6490 compatibles.
>     
>     Register the qcm6490, sc7280 and x1e80100 compatibles in tsens_table.
>     
>     Signed-off-by: Amit Kucheria <amit.kucheria@oss.qualcomm.com>
>     Co-developed-by: Gaurav Kohli <gaurav.kohli@oss.qualcomm.com>
>     Signed-off-by: Gaurav Kohli <gaurav.kohli@oss.qualcomm.com>

[Severity: Low]
The commit message states that tsens_hw_bin_desc_i_temp reads a single
i-temp bit and reports it directly as the hw-bin value.

In tsens_compute_hw_version_i_temp(), the single bit (fuse[0] & BIT(0)) is
actually mapped to bitmask values 0x1u for the standard bin or 0x2u for the
high-temp bin:

	/* Single bit: set bit 1 for high-temp bin, bit 0 for standard bin */
	hw[0]  = (fuse[0] & BIT(0)) ? 0x2u : 0x1u;

Should the commit message be updated to match what
tsens_compute_hw_version_i_temp() does?

> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index b5ec70201e2f3..9ba6b3c4c133a 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c

[ ... ]

> +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;
> +}

[ ... ]

> @@ -1425,6 +1601,14 @@ static int tsens_probe(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);

[Severity: Medium]
How should errors from tsens_populate_hw_bin_info() be handled in
tsens_probe()?

In tsens_populate_hw_bin_info(), when nvmem_cell_read_variable_le_u32()
returns a negative error code, dev_err() is unconditionally called before
returning ret.

When the qfprom nvmem provider has not probed yet,
nvmem_cell_read_variable_le_u32() returns -EPROBE_DEFER. Although
tsens_probe() checks for -EPROBE_DEFER and returns early,
tsens_populate_hw_bin_info() has already logged a dev_err() message on every
deferred probe attempt.

When booting with an older or existing DTB for qcom,x1e80100-tsens or
qcom,sc7280-tsens that omits the optional nvmem-cells property,
nvmem_cell_read_variable_le_u32() returns -ENOENT. That causes
tsens_populate_hw_bin_info() to emit a dev_err() and tsens_probe() to emit a
redundant dev_warn() (which also passes &pdev->dev instead of the resolved
dev pointer).

Conversely, if devm_kmemdup() in tsens_populate_hw_bin_info() fails and
returns -ENOMEM, tsens_probe() only aborts on -EPROBE_DEFER and downgrades
-ENOMEM to the fuse warning instead of failing the probe.

> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index e8376accdff3d..1ebdc6935b794 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

[Severity: Low]
Is the kernel-doc for @cell_names in struct tsens_hw_bin_desc accurate in
saying it is a NULL-terminated array?

In drivers/thermal/qcom/tsens.c, tsens_cells_qcm6490 and tsens_cells_i_temp
are defined without a NULL terminator and initialize .ncells using
ARRAY_SIZE():

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,
};

Since tsens_populate_hw_bin_info() iterates from 0 to desc->ncells - 1 and
passes desc->cell_names[i] to nvmem_cell_read_variable_le_u32(), adding a
NULL terminator alongside ARRAY_SIZE() would pass a NULL cell_id to
nvmem_cell_read_variable_le_u32(). Should the NULL-terminated wording be
dropped from the comment?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-thermal_bin-v2-0-d76fd073ae98@oss.qualcomm.com?part=4