Skip to contentSkip to conversation

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

DISCUSSION NeilBrown 2 participants 5 messages Jul 26, 2013

lore
Share & tools
lore

NeilBrown

author
#0 lore raw

Hi,
the commit:

commit 09fc7d22b024692b2fe8a943b246de1af307132b

Author: Felipe Balbi <[email protected]>
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

#1 lore raw
Quoted 19 lines · NeilBrown

Hi,
the commit:

commit 09fc7d22b024692b2fe8a943b246de1af307132b

Author: Felipe Balbi <[email protected]>
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.

Quoted 8 lines

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

Quoted 10 lines

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

Signature · 1 line
balbi
author
#2 lore raw
Quoted 23 lines · On Fri, 26 Jul 2013 13:20:08 +0300 Felipe Balbi <[email protected]> wrote:
Quoted 22 lines · NeilBrown

Hi,
the commit:

commit 09fc7d22b024692b2fe8a943b246de1af307132b

Author: Felipe Balbi <[email protected]>
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.

Quoted 8 lines
Quoted 5 lines

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.

Quoted 5 lines
Quoted 5 lines

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.

Quoted 9 lines
Quoted 6 lines

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

#3 lore raw
Quoted 48 lines · NeilBrown
Quoted 23 lines · On Fri, 26 Jul 2013 13:20:08 +0300 Felipe Balbi <[email protected]> wrote:
Quoted 22 lines · NeilBrown

Hi,
the commit:

commit 09fc7d22b024692b2fe8a943b246de1af307132b

Author: Felipe Balbi <[email protected]>
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.

Quoted 8 lines
Quoted 5 lines

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.

Quoted 12 lines
Quoted 11 lines

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

Quoted 7 lines
Quoted 2 lines

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

Signature · 1 line
balbi
author
#4 lore raw
Quoted 52 lines · On Fri, 26 Jul 2013 16:51:08 +0300 Felipe Balbi <[email protected]> wrote:
Quoted 51 lines · NeilBrown
Quoted 23 lines · On Fri, 26 Jul 2013 13:20:08 +0300 Felipe Balbi <[email protected]> wrote:
Quoted 22 lines · NeilBrown

Hi,
the commit:

commit 09fc7d22b024692b2fe8a943b246de1af307132b

Author: Felipe Balbi <[email protected]>
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.

Quoted 8 lines
Quoted 5 lines

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.

Quoted 14 lines
Quoted 11 lines

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

Quoted 8 lines
Quoted 4 lines

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