| Message ID | 1290741661-28108-1-git-send-email-leiwen@marvell.com |
|---|---|
| State | Accepted |
| Delegated to: | Remy Bohmer |
| Headers | show |
Hi Lei, Lei Wen wrote: > Since the ether may not be the only one usb gadget would be used > in the uboot, it is neccessary to do the register each time the > eth begin to work to make usb gadget driver less confussed when > we want to use two different usb gadget at the same time. > [...] > @@ -1788,6 +1788,8 @@ static int usb_eth_init(struct eth_device *netdev, bd_t *bd) > error("received NULL ptr"); > goto fail; > } > + if (usb_gadget_register_driver(ð_driver) < 0) > + goto fail; You probably need then at least to remove usb_gadget_register_driver call from usb_eth_initialize. And add usb_gadget_unregister_driver in usb_eth_halt. For example, my UDC driver returns EBUSY if another gadget driver tries to register itself. (Yes, I do not want to have more than 1 gadget). With your patch the gadget driver will fail each time when initializing. Anyway if I "fix" my UDC driver some day the Ether gadget will try to register itself twice (and more times later). So I will have to protect it against this bad behavior.
Hi Vitaly, On Sat, Nov 27, 2010 at 2:23 AM, Vitaly Kuzmichev <vkuzmichev@mvista.com> wrote: > Hi Lei, > > Lei Wen wrote: >> Since the ether may not be the only one usb gadget would be used >> in the uboot, it is neccessary to do the register each time the >> eth begin to work to make usb gadget driver less confussed when >> we want to use two different usb gadget at the same time. >> [...] >> @@ -1788,6 +1788,8 @@ static int usb_eth_init(struct eth_device *netdev, bd_t *bd) >> error("received NULL ptr"); >> goto fail; >> } >> + if (usb_gadget_register_driver(ð_driver) < 0) >> + goto fail; > > You probably need then at least to remove usb_gadget_register_driver > call from usb_eth_initialize. And add usb_gadget_unregister_driver in > usb_eth_halt. I am afraid that remove usb_gadget_register_driver call from usb_eth_initialize is not possible. For tftp as example, it would call eth_halt before the eth_init. If we do the remove behavior, the gadget still not valid yet in the first eth_halt call which would cause the uboot becomes halt. > For example, my UDC driver returns EBUSY if another gadget driver tries > to register itself. (Yes, I do not want to have more than 1 gadget). > With your patch the gadget driver will fail each time when initializing. > Anyway if I "fix" my UDC driver some day the Ether gadget will try to > register itself twice (and more times later). So I will have to protect > it against this bad behavior. Two gadget example may comes from what I am using now. I am current attempting to make the tftp and fastboot cowork togetther. Since fastboot is a cool tool, I think people may want it beside they still use the tftp. Would that be enough convincing?:) I think the gadget driver should adopt like this to allow two gadget coexist. And from gadget driver's review, the switch from one gadget to another is easy, just register it to another should be enough. Does it really need to return EBUSY? Best regards, Lei
Hi Lei, Lei Wen wrote: > Hi Vitaly, > [...] >>> + if (usb_gadget_register_driver(ð_driver) < 0) >>> + goto fail; >> >> You probably need then at least to remove usb_gadget_register_driver >> call from usb_eth_initialize. And add usb_gadget_unregister_driver in >> usb_eth_halt. > > I am afraid that remove usb_gadget_register_driver call from usb_eth_initialize > is not possible. For tftp as example, it would call eth_halt before > the eth_init. > If we do the remove behavior, the gadget still not valid yet in the > first eth_halt call > which would cause the uboot becomes halt. The gadget driver handles this properly. Right now it just does usb_gadget_disconnect which has no effect when no gadget was requested from the UDC driver yet. And additional calling of usb_gadget_unregister_driver will at most return an error. >> For example, my UDC driver returns EBUSY if another gadget driver tries >> to register itself. (Yes, I do not want to have more than 1 gadget). >> With your patch the gadget driver will fail each time when initializing. >> Anyway if I "fix" my UDC driver some day the Ether gadget will try to >> register itself twice (and more times later). So I will have to protect >> it against this bad behavior. > > Two gadget example may comes from what I am using now. > I am current attempting to make the tftp and fastboot cowork togetther. > Since fastboot is a cool tool, I think people may want it beside they > still use the tftp. > Would that be enough convincing?:) > > I think the gadget driver should adopt like this to allow two gadget coexist. > And from gadget driver's review, the switch from one gadget to another > is easy, just > register it to another should be enough. Does it really need to return EBUSY? If the UDC (not a gadget) driver does not support more than 1 gadget driver in the same time it must return an error (EBUSY is fine for now) when another gadget tries to register itself. The reason why does not it support 2 (and more) gadgets registered simultaneously is another question. Even if I fix this, your patch is not needed because the UDC driver will give USB requests to the appropriate gadget driver properly. I really do not understand what a problem with using 2 gadgets in the same time without your patch if the UDC driver supports this. Especially I do not understand... > it is neccessary to do the register each time the > eth begin to work to make usb gadget driver less confussed when > we want to use two different usb gadget at the same time. ...how can the gadget (or UDC?) driver be confused here? Right now I can tell that it will be more confused rather than less. I just can suppose that you are trying to implement something like switching active gadget driver in UDC driver which has only one variable to store info about gadget driver (rather than having an array at least). In this case you need to fix UDC driver rather than hacking all available gadget drivers. Also this way will not work when someone needs two gadgets active simultaneously, e.g. USB ether with USB serial.
Vitaly Kuzmichev wrote: > Hi Lei, > > Lei Wen wrote: >> Hi Vitaly, >> [...] >>>> + if (usb_gadget_register_driver(ð_driver) < 0) >>>> + goto fail; >>> >>> You probably need then at least to remove usb_gadget_register_driver >>> call from usb_eth_initialize. And add usb_gadget_unregister_driver in >>> usb_eth_halt. >> >> I am afraid that remove usb_gadget_register_driver call from usb_eth_initialize >> is not possible. For tftp as example, it would call eth_halt before >> the eth_init. >> If we do the remove behavior, the gadget still not valid yet in the >> first eth_halt call >> which would cause the uboot becomes halt. > > The gadget driver handles this properly. Right now it just does > usb_gadget_disconnect which has no effect when no gadget was requested > from the UDC driver yet. I was wrong... usb_gadget_disconnect does not care about NULL pointer passed. :/ Best regards, Vitaly.
Hi Vitaly, On Mon, Nov 29, 2010 at 9:56 PM, Vitaly Kuzmichev <vkuzmichev@mvista.com> wrote: > Hi Lei, > > Lei Wen wrote: >> Hi Vitaly, >> [...] >>>> + if (usb_gadget_register_driver(ð_driver) < 0) >>>> + goto fail; >>> >>> You probably need then at least to remove usb_gadget_register_driver >>> call from usb_eth_initialize. And add usb_gadget_unregister_driver in >>> usb_eth_halt. >> >> I am afraid that remove usb_gadget_register_driver call from usb_eth_initialize >> is not possible. For tftp as example, it would call eth_halt before >> the eth_init. >> If we do the remove behavior, the gadget still not valid yet in the >> first eth_halt call >> which would cause the uboot becomes halt. > > The gadget driver handles this properly. Right now it just does > usb_gadget_disconnect which has no effect when no gadget was requested > from the UDC driver yet. > And additional calling of usb_gadget_unregister_driver will at most > return an error. > For current ether.c state, there is no usb_gadget_unregister_driver in it. Even it has, we still need usb_gadget_register_driver call in each eth_init(). >>> For example, my UDC driver returns EBUSY if another gadget driver tries >>> to register itself. (Yes, I do not want to have more than 1 gadget). >>> With your patch the gadget driver will fail each time when initializing. >>> Anyway if I "fix" my UDC driver some day the Ether gadget will try to >>> register itself twice (and more times later). So I will have to protect >>> it against this bad behavior. >> >> Two gadget example may comes from what I am using now. >> I am current attempting to make the tftp and fastboot cowork togetther. >> Since fastboot is a cool tool, I think people may want it beside they >> still use the tftp. >> Would that be enough convincing?:) >> >> I think the gadget driver should adopt like this to allow two gadget coexist. >> And from gadget driver's review, the switch from one gadget to another >> is easy, just >> register it to another should be enough. Does it really need to return EBUSY? > > If the UDC (not a gadget) driver does not support more than 1 gadget > driver in the same time it must return an error (EBUSY is fine for now) > when another gadget tries to register itself. Right, if at the same time certainly need to return error. > The reason why does not it support 2 (and more) gadgets registered > simultaneously is another question. Even if I fix this, your patch is > not needed because the UDC driver will give USB requests to the > appropriate gadget driver properly. > > I really do not understand what a problem with using 2 gadgets in the > same time without your patch if the UDC driver supports this. > Especially I do not understand... > Sorry, maybe I make a confused saying here. I don't mean to support two gadget at the "same time", but to let one gadget could work after another gadget is finished. Like after tftp, I could use fastboot, or after return from fastboot, I could still use the tftp. >> it is neccessary to do the register each time the >> eth begin to work to make usb gadget driver less confussed when >> we want to use two different usb gadget at the same time. > > ...how can the gadget (or UDC?) driver be confused here? > Right now I can tell that it will be more confused rather than less. > I just can suppose that you are trying to implement something like > switching active gadget driver in UDC driver which has only one variable > to store info about gadget driver (rather than having an array at > least). In this case you need to fix UDC driver rather than hacking all > available gadget drivers. > Also this way will not work when someone needs two gadgets active > simultaneously, e.g. USB ether with USB serial. > The confused thing is for the ether.c would register its gadget at the usb_eth_initialize() call, which is a very begining of the uboot. If we use some other gadget, like fastboot, it would take place of gadget registered in the gadget driver. Then if we return to the USB ether, for there is no other chance to register itself, the ether gadget would use other gadget setting to make the ethernet connection. That is where the confussion start. Best regards, Lei
Hi Lei, Lei Wen wrote: > Hi Vitaly, > [...] >> And additional calling of usb_gadget_unregister_driver will at most >> return an error. >> > > For current ether.c state, there is no usb_gadget_unregister_driver in > it. Even it has, we still need > usb_gadget_register_driver call in each eth_init(). Yes, if we do 'unregister' we should do 'register' somewhere before. If we do 'register' we should do 'unregister' somewhere after. This is the symmetry such like in dynamic data allocation (like malloc/free), and we should comply it. The reason why there is no 'usb_gadget_unregister_driver' in the Ether driver yet is that there is no symmetrical function for 'usb_eth_initialize' because there is no way to remove a network device from the U-Boot environment. > [...] >>> I think the gadget driver should adopt like this to allow two gadget coexist. >>> And from gadget driver's review, the switch from one gadget to another >>> is easy, just >>> register it to another should be enough. Does it really need to return EBUSY? >> >> If the UDC (not a gadget) driver does not support more than 1 gadget >> driver in the same time it must return an error (EBUSY is fine for now) >> when another gadget tries to register itself. > Right, if at the same time certainly need to return error. So your code that expects successful register will make Ether gadget broken... > [...] >> I really do not understand what a problem with using 2 gadgets in the >> same time without your patch if the UDC driver supports this. >> Especially I do not understand... >> > Sorry, maybe I make a confused saying here. I don't mean to support two gadget > at the "same time", but to let one gadget could work after another > gadget is finished. > Like after tftp, I could use fastboot, or after return from fastboot, > I could still use the tftp. ...But if you add usb_gadget_unregister_driver in eth_halt (and additional checking that dev->gadget is not equal to NULL) all existing UDC drivers (mine and Remy's at91_udc) will be working fine. And your fastboot will be working too. > [...] >> ...how can the gadget (or UDC?) driver be confused here? > [...] >> > The confused thing is for the ether.c would register its gadget at the > usb_eth_initialize() call, > which is a very begining of the uboot. If we use some other gadget, > like fastboot, it would take > place of gadget registered in the gadget driver. You probably meant "in the UDC driver"? Note that USB gadget stack consists of 2 sorts of drivers: gadget and controller (UDC, USB device controller). The gadget driver provides hardware independent protocol for talking with host machine. The controller driver interacts with hardware - USB device chip. The UDC driver allocates 'struct gadget' (passed in 'usb_gadget_driver.bind' callback) for each gadget driver which tries to register itself. However, all existing UDC drivers (both in Linux and U-Boot) are not able to handle more than 1 gadget driver in the same time. And now I guess that this impossible. So they all return EBUSY if another gadget tries to register itself. This means that if you want 2 gadgets you need to register each one right before transferring data and unregister right after the data was transferred. By doing gadget unregister you will free allocated resources (such as USB endpoints and USB requests) in UDC and Ether drivers properly. Otherwise you will have memory leaks.
Hi Vitaly, On Tue, Nov 30, 2010 at 11:13 PM, Vitaly Kuzmichev <vkuzmichev@mvista.com> wrote: > Hi Lei, > > Lei Wen wrote: >> Hi Vitaly, >> [...] >>> And additional calling of usb_gadget_unregister_driver will at most >>> return an error. >>> >> >> For current ether.c state, there is no usb_gadget_unregister_driver in >> it. Even it has, we still need >> usb_gadget_register_driver call in each eth_init(). > > Yes, if we do 'unregister' we should do 'register' somewhere before. If > we do 'register' we should do 'unregister' somewhere after. > This is the symmetry such like in dynamic data allocation (like > malloc/free), and we should comply it. > The reason why there is no 'usb_gadget_unregister_driver' in the Ether > driver yet is that there is no symmetrical function for > 'usb_eth_initialize' because there is no way to remove a network device > from the U-Boot environment. > What we need to do is not trying to remove the network device, but to register another gadget when we want to switch from tftp to fastboot, and make UDC take a different protocol to communicate with host. > >> [...] >>>> I think the gadget driver should adopt like this to allow two gadget coexist. >>>> And from gadget driver's review, the switch from one gadget to another >>>> is easy, just >>>> register it to another should be enough. Does it really need to return EBUSY? >>> >>> If the UDC (not a gadget) driver does not support more than 1 gadget >>> driver in the same time it must return an error (EBUSY is fine for now) >>> when another gadget tries to register itself. >> Right, if at the same time certainly need to return error. > > So your code that expects successful register will make Ether gadget > broken... Don't understand here, why the Ether gadget would be broken? > > >> [...] >>> I really do not understand what a problem with using 2 gadgets in the >>> same time without your patch if the UDC driver supports this. >>> Especially I do not understand... >>> >> Sorry, maybe I make a confused saying here. I don't mean to support two gadget >> at the "same time", but to let one gadget could work after another >> gadget is finished. >> Like after tftp, I could use fastboot, or after return from fastboot, >> I could still use the tftp. > > ...But if you add usb_gadget_unregister_driver in eth_halt (and > additional checking that dev->gadget is not equal to NULL) all existing > UDC drivers (mine and Remy's at91_udc) will be working fine. And your > fastboot will be working too. Yep. > >> [...] >>> ...how can the gadget (or UDC?) driver be confused here? >> [...] >>> >> The confused thing is for the ether.c would register its gadget at the >> usb_eth_initialize() call, >> which is a very begining of the uboot. If we use some other gadget, >> like fastboot, it would take >> place of gadget registered in the gadget driver. > > You probably meant "in the UDC driver"? > Note that USB gadget stack consists of 2 sorts of drivers: gadget and > controller (UDC, USB device controller). The gadget driver provides > hardware independent protocol for talking with host machine. The > controller driver interacts with hardware - USB device chip. The UDC > driver allocates 'struct gadget' (passed in 'usb_gadget_driver.bind' > callback) for each gadget driver which tries to register itself. > > However, all existing UDC drivers (both in Linux and U-Boot) are not > able to handle more than 1 gadget driver in the same time. And now I > guess that this impossible. So they all return EBUSY if another gadget > tries to register itself. I agree with you, and my intent is not support two gadget at the same time, but let two gadget would work one by one, that is after one stop its work, the other could work fines. > > This means that if you want 2 gadgets you need to register each one > right before transferring data and unregister right after the data was > transferred. By doing gadget unregister you will free allocated > resources (such as USB endpoints and USB requests) in UDC and Ether > drivers properly. Otherwise you will have memory leaks. Sure, so we comes into a conclusion that add register call in the eth_init and unregister call in eth_halt? In unregister call, the UDC driver should free the ep as you said. Best regards, Lei
Hi Lei, Lei Wen wrote: > [...] >>> For current ether.c state, there is no usb_gadget_unregister_driver in >>> it. Even it has, we still need >>> usb_gadget_register_driver call in each eth_init(). >> >> Yes, if we do 'unregister' we should do 'register' somewhere before. If >> we do 'register' we should do 'unregister' somewhere after. >> This is the symmetry such like in dynamic data allocation (like >> malloc/free), and we should comply it. >> The reason why there is no 'usb_gadget_unregister_driver' in the Ether >> driver yet is that there is no symmetrical function for >> 'usb_eth_initialize' because there is no way to remove a network device >> from the U-Boot environment. >> > What we need to do is not trying to remove the network device, but to register > another gadget when we want to switch from tftp to fastboot, and make UDC take > a different protocol to communicate with host. I have not asked you to remove network device, I have talked about the reason for the following: > For current ether.c state, there is no usb_gadget_unregister_driver in > it. > [...] >>> Right, if at the same time certainly need to return error. >> >> So your code that expects successful register will make Ether gadget >> broken... > > Don't understand here, why the Ether gadget would be broken? Because UDC driver does not allow to register more than 1 gadget driver in the same time. It just returns EBUSY. Even if the first gadget driver is idle (when Ether gadget driver is registered but tftp command is not issued). This is done to protect against memory leaks. Just checkout cdc-at91 branch in u-boot-usb.git for example of UDC driver (drivers/usb/gadget/at91_udc.c). Or look at any UDC driver in linux kernel. > [...] >> This means that if you want 2 gadgets you need to register each one >> right before transferring data and unregister right after the data was >> transferred. By doing gadget unregister you will free allocated >> resources (such as USB endpoints and USB requests) in UDC and Ether >> drivers properly. Otherwise you will have memory leaks. > > Sure, so we comes into a conclusion that add register call in the > eth_init and unregister call > in eth_halt? In unregister call, the UDC driver should free the ep as you said. Yes, it will free by 'eth_unbind' called from 'usb_gadget_unregister_driver'. Note that 'usb_gadget_register_driver' should be removed from 'usb_eth_initialize' and dev->gadget checking should be added in eth_halt. > > Best regards, > Lei Best regards, Vitaly.
Hi Vitaly, On Wed, Dec 1, 2010 at 12:41 AM, Vitaly Kuzmichev <vkuzmichev@mvista.com> wrote: > Hi Lei, > > Lei Wen wrote: >> [...] >>>> For current ether.c state, there is no usb_gadget_unregister_driver in >>>> it. Even it has, we still need >>>> usb_gadget_register_driver call in each eth_init(). >>> >>> Yes, if we do 'unregister' we should do 'register' somewhere before. If >>> we do 'register' we should do 'unregister' somewhere after. >>> This is the symmetry such like in dynamic data allocation (like >>> malloc/free), and we should comply it. >>> The reason why there is no 'usb_gadget_unregister_driver' in the Ether >>> driver yet is that there is no symmetrical function for >>> 'usb_eth_initialize' because there is no way to remove a network device >>> from the U-Boot environment. >>> >> What we need to do is not trying to remove the network device, but to register >> another gadget when we want to switch from tftp to fastboot, and make UDC take >> a different protocol to communicate with host. > > I have not asked you to remove network device, I have talked about the > reason for the following: >> For current ether.c state, there is no usb_gadget_unregister_driver in >> it. > >> [...] >>>> Right, if at the same time certainly need to return error. >>> >>> So your code that expects successful register will make Ether gadget >>> broken... >> >> Don't understand here, why the Ether gadget would be broken? > > Because UDC driver does not allow to register more than 1 gadget driver > in the same time. It just returns EBUSY. Even if the first gadget driver > is idle (when Ether gadget driver is registered but tftp command is not > issued). This is done to protect against memory leaks. > Just checkout cdc-at91 branch in u-boot-usb.git for example of UDC > driver (drivers/usb/gadget/at91_udc.c). Or look at any UDC driver in > linux kernel. > I add unregister function in eth_halt, then the udc driver could do the proper memory free to prevent the memory leak. > >> [...] >>> This means that if you want 2 gadgets you need to register each one >>> right before transferring data and unregister right after the data was >>> transferred. By doing gadget unregister you will free allocated >>> resources (such as USB endpoints and USB requests) in UDC and Ether >>> drivers properly. Otherwise you will have memory leaks. >> >> Sure, so we comes into a conclusion that add register call in the >> eth_init and unregister call >> in eth_halt? In unregister call, the UDC driver should free the ep as you said. > > Yes, it will free by 'eth_unbind' called from > 'usb_gadget_unregister_driver'. > > Note that 'usb_gadget_register_driver' should be removed from > 'usb_eth_initialize' and dev->gadget checking should be added in eth_halt. > Ack this idea. Apply to the next patch. Best regards, Lei
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index 5a18e03..d57b2ad 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -1788,6 +1788,8 @@ static int usb_eth_init(struct eth_device *netdev, bd_t *bd) error("received NULL ptr"); goto fail; } + if (usb_gadget_register_driver(ð_driver) < 0) + goto fail; dev->network_started = 0;
Since the ether may not be the only one usb gadget would be used in the uboot, it is neccessary to do the register each time the eth begin to work to make usb gadget driver less confussed when we want to use two different usb gadget at the same time. Usb gadget driver could simple ignore the register operation, if it find the driver has been registered already. Signed-off-by: Lei Wen <leiwen@marvell.com> --- drivers/usb/gadget/ether.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)