linux-kernel - Re: [PATCH 2/2] mmc: sdhci-of-ma35d1: Add Novoton MA35D1 SDHCI driver

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: <CAHp75VfpPR3Nat2dJrwLaxvnQNmn6KbpAfLcD-BvadwyHXDE1A@mail.gmail.com>
Date: Fri, 21 Jun 2024 13:25:52 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Shan-Chun Hung <shanchun1218@...il.com>
Cc: ulf.hansson@...aro.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, adrian.hunter@...el.com, p.zabel@...gutronix.de, 
	pbrobinson@...il.com, serghox@...il.com, mcgrof@...nel.org, 
	prabhakar.mahadev-lad.rj@...renesas.com, forbidden405@...look.com, 
	tmaimon77@...il.com, linux-arm-kernel@...ts.infradead.org, 
	linux-mmc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, ychuang3@...oton.com, schung@...oton.com
Subject: Re: [PATCH 2/2] mmc: sdhci-of-ma35d1: Add Novoton MA35D1 SDHCI driver

On Fri, Jun 21, 2024 at 10:06 AM Shan-Chun Hung <shanchun1218@...il.com> wrote:
> On 2024/6/20 上午 03:09, Andy Shevchenko wrote:
> > On Wed, Jun 19, 2024 at 7:47 AM Shan-Chun Hung<shanchun1218@...il.com>  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