Re: [PATCH 11/15] can: rx-fifo: Add support for can state tracking and error polling

[David Jander <david@protonic.nl>]

On Thu, 06 Nov 2014 11:20:23 +0100
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 11/05/2014 06:16 PM, David Jander wrote:
> > On Mon, 03 Nov 2014 14:24:59 +0100
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > 
> >> On 11/03/2014 02:09 PM, David Jander wrote:
> >>> On Mon, 03 Nov 2014 13:58:43 +0100
> >>> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >>>
> >>>> On 11/03/2014 01:51 PM, David Jander wrote:
> >>>>>> On 10/10/2014 05:46 PM, David Jander wrote:
> >>>>>>> The interrupt handler should store the error flags if needed and call
> >>>>>>> can_rx_fifo_irq_error() if there was an error flag set. This will
> >>>>>>> trigger a napi-poll that will call poll_can_state() and
> >>>>>>> poll_bus_error() callbacks. Those should handle can state machine and
> >>>>>>> send error frames as needed.
> >>>>>>>
> >>>>>>> Signed-off-by: David Jander <david@protonic.nl>
> >>>>>>> ---
> >>>>>>>  drivers/net/can/dev.c   | 23 ++++++++++++++++++++---
> >>>>>>>  include/linux/can/dev.h |  9 +++++++++
> >>>>>>>  2 files changed, 29 insertions(+), 3 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> >>>>>>> index fc35d28..dac7579 100644
> >>>>>>> --- a/drivers/net/can/dev.c
> >>>>>>> +++ b/drivers/net/can/dev.c
> >>>>>>> @@ -352,6 +352,9 @@ static int can_rx_fifo_napi_poll(struct
> >>>>>>> napi_struct *napi, int quota) unsigned int tail;
> >>>>>>>  
> >>>>>>>  restart_poll:
> >>>>>>> +	if (work_done < quota)
> >>>>>>> +		work_done += fifo->poll_can_state(fifo);
> >>>>>>> +
> >>>>>>
> >>>>>> Do we need two callbacks in the poll-loop? I think one should be
> >>>>>> enough. The driver can call as many non-rx related functions as
> >>>>>> needed then. E.g. state change, bus error handling, tx-completion
> >>>>>> handling, etc...
> >>>>>
> >>>>> Ok. I split it into two handlers, because I saw most drivers doing
> >>>>> first state-handling then message reception and last error handling...
> >>>>> in this particular order. If that order is not important, we can use
> >>>>> one poll() function for both state- and error-handling.
> >>>>> In that case... should the poll() handler be called _before_ or _after_
> >>>>> handling the mailboxes?
> >>>>
> >>>> Following the general rule zero-one-infinity ("Allow none of foo, one of
> >>>> foo, or any number of foo" [1]) one callback should be fine. As we are
> >>>> optimizing for not loosing RX CAN frames I tend to say it should come
> >>>> after rx.
> >>>
> >>> Hahaha, ok, I got it :)
> >>> Any idea on how to call this function? Just "poll" seems a little bit
> >>> under-documented...
> >>
> >> Yes, what about extra_poll, poll_extra?
> > 
> > Oops. I just realized this isn't so easy... nor is this really a
> > zero-one-infinity case. On top of that the solution will get quite ugly
> > too:
> > 
> > poll_can_state() and poll_bus_error() have two distinct functions and
> > _both_ can generate a (error-) CAN frame. If I consolidate this into just
> > one handler, that handler could potentially generate 0, 1 or 2 CAN frames,
> > so I'd be forced to introduce an extra parameter to keep track of NAPI
> > quota inside the driver callback! This seems quite ugly to me, don't you
> > think?
> 
> What about:
>     work_done += fifo->poll_error(fifo, quota - work_done);

That would produce extra code like the following in flexcan.c:

static unsigned int flexcan_poll_extra(struct can_rx_fifo *fifo, quota)
{
	unsigned int ret;

	ret = flexcan_poll_state(fifo);
	if (ret < quota)
		ret += flexcan_poll_bus_err(fifo);

	return ret;
}

... which is quite ugly IMHO.

I just posted a new version, and I did not include this change yet. If you
still prefer to do it with just one handler (although I think they have two
distinct and defined functions), I will change it in the next version.

Best regards,

-- 
David Jander
Protonic Holland.