Message ID | 80k4k45csy.fsf@merkur.tec.linutronix.de |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Hello, On 11/23/2010 03:34 PM, John Ogness wrote: > If bus error reporting is disabled and bus errors occur, the flexcan > driver will hog the system because it continually re-enables the IRQs > (work_done = 0). This patch changes the driver to only re-enable the > IRQs if some work was actually done. This allows the features of NAPI to > be used for bus errors as well (when bus error reporting is disabled). Good idea, however the chip has IMHO a bug: The problem with the error interrupt is, when disabled the can core doesn't issue any can bus warning or bus passive interrupts. Can you ensure that you get both error warning and error passive can error messages with disabled BERR and you patch applied? > This patch is against Linux next-20101123. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> > --- > drivers/net/can/flexcan.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > --- next-20101123-a/drivers/net/can/flexcan.c > +++ next-20101123-b/drivers/net/can/flexcan.c > @@ -535,9 +535,12 @@ static int flexcan_poll(struct napi_stru > > if (work_done < quota) { > napi_complete(napi); > - /* enable IRQs */ > - writel(FLEXCAN_IFLAG_DEFAULT, ®s->imask1); > - writel(priv->reg_ctrl_default, ®s->ctrl); > + > + if (work_done > 0) { > + /* enable IRQs */ > + writel(FLEXCAN_IFLAG_DEFAULT, ®s->imask1); > + writel(priv->reg_ctrl_default, ®s->ctrl); > + } > } > > return work_done; If a bus error occurs and bus error reporting is disabled, work_done will stay 0, so both the RX and the ERR interrupt stay disabled (which is done in flexcan_irq). I think we should always enable the RX interrupt. (However, if the bus is working again the chip can probably send messages again, so we get a TX-complete IRQ, but this depends on the application.) Having a look at flexcan_irq: > /* > * schedule NAPI in case of: > * - rx IRQ > * - state change IRQ > * - bus error IRQ and bus error reporting is activated > */ This mean with disabled BERR NAPI should only be scheduled in case of a RX or a state change interrupt. Both of these interrupts should generate a can_frame in flexcan_poll and this the work_done shoud be > 0, modulo out of memory situations. > if ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) || > (reg_esr & FLEXCAN_ESR_ERR_STATE) || > flexcan_has_and_handle_berr(priv, reg_esr)) { > /* > * The error bits are cleared on read, > * save them for later use. > */ > priv->reg_esr = reg_esr & FLEXCAN_ESR_ERR_BUS; > writel(FLEXCAN_IFLAG_DEFAULT & ~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, > ®s->imask1); > writel(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_ALL, > ®s->ctrl); > napi_schedule(&priv->napi); > } So thinking about the problem I don't see how your patch works. But there might be a bug in the driver or my logic. Cheers, Marc
--- next-20101123-a/drivers/net/can/flexcan.c +++ next-20101123-b/drivers/net/can/flexcan.c @@ -535,9 +535,12 @@ static int flexcan_poll(struct napi_stru if (work_done < quota) { napi_complete(napi); - /* enable IRQs */ - writel(FLEXCAN_IFLAG_DEFAULT, ®s->imask1); - writel(priv->reg_ctrl_default, ®s->ctrl); + + if (work_done > 0) { + /* enable IRQs */ + writel(FLEXCAN_IFLAG_DEFAULT, ®s->imask1); + writel(priv->reg_ctrl_default, ®s->ctrl); + } } return work_done;
If bus error reporting is disabled and bus errors occur, the flexcan driver will hog the system because it continually re-enables the IRQs (work_done = 0). This patch changes the driver to only re-enable the IRQs if some work was actually done. This allows the features of NAPI to be used for bus errors as well (when bus error reporting is disabled). This patch is against Linux next-20101123. Signed-off-by: John Ogness <john.ogness@linutronix.de> --- drivers/net/can/flexcan.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html