2024-02-23 16:38:15

by Michael J. Ruhl

[permalink] [raw]
Subject: [PATCH v2] clkdev: Update clkdev id usage to allow for longer names

clkdev DEV ID information is limited to an array of 20 bytes
(MAX_DEV_ID). It is possible that the ID could be longer than
that. If so, the lookup will fail because the "real ID" will
not match the copied value.

For instance, generating a device name for the I2C Designware
module using the PCI ID can result in a name of:

i2c_designware.39424

clkdev_create() will store:

i2c_designware.3942

The stored name is one off and will not match correctly during probe.

Increase the size of the ID to allow for a longer name.

v2: Removed CON_ID update and added example to commit

Signed-off-by: Michael J. Ruhl <[email protected]>
---
drivers/clk/clkdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index ee37d0be6877..9cd80522ca2d 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -144,7 +144,7 @@ void clkdev_add_table(struct clk_lookup *cl, size_t num)
mutex_unlock(&clocks_mutex);
}

-#define MAX_DEV_ID 20
+#define MAX_DEV_ID 24
#define MAX_CON_ID 16

struct clk_lookup_alloc {
--
2.41.0



2024-02-23 17:44:58

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2] clkdev: Update clkdev id usage to allow for longer names

On Fri, Feb 23, 2024 at 11:35:16AM -0500, Michael J. Ruhl wrote:
> clkdev DEV ID information is limited to an array of 20 bytes
> (MAX_DEV_ID). It is possible that the ID could be longer than
> that. If so, the lookup will fail because the "real ID" will
> not match the copied value.
>
> For instance, generating a device name for the I2C Designware
> module using the PCI ID can result in a name of:
>
> i2c_designware.39424
>
> clkdev_create() will store:
>
> i2c_designware.3942
>
> The stored name is one off and will not match correctly during probe.
>
> Increase the size of the ID to allow for a longer name.

> v2: Removed CON_ID update and added example to commit

Usually we put changelog out of the commit messages...

> Signed-off-by: Michael J. Ruhl <[email protected]>
> ---

..somewhere here.

Also just noticed that you forgot CCF maintainers to be included.
I'm suggesting to use my script [1] which harvests more or less
adequate list of people and mailing lists to Cc email to.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

--
With Best Regards,
Andy Shevchenko



2024-02-23 18:22:37

by Michael J. Ruhl

[permalink] [raw]
Subject: RE: [PATCH v2] clkdev: Update clkdev id usage to allow for longer names

>-----Original Message-----
>From: Andy Shevchenko <[email protected]>
>Sent: Friday, February 23, 2024 12:43 PM
>To: Ruhl, Michael J <[email protected]>
>Cc: [email protected]; [email protected]; linux-
>[email protected]
>Subject: Re: [PATCH v2] clkdev: Update clkdev id usage to allow for longer
>names
>
>On Fri, Feb 23, 2024 at 11:35:16AM -0500, Michael J. Ruhl wrote:
>> clkdev DEV ID information is limited to an array of 20 bytes
>> (MAX_DEV_ID). It is possible that the ID could be longer than
>> that. If so, the lookup will fail because the "real ID" will
>> not match the copied value.
>>
>> For instance, generating a device name for the I2C Designware
>> module using the PCI ID can result in a name of:
>>
>> i2c_designware.39424
>>
>> clkdev_create() will store:
>>
>> i2c_designware.3942
>>
>> The stored name is one off and will not match correctly during probe.
>>
>> Increase the size of the ID to allow for a longer name.
>
>> v2: Removed CON_ID update and added example to commit
>
>Usually we put changelog out of the commit messages...

Yeah, usually put it in the cover letter. I will remove. I see your script automatically
does a cover page...will use that format int the future.

>> Signed-off-by: Michael J. Ruhl <[email protected]>
>> ---
>
>...somewhere here.

Ok. Will keep for future reference.

>Also just noticed that you forgot CCF maintainers to be included.
>I'm suggesting to use my script [1] which harvests more or less
>adequate list of people and mailing lists to Cc email to.
>
>[1]: https://github.com/andy-shev/home-bin-
>tools/blob/master/ge2maintainer.sh

Using your script I got:

To: "Michael J. Ruhl" <[email protected]>,
[email protected],
[email protected]
Cc: Russell King <[email protected]>

My list (using get_maintainers.pl) is:

[email protected]
[email protected]
[email protected]

They appear to be the same....

I don't have the plain text part on Russel's email ([email protected]).. Is that what is missing?

Not sure what CCF is?

M

>
>--
>With Best Regards,
>Andy Shevchenko
>


2024-02-23 18:32:42

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2] clkdev: Update clkdev id usage to allow for longer names

On Fri, Feb 23, 2024 at 06:22:13PM +0000, Ruhl, Michael J wrote:
> >From: Andy Shevchenko <[email protected]>
> >Sent: Friday, February 23, 2024 12:43 PM
> >On Fri, Feb 23, 2024 at 11:35:16AM -0500, Michael J. Ruhl wrote:

..

> I will remove.

Not remove, but move to the comments/changelog (after '---' line)

> I see your script automatically does a cover page...will use that format int
> the future.

Only if there are more than a single patch.

..

> >[1]: https://github.com/andy-shev/home-bin-
> >tools/blob/master/ge2maintainer.sh
>
> Using your script I got:
>
> To: "Michael J. Ruhl" <[email protected]>,
> [email protected],
> [email protected]
> Cc: Russell King <[email protected]>
>
> My list (using get_maintainers.pl) is:
>
> [email protected]
> [email protected]
> [email protected]
>
> They appear to be the same....

Ah, the Russel's email looked like a mailing list, that what confused me.

> I don't have the plain text part on Russel's email ([email protected])... Is that what is missing?

Yes :-)
But my script also uses a heuristics (which is not visible here) to add active
developers of the code in question based on the git history.

> Not sure what CCF is?

Common Clock Framework (This is how it's advertised in MAINTAINERS)

--
With Best Regards,
Andy Shevchenko



2024-02-23 19:50:01

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2] clkdev: Update clkdev id usage to allow for longer names

On Fri, Feb 23, 2024 at 08:32:27PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 23, 2024 at 06:22:13PM +0000, Ruhl, Michael J wrote:
> > >From: Andy Shevchenko <[email protected]>
> > >Sent: Friday, February 23, 2024 12:43 PM
> > >On Fri, Feb 23, 2024 at 11:35:16AM -0500, Michael J. Ruhl wrote:
>
> ...
>
> > I will remove.
>
> Not remove, but move to the comments/changelog (after '---' line)
>
> > I see your script automatically does a cover page...will use that format int
> > the future.
>
> Only if there are more than a single patch.
>
> ...
>
> > >[1]: https://github.com/andy-shev/home-bin-
> > >tools/blob/master/ge2maintainer.sh
> >
> > Using your script I got:
> >
> > To: "Michael J. Ruhl" <[email protected]>,
> > [email protected],
> > [email protected]
> > Cc: Russell King <[email protected]>
> >
> > My list (using get_maintainers.pl) is:
> >
> > [email protected]
> > [email protected]
> > [email protected]
> >
> > They appear to be the same....
>
> Ah, the Russel's email looked like a mailing list, that what confused me.

Joe, I think you know that I'll pick up on your mis-spelling of my
name... and I take that as an implicit right to call you something
other than your proper name. :D

Secondly, because the Cc contained my name, I fail to see how you can
confuse that with a mailing list. Maybe your script that you mentioned
strips the names from the email addresses, thereby adding to your
confusion - and maybe that isn't such a good idea after all? I'm not
the only one who uses linux@... There are six people in total listed in
MAINTAINERS who have a linux@... email address there.

> > I don't have the plain text part on Russel's email ([email protected])... Is that what is missing?
>
> Yes :-)
> But my script also uses a heuristics (which is not visible here) to add active
> developers of the code in question based on the git history.

The developers in question for this part of the code is me and not the
CCF. Therefore, what has been done by the patch author is reasonable
and no special scripts are necessary.

While my main git server is offline, I'm happy for the CCF folk
to pick this up, so:

Reviewed-by: Russell King (Oracle) <[email protected]>

Michael, please resubmit with my r-b line above, and include the CCF
folk in that posting:

Michael Turquette <[email protected]>
Stephen Boyd <[email protected]>
[email protected]

Thanks!

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-02-23 20:13:07

by Michael J. Ruhl

[permalink] [raw]
Subject: RE: [PATCH v2] clkdev: Update clkdev id usage to allow for longer names

>-----Original Message-----
>From: Russell King <[email protected]>
>Sent: Friday, February 23, 2024 2:50 PM
>To: Andy Shevchenko <[email protected]>; Ruhl, Michael J
><[email protected]>
>Cc: [email protected]; [email protected]
>Subject: Re: [PATCH v2] clkdev: Update clkdev id usage to allow for longer
>names
>
>On Fri, Feb 23, 2024 at 08:32:27PM +0200, Andy Shevchenko wrote:
>> On Fri, Feb 23, 2024 at 06:22:13PM +0000, Ruhl, Michael J wrote:
>> > >From: Andy Shevchenko <[email protected]>
>> > >Sent: Friday, February 23, 2024 12:43 PM
>> > >On Fri, Feb 23, 2024 at 11:35:16AM -0500, Michael J. Ruhl wrote:
>>
>> ...
>>
>> > I will remove.
>>
>> Not remove, but move to the comments/changelog (after '---' line)
>>
>> > I see your script automatically does a cover page...will use that format int
>> > the future.
>>
>> Only if there are more than a single patch.
>>
>> ...
>>
>> > >[1]: https://github.com/andy-shev/home-bin-
>> > >tools/blob/master/ge2maintainer.sh
>> >
>> > Using your script I got:
>> >
>> > To: "Michael J. Ruhl" <[email protected]>,
>> > [email protected],
>> > [email protected]
>> > Cc: Russell King <[email protected]>
>> >
>> > My list (using get_maintainers.pl) is:
>> >
>> > [email protected]
>> > [email protected]
>> > [email protected]
>> >
>> > They appear to be the same....
>>
>> Ah, the Russel's email looked like a mailing list, that what confused me.
>
>Joe, I think you know that I'll pick up on your mis-spelling of my
>name... and I take that as an implicit right to call you something
>other than your proper name. :D
>
>Secondly, because the Cc contained my name, I fail to see how you can
>confuse that with a mailing list. Maybe your script that you mentioned
>strips the names from the email addresses, thereby adding to your
>confusion - and maybe that isn't such a good idea after all? I'm not
>the only one who uses linux@... There are six people in total listed in
>MAINTAINERS who have a linux@... email address there.

????

Andy's script picked up your name for the CC... I stripped out all but the email text
for my posting... so this is my fault....

>
>> > I don't have the plain text part on Russel's email ([email protected])...
>Is that what is missing?
>>
>> Yes :-)
>> But my script also uses a heuristics (which is not visible here) to add active
>> developers of the code in question based on the git history.
>
>The developers in question for this part of the code is me and not the
>CCF. Therefore, what has been done by the patch author is reasonable
>and no special scripts are necessary.
>
>While my main git server is offline, I'm happy for the CCF folk
>to pick this up, so:
>
>Reviewed-by: Russell King (Oracle) <[email protected]>
>
>Michael, please resubmit with my r-b line above, and include the CCF
>folk in that posting:

I will get the appropriate names listed and repost.

Thank you!

M

>Michael Turquette <[email protected]>
>Stephen Boyd <[email protected]>
>[email protected]
>
>Thanks!
>
>--
>RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2024-02-23 20:14:55

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v2] clkdev: Update clkdev id usage to allow for longer names

On Fri, Feb 23, 2024 at 07:49:33PM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 23, 2024 at 08:32:27PM +0200, Andy Shevchenko wrote:
> > On Fri, Feb 23, 2024 at 06:22:13PM +0000, Ruhl, Michael J wrote:
> > > >From: Andy Shevchenko <[email protected]>
> > > >Sent: Friday, February 23, 2024 12:43 PM
> > > >On Fri, Feb 23, 2024 at 11:35:16AM -0500, Michael J. Ruhl wrote:

..

> > > >[1]: https://github.com/andy-shev/home-bin-
> > > >tools/blob/master/ge2maintainer.sh
> > >
> > > Using your script I got:
> > >
> > > To: "Michael J. Ruhl" <[email protected]>,
> > > [email protected],
> > > [email protected]
> > > Cc: Russell King <[email protected]>
> > >
> > > My list (using get_maintainers.pl) is:
> > >
> > > [email protected]
> > > [email protected]
> > > [email protected]
> > >
> > > They appear to be the same....
> >
> > Ah, the Russel's email looked like a mailing list, that what confused me.
>
> Joe, I think you know that I'll pick up on your mis-spelling of my
> name... and I take that as an implicit right to call you something
> other than your proper name. :D

Since, Javier, you told me that, I now remember some rumors... :D

> Secondly, because the Cc contained my name, I fail to see how you can
> confuse that with a mailing list. Maybe your script that you mentioned
> strips the names from the email addresses, thereby adding to your
> confusion - and maybe that isn't such a good idea after all?

It's other way around. My script uses full names.

> I'm not the only one who uses linux@... There are six people in total listed
> in MAINTAINERS who have a linux@... email address there.

Yes, but you are the only one which pops up WRT this file.

> > > I don't have the plain text part on Russel's email
> > > ([email protected])... Is that what is missing?
> >
> > Yes :-)
> > But my script also uses a heuristics (which is not visible here) to add active
> > developers of the code in question based on the git history.
>
> The developers in question for this part of the code is me and not the
> CCF.

Yes, get_maintainer.pl seems to return that. It's me who naively considered you
as CCF maintainer.

> Therefore, what has been done by the patch author is reasonable
> and no special scripts are necessary.

The scripts makes life easier and robust against changes.

--
With Best Regards,
Andy Shevchenko