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.

-- 
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 

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.

-- 
With Best Regards,
Andy Shevchenko

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
> 

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.

-- 
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

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

On Mon, 14 Aug 2023 15:12:24 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> 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.

This has always been a mess as I wasn't paying attention a long time back
and we ended up with some client->name entries being used for iio_dev->name
whereas it should be the part number.

Using id->name is correct choice.  This is supposed to be the same for multiple
instances of the same device.  There is label and a bunch of other options
for differentiating them including their parent devices.

Problem is that is exported to userspace and often used as part of the
matching when a userspace tool is trying to find the device.

We could 'give it a go' by setting the name in teh switch statement in the core code
and hope no one notices but it's not ideal
https://elixir.bootlin.com/linux/latest/source/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c#L1597

Jonathan



> 
> 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