| Message ID | 44b2ad71-dc4b-801c-237f-9c233f675c0d@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: fix erroneous logic inversion in spi_match_id() usage | expand |
On Tue, Nov 30, 2021 at 09:13:35PM +0100, Heiner Kallweit wrote: > We want to continue in case of a match. Fix the erroneously inverted > logic. We do? Why? I can't tell from this changelog what the problem is or why the patch would fix it. > @@ -471,10 +471,7 @@ int __spi_register_driver(struct module *owner, struct spi_driver *sdrv) > of_name = of_id->compatible; > > if (sdrv->id_table) { > - const struct spi_device_id *spi_id; > - > - spi_id = spi_match_id(sdrv->id_table, of_name); > - if (!spi_id) > + if (spi_match_id(sdrv->id_table, of_name)) > continue; > } else { > if (strcmp(sdrv->driver.name, of_name) == 0) This appears to correspond to the current code anyway?
On 01.12.2021 16:41, Mark Brown wrote: > On Tue, Nov 30, 2021 at 09:13:35PM +0100, Heiner Kallweit wrote: > >> We want to continue in case of a match. Fix the erroneously inverted >> logic. > > We do? Why? I can't tell from this changelog what the problem is or > why the patch would fix it. > That's the relevant part of 3f07657506df. Before this change we hit the continue path if spi_id->name[0] != NULL, means if a match was found. spi_match_id() returns NULL if no match was found. The commit message of 3f07657506df doesn't mention an intention to change the logic. @@ -474,12 +473,8 @@ int __spi_register_driver(struct module *owner, struct spi_driver *sdrv) if (sdrv->id_table) { const struct spi_device_id *spi_id; - for (spi_id = sdrv->id_table; spi_id->name[0]; - spi_id++) - if (strcmp(spi_id->name, of_name) == 0) - break; - - if (spi_id->name[0]) + spi_id = spi_match_id(sdrv->id_table, of_name); + if (!spi_id) continue; } else { >> @@ -471,10 +471,7 @@ int __spi_register_driver(struct module *owner, struct spi_driver *sdrv) >> of_name = of_id->compatible; >> >> if (sdrv->id_table) { >> - const struct spi_device_id *spi_id; >> - >> - spi_id = spi_match_id(sdrv->id_table, of_name); >> - if (!spi_id) >> + if (spi_match_id(sdrv->id_table, of_name)) >> continue; >> } else { >> if (strcmp(sdrv->driver.name, of_name) == 0) > > This appears to correspond to the current code anyway? > No. Now we (again) continue if spi_match_id() != NULL.
On Wed, Dec 01, 2021 at 07:49:19PM +0100, Heiner Kallweit wrote: > On 01.12.2021 16:41, Mark Brown wrote: > > On Tue, Nov 30, 2021 at 09:13:35PM +0100, Heiner Kallweit wrote: > > > >> We want to continue in case of a match. Fix the erroneously inverted > >> logic. > > > > We do? Why? I can't tell from this changelog what the problem is or > > why the patch would fix it. Isn't it fixed by b79332ef9d61 ("spi: Fix condition in the __spi_register_driver()")?
On 01.12.2021 20:18, Andy Shevchenko wrote: > On Wed, Dec 01, 2021 at 07:49:19PM +0100, Heiner Kallweit wrote: >> On 01.12.2021 16:41, Mark Brown wrote: >>> On Tue, Nov 30, 2021 at 09:13:35PM +0100, Heiner Kallweit wrote: >>> >>>> We want to continue in case of a match. Fix the erroneously inverted >>>> logic. >>> >>> We do? Why? I can't tell from this changelog what the problem is or >>> why the patch would fix it. > > Isn't it fixed by b79332ef9d61 ("spi: Fix condition in the > __spi_register_driver()")? > > Indeed, it has been fixed with this commit. I was using linux-next from Nov 24th that included the first change but not yet the fix. Sorry for the noise.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 5bf680fcb..4578c2fb5 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -471,10 +471,7 @@ int __spi_register_driver(struct module *owner, struct spi_driver *sdrv) of_name = of_id->compatible; if (sdrv->id_table) { - const struct spi_device_id *spi_id; - - spi_id = spi_match_id(sdrv->id_table, of_name); - if (!spi_id) + if (spi_match_id(sdrv->id_table, of_name)) continue; } else { if (strcmp(sdrv->driver.name, of_name) == 0)
We want to continue in case of a match. Fix the erroneously inverted logic. Fixes: 3f07657506df ("spi: deduplicate spi_match_id() in __spi_register_driver()") Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/spi/spi.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)