Patchwork [U-Boot] usb_ether: register usb ethernet gadget at each eth init

login
register
mail settings
Submitter Lei Wen
Date Nov. 26, 2010, 3:21 a.m.
Message ID <1290741661-28108-1-git-send-email-leiwen@marvell.com>
Download mbox | patch
Permalink /patch/73153/
State Accepted
Delegated to: Remy Bohmer
Headers show

Comments

Lei Wen - Nov. 26, 2010, 3:21 a.m.
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(-)
Vitaly Kuzmichev - Nov. 26, 2010, 6:23 p.m.
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(&eth_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.
Lei Wen - Nov. 27, 2010, 2:21 p.m.
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(&eth_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
Vitaly Kuzmichev - Nov. 29, 2010, 1:56 p.m.
Hi Lei,

Lei Wen wrote:
> Hi Vitaly,
> [...]
>>> +     if (usb_gadget_register_driver(&eth_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 - Nov. 29, 2010, 2:06 p.m.
Vitaly Kuzmichev wrote:
> Hi Lei,
> 
> Lei Wen wrote:
>> Hi Vitaly,
>> [...]
>>>> +     if (usb_gadget_register_driver(&eth_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.
Lei Wen - Nov. 30, 2010, 2:04 a.m.
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(&eth_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
Vitaly Kuzmichev - Nov. 30, 2010, 3:13 p.m.
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.
Lei Wen - Nov. 30, 2010, 3:55 p.m.
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
Vitaly Kuzmichev - Nov. 30, 2010, 4:41 p.m.
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.
Lei Wen - Dec. 1, 2010, 3:32 p.m.
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

Patch

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(&eth_driver) < 0)
+		goto fail;
 
 	dev->network_started = 0;