RE: [PATCH V2 1/4] can: flexcan: fix deadlock when using self wakeup

[Joakim Zhang <qiangqing.zhang@xxxxxxx>]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> Sent: 2019年12月4日 16:31
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>; sean@xxxxxxxxxx;
> linux-can@xxxxxxxxxxxxxxx
> Cc: dl-linux-imx <linux-imx@xxxxxxx>; netdev@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V2 1/4] can: flexcan: fix deadlock when using self wakeup
> 
> On 12/4/19 2:58 AM, Joakim Zhang wrote:
> > [...]
> >>>  drivers/net/can/flexcan.c | 19 +++++++++++--------
> >>>  1 file changed, 11 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> >>> index 2efa06119f68..2297663cacb2 100644
> >>> --- a/drivers/net/can/flexcan.c
> >>> +++ b/drivers/net/can/flexcan.c
> >>> @@ -134,8 +134,7 @@
> >>>  	(FLEXCAN_ESR_ERR_BUS | FLEXCAN_ESR_ERR_STATE)  #define
> >>> FLEXCAN_ESR_ALL_INT \
> >>>  	(FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
> >>> -	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT | \
> >>> -	 FLEXCAN_ESR_WAK_INT)
> >>> +	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)
> >>
> >> Why do you remove the FLEXCAN_ESR_WAK_INT from the
> >> FLEXCAN_ESR_ALL_INT?
> >>
> >>>
> >>>  /* FLEXCAN interrupt flag register (IFLAG) bits */
> >>>  /* Errata ERR005829 step7: Reserve first valid MB */ @@ -960,6
> >>> +959,12 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> >>>
> >>>  	reg_esr = priv->read(&regs->esr);
> >>>
> >>> +	/* ACK wakeup interrupt */
> >>> +	if (reg_esr & FLEXCAN_ESR_WAK_INT) {
> >>> +		handled = IRQ_HANDLED;
> >>> +		priv->write(reg_esr & FLEXCAN_ESR_WAK_INT, &regs->esr);
> >>> +	}
> >>> +
> >>
> >> If FLEXCAN_ESR_WAK_INT stays in FLEXCAN_ESR_ALL_INT, you don't need
> >> that explicit ACK here.
> >
> > Hi Marc,
> >
> > I remove the FLEXCAN_ESR_WAK_INT from the FLEXCAN_ESR_ALL_INT since
> > FLEXCAN_ESR_ALL_INT is for all bus error and state change IRQ sources,
> > wakeup interrupt does not belong to these. If you think this does not
> > need, I can remove this change.
> 
> I see, makes sense.
> 
> Make this a separate patch. Move the FLEXCAN_ESR_WAK_INT from the
> FLEXCAN_ESR_ALL_INT, but add it to the existing ack of the interrupts.
> Like this:
> 
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index b6f675a5e2d9..74f622b40b61 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -960,10 +960,10 @@ static irqreturn_t flexcan_irq(int irq, void
> > *dev_id)
> >
> >         reg_esr = priv->read(&regs->esr);
> >
> > -       /* ACK all bus error and state change IRQ sources */
> > -       if (reg_esr & FLEXCAN_ESR_ALL_INT) {
> > +       /* ACK all bus error, state change and wake IRQ sources */
> > +       if (reg_esr & (FLEXCAN_ESR_ALL_INT | FLEXCAN_ESR_WAK_INT)) {
> >                 handled = IRQ_HANDLED;
> > -               priv->write(reg_esr & FLEXCAN_ESR_ALL_INT,
> &regs->esr);
> > +               priv->write(reg_esr & (FLEXCAN_ESR_ALL_INT |
> > + FLEXCAN_ESR_WAK_INT), &regs->esr);
> >         }
> >
> >         /* state change interrupt or broken error state quirk fix is
> > enabled */

Got it! Thanks.

Best Regards,
Joakim Zhang
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |