Re: [PATCH 1/4] vdpa/octeon_ep: enable support for multiple interrupts per device

[Shijith Thotton]

>> @@ -63,44 +80,53 @@ static irqreturn_t octep_vdpa_intr_handler(int irq,
>void *data)
>>  static void octep_free_irqs(struct octep_hw *oct_hw)
>>  {
>>  	struct pci_dev *pdev = oct_hw->pdev;
>> +	int irq;
>> +
>> +	for (irq = 0; irq < oct_hw->nb_irqs && oct_hw->irqs; irq++) {
>> +		if (oct_hw->irqs[irq] < 0)
>> +			continue;
>>
>> -	if (oct_hw->irq != -1) {
>> -		devm_free_irq(&pdev->dev, oct_hw->irq, oct_hw);
>> -		oct_hw->irq = -1;
>> +		devm_free_irq(&pdev->dev, oct_hw->irqs[irq], oct_hw);
>>  	}
>> +
>>  	pci_free_irq_vectors(pdev);
>> +	kfree(oct_hw->irqs);
>
>You should add:
>
>	oct_hw->nb_irqs = 0;
>	oct_hw->irqs = NULL;
>
>Otherwise if reset is called twice in a row, before re-initializing the IRQs it
>results in a use after free.
>

I will fix this.

>>  }
>>
>>  static int octep_request_irqs(struct octep_hw *oct_hw)
>>  {
>>  	struct pci_dev *pdev = oct_hw->pdev;
>> -	int ret, irq;
>> +	int ret, irq, idx;
>>
>> -	/* Currently HW device provisions one IRQ per VF, hence
>> -	 * allocate one IRQ for all virtqueues call interface.
>> -	 */
>> -	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSIX);
>> +	ret = pci_alloc_irq_vectors(pdev, 1, oct_hw->nb_irqs, PCI_IRQ_MSIX);
>>  	if (ret < 0) {
>>  		dev_err(&pdev->dev, "Failed to alloc msix vector");
>>  		return ret;
>>  	}
>>
>> -	snprintf(oct_hw->vqs->msix_name, sizeof(oct_hw->vqs->msix_name),
>> -		 OCTEP_VDPA_DRIVER_NAME "-vf-%d", pci_iov_vf_id(pdev));
>> +	oct_hw->irqs = kcalloc(oct_hw->nb_irqs, sizeof(int), GFP_KERNEL);
>
>This isn't free on the ->release() path or whatever.  octep_free_irqs() is
>called on reset() but we rely on devm_ to free the IRQs on ->release().  Use
>devm_kcalloc() here as well, probably.
>

Sure. I will change to devm_kcalloc().

>> +	if (!oct_hw->irqs) {
>> +		ret = -ENOMEM;
>> +		goto free_irqs;
>> +	}
>>
>> -	irq = pci_irq_vector(pdev, 0);
>> -	ret = devm_request_irq(&pdev->dev, irq, octep_vdpa_intr_handler, 0,
>> -			       oct_hw->vqs->msix_name, oct_hw);
>> -	if (ret) {
>> -		dev_err(&pdev->dev, "Failed to register interrupt handler\n");
>> -		goto free_irq_vec;
>> +	memset(oct_hw->irqs, -1, sizeof(oct_hw->irqs));
>
>This works, but it would be more normal to just leave it zeroed and check for
>zero instead of checking for negatives.  There is never a zero IRQ.  See my blog
>for more details:

I assumed IRQ number 0 was valid since I saw it listed for the timer in  
`/proc/interrupts`. However, it might be legacy. I will update the code
to check against 0 instead of -1.

>
>> +
>> +	for (idx = 0; idx < oct_hw->nb_irqs; idx++) {
>> +		irq = pci_irq_vector(pdev, idx);
>> +		ret = devm_request_irq(&pdev->dev, irq,
>octep_vdpa_intr_handler, 0,
>> +				       dev_name(&pdev->dev), oct_hw);
>> +		if (ret) {
>> +			dev_err(&pdev->dev, "Failed to register interrupt
>handler\n");
>> +			goto free_irqs;
>> +		}
>> +		oct_hw->irqs[idx] = irq;
>>  	}
>> -	oct_hw->irq = irq;
>>
>>  	return 0;
>>
>> -free_irq_vec:
>> -	pci_free_irq_vectors(pdev);
>> +free_irqs:
>> +	octep_free_irqs(oct_hw);
>>  	return ret;
>>  }
>>

Thanks,
Shijith