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
