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