| Message ID | 1427748269-16851-1-git-send-email-joe.hershberger@ni.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
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
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 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@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot Regards, Simon
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;
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(-)