linux-kernel - Re: [PATCH v1 01/12] soc: qcom: geni-se: Refactor geni_icc_get() and make qup-memory ICC path optional

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c4qgjg3npsi6dkvqyj2z5drd7mfg2w2o4cjjcgepxdsrgiyiic@qdpkcic56iwv>
Date: Wed, 26 Nov 2025 09:07:02 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Praveen Talari <praveen.talari@....qualcomm.com>
Cc: Andi Shyti <andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Mukesh Kumar Savaliya <mukesh.savaliya@....qualcomm.com>, Viken Dadhaniya <viken.dadhaniya@....qualcomm.com>, 
	Konrad Dybcio <konradybcio@...nel.org>, linux-arm-msm@...r.kernel.org, linux-i2c@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, psodagud@...cinc.com, 
	djaggi@...cinc.com, quic_msavaliy@...cinc.com, quic_vtanuku@...cinc.com, 
	quic_arandive@...cinc.com, quic_shazhuss@...cinc.com, 
	Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH v1 01/12] soc: qcom: geni-se: Refactor geni_icc_get() and
 make qup-memory ICC path optional

On Sat, Nov 22, 2025 at 10:30:07AM +0530, Praveen Talari wrote:
> Refactor the geni_icc_get() function to replace the loop-based ICC path
> initialization with explicit handling of each interconnect path. This
> improves code readability and allows for different error handling per
> path type.

I don't think this "improves code readability", IMO you're turning a
clean loop into a unrolled mess.


But then comes the least significant portion of your "problem
description" (i.e. the last words of it), where you indicate that this
would allow you to have different error handling for "qup-memory".

This is actually a valid reason to make this change, so say that!


> 
> The "qup-core" and "qup-config" paths remain mandatory, while "qup-memory"
> is now optional and skipped if not defined in DT.
> 

Please rewrite this message to _start_ with the problem description.
Make it clear on the first line/sentence why the change should be done.

E.g. compare with something like this:

"""
"qup-memory" is an optional interconnect path, unroll the geni_icc_get()
loop in order to allow specific error handling for this path.
"""

You only need to read 4 words to understand exactly why this patch
exists.

> Co-developed-by: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> Signed-off-by: Praveen Talari <praveen.talari@....qualcomm.com>
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 36 +++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index cd1779b6a91a..b6167b968ef6 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -899,30 +899,32 @@ EXPORT_SYMBOL_GPL(geni_se_rx_dma_unprep);
>  
>  int geni_icc_get(struct geni_se *se, const char *icc_ddr)
>  {
> -	int i, err;
> -	const char *icc_names[] = {"qup-core", "qup-config", icc_ddr};
> +	struct geni_icc_path *icc_paths = se->icc_paths;
>  
>  	if (has_acpi_companion(se->dev))
>  		return 0;
>  
> -	for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) {
> -		if (!icc_names[i])
> -			continue;
> -
> -		se->icc_paths[i].path = devm_of_icc_get(se->dev, icc_names[i]);
> -		if (IS_ERR(se->icc_paths[i].path))
> -			goto err;
> +	icc_paths[GENI_TO_CORE].path = devm_of_icc_get(se->dev, "qup-core");
> +	if (IS_ERR(icc_paths[GENI_TO_CORE].path))
> +		return dev_err_probe(se->dev, PTR_ERR(icc_paths[GENI_TO_CORE].path),
> +				     "Failed to get 'qup-core' ICC path\n");

To taste, but I think a local variable would be helpful to make this
less dense.

	path = devm_of_icc_get(se->dev, "qup-core");
	if (IS_ERR(path))
		return dev_err_probe(se->dev, PTR_ERR(path), "Failed to get 'qup-core' ICC path\n");
	icc_paths[GENI_TO_CORE].path = path;

Regards,
Bjorn

> +
> +	icc_paths[CPU_TO_GENI].path = devm_of_icc_get(se->dev, "qup-config");
> +	if (IS_ERR(icc_paths[CPU_TO_GENI].path))
> +		return dev_err_probe(se->dev, PTR_ERR(icc_paths[CPU_TO_GENI].path),
> +				     "Failed to get 'qup-config' ICC path\n");
> +
> +	/* The DDR path is optional, depending on protocol and hw capabilities */
> +	icc_paths[GENI_TO_DDR].path = devm_of_icc_get(se->dev, "qup-memory");
> +	if (IS_ERR(icc_paths[GENI_TO_DDR].path)) {
> +		if (PTR_ERR(icc_paths[GENI_TO_DDR].path) == -ENODATA)
> +			icc_paths[GENI_TO_DDR].path = NULL;
> +		else
> +			return dev_err_probe(se->dev, PTR_ERR(icc_paths[GENI_TO_DDR].path),
> +					     "Failed to get 'qup-memory' ICC path\n");
>  	}
>  
>  	return 0;
> -
> -err:
> -	err = PTR_ERR(se->icc_paths[i].path);
> -	if (err != -EPROBE_DEFER)
> -		dev_err_ratelimited(se->dev, "Failed to get ICC path '%s': %d\n",
> -					icc_names[i], err);
> -	return err;
> -
>  }
>  EXPORT_SYMBOL_GPL(geni_icc_get);
>  
> -- 
> 2.34.1
>