| Message ID | 20230820184402.102486-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | onvert enum->pointer for data in the rt1711h match tables | expand |
On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote: > Convert enum->pointer for data in the match tables, so that > device_get_match_data() can do match against OF/ACPI/I2C tables, once i2c > bus type match support added to it and it returns NULL for non-match. > > Therefore it is better to convert enum->pointer for data match and extend > match support for both ID and OF tables by using i2c_get_match_data() by > adding struct rt1711h_chip_info with did variable and replacing did->info > in struct rt1711h_chip. Later patches will add more hw differences to > struct rt1711h_chip_info and avoid checking did for HW differences. ... > +struct rt1711h_chip_info { > + u16 did; > +}; > + > struct rt1711h_chip { > struct tcpci_data data; > struct tcpci *tcpci; > struct device *dev; > struct regulator *vbus; > bool src_en; > - u16 did; > + const struct rt1711h_chip_info *info; Have you run pahole? I believe now you wasting a few more bytes (besides the pointer requirement) due to (mis)placing a new member. > }; ... 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); 2) preservation of the const qualifier (despite kernel_ulong_t being used in the middle). With that added I believe you can sell this much more easier.
Hi Andy, 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: > > Convert enum->pointer for data in the match tables, so that > > device_get_match_data() can do match against OF/ACPI/I2C tables, once i2c > > bus type match support added to it and it returns NULL for non-match. > > > > Therefore it is better to convert enum->pointer for data match and extend > > match support for both ID and OF tables by using i2c_get_match_data() by > > adding struct rt1711h_chip_info with did variable and replacing did->info > > in struct rt1711h_chip. Later patches will add more hw differences to > > struct rt1711h_chip_info and avoid checking did for HW differences. > > ... > > > +struct rt1711h_chip_info { > > + u16 did; > > +}; > > + > > struct rt1711h_chip { > > struct tcpci_data data; > > struct tcpci *tcpci; > > struct device *dev; > > struct regulator *vbus; > > bool src_en; > > - u16 did; > > + const struct rt1711h_chip_info *info; > > Have you run pahole? I believe now you wasting a few more bytes > (besides the pointer requirement) due to (mis)placing a new member. Unfortunately you cannot really improve by reordering the members. The old u16 fit in the hole after sr_en. The new pointer info cannot fit in a hole anyway. > 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. Gr{oetje,eeting}s, Geert
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.
Hi Andy, 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. Gr{oetje,eeting}s, Geert
On Mon, Aug 21, 2023 at 05:40:05PM +0200, Geert Uytterhoeven wrote: > Hi Andy, > > 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. > Exactly. Guenter
Hi Andy, Thanks for the feedback. > Subject: Re: [PATCH 2/4] usb: typec: tcpci_rt1711h: Convert enum->pointer > for data in the match tables > > On Sun, Aug 20, 2023 at 07:44:00PM +0100, Biju Das wrote: > > Convert enum->pointer for data in the match tables, so that > > device_get_match_data() can do match against OF/ACPI/I2C tables, once > > i2c bus type match support added to it and it returns NULL for non-match. > > > > Therefore it is better to convert enum->pointer for data match and > > extend match support for both ID and OF tables by using > > i2c_get_match_data() by adding struct rt1711h_chip_info with did > > variable and replacing did->info in struct rt1711h_chip. Later patches > > will add more hw differences to struct rt1711h_chip_info and avoid > checking did for HW differences. > > ... > > > +struct rt1711h_chip_info { > > + u16 did; > > +}; > > + > > struct rt1711h_chip { > > struct tcpci_data data; > > struct tcpci *tcpci; > > struct device *dev; > > struct regulator *vbus; > > bool src_en; > > - u16 did; > > + const struct rt1711h_chip_info *info; > > Have you run pahole? I believe now you wasting a few more bytes (besides > the pointer requirement) due to (mis)placing a new member. > > > }; Just tried pahole for the first time. $ pahole -C rt1711h_chip drivers/usb/typec/tcpm/tcpci_rt1711h.o struct rt1711h_chip { struct tcpci_data data; /* 0 72 */ /* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */ struct tcpci * tcpci; /* 72 8 */ struct device * dev; /* 80 8 */ struct regulator * vbus; /* 88 8 */ bool src_en; /* 96 1 */ /* XXX 7 bytes hole, try to pack */ const struct rt1711h_chip_info * info; /* 104 8 */ /* size: 112, cachelines: 2, members: 6 */ /* sum members: 105, holes: 1, sum holes: 7 */ /* last cacheline: 48 bytes */ }; biju@biju-VirtualBox:~/linux-next-test$ pahole -C rt1711h_chip_info drivers/usb/typec/tcpm/tcpci_rt1711h.o struct rt1711h_chip_info { u16 did; /* 0 2 */ /* XXX 2 bytes hole, try to pack */ u32 rxdz_sel; /* 4 4 */ unsigned int enable_pd30_extended_message:1; /* 8: 0 4 */ /* size: 12, cachelines: 1, members: 3 */ /* sum members: 6, holes: 1, sum holes: 2 */ /* sum bitfield members: 1 bits (0 bytes) */ /* bit_padding: 31 bits */ /* last cacheline: 12 bytes */ }; Currently size is 12 bytes, it can be reduced to 8 by adding bool. biju@biju-VirtualBox:~/linux-next-test$ pahole -C rt1711h_chip_info drivers/usb/typec/tcpm/tcpci_rt1711h.o struct rt1711h_chip_info { u32 rxdz_sel; /* 0 4 */ u16 did; /* 4 2 */ bool enable_pd30_extended_message; /* 6 1 */ /* size: 8, cachelines: 1, members: 3 */ /* padding: 1 */ /* last cacheline: 8 bytes */ }; Cheers, Biju > ... > > 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); > 2) preservation of the const qualifier (despite kernel_ulong_t > being used in the middle). > > With that added I believe you can sell this much more easier. > > -- > With Best Regards, > Andy Shevchenko >
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. Since OF ID table structure is universal, it uses pointers. Maybe you need to update it to use plain integer instead? 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. 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.
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
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 :-)
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
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.
> 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
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.
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
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
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
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.
diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c index 1e13ce3c4b35..c9392919226a 100644 --- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c +++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c @@ -51,13 +51,17 @@ /* 1b0 as fixed rx threshold of rd/rp 0.55V, 1b1 depends on RTCRTL4[0] */ #define BMCIO_RXDZEN BIT(0) +struct rt1711h_chip_info { + u16 did; +}; + struct rt1711h_chip { struct tcpci_data data; struct tcpci *tcpci; struct device *dev; struct regulator *vbus; bool src_en; - u16 did; + const struct rt1711h_chip_info *info; }; static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val) @@ -105,7 +109,7 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata) return ret; /* Enable PD30 extended message for RT1715 */ - if (chip->did == RT1715_DID) { + if (chip->info->did == RT1715_DID) { ret = regmap_update_bits(regmap, RT1711H_RTCTRL8, RT1711H_ENEXTMSG, RT1711H_ENEXTMSG); if (ret < 0) @@ -200,7 +204,7 @@ static inline int rt1711h_init_cc_params(struct rt1711h_chip *chip, u8 status) if ((cc1 >= TYPEC_CC_RP_1_5 && cc2 < TYPEC_CC_RP_DEF) || (cc2 >= TYPEC_CC_RP_1_5 && cc1 < TYPEC_CC_RP_DEF)) { rxdz_en = BMCIO_RXDZEN; - if (chip->did == RT1715_DID) + if (chip->info->did == RT1715_DID) rxdz_sel = RT1711H_BMCIO_RXDZSEL; else rxdz_sel = 0; @@ -319,7 +323,7 @@ static int rt1711h_check_revision(struct i2c_client *i2c, struct rt1711h_chip *c ret = i2c_smbus_read_word_data(i2c, TCPC_BCD_DEV); if (ret < 0) return ret; - if (ret != chip->did) { + if (ret != chip->info->did) { dev_err(&i2c->dev, "did is not correct, 0x%04x\n", ret); return -ENODEV; } @@ -336,7 +340,7 @@ static int rt1711h_probe(struct i2c_client *client) if (!chip) return -ENOMEM; - chip->did = (size_t)device_get_match_data(&client->dev); + chip->info = i2c_get_match_data(client); ret = rt1711h_check_revision(client, chip); if (ret < 0) { @@ -391,17 +395,25 @@ static void rt1711h_remove(struct i2c_client *client) tcpci_unregister_port(chip->tcpci); } +static const struct rt1711h_chip_info rt1711h = { + .did = RT1711H_DID, +}; + +static const struct rt1711h_chip_info rt1715 = { + .did = RT1715_DID, +}; + static const struct i2c_device_id rt1711h_id[] = { - { "rt1711h", RT1711H_DID }, - { "rt1715", RT1715_DID }, + { "rt1711h", (kernel_ulong_t)&rt1711h }, + { "rt1715", (kernel_ulong_t)&rt1715 }, { } }; MODULE_DEVICE_TABLE(i2c, rt1711h_id); #ifdef CONFIG_OF static const struct of_device_id rt1711h_of_match[] = { - { .compatible = "richtek,rt1711h", .data = (void *)RT1711H_DID }, - { .compatible = "richtek,rt1715", .data = (void *)RT1715_DID }, + { .compatible = "richtek,rt1711h", .data = &rt1711h }, + { .compatible = "richtek,rt1715", .data = &rt1715 }, {}, }; MODULE_DEVICE_TABLE(of, rt1711h_of_match);
Convert enum->pointer for data in the match tables, so that device_get_match_data() can do match against OF/ACPI/I2C tables, once i2c bus type match support added to it and it returns NULL for non-match. Therefore it is better to convert enum->pointer for data match and extend match support for both ID and OF tables by using i2c_get_match_data() by adding struct rt1711h_chip_info with did variable and replacing did->info in struct rt1711h_chip. Later patches will add more hw differences to struct rt1711h_chip_info and avoid checking did for HW differences. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/usb/typec/tcpm/tcpci_rt1711h.c | 30 ++++++++++++++++++-------- 1 file changed, 21 insertions(+), 9 deletions(-)