Re: [PATCH v8] can: grcan: Add device driver for GRCAN and GRHCAN cores

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

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

On 11/14/2012 12:02 PM, Andreas Larsson wrote:
> On 2012-11-14 09:43, Marc Kleine-Budde wrote:
>> Handle incoming events (rx or tx-complete) until:
>> a) number of handled events == budget
>> or
>> b) no more events pending.
>>
>>     while (work_done < budget && interrupts_pending()) {
>>         work_done += handle_rx(budget - work_done);
>>         work_done += handle_tx(budget - work_done);
>>     }
> 
> That could starve handle_tx completely though under high rx pressure,
> but I can prevent that by making sure that half of the budget is held
> back in the first call to handle_rx.

What about making the budget big enough to handle both rx and tx in one
napi call. Have a look at the marvell driver [1] for inspiration.

[1]
http://lxr.free-electrons.com/source/drivers/net/ethernet/marvell/mv643xx_eth.c#L2140

> 
>> Then, if you have handled less events then budget:
>> 1) call napi_complete()
>> then
>> 2) enable interrupts.
>>
>>     if (work_done < budget) {
>>         napi_complete();
>>         enable_interrupts();
>>     }
>>
>> Then, return number of handled events:
>>
>>     return work_done;
> 
> Any additional remarks on the following implementation of the poll
> function?
> 
> static int grcan_poll(struct napi_struct *napi, int budget)
> {
>     struct grcan_priv *priv = container_of(napi, struct grcan_priv, napi);
>     struct net_device *dev = priv->dev;
>     struct grcan_registers __iomem *regs = priv->regs;
>     unsigned long flags;
>     int work_done = 0;
>     int reserved = budget / 2;
> 
>     while (work_done < budget) {
>         int old_work_done = work_done;
> 
>         /* Prevent grcan_transmit_catch_up from starving by reserving
>          * part of the budget in the first iteration when calling
>          * grcan_receive.
>          */
>         work_done += grcan_receive(dev, budget - reserved - work_done);
>         reserved = 0;
> 
>         /* Catch up echo skb according to same budget, as
>          * grcan_transmit_catch_up can trigger echo frames being
>          * received.
>          */
>         work_done += grcan_transmit_catch_up(dev, budget - work_done);
> 
>         /* Break out if nothing was done */
>         if (work_done == old_work_done)
>             break;
>     }
> 
>     if (work_done < budget) {
>         napi_complete(napi);
> 
>         /* Guarantee no interference with a running reset that otherwise
>          * could turn off interrupts.
>          */
>         spin_lock_irqsave(&priv->lock, flags);
> 
>         /* Enable tx and rx interrupts again. No need to check
>          * priv->closing as napi_disable in grcan_close is waiting for
>          * scheduled napi calls to finish.
>          */
>         grcan_set_bits(&regs->imr, GRCAN_IRQ_TX | GRCAN_IRQ_RX);
> 
>         spin_unlock_irqrestore(&priv->lock, flags);
>     }
> 
>     return work_done;
> }
> 
> 
> Cheers,
> Andreas
> 


-- 
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: 259 bytes --]