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@xxxxxxxxxxxx>
> ---
> 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
