[PATCH 0/4] onvert enum->pointer for data in the rt1711h match tables

[PATCH 0/4] onvert enum->pointer for data in the rt1711h match tables

[Biju Das]

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. see [1]

This patch series extend support for retrieving match data for ID lookup.
The first patch fixes the driver_data for ID table and second patch
convert enum->pointer to avoid non zero values as the proposed
solution returns NULL for non-match. The third and fourth patches
replaces comparison of did against hardware differences with data
and feature bit.

This patch series is only compile tested.

[1] https://lore.kernel.org/all/20230804161728.394920-1-biju.das.jz@bp.renesas.com/

Biju Das (4):
  usb: typec: tcpci_rt1711h: Make similar OF and ID table
  usb: typec: tcpci_rt1711h: Convert enum->pointer for data in the match
    tables
  usb: typec: tcpci_rt1711h: Add rxdz_sel variable to struct
    rt1711h_chip_info
  usb: typec: tcpci_rt1711h: Add enable_pd30_extended_message feature
    bit to struct rt1711h_chip_info

 drivers/usb/typec/tcpm/tcpci_rt1711h.c | 38 ++++++++++++++++++--------
 1 file changed, 26 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH 1/4] usb: typec: tcpci_rt1711h: Make similar OF and ID table

[Biju Das]

Make similar OF and ID table to extend support for ID match
using i2c_match_data()/device_get_match_data() later. Currently
it works only for OF match tables as the driver_data is wrong for
ID match.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/usb/typec/tcpm/tcpci_rt1711h.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
index 17ebc5fb684f..1e13ce3c4b35 100644
--- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
+++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
@@ -392,8 +392,8 @@ static void rt1711h_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id rt1711h_id[] = {
-	{ "rt1711h", 0 },
-	{ "rt1715", 0 },
+	{ "rt1711h", RT1711H_DID },
+	{ "rt1715", RT1715_DID },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rt1711h_id);
-- 
2.25.1

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

[Biju Das]

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

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);
-- 
2.25.1

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

[Andy Shevchenko]

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.

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

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

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

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

[Guenter Roeck]

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

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

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

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

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

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

[Biju Das]

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
> 

[PATCH 3/4] usb: typec: tcpci_rt1711h: Add rxdz_sel variable to struct rt1711h_chip_info

[Biju Das]

The RT1715 needs 0.35V/0.75V rx threshold for rd/rp whereas it is 0.4V/0.7V
for RT1711H. Add rxdz_sel variable to struct rt1711h_chip_info for
handling this difference.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/usb/typec/tcpm/tcpci_rt1711h.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
index c9392919226a..1b1753895ca5 100644
--- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
+++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
@@ -53,6 +53,7 @@
 
 struct rt1711h_chip_info {
 	u16 did;
+	u32 rxdz_sel;
 };
 
 struct rt1711h_chip {
@@ -204,10 +205,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->info->did == RT1715_DID)
-			rxdz_sel = RT1711H_BMCIO_RXDZSEL;
-		else
-			rxdz_sel = 0;
+		rxdz_sel = chip->info->rxdz_sel;
 	} else {
 		rxdz_en = 0;
 		rxdz_sel = RT1711H_BMCIO_RXDZSEL;
@@ -397,10 +395,12 @@ static void rt1711h_remove(struct i2c_client *client)
 
 static const struct rt1711h_chip_info rt1711h = {
 	.did = RT1711H_DID,
+	.rxdz_sel = 0,
 };
 
 static const struct rt1711h_chip_info rt1715 = {
 	.did = RT1715_DID,
+	.rxdz_sel = RT1711H_BMCIO_RXDZSEL,
 };
 
 static const struct i2c_device_id rt1711h_id[] = {
-- 
2.25.1

Re: [PATCH 3/4] usb: typec: tcpci_rt1711h: Add rxdz_sel variable to struct rt1711h_chip_info

[Andy Shevchenko]

On Sun, Aug 20, 2023 at 07:44:01PM +0100, Biju Das wrote:
> The RT1715 needs 0.35V/0.75V rx threshold for rd/rp whereas it is 0.4V/0.7V
> for RT1711H. Add rxdz_sel variable to struct rt1711h_chip_info for
> handling this difference.

...

>  struct rt1711h_chip_info {
>  	u16 did;
> +	u32 rxdz_sel;
>  };

Again, run pahole. And see the difference, if any, depending on the place of a
new member. Note, some 64-bit architectures may require 8-byte alignment even
for 4-byte members.

...

>  static const struct rt1711h_chip_info rt1711h = {
>  	.did = RT1711H_DID,
> +	.rxdz_sel = 0,
>  };

Unneeded change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] usb: typec: tcpci_rt1711h: Add rxdz_sel variable to struct rt1711h_chip_info

[Geert Uytterhoeven]

Hi Andy,

On Mon, Aug 21, 2023 at 3:06 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Sun, Aug 20, 2023 at 07:44:01PM +0100, Biju Das wrote:
> > The RT1715 needs 0.35V/0.75V rx threshold for rd/rp whereas it is 0.4V/0.7V
> > for RT1711H. Add rxdz_sel variable to struct rt1711h_chip_info for
> > handling this difference.
>
> ...
>
> >  struct rt1711h_chip_info {
> >       u16 did;
> > +     u32 rxdz_sel;
> >  };
>
> Again, run pahole. And see the difference, if any, depending on the place of a
> new member. Note, some 64-bit architectures may require 8-byte alignment even
> for 4-byte members.

Doesn't make a difference, the size and alignment of a structure are
always multiples of the largest alignment of each of the members, so
the structure size will be 8 bytes on both 32-bit and 64-bit (except
on m68k, where it will be 6 bytes).

Either you have 2 bytes did, 2 bytes hole, and 4 bytes rxdz_sel, or
4 bytes rxdz, 2 bytes did, and 2 bytes hole (except on m68k, where
there won't be any holes).

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 3/4] usb: typec: tcpci_rt1711h: Add rxdz_sel variable to struct rt1711h_chip_info

[Andy Shevchenko]

On Mon, Aug 21, 2023 at 03:33:29PM +0200, Geert Uytterhoeven wrote:
> On Mon, Aug 21, 2023 at 3:06 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Sun, Aug 20, 2023 at 07:44:01PM +0100, Biju Das wrote:
> > > The RT1715 needs 0.35V/0.75V rx threshold for rd/rp whereas it is 0.4V/0.7V
> > > for RT1711H. Add rxdz_sel variable to struct rt1711h_chip_info for
> > > handling this difference.

...

> > >  struct rt1711h_chip_info {
> > >       u16 did;
> > > +     u32 rxdz_sel;
> > >  };
> >
> > Again, run pahole. And see the difference, if any, depending on the place of a
> > new member. Note, some 64-bit architectures may require 8-byte alignment even
> > for 4-byte members.
> 
> Doesn't make a difference, the size and alignment of a structure are
> always multiples of the largest alignment of each of the members, so
> the structure size will be 8 bytes on both 32-bit and 64-bit (except
> on m68k, where it will be 6 bytes).
> 
> Either you have 2 bytes did, 2 bytes hole, and 4 bytes rxdz_sel, or
> 4 bytes rxdz, 2 bytes did, and 2 bytes hole (except on m68k, where
> there won't be any holes).

And I said "if any".
My suggestion is to check with pahole to be sure it's already good enough.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 4/4] usb: typec: tcpci_rt1711h: Add enable_pd30_extended_message feature bit to struct rt1711h_chip_info

[Biju Das]

The RT1715 has PD30 extended message compared to RT1711H. Add a feature bit
enable_pd30_extended_message to struct rt1711h_chip_info to enable this
feature only for RT1715.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/usb/typec/tcpm/tcpci_rt1711h.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm/tcpci_rt1711h.c b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
index 1b1753895ca5..e79cbb344c14 100644
--- a/drivers/usb/typec/tcpm/tcpci_rt1711h.c
+++ b/drivers/usb/typec/tcpm/tcpci_rt1711h.c
@@ -54,6 +54,7 @@
 struct rt1711h_chip_info {
 	u16 did;
 	u32 rxdz_sel;
+	unsigned enable_pd30_extended_message:1;
 };
 
 struct rt1711h_chip {
@@ -110,7 +111,7 @@ static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
 		return ret;
 
 	/* Enable PD30 extended message for RT1715 */
-	if (chip->info->did == RT1715_DID) {
+	if (chip->info->enable_pd30_extended_message) {
 		ret = regmap_update_bits(regmap, RT1711H_RTCTRL8,
 					 RT1711H_ENEXTMSG, RT1711H_ENEXTMSG);
 		if (ret < 0)
@@ -401,6 +402,7 @@ static const struct rt1711h_chip_info rt1711h = {
 static const struct rt1711h_chip_info rt1715 = {
 	.did = RT1715_DID,
 	.rxdz_sel = RT1711H_BMCIO_RXDZSEL,
+	.enable_pd30_extended_message = 1,
 };
 
 static const struct i2c_device_id rt1711h_id[] = {
-- 
2.25.1

Re: [PATCH 4/4] usb: typec: tcpci_rt1711h: Add enable_pd30_extended_message feature bit to struct rt1711h_chip_info

[Andy Shevchenko]

On Sun, Aug 20, 2023 at 07:44:02PM +0100, Biju Das wrote:
> The RT1715 has PD30 extended message compared to RT1711H. Add a feature bit
> enable_pd30_extended_message to struct rt1711h_chip_info to enable this
> feature only for RT1715.

...

>  struct rt1711h_chip_info {
>  	u16 did;
>  	u32 rxdz_sel;
> +	unsigned enable_pd30_extended_message:1;

Besides pahole results, the unsigned is deprecated to use, spell it as
unsigned int or u32 or... (find the best match).

>  };

...

> +	.enable_pd30_extended_message = 1,

Maybe even bool?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] usb: typec: tcpci_rt1711h: Add enable_pd30_extended_message feature bit to struct rt1711h_chip_info

[Guenter Roeck]

On Mon, Aug 21, 2023 at 04:08:15PM +0300, Andy Shevchenko wrote:
> On Sun, Aug 20, 2023 at 07:44:02PM +0100, Biju Das wrote:
> > The RT1715 has PD30 extended message compared to RT1711H. Add a feature bit
> > enable_pd30_extended_message to struct rt1711h_chip_info to enable this
> > feature only for RT1715.
> 
> ...
> 
> >  struct rt1711h_chip_info {
> >  	u16 did;
> >  	u32 rxdz_sel;
> > +	unsigned enable_pd30_extended_message:1;
> 
> Besides pahole results, the unsigned is deprecated to use, spell it as
> unsigned int or u32 or... (find the best match).

Just use bool.

Guenter

> 
> >  };
> 
> ...
> 
> > +	.enable_pd30_extended_message = 1,
> 
> Maybe even bool?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>