Hello Andy,
On Tue, Nov 01, 2022 at 04:54:52PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 01, 2022 at 12:38:43AM +0100, Uwe Kleine-König wrote:
> > On Mon, Oct 24, 2022 at 12:46:02PM +0300, Andy Shevchenko wrote:
> > > On Mon, Oct 24, 2022 at 11:14:56AM +0200, Uwe Kleine-König wrote:
>
> ...
>
> > > Exactly. And it means let's put my problem to someone's else shoulders.
> >
> > You have a problem that I fail to see. Why is defining the id table
> > before the probe function bad?
> >
> > Unless I misunderstand you, you seem to assume that in the nearer future
> > someone will have the urge to put the id table below the probe function
> > again. What would you think is their motivation?
>
> The problem with moving the table is the sparse locations in the code for
> semantically relative things, like all ID tables to be near to each other.
I don't understand that reasoning. Is that important for the compiler or
the human reader? What is "semantically relative"?
> With
> your approach you can easily break that and go for let's put one ID table on
> top, because some code fails to indirectly access it, and leave another
> somewhere else. I do not like this.
>
> Besides, your change making unneeded churn of "I like to move it, move it" for
> no real gain.
That's not true. It's not that I like to move it. Moving is necessary to
make use of the local symbol in .probe() without a forward declaration.
(If you claimed that adding a forward declaration was churn, I'd agree.)
> ...
>
> > > I don't see benefit of dereferencing tables by name. The table has to be
> > > available via struct driver, otherwise, how the heck we even got into the
> > > ->probe() there.
> >
> > It is possible, it's just cheaper (in cpu cycles) to calculate the
> > address of the table directly (i.e. via PC + $offset) instead of via
> > dereferencing two pointers.
>
> It's a slow path.
Somewhat agreed. It influences boot time though. (Well I guess, maybe
it's too little to be actually measureable for a single driver? Still in
my bubble people are sensitive to boot time and I'd consider this a low
hanging fruit for (admittedly small) optimisation.)
> ...
>
> > I fail to follow you again. I talked about drivers/rtc/rtc-isl1208.c as
> > it is in v6.1-rc1. There the probe function doesn't access the table at
> > all. Neither via the driver link nor by name. That driver just defines
> > the id table before the probe function.
>
> So, if it is on top, fine, it would be just an inconsistent style.
>
> ...
>
> > > Alternative is to avoid reordering to begin with, no?
> >
> > Yeah, that could be done. But I don't see the advantage and you fail to
> > explain it in a way for me to understand.
> >
> > So if i2c_match_id() was changed as follows:
>
> There is another approach in the discussion and Wolfram acknowledged it already
> (with a new API to retrieve the necessary data).
Yeah, saw it. And as expected the follow up patch converting
drivers/iio/pressure/bmp280-i2c.c "suffers" from the double pointer
dereference. But it looks nice because the effort to determine the table
via driver is well hidden.
> ...
>
> > +static const struct i2c_device_id kxcjk1013_id[] = {
> > + {"kxcjk1013", KXCJK1013},
> > + {"kxcj91008", KXCJ91008},
> > + {"kxtj21009", KXTJ21009},
> > + {"kxtf9", KXTF9},
> > + {"kx023-1025", KX0231025},
> > + {"SMO8500", KXCJ91008},
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);
> > +
> > static int kxcjk1013_probe(struct i2c_client *client)
> > {
> > - const struct i2c_device_id *id = i2c_match_id(NULL, client);
> > + const struct i2c_device_id *id = i2c_match_id(kxcjk1013_id, client);
> > struct kxcjk1013_data *data;
> > struct iio_dev *indio_dev;
> > struct kxcjk_1013_platform_data *pdata;
> > @@ -1720,18 +1731,6 @@ static const struct acpi_device_id kx_acpi_match[] = {
> > };
> > MODULE_DEVICE_TABLE(acpi, kx_acpi_match);
> >
> > -static const struct i2c_device_id kxcjk1013_id[] = {
> > - {"kxcjk1013", KXCJK1013},
> > - {"kxcj91008", KXCJ91008},
> > - {"kxtj21009", KXTJ21009},
> > - {"kxtf9", KXTF9},
> > - {"kx023-1025", KX0231025},
> > - {"SMO8500", KXCJ91008},
> > - {}
> > -};
> > -
> > -MODULE_DEVICE_TABLE(i2c, kxcjk1013_id);
>
> Can we avoid moving it?
Yes, but the alternatives are not better in my eyes. (You could use
i2c_client_get_device_id() or add a forward declaration.) And moving it
is easy enough and I still don't understand the downsides you care
about.
> > static const struct of_device_id kxcjk1013_of_match[] = {
> > { .compatible = "kionix,kxcjk1013", },
> > { .compatible = "kionix,kxcj91008", },
> >
> > on top to safe two pointer dereferences. The sum of the two driver
> > patches is exactly the effect of my patch just without the i2c core
> > change. (And the two pointer dereferences that are saved by the 2nd
> > patch are introduced by the first, so it's fine to not split that
> > change into two parts.)
>
> ...
>
> > > So, why not teach i2c_match_id() to handle this nicely for the caller?
> >
> > The metric for "nice" is obviously subjective. For me it's nice to pass
> > a local symbol to an API function to make the function's job a tad
> > easier and more effective to solve. And that even if I have to reorder
> > the caller a bit.
>
> So, it seems a preferred design paradigm: straightforward vs. OOP.
> Kernel is written with OOP in mind, why to avoid that?
>
> ...
>
> > > reduce churn with the using of current i2c_match_id() as you
> > > showed the long line to get that table.
> >
> > Do you still remember the original patch? That one doesn't have the long
> > i2c_match_id() line.
> >
> > (Do you see your statement is an argument for my approach? The long line
> > is an indication that it's complicated to determine the address of the
> > table via ->driver. You can hide that by pushing the needed effort into
> > i2c_match_id() or a macro, but in the end the complexity remains for the
> > CPU.)
>
> Does it matter?
What is "it"? The long line? (Then no, it doesn't matter because it
doesn't appear in neither your nor my approach.) The effort to pull the
table's address out of ->driver? (We seem to disagree. I think it's easy
enough not to do it to justify the optimisation.) Or to hide the effort?
(I think hiding effort and so making it easy to pick a more expensive
approach is at least questionable.)
> OTOH that will be aligned with SPI framework and idea behind ->probe_new()
> as I understood it.
>
> ...
>
> > > This might need a new API to avoid
> > > changing many drivers at once. But it's business as usual.
> >
> > My approach doesn't need a new API. That's nice, isn't it?
>
> It depends. In this case it's not nice since it requires
> "I like to move it, move it".
I still don't get your reasoning and I think we have to agree to not
agree. IMHO the judgement is subjective, you call the move of the id
table "churn" and I call it "tiny effort for a little optimisation".
What I consider "churn" though is this discussion. I will stop my
participation here. That's a bit sad because in my eyes all patches in
this series have a positive value and the discussion about (from my POV)
incomprehensible minor details destroyed my motivation to work on the
quest to convert all drivers to .probe_new() :-\
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
