Re: [PATCH RFC v3] m68k/q40: fix IO base selection for Q40 in pata_falcon.c

[Michael Schmitz <schmitzmic@gmail.com>]

Hi William,

On 17/08/23 10:36, William R Sowerbutts wrote:
> I tested this patch
>
> [    1.540000] atari-falcon-ide atari-falcon-ide.0: Atari Falcon and Q40/Q60 PATA controller
> [    1.570000] scsi host0: pata_falcon
> [    1.580000] ata1: PATA max PIO4 cmd 0x1f0 ctl 0x3f6 irq 14
> [    1.590000] atari-falcon-ide atari-falcon-ide.1: Atari Falcon and Q40/Q60 PATA controller
> [    1.630000] scsi host1: pata_falcon
> [    1.640000] ata2: PATA max PIO4 cmd 0x170 ctl 0x376 irq 15
>
> [    1.840000] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [    1.840000] Oops: 00000000
> [    1.840000] Modules linked in:
> [    1.840000] PC: [<00211a82>] pata_falcon_data_xfer+0x2e/0x220
> [    1.840000] SR: 2700  SP: (ptrval)  a2: 00853070
> [    1.840000] d0: 00915ea0    d1: 000010a0    d2: 000000b0    d3: 00000ea0
> [    1.840000] d4: 00000160    d5: 00000000    a0: 0091549a    a1: 00915d60
> [    1.840000] Process kworker/0:1 (pid: 14, task=(ptrval))
> [    1.840000] Frame format=7 eff addr=00859e64 ssw=0505 faddr=00000000
> [    1.840000] wb 1 stat/addr/data: 0005 00306b70 004666b4
> [    1.840000] wb 2 stat/addr/data: 0005 00032934 00466b22
> [    1.840000] wb 3 stat/addr/data: 0005 00859e44 00000000
> [    1.840000] push data: 004666b4 00032a56 00464982 00000003
> [    1.840000] Stack from 00859e44:
> [    1.840000]         00000000 00000ea0 00915658 00000001 00000000 004caff4 00914000 002109b0
> [    1.840000]         0020ff74 00000000 002109ec 0091549a 00915ea0 00000160 00000000 004caff4
> [    1.840000]         0091549a 00210ae8 0091549a 004caff4 00000ea0 00000160 00915458 00000008
> [    1.840000]         0091549a 00914000 00915550 00211072 0091549a 00915458 00000008 0091549a
> [    1.840000]         00914000 00211564 0091549a 0091549a 00211084 0031bcfa 00203eb4 00000000
> [    1.840000]         007ca5e0 0091405e 00914000 00915550 0020ff74 0000a7c6 0091549a 0091405e
> [    1.840000] Call Trace: [<002109b0>] ata_pio_xfer+0x0/0x8c
> [    1.840000]  [<0020ff74>] ata_sff_busy_wait+0x0/0x42
> [    1.840000]  [<002109ec>] ata_pio_xfer+0x3c/0x8c
> [    1.840000]  [<00210ae8>] ata_pio_sector+0xac/0x112
> [    1.840000]  [<00211072>] ata_pio_sectors+0x94/0xa6
> [    1.840000]  [<00211564>] ata_sff_hsm_move+0x4e0/0x56e
> [    1.840000]  [<00211084>] ata_sff_hsm_move+0x0/0x56e
> [    1.840000]  [<0031bcfa>] warn_slowpath_fmt+0x0/0x62
> [    1.840000]  [<00203eb4>] ata_msleep+0x0/0xa2
> [    1.840000]  [<0020ff74>] ata_sff_busy_wait+0x0/0x42
> [    1.840000]  [<0000a7c6>] ATANSM+0x62/0x70
> [    1.840000]  [<002116ea>] ata_sff_pio_task+0xf8/0x104
> [    1.840000]  [<00015cbc>] kernel_thread+0x0/0x68
> [    1.840000]  [<00024f36>] process_one_work+0x12a/0x1b2
> [    1.840000]  [<00022fc2>] worker_clr_flags+0x0/0x72
> [    1.840000]  [<00025406>] worker_thread+0x230/0x292
> [    1.840000]  [<00015cbc>] kernel_thread+0x0/0x68
> [    1.840000]  [<000295a4>] kthread_exit+0x0/0x14
> [    1.840000]  [<000251d6>] worker_thread+0x0/0x292
> [    1.840000]  [<00029668>] kthread+0x96/0xa0
> [    1.840000]  [<000295d2>] kthread+0x0/0xa0
> [    1.840000]  [<00002898>] ret_from_kernel_thread+0xc/0x14
> [    1.840000]
> [    1.840000] Code: 286d 0024 2c6d 24e8 2404 e28a 2a68 0008 <2055> 2068 00ac 2228 00fc 7601 c684 7001 b0a9 0108 6742 7007 c082 4a85 6700 0090
> [    1.840000] Disabling lock debugging due to kernel taint
> [    1.840000] note: kworker/0:1[14] exited with irqs disabled
>
> This line is the problem:
>
>> +	int dev_id = cmd->device->sdev_target->id;
> I changed this to 'dev_id = dev->devno' instead, which fixed it.
Odd - but dev->devno appears to be the correct one.
>
> I confirmed that the byte swapping on/off feature works.

Thanks - I'll work in Geert's and Finn's comments and will send a 
two-patch series.

Cheers,

     Michael

>
> Will
>
>
> On Wed, Aug 16, 2023 at 10:32:12AM +1200, Michael Schmitz wrote:
>> With commit 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver
>> with pata_falcon and falconide"), the Q40 IDE driver was
>> replaced by pata_falcon.c (and the later obsoleted
>> falconide.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).
>>
>> Add module parameter 'data_swap' to allow connecting drives
>> with non-native data byte order. Drives selected by the
>> data_swap bit mask will have their user data swapped to host
>> byte order, i.e. 'pata_falcon.data_swap=2' will swap all user
>> data on drive B, leaving data on drive A in native order.
>>
>> Reported-by: William R Sowerbutts <will@sowerbutts.com>
>> Closes: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com
>> Link: https://lore.kernel.org/r/CAMuHMdUU62jjunJh9cqSqHT87B0H0A4udOOPs=WN7WZKpcagVA@mail.gmail.com
>> Fixes: 44b1fbc0f5f3 ("m68k/q40: Replace q40ide driver with pata_falcon and falconide")
>> Cc: Finn Thain <fthain@linux-m68k.org>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>>
>> ---
>>
>> Changes from v2:
>>
>> - add driver parameter 'data_swap' as bit mask for drives to swap
>>
>> Changes from v1:
>>
>> Finn Thain:
>> - take care to supply IO address suitable for ioread8/iowrite8
>> - use MMIO address for data transfer
>> ---
>> drivers/ata/pata_falcon.c | 90 ++++++++++++++++++++++++++++++---------
>> 1 file changed, 69 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/ata/pata_falcon.c b/drivers/ata/pata_falcon.c
>> index 996516e64f13..e6038eca39d6 100644
>> --- a/drivers/ata/pata_falcon.c
>> +++ b/drivers/ata/pata_falcon.c
>> @@ -33,6 +33,16 @@
>> #define DRV_NAME "pata_falcon"
>> #define DRV_VERSION "0.1.0"
>>
>> +static int pata_falcon_swap_mask = 0;
>> +
>> +module_param_named(data_swap, pata_falcon_swap_mask, int, 0444);
>> +MODULE_PARM_DESC(data_swap, "Data byte swap enable/disable (0x1==drive1, 0x2==drive2, default==0)");
>> +
>> +struct pata_falcon_priv {
>> +	unsigned int swap_mask;
>> +	bool swap_data;
>> +};
>> +
>> static const struct scsi_host_template pata_falcon_sht = {
>> 	ATA_PIO_SHT(DRV_NAME),
>> };
>> @@ -44,13 +54,15 @@ static unsigned int pata_falcon_data_xfer(struct ata_queued_cmd *qc,
>> 	struct ata_device *dev = qc->dev;
>> 	struct ata_port *ap = dev->link->ap;
>> 	void __iomem *data_addr = ap->ioaddr.data_addr;
>> +	struct pata_falcon_priv *priv = ap->private_data;
>> 	unsigned int words = buflen >> 1;
>> 	struct scsi_cmnd *cmd = qc->scsicmd;
>> +	int dev_id = cmd->device->sdev_target->id;
>> 	bool swap = 1;
>>
>> 	if (dev->class == ATA_DEV_ATA && cmd &&
>> 	    !blk_rq_is_passthrough(scsi_cmd_to_rq(cmd)))
>> -		swap = 0;
>> +		swap = priv->swap_data && (priv->swap_mask & 1<<dev_id);
>>
>> 	/* Transfer multiple of 2 bytes */
>> 	if (rw == READ) {
>> @@ -123,6 +135,7 @@ 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;
>> +	struct pata_falcon_priv *priv;
>> 	void __iomem *base;
>> 	int irq = 0;
>>
>> @@ -165,26 +178,61 @@ 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);
>> +	priv = devm_kzalloc(&pdev->dev,
>> +		sizeof(struct pata_falcon_priv), GFP_KERNEL);
>> +
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	ap->private_data = priv;
>> +
>> +	priv->swap_mask = pata_falcon_swap_mask;
>> +	if (priv->swap_mask)
>> +		priv->swap_data = 1;
>> +
>> +	if (MACH_IS_Q40) {
>> +		base = (void __iomem *)base_mem_res->start;
>> +		ap->ioaddr.data_addr		= base + 0;
>> +		base = (void __iomem *)base_res->start;
>> +		ap->ioaddr.error_addr		= base + 0x10000 + 1;
>> +		ap->ioaddr.feature_addr		= base + 0x10000 + 1;
>> +		ap->ioaddr.nsect_addr		= base + 0x10000 + 2;
>> +		ap->ioaddr.lbal_addr		= base + 0x10000 + 3;
>> +		ap->ioaddr.lbam_addr		= base + 0x10000 + 4;
>> +		ap->ioaddr.lbah_addr		= base + 0x10000 + 5;
>> +		ap->ioaddr.device_addr		= base + 0x10000 + 6;
>> +		ap->ioaddr.status_addr		= base + 0x10000 + 7;
>> +		ap->ioaddr.command_addr		= base + 0x10000 + 7;
>> +
>> +		base = (void __iomem *)ctl_res->start;
>> +		ap->ioaddr.altstatus_addr	= base + 0x10000;
>> +		ap->ioaddr.ctl_addr		= base + 0x10000;
>> +
>> +		ata_port_desc(ap, "cmd 0x%lx ctl 0x%lx",
>> +			      (unsigned long)base_res->start,
>> +			      (unsigned long)ctl_res->start);
>> +	} else {
>> +		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);
>> +	}
>>
>> 	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> 	if (irq_res && irq_res->start > 0) {
>> -- 
>> 2.17.1
>>
> _________________________________________________________________________
> William R Sowerbutts                                  will@sowerbutts.com
> "Carpe post meridiem"                               http://sowerbutts.com
>           main(){char*s=">#=0> ^#X@#@^7=",c=0,m;for(;c<15;c++)for
>           (m=-1;m<7;putchar(m++/6&c%3/2?10:s[c]-31&1<<m?42:32));}
>