| Message ID | 20230804161728.394920-1-biju.das.jz@bp.renesas.com |
|---|---|
| Headers | show |
| Series | Extend device_get_match_data() to struct bus_type | expand |
Hi Jonathan Cameron, Thanks for the feedback. > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > bus_type > > On Fri, 4 Aug 2023 17:17:24 +0100 > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > This patch series extend device_get_match_data() to struct bus_type, > > so that buses like I2C can get matched data. > > > > There is a plan to replace > > i2c_get_match_data()->device_get_match_data() > > later, once this patch hits mainline as it is redundant. > > Are we sure we don't have any instances left of the pattern that used to > be common (typically for drivers where dt tables were added > later) of > > chip_info = device_get_match_data(); > if (!chip_info) { > chip_info = arrayofchipinfo[id->driver_data]; > } > > Looks like the first driver I checked, iio/adc/max1363.c does this still > and will I think break with this series. > > Or am I missing some reason this isn't a problem? Good catch, this will break. we need to make I2C table like OF/ACPI tables. Yes, before adding callback support to i2c_device_get_match_data(), We need to make similar table for OF/ACPI/I2C for all i2c drivers that use device_get_match_data()or of_get_device_match_data(). May be first intermediate step is to use i2c_get_match_data() For such table conversion. Once all table conversion is done then we can add i2c_device_get_match_data() callback. The below one is the recommendation from Andy. + * Besides the fact that some drivers abuse the device ID driver_data type + * and claim it to be integer, for the bus specific ID tables the driver_data + * may be defined as kernel_ulong_t. For these tables 0 is a valid response, + * but not for this function. It's recommended to convert those either to avoid + * 0 or use a real pointer to the predefined driver data. + */ > > Clearly this only matters if we get to the bus callback, but enabling that > is the whole point of this series. Hence I think a lot of auditing is > needed before this can be safely applied. Sure. Cheers, Biju > > Jonathan > > > v6->v7: > > * Added ack from Greg Kroah-Hartman for patch#1 > > * Swapped patch#2 and patch#3 from v6. > > * Added Rb tag from Andy for patch#2 and patch#4 > > * Updated commit description of patch#2 by removing unnecessary > wrapping. > > * Updated typo in commit description struct bus_type()->struct > bus_type. > > v5->v6: > > * Cced linux-rtc and linux-iio as these subsytems uses i2c_get_match_ > > data() and this function become redundant once this patch series hits > > mainline. > > * Added Rb tag from Sakari for patch#1. > > * Moved patch#3 from v5 to patch#2 and patch#2 from v5 to patch#4. > > * Added Rb tag from Andy for patch#2 > > * Separate patch#3 to prepare for better difference for > > i2c_match_id() changes. > > * Merged patch#4 from v5 with patch#4. > > v4->v5: > > * Added const struct device_driver variable 'drv' in > i2c_device_get_match > > _data(). > > * For code readability and maintenance perspective, added separate NULL > > check for drv and client variable and added comment for NULL check > for > > drv variable. > > * Created separate patch for converting i2c_of_match_device_sysfs() to > > non-static. > > * Removed export symbol for i2c_of_match_device_sysfs(). > > * Replaced 'dev->driver'->'drv'. > > * Replaced return value data->NULL to avoid (potentially) stale > pointers, > > if there is no match. > > v3->v4: > > * Documented corner case for device_get_match_data() > > * Dropped struct i2c_driver parameter from > > i2c_get_match_data_helper() > > * Split I2C sysfs handling in separate patch(patch#3) > > * Added space after of_device_id for i2c_of_match_device_sysfs() > > * Added const parameter for struct i2c_client, to prevent overriding > it's > > pointer. > > * Moved declaration from public i2c.h->i2c-core.h > > v2->v3: > > * Added Rb tag from Andy for patch#1. > > * Extended to support i2c_of_match_device() as suggested by Andy. > > * Changed i2c_of_match_device_sysfs() as non-static function as it is > > needed for i2c_device_get_match_data(). > > * Added a TODO comment to use i2c_verify_client() when it accepts const > > pointer. > > * Added multiple returns to make code path for device_get_match_data() > > faster in i2c_get_match_data(). > > RFC v1->v2: > > * Replaced "Signed-off-by"->"Suggested-by" tag for Dmitry. > > * Documented device_get_match_data(). > > * Added multiple returns to make code path for generic fwnode-based > > lookup faster. > > * Fixed build warnings reported by kernel test robot <lkp@intel.com> > > * Added const qualifier to return type and parameter struct i2c_driver > > in i2c_get_match_data_helper(). > > * Added const qualifier to struct i2c_driver in i2c_get_match_data() > > * Dropped driver variable from i2c_device_get_match_data() > > * Replaced to_i2c_client with logic for assigning verify_client as it > > returns non const pointer. > > > > Biju Das (4): > > drivers: fwnode: Extend device_get_match_data() to struct bus_type > > i2c: Enhance i2c_get_match_data() > > i2c: i2c-core-of: Convert i2c_of_match_device_sysfs() to non-static > > i2c: Add i2c_device_get_match_data() callback > > > > drivers/base/property.c | 27 ++++++++++++++++- > > drivers/i2c/i2c-core-base.c | 60 ++++++++++++++++++++++++++++++------- > > drivers/i2c/i2c-core-of.c | 4 +-- > > drivers/i2c/i2c-core.h | 9 ++++++ > > include/linux/device/bus.h | 3 ++ > > 5 files changed, 90 insertions(+), 13 deletions(-) > >
On Sat, 5 Aug 2023 17:42:21 +0000 Biju Das <biju.das.jz@bp.renesas.com> wrote: > Hi Jonathan Cameron, > > Thanks for the feedback. > > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > > bus_type > > > > On Fri, 4 Aug 2023 17:17:24 +0100 > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > This patch series extend device_get_match_data() to struct bus_type, > > > so that buses like I2C can get matched data. > > > > > > There is a plan to replace > > > i2c_get_match_data()->device_get_match_data() > > > later, once this patch hits mainline as it is redundant. > > > > Are we sure we don't have any instances left of the pattern that used to > > be common (typically for drivers where dt tables were added > > later) of > > > > chip_info = device_get_match_data(); > > if (!chip_info) { > > chip_info = arrayofchipinfo[id->driver_data]; > > } > > > > Looks like the first driver I checked, iio/adc/max1363.c does this still > > and will I think break with this series. > > > > Or am I missing some reason this isn't a problem? > > Good catch, this will break. > we need to make I2C table like OF/ACPI tables. > > Yes, before adding callback support to i2c_device_get_match_data(), > We need to make similar table for OF/ACPI/I2C for all i2c drivers > that use device_get_match_data()or of_get_device_match_data(). To throw another option out there, could we make the I2C subsystem use the of_device_id table in place of the i2c_device_id table? That is perform matches based only on the of_device_id table in all drivers (with some glue code making that work for the less common paths, remaining board files etc). The ACPI PRP0001 magic is doing similar already. I can't immediately see why that wouldn't work other than being a bit of a large job to implement in all drivers. Getting rid of the duplication would be good. Probably some rough corners to make it possible to do this in a gradual process. In particular some of the naming used for i2c_device_id table entries won't be 'valid' DT compatibles (minus the vendor id) > > May be first intermediate step is to use i2c_get_match_data() > For such table conversion. Once all table conversion is done > then we can add i2c_device_get_match_data() callback. > > The below one is the recommendation from Andy. > > + * Besides the fact that some drivers abuse the device ID driver_data type > + * and claim it to be integer, for the bus specific ID tables the driver_data > + * may be defined as kernel_ulong_t. For these tables 0 is a valid response, > + * but not for this function. It's recommended to convert those either to avoid > + * 0 or use a real pointer to the predefined driver data. > + */ We still need to maintain consistency across the two tables, which is a stronger requirement than avoiding 0. Some drivers already do that by forcing the enum used to start at 1 which doesn't solver the different data types issue. Jonathan > > > > > Clearly this only matters if we get to the bus callback, but enabling that > > is the whole point of this series. Hence I think a lot of auditing is > > needed before this can be safely applied. > > Sure. > > Cheers, > Biju > > > > > Jonathan > > > > > v6->v7: > > > * Added ack from Greg Kroah-Hartman for patch#1 > > > * Swapped patch#2 and patch#3 from v6. > > > * Added Rb tag from Andy for patch#2 and patch#4 > > > * Updated commit description of patch#2 by removing unnecessary > > wrapping. > > > * Updated typo in commit description struct bus_type()->struct > > bus_type. > > > v5->v6: > > > * Cced linux-rtc and linux-iio as these subsytems uses i2c_get_match_ > > > data() and this function become redundant once this patch series hits > > > mainline. > > > * Added Rb tag from Sakari for patch#1. > > > * Moved patch#3 from v5 to patch#2 and patch#2 from v5 to patch#4. > > > * Added Rb tag from Andy for patch#2 > > > * Separate patch#3 to prepare for better difference for > > > i2c_match_id() changes. > > > * Merged patch#4 from v5 with patch#4. > > > v4->v5: > > > * Added const struct device_driver variable 'drv' in > > i2c_device_get_match > > > _data(). > > > * For code readability and maintenance perspective, added separate NULL > > > check for drv and client variable and added comment for NULL check > > for > > > drv variable. > > > * Created separate patch for converting i2c_of_match_device_sysfs() to > > > non-static. > > > * Removed export symbol for i2c_of_match_device_sysfs(). > > > * Replaced 'dev->driver'->'drv'. > > > * Replaced return value data->NULL to avoid (potentially) stale > > pointers, > > > if there is no match. > > > v3->v4: > > > * Documented corner case for device_get_match_data() > > > * Dropped struct i2c_driver parameter from > > > i2c_get_match_data_helper() > > > * Split I2C sysfs handling in separate patch(patch#3) > > > * Added space after of_device_id for i2c_of_match_device_sysfs() > > > * Added const parameter for struct i2c_client, to prevent overriding > > it's > > > pointer. > > > * Moved declaration from public i2c.h->i2c-core.h > > > v2->v3: > > > * Added Rb tag from Andy for patch#1. > > > * Extended to support i2c_of_match_device() as suggested by Andy. > > > * Changed i2c_of_match_device_sysfs() as non-static function as it is > > > needed for i2c_device_get_match_data(). > > > * Added a TODO comment to use i2c_verify_client() when it accepts const > > > pointer. > > > * Added multiple returns to make code path for device_get_match_data() > > > faster in i2c_get_match_data(). > > > RFC v1->v2: > > > * Replaced "Signed-off-by"->"Suggested-by" tag for Dmitry. > > > * Documented device_get_match_data(). > > > * Added multiple returns to make code path for generic fwnode-based > > > lookup faster. > > > * Fixed build warnings reported by kernel test robot <lkp@intel.com> > > > * Added const qualifier to return type and parameter struct i2c_driver > > > in i2c_get_match_data_helper(). > > > * Added const qualifier to struct i2c_driver in i2c_get_match_data() > > > * Dropped driver variable from i2c_device_get_match_data() > > > * Replaced to_i2c_client with logic for assigning verify_client as it > > > returns non const pointer. > > > > > > Biju Das (4): > > > drivers: fwnode: Extend device_get_match_data() to struct bus_type > > > i2c: Enhance i2c_get_match_data() > > > i2c: i2c-core-of: Convert i2c_of_match_device_sysfs() to non-static > > > i2c: Add i2c_device_get_match_data() callback > > > > > > drivers/base/property.c | 27 ++++++++++++++++- > > > drivers/i2c/i2c-core-base.c | 60 ++++++++++++++++++++++++++++++------- > > > drivers/i2c/i2c-core-of.c | 4 +-- > > > drivers/i2c/i2c-core.h | 9 ++++++ > > > include/linux/device/bus.h | 3 ++ > > > 5 files changed, 90 insertions(+), 13 deletions(-) > > > >
Hi Jonathan Cameron, Thanks for the feedback. > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > bus_type > > On Sat, 5 Aug 2023 17:42:21 +0000 > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > > > bus_type > > > > > > On Fri, 4 Aug 2023 17:17:24 +0100 > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > > This patch series extend device_get_match_data() to struct > > > > bus_type, so that buses like I2C can get matched data. > > > > > > > > There is a plan to replace > > > > i2c_get_match_data()->device_get_match_data() > > > > later, once this patch hits mainline as it is redundant. > > > > > > Are we sure we don't have any instances left of the pattern that > > > used to be common (typically for drivers where dt tables were added > > > later) of > > > > > > chip_info = device_get_match_data(); > > > if (!chip_info) { > > > chip_info = arrayofchipinfo[id->driver_data]; > > > } > > > > > > Looks like the first driver I checked, iio/adc/max1363.c does this > > > still and will I think break with this series. > > > > > > Or am I missing some reason this isn't a problem? > > > > Good catch, this will break. > > we need to make I2C table like OF/ACPI tables. > > > > Yes, before adding callback support to i2c_device_get_match_data(), We > > need to make similar table for OF/ACPI/I2C for all i2c drivers that > > use device_get_match_data()or of_get_device_match_data(). > > To throw another option out there, could we make the I2C subsystem use the > of_device_id table in place of the i2c_device_id table? + device tree Not sure that require bindings to be updated as we are replacing name->compatible, and I guess 'check patch' will complain undocumented compatible as all the names in 'i2c_device_id' are not mapped to compatible in 'of_device_id'. for eg: drivers/rtc/ rtc-isl1208.c[1]?? https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/rtc/rtc-isl1208.c?h=next-20230804#n111 Cheers, Biju > That is perform matches based only on the of_device_id table in all > drivers (with some glue code making that work for the less common paths, > remaining board files etc). The ACPI PRP0001 magic is doing similar > already. > > I can't immediately see why that wouldn't work other than being a bit of a > large job to implement in all drivers. > > Getting rid of the duplication would be good. Probably some rough corners > to make it possible to do this in a gradual process. In particular some of > the naming used for i2c_device_id table entries won't be 'valid' > DT compatibles (minus the vendor id) > > > > > May be first intermediate step is to use i2c_get_match_data() For such > > table conversion. Once all table conversion is done then we can add > > i2c_device_get_match_data() callback. > > > > The below one is the recommendation from Andy. > > > > + * Besides the fact that some drivers abuse the device ID driver_data > > + type > > + * and claim it to be integer, for the bus specific ID tables the > > + driver_data > > + * may be defined as kernel_ulong_t. For these tables 0 is a valid > > + response, > > + * but not for this function. It's recommended to convert those > > + either to avoid > > + * 0 or use a real pointer to the predefined driver data. > > + */ > > We still need to maintain consistency across the two tables, which is a > stronger requirement than avoiding 0. Some drivers already do that by > forcing the enum used to start at 1 which doesn't solver the different > data types issue. > > Jonathan > > > > > > > > > Clearly this only matters if we get to the bus callback, but > > > enabling that is the whole point of this series. Hence I think a > > > lot of auditing is needed before this can be safely applied. > > > > Sure. > > > > Cheers, > > Biju > > > > > > > > Jonathan > > > > > > > v6->v7: > > > > * Added ack from Greg Kroah-Hartman for patch#1 > > > > * Swapped patch#2 and patch#3 from v6. > > > > * Added Rb tag from Andy for patch#2 and patch#4 > > > > * Updated commit description of patch#2 by removing unnecessary > > > wrapping. > > > > * Updated typo in commit description struct bus_type()->struct > > > bus_type. > > > > v5->v6: > > > > * Cced linux-rtc and linux-iio as these subsytems uses > i2c_get_match_ > > > > data() and this function become redundant once this patch series > hits > > > > mainline. > > > > * Added Rb tag from Sakari for patch#1. > > > > * Moved patch#3 from v5 to patch#2 and patch#2 from v5 to patch#4. > > > > * Added Rb tag from Andy for patch#2 > > > > * Separate patch#3 to prepare for better difference for > > > > i2c_match_id() changes. > > > > * Merged patch#4 from v5 with patch#4. > > > > v4->v5: > > > > * Added const struct device_driver variable 'drv' in > > > i2c_device_get_match > > > > _data(). > > > > * For code readability and maintenance perspective, added separate > NULL > > > > check for drv and client variable and added comment for NULL > > > > check > > > for > > > > drv variable. > > > > * Created separate patch for converting i2c_of_match_device_sysfs() > to > > > > non-static. > > > > * Removed export symbol for i2c_of_match_device_sysfs(). > > > > * Replaced 'dev->driver'->'drv'. > > > > * Replaced return value data->NULL to avoid (potentially) stale > > > pointers, > > > > if there is no match. > > > > v3->v4: > > > > * Documented corner case for device_get_match_data() > > > > * Dropped struct i2c_driver parameter from > > > > i2c_get_match_data_helper() > > > > * Split I2C sysfs handling in separate patch(patch#3) > > > > * Added space after of_device_id for i2c_of_match_device_sysfs() > > > > * Added const parameter for struct i2c_client, to prevent > > > > overriding > > > it's > > > > pointer. > > > > * Moved declaration from public i2c.h->i2c-core.h > > > > v2->v3: > > > > * Added Rb tag from Andy for patch#1. > > > > * Extended to support i2c_of_match_device() as suggested by Andy. > > > > * Changed i2c_of_match_device_sysfs() as non-static function as it > is > > > > needed for i2c_device_get_match_data(). > > > > * Added a TODO comment to use i2c_verify_client() when it accepts > const > > > > pointer. > > > > * Added multiple returns to make code path for > device_get_match_data() > > > > faster in i2c_get_match_data(). > > > > RFC v1->v2: > > > > * Replaced "Signed-off-by"->"Suggested-by" tag for Dmitry. > > > > * Documented device_get_match_data(). > > > > * Added multiple returns to make code path for generic fwnode-based > > > > lookup faster. > > > > * Fixed build warnings reported by kernel test robot > > > > <lkp@intel.com> > > > > * Added const qualifier to return type and parameter struct > i2c_driver > > > > in i2c_get_match_data_helper(). > > > > * Added const qualifier to struct i2c_driver in > > > > i2c_get_match_data() > > > > * Dropped driver variable from i2c_device_get_match_data() > > > > * Replaced to_i2c_client with logic for assigning verify_client as > it > > > > returns non const pointer. > > > > > > > > Biju Das (4): > > > > drivers: fwnode: Extend device_get_match_data() to struct bus_type > > > > i2c: Enhance i2c_get_match_data() > > > > i2c: i2c-core-of: Convert i2c_of_match_device_sysfs() to non- > static > > > > i2c: Add i2c_device_get_match_data() callback > > > > > > > > drivers/base/property.c | 27 ++++++++++++++++- > > > > drivers/i2c/i2c-core-base.c | 60 ++++++++++++++++++++++++++++++---- > --- > > > > drivers/i2c/i2c-core-of.c | 4 +-- > > > > drivers/i2c/i2c-core.h | 9 ++++++ > > > > include/linux/device/bus.h | 3 ++ > > > > 5 files changed, 90 insertions(+), 13 deletions(-) > > > > > >
On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote: > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote: > > On Sat, 5 Aug 2023 17:42:21 +0000 > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > On Fri, 4 Aug 2023 17:17:24 +0100 > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > ... > > > > + * Besides the fact that some drivers abuse the device ID driver_data type > > > + * and claim it to be integer, for the bus specific ID tables the driver_data > > > + * may be defined as kernel_ulong_t. For these tables 0 is a valid response, > > > + * but not for this function. It's recommended to convert those either to avoid > > > + * 0 or use a real pointer to the predefined driver data. > > > We still need to maintain consistency across the two tables, which > > is a stronger requirement than avoiding 0. > > True. Any suggestion how to amend the above comment? Because the documentation > makes sense on its own (may be split from the series?). > > > Some drivers already do that by forcing the enum used to start at 1 which > > doesn't solver the different data types issue. > > And some maintainers do not want to see non-enum values in i2c ID table. > *Shrug*. So in legacy ID lookup path we can safely assume that values below 4096 are scalars and return NULL from the new device_get_match_data(). This way current drivers using the values as indices or doing direct comparisons against them can continue doing manual look up and using them as they see fit. And we can convert the drivers at our leisure. Thanks.
On Mon, Aug 07, 2023 at 08:45:05PM +0100, Jonathan Cameron wrote: > On Mon, 7 Aug 2023 17:54:07 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote: > > > On Sat, 5 Aug 2023 17:42:21 +0000 > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > On Fri, 4 Aug 2023 17:17:24 +0100 > > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: ... > > > > + * Besides the fact that some drivers abuse the device ID driver_data type > > > > + * and claim it to be integer, for the bus specific ID tables the driver_data > > > > + * may be defined as kernel_ulong_t. For these tables 0 is a valid response, > > > > + * but not for this function. It's recommended to convert those either to avoid > > > > + * 0 or use a real pointer to the predefined driver data. > > > > > We still need to maintain consistency across the two tables, which > > > is a stronger requirement than avoiding 0. > > > > True. Any suggestion how to amend the above comment? Because the documentation > > makes sense on its own (may be split from the series?). > > For bus ID tables it is fine right now as long as no one checks for NULL. > I guess adding this to the i2c_get_match_data and spi equivalent wrapper > functions might avoid someone shooting themselves in the foot (I've done it > for starters more than once). Ah, okay. ... > > > Some drivers already do that by forcing the enum used to start at 1 which > > > doesn't solver the different data types issue. > > > > And some maintainers do not want to see non-enum values in i2c ID table. > > *Shrug*. > > That leaves us stuck unless we move to a form where the i2c ID table isn't > used if there is an of_device_id table (or maybe we invent yet another table > and if that is present it is used for dt and were i2c_device_id is used and > hence becomes an opt in? That will also be tricky however. This won't ever happen as it will break an ABI (as far as I understand the current state of affairs). Seems the easiest path is to try to sell the point that having pointers in I2C ID tables is okay to the maintainers who don't see it that way.
On Mon, Aug 07, 2023 at 01:37:12PM -0700, Dmitry Torokhov wrote: > On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote: > > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote: > > > On Sat, 5 Aug 2023 17:42:21 +0000 > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > On Fri, 4 Aug 2023 17:17:24 +0100 > > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: ... > > > > + * Besides the fact that some drivers abuse the device ID driver_data type > > > > + * and claim it to be integer, for the bus specific ID tables the driver_data > > > > + * may be defined as kernel_ulong_t. For these tables 0 is a valid response, > > > > + * but not for this function. It's recommended to convert those either to avoid > > > > + * 0 or use a real pointer to the predefined driver data. > > > > > We still need to maintain consistency across the two tables, which > > > is a stronger requirement than avoiding 0. > > > > True. Any suggestion how to amend the above comment? Because the documentation > > makes sense on its own (may be split from the series?). > > > > > Some drivers already do that by forcing the enum used to start at 1 which > > > doesn't solver the different data types issue. > > > > And some maintainers do not want to see non-enum values in i2c ID table. > > *Shrug*. > > So in legacy ID lookup path we can safely assume that values below 4096 > are scalars and return NULL from the new device_get_match_data(). This > way current drivers using the values as indices or doing direct > comparisons against them can continue doing manual look up and using > them as they see fit. And we can convert the drivers at our leisure. It's a good idea, but I believe will be received as hack. But why not to try? We indeed have an error pointers for the last page and NULL (which is only up to 16 IIRC) and reserved space in the first page. To be more robust I would check all enums that are being used in I2C ID tables for maximum value and if that is less than 16, use ZERO_OR_NULL_PTR() instead of custom stuff.
On Mon, 7 Aug 2023 13:37:12 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote: > > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote: > > > On Sat, 5 Aug 2023 17:42:21 +0000 > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > On Fri, 4 Aug 2023 17:17:24 +0100 > > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > ... > > > > > > + * Besides the fact that some drivers abuse the device ID driver_data type > > > > + * and claim it to be integer, for the bus specific ID tables the driver_data > > > > + * may be defined as kernel_ulong_t. For these tables 0 is a valid response, > > > > + * but not for this function. It's recommended to convert those either to avoid > > > > + * 0 or use a real pointer to the predefined driver data. > > > > > We still need to maintain consistency across the two tables, which > > > is a stronger requirement than avoiding 0. > > > > True. Any suggestion how to amend the above comment? Because the documentation > > makes sense on its own (may be split from the series?). > > > > > Some drivers already do that by forcing the enum used to start at 1 which > > > doesn't solver the different data types issue. > > > > And some maintainers do not want to see non-enum values in i2c ID table. > > *Shrug*. > > So in legacy ID lookup path we can safely assume that values below 4096 > are scalars and return NULL from the new device_get_match_data(). This > way current drivers using the values as indices or doing direct > comparisons against them can continue doing manual look up and using > them as they see fit. And we can convert the drivers at our leisure. Good idea. Though I suspect there may still be nasty cases. People have been known to put chip ID values in these fields so that they can then match them against a who am I register as a 'detect it's the right part' check. No idea if we have any drivers doing that but if there are hopefully not too many! Jonathan > > Thanks. >
On Wed, Aug 09, 2023 at 06:25:51PM +0100, Jonathan Cameron wrote: > On Tue, 8 Aug 2023 15:18:52 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Aug 07, 2023 at 01:37:12PM -0700, Dmitry Torokhov wrote: > > > On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote: > > > > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote: > > > > > On Sat, 5 Aug 2023 17:42:21 +0000 > > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > > > > On Fri, 4 Aug 2023 17:17:24 +0100 > > > > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: ... > > > > > > + * Besides the fact that some drivers abuse the device ID driver_data type > > > > > > + * and claim it to be integer, for the bus specific ID tables the driver_data > > > > > > + * may be defined as kernel_ulong_t. For these tables 0 is a valid response, > > > > > > + * but not for this function. It's recommended to convert those either to avoid > > > > > > + * 0 or use a real pointer to the predefined driver data. > > > > > > > > > We still need to maintain consistency across the two tables, which > > > > > is a stronger requirement than avoiding 0. > > > > > > > > True. Any suggestion how to amend the above comment? Because the documentation > > > > makes sense on its own (may be split from the series?). > > > > > > > > > Some drivers already do that by forcing the enum used to start at 1 which > > > > > doesn't solver the different data types issue. > > > > > > > > And some maintainers do not want to see non-enum values in i2c ID table. > > > > *Shrug*. > > > > > > So in legacy ID lookup path we can safely assume that values below 4096 > > > are scalars and return NULL from the new device_get_match_data(). This > > > way current drivers using the values as indices or doing direct > > > comparisons against them can continue doing manual look up and using > > > them as they see fit. And we can convert the drivers at our leisure. > > > > It's a good idea, but I believe will be received as hack. > > But why not to try? We indeed have an error pointers for the last page > > and NULL (which is only up to 16 IIRC) and reserved space in the first > > page. To be more robust I would check all enums that are being used > > in I2C ID tables for maximum value and if that is less than 16, use > > ZERO_OR_NULL_PTR() instead of custom stuff. > > > See iio/adc/max1363 example that has 37ish. > > Could tidy that one up first and hopefully not find any others that > are in subsystems not keen on the move away from enums. Oh, yep, this needs a treewide audit and fixes around before going further. As you said.
Hi Andy Shevchenko, > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > bus_type > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote: > > > On Tue, 8 Aug 2023 15:18:52 +0300 > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Aug 07, 2023 at 01:37:12PM -0700, Dmitry Torokhov wrote: > > > > > On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote: > > ... > > > > > > So in legacy ID lookup path we can safely assume that values > > > > > below > > > > > 4096 are scalars and return NULL from the new > > > > > device_get_match_data(). This way current drivers using the > > > > > values as indices or doing direct comparisons against them can > > > > > continue doing manual look up and using them as they see fit. > > > > > And we can > > > convert the drivers at our leisure. > > > > > > > > It's a good idea, but I believe will be received as hack. > > > > But why not to try? We indeed have an error pointers for the last > > > > page and NULL (which is only up to 16 IIRC) and reserved space in > > > > the first page. To be more robust I would check all enums that are > > > > being used in I2C ID tables for maximum value and if that is less > > > > than 16, use > > > > ZERO_OR_NULL_PTR() instead of custom stuff. > > > > > > > See iio/adc/max1363 example that has 37ish. > > > > > > Could tidy that one up first and hopefully not find any others that > > > are in subsystems not keen on the move away from enums. > > > > If there is no objection, I can fix this using i2c_get_match_data() > > for > > iio/adc/max1363 and device_match_data() will return ZERO_OR_NULL_PTR() > > if max enum ID in the ID lookup table is less than 16. And the drivers > > that use legacy ID's will fallback to ID lookup. So that there won't > > be any regression. > > I'm good with this approach, but make sure you checked the whole kernel > source tree for a such. Checking against 16 is too short I guess?? drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums. /*device enum */ enum inv_devices { INV_MPU6050, INV_MPU6500, INV_MPU6515, INV_MPU6880, INV_MPU6000, INV_MPU9150, INV_MPU9250, INV_MPU9255, INV_ICM20608, INV_ICM20608D, INV_ICM20609, INV_ICM20689, INV_ICM20600, INV_ICM20602, INV_ICM20690, INV_IAM20680, INV_NUM_PARTS }; The new helper function +static bool i2c_is_client_uses_legacy_id_table(const struct i2c_driver *driver) +{ + const struct i2c_device_id *id = driver->id_table; + kernel_ulong_t max_val = 0; + + if (!id) + return FALSE; + + while (id->name[0]) { + if (id->driver_data > max_val) + max_val = id->driver_data; + id++; + } + + return ZERO_OR_NULL_PTR(max_val); +} + Cheers, Biju
On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote: > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote: ... > > I'm good with this approach, but make sure you checked the whole kernel > > source tree for a such. > > Checking against 16 is too short I guess?? > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums. So, what does prevent us from moving that tables to use pointers? > /*device enum */ > enum inv_devices { > INV_MPU6050, > INV_MPU6500, > INV_MPU6515, > INV_MPU6880, > INV_MPU6000, > INV_MPU9150, > INV_MPU9250, > INV_MPU9255, > INV_ICM20608, > INV_ICM20608D, > INV_ICM20609, > INV_ICM20689, > INV_ICM20600, > INV_ICM20602, > INV_ICM20690, > INV_IAM20680, > INV_NUM_PARTS > }; > > The new helper function You mean for debugging? We don't need that in production. I think what you need is a coccinelle script to find these.
Hi Andy, > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > bus_type > > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote: > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote: > > ... > > > > I'm good with this approach, but make sure you checked the whole > > > kernel source tree for a such. > > > > Checking against 16 is too short I guess?? > > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums. > > So, what does prevent us from moving that tables to use pointers? I think that will lead to ABI breakage(client->name vs id->name) match = device_get_match_data(&client->dev); if (match) { chip_type = (uintptr_t)match; name = client->name; } else if (id) { chip_type = (enum inv_devices) id->driver_data; name = id->name; } else { return -ENOSYS; } > > > /*device enum */ > > enum inv_devices { > > INV_MPU6050, > > INV_MPU6500, > > INV_MPU6515, > > INV_MPU6880, > > INV_MPU6000, > > INV_MPU9150, > > INV_MPU9250, > > INV_MPU9255, > > INV_ICM20608, > > INV_ICM20608D, > > INV_ICM20609, > > INV_ICM20689, > > INV_ICM20600, > > INV_ICM20602, > > INV_ICM20690, > > INV_IAM20680, > > INV_NUM_PARTS > > }; > > > > The new helper function > > You mean for debugging? We don't need that in production. That is sample code for iterating through id table to find max enum and check against ZERO_OR_NULL_PTR > > I think what you need is a coccinelle script to find these. I need to explore using coccinelle script as I have n't tried before. Cheers, Biju
Hi Andy, On Mon, Aug 7, 2023 at 4:54 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote: > > On Sat, 5 Aug 2023 17:42:21 +0000 > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > On Fri, 4 Aug 2023 17:17:24 +0100 > > > > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > ... > > > > + * Besides the fact that some drivers abuse the device ID driver_data type > > > + * and claim it to be integer, for the bus specific ID tables the driver_data > > > + * may be defined as kernel_ulong_t. For these tables 0 is a valid response, > > > + * but not for this function. It's recommended to convert those either to avoid > > > + * 0 or use a real pointer to the predefined driver data. > > > We still need to maintain consistency across the two tables, which > > is a stronger requirement than avoiding 0. > > True. Any suggestion how to amend the above comment? Because the documentation > makes sense on its own (may be split from the series?). I do have an idea how to handle that (inspired by the macro-heavy R-Car pin control drivers ;-) #define DEVICES(fn) \ fn("vendor1", "device1", &driver_data1) \ fn("vendor3", "device2", &driver_data2) \ fn("vendor3", "device2", &driver_data3) OF_MATCH_TABLE(driver_of_match, DEVICES); I2C_MATCH_TABLE(driver_i2c_ids, DEVICES); and in the of resp. i2c headers: #define EMIT_OF_ENTRY(_vendor, _device, _data) \ { .name = _vendor ## "," ## _device, .data = _data) }, #define EMIT_OF_ENTRIES(_name, _devs) \ static const struct of_device_id _name[] = { \ _devs(EMIT_OF_ENTRY) \ { } \ } \ #define EMIT_I2C_ENTRY(_vendor, _device, _data) \ { .name = _device, .driver_data = (void *)_data) }, #define EMIT_I2C_ENTRIES(_name, _devs) \ static const struct i2c_device_id _name[] = { \ _devs(EMIT_I2C_ENTRY) \ { } \ } \ I didn't try to compile this, so I may have missed something (e.g. a required intermediate macro to ensure proper expansion). Unfortunately this would break grep'ability for compatible values... Gr{oetje,eeting}s, Geert
Hi Biju, On Fri, Aug 11, 2023 at 4:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > > bus_type > > > > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote: > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote: > > > > ... > > > > > > I'm good with this approach, but make sure you checked the whole > > > > kernel source tree for a such. > > > > > > Checking against 16 is too short I guess?? > > > > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums. > > > > So, what does prevent us from moving that tables to use pointers? > > I think that will lead to ABI breakage(client->name vs id->name) > > match = device_get_match_data(&client->dev); > if (match) { > chip_type = (uintptr_t)match; > name = client->name; > } else if (id) { > chip_type = (enum inv_devices) > id->driver_data; > name = id->name; > } else { > return -ENOSYS; > } I don't consider that part of the ABI, as e.g. converting from board files to DT would change the name. In addition, using id->name breaks multiple instances of the same device. I applaud more unification ;-) Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > bus_type > > Hi Biju, > > On Fri, Aug 11, 2023 at 4:46 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > > > bus_type > > > > > > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote: > > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote: > > > > > > ... > > > > > > > > I'm good with this approach, but make sure you checked the whole > > > > > kernel source tree for a such. > > > > > > > > Checking against 16 is too short I guess?? > > > > > > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums. > > > > > > So, what does prevent us from moving that tables to use pointers? > > > > I think that will lead to ABI breakage(client->name vs id->name) > > > > match = device_get_match_data(&client->dev); > > if (match) { > > chip_type = (uintptr_t)match; > > name = client->name; > > } else if (id) { > > chip_type = (enum inv_devices) > > id->driver_data; > > name = id->name; > > } else { > > return -ENOSYS; > > } > > I don't consider that part of the ABI, as e.g. converting from board files > to DT would change the name. > In addition, using id->name breaks multiple instances of the same device. OK, then according to you this patch is ok [1]? https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230807172548.258247-2-biju.das.jz@bp.renesas.com/ Cheers, Biju
On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote: > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > > bus_type > > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote: > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote: ... > > > > I'm good with this approach, but make sure you checked the whole > > > > kernel source tree for a such. > > > > > > Checking against 16 is too short I guess?? > > > > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums. > > > > So, what does prevent us from moving that tables to use pointers? > > I think that will lead to ABI breakage(client->name vs id->name) > > match = device_get_match_data(&client->dev); > if (match) { > chip_type = (uintptr_t)match; > name = client->name; > } else if (id) { > chip_type = (enum inv_devices) > id->driver_data; > name = id->name; > } else { > return -ENOSYS; > } It's easy to work around (may be better fix can be found, haven't checked, just what first comes to my mind): match ... name = match->name; /* If enumerated via firmware node, fix the ABI */ if (dev_fwnode()) client->name > > > /*device enum */ > > > enum inv_devices { > > > INV_MPU6050, > > > INV_MPU6500, > > > INV_MPU6515, > > > INV_MPU6880, > > > INV_MPU6000, > > > INV_MPU9150, > > > INV_MPU9250, > > > INV_MPU9255, > > > INV_ICM20608, > > > INV_ICM20608D, > > > INV_ICM20609, > > > INV_ICM20689, > > > INV_ICM20600, > > > INV_ICM20602, > > > INV_ICM20690, > > > INV_IAM20680, > > > INV_NUM_PARTS > > > }; > > > > > > The new helper function > > > > You mean for debugging? We don't need that in production. > > That is sample code for iterating through id table to find max enum > and check against ZERO_OR_NULL_PTR Much better with a coccinelle. You will find all or almost all occurrences without too much effort done. > > I think what you need is a coccinelle script to find these. > > I need to explore using coccinelle script as I have n't tried before.
Hi Andy Shevchenko, Thanks for the feedback. > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > bus_type > > On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote: > > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > > > bus_type On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote: > > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote: > > ... > > > > > > I'm good with this approach, but make sure you checked the whole > > > > > kernel source tree for a such. > > > > > > > > Checking against 16 is too short I guess?? > > > > > > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums. > > > > > > So, what does prevent us from moving that tables to use pointers? > > > > I think that will lead to ABI breakage(client->name vs id->name) > > > > match = device_get_match_data(&client->dev); > > if (match) { > > chip_type = (uintptr_t)match; > > name = client->name; > > } else if (id) { > > chip_type = (enum inv_devices) > > id->driver_data; > > name = id->name; > > } else { > > return -ENOSYS; > > } > > > It's easy to work around (may be better fix can be found, haven't checked, > just what first comes to my mind): > > match ... > name = match->name; The device_get_match_data()API returns matched data, so we cannot use that one here. Maybe?? /* If enumerated via ID lookup, fix the ABI */ if (!dev_fwnode()) name = id->name; Cheers, Biju > > /* If enumerated via firmware node, fix the ABI */ > if (dev_fwnode()) > client->name > > > > > /*device enum */ > > > > enum inv_devices { > > > > INV_MPU6050, > > > > INV_MPU6500, > > > > INV_MPU6515, > > > > INV_MPU6880, > > > > INV_MPU6000, > > > > INV_MPU9150, > > > > INV_MPU9250, > > > > INV_MPU9255, > > > > INV_ICM20608, > > > > INV_ICM20608D, > > > > INV_ICM20609, > > > > INV_ICM20689, > > > > INV_ICM20600, > > > > INV_ICM20602, > > > > INV_ICM20690, > > > > INV_IAM20680, > > > > INV_NUM_PARTS > > > > }; > > > > > > > > The new helper function > > > > > > You mean for debugging? We don't need that in production. > > > > That is sample code for iterating through id table to find max enum > > and check against ZERO_OR_NULL_PTR > > Much better with a coccinelle. You will find all or almost all occurrences > without too much effort done. > > > > I think what you need is a coccinelle script to find these. > > > > I need to explore using coccinelle script as I have n't tried before. > > -- > With Best Regards, > Andy Shevchenko >
> Subject: RE: [PATCH v7 0/4] Extend device_get_match_data() to struct > bus_type > > > Hi Andy Shevchenko, > > Thanks for the feedback. > > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > > bus_type > > > > On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote: > > > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to > > > > struct bus_type On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das > wrote: > > > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote: > > > > ... > > > > > > > > I'm good with this approach, but make sure you checked the > > > > > > whole kernel source tree for a such. > > > > > > > > > > Checking against 16 is too short I guess?? > > > > > > > > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums. > > > > > > > > So, what does prevent us from moving that tables to use pointers? > > > > > > I think that will lead to ABI breakage(client->name vs id->name) > > > > > > match = device_get_match_data(&client->dev); > > > if (match) { > > > chip_type = (uintptr_t)match; > > > name = client->name; > > > } else if (id) { > > > chip_type = (enum inv_devices) > > > id->driver_data; > > > name = id->name; > > > } else { > > > return -ENOSYS; > > > } > > > > > > It's easy to work around (may be better fix can be found, haven't > > checked, just what first comes to my mind): > > > > match ... > > name = match->name; > > The device_get_match_data()API returns matched data, so we cannot use that > one here. > > Maybe?? > > /* If enumerated via ID lookup, fix the ABI */ if (!dev_fwnode()) > name = id->name; Looks this will work. /* If enumerated via firmware node, fix the ABI */ if (dev_fwnode()) name = client->name else name = id->name Cheers, Biju
On Tue, Aug 15, 2023 at 06:58:41AM +0000, Biju Das wrote: > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct > > bus_type > > On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote: ... > > It's easy to work around (may be better fix can be found, haven't checked, > > just what first comes to my mind): > > > > match ... > > name = match->name; > > The device_get_match_data()API returns matched data, so we cannot use that one here. > > Maybe?? > > /* If enumerated via ID lookup, fix the ABI */ > if (!dev_fwnode()) > name = id->name; Yeah, you got the idea.