linux-kernel - Re: [PATCH v2 1/2] Input: exc3000

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: <CAKdAkRT9tMnLnDLgWAevE_4HQ0wYMPehvsYaAeYrXdGGiyjXRA@mail.gmail.com>
Date:   Sat, 22 Jul 2023 18:17:02 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Biju Das <biju.das.jz@...renesas.com>
Cc:     Mark Brown <broonie@...nel.org>,
        Mike Looijmans <mike.looijmans@...ic.nl>,
        Andreas Helbech Kleist <andreaskleist@...il.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...renesas.com>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()

On Sat, Jul 22, 2023 at 05:51:17PM +0000, Biju Das wrote:
> Hi Dmitry Torokhov,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
> >
> > On Wed, Jul 19, 2023 at 06:43:47AM +0000, Biju Das wrote:
> > > Hi Dmitry Torokhov,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
> > > >
> > > > On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote:
> > > > > Hi Dmitry,
> > > > >
> > > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
> > > > > >
> > > > > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote:
> > > > > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote:
> > > > > > >
> > > > > > > > The .device_get_match_data callbacks are missing for I2C and
> > > > > > > > SPI bus
> > > > > > subsystems.
> > > > > > > > Can you please throw some lights on this?
> > > > > > >
> > > > > > > It's the first time I've ever heard of that callback, I don't
> > > > > > > know why whoever added it wouldn't have done those buses in
> > > > > > > particular or if it just didn't happen.  Try adding it and if
> > > > > > > it works send
> > > > the patches?
> > > > > >
> > > > > > I think there is a disconnect. Right now device_get_match_data
> > > > > > callbacks are part of fwnode_operations. I was proposing to add
> > > > > > another optional device_get_match_data callback to 'struct
> > bus_type'
> > > > > > to allow individual buses control how match data is handled,
> > > > > > before (or after) jumping into the fwnode-backed
> > > > > > device_get_match_data
> > > > callbacks.
> > > > >
> > > > > That is what implemented here [1] and [2] right?
> > > > >
> > > > >
> > > > > First it check for fwnode-backed device_get_match_data callbacks
> > > > > and Fallback is bus-type based match.
> > > > >
> > > > > Looks like you are proposing to unify [1] and [2] and you want the
> > > > > logic to be other way around. ie, first bus-type match, then
> > > > > fwnode-backed callbacks?
> > > > >
> > > >
> > > > I do not have a strong preference for the ordering, i.e. I think it
> > > > is perfectly fine to do the generic fwnode-based lookup and if there
> > > > is no match have bus method called as a fallback,
> > >
> > > That involves a bit of work.
> > >
> > > const void *device_get_match_data(const struct device *dev);
> > >
> > > const struct i2c_device_id *i2c_match_id(const struct i2c_device_id
> > *id,
> > >                                    const struct i2c_client *client);
> > >
> > > const struct spi_device_id *spi_get_device_id(const struct spi_device
> > > *sdev);
> > >
> > > Basically, the bus-client driver(such as exc3000) needs to pass struct
> > > device and device_get_match_data after generic fwnode-based lookup,
> > > needs to find the bus type based on struct device and call a new
> > > generic
> > > void* bus_get_match_data(void*) callback, so that each bus interface
> > > can do a match.
> >
> > Yes, something like this (which does not seem that involved to me...):
>
> Looks it will work.
>
> But there is some 2 additional checks in core code, every driver which is not bus type need to go through this checks.
>
> Also in Bus specific callback, there are 2 additional checks.
>
> So, performance wise [1] is better.

I do not believe this is a concern whatsoever: majority of
architectures/boards have been converted to ACPI/DT, which are being
matched first as they are now, so the fallback to bus-specific matching
against bus-specific device ID tables will be very infrequent.
Additionally, device_get_match_data() is predominantly called from
driver probe paths, so we need not be concerned with it being used with
class devices or other kinds of devices not associated with a bus.

>
> Moreover, we need to avoid code duplication with [1]
>
> [1] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L125

If and when my proposed solution gets into the kernel we can drop
i2c_get_match_data() altogether.

Thanks.


--
Dmitry