On Fri, Jun 21, 2024 at 10:06 AM Shan-Chun Hung <shanchun1218@xxxxxxxxx> wrote:
> On 2024/6/20 上午 03:09, Andy Shevchenko wrote:
> > On Wed, Jun 19, 2024 at 7:47 AM Shan-Chun Hung<shanchun1218@xxxxxxxxx> wrote:
...
> > You are missing a lot of header inclusions, please follow IWYU principle.
> I am not familiar with IWYU yet, but I will learn it and use it for
> checks later on.
"Include What You Use". But some of the headers may be omitted as they
are guaranteed to be included by others. It's a bit hard because one
should know and follow the kernel development, currently the headers
in the kernel are a bit of a mess.
...
> >> +#define BOUNDARY_OK(addr, len) \
> >> + ((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
> > Besides sizes.h being missed, this can be done with help of ALIGN()
> > macro (or alike). So, kill this and use the globally defined macro
> > inline.
> I will add sizes.h and directly apply globally defined ALIGN() macro
> instead
Also check what header should be included for that macro, IIRC it's align.h.
...
> >> + for (idx = 0; idx < ARRAY_SIZE(restore_data); idx++) {
> >> + if (restore_data[idx].width == 32)
> > sizeof(u32) ?
> Your idea is better, I will change it.
You might probably want to use the same in the restore_data array initialiser.
> >> + val[idx] = sdhci_readl(host, restore_data[idx].reg);
> >> + else if (restore_data[idx].width == 8)
> > sizeof(u8) ?
> I will fix it.
> >> + val[idx] = sdhci_readb(host, restore_data[idx].reg);
> >> + }
...
> >> + pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> >> + if (IS_ERR(pltfm_host->clk)) {
> >> + err = PTR_ERR(pltfm_host->clk);
> >> + dev_err(&pdev->dev, "failed to get clk: %d\n", err);
> > Use
> >
> > return dev_err_probe(...);
> I will use dev_err_probe() instead of dev_err()
> >> + goto free_pltfm;
> > This is wrong. You may not call non-devm before devm ones, otherwise
> > it makes a room for subtle mistakes on remove-probe or unbind-bind
> > cycles. Have you tested that?
> I have tested it, there is no error messages during driver initial process.
>
> My thought is that sdhci_pltfm_init() and sdhci_pltfm_free() are used together.
>
> If there's any error after the successful execution of sdhci_pltfm_init(),
> I'll use sdhci_pltfm_free().
>
> I am not entirely sure if this answers your question.
Yes, they are, the problem is that freeing resources happens in
non-reversed order (for some of the resources). This might lead to
subtle mistakes as I said above. The rule of thumb is to avoid mixing
devm_*() with non-devm_*() calls. If you have both, they have to be
grouped as all devm_*() followed by all non-devm_*().
In some cases you might need to wrap existing calls to become managed.
This may be done with the help of devm_add_action_or_reset(). Check
other drivers which are using that.
> >> + }
> >> + err = clk_prepare_enable(pltfm_host->clk);
> >> + if (err)
> >> + goto free_pltfm;
> > Use _enabled variant of devm_clk_get() instead.
> I will use devm_clk_get_optional_enabled() instead.
> >> + }
...
> >> +free_pltfm:
> >> + sdhci_pltfm_free(pdev);
> > This should go to be correct in ordering.
>
> I am not entirely sure if it is similar to the "goto free_pltfm;" issue.
Yes. It's part of the same issue.
> >> + return err;
> >> +}
> >> +
> >> +static int ma35_remove(struct platform_device *pdev)
> > Use remove_new callback.
> I will fix it.
> >> +{
> >> + struct sdhci_host *host = platform_get_drvdata(pdev);
> >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >> +
> >> + sdhci_remove_host(host, 0);
> >> + clk_disable_unprepare(pltfm_host->clk);
> >> + sdhci_pltfm_free(pdev);
> > At least these two will go away as per probe error path.
> I will use sdhci_pltfm_remove instead of the ma35_remove.
After fixing the ordering issues in ->probe() this might need more
modifications.
> >> + return 0;
> >> +}
--
With Best Regards,
Andy Shevchenko
