With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide"), the Q40 IDE driver was replaced by pata_falcon.c.
Both IO and memory resources were defined for the Q40 IDE platform device, but definition of the IDE register addresses was modeled after the Falcon case, both in use of the memory resources and in including register scale and byte vs. word offset in the address.
This was correct for the Falcon case, which does not apply any address translation to the register addresses. In the Q40 case, all of device base address, byte access offset and register scaling is included in the platform specific ISA access translation (in asm/mm_io.h).
As a consequence, such address translation gets applied twice, and register addresses are mangled.
Use the device base address from the platform IO resource, and use standard register offsets from that base in order to calculate register addresses (the IO address translation will then apply the correct ISA window base and scaling).
Encode PIO_OFFSET into IO port addresses for all registers except the data transfer register. Encode the MMIO offset there (pata_falcon_data_xfer() directly uses raw IO with no address translation).
Reported-by: William R Sowerbutts will@sowerbutts.com Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagV... Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagV... Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide") Cc: stable@vger.kernel.org Cc: Finn Thain fthain@linux-m68k.org Cc: Geert Uytterhoeven geert@linux-m68k.org Tested-by: William R Sowerbutts will@sowerbutts.com Signed-off-by: Michael Schmitz schmitzmic@gmail.com
---
Changes from v2:
Finn Thain: - add back stable Cc:
Changes from v1:
Damien Le Moal: - change patch title - drop stable backport tag
Changes from RFC v3:
- split off byte swap option into separate patch
Geert Uytterhoeven: - review comments
Changes from RFC v2: - add driver parameter 'data_swap' as bit mask for drives to swap
Changes from RFC v1:
Finn Thain: - take care to supply IO address suitable for ioread8/iowrite8 - use MMIO address for data transfer --- drivers/ata/pata_falcon.c | 55 ++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 21 deletions(-)
diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c index 996516e64f13..346259e3bbc8 100644 --- a/drivers/ata/pata_falcon.c +++ b/drivers/ata/pata_falcon.c @@ -123,8 +123,8 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) struct resource *base_res, *ctl_res, *irq_res; struct ata_host *host; struct ata_port *ap; - void __iomem *base; - int irq = 0; + void __iomem *base, *ctl_base; + int irq = 0, io_offset = 1, reg_scale = 4;
dev_info(&pdev->dev, "Atari Falcon and Q40/Q60 PATA controller\n");
@@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) ap->pio_mask = ATA_PIO4; ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY;
- base = (void __iomem *)base_mem_res->start; /* N.B. this assumes data_addr will be used for word-sized I/O only */ - ap->ioaddr.data_addr = base + 0 + 0 * 4; - ap->ioaddr.error_addr = base + 1 + 1 * 4; - ap->ioaddr.feature_addr = base + 1 + 1 * 4; - ap->ioaddr.nsect_addr = base + 1 + 2 * 4; - ap->ioaddr.lbal_addr = base + 1 + 3 * 4; - ap->ioaddr.lbam_addr = base + 1 + 4 * 4; - ap->ioaddr.lbah_addr = base + 1 + 5 * 4; - ap->ioaddr.device_addr = base + 1 + 6 * 4; - ap->ioaddr.status_addr = base + 1 + 7 * 4; - ap->ioaddr.command_addr = base + 1 + 7 * 4; - - base = (void __iomem *)ctl_mem_res->start; - ap->ioaddr.altstatus_addr = base + 1; - ap->ioaddr.ctl_addr = base + 1; - - ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx", - (unsigned long)base_mem_res->start, - (unsigned long)ctl_mem_res->start); + ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start; + + if (base_res) { /* only Q40 has IO resources */ + io_offset = 0x10000; + reg_scale = 1; + base = (void __iomem *)base_res->start; + ctl_base = (void __iomem *)ctl_res->start; + + ata_port_desc(ap, "cmd %pa ctl %pa", + &base_res->start, + &ctl_res->start); + } else { + base = (void __iomem *)base_mem_res->start; + ctl_base = (void __iomem *)ctl_mem_res->start; + + ata_port_desc(ap, "cmd %pa ctl %pa", + &base_mem_res->start, + &ctl_mem_res->start); + } + + ap->ioaddr.error_addr = base + io_offset + 1 * reg_scale; + ap->ioaddr.feature_addr = base + io_offset + 1 * reg_scale; + ap->ioaddr.nsect_addr = base + io_offset + 2 * reg_scale; + ap->ioaddr.lbal_addr = base + io_offset + 3 * reg_scale; + ap->ioaddr.lbam_addr = base + io_offset + 4 * reg_scale; + ap->ioaddr.lbah_addr = base + io_offset + 5 * reg_scale; + ap->ioaddr.device_addr = base + io_offset + 6 * reg_scale; + ap->ioaddr.status_addr = base + io_offset + 7 * reg_scale; + ap->ioaddr.command_addr = base + io_offset + 7 * reg_scale; + + ap->ioaddr.altstatus_addr = ctl_base + io_offset; + ap->ioaddr.ctl_addr = ctl_base + io_offset;
irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); if (irq_res && irq_res->start > 0) {
Hi Michael,
On Sat, Aug 19, 2023 at 1:49 AM Michael Schmitz schmitzmic@gmail.com wrote:
With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide"), the Q40 IDE driver was replaced by pata_falcon.c.
Both IO and memory resources were defined for the Q40 IDE platform device, but definition of the IDE register addresses was modeled after the Falcon case, both in use of the memory resources and in including register scale and byte vs. word offset in the address.
This was correct for the Falcon case, which does not apply any address translation to the register addresses. In the Q40 case, all of device base address, byte access offset and register scaling is included in the platform specific ISA access translation (in asm/mm_io.h).
As a consequence, such address translation gets applied twice, and register addresses are mangled.
Use the device base address from the platform IO resource, and use standard register offsets from that base in order to calculate register addresses (the IO address translation will then apply the correct ISA window base and scaling).
Encode PIO_OFFSET into IO port addresses for all registers except the data transfer register. Encode the MMIO offset there (pata_falcon_data_xfer() directly uses raw IO with no address translation).
Reported-by: William R Sowerbutts will@sowerbutts.com Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagV... Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagV... Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide") Cc: stable@vger.kernel.org Cc: Finn Thain fthain@linux-m68k.org Cc: Geert Uytterhoeven geert@linux-m68k.org Tested-by: William R Sowerbutts will@sowerbutts.com Signed-off-by: Michael Schmitz schmitzmic@gmail.com
Thanks for the update!
--- a/drivers/ata/pata_falcon.c +++ b/drivers/ata/pata_falcon.c @@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) ap->pio_mask = ATA_PIO4; ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY;
base = (void __iomem *)base_mem_res->start; /* N.B. this assumes data_addr will be used for word-sized I/O only */ap->ioaddr.data_addr = base + 0 + 0 * 4;ap->ioaddr.error_addr = base + 1 + 1 * 4;ap->ioaddr.feature_addr = base + 1 + 1 * 4;ap->ioaddr.nsect_addr = base + 1 + 2 * 4;ap->ioaddr.lbal_addr = base + 1 + 3 * 4;ap->ioaddr.lbam_addr = base + 1 + 4 * 4;ap->ioaddr.lbah_addr = base + 1 + 5 * 4;ap->ioaddr.device_addr = base + 1 + 6 * 4;ap->ioaddr.status_addr = base + 1 + 7 * 4;ap->ioaddr.command_addr = base + 1 + 7 * 4;base = (void __iomem *)ctl_mem_res->start;ap->ioaddr.altstatus_addr = base + 1;ap->ioaddr.ctl_addr = base + 1;ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",(unsigned long)base_mem_res->start,(unsigned long)ctl_mem_res->start);
ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;if (base_res) { /* only Q40 has IO resources */io_offset = 0x10000;reg_scale = 1;base = (void __iomem *)base_res->start;ctl_base = (void __iomem *)ctl_res->start;ata_port_desc(ap, "cmd %pa ctl %pa",&base_res->start,&ctl_res->start);
This can be moved outside the else, using %px to format base and ctl_base.
} else {base = (void __iomem *)base_mem_res->start;ctl_base = (void __iomem *)ctl_mem_res->start;ata_port_desc(ap, "cmd %pa ctl %pa",&base_mem_res->start,&ctl_mem_res->start);}
Gr{oetje,eeting}s,
Geert
Hi Geert,
On 21/08/23 19:50, Geert Uytterhoeven wrote:
Hi Michael,
On Sat, Aug 19, 2023 at 1:49 AM Michael Schmitz schmitzmic@gmail.com wrote:
With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide"), the Q40 IDE driver was replaced by pata_falcon.c.
Both IO and memory resources were defined for the Q40 IDE platform device, but definition of the IDE register addresses was modeled after the Falcon case, both in use of the memory resources and in including register scale and byte vs. word offset in the address.
This was correct for the Falcon case, which does not apply any address translation to the register addresses. In the Q40 case, all of device base address, byte access offset and register scaling is included in the platform specific ISA access translation (in asm/mm_io.h).
As a consequence, such address translation gets applied twice, and register addresses are mangled.
Use the device base address from the platform IO resource, and use standard register offsets from that base in order to calculate register addresses (the IO address translation will then apply the correct ISA window base and scaling).
Encode PIO_OFFSET into IO port addresses for all registers except the data transfer register. Encode the MMIO offset there (pata_falcon_data_xfer() directly uses raw IO with no address translation).
Reported-by: William R Sowerbutts will@sowerbutts.com Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagV... Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagV... Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide") Cc: stable@vger.kernel.org Cc: Finn Thain fthain@linux-m68k.org Cc: Geert Uytterhoeven geert@linux-m68k.org Tested-by: William R Sowerbutts will@sowerbutts.com Signed-off-by: Michael Schmitz schmitzmic@gmail.com
Thanks for the update!
--- a/drivers/ata/pata_falcon.c +++ b/drivers/ata/pata_falcon.c @@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) ap->pio_mask = ATA_PIO4; ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY;
base = (void __iomem *)base_mem_res->start; /* N.B. this assumes data_addr will be used for word-sized I/O only */ap->ioaddr.data_addr = base + 0 + 0 * 4;ap->ioaddr.error_addr = base + 1 + 1 * 4;ap->ioaddr.feature_addr = base + 1 + 1 * 4;ap->ioaddr.nsect_addr = base + 1 + 2 * 4;ap->ioaddr.lbal_addr = base + 1 + 3 * 4;ap->ioaddr.lbam_addr = base + 1 + 4 * 4;ap->ioaddr.lbah_addr = base + 1 + 5 * 4;ap->ioaddr.device_addr = base + 1 + 6 * 4;ap->ioaddr.status_addr = base + 1 + 7 * 4;ap->ioaddr.command_addr = base + 1 + 7 * 4;base = (void __iomem *)ctl_mem_res->start;ap->ioaddr.altstatus_addr = base + 1;ap->ioaddr.ctl_addr = base + 1;ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",(unsigned long)base_mem_res->start,(unsigned long)ctl_mem_res->start);
ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;if (base_res) { /* only Q40 has IO resources */io_offset = 0x10000;reg_scale = 1;base = (void __iomem *)base_res->start;ctl_base = (void __iomem *)ctl_res->start;ata_port_desc(ap, "cmd %pa ctl %pa",&base_res->start,&ctl_res->start);This can be moved outside the else, using %px to format base and ctl_base.
Right - do we need some additional message spelling out what address Q40 uses for data transfers? (Redundant for Falcon, of course ...)
Though that could be handled outside the else, too:
ata_port_desc(ap, "cmd %px ctl %px data %pa", base, ctl_base, &ap->ioaddr.data_addr);
Cheers,
Michael
} else {base = (void __iomem *)base_mem_res->start;ctl_base = (void __iomem *)ctl_mem_res->start;ata_port_desc(ap, "cmd %pa ctl %pa",&base_mem_res->start,&ctl_mem_res->start);}Gr{oetje,eeting}s,
Geert
Hi Michael,
On Tue, Aug 22, 2023 at 1:57 AM Michael Schmitz schmitzmic@gmail.com wrote:
On 21/08/23 19:50, Geert Uytterhoeven wrote:
On Sat, Aug 19, 2023 at 1:49 AM Michael Schmitz schmitzmic@gmail.com wrote:
With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide"), the Q40 IDE driver was replaced by pata_falcon.c.
Both IO and memory resources were defined for the Q40 IDE platform device, but definition of the IDE register addresses was modeled after the Falcon case, both in use of the memory resources and in including register scale and byte vs. word offset in the address.
This was correct for the Falcon case, which does not apply any address translation to the register addresses. In the Q40 case, all of device base address, byte access offset and register scaling is included in the platform specific ISA access translation (in asm/mm_io.h).
As a consequence, such address translation gets applied twice, and register addresses are mangled.
Use the device base address from the platform IO resource, and use standard register offsets from that base in order to calculate register addresses (the IO address translation will then apply the correct ISA window base and scaling).
Encode PIO_OFFSET into IO port addresses for all registers except the data transfer register. Encode the MMIO offset there (pata_falcon_data_xfer() directly uses raw IO with no address translation).
Reported-by: William R Sowerbutts will@sowerbutts.com Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagV... Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagV... Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide") Cc: stable@vger.kernel.org Cc: Finn Thain fthain@linux-m68k.org Cc: Geert Uytterhoeven geert@linux-m68k.org Tested-by: William R Sowerbutts will@sowerbutts.com Signed-off-by: Michael Schmitz schmitzmic@gmail.com
Thanks for the update!
--- a/drivers/ata/pata_falcon.c +++ b/drivers/ata/pata_falcon.c @@ -165,26 +165,39 @@ static int __init pata_falcon_init_one(struct platform_device *pdev) ap->pio_mask = ATA_PIO4; ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY;
base = (void __iomem *)base_mem_res->start; /* N.B. this assumes data_addr will be used for word-sized I/O only */ap->ioaddr.data_addr = base + 0 + 0 * 4;ap->ioaddr.error_addr = base + 1 + 1 * 4;ap->ioaddr.feature_addr = base + 1 + 1 * 4;ap->ioaddr.nsect_addr = base + 1 + 2 * 4;ap->ioaddr.lbal_addr = base + 1 + 3 * 4;ap->ioaddr.lbam_addr = base + 1 + 4 * 4;ap->ioaddr.lbah_addr = base + 1 + 5 * 4;ap->ioaddr.device_addr = base + 1 + 6 * 4;ap->ioaddr.status_addr = base + 1 + 7 * 4;ap->ioaddr.command_addr = base + 1 + 7 * 4;base = (void __iomem *)ctl_mem_res->start;ap->ioaddr.altstatus_addr = base + 1;ap->ioaddr.ctl_addr = base + 1;ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",(unsigned long)base_mem_res->start,(unsigned long)ctl_mem_res->start);
ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;if (base_res) { /* only Q40 has IO resources */io_offset = 0x10000;reg_scale = 1;base = (void __iomem *)base_res->start;ctl_base = (void __iomem *)ctl_res->start;ata_port_desc(ap, "cmd %pa ctl %pa",&base_res->start,&ctl_res->start);This can be moved outside the else, using %px to format base and ctl_base.
Right - do we need some additional message spelling out what address Q40 uses for data transfers? (Redundant for Falcon, of course ...)
Though that could be handled outside the else, too:
ata_port_desc(ap, "cmd %px ctl %px data %pa", base, ctl_base, &ap->ioaddr.data_addr);
I guess that wouldn't hurt.
Gr{oetje,eeting}s,
Geert
Hi Geert,
Am 22.08.2023 um 19:05 schrieb Geert Uytterhoeven:
if (base_res) { /* only Q40 has IO resources */io_offset = 0x10000;reg_scale = 1;base = (void __iomem *)base_res->start;ctl_base = (void __iomem *)ctl_res->start;ata_port_desc(ap, "cmd %pa ctl %pa",&base_res->start,&ctl_res->start);This can be moved outside the else, using %px to format base and ctl_base.
Right - do we need some additional message spelling out what address Q40 uses for data transfers? (Redundant for Falcon, of course ...)
Though that could be handled outside the else, too:
ata_port_desc(ap, "cmd %px ctl %px data %pa", base, ctl_base, &ap->ioaddr.data_addr);
I guess that wouldn't hurt.
Done - I'll send out v4 tomorrow.
Cheers,
Michael
Gr{oetje,eeting}s,
Geert
Hi Geert,
Am 21.08.2023 um 19:50 schrieb Geert Uytterhoeven:
ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;if (base_res) { /* only Q40 has IO resources */io_offset = 0x10000;reg_scale = 1;base = (void __iomem *)base_res->start;ctl_base = (void __iomem *)ctl_res->start;ata_port_desc(ap, "cmd %pa ctl %pa",&base_res->start,&ctl_res->start);This can be moved outside the else, using %px to format base and ctl_base.
I get a checkpatch warning for %px, but not for %pa (used for . &ap->ioaddr.data_addr). What gives?
WARNING: Using vsprintf specifier '%px' potentially exposes the kernel memory layout, if you don't really need the address please consider using '%p'. #148: FILE: drivers/ata/pata_falcon.c:194: + ata_port_desc(ap, "cmd %px ctl %px data %pa", + base, ctl_base, &ap->ioaddr.data_addr);
Using %pa and &base, &ctl_base just to shut that up seems a little silly ...
Cheers,
Michael
} else {base = (void __iomem *)base_mem_res->start;ctl_base = (void __iomem *)ctl_mem_res->start;ata_port_desc(ap, "cmd %pa ctl %pa",&base_mem_res->start,&ctl_mem_res->start);}Gr{oetje,eeting}s,
Geert
Hi Michael,
On Tue, Aug 22, 2023 at 10:27 AM Michael Schmitz schmitzmic@gmail.com wrote:
Am 21.08.2023 um 19:50 schrieb Geert Uytterhoeven:
ap->ioaddr.data_addr = (void __iomem *)base_mem_res->start;if (base_res) { /* only Q40 has IO resources */io_offset = 0x10000;reg_scale = 1;base = (void __iomem *)base_res->start;ctl_base = (void __iomem *)ctl_res->start;ata_port_desc(ap, "cmd %pa ctl %pa",&base_res->start,&ctl_res->start);This can be moved outside the else, using %px to format base and ctl_base.
I get a checkpatch warning for %px, but not for %pa (used for . &ap->ioaddr.data_addr). What gives?
WARNING: Using vsprintf specifier '%px' potentially exposes the kernel memory layout, if you don't really need the address please consider using '%p'. #148: FILE: drivers/ata/pata_falcon.c:194:
ata_port_desc(ap, "cmd %px ctl %px data %pa",base, ctl_base, &ap->ioaddr.data_addr);
You can ignore this. %p prints obfuscated pointer values, so they are useless here. %px prints an unobfuscated pointer value, which is fine here, as it's not a pointer to kernel memory that an attacker can use, but just the virtual address of an I/O device mapped one-to-one.
Gr{oetje,eeting}s,
Geert
linux-stable-mirror@lists.linaro.org
-
Geert Uytterhoeven -
Michael Schmitz