netdev - Re: [PATCH v9 08/24] wfx: add bus

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]

Date:   Wed, 12 Jan 2022 16:03:56 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Pali Rohár <pali@...nel.org>,
        Jérôme Pouiller <Jerome.Pouiller@...abs.com>
Cc:     linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        Kalle Valo <kvalo@...eaurora.org>, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "David S . Miller" <davem@...emloft.net>,
        devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        linux-mmc@...r.kernel.org
Subject: Re: [PATCH v9 08/24] wfx: add bus_sdio.c

On Wed, 12 Jan 2022 at 12:43, Pali Rohár <pali@...nel.org> wrote:
>
> On Wednesday 12 January 2022 12:18:58 Jérôme Pouiller wrote:
> > On Wednesday 12 January 2022 11:58:59 CET Pali Rohár wrote:
> > > On Tuesday 11 January 2022 18:14:08 Jerome Pouiller wrote:
> > > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > > +     { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
> > > > +     { },
> > > > +};
> > >
> > > Hello! Is this table still required?
> >
> > As far as I understand, if the driver does not provide an id_table, the
> > probe function won't be never called (see sdio_match_device()).
> >
> > Since, we rely on the device tree, we could replace SDIO_VENDOR_ID_SILABS
> > and SDIO_DEVICE_ID_SILABS_WF200 by SDIO_ANY_ID. However, it does not hurt
> > to add an extra filter here.
>
> Now when this particular id is not required, I'm thinking if it is still
> required and it is a good idea to define these SDIO_VENDOR_ID_SILABS
> macros into kernel include files. As it would mean that other broken
> SDIO devices could define these bogus numbers too... And having them in
> common kernel includes files can cause issues... e.g. other developers
> could think that it is correct to use them as they are defined in common
> header files. But as these numbers are not reliable (other broken cards
> may have same ids as wf200) and their usage may cause issues in future.
>
> Ulf, any opinion?

The sdio_match_device() is what is being used to match the device to
its sdio_driver, which is being called from the sdio_bus_type's
->match() callback.

In regards to the DT compatible strings from a drivers'
.of_match_table, that is currently left to be matched by the sdio
driver's ->probe() function internally, by calling
of_driver_match_device().

In other words, I think what Jerome has suggested here seems
reasonable to me. Matching on "SDIO_ANY_ID" would work too, but I
think it's better with a poor filter like SDIO_VENDOR_ID_SILABS*,
rather than none.

An entirely different and new approach would be to extend
sdio_match_device() to call of_driver_match_device() too. However, in
that case we would also need to add a new corresponding ->probe()
callback for the sdio_driver, as the current one takes a const struct
sdio_device_id, which doesn't work when matching on DT compatibles.

>
> Btw, is there any project which maintains SDIO ids, like there is
> pci-ids.ucw.cz for PCI or www.linux-usb.org/usb-ids.html for USB?
>
> > > > +MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
> > > > +
> > > > +struct sdio_driver wfx_sdio_driver = {
> > > > +     .name = "wfx-sdio",
> > > > +     .id_table = wfx_sdio_ids,
> > > > +     .probe = wfx_sdio_probe,
> > > > +     .remove = wfx_sdio_remove,
> > > > +     .drv = {
> > > > +             .owner = THIS_MODULE,
> > > > +             .of_match_table = wfx_sdio_of_match,
> > > > +     }
> > > > +};
> > > > --
> > > > 2.34.1
> > > >
> > >
> >
> >
> > --
> > Jérôme Pouiller

Kind regards
Uffe