omap2430 regression due to "usb: musb: fix incorrect usage of resource pointer"

omap2430 regression due to "usb: musb: fix incorrect usage of resource pointer"

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]


Hi,
 the commit:

commit 09fc7d22b024692b2fe8a943b246de1af307132b
Author: Felipe Balbi <balbi@ti.com>
Date:   Wed Apr 24 17:21:42 2013 +0300

    usb: musb: fix incorrect usage of resource pointer
    
    We can't simply pass the resource pointer from our
    device down to our children, otherwise module
    reinsertion will not work as the resource will
    continue to be marked as busy.
    
    Fix it by building a proper struct resource for
    our child musb device.
 

(even with the subsequent compile fix applied) breaks my USB-OTG port on my
GTA04.

I must admit that I don't see the point of the patch at all.
Instead of passing one array to platform_device_add_resources()
which will copy it into malloced space, it copies bits of the array onto
the stack on passes that for platform_device_add_resources to copy.
What is the value of the second copy??

But that isn't the real problem.  The real problem is that in omap2430_probe,
on my board at least, pdev->num_resources == 3.  But only the first 2 entries
in the array are copied.

If I make the array one entry larger and copy the extra entry it works.  Of
course that might break for someone else who only has 2 resources to copy....

Can we go back to just leaving it to platform_device_add_resources to do the
copying it have I missed something?  (I can't see where the new old code
would mark something as 'busy' that the new code won't mark as busy...)

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: omap2430 regression due to "usb: musb: fix incorrect usage of resource pointer"

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1753 bytes --]

On Fri, Jul 26, 2013 at 07:14:45PM +1000, NeilBrown wrote:
> 
> Hi,
>  the commit:
> 
> commit 09fc7d22b024692b2fe8a943b246de1af307132b
> Author: Felipe Balbi <balbi@ti.com>
> Date:   Wed Apr 24 17:21:42 2013 +0300
> 
>     usb: musb: fix incorrect usage of resource pointer
>     
>     We can't simply pass the resource pointer from our
>     device down to our children, otherwise module
>     reinsertion will not work as the resource will
>     continue to be marked as busy.
>     
>     Fix it by building a proper struct resource for
>     our child musb device.
>  
> 
> (even with the subsequent compile fix applied) breaks my USB-OTG port on my
> GTA04.
> 
> I must admit that I don't see the point of the patch at all.
> Instead of passing one array to platform_device_add_resources()
> which will copy it into malloced space, it copies bits of the array onto
> the stack on passes that for platform_device_add_resources to copy.
> What is the value of the second copy??

if you don't do that, the resource will already be busy and ioremap will
fail on second modprobe.

> But that isn't the real problem.  The real problem is that in omap2430_probe,
> on my board at least, pdev->num_resources == 3.  But only the first 2 entries
> in the array are copied.
> 
> If I make the array one entry larger and copy the extra entry it works.  Of
> course that might break for someone else who only has 2 resources to copy....
> 
> Can we go back to just leaving it to platform_device_add_resources to do the
> copying it have I missed something?  (I can't see where the new old code
> would mark something as 'busy' that the new code won't mark as busy...)

patches already in Greg's queue

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: omap2430 regression due to "usb: musb: fix incorrect usage of resource pointer"

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]

On Fri, 26 Jul 2013 13:20:08 +0300 Felipe Balbi <balbi@ti.com> wrote:

> On Fri, Jul 26, 2013 at 07:14:45PM +1000, NeilBrown wrote:
> > 
> > Hi,
> >  the commit:
> > 
> > commit 09fc7d22b024692b2fe8a943b246de1af307132b
> > Author: Felipe Balbi <balbi@ti.com>
> > Date:   Wed Apr 24 17:21:42 2013 +0300
> > 
> >     usb: musb: fix incorrect usage of resource pointer
> >     
> >     We can't simply pass the resource pointer from our
> >     device down to our children, otherwise module
> >     reinsertion will not work as the resource will
> >     continue to be marked as busy.
> >     
> >     Fix it by building a proper struct resource for
> >     our child musb device.
> >  
> > 
> > (even with the subsequent compile fix applied) breaks my USB-OTG port on my
> > GTA04.
> > 
> > I must admit that I don't see the point of the patch at all.
> > Instead of passing one array to platform_device_add_resources()
> > which will copy it into malloced space, it copies bits of the array onto
> > the stack on passes that for platform_device_add_resources to copy.
> > What is the value of the second copy??
> 
> if you don't do that, the resource will already be busy and ioremap will
> fail on second modprobe.

Ahhh... the parent/sibling/child linkage - got it.

> 
> > But that isn't the real problem.  The real problem is that in omap2430_probe,
> > on my board at least, pdev->num_resources == 3.  But only the first 2 entries
> > in the array are copied.
> > 
> > If I make the array one entry larger and copy the extra entry it works.  Of
> > course that might break for someone else who only has 2 resources to copy....
> > 
> > Can we go back to just leaving it to platform_device_add_resources to do the
> > copying it have I missed something?  (I can't see where the new old code
> > would mark something as 'busy' that the new code won't mark as busy...)
> 
> patches already in Greg's queue
> 

Thanks. 
Doesn't seem to be in the usb-next or usb-linus branches of 
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
Can you give me a pointer?

Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: omap2430 regression due to "usb: musb: fix incorrect usage of resource pointer"

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]

On Fri, Jul 26, 2013 at 10:24:58PM +1000, NeilBrown wrote:
> On Fri, 26 Jul 2013 13:20:08 +0300 Felipe Balbi <balbi@ti.com> wrote:
> 
> > On Fri, Jul 26, 2013 at 07:14:45PM +1000, NeilBrown wrote:
> > > 
> > > Hi,
> > >  the commit:
> > > 
> > > commit 09fc7d22b024692b2fe8a943b246de1af307132b
> > > Author: Felipe Balbi <balbi@ti.com>
> > > Date:   Wed Apr 24 17:21:42 2013 +0300
> > > 
> > >     usb: musb: fix incorrect usage of resource pointer
> > >     
> > >     We can't simply pass the resource pointer from our
> > >     device down to our children, otherwise module
> > >     reinsertion will not work as the resource will
> > >     continue to be marked as busy.
> > >     
> > >     Fix it by building a proper struct resource for
> > >     our child musb device.
> > >  
> > > 
> > > (even with the subsequent compile fix applied) breaks my USB-OTG port on my
> > > GTA04.
> > > 
> > > I must admit that I don't see the point of the patch at all.
> > > Instead of passing one array to platform_device_add_resources()
> > > which will copy it into malloced space, it copies bits of the array onto
> > > the stack on passes that for platform_device_add_resources to copy.
> > > What is the value of the second copy??
> > 
> > if you don't do that, the resource will already be busy and ioremap will
> > fail on second modprobe.
> 
> Ahhh... the parent/sibling/child linkage - got it.
> 
> > 
> > > But that isn't the real problem.  The real problem is that in omap2430_probe,
> > > on my board at least, pdev->num_resources == 3.  But only the first 2 entries
> > > in the array are copied.
> > > 
> > > If I make the array one entry larger and copy the extra entry it works.  Of
> > > course that might break for someone else who only has 2 resources to copy....
> > > 
> > > Can we go back to just leaving it to platform_device_add_resources to do the
> > > copying it have I missed something?  (I can't see where the new old code
> > > would mark something as 'busy' that the new code won't mark as busy...)
> > 
> > patches already in Greg's queue
> > 
> 
> Thanks. 
> Doesn't seem to be in the usb-next or usb-linus branches of 
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> Can you give me a pointer?

hmmm, looks like this is not in greg/usb-linus indeed. I'll send him
another pull request on monday. If you wanna try, use my 'fixes' branch
on kernel.org.

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: omap2430 regression due to "usb: musb: fix incorrect usage of resource pointer"

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2761 bytes --]

On Fri, 26 Jul 2013 16:51:08 +0300 Felipe Balbi <balbi@ti.com> wrote:

> On Fri, Jul 26, 2013 at 10:24:58PM +1000, NeilBrown wrote:
> > On Fri, 26 Jul 2013 13:20:08 +0300 Felipe Balbi <balbi@ti.com> wrote:
> > 
> > > On Fri, Jul 26, 2013 at 07:14:45PM +1000, NeilBrown wrote:
> > > > 
> > > > Hi,
> > > >  the commit:
> > > > 
> > > > commit 09fc7d22b024692b2fe8a943b246de1af307132b
> > > > Author: Felipe Balbi <balbi@ti.com>
> > > > Date:   Wed Apr 24 17:21:42 2013 +0300
> > > > 
> > > >     usb: musb: fix incorrect usage of resource pointer
> > > >     
> > > >     We can't simply pass the resource pointer from our
> > > >     device down to our children, otherwise module
> > > >     reinsertion will not work as the resource will
> > > >     continue to be marked as busy.
> > > >     
> > > >     Fix it by building a proper struct resource for
> > > >     our child musb device.
> > > >  
> > > > 
> > > > (even with the subsequent compile fix applied) breaks my USB-OTG port on my
> > > > GTA04.
> > > > 
> > > > I must admit that I don't see the point of the patch at all.
> > > > Instead of passing one array to platform_device_add_resources()
> > > > which will copy it into malloced space, it copies bits of the array onto
> > > > the stack on passes that for platform_device_add_resources to copy.
> > > > What is the value of the second copy??
> > > 
> > > if you don't do that, the resource will already be busy and ioremap will
> > > fail on second modprobe.
> > 
> > Ahhh... the parent/sibling/child linkage - got it.
> > 
> > > 
> > > > But that isn't the real problem.  The real problem is that in omap2430_probe,
> > > > on my board at least, pdev->num_resources == 3.  But only the first 2 entries
> > > > in the array are copied.
> > > > 
> > > > If I make the array one entry larger and copy the extra entry it works.  Of
> > > > course that might break for someone else who only has 2 resources to copy....
> > > > 
> > > > Can we go back to just leaving it to platform_device_add_resources to do the
> > > > copying it have I missed something?  (I can't see where the new old code
> > > > would mark something as 'busy' that the new code won't mark as busy...)
> > > 
> > > patches already in Greg's queue
> > > 
> > 
> > Thanks. 
> > Doesn't seem to be in the usb-next or usb-linus branches of 
> > git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> > Can you give me a pointer?
> 
> hmmm, looks like this is not in greg/usb-linus indeed. I'll send him
> another pull request on monday. If you wanna try, use my 'fixes' branch
> on kernel.org.

Thanks.  I found that one, pulled it in, and can confirm that it fixed my USB
problem.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]