Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

[Geert Uytterhoeven]

Hi Andy,

CC DT

On Mon, Aug 21, 2023 at 7:09 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Aug 21, 2023 at 05:40:05PM +0200, Geert Uytterhoeven wrote:
> > On Mon, Aug 21, 2023 at 5:25 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Aug 21, 2023 at 03:27:43PM +0200, Geert Uytterhoeven wrote:
> > > > On Mon, Aug 21, 2023 at 3:04 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote:
>
> ...
>
> > > > > For all your work likes this as I noted in the reply to Guenter that
> > > > > the couple of the selling points here are:
> > > > > 1) avoidance of the pointer abuse in OF table
> > > > >    (we need that to be a valid pointer);
> > > >
> > > > There is no pointer abuse: both const void * (in e.g. of_device_id)
> > > > and kernel_ulong_t (in e.g. i2c_device_id) can be used by drivers
> > > > to store a magic cookie, being either a pointer, or an integer value.
> > > > The same is true for the various unsigned long and void * "driver_data"
> > > > fields in subsystem-specific driver structures.
> > >
> > > (void *)5 is the abuse of the pointer.
> > > We carry something which is not a valid pointer from kernel perspective.
> >
> > But the data field is not required to be a valid pointer.
> > What kind and type of information it represents is specific to the driver.
>
> Where to find necessary information which is not always an integer constant.
> For example, for the driver data that has callbacks it can't be invalid pointer.

If the driver uses it to store callbacks, of course it needs to be a
valid pointer. But that is internal to the driver.  It is not that
we're passing random integer values to a function that expects a
pointer that can actually be dereferenced.

> Since OF ID table structure is universal, it uses pointers. Maybe you need to
> update it to use plain integer instead?

It is fairly common in the kernel to use void * to indicate a
driver-specific cookie, being either a real pointer or an integral
value, that is passed verbatim.  See also e.g. the "dev" parameter
of request_irq().

> I think there is no more sense to continue this. We have to admit we have
> a good disagreement on this and I do not see any way I can agree with your
> arguments. Note, I'm fine if you "fix" OF ID structure to use kernel_ulong_t.

of_device_id is also used in userspace (e.g. modutils), but I believe
that uses a copy of the structure definition, not the definition from
the kernel headers. Still, changing the type would be a lot of work,
for IMHO no real gain.

> The only objection there is that it may not carry on the const qualifier,
> which I personally find being a huge downside of the whole driver_data.
> I believe you haven't objected that.

Having const is nice, indeed.

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

Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

[Andy Shevchenko]

On Tue, Aug 22, 2023 at 09:21:19AM +0200, Geert Uytterhoeven wrote:
> On Mon, Aug 21, 2023 at 7:09 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Aug 21, 2023 at 05:40:05PM +0200, Geert Uytterhoeven wrote:
> > > On Mon, Aug 21, 2023 at 5:25 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Aug 21, 2023 at 03:27:43PM +0200, Geert Uytterhoeven wrote:
> > > > > On Mon, Aug 21, 2023 at 3:04 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote:

...

> > > > > > For all your work likes this as I noted in the reply to Guenter that
> > > > > > the couple of the selling points here are:
> > > > > > 1) avoidance of the pointer abuse in OF table
> > > > > >    (we need that to be a valid pointer);
> > > > >
> > > > > There is no pointer abuse: both const void * (in e.g. of_device_id)
> > > > > and kernel_ulong_t (in e.g. i2c_device_id) can be used by drivers
> > > > > to store a magic cookie, being either a pointer, or an integer value.
> > > > > The same is true for the various unsigned long and void * "driver_data"
> > > > > fields in subsystem-specific driver structures.
> > > >
> > > > (void *)5 is the abuse of the pointer.
> > > > We carry something which is not a valid pointer from kernel perspective.
> > >
> > > But the data field is not required to be a valid pointer.
> > > What kind and type of information it represents is specific to the driver.
> >
> > Where to find necessary information which is not always an integer constant.
> > For example, for the driver data that has callbacks it can't be invalid pointer.
> 
> If the driver uses it to store callbacks, of course it needs to be a
> valid pointer. But that is internal to the driver.  It is not that
> we're passing random integer values to a function that expects a
> pointer that can actually be dereferenced.
> 
> > Since OF ID table structure is universal, it uses pointers. Maybe you need to
> > update it to use plain integer instead?
> 
> It is fairly common in the kernel to use void * to indicate a
> driver-specific cookie, being either a real pointer or an integral
> value, that is passed verbatim.  See also e.g. the "dev" parameter
> of request_irq().

Yes, that parameter is void * due to calling kfree(free_irq(...)).
So, that's argument for my concerns.

> > I think there is no more sense to continue this. We have to admit we have
> > a good disagreement on this and I do not see any way I can agree with your
> > arguments. Note, I'm fine if you "fix" OF ID structure to use kernel_ulong_t.
> 
> of_device_id is also used in userspace (e.g. modutils), but I believe
> that uses a copy of the structure definition, not the definition from
> the kernel headers.

Nope, it uses the very same mod_devicetable.h in both.

> Still, changing the type would be a lot of work,
> for IMHO no real gain.

So, stale mate here, then?

> > The only objection there is that it may not carry on the const qualifier,
> > which I personally find being a huge downside of the whole driver_data.
> > I believe you haven't objected that.
> 
> Having const is nice, indeed.

At least something we have agreed on :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

[Geert Uytterhoeven]

Hi Andy,

On Tue, Aug 22, 2023 at 1:44 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Aug 22, 2023 at 09:21:19AM +0200, Geert Uytterhoeven wrote:
> > On Mon, Aug 21, 2023 at 7:09 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Aug 21, 2023 at 05:40:05PM +0200, Geert Uytterhoeven wrote:
> > > > On Mon, Aug 21, 2023 at 5:25 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Mon, Aug 21, 2023 at 03:27:43PM +0200, Geert Uytterhoeven wrote:
> > > > > > On Mon, Aug 21, 2023 at 3:04 PM Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote:
>
> ...
>
> > > > > > > For all your work likes this as I noted in the reply to Guenter that
> > > > > > > the couple of the selling points here are:
> > > > > > > 1) avoidance of the pointer abuse in OF table
> > > > > > >    (we need that to be a valid pointer);
> > > > > >
> > > > > > There is no pointer abuse: both const void * (in e.g. of_device_id)
> > > > > > and kernel_ulong_t (in e.g. i2c_device_id) can be used by drivers
> > > > > > to store a magic cookie, being either a pointer, or an integer value.
> > > > > > The same is true for the various unsigned long and void * "driver_data"
> > > > > > fields in subsystem-specific driver structures.
> > > > >
> > > > > (void *)5 is the abuse of the pointer.
> > > > > We carry something which is not a valid pointer from kernel perspective.
> > > >
> > > > But the data field is not required to be a valid pointer.
> > > > What kind and type of information it represents is specific to the driver.
> > >
> > > Where to find necessary information which is not always an integer constant.
> > > For example, for the driver data that has callbacks it can't be invalid pointer.
> >
> > If the driver uses it to store callbacks, of course it needs to be a
> > valid pointer. But that is internal to the driver.  It is not that
> > we're passing random integer values to a function that expects a
> > pointer that can actually be dereferenced.
> >
> > > Since OF ID table structure is universal, it uses pointers. Maybe you need to
> > > update it to use plain integer instead?
> >
> > It is fairly common in the kernel to use void * to indicate a
> > driver-specific cookie, being either a real pointer or an integral
> > value, that is passed verbatim.  See also e.g. the "dev" parameter
> > of request_irq().
>
> Yes, that parameter is void * due to calling kfree(free_irq(...)).
> So, that's argument for my concerns.

Sorry, I don't understand this comment.
(kfree(free_irq(...)) is only called in pci_free_irq()?)

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

Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

[Andy Shevchenko]

On Tue, Aug 22, 2023 at 02:00:05PM +0200, Geert Uytterhoeven wrote:
> On Tue, Aug 22, 2023 at 1:44 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Aug 22, 2023 at 09:21:19AM +0200, Geert Uytterhoeven wrote:
> > > On Mon, Aug 21, 2023 at 7:09 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Aug 21, 2023 at 05:40:05PM +0200, Geert Uytterhoeven wrote:
> > > > > On Mon, Aug 21, 2023 at 5:25 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Mon, Aug 21, 2023 at 03:27:43PM +0200, Geert Uytterhoeven wrote:
> > > > > > > On Mon, Aug 21, 2023 at 3:04 PM Andy Shevchenko
> > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote:

...

> > > > > > > > For all your work likes this as I noted in the reply to Guenter that
> > > > > > > > the couple of the selling points here are:
> > > > > > > > 1) avoidance of the pointer abuse in OF table
> > > > > > > >    (we need that to be a valid pointer);
> > > > > > >
> > > > > > > There is no pointer abuse: both const void * (in e.g. of_device_id)
> > > > > > > and kernel_ulong_t (in e.g. i2c_device_id) can be used by drivers
> > > > > > > to store a magic cookie, being either a pointer, or an integer value.
> > > > > > > The same is true for the various unsigned long and void * "driver_data"
> > > > > > > fields in subsystem-specific driver structures.
> > > > > >
> > > > > > (void *)5 is the abuse of the pointer.
> > > > > > We carry something which is not a valid pointer from kernel perspective.
> > > > >
> > > > > But the data field is not required to be a valid pointer.
> > > > > What kind and type of information it represents is specific to the driver.
> > > >
> > > > Where to find necessary information which is not always an integer constant.
> > > > For example, for the driver data that has callbacks it can't be invalid pointer.
> > >
> > > If the driver uses it to store callbacks, of course it needs to be a
> > > valid pointer. But that is internal to the driver.  It is not that
> > > we're passing random integer values to a function that expects a
> > > pointer that can actually be dereferenced.
> > >
> > > > Since OF ID table structure is universal, it uses pointers. Maybe you need to
> > > > update it to use plain integer instead?
> > >
> > > It is fairly common in the kernel to use void * to indicate a
> > > driver-specific cookie, being either a real pointer or an integral
> > > value, that is passed verbatim.  See also e.g. the "dev" parameter
> > > of request_irq().
> >
> > Yes, that parameter is void * due to calling kfree(free_irq(...)).
> > So, that's argument for my concerns.
> 
> Sorry, I don't understand this comment.
> (kfree(free_irq(...)) is only called in pci_free_irq()?)

Passing void * for a "driver cookie" makes sense due to possibility of the
passing it to other functions that want to have void * as your example shows.
And that supports my idea of having void * over the unsigned long.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

[Biju Das]

> Subject: Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer
> for data in the match tables
> 
> On Tue, Aug 22, 2023 at 02:00:05PM +0200, Geert Uytterhoeven wrote:
> > On Tue, Aug 22, 2023 at 1:44 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Aug 22, 2023 at 09:21:19AM +0200, Geert Uytterhoeven wrote:
> > > > On Mon, Aug 21, 2023 at 7:09 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Mon, Aug 21, 2023 at 05:40:05PM +0200, Geert Uytterhoeven wrote:
> > > > > > On Mon, Aug 21, 2023 at 5:25 PM Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > On Mon, Aug 21, 2023 at 03:27:43PM +0200, Geert Uytterhoeven
> wrote:
> > > > > > > > On Mon, Aug 21, 2023 at 3:04 PM Andy Shevchenko
> > > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > > On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote:
> 
> ...
> 
> > > > > > > > > For all your work likes this as I noted in the reply to
> > > > > > > > > Guenter that the couple of the selling points here are:
> > > > > > > > > 1) avoidance of the pointer abuse in OF table
> > > > > > > > >    (we need that to be a valid pointer);
> > > > > > > >
> > > > > > > > There is no pointer abuse: both const void * (in e.g.
> > > > > > > > of_device_id) and kernel_ulong_t (in e.g. i2c_device_id)
> > > > > > > > can be used by drivers to store a magic cookie, being either
> a pointer, or an integer value.
> > > > > > > > The same is true for the various unsigned long and void *
> "driver_data"
> > > > > > > > fields in subsystem-specific driver structures.
> > > > > > >
> > > > > > > (void *)5 is the abuse of the pointer.
> > > > > > > We carry something which is not a valid pointer from kernel
> perspective.
> > > > > >
> > > > > > But the data field is not required to be a valid pointer.
> > > > > > What kind and type of information it represents is specific to
> the driver.
> > > > >
> > > > > Where to find necessary information which is not always an integer
> constant.
> > > > > For example, for the driver data that has callbacks it can't be
> invalid pointer.
> > > >
> > > > If the driver uses it to store callbacks, of course it needs to be
> > > > a valid pointer. But that is internal to the driver.  It is not
> > > > that we're passing random integer values to a function that
> > > > expects a pointer that can actually be dereferenced.
> > > >
> > > > > Since OF ID table structure is universal, it uses pointers.
> > > > > Maybe you need to update it to use plain integer instead?
> > > >
> > > > It is fairly common in the kernel to use void * to indicate a
> > > > driver-specific cookie, being either a real pointer or an integral
> > > > value, that is passed verbatim.  See also e.g. the "dev" parameter
> > > > of request_irq().
> > >
> > > Yes, that parameter is void * due to calling kfree(free_irq(...)).
> > > So, that's argument for my concerns.
> >
> > Sorry, I don't understand this comment.
> > (kfree(free_irq(...)) is only called in pci_free_irq()?)
> 
> Passing void * for a "driver cookie" makes sense due to possibility of the
> passing it to other functions that want to have void * as your example
> shows.
> And that supports my idea of having void * over the unsigned long.

U-boot also uses unsigned long for .data in struct udevice_id. There may be a reason for it instead of void* ??

Cheers,
Biju

Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

[Andy Shevchenko]

On Tue, Aug 22, 2023 at 12:51:04PM +0000, Biju Das wrote:
> > On Tue, Aug 22, 2023 at 02:00:05PM +0200, Geert Uytterhoeven wrote:
> > > On Tue, Aug 22, 2023 at 1:44 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, Aug 22, 2023 at 09:21:19AM +0200, Geert Uytterhoeven wrote:
> > > > > On Mon, Aug 21, 2023 at 7:09 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Mon, Aug 21, 2023 at 05:40:05PM +0200, Geert Uytterhoeven wrote:
> > > > > > > On Mon, Aug 21, 2023 at 5:25 PM Andy Shevchenko
> > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > On Mon, Aug 21, 2023 at 03:27:43PM +0200, Geert Uytterhoeven
> > wrote:
> > > > > > > > > On Mon, Aug 21, 2023 at 3:04 PM Andy Shevchenko
> > > > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > > > On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote:

...

> > > > > > > > > > For all your work likes this as I noted in the reply to
> > > > > > > > > > Guenter that the couple of the selling points here are:
> > > > > > > > > > 1) avoidance of the pointer abuse in OF table
> > > > > > > > > >    (we need that to be a valid pointer);
> > > > > > > > >
> > > > > > > > > There is no pointer abuse: both const void * (in e.g.
> > > > > > > > > of_device_id) and kernel_ulong_t (in e.g. i2c_device_id)
> > > > > > > > > can be used by drivers to store a magic cookie, being either
> > a pointer, or an integer value.
> > > > > > > > > The same is true for the various unsigned long and void *
> > "driver_data"
> > > > > > > > > fields in subsystem-specific driver structures.
> > > > > > > >
> > > > > > > > (void *)5 is the abuse of the pointer.
> > > > > > > > We carry something which is not a valid pointer from kernel
> > perspective.
> > > > > > >
> > > > > > > But the data field is not required to be a valid pointer.
> > > > > > > What kind and type of information it represents is specific to
> > the driver.
> > > > > >
> > > > > > Where to find necessary information which is not always an integer
> > constant.
> > > > > > For example, for the driver data that has callbacks it can't be
> > invalid pointer.
> > > > >
> > > > > If the driver uses it to store callbacks, of course it needs to be
> > > > > a valid pointer. But that is internal to the driver.  It is not
> > > > > that we're passing random integer values to a function that
> > > > > expects a pointer that can actually be dereferenced.
> > > > >
> > > > > > Since OF ID table structure is universal, it uses pointers.
> > > > > > Maybe you need to update it to use plain integer instead?
> > > > >
> > > > > It is fairly common in the kernel to use void * to indicate a
> > > > > driver-specific cookie, being either a real pointer or an integral
> > > > > value, that is passed verbatim.  See also e.g. the "dev" parameter
> > > > > of request_irq().
> > > >
> > > > Yes, that parameter is void * due to calling kfree(free_irq(...)).
> > > > So, that's argument for my concerns.
> > >
> > > Sorry, I don't understand this comment.
> > > (kfree(free_irq(...)) is only called in pci_free_irq()?)
> > 
> > Passing void * for a "driver cookie" makes sense due to possibility of the
> > passing it to other functions that want to have void * as your example
> > shows.
> > And that supports my idea of having void * over the unsigned long.
> 
> U-boot also uses unsigned long for .data in struct udevice_id. There may be a
> reason for it instead of void* ??

U-Boot to be honest not the best example of the code and I believe the reason
why is pure historical. unsigned long and void * are both architecture dependent,
so the one vs. another is only for the convenience here or there. The only thing
so far is to preserve the const qualifier, which you can not achieve with integer.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

[Andi Shyti]

Hi,

On Tue, Aug 22, 2023 at 03:08:38PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 22, 2023 at 02:00:05PM +0200, Geert Uytterhoeven wrote:
> > On Tue, Aug 22, 2023 at 1:44 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Aug 22, 2023 at 09:21:19AM +0200, Geert Uytterhoeven wrote:
> > > > On Mon, Aug 21, 2023 at 7:09 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Mon, Aug 21, 2023 at 05:40:05PM +0200, Geert Uytterhoeven wrote:
> > > > > > On Mon, Aug 21, 2023 at 5:25 PM Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > On Mon, Aug 21, 2023 at 03:27:43PM +0200, Geert Uytterhoeven wrote:
> > > > > > > > On Mon, Aug 21, 2023 at 3:04 PM Andy Shevchenko
> > > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > > On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote:
> 
> ...
> 
> > > > > > > > > For all your work likes this as I noted in the reply to Guenter that
> > > > > > > > > the couple of the selling points here are:
> > > > > > > > > 1) avoidance of the pointer abuse in OF table
> > > > > > > > >    (we need that to be a valid pointer);
> > > > > > > >
> > > > > > > > There is no pointer abuse: both const void * (in e.g. of_device_id)
> > > > > > > > and kernel_ulong_t (in e.g. i2c_device_id) can be used by drivers
> > > > > > > > to store a magic cookie, being either a pointer, or an integer value.
> > > > > > > > The same is true for the various unsigned long and void * "driver_data"
> > > > > > > > fields in subsystem-specific driver structures.
> > > > > > >
> > > > > > > (void *)5 is the abuse of the pointer.
> > > > > > > We carry something which is not a valid pointer from kernel perspective.
> > > > > >
> > > > > > But the data field is not required to be a valid pointer.
> > > > > > What kind and type of information it represents is specific to the driver.
> > > > >
> > > > > Where to find necessary information which is not always an integer constant.
> > > > > For example, for the driver data that has callbacks it can't be invalid pointer.
> > > >
> > > > If the driver uses it to store callbacks, of course it needs to be a
> > > > valid pointer. But that is internal to the driver.  It is not that
> > > > we're passing random integer values to a function that expects a
> > > > pointer that can actually be dereferenced.
> > > >
> > > > > Since OF ID table structure is universal, it uses pointers. Maybe you need to
> > > > > update it to use plain integer instead?
> > > >
> > > > It is fairly common in the kernel to use void * to indicate a
> > > > driver-specific cookie, being either a real pointer or an integral
> > > > value, that is passed verbatim.  See also e.g. the "dev" parameter
> > > > of request_irq().
> > >
> > > Yes, that parameter is void * due to calling kfree(free_irq(...)).
> > > So, that's argument for my concerns.
> > 
> > Sorry, I don't understand this comment.
> > (kfree(free_irq(...)) is only called in pci_free_irq()?)
> 
> Passing void * for a "driver cookie" makes sense due to possibility of the
> passing it to other functions that want to have void * as your example shows.
> And that supports my idea of having void * over the unsigned long.

I actually agree with Andy here... not much to add to his
arguments but if a void * is used as an integer then just change
the type.

I also was quite puzzled when I started seeing this flow of
patches.

I would rather prefer to store pointers in u64 variables rather
than integers in a pointer.

Andi

Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

[Geert Uytterhoeven]

Hi Andi,

On Wed, Aug 23, 2023 at 4:49 PM Andi Shyti <andi.shyti@kernel.org> wrote:
> I would rather prefer to store pointers in u64 variables rather
> than integers in a pointer.

"u64" is overkill, as it is too large on 32-bit platforms.
"uintptr_t" (or ("unsigned long") in legacy code) is the correct integer type.

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

Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

[Greg Kroah-Hartman]

On Wed, Aug 23, 2023 at 05:10:22PM +0200, Geert Uytterhoeven wrote:
> Hi Andi,
> 
> On Wed, Aug 23, 2023 at 4:49 PM Andi Shyti <andi.shyti@kernel.org> wrote:
> > I would rather prefer to store pointers in u64 variables rather
> > than integers in a pointer.
> 
> "u64" is overkill, as it is too large on 32-bit platforms.
> "uintptr_t" (or ("unsigned long") in legacy code) is the correct integer type.

"unsigned long" in kernel code is the guaranteed size of a pointer.

unitptr_t is for userspace code, it should not be used here in the
kernel as that's the wrong variable namespace.

thanks,

greg k-h

Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match tables

[Dmitry Torokhov]

On Wed, Aug 23, 2023 at 04:49:05PM +0200, Andi Shyti wrote:
> Hi,
> 
> On Tue, Aug 22, 2023 at 03:08:38PM +0300, Andy Shevchenko wrote:
> > On Tue, Aug 22, 2023 at 02:00:05PM +0200, Geert Uytterhoeven wrote:
> > > On Tue, Aug 22, 2023 at 1:44 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, Aug 22, 2023 at 09:21:19AM +0200, Geert Uytterhoeven wrote:
> > > > > On Mon, Aug 21, 2023 at 7:09 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Mon, Aug 21, 2023 at 05:40:05PM +0200, Geert Uytterhoeven wrote:
> > > > > > > On Mon, Aug 21, 2023 at 5:25 PM Andy Shevchenko
> > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > On Mon, Aug 21, 2023 at 03:27:43PM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > On Mon, Aug 21, 2023 at 3:04 PM Andy Shevchenko
> > > > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > > > On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote:
> > 
> > ...
> > 
> > > > > > > > > > For all your work likes this as I noted in the reply to Guenter that
> > > > > > > > > > the couple of the selling points here are:
> > > > > > > > > > 1) avoidance of the pointer abuse in OF table
> > > > > > > > > >    (we need that to be a valid pointer);
> > > > > > > > >
> > > > > > > > > There is no pointer abuse: both const void * (in e.g. of_device_id)
> > > > > > > > > and kernel_ulong_t (in e.g. i2c_device_id) can be used by drivers
> > > > > > > > > to store a magic cookie, being either a pointer, or an integer value.
> > > > > > > > > The same is true for the various unsigned long and void * "driver_data"
> > > > > > > > > fields in subsystem-specific driver structures.
> > > > > > > >
> > > > > > > > (void *)5 is the abuse of the pointer.
> > > > > > > > We carry something which is not a valid pointer from kernel perspective.
> > > > > > >
> > > > > > > But the data field is not required to be a valid pointer.
> > > > > > > What kind and type of information it represents is specific to the driver.
> > > > > >
> > > > > > Where to find necessary information which is not always an integer constant.
> > > > > > For example, for the driver data that has callbacks it can't be invalid pointer.
> > > > >
> > > > > If the driver uses it to store callbacks, of course it needs to be a
> > > > > valid pointer. But that is internal to the driver.  It is not that
> > > > > we're passing random integer values to a function that expects a
> > > > > pointer that can actually be dereferenced.
> > > > >
> > > > > > Since OF ID table structure is universal, it uses pointers. Maybe you need to
> > > > > > update it to use plain integer instead?
> > > > >
> > > > > It is fairly common in the kernel to use void * to indicate a
> > > > > driver-specific cookie, being either a real pointer or an integral
> > > > > value, that is passed verbatim.  See also e.g. the "dev" parameter
> > > > > of request_irq().
> > > >
> > > > Yes, that parameter is void * due to calling kfree(free_irq(...)).
> > > > So, that's argument for my concerns.
> > > 
> > > Sorry, I don't understand this comment.
> > > (kfree(free_irq(...)) is only called in pci_free_irq()?)
> > 
> > Passing void * for a "driver cookie" makes sense due to possibility of the
> > passing it to other functions that want to have void * as your example shows.
> > And that supports my idea of having void * over the unsigned long.
> 
> I actually agree with Andy here... not much to add to his
> arguments but if a void * is used as an integer then just change
> the type.
> 
> I also was quite puzzled when I started seeing this flow of
> patches.
> 
> I would rather prefer to store pointers in u64 variables rather
> than integers in a pointer.

I think pointers should be stored in pointers, and preserve constness.
The reason that many legacy device id structures use kernel_ulong_t to
store driver "cookie" is shortsightedness on our part long time ago.
Back then we just needed "a simple piece of data" to be attached, a
true/false flag or maybe a combination of flags. Nowadays we often
want to attach a structure describing a chip's parameters there with
several flags, maybe some methods, etc. So OF/DT folks did the right
thing, and used const void *, and we should try and convert the rest to
use const void * as well.

I posted RFC for this for I2C here:

https://lore.kernel.org/all/20230814-i2c-id-rework-v1-0-3e5bc71c49ee@gmail.com/

And then we can unify OF and legacy handling if driver tables.

Thanks.

-- 
Dmitry