Re: [U-Boot] Driver model Ethernet issue
+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
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;
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 toindicate 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
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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 erroror 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 willonly 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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Regards, Simon
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)
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
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
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,
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
participants (4)
-
Albert ARIBAUD -
Joe Hershberger -
Joe Hershberger -
Simon Glass