Re: [PATCH 4/4] memory: brcmstb_dpfe: introduce best-effort API detection

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 05/12/2023 19:47, Markus Mayer wrote:
> Add a best-effort probe function that tries all known DPFE versions to
> see if one might actually work. This helps in cases where device tree
> doesn't provide the proper version information for whatever reason. In

So for incomplete DTS you now add elaborate, own, custom matching
function. That's not how the code should work.

> that case, the driver may still be able to register if one of the known
> API versions ends up working.
> 
> Caveat: we have to skip "v1" during our best effort attempts. This is
> due to the fact that attempting a firmware download as required by v1
> will result in a memory access violation on anything but v1 hardware.
> This would crash the kernel. Since we don't know the HW version, we need
> to play it safe and skip v1.

None of this commit explains what is real problem being solved.


> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/memory/brcmstb_dpfe.c | 58 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
> index 0b0a9b85b605..15f4ee3b8535 100644
> --- a/drivers/memory/brcmstb_dpfe.c
> +++ b/drivers/memory/brcmstb_dpfe.c
> @@ -879,6 +879,50 @@ static int brcmstb_dpfe_resume(struct platform_device *pdev)
>  	return brcmstb_dpfe_download_firmware(priv);
>  }
>  
> +static int brcmstb_dpfe_probe_best_effort(struct platform_device *pdev)
> +{
> +	const char versioned_compat[] = "brcm,dpfe-cpu-v";
> +	const char v1_str[] = "-v1";
> +	const struct of_device_id *matches;
> +	const struct dpfe_api *orig_dpfe_api;
> +	struct device *dev = &pdev->dev;
> +	struct brcmstb_dpfe_priv *priv;
> +	int ret = -ENODEV;
> +
> +	priv = dev_get_drvdata(dev);
> +	orig_dpfe_api = priv->dpfe_api;
> +	matches = dev->driver->of_match_table;
> +
> +	/* Loop over all compatible strings */
> +	for (; matches->compatible[0]; matches++) {
> +		const char *compat = matches->compatible;
> +		/* Find the ones that start with "brcm,dpfe-cpu-v" */
> +		if (strstr(compat, versioned_compat) == compat) {
> +			char *v1_ptr = strstr(compat, v1_str);
> +			/*
> +			 * We must skip v1, since we don't know the hardware
> +			 * version and attempting a firmware download on v2 and
> +			 * newer would crash the kernel due to a memory access
> +			 * violation.
> +			 * We make sure to match "-v1" at the end of the string
> +			 * only.
> +			 */
> +			if (v1_ptr && v1_ptr[sizeof(v1_str)] == '\0')
> +				continue;
> +			priv->dpfe_api = matches->data;
> +			/* Fingers crossed... */
> +			ret = brcmstb_dpfe_download_firmware(priv);
> +			if (!ret)
> +				return 0;
> +		}
> +	}
> +
> +	/* It didn't work, so let's clean up. */
> +	priv->dpfe_api = orig_dpfe_api;
> +
> +	return ret;
> +}
> +
>  static int brcmstb_dpfe_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -923,8 +967,20 @@ static int brcmstb_dpfe_probe(struct platform_device *pdev)
>  	}
>  
>  	ret = brcmstb_dpfe_download_firmware(priv);
> +	if (ret && ret != -EPROBE_DEFER) {
> +		/*
> +		 * If the information provided by Device Tree didn't work, let's
> +		 * try all known version. Maybe one will work.

I don't understand how this comment is related to downloading firmware.

> +		 */
> +		dev_warn(dev,
> +			"DPFE v%d didn't work, reverting to best-effort\n",
> +			priv->dpfe_api->version);
> +		dev_warn(dev,
> +			"Device Tree and / or the driver should be updated\n");

You are now introducing new warnings?

NAK

Best regards,
Krzysztof