Re: [PATCH 2/3] mei: Migrate to pci_alloc_irq_vectors API

[Heiner Kallweit <hkallweit1@gmail.com>]

On 05.08.2018 13:02, Winkler, Tomas wrote:
>>
>> Update the driver to the pci_alloc_irq_vectors API, this allows to get rid of
>> legacy calls like pci_enable_msi(). Another benefit is that no driver change
>> would be needed in case the hardware starts supporting MSI-X.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Sorry, we are losing the ONE_SHOT flag here,  In spite that pci_enable_msi is marked as deprecated,  
ONE_SHOT flag isn't needed here. See commit f7368a550275 ("PCI: Use IRQF_ONESHOT if
pci_request_irq() called with no handler") and also following comment in __setup_irq():

* Drivers are often written to work w/o knowledge about the
* underlying irq chip implementation, so a request for a
* threaded irq without a primary hard irq context handler
* requires the ONESHOT flag to be set. Some irq chips like
* MSI based interrupts are per se one shot safe. Check the
* chip flags, so we can avoid the unmask dance at the end of
* the threaded handler for those.

> I don't see overall action on LKML to actually  deprecate it.
> For this change I would need to extensice testing on future and legacy devices, frankly  I prefer not to dig into it right now.

Sure, up to you.

> Thanks
> Tomas
> 
Rgds, Heiner

>> ---
>>  drivers/misc/mei/pci-me.c  | 25 +++++++++++--------------
>> drivers/misc/mei/pci-txe.c | 30 ++++++++++++++----------------
>>  2 files changed, 25 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c index
>> 55ba1c41..a60376f0 100644
>> --- a/drivers/misc/mei/pci-me.c
>> +++ b/drivers/misc/mei/pci-me.c
>> @@ -149,7 +149,6 @@ static int mei_me_probe(struct pci_dev *pdev, const
>> struct pci_device_id *ent)
>>  	const struct mei_cfg *cfg;
>>  	struct mei_device *dev;
>>  	struct mei_me_hw *hw;
>> -	unsigned int irqflags;
>>  	int err;
>>
>>  	cfg = mei_me_get_cfg(ent->driver_data); @@ -196,18 +195,16 @@
>> static int mei_me_probe(struct pci_dev *pdev, const struct pci_device_id
>> *ent)
>>  	hw = to_me_hw(dev);
>>  	hw->mem_addr = pcim_iomap_table(pdev)[0];
>>
>> -	pci_enable_msi(pdev);
>> -
>> -	 /* request and enable interrupt */
>> -	irqflags = pci_dev_msi_enabled(pdev) ? IRQF_ONESHOT :
>> IRQF_SHARED;
>> +	err = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
>> +	if (err < 0)
>> +		goto end;
>>
>> -	err = request_threaded_irq(pdev->irq,
>> -			mei_me_irq_quick_handler,
>> -			mei_me_irq_thread_handler,
>> -			irqflags, KBUILD_MODNAME, dev);
>> +	/* request and enable interrupt */
>> +	err = pci_request_irq(pdev, 0, mei_me_irq_quick_handler,
>> +			      mei_me_irq_thread_handler, dev,
>> KBUILD_MODNAME);
>>  	if (err) {
>> -		dev_err(&pdev->dev, "request_threaded_irq failure. irq =
>> %d\n",
>> -		       pdev->irq);
>> +		pci_err(pdev, "pci_request_irq failure. irq = %d\n",
>> +			pci_irq_vector(pdev, 0));
>>  		goto end;
>>  	}
>>
>> @@ -258,7 +255,7 @@ static int mei_me_probe(struct pci_dev *pdev, const
>> struct pci_device_id *ent)
>>  release_irq:
>>  	mei_cancel_work(dev);
>>  	mei_disable_interrupts(dev);
>> -	free_irq(pdev->irq, dev);
>> +	pci_free_irq(pdev, 0, dev);
>>  end:
>>  	dev_err(&pdev->dev, "initialization failed.\n");
>>  	return err;
>> @@ -287,7 +284,7 @@ static void mei_me_shutdown(struct pci_dev *pdev)
>>  	mei_me_unset_pm_domain(dev);
>>
>>  	mei_disable_interrupts(dev);
>> -	free_irq(pdev->irq, dev);
>> +	pci_free_irq(pdev, 0, dev);
>>  }
>>
>>  /**
>> @@ -316,7 +313,7 @@ static void mei_me_remove(struct pci_dev *pdev)
>>
>>  	mei_disable_interrupts(dev);
>>
>> -	free_irq(pdev->irq, dev);
>> +	pci_free_irq(pdev, 0, dev);
>>
>>  	mei_deregister(dev);
>>  }
>> diff --git a/drivers/misc/mei/pci-txe.c b/drivers/misc/mei/pci-txe.c index
>> b8a49581..3ea2b8f6 100644
>> --- a/drivers/misc/mei/pci-txe.c
>> +++ b/drivers/misc/mei/pci-txe.c
>> @@ -64,6 +64,7 @@ static int mei_txe_probe(struct pci_dev *pdev, const
>> struct pci_device_id *ent)  {
>>  	struct mei_device *dev;
>>  	struct mei_txe_hw *hw;
>> +	irq_handler_t qh = NULL;
>>  	const int mask = BIT(SEC_BAR) | BIT(BRIDGE_BAR);
>>  	int err;
>>
>> @@ -100,25 +101,22 @@ static int mei_txe_probe(struct pci_dev *pdev,
>> const struct pci_device_id *ent)
>>  	hw = to_txe_hw(dev);
>>  	hw->mem_addr = pcim_iomap_table(pdev);
>>
>> -	pci_enable_msi(pdev);
>> +	err = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
>> +	if (err < 0)
>> +		goto end;
>>
>>  	/* clear spurious interrupts */
>>  	mei_clear_interrupts(dev);
>>
>> +	if (!pci_dev_msi_enabled(pdev))
>> +		qh = mei_txe_irq_quick_handler;
>> +
>>  	/* request and enable interrupt  */
>> -	if (pci_dev_msi_enabled(pdev))
>> -		err = request_threaded_irq(pdev->irq,
>> -			NULL,
>> -			mei_txe_irq_thread_handler,
>> -			IRQF_ONESHOT, KBUILD_MODNAME, dev);
>> -	else
>> -		err = request_threaded_irq(pdev->irq,
>> -			mei_txe_irq_quick_handler,
>> -			mei_txe_irq_thread_handler,
>> -			IRQF_SHARED, KBUILD_MODNAME, dev);
>> +	err = pci_request_irq(pdev, 0, qh, mei_txe_irq_thread_handler,
>> +			      dev, KBUILD_MODNAME);
>>  	if (err) {
>> -		dev_err(&pdev->dev, "mei: request_threaded_irq failure. irq
>> = %d\n",
>> -			pdev->irq);
>> +		pci_err(pdev, "mei: pci_request_irq failure. irq = %d\n",
>> +			pci_irq_vector(pdev, 0));
>>  		goto end;
>>  	}
>>
>> @@ -162,7 +160,7 @@ static int mei_txe_probe(struct pci_dev *pdev, const
>> struct pci_device_id *ent)
>>  release_irq:
>>  	mei_cancel_work(dev);
>>  	mei_disable_interrupts(dev);
>> -	free_irq(pdev->irq, dev);
>> +	pci_free_irq(pdev, 0, dev);
>>  end:
>>  	dev_err(&pdev->dev, "initialization failed.\n");
>>  	return err;
>> @@ -191,7 +189,7 @@ static void mei_txe_shutdown(struct pci_dev *pdev)
>>  	mei_txe_unset_pm_domain(dev);
>>
>>  	mei_disable_interrupts(dev);
>> -	free_irq(pdev->irq, dev);
>> +	pci_free_irq(pdev, 0, dev);
>>  }
>>
>>  /**
>> @@ -219,7 +217,7 @@ static void mei_txe_remove(struct pci_dev *pdev)
>>  	mei_txe_unset_pm_domain(dev);
>>
>>  	mei_disable_interrupts(dev);
>> -	free_irq(pdev->irq, dev);
>> +	pci_free_irq(pdev, 0, dev);
>>
>>  	mei_deregister(dev);
>>  }
>> --
>> 2.18.0
>>
>