[U-Boot] Net driver questions

[U-Boot] Net driver questions

[Krzysztof Halasa]

Hello,

Please forgive me if answers to my questions are obvious, I'm new to
U-Boot.

I'm trying to port the Ethernet driver for Intel IXP4xx CPU from Linux
(I know there is Intel's code already ported). I've read the
doc/README.drivers.eth. I came across some problems:
- dev->halt() seems to be called before the first call to
  dev->init() (i.e., before the hardware is initialized). Is it on
  purpose?

- dev->recv() seems to be called recursively, for example while doing
  "dhcp" or "bootp" (ping is ok). dev->recv() in my driver calls
  NetReceive(), which in turn (without returning to the caller, i.e.,
  dev->recv(), first) reinitializes the driver on error (calls
  dev->halt() and dev->init()). This makes a lot of mess in the driver,
  should it stay this way? Perhaps we should queue the received packets
  and process them on return from dev->recv()? Or maybe return all those
  packets together?

- dev->recv() is provided with RX packet buffers. IXP4xx can only
  receive to already allocated memory so the driver has to provide it's
  own buffers prior to dev->recv(). I assume it's common situation with
  all hardware recent enough. Does the driver have to copy data to
  NetRxPackets[], or is it ok to simply call NetReceive using driver's
  buffers?
-- 
Krzysztof Halasa

[U-Boot] Net driver questions

[Mike Frysinger]

On Sunday 01 November 2009 18:07:43 Krzysztof Halasa wrote:
> I'm trying to port the Ethernet driver for Intel IXP4xx CPU from Linux
> (I know there is Intel's code already ported). I've read the
> doc/README.drivers.eth. I came across some problems:
> - dev->halt() seems to be called before the first call to
>   dev->init() (i.e., before the hardware is initialized). Is it on
>   purpose?

it's on purpose because it makes the code simpler -- no need to maintain 
state.  drivers have to be able to handle halt() irregardless of init().  i 
dont see this being a problem for anyone.

> - dev->recv() seems to be called recursively, for example while doing
>   "dhcp" or "bootp" (ping is ok). dev->recv() in my driver calls
>   NetReceive(), which in turn (without returning to the caller, i.e.,
>   dev->recv(), first) reinitializes the driver on error (calls
>   dev->halt() and dev->init()). This makes a lot of mess in the driver,
>   should it stay this way? Perhaps we should queue the received packets
>   and process them on return from dev->recv()? Or maybe return all those
>   packets together?

where exactly do you see that call path ?  i dont see it anywhere ...

NetReceive() may call eth_send(), but that only expands into dev->send()

> - dev->recv() is provided with RX packet buffers. IXP4xx can only
>   receive to already allocated memory so the driver has to provide it's
>   own buffers prior to dev->recv(). I assume it's common situation with
>   all hardware recent enough. Does the driver have to copy data to
>   NetRxPackets[], or is it ok to simply call NetReceive using driver's
>   buffers?

the NetRxPackets[] are set up for you by default and are merely a convenience.  
you can use them or not, it doesnt really matter.  after all, your driver is 
what calls NetReceive() and the first argument there is the buffer that you're 
receiving.  none of the internal network code relies on these pointers.

i'll send out a patch to cover some of the finer details in the drivers doc.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20091101/d705474f/attachment.pgp 

[U-Boot] Net driver questions

[Krzysztof Halasa]

Mike Frysinger <vapier@gentoo.org> writes:

> it's on purpose because it makes the code simpler -- no need to maintain 
> state.  drivers have to be able to handle halt() irregardless of init().  i 
> dont see this being a problem for anyone.

Ok. Sure, that's not a problem for me, I just noted the README doesn't
talk about this.

>> - dev->recv() seems to be called recursively, for example while doing
>>   "dhcp" or "bootp" (ping is ok). dev->recv() in my driver calls
>>   NetReceive(), which in turn (without returning to the caller, i.e.,
>>   dev->recv(), first) reinitializes the driver on error (calls
>>   dev->halt() and dev->init()). This makes a lot of mess in the driver,
>>   should it stay this way? Perhaps we should queue the received packets
>>   and process them on return from dev->recv()? Or maybe return all those
>>   packets together?
>
> where exactly do you see that call path ?  i dont see it anywhere ...
>
> NetReceive() may call eth_send(), but that only expands into
> dev->send()

Let's look... The code does NetSetHandler(TftpHandler).
I think NetReceive() calls (*packetHandler)() = TftpHandler and this one
may call NetStartAgain().

> the NetRxPackets[] are set up for you by default and are merely a convenience.
> you can use them or not, it doesnt really matter.  after all, your driver is
> what calls NetReceive() and the first argument there is the buffer that you're
> receiving.  none of the internal network code relies on these pointers.

I see. Thanks for your mail.
-- 
Krzysztof Halasa

[U-Boot] Net driver questions

[Mike Frysinger]

On Monday 02 November 2009 10:08:00 Krzysztof Halasa wrote:
> Mike Frysinger <vapier@gentoo.org> writes:
> > it's on purpose because it makes the code simpler -- no need to maintain
> > state.  drivers have to be able to handle halt() irregardless of init(). 
> > i dont see this being a problem for anyone.
> 
> Ok. Sure, that's not a problem for me, I just noted the README doesn't
> talk about this.

that's because the README covered whatever i could think of at the time, and i 
basically brain dumped what little i knew of the net framework.  getting more 
people to read & question edge cases is good so those can be filled out.

(i already posted a patch for your first and last point)

> >> - dev->recv() seems to be called recursively, for example while doing
> >>   "dhcp" or "bootp" (ping is ok). dev->recv() in my driver calls
> >>   NetReceive(), which in turn (without returning to the caller, i.e.,
> >>   dev->recv(), first) reinitializes the driver on error (calls
> >>   dev->halt() and dev->init()). This makes a lot of mess in the driver,
> >>   should it stay this way? Perhaps we should queue the received packets
> >>   and process them on return from dev->recv()? Or maybe return all those
> >>   packets together?
> >
> > where exactly do you see that call path ?  i dont see it anywhere ...
> >
> > NetReceive() may call eth_send(), but that only expands into
> > dev->send()
> 
> Let's look... The code does NetSetHandler(TftpHandler).
> I think NetReceive() calls (*packetHandler)() = TftpHandler and this one
> may call NetStartAgain().

but this doesnt call recv(), and NetStartAgain() changes the handler, so i 
still dont see recursion with the recv() function.  but perhaps your 
definition of recursive is different from mine.  recv() does not turn around 
and call recv(), but recv() may turn around and call other driver functions.

the tftp error behavior is to basically discard all pending packets, so in 
practice, it shouldnt be a big deal.  it would probably be cleaner though if 
the packet handler returned an integer indicating errors that NetReceive() 
would pass up so that the driver recv knew to stop processing packets right 
away.  Ben would have to comment here though as this is beyond the extent of 
my networking knowledge.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20091102/f23f0903/attachment.pgp 

[U-Boot] Net driver questions

[Krzysztof Halasa]

Mike Frysinger <vapier@gentoo.org> writes:

>> Let's look... The code does NetSetHandler(TftpHandler).
>> I think NetReceive() calls (*packetHandler)() = TftpHandler and this one
>> may call NetStartAgain().
>
> but this doesnt call recv(), and NetStartAgain() changes the handler, so i 
> still dont see recursion with the recv() function.  but perhaps your 
> definition of recursive is different from mine.  recv() does not turn around 
> and call recv(), but recv() may turn around and call other driver functions.

I haven't debugged it yet (is there a way to print a backtrace?) but
the first recv() doesn't return from NetReceive().

> the tftp error behavior is to basically discard all pending packets, so in 
> practice, it shouldnt be a big deal.  it would probably be cleaner though if 
> the packet handler returned an integer indicating errors that NetReceive() 
> would pass up so that the driver recv knew to stop processing packets right 
> away.

I think so. I'll look at it too.
-- 
Krzysztof Halasa

[U-Boot] Net driver questions

[Mike Frysinger]

On Monday 02 November 2009 18:38:59 Krzysztof Halasa wrote:
> Mike Frysinger writes:
> >> Let's look... The code does NetSetHandler(TftpHandler).
> >> I think NetReceive() calls (*packetHandler)() = TftpHandler and this one
> >> may call NetStartAgain().
> >
> > but this doesnt call recv(), and NetStartAgain() changes the handler, so
> > i still dont see recursion with the recv() function.  but perhaps your
> > definition of recursive is different from mine.  recv() does not turn
> > around and call recv(), but recv() may turn around and call other driver
> > functions.
> 
> I haven't debugged it yet (is there a way to print a backtrace?) but
> the first recv() doesn't return from NetReceive().

the Blackfin port has a way of dumping a backtrace, but i dont know about any 
other arch
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20091102/e7a305fb/attachment.pgp