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

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

[Heiner Kallweit]

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.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/ata/ahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 2d3d3d67b..09090294c 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1985,7 +1985,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	if (ahci_init_msi(pdev, n_ports, hpriv) < 0) {
 		/* legacy intx interrupts */
-		pci_intx(pdev, 1);
+		pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
 	}
 	hpriv->irq = pci_irq_vector(pdev, 0);
 
-- 
2.47.0

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

[Niklas Cassel]

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/


Kind regards,
Niklas

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

[Heiner Kallweit]

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.
> 
> Kind regards,
> Niklas

Heiner

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

[Niklas Cassel]

On Mon, Nov 04, 2024 at 02:23:43PM +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.

Well, if you look at Philipp's patch it:

1) Doesn't only update drivers/ata/ahci.c,
it also updates:
drivers/ata/ata_piix.c
drivers/ata/pata_rdc.c
drivers/ata/sata_sil24.c
drivers/ata/sata_sis.c
drivers/ata/sata_uli.c
drivers/ata/sata_vsc.c

Why don't you update the other drivers in drivers/ata/* ?


2) Doesn't just bother to fix a single subsystem (drivers/ata/),
it is actually part of a series that fixes all affected subsystems.

Why don't you send out this fix as part of a series that fixes all the
affected subsystems?


Kind regards,
Niklas

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

[Heiner Kallweit]

On 04.11.2024 19:36, Niklas Cassel wrote:
> On Mon, Nov 04, 2024 at 02:23:43PM +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.
> 
> Well, if you look at Philipp's patch it:
> 
> 1) Doesn't only update drivers/ata/ahci.c,
> it also updates:
> drivers/ata/ata_piix.c
> drivers/ata/pata_rdc.c
> drivers/ata/sata_sil24.c
> drivers/ata/sata_sis.c
> drivers/ata/sata_uli.c
> drivers/ata/sata_vsc.c
> 
> Why don't you update the other drivers in drivers/ata/* ?
> 
Because I don't have hw for testing the changes and usually I'm
somewhat reluctant to submit patches which are compile-tested only.

> 
> 2) Doesn't just bother to fix a single subsystem (drivers/ata/),
> it is actually part of a series that fixes all affected subsystems.
> 
> Why don't you send out this fix as part of a series that fixes all the
> affected subsystems?
> 
Because for some drivers it's complex (e.g. bnx2x) and I don't have
hw to test the changes.

> 
> Kind regards,
> Niklas

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

[Philipp Stanner]

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.

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
> 

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

[Heiner Kallweit]

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
>>
>