On 11/14/2012 08:50 AM, Andreas Larsson wrote:

On 11/13/2012 10:15 PM, Marc Kleine-Budde wrote:

[...]

quoted

On 11/12/2012 03:57 PM, Andreas Larsson wrote:

quoted

quoted

+    bpr = 0; /* Note bpr and brp are different concepts */
+    rsj = bt->sjw;
+    ps1 = (bt->prop_seg + bt->phase_seg1) - 1; /* tseg1 - 1 */
+    ps2 = bt->phase_seg2;
+    scaler = (bt->brp - 1);
+    timing |= (bpr << GRCAN_CONF_BPR_BIT) & GRCAN_CONF_BPR;
+    timing |= (rsj << GRCAN_CONF_RSJ_BIT) & GRCAN_CONF_RSJ;
+    timing |= (ps1 << GRCAN_CONF_PS1_BIT) & GRCAN_CONF_PS1;
+    timing |= (ps2 << GRCAN_CONF_PS2_BIT) & GRCAN_CONF_PS2;
+    timing |= (scaler << GRCAN_CONF_SCALER_BIT) & GRCAN_CONF_SCALER;
+
+    netdev_info(dev, "setting timing=0x%x\n", timing);

what about moving the sanity check before putting together the "timing"
variable and doing the netdev_info()?

The idea was for the user to have the full context of the problem when
getting the error (e.g., when using the bitrate method to set the timing
parameters, the calculated parameters are not otherwise known to the
user). But I can do that with a separate netdev_dbg and move the sanity
check as suggested.

I'm worried about the first stating "setting timing", but then not
writing it to the register.

quoted

quoted

quoted

+    if (!(ps1 > ps2)) {
+        netdev_err(dev, "PS1 > PS2 must hold: PS1=%d, PS2=%d\n",
+               ps1, ps2);
+        return -EINVAL;
+    }
+    if (!(ps2 >= rsj)) {
+        netdev_err(dev, "PS2 >= RSJ must hold: PS2=%d, RSJ=%d\n",
+               ps2, rsj);
+        return -EINVAL;
+    }
+
+    grcan_write_bits(&regs->conf, timing, GRCAN_CONF_TIMING);
+    return 0;
+}

[...]

quoted

quoted

quoted

+static int grcan_poll(struct napi_struct *napi, int rx_budget)
+{
+    struct grcan_priv *priv = container_of(napi, struct grcan_priv,

napi);

quoted

+    struct net_device *dev = priv->dev;
+    struct grcan_registers __iomem *regs = priv->regs;
+    int rx_work_done;
+    unsigned long flags;
+
+    /* Receive according to given budget */
+    rx_work_done = grcan_receive(dev, rx_budget);
+
+    /* Catch up echo skb according to separate budget to get the

benefits of

quoted

+     * napi for tx as well. The given rx_budget might not be

appropriate for

quoted

+     * the tx side.
+     */
+    grcan_transmit_catch_up(dev, GRCAN_TX_BUDGET);
+
+    spin_lock_irqsave(&priv->lock, flags);
+
+    if (grcan_poll_all_done(dev)) {

Just make it:
    if (work_done < budget) {
        napi_complete();
        enable_interrupts();
    }

If there are CAN frames pending, an interrupt will kick in and
reschedule the NAPI.

Sure, I can do that for the first check (and add back checking
tx_work_done as well). That misses to call napi_complete and start

Hmmm...Either move tx-complete handling to the interrupt handler, or
just use a budget big enough for rx and tx-complete.

interrupts in the case in which handling of frames are complete
work_done == budget though. But on the other hand, then grcan_poll will
be triggered once again and then detect that nothing is to be done if
that is still the case.

However, the problem with skipping the check after turning on interrupts
is that more frames can have arrived and/or have been sent after
calculating work_done and before turning on interrupts. For those
frames, unless I have misunderstood something, interrupts will not be
raised and they can get stuck until (if ever) later frames once again
trigger rescheduling of napi.

No, if correctly used, NAPI is race free and no events will be lost:

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

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;

quoted

quoted

quoted

+        bool complete = true;
+
+        if (!priv->closing) {
+            /* Enable tx and rx interrupts again */
+            grcan_set_bits(&regs->imr, GRCAN_IRQ_TX | GRCAN_IRQ_RX);
+
+            /* If more work arrived between detecting completion and
+             * turning on interrupts, we need to keep napi running
+             */
+            if (!grcan_poll_all_done(dev)) {
+                complete = false;
+                grcan_clear_bits(&regs->imr,
+                         GRCAN_IRQ_TX | GRCAN_IRQ_RX);
+            }
+        }
+        if (complete)
+            napi_complete(napi);
+    }
+
+    spin_unlock_irqrestore(&priv->lock, flags);
+
+    return rx_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   |