Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
W dniu 17.02.2015 o 22:02, Ruslan Bilovol pisze: Hi Andrzej, On Mon, Feb 16, 2015 at 10:07 AM, Andrzej Pietrasiewicz andrze...@samsung.com wrote: W dniu 15.02.2015 o 23:43, Ruslan Bilovol pisze: snip In my opinion all things which you have described are working out-of-box when you use configfs interface. It's mostly ready so you may create equivalent of most legacy gadgets (apart from printer and tcm) and just bind from one udc to another whenever you want. It's because legacy gadgets are easy to use and usually don't need any userspace-side configuration. Are there any plans to remove legacy drivers in the future? I'm not going to express strong opinions here, but their name implies that this can happen, some time in the future. And I also think it will not happen before the userspace part (libusbg, gt, gadgetd etc) is mature enough. My personal opinion in that matter is that it will take at least a couple of years to remove legacy gadgets entirely. OK, so it looks like there is a sense even to add new gadget/functions with legacy support I'm not sure what you mean exactly. For sure legacy gadgets are supported as long as they are a part of the mainline kernel. So any changes you make to the kernel must not affect the legacy gadgets, or you need to modify the legacy gadgets too and have them working. But adding new legacy-style gadgets is a completely different story. IMHO you need a _very_ good reason to succeed, but I remember Felipe expressing an opinion that chances or merging another legacy gadget were zero. AP -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Hi Andrzej, On Mon, Feb 16, 2015 at 10:07 AM, Andrzej Pietrasiewicz andrze...@samsung.com wrote: W dniu 15.02.2015 o 23:43, Ruslan Bilovol pisze: snip In my opinion all things which you have described are working out-of-box when you use configfs interface. It's mostly ready so you may create equivalent of most legacy gadgets (apart from printer and tcm) and just bind from one udc to another whenever you want. It's because legacy gadgets are easy to use and usually don't need any userspace-side configuration. Are there any plans to remove legacy drivers in the future? I'm not going to express strong opinions here, but their name implies that this can happen, some time in the future. And I also think it will not happen before the userspace part (libusbg, gt, gadgetd etc) is mature enough. My personal opinion in that matter is that it will take at least a couple of years to remove legacy gadgets entirely. OK, so it looks like there is a sense even to add new gadget/functions with legacy support Thanks, Ruslan AP -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
W dniu 15.02.2015 o 23:43, Ruslan Bilovol pisze: snip In my opinion all things which you have described are working out-of-box when you use configfs interface. It's mostly ready so you may create equivalent of most legacy gadgets (apart from printer and tcm) and just bind from one udc to another whenever you want. It's because legacy gadgets are easy to use and usually don't need any userspace-side configuration. Are there any plans to remove legacy drivers in the future? I'm not going to express strong opinions here, but their name implies that this can happen, some time in the future. And I also think it will not happen before the userspace part (libusbg, gt, gadgetd etc) is mature enough. My personal opinion in that matter is that it will take at least a couple of years to remove legacy gadgets entirely. AP -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Hi Alan, On Mon, Feb 9, 2015 at 10:00 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 9 Feb 2015, Krzysztof Opasiak wrote: Why bother matching by name? Why not simply take the first available UDC? Because you may have more than one udc. This would allow to pick one by name just like using configfs interface. Clearly it would be more flexible to allow the user to do the matching, the way configfs does (sysfs too). Main feature of my path is not only deferred binding of gadget driver, but also possibility to register/unregister udc at any time. This is useful for user who can load, for example, udc module if needed and unload it safely, not touching gadget driver. We can already do that with the existing code. There's no need for a patch. Also, it's not clear that the existing gadget drivers will work properly if they are unbound from one UDC and then bound again to another one. They were not written with that sort of thing in mind. What you have described is one of basics configfs features. You should be able to bind and unbind your gadget whenever you want and it should work properly after doing: ## create gadget $ echo udc.0 UDC $ echo UDC $ echo udc.1 UDC Function shouldn't care which udc it has been bound previously. Only current one is important and on each unbind each function should cleanup its state and prepare to be bound to another udc. Configfs interface doesn't prohibit this and I haven't seen any info about such restriction. It's not prohibited, but it also was never required. Therefore it may not be implemented in all gadget drivers. If some function is not working in such situation there is a bug in that function and it should be fixed. That's fine. I wasn't pointing out a fundamental limitation, just a fact that will have to be taken into account. Anyway, instead of going through all this, why not do what I suggested earlier (see http://marc.info/?l=linux-usbm=139888691230119w=2) and create a gadget bus type? That would give userspace explicit control over which gadget driver was bound to which UDC. Or maybe that's not a very good fit with the existing code, since most gadget drivers assume they will be bound to only one UDC at a time. So instead, why not create a sysfs interface that allows userspace to control which gadget drivers are bound to which UDCs? As I recall, the original problem people were complaining about was deferred probing. They didn't need fancy matching; all they wanted was the ability to bind a gadget driver to a UDC some time after the gadget driver was loaded and initialized. Something simple like Robert Baldyga's patch is enough to do that. I looked over my patch and see that it doesn't automatically rebind gadget driver to another available UDC after previous UDC is unbound. The Gadget Bus mentioned previously is good thing but so far it seems there is no users of such approach. So I left only deferred registration from my patch. I still keep it inside of udc-core since we also need to keep tracking UDC name if somebody wanted to bind gadget driver to specific UDC and it looks like good place to maintain this. I'll send second version of patches soon -- Best regards, Ruslan Bilvol -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Hi Krzysztof, On Tue, Feb 10, 2015 at 10:47 AM, Krzysztof Opasiak k.opas...@samsung.com wrote: -Original Message- From: Ruslan Bilovol [mailto:ruslan.bilo...@gmail.com] Sent: Tuesday, February 10, 2015 12:46 AM To: Alan Stern Cc: Krzysztof Opasiak; Peter Chen; linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; Balbi, Felipe; gre...@linuxfoundation.org; Andrzej Pietrasiewicz Subject: Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers Hi guys, On Mon, Feb 9, 2015 at 10:00 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 9 Feb 2015, Krzysztof Opasiak wrote: Why bother matching by name? Why not simply take the first available UDC? Because you may have more than one udc. This would allow to pick one by name just like using configfs interface. Clearly it would be more flexible to allow the user to do the matching, the way configfs does (sysfs too). Main feature of my path is not only deferred binding of gadget driver, but also possibility to register/unregister udc at any time. This is useful for user who can load, for example, udc module if needed and unload it safely, not touching gadget driver. We can already do that with the existing code. There's no need for a patch. Also, it's not clear that the existing gadget drivers will work properly if they are unbound from one UDC and then bound again to another one. They were not written with that sort of thing in mind. What you have described is one of basics configfs features. You should be able to bind and unbind your gadget whenever you want and it should work properly after doing: ## create gadget $ echo udc.0 UDC $ echo UDC $ echo udc.1 UDC Function shouldn't care which udc it has been bound previously. Only current one is important and on each unbind each function should cleanup its state and prepare to be bound to another udc. Configfs interface doesn't prohibit this and I haven't seen any info about such restriction. Thank you Krzysztof for this explanation that makes things more clear for reviewers. It's not prohibited, but it also was never required. Therefore it may not be implemented in all gadget drivers. If some function is not working in such situation there is a bug in that function and it should be fixed. That's fine. I wasn't pointing out a fundamental limitation, just a fact that will have to be taken into account. I also don't see any restrictions to bind/unbind gadget driver few times and in case of such bug we just can fix it. I didn't see any issue with gadget drivers that I used for verification, however, to be honest, I didn't test it with all possible ones. Anyway, it should work in legacy way (one gadget driver is bound to one uds) so current behavior at least is not broken. Anyway, instead of going through all this, why not do what I suggested earlier (see http://marc.info/?l=linux-usbm=139888691230119w=2) and create a gadget bus type? That would give userspace explicit control over which gadget driver was bound to which UDC. Exactly, my patch transforms udc-core to something like tiny bus with very basic functionality. But in mentioned mailthread I see good ideas that is possible to implement with small overhead. Or maybe that's not a very good fit with the existing code, since most gadget drivers assume they will be bound to only one UDC at a time. So instead, why not create a sysfs interface that allows userspace to control which gadget drivers are bound to which UDCs? As I recall, the original problem people were complaining about was deferred probing. They didn't need fancy matching; all they wanted was the ability to bind a gadget driver to a UDC some time after the gadget driver was loaded and initialized. Something simple like Robert Baldyga's patch is enough to do that. This simplicity is also a limitation. As I mentioned before, sometimes it is needed to be able to bind/unbind gadget driver multiple times to different UDCs. A real case I faced recently is SoC with HighSpeed-only and SuperSpeed-only UDCs. SuperSpeed-only UDC can't work on USB 2.0 speeds and vice versa. The system has USB3.0 USB connector with soldered HS lines from mentioned HS-only and SS lines from SS-only UDCs. Each UDC has it's own device driver, so if we want to use both of them with one gadget driver, we must be able to bind/unbind it multiple times to different UDCs. Another one usecase is users who can unload udc drivers without carrying about gadget drivers. Third usecase is, for example, developers who can switch between dummy_hcd and real UDC hardware without unloading gadget driver. Just a stupid question. Why don't you use configfs composite gadget instead of legacy gadgets? In my opinion all things which you have described
RE: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
-Original Message- From: Ruslan Bilovol [mailto:ruslan.bilo...@gmail.com] Sent: Tuesday, February 10, 2015 12:46 AM To: Alan Stern Cc: Krzysztof Opasiak; Peter Chen; linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; Balbi, Felipe; gre...@linuxfoundation.org; Andrzej Pietrasiewicz Subject: Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers Hi guys, On Mon, Feb 9, 2015 at 10:00 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 9 Feb 2015, Krzysztof Opasiak wrote: Why bother matching by name? Why not simply take the first available UDC? Because you may have more than one udc. This would allow to pick one by name just like using configfs interface. Clearly it would be more flexible to allow the user to do the matching, the way configfs does (sysfs too). Main feature of my path is not only deferred binding of gadget driver, but also possibility to register/unregister udc at any time. This is useful for user who can load, for example, udc module if needed and unload it safely, not touching gadget driver. We can already do that with the existing code. There's no need for a patch. Also, it's not clear that the existing gadget drivers will work properly if they are unbound from one UDC and then bound again to another one. They were not written with that sort of thing in mind. What you have described is one of basics configfs features. You should be able to bind and unbind your gadget whenever you want and it should work properly after doing: ## create gadget $ echo udc.0 UDC $ echo UDC $ echo udc.1 UDC Function shouldn't care which udc it has been bound previously. Only current one is important and on each unbind each function should cleanup its state and prepare to be bound to another udc. Configfs interface doesn't prohibit this and I haven't seen any info about such restriction. Thank you Krzysztof for this explanation that makes things more clear for reviewers. It's not prohibited, but it also was never required. Therefore it may not be implemented in all gadget drivers. If some function is not working in such situation there is a bug in that function and it should be fixed. That's fine. I wasn't pointing out a fundamental limitation, just a fact that will have to be taken into account. I also don't see any restrictions to bind/unbind gadget driver few times and in case of such bug we just can fix it. I didn't see any issue with gadget drivers that I used for verification, however, to be honest, I didn't test it with all possible ones. Anyway, it should work in legacy way (one gadget driver is bound to one uds) so current behavior at least is not broken. Anyway, instead of going through all this, why not do what I suggested earlier (see http://marc.info/?l=linux-usbm=139888691230119w=2) and create a gadget bus type? That would give userspace explicit control over which gadget driver was bound to which UDC. Exactly, my patch transforms udc-core to something like tiny bus with very basic functionality. But in mentioned mailthread I see good ideas that is possible to implement with small overhead. Or maybe that's not a very good fit with the existing code, since most gadget drivers assume they will be bound to only one UDC at a time. So instead, why not create a sysfs interface that allows userspace to control which gadget drivers are bound to which UDCs? As I recall, the original problem people were complaining about was deferred probing. They didn't need fancy matching; all they wanted was the ability to bind a gadget driver to a UDC some time after the gadget driver was loaded and initialized. Something simple like Robert Baldyga's patch is enough to do that. This simplicity is also a limitation. As I mentioned before, sometimes it is needed to be able to bind/unbind gadget driver multiple times to different UDCs. A real case I faced recently is SoC with HighSpeed-only and SuperSpeed-only UDCs. SuperSpeed-only UDC can't work on USB 2.0 speeds and vice versa. The system has USB3.0 USB connector with soldered HS lines from mentioned HS-only and SS lines from SS-only UDCs. Each UDC has it's own device driver, so if we want to use both of them with one gadget driver, we must be able to bind/unbind it multiple times to different UDCs. Another one usecase is users who can unload udc drivers without carrying about gadget drivers. Third usecase is, for example, developers who can switch between dummy_hcd and real UDC hardware without unloading gadget driver. Just a stupid question. Why don't you use configfs composite gadget instead of legacy gadgets? In my opinion all things which you have described are working out-of-box when you use configfs interface. It's mostly ready so you may create
Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
On Sun, Feb 08, 2015 at 09:04:32PM +0200, Ruslan Bilovol wrote:
Hi Alan,
On Thu, Jan 29, 2015 at 5:56 PM, Alan Stern st...@rowland.harvard.edu wrote:
On Thu, 29 Jan 2015, Ruslan Bilovol wrote:
Change behavior during registration of gadgets and
gadget drivers in udc-core. Instead of previous
approach when for successful probe of usb gadget driver
at least one usb gadget should be already registered
use another one where gadget drivers and gadgets
can be registered in udc-core independently.
Independent registration of gadgets and gadget drivers
is useful for built-in into kernel gadget and gadget
driver case - because it's possible that gadget is
really probed only on late_init stage (due to deferred
probe) whereas gadget driver's probe is silently failed
on module_init stage due to no any UDC added.
Also it is useful for modules case - now there is no
difference what module to insert first: gadget module
or gadget driver one.
It's possible to do all this much more simply. In fact, I posted a
patch some time ago to do exactly this (but I can't find a copy of it
anywhere).
Unfortunately I didn't find your patch.
Signed-off-by: Ruslan Bilovol ruslan.bilo...@gmail.com
---
drivers/usb/gadget/udc/udc-core.c | 113
+++---
1 file changed, 105 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index e31d574..4c9412b 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -43,13 +43,23 @@ struct usb_udc {
struct usb_gadget_driver*driver;
struct usb_gadget *gadget;
struct device dev;
+ boolbind_by_name;
+ struct list_headlist;
+};
+
+struct pending_gadget_driver {
+ struct usb_gadget_driver*driver;
+ char*udc_name;
struct list_headlist;
};
You don't need all this stuff. What's the point of keeping track of
names? If there are any unbound gadget drivers pending, a newly
registered UDC should bind to the first one available.
It's because gadget driver may be bound to usb_gadget in two ways:
- standard way - in this case any available udc will be picked up
- by name of udc, in this case only matching udc will be picked up
Main feature of my path is not only deferred binding of gadget driver,
but also possibility to register/unregister udc at any time.
This is useful for user who can load, for example, udc module
if needed and unload it safely, not touching gadget driver.
Another example is USB device controllers that consist of pair of
HS+SS controllers, each one having its own udc driver. In this case
it's possible to switch SS/HS by registering/unregistering corresponding
udc and not touching gadget driver.
I did all of this inside of udc-core because it looks like to be best place
for
udc - gadget driver housekeeping. Also it is verified on lot of combinations
of udc and gadget drivers that can be built-in or build as modules
In fact, both I and Robert Baldyga posted patches to try fix this
problem.
http://marc.info/?l=linux-usbm=139287784610046w=2
http://lwn.net/Articles/601839/
I tried to use Robert's solution (fix some bugs) in internal tree, but
the mass storage gadget still has problems to work if unload udc first.
The possible reason should be: it has two places to call
usb_composite_unregister.
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
On Sun, 8 Feb 2015, Ruslan Bilovol wrote: Hi Alan, On Thu, Jan 29, 2015 at 5:56 PM, Alan Stern st...@rowland.harvard.edu wrote: On Thu, 29 Jan 2015, Ruslan Bilovol wrote: Change behavior during registration of gadgets and gadget drivers in udc-core. Instead of previous approach when for successful probe of usb gadget driver at least one usb gadget should be already registered use another one where gadget drivers and gadgets can be registered in udc-core independently. Independent registration of gadgets and gadget drivers is useful for built-in into kernel gadget and gadget driver case - because it's possible that gadget is really probed only on late_init stage (due to deferred probe) whereas gadget driver's probe is silently failed on module_init stage due to no any UDC added. Also it is useful for modules case - now there is no difference what module to insert first: gadget module or gadget driver one. It's possible to do all this much more simply. In fact, I posted a patch some time ago to do exactly this (but I can't find a copy of it anywhere). Unfortunately I didn't find your patch. You don't need all this stuff. What's the point of keeping track of names? If there are any unbound gadget drivers pending, a newly registered UDC should bind to the first one available. It's because gadget driver may be bound to usb_gadget in two ways: - standard way - in this case any available udc will be picked up - by name of udc, in this case only matching udc will be picked up Where did this by name feature come from? You did not mention it in the patch description. Why bother matching by name? Why not simply take the first available UDC? Main feature of my path is not only deferred binding of gadget driver, but also possibility to register/unregister udc at any time. This is useful for user who can load, for example, udc module if needed and unload it safely, not touching gadget driver. We can already do that with the existing code. There's no need for a patch. Also, it's not clear that the existing gadget drivers will work properly if they are unbound from one UDC and then bound again to another one. They were not written with that sort of thing in mind. On Mon, 9 Feb 2015, Peter Chen wrote: In fact, both I and Robert Baldyga posted patches to try fix this problem. http://marc.info/?l=linux-usbm=139287784610046w=2 http://lwn.net/Articles/601839/ That's right. The patch I wrote was a lot like Robert's patch (the marc.info URL above). His approach can be simplified a little; the attached field isn't needed if the driver_list holds only unbound gadget drivers. I tried to use Robert's solution (fix some bugs) in internal tree, but the mass storage gadget still has problems to work if unload udc first. The possible reason should be: it has two places to call usb_composite_unregister. The mass-storage gadget driver can be fixed, if necessary. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
On Mon, 9 Feb 2015, Krzysztof Opasiak wrote: Why bother matching by name? Why not simply take the first available UDC? Because you may have more than one udc. This would allow to pick one by name just like using configfs interface. Clearly it would be more flexible to allow the user to do the matching, the way configfs does (sysfs too). Main feature of my path is not only deferred binding of gadget driver, but also possibility to register/unregister udc at any time. This is useful for user who can load, for example, udc module if needed and unload it safely, not touching gadget driver. We can already do that with the existing code. There's no need for a patch. Also, it's not clear that the existing gadget drivers will work properly if they are unbound from one UDC and then bound again to another one. They were not written with that sort of thing in mind. What you have described is one of basics configfs features. You should be able to bind and unbind your gadget whenever you want and it should work properly after doing: ## create gadget $ echo udc.0 UDC $ echo UDC $ echo udc.1 UDC Function shouldn't care which udc it has been bound previously. Only current one is important and on each unbind each function should cleanup its state and prepare to be bound to another udc. Configfs interface doesn't prohibit this and I haven't seen any info about such restriction. It's not prohibited, but it also was never required. Therefore it may not be implemented in all gadget drivers. If some function is not working in such situation there is a bug in that function and it should be fixed. That's fine. I wasn't pointing out a fundamental limitation, just a fact that will have to be taken into account. Anyway, instead of going through all this, why not do what I suggested earlier (see http://marc.info/?l=linux-usbm=139888691230119w=2) and create a gadget bus type? That would give userspace explicit control over which gadget driver was bound to which UDC. Or maybe that's not a very good fit with the existing code, since most gadget drivers assume they will be bound to only one UDC at a time. So instead, why not create a sysfs interface that allows userspace to control which gadget drivers are bound to which UDCs? As I recall, the original problem people were complaining about was deferred probing. They didn't need fancy matching; all they wanted was the ability to bind a gadget driver to a UDC some time after the gadget driver was loaded and initialized. Something simple like Robert Baldyga's patch is enough to do that. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Hi, (... snip ...) You don't need all this stuff. What's the point of keeping track of names? If there are any unbound gadget drivers pending, a newly registered UDC should bind to the first one available. It's because gadget driver may be bound to usb_gadget in two ways: - standard way - in this case any available udc will be picked up - by name of udc, in this case only matching udc will be picked up Where did this by name feature come from? You did not mention it in the patch description. Why bother matching by name? Why not simply take the first available UDC? Because you may have more than one udc. This would allow to pick one by name just like using configfs interface. Main feature of my path is not only deferred binding of gadget driver, but also possibility to register/unregister udc at any time. This is useful for user who can load, for example, udc module if needed and unload it safely, not touching gadget driver. We can already do that with the existing code. There's no need for a patch. Also, it's not clear that the existing gadget drivers will work properly if they are unbound from one UDC and then bound again to another one. They were not written with that sort of thing in mind. What you have described is one of basics configfs features. You should be able to bind and unbind your gadget whenever you want and it should work properly after doing: ## create gadget $ echo udc.0 UDC $ echo UDC $ echo udc.1 UDC Function shouldn't care which udc it has been bound previously. Only current one is important and on each unbind each function should cleanup its state and prepare to be bound to another udc. Configfs interface doesn't prohibit this and I haven't seen any info about such restriction. If some function is not working in such situation there is a bug in that function and it should be fixed. I have tried to test this on my odroid with dwc2 and dummy_hcd. Most of functions seems to be working but for example ecm isn't. After some digging with Robert we found that it's always reusing endpoints received in first bind(). Once again in my opinion it's a bug which should be fixed and not treated as general assumption. -- Krzysztof Opasiak Samsung RD Institute Poland Samsung Electronics k.opas...@samsung.com -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
-Original Message- From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb- ow...@vger.kernel.org] On Behalf Of Krzysztof Opasiak Sent: Monday, February 09, 2015 7:06 PM To: 'Alan Stern'; 'Ruslan Bilovol'; 'Peter Chen' Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; 'Balbi, Felipe'; gre...@linuxfoundation.org; Andrzej Pietrasiewicz Subject: RE: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers Hi, (... snip ...) You don't need all this stuff. What's the point of keeping track of names? If there are any unbound gadget drivers pending, a newly registered UDC should bind to the first one available. It's because gadget driver may be bound to usb_gadget in two ways: - standard way - in this case any available udc will be picked up - by name of udc, in this case only matching udc will be picked up Where did this by name feature come from? You did not mention it in the patch description. Why bother matching by name? Why not simply take the first available UDC? Because you may have more than one udc. This would allow to pick one by name just like using configfs interface. Main feature of my path is not only deferred binding of gadget driver, but also possibility to register/unregister udc at any time. This is useful for user who can load, for example, udc module if needed and unload it safely, not touching gadget driver. We can already do that with the existing code. There's no need for a patch. Also, it's not clear that the existing gadget drivers will work properly if they are unbound from one UDC and then bound again to another one. They were not written with that sort of thing in mind. What you have described is one of basics configfs features. You should be able to bind and unbind your gadget whenever you want and it should work properly after doing: ## create gadget $ echo udc.0 UDC $ echo UDC $ echo udc.1 UDC Function shouldn't care which udc it has been bound previously. Only current one is important and on each unbind each function should cleanup its state and prepare to be bound to another udc. Configfs interface doesn't prohibit this and I haven't seen any info about such restriction. If some function is not working in such situation there is a bug in that function and it should be fixed. I have tried to test this on my odroid with dwc2 and dummy_hcd. Most of functions seems to be working but for example ecm isn't. ^ above is ok After some digging with Robert we found that it's always reusing endpoints received in first bind(). Fixup: That's bullshit ignore it please. ecm_opts-bound is not used to take endpoints but only to register net device. Went too far after short reading. All in all, ecm is not working when binding form one udc to another. Don't know exact reason, but in my opinion it's more a bug than common assumption. -- Krzysztof Opasiak Samsung RD Institute Poland Samsung Electronics k.opas...@samsung.com -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Hi guys, On Mon, Feb 9, 2015 at 10:00 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 9 Feb 2015, Krzysztof Opasiak wrote: Why bother matching by name? Why not simply take the first available UDC? Because you may have more than one udc. This would allow to pick one by name just like using configfs interface. Clearly it would be more flexible to allow the user to do the matching, the way configfs does (sysfs too). Main feature of my path is not only deferred binding of gadget driver, but also possibility to register/unregister udc at any time. This is useful for user who can load, for example, udc module if needed and unload it safely, not touching gadget driver. We can already do that with the existing code. There's no need for a patch. Also, it's not clear that the existing gadget drivers will work properly if they are unbound from one UDC and then bound again to another one. They were not written with that sort of thing in mind. What you have described is one of basics configfs features. You should be able to bind and unbind your gadget whenever you want and it should work properly after doing: ## create gadget $ echo udc.0 UDC $ echo UDC $ echo udc.1 UDC Function shouldn't care which udc it has been bound previously. Only current one is important and on each unbind each function should cleanup its state and prepare to be bound to another udc. Configfs interface doesn't prohibit this and I haven't seen any info about such restriction. Thank you Krzysztof for this explanation that makes things more clear for reviewers. It's not prohibited, but it also was never required. Therefore it may not be implemented in all gadget drivers. If some function is not working in such situation there is a bug in that function and it should be fixed. That's fine. I wasn't pointing out a fundamental limitation, just a fact that will have to be taken into account. I also don't see any restrictions to bind/unbind gadget driver few times and in case of such bug we just can fix it. I didn't see any issue with gadget drivers that I used for verification, however, to be honest, I didn't test it with all possible ones. Anyway, it should work in legacy way (one gadget driver is bound to one uds) so current behavior at least is not broken. Anyway, instead of going through all this, why not do what I suggested earlier (see http://marc.info/?l=linux-usbm=139888691230119w=2) and create a gadget bus type? That would give userspace explicit control over which gadget driver was bound to which UDC. Exactly, my patch transforms udc-core to something like tiny bus with very basic functionality. But in mentioned mailthread I see good ideas that is possible to implement with small overhead. Or maybe that's not a very good fit with the existing code, since most gadget drivers assume they will be bound to only one UDC at a time. So instead, why not create a sysfs interface that allows userspace to control which gadget drivers are bound to which UDCs? As I recall, the original problem people were complaining about was deferred probing. They didn't need fancy matching; all they wanted was the ability to bind a gadget driver to a UDC some time after the gadget driver was loaded and initialized. Something simple like Robert Baldyga's patch is enough to do that. This simplicity is also a limitation. As I mentioned before, sometimes it is needed to be able to bind/unbind gadget driver multiple times to different UDCs. A real case I faced recently is SoC with HighSpeed-only and SuperSpeed-only UDCs. SuperSpeed-only UDC can't work on USB 2.0 speeds and vice versa. The system has USB3.0 USB connector with soldered HS lines from mentioned HS-only and SS lines from SS-only UDCs. Each UDC has it's own device driver, so if we want to use both of them with one gadget driver, we must be able to bind/unbind it multiple times to different UDCs. Another one usecase is users who can unload udc drivers without carrying about gadget drivers. Third usecase is, for example, developers who can switch between dummy_hcd and real UDC hardware without unloading gadget driver. I'll work on improved version and will send new patch soon... Best regards, Ruslan Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Hi Alan,
On Thu, Jan 29, 2015 at 5:56 PM, Alan Stern st...@rowland.harvard.edu wrote:
On Thu, 29 Jan 2015, Ruslan Bilovol wrote:
Change behavior during registration of gadgets and
gadget drivers in udc-core. Instead of previous
approach when for successful probe of usb gadget driver
at least one usb gadget should be already registered
use another one where gadget drivers and gadgets
can be registered in udc-core independently.
Independent registration of gadgets and gadget drivers
is useful for built-in into kernel gadget and gadget
driver case - because it's possible that gadget is
really probed only on late_init stage (due to deferred
probe) whereas gadget driver's probe is silently failed
on module_init stage due to no any UDC added.
Also it is useful for modules case - now there is no
difference what module to insert first: gadget module
or gadget driver one.
It's possible to do all this much more simply. In fact, I posted a
patch some time ago to do exactly this (but I can't find a copy of it
anywhere).
Unfortunately I didn't find your patch.
Signed-off-by: Ruslan Bilovol ruslan.bilo...@gmail.com
---
drivers/usb/gadget/udc/udc-core.c | 113
+++---
1 file changed, 105 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index e31d574..4c9412b 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -43,13 +43,23 @@ struct usb_udc {
struct usb_gadget_driver*driver;
struct usb_gadget *gadget;
struct device dev;
+ boolbind_by_name;
+ struct list_headlist;
+};
+
+struct pending_gadget_driver {
+ struct usb_gadget_driver*driver;
+ char*udc_name;
struct list_headlist;
};
You don't need all this stuff. What's the point of keeping track of
names? If there are any unbound gadget drivers pending, a newly
registered UDC should bind to the first one available.
It's because gadget driver may be bound to usb_gadget in two ways:
- standard way - in this case any available udc will be picked up
- by name of udc, in this case only matching udc will be picked up
Main feature of my path is not only deferred binding of gadget driver,
but also possibility to register/unregister udc at any time.
This is useful for user who can load, for example, udc module
if needed and unload it safely, not touching gadget driver.
Another example is USB device controllers that consist of pair of
HS+SS controllers, each one having its own udc driver. In this case
it's possible to switch SS/HS by registering/unregistering corresponding
udc and not touching gadget driver.
I did all of this inside of udc-core because it looks like to be best place for
udc - gadget driver housekeeping. Also it is verified on lot of combinations
of udc and gadget drivers that can be built-in or build as modules
Best regards,
Ruslan
Just add a pending list_head into the usb_gadget_driver structure and
forget about all the rest. (Or try to find my patch in the mailing
list archives somehow see if you think it needs to be changed.)
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
On Thu, 29 Jan 2015, Ruslan Bilovol wrote:
Change behavior during registration of gadgets and
gadget drivers in udc-core. Instead of previous
approach when for successful probe of usb gadget driver
at least one usb gadget should be already registered
use another one where gadget drivers and gadgets
can be registered in udc-core independently.
Independent registration of gadgets and gadget drivers
is useful for built-in into kernel gadget and gadget
driver case - because it's possible that gadget is
really probed only on late_init stage (due to deferred
probe) whereas gadget driver's probe is silently failed
on module_init stage due to no any UDC added.
Also it is useful for modules case - now there is no
difference what module to insert first: gadget module
or gadget driver one.
It's possible to do all this much more simply. In fact, I posted a
patch some time ago to do exactly this (but I can't find a copy of it
anywhere).
Signed-off-by: Ruslan Bilovol ruslan.bilo...@gmail.com
---
drivers/usb/gadget/udc/udc-core.c | 113
+++---
1 file changed, 105 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index e31d574..4c9412b 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -43,13 +43,23 @@ struct usb_udc {
struct usb_gadget_driver*driver;
struct usb_gadget *gadget;
struct device dev;
+ boolbind_by_name;
+ struct list_headlist;
+};
+
+struct pending_gadget_driver {
+ struct usb_gadget_driver*driver;
+ char*udc_name;
struct list_headlist;
};
You don't need all this stuff. What's the point of keeping track of
names? If there are any unbound gadget drivers pending, a newly
registered UDC should bind to the first one available.
Just add a pending list_head into the usb_gadget_driver structure and
forget about all the rest. (Or try to find my patch in the mailing
list archives somehow see if you think it needs to be changed.)
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] usb: gadget: udc-core: independent registration of gadgets and gadget drivers
Change behavior during registration of gadgets and
gadget drivers in udc-core. Instead of previous
approach when for successful probe of usb gadget driver
at least one usb gadget should be already registered
use another one where gadget drivers and gadgets
can be registered in udc-core independently.
Independent registration of gadgets and gadget drivers
is useful for built-in into kernel gadget and gadget
driver case - because it's possible that gadget is
really probed only on late_init stage (due to deferred
probe) whereas gadget driver's probe is silently failed
on module_init stage due to no any UDC added.
Also it is useful for modules case - now there is no
difference what module to insert first: gadget module
or gadget driver one.
Signed-off-by: Ruslan Bilovol ruslan.bilo...@gmail.com
---
drivers/usb/gadget/udc/udc-core.c | 113 +++---
1 file changed, 105 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index e31d574..4c9412b 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -43,13 +43,23 @@ struct usb_udc {
struct usb_gadget_driver*driver;
struct usb_gadget *gadget;
struct device dev;
+ boolbind_by_name;
+ struct list_headlist;
+};
+
+struct pending_gadget_driver {
+ struct usb_gadget_driver*driver;
+ char*udc_name;
struct list_headlist;
};
static struct class *udc_class;
static LIST_HEAD(udc_list);
+static LIST_HEAD(gadget_driver_pending_list);
static DEFINE_MUTEX(udc_lock);
+static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver
*driver,
+ bool
bind_by_name);
/* - */
#ifdef CONFIG_HAS_DMA
@@ -244,6 +254,7 @@ int usb_add_gadget_udc_release(struct device *parent,
struct usb_gadget *gadget,
{
struct usb_udc *udc;
int ret = -ENOMEM;
+ struct pending_gadget_driver *pending;
udc = kzalloc(sizeof(*udc), GFP_KERNEL);
if (!udc)
@@ -288,6 +299,24 @@ int usb_add_gadget_udc_release(struct device *parent,
struct usb_gadget *gadget,
usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
+ if (!list_empty(gadget_driver_pending_list)) {
+ pending = list_first_entry(gadget_driver_pending_list,
+ struct pending_gadget_driver, list);
+
+ if (pending-udc_name) {
+ if (!strcmp(pending-udc_name, dev_name(udc-dev))) {
+ udc_bind_to_driver(udc, pending-driver, true);
+ list_del(pending-list);
+ kfree(pending-udc_name);
+ kfree(pending);
+ }
+ } else {
+ udc_bind_to_driver(udc, pending-driver, false);
+ list_del(pending-list);
+ kfree(pending);
+ }
+ }
+
mutex_unlock(udc_lock);
return 0;
@@ -364,10 +393,32 @@ found:
dev_vdbg(gadget-dev.parent, unregistering gadget\n);
list_del(udc-list);
- mutex_unlock(udc_lock);
- if (udc-driver)
+ if (udc-driver) {
+ struct pending_gadget_driver *pending;
+
+ pending = kzalloc(sizeof(*pending), GFP_KERNEL);
+ if (!pending)
+ goto err;
+
+ if (udc-bind_by_name) {
+ pending-udc_name = kstrdup(dev_name(udc-dev),
+ GFP_KERNEL);
+ if (!pending-udc_name) {
+ kfree(pending);
+ goto err;
+ }
+ }
+
+ pending-driver = udc-driver;
+ list_add_tail(pending-list, gadget_driver_pending_list);
+
+ pr_info(udc-core: added [%s] to list of pending drivers\n,
+ pending-driver-function);
+err:
usb_gadget_remove_driver(udc);
+ }
+ mutex_unlock(udc_lock);
kobject_uevent(udc-dev.kobj, KOBJ_REMOVE);
flush_work(gadget-work);
@@ -378,7 +429,8 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
/* - */
-static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver
*driver)
+static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver
*driver,
+ bool
bind_by_name)
{
int ret;
@@
