Hi all,
> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
>
> On Tue, 8 Aug 2023 15:18:52 +0300
> Andy Shevchenko <[email protected]> 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
> > > > > <[email protected]> wrote:
> > > > > > > On Fri, 4 Aug 2023 17:17:24 +0100 Biju Das
> > > > > > > <[email protected]> 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.
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.
Cheers,
Biju
On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> > On Tue, 8 Aug 2023 15:18:52 +0300
> > Andy Shevchenko <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
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 <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
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 Biju,
On Fri, Aug 11, 2023 at 4:46 PM Biju Das <[email protected]> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
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 <[email protected]>
> 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/[email protected]/
Cheers,
Biju