From: Michael Ellerman <mpe@ellerman.id.au> Date: 2022-10-12 06:21:40
"Michael S. Tsirkin" [off-list ref] writes:
The following changes since commit 4fe89d07dcc2804c8b562f6c7896a45643d34b2f:
Linux 6.0 (2022-10-02 14:09:07 -0700)
are available in the Git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to 71491c54eafa318fdd24a1f26a1c82b28e1ac21d:
virtio_pci: don't try to use intxif pin is zero (2022-10-07 20:00:44 -0400)
----------------------------------------------------------------
virtio: fixes, features
9k mtu perf improvements
vdpa feature provisioning
virtio blk SECURE ERASE support
Fixes, cleanups all over the place.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Alvaro Karsz (1):
virtio_blk: add SECURE ERASE command support
Angus Chen (1):
virtio_pci: don't try to use intxif pin is zero
This commit breaks virtio_pci for me on powerpc, when running as a qemu
guest.
vp_find_vqs() bails out because pci_dev->pin == 0.
But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
succeed if we called it - which is what the code used to do.
I think this happens because pci_dev->pin is not populated in
pci_assign_irq().
I would absolutely believe this is bug in our PCI code, but I think it
may also affect other platforms that use of_irq_parse_and_map_pci().
cheers
virtio_pci: don't try to use intxif pin is zero (2022-10-07 20:00:44 -0400)
----------------------------------------------------------------
virtio: fixes, features
9k mtu perf improvements
vdpa feature provisioning
virtio blk SECURE ERASE support
Fixes, cleanups all over the place.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Alvaro Karsz (1):
virtio_blk: add SECURE ERASE command support
Angus Chen (1):
virtio_pci: don't try to use intxif pin is zero
This commit breaks virtio_pci for me on powerpc, when running as a qemu
guest.
vp_find_vqs() bails out because pci_dev->pin == 0.
But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
succeed if we called it - which is what the code used to do.
I think this happens because pci_dev->pin is not populated in
pci_assign_irq().
Yes,you are right.
I would absolutely believe this is bug in our PCI code, but I think it
may also affect other platforms that use of_irq_parse_and_map_pci().
virtio_pci: don't try to use intxif pin is zero (2022-10-07 20:00:44 -0400)
----------------------------------------------------------------
virtio: fixes, features
9k mtu perf improvements
vdpa feature provisioning
virtio blk SECURE ERASE support
Fixes, cleanups all over the place.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Alvaro Karsz (1):
virtio_blk: add SECURE ERASE command support
Angus Chen (1):
virtio_pci: don't try to use intxif pin is zero
This commit breaks virtio_pci for me on powerpc, when running as a qemu
guest.
vp_find_vqs() bails out because pci_dev->pin == 0.
But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
succeed if we called it - which is what the code used to do.
I think this happens because pci_dev->pin is not populated in
pci_assign_irq().
I would absolutely believe this is bug in our PCI code, but I think it
may also affect other platforms that use of_irq_parse_and_map_pci().
cheers
HI,sorry for reply again. If I change the code like blew:
pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin);
if (!pin) {
warn_on("some thing");
return 0;
}
It will fix the orign bug.
Or we should populated the pci_dev->pin value correctly according to PCI spec about "Interrupt Pin" Register.
I have no idea about it, any suggestions are welcome.
Thank you.
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2022-10-12 11:11:20
On Wed, Oct 12, 2022 at 05:21:24PM +1100, Michael Ellerman wrote:
"Michael S. Tsirkin" [off-list ref] writes:
quoted
The following changes since commit 4fe89d07dcc2804c8b562f6c7896a45643d34b2f:
Linux 6.0 (2022-10-02 14:09:07 -0700)
are available in the Git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to 71491c54eafa318fdd24a1f26a1c82b28e1ac21d:
virtio_pci: don't try to use intxif pin is zero (2022-10-07 20:00:44 -0400)
----------------------------------------------------------------
virtio: fixes, features
9k mtu perf improvements
vdpa feature provisioning
virtio blk SECURE ERASE support
Fixes, cleanups all over the place.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Alvaro Karsz (1):
virtio_blk: add SECURE ERASE command support
Angus Chen (1):
virtio_pci: don't try to use intxif pin is zero
This commit breaks virtio_pci for me on powerpc, when running as a qemu
guest.
vp_find_vqs() bails out because pci_dev->pin == 0.
But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
succeed if we called it - which is what the code used to do.
I think this happens because pci_dev->pin is not populated in
pci_assign_irq().
I would absolutely believe this is bug in our PCI code, but I think it
may also affect other platforms that use of_irq_parse_and_map_pci().
cheers
How about fixing this in of_irq_parse_and_map_pci then?
Something like the below maybe?
@@ -446,6 +446,8 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *if(pin==0)return-ENODEV;+pdev->pin=pin;+/* Local interrupt-map in the device node? Use it! */if(of_get_property(dn,"interrupt-map",NULL)){pin=pci_swizzle_interrupt_pin(pdev,pin);
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2022-10-12 13:28:46
[ Cc += Bjorn & linux-pci ]
"Michael S. Tsirkin" [off-list ref] writes:
On Wed, Oct 12, 2022 at 05:21:24PM +1100, Michael Ellerman wrote:
quoted
"Michael S. Tsirkin" [off-list ref] writes:
...
quoted hunk
quoted
quoted
----------------------------------------------------------------
virtio: fixes, features
9k mtu perf improvements
vdpa feature provisioning
virtio blk SECURE ERASE support
Fixes, cleanups all over the place.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Alvaro Karsz (1):
virtio_blk: add SECURE ERASE command support
Angus Chen (1):
virtio_pci: don't try to use intxif pin is zero
This commit breaks virtio_pci for me on powerpc, when running as a qemu
guest.
vp_find_vqs() bails out because pci_dev->pin == 0.
But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
succeed if we called it - which is what the code used to do.
I think this happens because pci_dev->pin is not populated in
pci_assign_irq().
I would absolutely believe this is bug in our PCI code, but I think it
may also affect other platforms that use of_irq_parse_and_map_pci().
How about fixing this in of_irq_parse_and_map_pci then?
Something like the below maybe?
@@ -446,6 +446,8 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *if(pin==0)return-ENODEV;+pdev->pin=pin;+/* Local interrupt-map in the device node? Use it! */if(of_get_property(dn,"interrupt-map",NULL)){pin=pci_swizzle_interrupt_pin(pdev,pin);
That doesn't fix it in all cases, because there's an early return if
there's a struct device_node associated with the pci_dev, before we even
read the pin.
Also the pci_dev is const, and removing the const would propagate to a
few other places.
The other obvious place to fix it would be in pci_assign_irq(), as
below. That fixes this bug for me, but is otherwise very lightly tested.
cheers
@@ -22,6 +22,15 @@ void pci_assign_irq(struct pci_dev *dev)intirq=0;structpci_host_bridge*hbrg=pci_find_host_bridge(dev->bus);+/* Make sure dev->pin is populated */+pci_read_config_byte(dev,PCI_INTERRUPT_PIN,&pin);++/* Cope with illegal. */+if(pin>4)+pin=1;++dev->pin=pin;+if(!(hbrg->map_irq)){pci_dbg(dev,"runtime IRQ mapping not provided by arch\n");return;
@@ -34,11 +43,6 @@ void pci_assign_irq(struct pci_dev *dev)*timetheinterruptlinepassesthroughaPCI-PCIbridgewemust*applytheswizzlefunction.*/-pci_read_config_byte(dev,PCI_INTERRUPT_PIN,&pin);-/* Cope with illegal. */-if(pin>4)-pin=1;-if(pin){/* Follow the chain of bridges, swizzling as we go. */if(hbrg->swizzle_irq)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2022-10-12 14:34:14
Michael Ellerman [off-list ref] writes:
[ Cc += Bjorn & linux-pci ]
"Michael S. Tsirkin" [off-list ref] writes:
quoted
On Wed, Oct 12, 2022 at 05:21:24PM +1100, Michael Ellerman wrote:
quoted
"Michael S. Tsirkin" [off-list ref] writes:
...
quoted
quoted
quoted
----------------------------------------------------------------
virtio: fixes, features
9k mtu perf improvements
vdpa feature provisioning
virtio blk SECURE ERASE support
Fixes, cleanups all over the place.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Alvaro Karsz (1):
virtio_blk: add SECURE ERASE command support
Angus Chen (1):
virtio_pci: don't try to use intxif pin is zero
This commit breaks virtio_pci for me on powerpc, when running as a qemu
guest.
vp_find_vqs() bails out because pci_dev->pin == 0.
But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
succeed if we called it - which is what the code used to do.
I think this happens because pci_dev->pin is not populated in
pci_assign_irq().
I would absolutely believe this is bug in our PCI code, but I think it
may also affect other platforms that use of_irq_parse_and_map_pci().
How about fixing this in of_irq_parse_and_map_pci then?
Something like the below maybe?
@@ -446,6 +446,8 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *if(pin==0)return-ENODEV;+pdev->pin=pin;+/* Local interrupt-map in the device node? Use it! */if(of_get_property(dn,"interrupt-map",NULL)){pin=pci_swizzle_interrupt_pin(pdev,pin);
Backing up a bit. Should the virtio code be looking at pci_dev->pin in
the first place?
Shouldn't it be checking pci_dev->irq instead?
The original commit talks about irq being 0 and colliding with the timer
interrupt.
But all (most?) platforms have converged on 0 meaning NO_IRQ since quite
a fews ago AFAIK.
And the timer irq == 0 is a special case AIUI:
https://lore.kernel.org/all/CA+55aFwiLp1z+2mzkrFsid1WZQ0TQkcn8F2E6NL_AVR+m1fZ2w@mail.gmail.com/
cheers
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2022-10-12 15:51:55
On Thu, Oct 13, 2022 at 01:33:59AM +1100, Michael Ellerman wrote:
Michael Ellerman [off-list ref] writes:
quoted
[ Cc += Bjorn & linux-pci ]
"Michael S. Tsirkin" [off-list ref] writes:
quoted
On Wed, Oct 12, 2022 at 05:21:24PM +1100, Michael Ellerman wrote:
quoted
"Michael S. Tsirkin" [off-list ref] writes:
...
quoted
quoted
quoted
----------------------------------------------------------------
virtio: fixes, features
9k mtu perf improvements
vdpa feature provisioning
virtio blk SECURE ERASE support
Fixes, cleanups all over the place.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Alvaro Karsz (1):
virtio_blk: add SECURE ERASE command support
Angus Chen (1):
virtio_pci: don't try to use intxif pin is zero
This commit breaks virtio_pci for me on powerpc, when running as a qemu
guest.
vp_find_vqs() bails out because pci_dev->pin == 0.
But pci_dev->irq is populated correctly, so vp_find_vqs_intx() would
succeed if we called it - which is what the code used to do.
I think this happens because pci_dev->pin is not populated in
pci_assign_irq().
I would absolutely believe this is bug in our PCI code, but I think it
may also affect other platforms that use of_irq_parse_and_map_pci().
How about fixing this in of_irq_parse_and_map_pci then?
Something like the below maybe?
@@ -446,6 +446,8 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *if(pin==0)return-ENODEV;+pdev->pin=pin;+/* Local interrupt-map in the device node? Use it! */if(of_get_property(dn,"interrupt-map",NULL)){pin=pci_swizzle_interrupt_pin(pdev,pin);
Backing up a bit. Should the virtio code be looking at pci_dev->pin in
the first place?
Shouldn't it be checking pci_dev->irq instead?
The original commit talks about irq being 0 and colliding with the timer
interrupt.
But all (most?) platforms have converged on 0 meaning NO_IRQ since quite
a fews ago AFAIK.
Are you sure?
arch/arm/include/asm/irq.h:#ifndef NO_IRQ
arch/arm/include/asm/irq.h:#define NO_IRQ ((unsigned int)(-1))
that I've tried to get rid of for years, but for some reason it just won't die.
NO_IRQ should be zero. Or rather, it shouldn't exist at all. It's a bogus thing.
You can see just how bogus it is from grepping for it - the users are
all completely and utterly confused, and all are entirely historical
brokenness.
The correct way to check for "no irq" doesn't use NO_IRQ at all, it just does
if (dev->irq) ...
which is why you will only find a few instances of NO_IRQ in the tree
in the first place.
The NO_IRQ thing is mainly actually defined by a few drivers that just
never got converted to the proper world order, and even then you can
see the confusion (ie some drivers use "-1", others use "0", and yet
others use "((unsigned int)(-1)".
Linus
On Wed, Oct 12, 2022, at 7:22 PM, Linus Torvalds wrote:
The NO_IRQ thing is mainly actually defined by a few drivers that just
never got converted to the proper world order, and even then you can
see the confusion (ie some drivers use "-1", others use "0", and yet
others use "((unsigned int)(-1)".
The last time I looked at removing it for arch/arm/, one problem was
that there were a number of platforms using IRQ 0 as a valid number.
We have converted most of them in the meantime, leaving now only
mach-rpc and mach-footbridge. For the other platforms, we just
renumbered all interrupts to add one, but footbridge apparently
relies on hardcoded ISA interrupts in device drivers. For rpc,
it looks like IRQ 0 (printer) already wouldn't work, and it
looks like there was never a driver referencing it either.
I see that openrisc and parisc also still define NO_IRQ to -1, but at
least openrisc already relies on 0 being the invalid IRQ (from
CONFIG_IRQ_DOMAIN), probably parisc as well.
Arnd
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2022-10-12 22:10:40
On Wed, Oct 12, 2022 at 11:06:54PM +0200, Arnd Bergmann wrote:
On Wed, Oct 12, 2022, at 7:22 PM, Linus Torvalds wrote:
quoted
The NO_IRQ thing is mainly actually defined by a few drivers that just
never got converted to the proper world order, and even then you can
see the confusion (ie some drivers use "-1", others use "0", and yet
others use "((unsigned int)(-1)".
The last time I looked at removing it for arch/arm/, one problem was
that there were a number of platforms using IRQ 0 as a valid number.
We have converted most of them in the meantime, leaving now only
mach-rpc and mach-footbridge. For the other platforms, we just
renumbered all interrupts to add one, but footbridge apparently
relies on hardcoded ISA interrupts in device drivers. For rpc,
it looks like IRQ 0 (printer) already wouldn't work, and it
looks like there was never a driver referencing it either.
Do these two boxes even have pci?
I see that openrisc and parisc also still define NO_IRQ to -1, but at
least openrisc already relies on 0 being the invalid IRQ (from
CONFIG_IRQ_DOMAIN), probably parisc as well.
Arnd
On Thu, Oct 13, 2022, at 12:08 AM, Michael S. Tsirkin wrote:
On Wed, Oct 12, 2022 at 11:06:54PM +0200, Arnd Bergmann wrote:
quoted
On Wed, Oct 12, 2022, at 7:22 PM, Linus Torvalds wrote:
quoted
The NO_IRQ thing is mainly actually defined by a few drivers that just
never got converted to the proper world order, and even then you can
see the confusion (ie some drivers use "-1", others use "0", and yet
others use "((unsigned int)(-1)".
The last time I looked at removing it for arch/arm/, one problem was
that there were a number of platforms using IRQ 0 as a valid number.
We have converted most of them in the meantime, leaving now only
mach-rpc and mach-footbridge. For the other platforms, we just
renumbered all interrupts to add one, but footbridge apparently
relies on hardcoded ISA interrupts in device drivers. For rpc,
it looks like IRQ 0 (printer) already wouldn't work, and it
looks like there was never a driver referencing it either.
Do these two boxes even have pci?
Footbridge/netwinder has PCI and PC-style ISA on-board devices
(floppy, ps2 mouse/keyboard, parport, soundblaster, ...), RiscPC
has neither.
Arnd
On Wed, Oct 12, 2022 at 11:29 PM Arnd Bergmann [off-list ref] wrote:
On Thu, Oct 13, 2022, at 12:08 AM, Michael S. Tsirkin wrote:
quoted
Do these two boxes even have pci?
Footbridge/netwinder has PCI and PC-style ISA on-board devices
(floppy, ps2 mouse/keyboard, parport, soundblaster, ...), RiscPC
has neither.
It's worth noting that changing a driver that does
if (dev->irq == NO_IRQ)
return -ENODEV;
to use
if (!dev->irq)
return -ENODEV;
should be pretty much always fine.
Even *if* that driver is then compiled and used on an architecture
where NO_IRQ is one of the odd values, you end up having only two
cases
(a) irq 0 was actually a valid irq after all
(b) you just get the error later when actually trying to use the odd
NO_IRQ interrupt with request_irq() and friends
and here (a) basically never happens - certainly not for any PCI setup
- and (b) is harmless unless the driver was already terminally broken
anyway.
The one exception for (a) might be some platform irq code. On x86,
that would be the legacy timer interrupt, of course.
So if some odd platform actually has a "real" interrupt on irq0, that
platform should either just fix the irq number mapping, or should
consider that interrupt to be a platform-specific thing and handle it
very very specially.
On x86, for example, we do
if (request_irq(0, timer_interrupt, flags, "timer", NULL))
early in boot, and that's basically what then makes sure that no
driver can get that irq. It's done through the platform "timer_init"
code at the "late_time_init()" call.
(And that "late_time_init()" - despite the name - isn't very late at
all. It's just later than the very early timekeeping init - after
interrupts have been enabled at all.
Linus