Platform device matching, & weird strncmp usage

Platform device matching, & weird strncmp usage

[Benjamin Herrenschmidt]

Hi !

In 2.6.15, platform device matching works according to this comment in
the code, or rather are supposed to:


 *	Platform device IDs are assumed to be encoded like this:
 *	"<name><instance>", where <name> is a short description of the
 *	type of device, like "pci" or "floppy", and <instance> is the
 *	enumerated instance of the device, like '0' or '42'.

However, looking a few lines below, I see the actual implemetation:

static int platform_match(struct device * dev, struct device_driver * drv)
{
	struct platform_device *pdev = container_of(dev, struct platform_device, dev);

	return (strncmp(pdev->name, drv->name, BUS_ID_SIZE) == 0);
}

As far as I know, strncmp() is _NOT_ supposed to return 0 if one string
is shorter than the other and they match until that point. Thus the
above will never match unless the <name> portion of pdev->name is
exactly of size BUS_ID_SIZE which is obviously not the case...

Did I miss something or do we expect a "special" semantic for strncmp in
the kernel ?

Ben.

Re: Platform device matching, & weird strncmp usage

[Benjamin Herrenschmidt]

On Fri, 2006-01-06 at 16:59 +1100, Benjamin Herrenschmidt wrote:
> Hi !
> 
> In 2.6.15, platform device matching works according to this comment in
> the code, or rather are supposed to:

  .../...

Just in case my analysis is correct, here's an untested fix:

---

Platform device matching doesn't work when an "id" is used.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

--- linux-work.orig/drivers/base/platform.c	2005-11-24 17:18:43.000000000 +1100
+++ linux-work/drivers/base/platform.c	2006-01-06 16:59:59.000000000 +1100
@@ -447,7 +447,8 @@ static int platform_match(struct device 
 {
 	struct platform_device *pdev = container_of(dev, struct platform_device, dev);
 
-	return (strncmp(pdev->name, drv->name, BUS_ID_SIZE) == 0);
+	return strncmp(pdev->name, drv->name,
+			min(BUS_ID_SIZE, strlen(drv->name))) == 0;
 }
 
 static int platform_suspend(struct device * dev, pm_message_t state)

Re: Platform device matching, & weird strncmp usage

[Russell King]

On Fri, Jan 06, 2006 at 04:59:39PM +1100, Benjamin Herrenschmidt wrote:
> static int platform_match(struct device * dev, struct device_driver * drv)
> {
> 	struct platform_device *pdev = container_of(dev, struct platform_device, dev);
> 
> 	return (strncmp(pdev->name, drv->name, BUS_ID_SIZE) == 0);
> }
> 
> As far as I know, strncmp() is _NOT_ supposed to return 0 if one string
> is shorter than the other and they match until that point. Thus the
> above will never match unless the <name> portion of pdev->name is
> exactly of size BUS_ID_SIZE which is obviously not the case...

pdev->name is just the <name> part - it's pdev->dev.name which has
both the <name> and <instance>.  I think the strncmp is unnecessary,
and it can be replaced by a plain strcmp.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Platform device matching, & weird strncmp usage

[Benjamin Herrenschmidt]

On Fri, 2006-01-06 at 18:53 +0000, Russell King wrote:
> On Fri, Jan 06, 2006 at 04:59:39PM +1100, Benjamin Herrenschmidt wrote:
> > static int platform_match(struct device * dev, struct device_driver * drv)
> > {
> > 	struct platform_device *pdev = container_of(dev, struct platform_device, dev);
> > 
> > 	return (strncmp(pdev->name, drv->name, BUS_ID_SIZE) == 0);
> > }
> > 
> > As far as I know, strncmp() is _NOT_ supposed to return 0 if one string
> > is shorter than the other and they match until that point. Thus the
> > above will never match unless the <name> portion of pdev->name is
> > exactly of size BUS_ID_SIZE which is obviously not the case...
> 
> pdev->name is just the <name> part - it's pdev->dev.name which has
> both the <name> and <instance>.  I think the strncmp is unnecessary,
> and it can be replaced by a plain strcmp.

Ah indeed, I got confused by having both strings.

Ben.

Re: Platform device matching, & weird strncmp usage

[Kurtis D. Rader]

On Fri, 2006-01-06 16:59:39, Benjamin Herrenschmidt wrote:
> In 2.6.15, platform device matching works according to this comment in
> the code, or rather are supposed to:
> 
>  *	Platform device IDs are assumed to be encoded like this:
>  *	"<name><instance>", where <name> is a short description of the
>  *	type of device, like "pci" or "floppy", and <instance> is the
>  *	enumerated instance of the device, like '0' or '42'.
> 
> However, looking a few lines below, I see the actual implemetation:
> 
> static int platform_match(struct device * dev, struct device_driver * drv)
> {
> 	struct platform_device *pdev = container_of(dev, struct platform_device, dev);
> 
> 	return (strncmp(pdev->name, drv->name, BUS_ID_SIZE) == 0);
> }
> 
> As far as I know, strncmp() is _NOT_ supposed to return 0 if one string
> is shorter than the other and they match until that point. Thus the
> above will never match unless the <name> portion of pdev->name is
> exactly of size BUS_ID_SIZE which is obviously not the case...
> 
> Did I miss something or do we expect a "special" semantic for strncmp in
> the kernel ?

I can't speak to the correctness of that code but your understanding of
strncmp() is incorrect. From "GNU C Library Application Fundamentals":

    This function is the [sic] similar to strcmp, except that no more
    than size wide characters are compared. In other words, if the two
    strings are the same in their first size wide characters, the return
    value is zero.

And this has been may experience for the past 20 years and is confirmed by
this trivial program which prints zero in both cases:

#include <string.h>
#include <stdio.h>
int main() {
    printf("%d\n", strncmp("abc","abcd",3));
    printf("%d\n", strncmp("abcd","abc",3));
}

Re: Platform device matching, & weird strncmp usage

[Bob Copeland]

On 1/7/06, Kurtis D. Rader <kdrader@us.ibm.com> wrote:
> On Fri, 2006-01-06 16:59:39, Benjamin Herrenschmidt wrote:
> > As far as I know, strncmp() is _NOT_ supposed to return 0 if one string
> > is shorter than the other and they match until that point

> I can't speak to the correctness of that code but your understanding of
> strncmp() is incorrect. From "GNU C Library Application Fundamentals":

I believe the original poster was asserting that
'strncmp("abc","abcd",100) should never return zero,' and not
'strncmp("abc","abcd",3) should never return zero.'  Though I also
found the statement confusing at first.

-Bob

Re: Platform device matching, & weird strncmp usage

[Benjamin Herrenschmidt]

> I can't speak to the correctness of that code but your understanding of
> strncmp() is incorrect. From "GNU C Library Application Fundamentals":
> 
>     This function is the [sic] similar to strcmp, except that no more
>     than size wide characters are compared. In other words, if the two
>     strings are the same in their first size wide characters, the return
>     value is zero.
> 
> And this has been may experience for the past 20 years and is confirmed by
> this trivial program which prints zero in both cases:
> 
> #include <string.h>
> #include <stdio.h>
> int main() {
>     printf("%d\n", strncmp("abc","abcd",3));
>     printf("%d\n", strncmp("abcd","abc",3));
> }

Except that in my example, the count was larger than both strings...

Anyway, it happens to not be a problem as the string I was comparing was
not in fact different.

Ben.