linux-kernel - Re: [PATCH v7 3/3] mmc: Add driver for LiteX's LiteSDCard interface

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAHp75Ve5T-yNV-BJww_kN+6y8P9FyHodKfZ4nfi2POynp6BPVg@mail.gmail.com>
Date:   Fri, 7 Jan 2022 22:50:02 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     "Gabriel L. Somlo" <gsomlo@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        linux-mmc <linux-mmc@...r.kernel.org>,
        Karol Gugala <kgugala@...micro.com>,
        Mateusz Holenko <mholenko@...micro.com>,
        Kamil Rakoczy <krakoczy@...micro.com>,
        mdudek@...ernships.antmicro.com,
        Paul Mackerras <paulus@...abs.org>,
        Joel Stanley <joel@....id.au>,
        Stafford Horne <shorne@...il.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        david.abdurachmanov@...ive.com,
        Florent Kermarrec <florent@...oy-digital.fr>,
        Randy Dunlap <rdunlap@...radead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 3/3] mmc: Add driver for LiteX's LiteSDCard interface

On Fri, Jan 7, 2022 at 7:08 PM Gabriel L. Somlo <gsomlo@...il.com> wrote:
> On Fri, Jan 07, 2022 at 12:06:16PM -0500, Gabriel Somlo wrote:

...

> > Cc: Mateusz Holenko <mholenko@...micro.com>
> > Cc: Karol Gugala <kgugala@...micro.com>
> > Cc: Joel Stanley <joel@....id.au>
> > Cc: Stafford Horne <shorne@...il.com>
> > Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
> > Cc: David Abdurachmanov <david.abdurachmanov@...ive.com>
> > Cc: Florent Kermarrec <florent@...oy-digital.fr>

It would be nice if you can use `git send-email --cc ...` instead of
putting a long list into a commit message.

...

> It looked to me like you thought I should `#include <linux/bits.h>` here
> (even though I'm not getting any compiler warnings regarding it). If so,
> why? If not, apologies for the misunderstanding :)

The rule of thumb is to explicitly use the headers you are the direct
user of with the remark that some of them are guaranteed to be
included by others and some of them should be used (in most cases)
instead of their low-level parts (the example is types.h vs
compiler_attributes.h, so former is more standard than the letter and
it's almost 100% guarantee you want to have something from types.h
anyway in your code).

So, BIT() is defined in bits.h and in the below list none of the
header _guarantees_ its indirect inclusion.

> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/litex.h>
> > +#include <linux/module.h>
> > +#include <linux/mmc/host.h>
> > +#include <linux/mmc/mmc.h>
> > +#include <linux/mmc/sd.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>

...

> Any more ordering or devm vs. non-devm mixing violations here? If so,
> can you please link me to an example or some docs where I ould figure
> out what it is I'm still doing wrong?

Device managed resources are attached to the instance of the device
object and removed in the order they have been attached to, but with
the caveat that they have no clue about non-managed calls in between.
Now you may figure out what happens. Ex.:

probe()
  A
  devm_B
  C
  devm_D

remove()
  un_C
  un_A

WRONG!

> > +static int litex_mmc_remove(struct platform_device *pdev)
> > +{
> > +     struct litex_mmc_host *host = dev_get_drvdata(&pdev->dev);
> > +     struct mmc_host *mmc = host->mmc;
> > +
> > +     mmc_remove_host(mmc);
> > +     if (host->irq > 0)
> > +             free_irq(host->irq, mmc);
> > +     mmc_free_host(mmc);
> > +
> > +     return 0;
> > +}
>
> Ditto here...

Ditto.

...

> > +             .of_match_table = of_match_ptr(litex_match),
>
> You said "Wrong usage of of_match_ptr()" here, and all I have to go by
> is a bunch of other `drivers/mmc/host/*.c` files that use it in a
> similar way, so can you please clarify and/or provide an example of how
> to do it properly?

First of all, you have a dependency to OF, try to remove it and
compile with OF=n and you will immediately see the issue. You may also
go for  `git log --no-merges --grep of_match_ptr` and analyze the
result.

-- 
With Best Regards,
Andy Shevchenko