Re: [PATCH] ata: ahci: Don't call pci_intx() directly

[Heiner Kallweit <hkallweit1@gmail.com>]

On 05.11.2024 16:22, Philipp Stanner wrote:
> On Mon, 2024-11-04 at 14:23 +0100, Heiner Kallweit wrote:
>> On 04.11.2024 09:30, Niklas Cassel wrote:
>>> On Fri, Nov 01, 2024 at 11:38:53PM +0100, Heiner Kallweit wrote:
>>>> pci_intx() should be called by PCI core and some virtualization
>>>> code
>>>> only. In PCI device drivers use the appropriate
>>>> pci_alloc_irq_vectors()
>>>> call.
>>>
>>> Hello Heiner,
>>>
>>> as you might or might not know, this patch conflicts with a
>>> Philipp's
>>> already acked patch:
>>> https://lore.kernel.org/linux-ide/20241015185124.64726-10-pstanner@redhat.com/
>>>
>> I know, therefore he's on cc. Fully migrating PCI device drivers to
>> the
>> pci_alloc_irq_vectors() should be done anyway and is the cleaner
>> alternative to changing pci_intx(). However for some drivers this is
>> a rather
>> complex task, therefore I understand Philipp's approach to adjust
>> pci_intx()
>> first. He's incorporating other review feedback in his series, so
>> with the
>> next re-spin he could remove the ahci patch from his series.
> 
> As I have stated before, this is not just about cleaning up pci_intx().
> 
> Again:
> pci_alloc_irq_vectors() USES pci_intx(), and my series does address
> that in its MSI patch.
> 
That's clear. The point is that in the end only PCI core and some
virtualization stuff should use a low-level function like pci_intx().
Device drivers should never use pci_intx() directly, managed or not.

I think all the hassle with managed intx could be avoided if PCI core
would unconditionally reset register PCI_COMMAND (or at least the most
relevant bits) to the initial value on driver exit.

> If you want to help, a careful review of the MSI bits would be helpful.
> Your patch here uses pci_intx(), you just don't see it anymore.
> 
> That said, in principle it's of course possible for me to drop patches
> while we're still in review, but I tend to think that it's causing both
> you and me more work if the pci_intx() vs. pci_alloc_irq_vectors() is
> worked on out of all times right now.
> 
> It also causes more work load for the reviewers, since they'd have
> reviewed my patch for nothing and would have to review yours then.
> 
> Also be aware that I am not yet sure whether the devres aspect should
> also be removed or made more explicit in MSI. Take a look at
> pcim_setup_msi_release().
> 
> In the worst case you might just replace one problem with another
> before we figured it all out.
> 
> Regards,
> P.
> 
>>>
>>> Kind regards,
>>> Niklas
>>
>> Heiner
>>
>