> On Feb 24, 2022, at 2:03 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Thu, Feb 24, 2022 at 06:53:10AM +0000, Vishnu Dasa wrote:
>>
>>> On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:
>>>
>>> Le 11/02/2022 à 15:09, Dan Carpenter a écrit :
>>>> On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:
>
>>>> Whatever... But where this really
>>>> hurts is with:
>>>> Alloc:
>>>> if (vmci_dev->exclusive_vectors) {
>>>> error = request_irq(pci_irq_vector(pdev, 1), ...
>>>> Free:
>>>> free_irq(pci_irq_vector(pdev, 1), vmci_dev);
>>>> No if statement. It works because it's the last allocation but it's
>>>> confusing and fragile.
>>>
>>> Agreed.
>>
>> Sorry, I hope I'm not missing something obvious or misunderstanding the point.
>> But I don't think the problem implied here exists?
>>
>> If 'request_irq(pci_irq_vector(pdev, 0), ...' fails we goto err_disable_msi and there
>> is no free_irq in this path. If 'request_irq(pci_irq_vector(pdev, 1), ...' fails then we
>> goto err_free_irq and we do 'free_irq(pci_irq_vector(pdev, 0), vmci_dev)'. Note that
>> this is for the previous one without the check for vmci_dev->exclusive_vectors.
>
> It's not a bug, but it's bad style.
>
> Ideally the allocation code and the free code should mirror each other
> as much as possible. If there is an if statement in the allocation code
> we should copy that same if statement into the free code. Even if we
> can leave the if statement out, we should still include it for
> readability.
>
> Also if we add another allocation at the end of the function then we
> will have to add the if statement back. Maybe the person who adds the
> if statement will forget. So it's fragile.
No disagreements about that. Making sure I wasn't missing anything
else.
Could we have a separate fix for this which fixes?
"VMCI: dma dg: register dummy IRQ handlers for DMA datagrams"
Thanks!
