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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3409 bytes --]

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);

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]