[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 at gmail.com> wrote:
> Hi Simon,
>
>
> On Tue, Mar 31, 2015 at 10:32 PM, Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Joe,
>>
>> On 30 March 2015 at 14:44, Joe Hershberger <joe.hershberger at 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 at chromium.org>
>> > Signed-off-by: Joe Hershberger <joe.hershberger at 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 at chromium.org>
>> Acked-by: Simon Glass <sjg at chromium.org>
>>
>> Regards,
>> Simon
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot

Regards,
Simon