diff mbox series

ata: ahci: Don't call pci_intx() directly

Message ID c604a8ac-8025-4078-ab90-834d95872e31@gmail.com (mailing list archive)
State Handled Elsewhere
Delegated to: Krzysztof WilczyƄski
Headers show
Series ata: ahci: Don't call pci_intx() directly | expand

Commit Message

Heiner Kallweit Nov. 1, 2024, 10:38 p.m. UTC
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(-)

Comments

Niklas Cassel Nov. 4, 2024, 8:30 a.m. UTC | #1
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
Heiner Kallweit Nov. 4, 2024, 1:23 p.m. UTC | #2
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
Niklas Cassel Nov. 4, 2024, 6:36 p.m. UTC | #3
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
Heiner Kallweit Nov. 4, 2024, 7:40 p.m. UTC | #4
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
diff mbox series

Patch

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