| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bfa5200d-6e56-417d-ac3b-52390398dba2@oss.nxp.com>
Date: Tue, 19 Nov 2024 12:01:46 +0200
From: Ciprian Marian Costea <ciprianmarian.costea@....nxp.com>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>,
Marc Kleine-Budde <mkl@...gutronix.de>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S . Miller" <davem@...emloft.net>, Eric Dumazet
<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: linux-can@...r.kernel.org, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
imx@...ts.linux.dev, NXP Linux Team <s32@....com>,
Christophe Lizzi <clizzi@...hat.com>, Alberto Ruiz <aruizrui@...hat.com>,
Enric Balletbo <eballetb@...hat.com>
Subject: Re: [PATCH 3/3] can: flexcan: handle S32G2/S32G3 separate interrupt
lines
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@....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@....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
>
Powered by blists - more mailing lists