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

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

[-- Attachment #1.1: Type: text/plain, Size: 2917 bytes --]

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 */

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 |


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