diff mbox series

[3/3] can: flexcan: handle S32G2/S32G3 separate interrupt lines

Message ID 20241119081053.4175940-4-ciprianmarian.costea@oss.nxp.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series add FlexCAN support for S32G2/S32G3 SoCs | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 3 maintainers not CCed: haibo.chen@nxp.com Frank.Li@nxp.com han.xu@nxp.com
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 71 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-11-19--09-00 (tests: 788)

Commit Message

Ciprian Marian Costea Nov. 19, 2024, 8:10 a.m. UTC 
From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>

On S32G2/S32G3 SoC, there are separate interrupts
for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.

In order to handle this FlexCAN hardware particularity, reuse
the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
handling support.

Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
which can be used in case there are two separate mailbox ranges
controlled by independent hardware interrupt lines, as it is
the case on S32G2/S32G3 SoC.

Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
---
 drivers/net/can/flexcan/flexcan-core.c | 25 +++++++++++++++++++++++--
 drivers/net/can/flexcan/flexcan.h      |  3 +++
 2 files changed, 26 insertions(+), 2 deletions(-)

Comments

Vincent Mailhol Nov. 19, 2024, 9:26 a.m. UTC | #1 
On 19/11/2024 at 17:10, Ciprian Costea wrote:
> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> On S32G2/S32G3 SoC, there are separate interrupts
> for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
> 
> In order to handle this FlexCAN hardware particularity, reuse
> the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
> handling support.
> 
> Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
> which can be used in case there are two separate mailbox ranges
> controlled by independent hardware interrupt lines, as it is
> the case on S32G2/S32G3 SoC.
> 
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> ---
>  drivers/net/can/flexcan/flexcan-core.c | 25 +++++++++++++++++++++++--
>  drivers/net/can/flexcan/flexcan.h      |  3 +++
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> index f0dee04800d3..dc56d4a7d30b 100644
> --- a/drivers/net/can/flexcan/flexcan-core.c
> +++ b/drivers/net/can/flexcan/flexcan-core.c
> @@ -390,9 +390,10 @@ static const struct flexcan_devtype_data nxp_s32g2_devtype_data = {
>  	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>  		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>  		FLEXCAN_QUIRK_USE_RX_MAILBOX | FLEXCAN_QUIRK_SUPPORT_FD |
> -		FLEXCAN_QUIRK_SUPPORT_ECC |
> +		FLEXCAN_QUIRK_SUPPORT_ECC | FLEXCAN_QUIRK_NR_IRQ_3 |
>  		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
> -		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
> +		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
> +		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
>  };
>  
>  static const struct can_bittiming_const flexcan_bittiming_const = {
> @@ -1771,12 +1772,21 @@ static int flexcan_open(struct net_device *dev)
>  			goto out_free_irq_boff;
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> +		err = request_irq(priv->irq_secondary_mb,
> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
> +		if (err)
> +			goto out_free_irq_err;
> +	}

Is the logic here correct?

  request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);

is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.

So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
not initialized.

Did you confirm if it is safe to call free_irq() on an uninitialized irq?

(and I can see that currently there is no such device with
FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
who knows if such device will be introduced in the future?)

>  	flexcan_chip_interrupts_enable(dev);
>  
>  	netif_start_queue(dev);
>  
>  	return 0;
>  
> + out_free_irq_err:
> +	free_irq(priv->irq_err, dev);
>   out_free_irq_boff:
>  	free_irq(priv->irq_boff, dev);
>   out_free_irq:
> @@ -1808,6 +1818,9 @@ static int flexcan_close(struct net_device *dev)
>  		free_irq(priv->irq_boff, dev);
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
> +		free_irq(priv->irq_secondary_mb, dev);
> +
>  	free_irq(dev->irq, dev);
>  	can_rx_offload_disable(&priv->offload);
>  	flexcan_chip_stop_disable_on_error(dev);
> @@ -2197,6 +2210,14 @@ static int flexcan_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> +		priv->irq_secondary_mb = platform_get_irq(pdev, 3);
> +		if (priv->irq_secondary_mb < 0) {
> +			err = priv->irq_secondary_mb;
> +			goto failed_platform_get_irq;
> +		}
> +	}
> +
>  	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>  		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
>  			CAN_CTRLMODE_FD_NON_ISO;
> diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
> index 4933d8c7439e..d4b1a954c538 100644
> --- a/drivers/net/can/flexcan/flexcan.h
> +++ b/drivers/net/can/flexcan/flexcan.h
> @@ -70,6 +70,8 @@
>  #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
>  /* Setup stop mode with ATF SCMI protocol to support wakeup */
>  #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17)
> +/* Setup secondary mailbox interrupt */
> +#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
>  
>  struct flexcan_devtype_data {
>  	u32 quirks;		/* quirks needed for different IP cores */
> @@ -105,6 +107,7 @@ struct flexcan_priv {
>  	struct regulator *reg_xceiver;
>  	struct flexcan_stop_mode stm;
>  
> +	int irq_secondary_mb;
>  	int irq_boff;
>  	int irq_err;
>  

Yours sincerely,
Vincent Mailhol
Ciprian Marian Costea Nov. 19, 2024, 10:01 a.m. UTC | #2 
On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
> On 19/11/2024 at 17:10, Ciprian Costea wrote:
>> From: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>>
>> On S32G2/S32G3 SoC, there are separate interrupts
>> for state change, bus errors, MBs 0-7 and MBs 8-127 respectively.
>>
>> In order to handle this FlexCAN hardware particularity, reuse
>> the 'FLEXCAN_QUIRK_NR_IRQ_3' quirk provided by mcf5441x's irq
>> handling support.
>>
>> Additionally, introduce 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ' quirk,
>> which can be used in case there are two separate mailbox ranges
>> controlled by independent hardware interrupt lines, as it is
>> the case on S32G2/S32G3 SoC.
>>
>> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
>> ---
>>   drivers/net/can/flexcan/flexcan-core.c | 25 +++++++++++++++++++++++--
>>   drivers/net/can/flexcan/flexcan.h      |  3 +++
>>   2 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
>> index f0dee04800d3..dc56d4a7d30b 100644
>> --- a/drivers/net/can/flexcan/flexcan-core.c
>> +++ b/drivers/net/can/flexcan/flexcan-core.c
>> @@ -390,9 +390,10 @@ static const struct flexcan_devtype_data nxp_s32g2_devtype_data = {
>>   	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
>>   		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
>>   		FLEXCAN_QUIRK_USE_RX_MAILBOX | FLEXCAN_QUIRK_SUPPORT_FD |
>> -		FLEXCAN_QUIRK_SUPPORT_ECC |
>> +		FLEXCAN_QUIRK_SUPPORT_ECC | FLEXCAN_QUIRK_NR_IRQ_3 |
>>   		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
>> -		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
>> +		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
>> +		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
>>   };
>>   
>>   static const struct can_bittiming_const flexcan_bittiming_const = {
>> @@ -1771,12 +1772,21 @@ static int flexcan_open(struct net_device *dev)
>>   			goto out_free_irq_boff;
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>> +		err = request_irq(priv->irq_secondary_mb,
>> +				  flexcan_irq, IRQF_SHARED, dev->name, dev);
>> +		if (err)
>> +			goto out_free_irq_err;
>> +	}
> 
> Is the logic here correct?
> 
>    request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
> 
> is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
> 
> So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
> FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
> not initialized.
> 
> Did you confirm if it is safe to call free_irq() on an uninitialized irq?
> 
> (and I can see that currently there is no such device with
> FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
> who knows if such device will be introduced in the future?)
> 

Hello Vincent,

Thanks for your review. Indeed this seems to be an incorrect logic since 
I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3' 
and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.

I will change the impacted section to:
	if (err) {
		if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
			goto out_free_irq_err;
		else
			goto out_free_irq;
	}

Best Regards,
Ciprian

>>   	flexcan_chip_interrupts_enable(dev);
>>   
>>   	netif_start_queue(dev);
>>   
>>   	return 0;
>>   
>> + out_free_irq_err:
>> +	free_irq(priv->irq_err, dev);
>>    out_free_irq_boff:
>>   	free_irq(priv->irq_boff, dev);
>>    out_free_irq:
>> @@ -1808,6 +1818,9 @@ static int flexcan_close(struct net_device *dev)
>>   		free_irq(priv->irq_boff, dev);
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
>> +		free_irq(priv->irq_secondary_mb, dev);
>> +
>>   	free_irq(dev->irq, dev);
>>   	can_rx_offload_disable(&priv->offload);
>>   	flexcan_chip_stop_disable_on_error(dev);
>> @@ -2197,6 +2210,14 @@ static int flexcan_probe(struct platform_device *pdev)
>>   		}
>>   	}
>>   
>> +	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>> +		priv->irq_secondary_mb = platform_get_irq(pdev, 3);
>> +		if (priv->irq_secondary_mb < 0) {
>> +			err = priv->irq_secondary_mb;
>> +			goto failed_platform_get_irq;
>> +		}
>> +	}
>> +
>>   	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
>>   		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
>>   			CAN_CTRLMODE_FD_NON_ISO;
>> diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
>> index 4933d8c7439e..d4b1a954c538 100644
>> --- a/drivers/net/can/flexcan/flexcan.h
>> +++ b/drivers/net/can/flexcan/flexcan.h
>> @@ -70,6 +70,8 @@
>>   #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
>>   /* Setup stop mode with ATF SCMI protocol to support wakeup */
>>   #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17)
>> +/* Setup secondary mailbox interrupt */
>> +#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
>>   
>>   struct flexcan_devtype_data {
>>   	u32 quirks;		/* quirks needed for different IP cores */
>> @@ -105,6 +107,7 @@ struct flexcan_priv {
>>   	struct regulator *reg_xceiver;
>>   	struct flexcan_stop_mode stm;
>>   
>> +	int irq_secondary_mb;
>>   	int irq_boff;
>>   	int irq_err;
>>   
> 
> Yours sincerely,
> Vincent Mailhol
>
Vincent Mailhol Nov. 19, 2024, 11:26 a.m. UTC | #3 
On 19/11/2024 at 19:01, Ciprian Marian Costea wrote:
> On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
>> On 19/11/2024 at 17:10, Ciprian Costea wrote:

(...)

>>>   +    if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>>> +        err = request_irq(priv->irq_secondary_mb,
>>> +                  flexcan_irq, IRQF_SHARED, dev->name, dev);
>>> +        if (err)
>>> +            goto out_free_irq_err;
>>> +    }
>>
>> Is the logic here correct?
>>
>>    request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
>>
>> is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
>>
>> So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
>> FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
>> not initialized.
>>
>> Did you confirm if it is safe to call free_irq() on an uninitialized irq?
>>
>> (and I can see that currently there is no such device with
>> FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
>> who knows if such device will be introduced in the future?)
>>
> 
> Hello Vincent,
> 
> Thanks for your review. Indeed this seems to be an incorrect logic since
> I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3'
> and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.
> 
> I will change the impacted section to:
>     if (err) {
>         if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>             goto out_free_irq_err;
>         else
>             goto out_free_irq;
>     }

This is better. Alternatively, you could move the check into the label:

  out_free_irq_err:
  	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
  		free_irq(priv->irq_err, dev);

But this is not a strong preference, I let you pick the one which you
prefer.

>>>       flexcan_chip_interrupts_enable(dev);
>>>         netif_start_queue(dev);
>>>         return 0;
>>>   + out_free_irq_err:
>>> +    free_irq(priv->irq_err, dev);
>>>    out_free_irq_boff:
>>>       free_irq(priv->irq_boff, dev);
>>>    out_free_irq:

(...)


Yours sincerely,
Vincent Mailhol
Marc Kleine-Budde Nov. 19, 2024, 11:28 a.m. UTC | #4 
On 19.11.2024 20:26:26, Vincent Mailhol wrote:
> On 19/11/2024 at 19:01, Ciprian Marian Costea wrote:
> > On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
> >> On 19/11/2024 at 17:10, Ciprian Costea wrote:
> 
> (...)
> 
> >>>   +    if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
> >>> +        err = request_irq(priv->irq_secondary_mb,
> >>> +                  flexcan_irq, IRQF_SHARED, dev->name, dev);
> >>> +        if (err)
> >>> +            goto out_free_irq_err;
> >>> +    }
> >>
> >> Is the logic here correct?
> >>
> >>    request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
> >>
> >> is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
> >>
> >> So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
> >> FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
> >> not initialized.
> >>
> >> Did you confirm if it is safe to call free_irq() on an uninitialized irq?
> >>
> >> (and I can see that currently there is no such device with
> >> FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
> >> who knows if such device will be introduced in the future?)
> >>
> > 
> > Hello Vincent,
> > 
> > Thanks for your review. Indeed this seems to be an incorrect logic since
> > I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3'
> > and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.
> > 
> > I will change the impacted section to:
> >     if (err) {
> >         if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
> >             goto out_free_irq_err;
> >         else
> >             goto out_free_irq;
> >     }
> 
> This is better. Alternatively, you could move the check into the label:

+1

>   out_free_irq_err:
>   	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>   		free_irq(priv->irq_err, dev);
> 
> But this is not a strong preference, I let you pick the one which you
> prefer.

regards,
Marc
Vincent Mailhol Nov. 19, 2024, 11:36 a.m. UTC | #5 
On 19/11/2024 at 20:26, Vincent Mailhol wrote:
> On 19/11/2024 at 19:01, Ciprian Marian Costea wrote:
>> On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
>>> On 19/11/2024 at 17:10, Ciprian Costea wrote:
> 
> (...)
> 
>>>>   +    if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>>>> +        err = request_irq(priv->irq_secondary_mb,
>>>> +                  flexcan_irq, IRQF_SHARED, dev->name, dev);
>>>> +        if (err)
>>>> +            goto out_free_irq_err;
>>>> +    }
>>>
>>> Is the logic here correct?
>>>
>>>    request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
>>>
>>> is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
>>>
>>> So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
>>> FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
>>> not initialized.
>>>
>>> Did you confirm if it is safe to call free_irq() on an uninitialized irq?
>>>
>>> (and I can see that currently there is no such device with
>>> FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
>>> who knows if such device will be introduced in the future?)
>>>
>>
>> Hello Vincent,
>>
>> Thanks for your review. Indeed this seems to be an incorrect logic since
>> I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3'
>> and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.
>>
>> I will change the impacted section to:
>>     if (err) {
>>         if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>>             goto out_free_irq_err;
>>         else
>>             goto out_free_irq;
>>     }
> 
> This is better. Alternatively, you could move the check into the label:
> 
>   out_free_irq_err:
>   	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>   		free_irq(priv->irq_err, dev);
> 
> But this is not a strong preference, I let you pick the one which you
> prefer.

On second thought, it is a strong preference. If you keep the

	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
		goto out_free_irq_err;
	else
		goto out_free_irq;

then what if more code with a clean-up label is added to flexcan_open()?
I am thinking of this:

  out_free_foo:
  	free(foo);
  out_free_irq_err:
  	free_irq(priv-irq_err, dev);
  out_free_irq_boff:
  	free_irq(priv->irq_boff, dev);

Jumping to out_free_foo would now be incorrect because the
out_free_irq_err label would also be visited.

>>>>       flexcan_chip_interrupts_enable(dev);
>>>>         netif_start_queue(dev);
>>>>         return 0;
>>>>   + out_free_irq_err:
>>>> +    free_irq(priv->irq_err, dev);
>>>>    out_free_irq_boff:
>>>>       free_irq(priv->irq_boff, dev);
>>>>    out_free_irq:
> 
> (...)

Yours sincerely,
Vincent Mailhol
Ciprian Marian Costea Nov. 19, 2024, 11:40 a.m. UTC | #6 
On 11/19/2024 1:36 PM, Vincent Mailhol wrote:
> On 19/11/2024 at 20:26, Vincent Mailhol wrote:
>> On 19/11/2024 at 19:01, Ciprian Marian Costea wrote:
>>> On 11/19/2024 11:26 AM, Vincent Mailhol wrote:
>>>> On 19/11/2024 at 17:10, Ciprian Costea wrote:
>>
>> (...)
>>
>>>>>    +    if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
>>>>> +        err = request_irq(priv->irq_secondary_mb,
>>>>> +                  flexcan_irq, IRQF_SHARED, dev->name, dev);
>>>>> +        if (err)
>>>>> +            goto out_free_irq_err;
>>>>> +    }
>>>>
>>>> Is the logic here correct?
>>>>
>>>>     request_irq(priv->irq_err, flexcan_irq, IRQF_SHARED, dev->name, dev);
>>>>
>>>> is called only if the device has the FLEXCAN_QUIRK_NR_IRQ_3 quirk.
>>>>
>>>> So, if the device has the FLEXCAN_QUIRK_SECONDARY_MB_IRQ but not the
>>>> FLEXCAN_QUIRK_NR_IRQ_3, you may end up trying to free an irq which was
>>>> not initialized.
>>>>
>>>> Did you confirm if it is safe to call free_irq() on an uninitialized irq?
>>>>
>>>> (and I can see that currently there is no such device with
>>>> FLEXCAN_QUIRK_SECONDARY_MB_IRQ but without FLEXCAN_QUIRK_NR_IRQ_3, but
>>>> who knows if such device will be introduced in the future?)
>>>>
>>>
>>> Hello Vincent,
>>>
>>> Thanks for your review. Indeed this seems to be an incorrect logic since
>>> I do not want to create any dependency between 'FLEXCAN_QUIRK_NR_IRQ_3'
>>> and 'FLEXCAN_QUIRK_SECONDARY_MB_IRQ'.
>>>
>>> I will change the impacted section to:
>>>      if (err) {
>>>          if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>>>              goto out_free_irq_err;
>>>          else
>>>              goto out_free_irq;
>>>      }
>>
>> This is better. Alternatively, you could move the check into the label:
>>
>>    out_free_irq_err:
>>    	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
>>    		free_irq(priv->irq_err, dev);
>>
>> But this is not a strong preference, I let you pick the one which you
>> prefer.
> 
> On second thought, it is a strong preference. If you keep the
> 
> 	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_NR_IRQ_3)
> 		goto out_free_irq_err;
> 	else
> 		goto out_free_irq;
> 
> then what if more code with a clean-up label is added to flexcan_open()?
> I am thinking of this:
> 
>    out_free_foo:
>    	free(foo);
>    out_free_irq_err:
>    	free_irq(priv-irq_err, dev);
>    out_free_irq_boff:
>    	free_irq(priv->irq_boff, dev);
> 
> Jumping to out_free_foo would now be incorrect because the
> out_free_irq_err label would also be visited.
> 

Correct, moving the check under the label would be better. Thanks.
I will change accordingly in V2.


Best Regards,
Ciprian

>>>>>        flexcan_chip_interrupts_enable(dev);
>>>>>          netif_start_queue(dev);
>>>>>          return 0;
>>>>>    + out_free_irq_err:
>>>>> +    free_irq(priv->irq_err, dev);
>>>>>     out_free_irq_boff:
>>>>>        free_irq(priv->irq_boff, dev);
>>>>>     out_free_irq:
>>
>> (...)
> 
> Yours sincerely,
> Vincent Mailhol
>
diff mbox series

Patch

diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
index f0dee04800d3..dc56d4a7d30b 100644
--- a/drivers/net/can/flexcan/flexcan-core.c
+++ b/drivers/net/can/flexcan/flexcan-core.c
@@ -390,9 +390,10 @@  static const struct flexcan_devtype_data nxp_s32g2_devtype_data = {
 	.quirks = FLEXCAN_QUIRK_DISABLE_RXFG | FLEXCAN_QUIRK_ENABLE_EACEN_RRS |
 		FLEXCAN_QUIRK_DISABLE_MECR | FLEXCAN_QUIRK_BROKEN_PERR_STATE |
 		FLEXCAN_QUIRK_USE_RX_MAILBOX | FLEXCAN_QUIRK_SUPPORT_FD |
-		FLEXCAN_QUIRK_SUPPORT_ECC |
+		FLEXCAN_QUIRK_SUPPORT_ECC | FLEXCAN_QUIRK_NR_IRQ_3 |
 		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX |
-		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR,
+		FLEXCAN_QUIRK_SUPPORT_RX_MAILBOX_RTR |
+		FLEXCAN_QUIRK_SECONDARY_MB_IRQ,
 };
 
 static const struct can_bittiming_const flexcan_bittiming_const = {
@@ -1771,12 +1772,21 @@  static int flexcan_open(struct net_device *dev)
 			goto out_free_irq_boff;
 	}
 
+	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
+		err = request_irq(priv->irq_secondary_mb,
+				  flexcan_irq, IRQF_SHARED, dev->name, dev);
+		if (err)
+			goto out_free_irq_err;
+	}
+
 	flexcan_chip_interrupts_enable(dev);
 
 	netif_start_queue(dev);
 
 	return 0;
 
+ out_free_irq_err:
+	free_irq(priv->irq_err, dev);
  out_free_irq_boff:
 	free_irq(priv->irq_boff, dev);
  out_free_irq:
@@ -1808,6 +1818,9 @@  static int flexcan_close(struct net_device *dev)
 		free_irq(priv->irq_boff, dev);
 	}
 
+	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ)
+		free_irq(priv->irq_secondary_mb, dev);
+
 	free_irq(dev->irq, dev);
 	can_rx_offload_disable(&priv->offload);
 	flexcan_chip_stop_disable_on_error(dev);
@@ -2197,6 +2210,14 @@  static int flexcan_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SECONDARY_MB_IRQ) {
+		priv->irq_secondary_mb = platform_get_irq(pdev, 3);
+		if (priv->irq_secondary_mb < 0) {
+			err = priv->irq_secondary_mb;
+			goto failed_platform_get_irq;
+		}
+	}
+
 	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_SUPPORT_FD) {
 		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD |
 			CAN_CTRLMODE_FD_NON_ISO;
diff --git a/drivers/net/can/flexcan/flexcan.h b/drivers/net/can/flexcan/flexcan.h
index 4933d8c7439e..d4b1a954c538 100644
--- a/drivers/net/can/flexcan/flexcan.h
+++ b/drivers/net/can/flexcan/flexcan.h
@@ -70,6 +70,8 @@ 
 #define FLEXCAN_QUIRK_SUPPORT_RX_FIFO BIT(16)
 /* Setup stop mode with ATF SCMI protocol to support wakeup */
 #define FLEXCAN_QUIRK_SETUP_STOP_MODE_SCMI BIT(17)
+/* Setup secondary mailbox interrupt */
+#define FLEXCAN_QUIRK_SECONDARY_MB_IRQ	BIT(18)
 
 struct flexcan_devtype_data {
 	u32 quirks;		/* quirks needed for different IP cores */
@@ -105,6 +107,7 @@  struct flexcan_priv {
 	struct regulator *reg_xceiver;
 	struct flexcan_stop_mode stm;
 
+	int irq_secondary_mb;
 	int irq_boff;
 	int irq_err;