[U-Boot] Driver model Ethernet issue

[U-Boot] Driver model Ethernet issue

[Simon Glass]

Hi Joe,

I think moving the packet processing into the uclass was a good idea,
but I did not think it through.

If you look at the designware driver, when it receives a packet it
processes it and then retires that buffer for later reuse. The few
others I have looked at are similar.

We can't retire it in the recv() method since the packet is still
needed at that point, and may be overwritten by a new packet if we
retire its buffer space.

I am wondering if we should have a new method, like:

int free_pkt(struct udevice *dev, uchar *packet, int len)

which is called by the uclass after the packet is processed (assuming
that recv() returns a packet). It would pass the packet pointer and
length to the driver.

This would allow the driver to do what it needs to do.

What do you think? I'd like to figure this out and send v2 of the
sunxi Ethernet conversion.

Regards,
Simon

[U-Boot] Driver model Ethernet issue

[Simon Glass]

+mailing list which got dropped off at some point

Hi Joe,

On 30 March 2015 at 11:50, Joe Hershberger <joe.hershberger@gmail.com> wrote:
> Hi Simon,
>
> On Thu, Mar 26, 2015 at 3:11 AM, Joe Hershberger <joe.hershberger@gmail.com>
> wrote:
>>
>> Hi Simon,
>>
>> On Wed, Mar 25, 2015 at 10:10 PM, Simon Glass <sjg@chromium.org> wrote:
>> >
>> > Hi Joe,
>> >
>> > I think moving the packet processing into the uclass was a good idea,
>> > but I did not think it through.
>> >
>> > If you look at the designware driver, when it receives a packet it
>> > processes it and then retires that buffer for later reuse. The few
>> > others I have looked at are similar.
>> >
>> > We can't retire it in the recv() method since the packet is still
>> > needed at that point, and may be overwritten by a new packet if we
>> > retire its buffer space.
>> >
>> > I am wondering if we should have a new method, like:
>> >
>> > int free_pkt(struct udevice *dev, uchar *packet, int len)
>> >
>> > which is called by the uclass after the packet is processed (assuming
>> > that recv() returns a packet). It would pass the packet pointer and
>> > length to the driver.
>>
>> In light of this behavior that I also did not realize some drivers were
>> exhibiting, it is not simpler to revert the change? It seems that we are
>> adding complexity overall by having two functions to be implemented bu
>> drivers instead of one function to be called by the driver. Perhaps you can
>> convince me that the two functions are a better design, but I'd like to hear
>> the argument.
>
> What is your preference here? I can push the change either way. I can
> certainly see how it's a bit unusual to have a driver call back into the
> stack. Do you think two ops makes the driver cleaner than having the drivers
> call one function to process a packet?

Sorry for not coming back on this earlier. I was planning on doing
another spin of the sunxi Ethernet series but I have not got to it.

I do have a preference for keeping the control flow out of the driver.
It worries me that the driver calls a packet reception function, and
then the driver sits in the call stack for a while while it is
processed. Seems wrong to me.

Also you can argue that buffer management is a reasonable thing to
provide a method for. Using a buffer to receive a packet, sending that
packet for processing, and retiring the buffer are all conceptually
separate things. We could even consider (in the future!) supporting a
buffer ring in the uclass since all of the drivers I have looked at
have one.

So yes I think a new method is the best way to handle this situation.

>
>> > This would allow the driver to do what it needs to do.
>> >
>> > What do you think? I'd like to figure this out and send v2 of the
>> > sunxi Ethernet conversion.
>>
>> One comment about new driver-model network drivers is I'd like to see new
>> driver header files (if needed) live in include/net. I figure DM is a good
>> excuse to enforce that these headers get organized a bit.

Sounds good.

>
> Thanks,
> -Joe

Regards,
Simon

[U-Boot] [PATCH] dm: eth: Provide a way for drivers to manage packet buffers

[Joe Hershberger]

Some drivers need a chance to manage their receive buffers after the
packet has been handled by the network stack. Add an operation that
will allow the driver to be called in that case.

Reported-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---
This patch depends on dm/next

 include/net.h | 4 ++++
 net/eth.c     | 8 ++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/net.h b/include/net.h
index e7f28d7..f9df532 100644
--- a/include/net.h
+++ b/include/net.h
@@ -98,6 +98,9 @@ struct eth_pdata {
  * recv: Check if the hardware received a packet. If so, set the pointer to the
  *	 packet buffer in the packetp parameter. If not, return an error or 0 to
  *	 indicate that the hardware receive FIFO is empty
+ * free_pkt: Give the driver an opportunity to manage its packet buffer memory
+ *	     when the network stack is finished processing it. This will only be
+ *	     called when a packet was successfully returned from recv - optional
  * stop: Stop the hardware from looking for packets - may be called even if
  *	 state == PASSIVE
  * mcast: Join or leave a multicast group (for TFTP) - optional
@@ -113,6 +116,7 @@ struct eth_ops {
 	int (*start)(struct udevice *dev);
 	int (*send)(struct udevice *dev, void *packet, int length);
 	int (*recv)(struct udevice *dev, uchar **packetp);
+	int (*free_pkt)(struct udevice *dev, uchar *packet, int length);
 	void (*stop)(struct udevice *dev);
 #ifdef CONFIG_MCAST_TFTP
 	int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
diff --git a/net/eth.c b/net/eth.c
index 13b7723..889ad8f 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -342,10 +342,14 @@ int eth_rx(void)
 	/* Process up to 32 packets at one time */
 	for (i = 0; i < 32; i++) {
 		ret = eth_get_ops(current)->recv(current, &packet);
-		if (ret > 0)
+		if (ret > 0) {
 			net_process_received_packet(packet, ret);
-		else
+			if (eth_get_ops(current)->free_pkt)
+				eth_get_ops(current)->free_pkt(current, packet,
+							       ret);
+		} else {
 			break;
+		}
 	}
 	if (ret == -EAGAIN)
 		ret = 0;
-- 
1.7.11.5

[U-Boot] [PATCH] dm: eth: Provide a way for drivers to manage packet buffers

[Simon Glass]

Hi Joe,

On 30 March 2015 at 14:44, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Some drivers need a chance to manage their receive buffers after the
> packet has been handled by the network stack. Add an operation that
> will allow the driver to be called in that case.
>
> Reported-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
> This patch depends on dm/next
>
>  include/net.h | 4 ++++
>  net/eth.c     | 8 ++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index e7f28d7..f9df532 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -98,6 +98,9 @@ struct eth_pdata {
>   * recv: Check if the hardware received a packet. If so, set the pointer to the
>   *      packet buffer in the packetp parameter. If not, return an error or 0 to
>   *      indicate that the hardware receive FIFO is empty
> + * free_pkt: Give the driver an opportunity to manage its packet buffer memory
> + *          when the network stack is finished processing it. This will only be
> + *          called when a packet was successfully returned from recv - optional
>   * stop: Stop the hardware from looking for packets - may be called even if
>   *      state == PASSIVE
>   * mcast: Join or leave a multicast group (for TFTP) - optional
> @@ -113,6 +116,7 @@ struct eth_ops {
>         int (*start)(struct udevice *dev);
>         int (*send)(struct udevice *dev, void *packet, int length);
>         int (*recv)(struct udevice *dev, uchar **packetp);
> +       int (*free_pkt)(struct udevice *dev, uchar *packet, int length);
>         void (*stop)(struct udevice *dev);
>  #ifdef CONFIG_MCAST_TFTP
>         int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
> diff --git a/net/eth.c b/net/eth.c
> index 13b7723..889ad8f 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -342,10 +342,14 @@ int eth_rx(void)
>         /* Process up to 32 packets at one time */
>         for (i = 0; i < 32; i++) {
>                 ret = eth_get_ops(current)->recv(current, &packet);
> -               if (ret > 0)
> +               if (ret > 0) {

To match the old net stack behaviour I wonder if we should process the
packet when it is length 0, and require recv() to return -EAGAIN when
there is no packet? At least with designware, it processes a 0-length
packet for some reason, and we need to call free_pkt() in that case.

>                         net_process_received_packet(packet, ret);
> -               else
> +                       if (eth_get_ops(current)->free_pkt)
> +                               eth_get_ops(current)->free_pkt(current, packet,
> +                                                              ret);
> +               } else {
>                         break;
> +               }
>         }
>         if (ret == -EAGAIN)
>                 ret = 0;
> --
> 1.7.11.5
>

Tested on pcduino3:

Tested-by: Simon Glass <sjg@chromium.org>
Acked-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

[U-Boot] [PATCH] dm: eth: Provide a way for drivers to manage packet buffers

[Joe Hershberger]

Hi Simon,

On Tue, Mar 31, 2015 at 10:32 PM, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Joe,
>
> On 30 March 2015 at 14:44, Joe Hershberger <joe.hershberger@ni.com> wrote:
> > Some drivers need a chance to manage their receive buffers after the
> > packet has been handled by the network stack. Add an operation that
> > will allow the driver to be called in that case.
> >
> > Reported-by: Simon Glass <sjg@chromium.org>
> > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> > ---
> > This patch depends on dm/next
> >
> >  include/net.h | 4 ++++
> >  net/eth.c     | 8 ++++++--
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net.h b/include/net.h
> > index e7f28d7..f9df532 100644
> > --- a/include/net.h
> > +++ b/include/net.h
> > @@ -98,6 +98,9 @@ struct eth_pdata {
> >   * recv: Check if the hardware received a packet. If so, set the
pointer to the
> >   *      packet buffer in the packetp parameter. If not, return an
error or 0 to
> >   *      indicate that the hardware receive FIFO is empty
> > + * free_pkt: Give the driver an opportunity to manage its packet
buffer memory
> > + *          when the network stack is finished processing it. This
will only be
> > + *          called when a packet was successfully returned from recv -
optional
> >   * stop: Stop the hardware from looking for packets - may be called
even if
> >   *      state == PASSIVE
> >   * mcast: Join or leave a multicast group (for TFTP) - optional
> > @@ -113,6 +116,7 @@ struct eth_ops {
> >         int (*start)(struct udevice *dev);
> >         int (*send)(struct udevice *dev, void *packet, int length);
> >         int (*recv)(struct udevice *dev, uchar **packetp);
> > +       int (*free_pkt)(struct udevice *dev, uchar *packet, int length);
> >         void (*stop)(struct udevice *dev);
> >  #ifdef CONFIG_MCAST_TFTP
> >         int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
> > diff --git a/net/eth.c b/net/eth.c
> > index 13b7723..889ad8f 100644
> > --- a/net/eth.c
> > +++ b/net/eth.c
> > @@ -342,10 +342,14 @@ int eth_rx(void)
> >         /* Process up to 32 packets at one time */
> >         for (i = 0; i < 32; i++) {
> >                 ret = eth_get_ops(current)->recv(current, &packet);
> > -               if (ret > 0)
> > +               if (ret > 0) {
>
> To match the old net stack behaviour I wonder if we should process the
> packet when it is length 0, and require recv() to return -EAGAIN when
> there is no packet? At least with designware, it processes a 0-length
> packet for some reason, and we need to call free_pkt() in that case.

I pretty much assumed that since the driver is not expecting the network
stack to do anything with the buffer in the retval == 0 case, the driver
would handle its buffer management before returning from recv().

I'm not sure which is more clear to the driver writer... to expect the
free_pkt() call when returning 0 or to not expect it.  I guess my initial
instinct is that you would not expect it.

> >                         net_process_received_packet(packet, ret);
> > -               else
> > +                       if (eth_get_ops(current)->free_pkt)
> > +                               eth_get_ops(current)->free_pkt(current,
packet,
> > +                                                              ret);
> > +               } else {
> >                         break;
> > +               }
> >         }
> >         if (ret == -EAGAIN)
> >                 ret = 0;
> > --
> > 1.7.11.5
> >
>
> Tested on pcduino3:
>
> Tested-by: Simon Glass <sjg@chromium.org>
> Acked-by: Simon Glass <sjg@chromium.org>
>
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] dm: eth: Provide a way for drivers to manage packet buffers

[Simon Glass]

Hi Joe,

On 1 April 2015 at 10:03, Joe Hershberger <joe.hershberger@gmail.com> wrote:
> Hi Simon,
>
>
> On Tue, Mar 31, 2015 at 10:32 PM, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Joe,
>>
>> On 30 March 2015 at 14:44, Joe Hershberger <joe.hershberger@ni.com> wrote:
>> > Some drivers need a chance to manage their receive buffers after the
>> > packet has been handled by the network stack. Add an operation that
>> > will allow the driver to be called in that case.
>> >
>> > Reported-by: Simon Glass <sjg@chromium.org>
>> > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>> > ---
>> > This patch depends on dm/next
>> >
>> >  include/net.h | 4 ++++
>> >  net/eth.c     | 8 ++++++--
>> >  2 files changed, 10 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/include/net.h b/include/net.h
>> > index e7f28d7..f9df532 100644
>> > --- a/include/net.h
>> > +++ b/include/net.h
>> > @@ -98,6 +98,9 @@ struct eth_pdata {
>> >   * recv: Check if the hardware received a packet. If so, set the
>> > pointer to the
>> >   *      packet buffer in the packetp parameter. If not, return an error
>> > or 0 to
>> >   *      indicate that the hardware receive FIFO is empty
>> > + * free_pkt: Give the driver an opportunity to manage its packet buffer
>> > memory
>> > + *          when the network stack is finished processing it. This will
>> > only be
>> > + *          called when a packet was successfully returned from recv -
>> > optional
>> >   * stop: Stop the hardware from looking for packets - may be called
>> > even if
>> >   *      state == PASSIVE
>> >   * mcast: Join or leave a multicast group (for TFTP) - optional
>> > @@ -113,6 +116,7 @@ struct eth_ops {
>> >         int (*start)(struct udevice *dev);
>> >         int (*send)(struct udevice *dev, void *packet, int length);
>> >         int (*recv)(struct udevice *dev, uchar **packetp);
>> > +       int (*free_pkt)(struct udevice *dev, uchar *packet, int length);
>> >         void (*stop)(struct udevice *dev);
>> >  #ifdef CONFIG_MCAST_TFTP
>> >         int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
>> > diff --git a/net/eth.c b/net/eth.c
>> > index 13b7723..889ad8f 100644
>> > --- a/net/eth.c
>> > +++ b/net/eth.c
>> > @@ -342,10 +342,14 @@ int eth_rx(void)
>> >         /* Process up to 32 packets at one time */
>> >         for (i = 0; i < 32; i++) {
>> >                 ret = eth_get_ops(current)->recv(current, &packet);
>> > -               if (ret > 0)
>> > +               if (ret > 0) {
>>
>> To match the old net stack behaviour I wonder if we should process the
>> packet when it is length 0, and require recv() to return -EAGAIN when
>> there is no packet? At least with designware, it processes a 0-length
>> packet for some reason, and we need to call free_pkt() in that case.
>
> I pretty much assumed that since the driver is not expecting the network
> stack to do anything with the buffer in the retval == 0 case, the driver
> would handle its buffer management before returning from recv().
>
> I'm not sure which is more clear to the driver writer... to expect the
> free_pkt() call when returning 0 or to not expect it.  I guess my initial
> instinct is that you would not expect it.

Fair enough - should be documented one way or the other in the uclass
header net.h. I think a case can be made that a 0-length packet should
be handled differently in the uclass if there is any special behaviour
required, i.e. that the uclass should still call free_pkt() but may
skip processing the packet. But I'm really not sure why this happens
at all.

>
>> >                         net_process_received_packet(packet, ret);
>> > -               else
>> > +                       if (eth_get_ops(current)->free_pkt)
>> > +                               eth_get_ops(current)->free_pkt(current,
>> > packet,
>> > +                                                              ret);
>> > +               } else {
>> >                         break;
>> > +               }
>> >         }
>> >         if (ret == -EAGAIN)
>> >                 ret = 0;
>> > --
>> > 1.7.11.5
>> >
>>
>> Tested on pcduino3:
>>
>> Tested-by: Simon Glass <sjg@chromium.org>
>> Acked-by: Simon Glass <sjg@chromium.org>
>>
>> Regards,
>> Simon
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot

Regards,
Simon

[U-Boot] [PATCH v2] dm: eth: Provide a way for drivers to manage packet buffers

[Joe Hershberger]

Some drivers need a chance to manage their receive buffers after the
packet has been handled by the network stack. Add an operation that
will allow the driver to be called in that case.

Reported-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
Acked-by: Simon Glass <sjg@chromium.org>
Tested-by: Simon Glass <sjg@chromium.org>
Tested-on: pcduino3
---
This patch depends on dm/next

Changes in v2:
-Call free_pkt() even when driver returns 0
-Add more comments about this new behavior

 include/net.h | 8 +++++++-
 net/eth.c     | 4 +++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/net.h b/include/net.h
index e7f28d7..35602cd 100644
--- a/include/net.h
+++ b/include/net.h
@@ -97,7 +97,12 @@ struct eth_pdata {
  * send: Send the bytes passed in "packet" as a packet on the wire
  * recv: Check if the hardware received a packet. If so, set the pointer to the
  *	 packet buffer in the packetp parameter. If not, return an error or 0 to
- *	 indicate that the hardware receive FIFO is empty
+ *	 indicate that the hardware receive FIFO is empty. If 0 is returned, the
+ *	 network stack will not process the empty packet, but free_pkt() will be
+ *	 called if supplied
+ * free_pkt: Give the driver an opportunity to manage its packet buffer memory
+ *	     when the network stack is finished processing it. This will only be
+ *	     called when no error was returned from recv - optional
  * stop: Stop the hardware from looking for packets - may be called even if
  *	 state == PASSIVE
  * mcast: Join or leave a multicast group (for TFTP) - optional
@@ -113,6 +118,7 @@ struct eth_ops {
 	int (*start)(struct udevice *dev);
 	int (*send)(struct udevice *dev, void *packet, int length);
 	int (*recv)(struct udevice *dev, uchar **packetp);
+	int (*free_pkt)(struct udevice *dev, uchar *packet, int length);
 	void (*stop)(struct udevice *dev);
 #ifdef CONFIG_MCAST_TFTP
 	int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
diff --git a/net/eth.c b/net/eth.c
index 13b7723..05411f1 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -344,7 +344,9 @@ int eth_rx(void)
 		ret = eth_get_ops(current)->recv(current, &packet);
 		if (ret > 0)
 			net_process_received_packet(packet, ret);
-		else
+		if (ret >= 0 && eth_get_ops(current)->free_pkt)
+			eth_get_ops(current)->free_pkt(current, packet, ret);
+		if (ret <= 0)
 			break;
 	}
 	if (ret == -EAGAIN)
-- 
1.7.11.5

[U-Boot] [PATCH v2] dm: eth: Provide a way for drivers to manage packet buffers

[Simon Glass]

Hi Joe,

On 3 April 2015 at 19:09, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Some drivers need a chance to manage their receive buffers after the
> packet has been handled by the network stack. Add an operation that
> will allow the driver to be called in that case.
>
> Reported-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> Acked-by: Simon Glass <sjg@chromium.org>
> Tested-by: Simon Glass <sjg@chromium.org>
> Tested-on: pcduino3
> ---
> This patch depends on dm/next
>
> Changes in v2:
> -Call free_pkt() even when driver returns 0
> -Add more comments about this new behavior
>
>  include/net.h | 8 +++++++-
>  net/eth.c     | 4 +++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index e7f28d7..35602cd 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -97,7 +97,12 @@ struct eth_pdata {
>   * send: Send the bytes passed in "packet" as a packet on the wire
>   * recv: Check if the hardware received a packet. If so, set the pointer to the
>   *      packet buffer in the packetp parameter. If not, return an error or 0 to
> - *      indicate that the hardware receive FIFO is empty
> + *      indicate that the hardware receive FIFO is empty. If 0 is returned, the
> + *      network stack will not process the empty packet, but free_pkt() will be
> + *      called if supplied
> + * free_pkt: Give the driver an opportunity to manage its packet buffer memory
> + *          when the network stack is finished processing it. This will only be
> + *          called when no error was returned from recv - optional
>   * stop: Stop the hardware from looking for packets - may be called even if
>   *      state == PASSIVE
>   * mcast: Join or leave a multicast group (for TFTP) - optional
> @@ -113,6 +118,7 @@ struct eth_ops {
>         int (*start)(struct udevice *dev);
>         int (*send)(struct udevice *dev, void *packet, int length);
>         int (*recv)(struct udevice *dev, uchar **packetp);
> +       int (*free_pkt)(struct udevice *dev, uchar *packet, int length);
>         void (*stop)(struct udevice *dev);
>  #ifdef CONFIG_MCAST_TFTP
>         int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
> diff --git a/net/eth.c b/net/eth.c
> index 13b7723..05411f1 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -344,7 +344,9 @@ int eth_rx(void)
>                 ret = eth_get_ops(current)->recv(current, &packet);
>                 if (ret > 0)
>                         net_process_received_packet(packet, ret);
> -               else
> +               if (ret >= 0 && eth_get_ops(current)->free_pkt)
> +                       eth_get_ops(current)->free_pkt(current, packet, ret);
> +               if (ret <= 0)
>                         break;
>         }
>         if (ret == -EAGAIN)
> --
> 1.7.11.5
>

Looks good, I will pick this up for u-boot-dm/next.

Regards,
Simon

[U-Boot] [PATCH v2] dm: eth: Provide a way for drivers to manage packet buffers

[Simon Glass]

On 5 April 2015 at 12:31, Simon Glass <sjg@chromium.org> wrote:
> Hi Joe,
>
> On 3 April 2015 at 19:09, Joe Hershberger <joe.hershberger@ni.com> wrote:
>> Some drivers need a chance to manage their receive buffers after the
>> packet has been handled by the network stack. Add an operation that
>> will allow the driver to be called in that case.
>>
>> Reported-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>> Acked-by: Simon Glass <sjg@chromium.org>
>> Tested-by: Simon Glass <sjg@chromium.org>
>> Tested-on: pcduino3
>> ---
>> This patch depends on dm/next
>>
>> Changes in v2:
>> -Call free_pkt() even when driver returns 0
>> -Add more comments about this new behavior
>>
>>  include/net.h | 8 +++++++-
>>  net/eth.c     | 4 +++-
>>  2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net.h b/include/net.h
>> index e7f28d7..35602cd 100644
>> --- a/include/net.h
>> +++ b/include/net.h
>> @@ -97,7 +97,12 @@ struct eth_pdata {
>>   * send: Send the bytes passed in "packet" as a packet on the wire
>>   * recv: Check if the hardware received a packet. If so, set the pointer to the
>>   *      packet buffer in the packetp parameter. If not, return an error or 0 to
>> - *      indicate that the hardware receive FIFO is empty
>> + *      indicate that the hardware receive FIFO is empty. If 0 is returned, the
>> + *      network stack will not process the empty packet, but free_pkt() will be
>> + *      called if supplied
>> + * free_pkt: Give the driver an opportunity to manage its packet buffer memory
>> + *          when the network stack is finished processing it. This will only be
>> + *          called when no error was returned from recv - optional
>>   * stop: Stop the hardware from looking for packets - may be called even if
>>   *      state == PASSIVE
>>   * mcast: Join or leave a multicast group (for TFTP) - optional
>> @@ -113,6 +118,7 @@ struct eth_ops {
>>         int (*start)(struct udevice *dev);
>>         int (*send)(struct udevice *dev, void *packet, int length);
>>         int (*recv)(struct udevice *dev, uchar **packetp);
>> +       int (*free_pkt)(struct udevice *dev, uchar *packet, int length);
>>         void (*stop)(struct udevice *dev);
>>  #ifdef CONFIG_MCAST_TFTP
>>         int (*mcast)(struct udevice *dev, const u8 *enetaddr, int join);
>> diff --git a/net/eth.c b/net/eth.c
>> index 13b7723..05411f1 100644
>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -344,7 +344,9 @@ int eth_rx(void)
>>                 ret = eth_get_ops(current)->recv(current, &packet);
>>                 if (ret > 0)
>>                         net_process_received_packet(packet, ret);
>> -               else
>> +               if (ret >= 0 && eth_get_ops(current)->free_pkt)
>> +                       eth_get_ops(current)->free_pkt(current, packet, ret);
>> +               if (ret <= 0)
>>                         break;
>>         }
>>         if (ret == -EAGAIN)
>> --
>> 1.7.11.5
>>
>
> Looks good, I will pick this up for u-boot-dm/next.

Applied to u-boot-dm/next, thanks!

- Simon

[U-Boot] Driver model Ethernet issue

[Albert ARIBAUD]

Hello Simon,

On Mon, 30 Mar 2015 12:03:41 -0600, Simon Glass <sjg@chromium.org>
wrote:

> Also you can argue that buffer management is a reasonable thing to
> provide a method for. Using a buffer to receive a packet, sending that
> packet for processing, and retiring the buffer are all conceptually
> separate things. We could even consider (in the future!) supporting a
> buffer ring in the uclass since all of the drivers I have looked at
> have one.

Just in case: remember that some drivers use SRAM-based buffers while
others use DDR-based buffers. Buffer management should support either
use case. Also, there's more than just buffers; descriptors might be
needed too -- whether they should be generalized or not, I have no
clear idea, though.

Amicalement,
-- 
Albert.

[U-Boot] Driver model Ethernet issue

[Simon Glass]

Hi,

On 2 April 2015 at 01:50, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote:
>
> Hello Simon,
>
> On Mon, 30 Mar 2015 12:03:41 -0600, Simon Glass <sjg@chromium.org>
> wrote:
>
> > Also you can argue that buffer management is a reasonable thing to
> > provide a method for. Using a buffer to receive a packet, sending that
> > packet for processing, and retiring the buffer are all conceptually
> > separate things. We could even consider (in the future!) supporting a
> > buffer ring in the uclass since all of the drivers I have looked at
> > have one.
>
> Just in case: remember that some drivers use SRAM-based buffers while
> others use DDR-based buffers. Buffer management should support either
> use case. Also, there's more than just buffers; descriptors might be
> needed too -- whether they should be generalized or not, I have no
> clear idea, though.

Me neither, it's not important for now. If we identify common areas in
the drivers another way is to add library code to the uclass which
some drivers can call.

Regards,
Simon