| 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
| ||
|
Message-ID: <OS0PR01MB5922328753110C5C35101357863DA@OS0PR01MB5922.jpnprd01.prod.outlook.com>
Date: Sun, 23 Jul 2023 06:50:29 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.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()
> Subject: RE: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
>
> Hi Dmitry,
>
> Thanks for the feedback.
>
> > 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.
>
> Looks like most of the i2c client driver uses similar handling for
> ACPI/DT and ID tables. If that is the case, it is good to have this
> proposed change which will simplify most of the drivers listed in [1]
>
> [1]
> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir
> .bootlin.com%2Flinux%2Flatest%2FA%2Fident%2Fi2c_match_id&data=05%7C01%7C
> biju.das.jz%40bp.renesas.com%7C2a07c353ab7649fdf29a08db8b42cca3%7C53d825
> 71da1947e49cb4625a166a4a2a%7C0%7C0%7C638256891245437404%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%
> 3D%7C3000%7C%7C%7C&sdata=tOxuTgGKc%2FQYFx94rYUJ8TDTWmGKkETzASV3qUjP2vk%3
> D&reserved=0
>
> Eg: drivers/hwmon/pmbus/ibm-cffps.c
>
> enum versions vs = cffps_unknown;
> const void *md = of_device_get_match_data(&client->dev);
> const struct i2c_device_id *id;
>
> if (md) {
> vs = (enum versions)md;
> } else {
> id = i2c_match_id(ibm_cffps_id, client);
> if (id)
> vs = (enum versions)id->driver_data;
> }
>
> The above code can be converted to
> vs = (enum versions)device_get_match_data(&client->dev);
>
> >
> > >
> > > Moreover, we need to avoid code duplication with [1]
> > >
> > > [1]
> >
> > If and when my proposed solution gets into the kernel we can drop
> > i2c_get_match_data() altogether.
>
> Agreed. Will wait for other people's view on this topic.
Also remove spi_get_device_match_data and
Make i2c_match_id() and spi_get_device_id() as static and
Replace all these with device_get_natch_data() from all i2c/spi client drivers.
Can you please post a patch based on this?
Cheers,
Biju
Powered by blists - more mailing lists