pci-legacy.c under MIPS has a copy of pci_enable_resources() named as
pcibios_enable_resources(). Having own copy of same functionality could
lead to inconsistencies in behavior, especially now as
pci_enable_resources() and the bridge window resource flags behavior are
going to be altered by upcoming changes.

The check for !r->start && r->end is already covered by the more generic
checks done in pci_enable_resources().

Call pci_enable_resources() from MIPS's pcibios_enable_device() and remove
pcibios_enable_resources().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
 arch/mips/pci/pci-legacy.c | 38 ++------------------------------------
 1 file changed, 2 insertions(+), 36 deletions(-)

diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c
index 66898fd182dc..d04b7c1294b6 100644
--- a/arch/mips/pci/pci-legacy.c
+++ b/arch/mips/pci/pci-legacy.c
@@ -249,45 +249,11 @@ static int __init pcibios_init(void)
 
 subsys_initcall(pcibios_init);
 
-static int pcibios_enable_resources(struct pci_dev *dev, int mask)
-{
-	u16 cmd, old_cmd;
-	int idx;
-	struct resource *r;
-
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	old_cmd = cmd;
-	pci_dev_for_each_resource(dev, r, idx) {
-		/* Only set up the requested stuff */
-		if (!(mask & (1<<idx)))
-			continue;
-
-		if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
-			continue;
-		if ((idx == PCI_ROM_RESOURCE) &&
-				(!(r->flags & IORESOURCE_ROM_ENABLE)))
-			continue;
-		if (!r->start && r->end) {
-			pci_err(dev,
-				"can't enable device: resource collisions\n");
-			return -EINVAL;
-		}
-		if (r->flags & IORESOURCE_IO)
-			cmd |= PCI_COMMAND_IO;
-		if (r->flags & IORESOURCE_MEM)
-			cmd |= PCI_COMMAND_MEMORY;
-	}
-	if (cmd != old_cmd) {
-		pci_info(dev, "enabling device (%04x -> %04x)\n", old_cmd, cmd);
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	return 0;
-}
-
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
-	int err = pcibios_enable_resources(dev, mask);
+	int err;
 
+	err = pci_enable_resources(dev, mask);
 	if (err < 0)
 		return err;
 
-- 
2.39.5

Hi,

On Fri, Aug 29, 2025 at 04:10:52PM +0300, Ilpo Järvinen wrote:
> pci-legacy.c under MIPS has a copy of pci_enable_resources() named as
> pcibios_enable_resources(). Having own copy of same functionality could
> lead to inconsistencies in behavior, especially now as
> pci_enable_resources() and the bridge window resource flags behavior are
> going to be altered by upcoming changes.
> 
> The check for !r->start && r->end is already covered by the more generic
> checks done in pci_enable_resources().
> 
> Call pci_enable_resources() from MIPS's pcibios_enable_device() and remove
> pcibios_enable_resources().
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

This patch causes boot failures when trying to boot mips images from
ide drive in qemu. As far as I can see the interface no longer instantiates.

Reverting this patch fixes the problem. Bisect log attached for reference.

Guenter

---
# bad: [3a8660878839faadb4f1a6dd72c3179c1df56787] Linux 6.18-rc1
# good: [e5f0a698b34ed76002dc5cff3804a61c80233a7a] Linux 6.17
git bisect start 'HEAD' 'v6.17'
# good: [58809f614e0e3f4e12b489bddf680bfeb31c0a20] Merge tag 'drm-next-2025-10-01' of https://gitlab.freedesktop.org/drm/kernel
git bisect good 58809f614e0e3f4e12b489bddf680bfeb31c0a20
# good: [bed0653fe2aacb0ca8196075cffc9e7062e74927] Merge tag 'iommu-updates-v6.18' of git://git.kernel.org/pub/scm/linux/kernel/git/iommu/linux
git bisect good bed0653fe2aacb0ca8196075cffc9e7062e74927
# good: [6a74422b9710e987c7d6b85a1ade7330b1e61626] Merge tag 'mips_6.18' of git://git.kernel.org/pub/scm/linux/kernel/git/mips/linux
git bisect good 6a74422b9710e987c7d6b85a1ade7330b1e61626
# bad: [522ba450b56fff29f868b1552bdc2965f55de7ed] Merge tag 'clk-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux
git bisect bad 522ba450b56fff29f868b1552bdc2965f55de7ed
# bad: [256e3417065b2721f77bcd37331796b59483ef3b] Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm
git bisect bad 256e3417065b2721f77bcd37331796b59483ef3b
# bad: [2f2c7254931f41b5736e3ba12aaa9ac1bbeeeb92] Merge tag 'pci-v6.18-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci
git bisect bad 2f2c7254931f41b5736e3ba12aaa9ac1bbeeeb92
# bad: [531abff0fa53bc3a2f7f69b2693386eb6bda96e5] Merge branch 'pci/controller/qcom'
git bisect bad 531abff0fa53bc3a2f7f69b2693386eb6bda96e5
# bad: [fead6a0b15bf3b33dba877efec6b4e7b4cc4abc3] Merge branch 'pci/resource'
git bisect bad fead6a0b15bf3b33dba877efec6b4e7b4cc4abc3
# good: [0bb65e32495e6235a069b60e787140da99e9c122] Merge branch 'pci/p2pdma'
git bisect good 0bb65e32495e6235a069b60e787140da99e9c122
# bad: [ebe091ad81e1d3e5cbb1592ebc18175b5ca3d2bd] PCI: Use pbus_select_window_for_type() during IO window sizing
git bisect bad ebe091ad81e1d3e5cbb1592ebc18175b5ca3d2bd
# bad: [2ee33aa14d3f2e92ba8ae80443f2cd9b575f08cb] PCI: Always claim bridge window before its setup
git bisect bad 2ee33aa14d3f2e92ba8ae80443f2cd9b575f08cb
# good: [2657a0c982239fecc41d0df5a69091ca4297647c] m68k/PCI: Use pci_enable_resources() in pcibios_enable_device()
git bisect good 2657a0c982239fecc41d0df5a69091ca4297647c
# bad: [ae81aad5c2e17fd1fafd930e75b81aedc837f705] MIPS: PCI: Use pci_enable_resources()
git bisect bad ae81aad5c2e17fd1fafd930e75b81aedc837f705
# good: [754babaaf33349d9ef27bb1ac6f5d9d5a503a2a6] sparc/PCI: Remove pcibios_enable_device() as they do nothing extra
git bisect good 754babaaf33349d9ef27bb1ac6f5d9d5a503a2a6
# first bad commit: [ae81aad5c2e17fd1fafd930e75b81aedc837f705] MIPS: PCI: Use pci_enable_resources()

On Mon, Oct 13, 2025 at 12:54:25PM -0700, Guenter Roeck wrote:
> Hi,
> 
> On Fri, Aug 29, 2025 at 04:10:52PM +0300, Ilpo Järvinen wrote:
> > pci-legacy.c under MIPS has a copy of pci_enable_resources() named as
> > pcibios_enable_resources(). Having own copy of same functionality could
> > lead to inconsistencies in behavior, especially now as
> > pci_enable_resources() and the bridge window resource flags behavior are
> > going to be altered by upcoming changes.
> > 
> > The check for !r->start && r->end is already covered by the more generic
> > checks done in pci_enable_resources().
> > 
> > Call pci_enable_resources() from MIPS's pcibios_enable_device() and remove
> > pcibios_enable_resources().
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> 
> This patch causes boot failures when trying to boot mips images from
> ide drive in qemu. As far as I can see the interface no longer instantiates.
> 
> Reverting this patch fixes the problem. Bisect log attached for reference.

Patch below fixes my qemu malta setup. Now I'm wondering, why this is
needed. It was added with commit

aa0980b80908 ("Fixes for system controllers for Atlas/Malta core cards.")

Maciej, do you remember why this is needed ?

Thomas.

diff --git a/arch/mips/pci/pci-malta.c b/arch/mips/pci/pci-malta.c
index 6aefdf20ca05..e0270b818b03 100644
--- a/arch/mips/pci/pci-malta.c
+++ b/arch/mips/pci/pci-malta.c
@@ -229,10 +229,6 @@ void __init mips_pcibios_init(void)
 		return;
 	}
 
-	/* PIIX4 ACPI starts at 0x1000 */
-	if (controller->io_resource->start < 0x00001000UL)
-		controller->io_resource->start = 0x00001000UL;
-
 	iomem_resource.end &= 0xfffffffffULL;			/* 64 GB */
 	ioport_resource.end = controller->io_resource->end;
 
-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

On Mon, 13 Oct 2025, Thomas Bogendoerfer wrote:

> > This patch causes boot failures when trying to boot mips images from
> > ide drive in qemu. As far as I can see the interface no longer instantiates.
> > 
> > Reverting this patch fixes the problem. Bisect log attached for reference.
> 
> Patch below fixes my qemu malta setup. Now I'm wondering, why this is
> needed. It was added with commit
> 
> aa0980b80908 ("Fixes for system controllers for Atlas/Malta core cards.")
> 
> Maciej, do you remember why this is needed ?

 I do.  The reason is preventing PCI port I/O mappings below 0x100, which 
interferes badly with how the PIIX4 decodes port I/O cycles.  That did 
happen in the field, wreaking havoc and prompting my change.

 By the look of the code it would definitely trigger for the Bonito64 
system controller, which has a fixed port I/O target address range and, 
depending on the settings left by the firmware, it might also trigger for 
the Galileo GT64120A and SOC-it 101 system controllers, which have 
variable port I/O target address ranges.

 Here's an example map of Malta port I/O resources (SOC-it 101 variant):

00000000-0000001f : dma1
00000020-00000021 : pic1
00000040-0000005f : timer
00000060-0000006f : keyboard
00000070-00000077 : rtc0
00000080-0000008f : dma page reg
000000a0-000000a1 : pic2
000000c0-000000df : dma2
00000170-00000177 : ata_piix
000001f0-000001f7 : ata_piix
000002f8-000002ff : serial
00000376-00000376 : ata_piix
00000378-0000037a : parport0
0000037b-0000037f : parport0
000003f6-000003f6 : ata_piix
000003f8-000003ff : serial
00001000-00ffffff : MSC PCI I/O
  00001000-0000103f : 0000:00:0a.3
  00001040-0000105f : 0000:00:0a.2
    00001040-0000105f : uhci_hcd
  00001060-0000107f : 0000:00:0b.0
    00001060-0000107f : pcnet32_probe_pci
  00001080-000010ff : 0000:00:12.0
    00001080-000010ff : defxx
  00001100-0000110f : 0000:00:0a.3
  00001400-000014ff : 0000:00:13.0
  00001800-0000180f : 0000:00:0a.1
    00001800-0000180f : ata_piix

As you can see there are holes in the map below 0x100, so e.g. if the bus 
master IDE I/O space registers (claimed last in the list by `ata_piix') 
were assigned to 00000030-0000003f, then all hell would break loose.  It 
is exactly the mapping that happened in the absence of the code piece in 
question IIRC.

 The choice of 0x1000 as the lower boundary IIRC has something to do with 
alignment; I think the decoding base has to be a multiple of 0x1000 and 
given that the ACPI resource is decoded by a non-standard BAR at 0x40 in 
the configuration space (set up by `malta_piix_func3_base_fixup' BTW) we 
just need to match its setting.

 Can you please check what the port I/O map looks like with your setup 
with and without your patch applied?

 NB there is still something fishy with the setup of SOC-it 101's PCI 
decoding windows, which is why I have forced `defxx' with a patch to use 
port I/O, as reported above.  The driver uses MMIO unconditionally on PCI 
systems nowadays, but using MMIO prevents it from working with the SOC-it 
101 system controller and I yet need to debug it.  Conversely MMIO used to 
work just fine with the Galileo GT64120A system controller while I still 
had one operational.

 HTH,

  Maciej

On Tue, 14 Oct 2025, Maciej W. Rozycki wrote:

> On Mon, 13 Oct 2025, Thomas Bogendoerfer wrote:
> 
> > > This patch causes boot failures when trying to boot mips images from
> > > ide drive in qemu. As far as I can see the interface no longer instantiates.
> > > 
> > > Reverting this patch fixes the problem. Bisect log attached for reference.
> > 
> > Patch below fixes my qemu malta setup. Now I'm wondering, why this is
> > needed. It was added with commit
> > 
> > aa0980b80908 ("Fixes for system controllers for Atlas/Malta core cards.")
> > 
> > Maciej, do you remember why this is needed ?
> 
>  I do.  The reason is preventing PCI port I/O mappings below 0x100, which 
> interferes badly with how the PIIX4 decodes port I/O cycles.  That did 
> happen in the field, wreaking havoc and prompting my change.
> 
>  By the look of the code it would definitely trigger for the Bonito64 
> system controller, which has a fixed port I/O target address range and, 
> depending on the settings left by the firmware, it might also trigger for 
> the Galileo GT64120A and SOC-it 101 system controllers, which have 
> variable port I/O target address ranges.
> 
>  Here's an example map of Malta port I/O resources (SOC-it 101 variant):
> 
> 00000000-0000001f : dma1
> 00000020-00000021 : pic1
> 00000040-0000005f : timer
> 00000060-0000006f : keyboard
> 00000070-00000077 : rtc0
> 00000080-0000008f : dma page reg
> 000000a0-000000a1 : pic2
> 000000c0-000000df : dma2
> 00000170-00000177 : ata_piix
> 000001f0-000001f7 : ata_piix
> 000002f8-000002ff : serial
> 00000376-00000376 : ata_piix
> 00000378-0000037a : parport0
> 0000037b-0000037f : parport0
> 000003f6-000003f6 : ata_piix
> 000003f8-000003ff : serial
> 00001000-00ffffff : MSC PCI I/O
>   00001000-0000103f : 0000:00:0a.3
>   00001040-0000105f : 0000:00:0a.2
>     00001040-0000105f : uhci_hcd
>   00001060-0000107f : 0000:00:0b.0
>     00001060-0000107f : pcnet32_probe_pci
>   00001080-000010ff : 0000:00:12.0
>     00001080-000010ff : defxx
>   00001100-0000110f : 0000:00:0a.3
>   00001400-000014ff : 0000:00:13.0
>   00001800-0000180f : 0000:00:0a.1
>     00001800-0000180f : ata_piix
> 
> As you can see there are holes in the map below 0x100, so e.g. if the bus 
> master IDE I/O space registers (claimed last in the list by `ata_piix') 
> were assigned to 00000030-0000003f, then all hell would break loose.  It 
> is exactly the mapping that happened in the absence of the code piece in 
> question IIRC.
> 
>  The choice of 0x1000 as the lower boundary IIRC has something to do with 
> alignment; I think the decoding base has to be a multiple of 0x1000 and 
> given that the ACPI resource is decoded by a non-standard BAR at 0x40 in 
> the configuration space (set up by `malta_piix_func3_base_fixup' BTW) we 
> just need to match its setting.
> 
>  Can you please check what the port I/O map looks like with your setup 
> with and without your patch applied?
> 
>  NB there is still something fishy with the setup of SOC-it 101's PCI 
> decoding windows, which is why I have forced `defxx' with a patch to use 
> port I/O, as reported above.  The driver uses MMIO unconditionally on PCI 
> systems nowadays, but using MMIO prevents it from working with the SOC-it 
> 101 system controller and I yet need to debug it.  Conversely MMIO used to 
> work just fine with the Galileo GT64120A system controller while I still 
> had one operational.

Are you sure pci-malta.c has to do anything like this as 
pcibios_align_resource() does lower bound IO resource start addresses if 
PCIBIOS_MIN_IO is set?

How about this patch below?

(I'm not sure if it should actually be
	PCIBIOS_MIN_IO = 0x1000 - hose->io_resource->start;
to allow resources starting from 0x1000 if ->start is not at 0.)

--
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Tue, 14 Oct 2025 13:47:49 +0300
Subject: [PATCH 1/1] MIPS: Malta: Use pcibios_align_resource() to block io range
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

According to Maciej W. Rozycki <macro@orcam.me.uk>, the
mips_pcibios_init() for malta adjusts root bus IO resource start
address to prevent interfering with PIIX4 I/O cycle decoding. Adjusting
lower bound leaves PIIX4 IO resources outside of the root bus resource
and assign_fixed_resource_on_bus() does not put the resources into the
resource tree.

Prior to commit ae81aad5c2e1 ("MIPS: PCI: Use pci_enable_resources()")
the arch specific pcibios_enable_resources() did not check if the
resources were assigned which diverges from what PCI core checks,
effectively hiding the PIIX4 IO resources were not properly within the
resource tree. After starting to use pcibios_enable_resources() from
PCI core, enabling PIIX4 fails:

ata_piix 0000:00:0a.1: BAR 0 [io  0x01f0-0x01f7]: not claimed; can't enable device
ata_piix 0000:00:0a.1: probe with driver ata_piix failed with error -22

MIPS PCI code already has support for enforcing lower bounds using
PCIBIOS_MIN_IO in pcibios_align_resource(). Make malta PCI code too to
use PCIBIOS_MIN_IO.

Fixes: ae81aad5c2e1 ("MIPS: PCI: Use pci_enable_resources()")
Fixes: aa0980b80908 ("Fixes for system controllers for Atlas/Malta core cards.")
Link: https://lore.kernel.org/linux-pci/9085ab12-1559-4462-9b18-f03dcb9a4088@roeck-us.net/
Link: https://lore.kernel.org/linux-pci/alpine.DEB.2.21.2510132229120.39634@angie.orcam.me.uk/
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/mips/pci/pci-malta.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/pci/pci-malta.c b/arch/mips/pci/pci-malta.c
index 6aefdf20ca05..f4ea1c99852f 100644
--- a/arch/mips/pci/pci-malta.c
+++ b/arch/mips/pci/pci-malta.c
@@ -231,7 +231,7 @@ void __init mips_pcibios_init(void)
 
 	/* PIIX4 ACPI starts at 0x1000 */
 	if (controller->io_resource->start < 0x00001000UL)
-		controller->io_resource->start = 0x00001000UL;
+		PCIBIOS_MIN_IO = 0x1000;
 
 	iomem_resource.end &= 0xfffffffffULL;			/* 64 GB */
 	ioport_resource.end = controller->io_resource->end;

base-commit: 2f2c7254931f41b5736e3ba12aaa9ac1bbeeeb92
-- 
2.39.5

On Tue, 14 Oct 2025, Ilpo Järvinen wrote:

> > As you can see there are holes in the map below 0x100, so e.g. if the bus 
> > master IDE I/O space registers (claimed last in the list by `ata_piix') 
> > were assigned to 00000030-0000003f, then all hell would break loose.  It 
> > is exactly the mapping that happened in the absence of the code piece in 
> > question IIRC.
> 
> Are you sure pci-malta.c has to do anything like this as 
> pcibios_align_resource() does lower bound IO resource start addresses if 
> PCIBIOS_MIN_IO is set?

 Well, PCIBIOS_MIN_IO is never set for Malta and therefore stays at 0.  I 
could boot 2.6.11 with the hunk reverted and see what happens, not a big 
deal (I should have old GCC somewhere as a kernel such old would surely be 
a pain to build with modern GCC).  This stuff was badly broken before 
commit ae81aad5c2e1 (and there was support there too for the Atlas board, 
a weird one with a Philips SAA9730 southbridge and supporting a subset of 
the same CPU core cards as the Malta board does).

> How about this patch below?
> 
> (I'm not sure if it should actually be
> 	PCIBIOS_MIN_IO = 0x1000 - hose->io_resource->start;
> to allow resources starting from 0x1000 if ->start is not at 0.)

 I'd have to go through the relevant datasheets to see whether it can 
actually happen in reality.  Perhaps we can just hardwire PCIBIOS_MIN_IO 
to 0x1000 instead, similarly to what other MIPS platforms do.  After all 
Malta's southbridge is on the mainboard and therefore always the same, 
regardless of the northbridge (system controller in MIPS-speak) which 
comes with the pluggable CPU core card.

 NB there are commit c5de50dada14 ("MIPS: Malta: Change start address to 
avoid conflicts.") and commit 27547abf36af ("MIPS: malta: Incorporate 
PIIX4 ACPI I/O region in PCI controller resources") that fiddled with this 
code piece.  Especially the latter one refers additional commits that may 
give further insights.  And the former one removed a "FIXME" annotation, 
which suggests I didn't consider the solution perfect back 20 years ago, 
but given how long it stayed there it was surely good enough for its time.

  Maciej

On Tue, 14 Oct 2025, Maciej W. Rozycki wrote:

> > > As you can see there are holes in the map below 0x100, so e.g. if the bus 
> > > master IDE I/O space registers (claimed last in the list by `ata_piix') 
> > > were assigned to 00000030-0000003f, then all hell would break loose.  It 
> > > is exactly the mapping that happened in the absence of the code piece in 
> > > question IIRC.
> > 
> > Are you sure pci-malta.c has to do anything like this as 
> > pcibios_align_resource() does lower bound IO resource start addresses if 
> > PCIBIOS_MIN_IO is set?
> 
>  Well, PCIBIOS_MIN_IO is never set for Malta and therefore stays at 0.  I 
> could boot 2.6.11 with the hunk reverted and see what happens, not a big 
> deal (I should have old GCC somewhere as a kernel such old would surely be 
> a pain to build with modern GCC).  This stuff was badly broken before 
> commit ae81aad5c2e1 (and there was support there too for the Atlas board, 
> a weird one with a Philips SAA9730 southbridge and supporting a subset of 
> the same CPU core cards as the Malta board does).

 Well, it is a big deal after all, since I've lost my old CoreLV CPU card
and the replacement Core74K one is too new for 2.6.x both in terms of the 
CPU and the system controller.  No doubt with some patching it should be 
able to get it booted, but it's not worth the effort.

 So instead I've just removed the hunk with my most recent compilation and 
what I've got is:

ata1: PATA max UDMA/33 cmd 0x1f0 ctl 0x3f6 bmdma 0x30 irq 14 lpm-pol 0
ata2: PATA max UDMA/33 cmd 0x170 ctl 0x376 bmdma 0x38 irq 15 lpm-pol 0

(notice the 0x30/0x38 bmdma allocations, which just confirms my memory) 
and then a temporary hang, a couple of more lines printed and the system 
silently rebooted back into YAMON.

 Having configured with ATA and PCNET32 disabled I get this:

00000000-00ffffff : MSC PCI I/O
  00000000-0000001f : 0000:00:0a.2
  00000020-00000021 : pic1
  00000030-0000003f : 0000:00:0a.1
  00000040-0000005f : 0000:00:0b.0
  00000060-0000006f : i8042
  00000070-00000077 : rtc0
  000000a0-000000a1 : pic2
  00000170-00000177 : 0000:00:0a.1
  000001f0-000001f7 : 0000:00:0a.1
  000002f8-000002ff : serial
  00000376-00000376 : 0000:00:0a.1
  00000378-0000037a : parport0
  0000037b-0000037f : parport0
  000003f6-000003f6 : 0000:00:0a.1
  000003f8-000003ff : serial
  00000400-000004ff : 0000:00:13.0
  00000800-0000087f : 0000:00:12.0
    00000800-0000087f : defxx
  00001000-0000103f : 0000:00:0a.3
  00001100-0000110f : 0000:00:0a.3

which does look nasty (although technically correctly shows southbridge 
resources within the system controller's PCI I/O window).  At least the 
defxx driver still works with the FDDI network interface at its port I/O 
assignment so the system boots multiuser NFS-rooted.

  Maciej

On Tue, 14 Oct 2025, Maciej W. Rozycki wrote:

> On Tue, 14 Oct 2025, Ilpo Järvinen wrote:
> 
> > > As you can see there are holes in the map below 0x100, so e.g. if the bus 
> > > master IDE I/O space registers (claimed last in the list by `ata_piix') 
> > > were assigned to 00000030-0000003f, then all hell would break loose.  It 
> > > is exactly the mapping that happened in the absence of the code piece in 
> > > question IIRC.
> > 
> > Are you sure pci-malta.c has to do anything like this as 
> > pcibios_align_resource() does lower bound IO resource start addresses if 
> > PCIBIOS_MIN_IO is set?
> 
>  Well, PCIBIOS_MIN_IO is never set for Malta and therefore stays at 0.

I meant whether pci-malta.c has to play with the ->start address at all 
if it would use PCIBIOS_MIN_IO.

> I could boot 2.6.11 with the hunk reverted and see what happens, not a big 
> deal (I should have old GCC somewhere as a kernel such old would surely be 
> a pain to build with modern GCC).  This stuff was badly broken before 
> commit ae81aad5c2e1 (and there was support there too for the Atlas board, 
> a weird one with a Philips SAA9730 southbridge and supporting a subset of 
> the same CPU core cards as the Malta board does).
> 
> > How about this patch below?
> > 
> > (I'm not sure if it should actually be
> > 	PCIBIOS_MIN_IO = 0x1000 - hose->io_resource->start;
> > to allow resources starting from 0x1000 if ->start is not at 0.)
> 
>  I'd have to go through the relevant datasheets to see whether it can 
> actually happen in reality.  Perhaps we can just hardwire PCIBIOS_MIN_IO 
> to 0x1000 instead, similarly to what other MIPS platforms do.

My patch did hardcode set it to 0x1000, I just noted before the patch that 
I'm not sure if the code should actually try to align the resulting "real 
start address" to 0x1000 if hose->io_resource->start != 0.

Or are you saying also the the if () check should be removed as well?

> After all 
> Malta's southbridge is on the mainboard and therefore always the same, 
> regardless of the northbridge (system controller in MIPS-speak) which 
> comes with the pluggable CPU core card.
> 
>  NB there are commit c5de50dada14 ("MIPS: Malta: Change start address to 
> avoid conflicts.") and commit 27547abf36af ("MIPS: malta: Incorporate 
> PIIX4 ACPI I/O region in PCI controller resources") that fiddled with this 
> code piece.  Especially the latter one refers additional commits that may 
> give further insights.  And the former one removed a "FIXME" annotation, 
> which suggests I didn't consider the solution perfect back 20 years ago, 
> but given how long it stayed there it was surely good enough for its time.

It was "good enough" only because the arch specific 
pcibios_enable_resources() was lacking the check for whether the resource 
truly got assigned or not. The PIIX4 driver must worked just fine without 
those IO resources which is what most drivers do despite using 
pci(m)_enable_device() and not pci_enable_device_mem() (the latter 
doesn't even seem to come with m variant).

-- 
 i.

On Tue, Oct 14, 2025 at 03:41:42PM +0300, Ilpo Järvinen wrote:
> [...]
> It was "good enough" only because the arch specific 
> pcibios_enable_resources() was lacking the check for whether the resource 
> truly got assigned or not. The PIIX4 driver must worked just fine without 
> those IO resources which is what most drivers do despite using 
> pci(m)_enable_device() and not pci_enable_device_mem() (the latter 
> doesn't even seem to come with m variant).

will you send a v2 of the patch ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

On Fri, 17 Oct 2025, Thomas Bogendoerfer wrote:

> On Tue, Oct 14, 2025 at 03:41:42PM +0300, Ilpo Järvinen wrote:
> > [...]
> > It was "good enough" only because the arch specific 
> > pcibios_enable_resources() was lacking the check for whether the resource 
> > truly got assigned or not. The PIIX4 driver must worked just fine without 
> > those IO resources which is what most drivers do despite using 
> > pci(m)_enable_device() and not pci_enable_device_mem() (the latter 
> > doesn't even seem to come with m variant).
> 
> will you send a v2 of the patch ?

Without the the if ()? I can do that, I was unsure how people wanted to 
progress with this.

-- 
 i.

On Fri, Oct 17, 2025 at 01:58:13PM +0300, Ilpo Järvinen wrote:
> On Fri, 17 Oct 2025, Thomas Bogendoerfer wrote:
> 
> > On Tue, Oct 14, 2025 at 03:41:42PM +0300, Ilpo Järvinen wrote:
> > > [...]
> > > It was "good enough" only because the arch specific 
> > > pcibios_enable_resources() was lacking the check for whether the resource 
> > > truly got assigned or not. The PIIX4 driver must worked just fine without 
> > > those IO resources which is what most drivers do despite using 
> > > pci(m)_enable_device() and not pci_enable_device_mem() (the latter 
> > > doesn't even seem to come with m variant).
> > 
> > will you send a v2 of the patch ?
> 
> Without the the if ()? I can do that, I was unsure how people wanted to 
> progress with this.

yes without the if(), thank you

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

On Tue, 14 Oct 2025, Ilpo Järvinen wrote:

> >  Well, PCIBIOS_MIN_IO is never set for Malta and therefore stays at 0.
> 
> I meant whether pci-malta.c has to play with the ->start address at all 
> if it would use PCIBIOS_MIN_IO.

 Yes, we need either, not both.

> >  I'd have to go through the relevant datasheets to see whether it can 
> > actually happen in reality.  Perhaps we can just hardwire PCIBIOS_MIN_IO 
> > to 0x1000 instead, similarly to what other MIPS platforms do.
> 
> My patch did hardcode set it to 0x1000, I just noted before the patch that 
> I'm not sure if the code should actually try to align the resulting "real 
> start address" to 0x1000 if hose->io_resource->start != 0.
> 
> Or are you saying also the the if () check should be removed as well?

 That's what I meant, sorry to be unclear.

> >  NB there are commit c5de50dada14 ("MIPS: Malta: Change start address to 
> > avoid conflicts.") and commit 27547abf36af ("MIPS: malta: Incorporate 
> > PIIX4 ACPI I/O region in PCI controller resources") that fiddled with this 
> > code piece.  Especially the latter one refers additional commits that may 
> > give further insights.  And the former one removed a "FIXME" annotation, 
> > which suggests I didn't consider the solution perfect back 20 years ago, 
> > but given how long it stayed there it was surely good enough for its time.
> 
> It was "good enough" only because the arch specific 
> pcibios_enable_resources() was lacking the check for whether the resource 
> truly got assigned or not. The PIIX4 driver must worked just fine without 
> those IO resources which is what most drivers do despite using 
> pci(m)_enable_device() and not pci_enable_device_mem() (the latter 
> doesn't even seem to come with m variant).

 As /proc/ioport contents indicate the resources did get assigned or there 
would be no claiming driver reported.  I'm sure I did double-check it back 
in the day too.

  Maciej

[+cc regressions]

On Mon, Oct 13, 2025 at 12:54:25PM -0700, Guenter Roeck wrote:
> On Fri, Aug 29, 2025 at 04:10:52PM +0300, Ilpo Järvinen wrote:
> > pci-legacy.c under MIPS has a copy of pci_enable_resources() named as
> > pcibios_enable_resources(). Having own copy of same functionality could
> > lead to inconsistencies in behavior, especially now as
> > pci_enable_resources() and the bridge window resource flags behavior are
> > going to be altered by upcoming changes.
> > 
> > The check for !r->start && r->end is already covered by the more generic
> > checks done in pci_enable_resources().
> > 
> > Call pci_enable_resources() from MIPS's pcibios_enable_device() and remove
> > pcibios_enable_resources().
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> 
> This patch causes boot failures when trying to boot mips images from
> ide drive in qemu. As far as I can see the interface no longer instantiates.
> 
> Reverting this patch fixes the problem. Bisect log attached for reference.
> 
> Guenter
> 
> ---
> # bad: [3a8660878839faadb4f1a6dd72c3179c1df56787] Linux 6.18-rc1
> # good: [e5f0a698b34ed76002dc5cff3804a61c80233a7a] Linux 6.17
> git bisect start 'HEAD' 'v6.17'
> # good: [58809f614e0e3f4e12b489bddf680bfeb31c0a20] Merge tag 'drm-next-2025-10-01' of https://gitlab.freedesktop.org/drm/kernel
> git bisect good 58809f614e0e3f4e12b489bddf680bfeb31c0a20
> # good: [bed0653fe2aacb0ca8196075cffc9e7062e74927] Merge tag 'iommu-updates-v6.18' of git://git.kernel.org/pub/scm/linux/kernel/git/iommu/linux
> git bisect good bed0653fe2aacb0ca8196075cffc9e7062e74927
> # good: [6a74422b9710e987c7d6b85a1ade7330b1e61626] Merge tag 'mips_6.18' of git://git.kernel.org/pub/scm/linux/kernel/git/mips/linux
> git bisect good 6a74422b9710e987c7d6b85a1ade7330b1e61626
> # bad: [522ba450b56fff29f868b1552bdc2965f55de7ed] Merge tag 'clk-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux
> git bisect bad 522ba450b56fff29f868b1552bdc2965f55de7ed
> # bad: [256e3417065b2721f77bcd37331796b59483ef3b] Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm
> git bisect bad 256e3417065b2721f77bcd37331796b59483ef3b
> # bad: [2f2c7254931f41b5736e3ba12aaa9ac1bbeeeb92] Merge tag 'pci-v6.18-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci
> git bisect bad 2f2c7254931f41b5736e3ba12aaa9ac1bbeeeb92
> # bad: [531abff0fa53bc3a2f7f69b2693386eb6bda96e5] Merge branch 'pci/controller/qcom'
> git bisect bad 531abff0fa53bc3a2f7f69b2693386eb6bda96e5
> # bad: [fead6a0b15bf3b33dba877efec6b4e7b4cc4abc3] Merge branch 'pci/resource'
> git bisect bad fead6a0b15bf3b33dba877efec6b4e7b4cc4abc3
> # good: [0bb65e32495e6235a069b60e787140da99e9c122] Merge branch 'pci/p2pdma'
> git bisect good 0bb65e32495e6235a069b60e787140da99e9c122
> # bad: [ebe091ad81e1d3e5cbb1592ebc18175b5ca3d2bd] PCI: Use pbus_select_window_for_type() during IO window sizing
> git bisect bad ebe091ad81e1d3e5cbb1592ebc18175b5ca3d2bd
> # bad: [2ee33aa14d3f2e92ba8ae80443f2cd9b575f08cb] PCI: Always claim bridge window before its setup
> git bisect bad 2ee33aa14d3f2e92ba8ae80443f2cd9b575f08cb
> # good: [2657a0c982239fecc41d0df5a69091ca4297647c] m68k/PCI: Use pci_enable_resources() in pcibios_enable_device()
> git bisect good 2657a0c982239fecc41d0df5a69091ca4297647c
> # bad: [ae81aad5c2e17fd1fafd930e75b81aedc837f705] MIPS: PCI: Use pci_enable_resources()
> git bisect bad ae81aad5c2e17fd1fafd930e75b81aedc837f705
> # good: [754babaaf33349d9ef27bb1ac6f5d9d5a503a2a6] sparc/PCI: Remove pcibios_enable_device() as they do nothing extra
> git bisect good 754babaaf33349d9ef27bb1ac6f5d9d5a503a2a6
> # first bad commit: [ae81aad5c2e17fd1fafd930e75b81aedc837f705] MIPS: PCI: Use pci_enable_resources()

#regzbot introduced: ae81aad5c2e1 ("MIPS: PCI: Use pci_enable_resources()")

On Mon, Oct 13, 2025 at 04:02:36PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 13, 2025 at 12:54:25PM -0700, Guenter Roeck wrote:
> > On Fri, Aug 29, 2025 at 04:10:52PM +0300, Ilpo Järvinen wrote:
> > > pci-legacy.c under MIPS has a copy of pci_enable_resources() named as
> > > pcibios_enable_resources(). Having own copy of same functionality could
> > > lead to inconsistencies in behavior, especially now as
> > > pci_enable_resources() and the bridge window resource flags behavior are
> > > going to be altered by upcoming changes.
> > > 
> > > The check for !r->start && r->end is already covered by the more generic
> > > checks done in pci_enable_resources().
> > > 
> > > Call pci_enable_resources() from MIPS's pcibios_enable_device() and remove
> > > pcibios_enable_resources().
> > > 
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > 
> > This patch causes boot failures when trying to boot mips images from
> > ide drive in qemu. As far as I can see the interface no longer instantiates.
> > 
> > Reverting this patch fixes the problem. Bisect log attached for reference.
> ...

> > # first bad commit: [ae81aad5c2e17fd1fafd930e75b81aedc837f705] MIPS: PCI: Use pci_enable_resources()
> 
> #regzbot introduced: ae81aad5c2e1 ("MIPS: PCI: Use pci_enable_resources()")

#regzbot fix: 1d5d1663619d ("MIPS: Malta: Fix PCI southbridge legacy resource reservations")
#regzbot fix: f294a5fd34db ("MIPS: Malta: Use pcibios_align_resource() to block io range")