[PATCH 0/3] ata: Module parameter clean-ups for pata_legacy and pata_platform

[PATCH 0/3] ata: Module parameter clean-ups for pata_legacy and pata_platform

[Maciej W. Rozycki]

Hi,

 In the course of looking into Christoph's recent proposal to drop legacy 
IDE drivers I have come across a number of issues with module parameters 
of the pata_legacy and pata_platform drivers: errors in documentation 
present in the comment form, missing user-visible documentation, and 
unconditional poking at ISA I/O ports in pata_legacy that isn't there with 
the old ide-generic driver (the lack of `probe_mask' parameter).

 Here's a small patch series that addresses these issues.  Overall I 
find the design of the pata_legacy driver's options a bit messy, e.g. the 
`all' vs the `probe_all' parameter, and the interpretation of masks where 
bits correspond to probed PATA locations in a particular system (rather 
than either all known or all existing), but it's been there long enough I 
think we have to keep it, so I merely tried to describe the current 
semantics.  See the individual change descriptions for details.

 The changes have been run-time verified with an EISA system and a single 
ISA PATA adapter at the usual primary I/O location.  They have also been 
verified (mainly for the correctness of MODULE_PARM_DESC use) with an 
x86/PC build (for pata_legacy) and a MIPS/SWARM build (for pata_platform).

 Please apply.

  Maciej

[PATCH 1/3] pata_legacy: Properly document module parameters

[Maciej W. Rozycki]

Most pata_legacy module parameters lack MODULE_PARM_DESC documentation 
and none is described in kernel-parameters.txt.  Also several comments 
are inaccurate or wrong.

Add the missing documentation pieces then and reorder parameters into a 
consistent block.  Remove inaccuracies as follows:

- `all' affects primary and secondary port ranges only rather than all,

- `probe_all' affects tertiary and further port ranges rather than all,

- `ht6560b' is for HT 6560B rather than HT 6560A.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
 Documentation/admin-guide/kernel-parameters.txt |   72 ++++++++++++++++++
 drivers/ata/pata_legacy.c                       |   92 ++++++++++++++----------
 2 files changed, 128 insertions(+), 36 deletions(-)

linux-pata-legacy-parm-desc.diff
Index: linux-dolch/Documentation/admin-guide/kernel-parameters.txt
===================================================================
--- linux-dolch.orig/Documentation/admin-guide/kernel-parameters.txt
+++ linux-dolch/Documentation/admin-guide/kernel-parameters.txt
@@ -3568,6 +3568,78 @@
 			Currently this function knows 686a and 8231 chips.
 			Format: [spp|ps2|epp|ecp|ecpepp]
 
+	pata_legacy.all=	[HW,LIBATA]
+			Format: <int>
+			Set to non-zero to probe primary and secondary ISA
+			port ranges on PCI systems where no PCI PATA device
+			has been found at either range.  Disabled by default.
+
+	pata_legacy.autospeed=	[HW,LIBATA]
+			Format: <int>
+			Set to non-zero if a chip is present that snoops speed
+			changes.  Disabled by default.
+
+	pata_legacy.ht6560a=	[HW,LIBATA]
+			Format: <int>
+			Set to 1, 2, or 3 for HT 6560A on the primary channel,
+			the secondary channel, or both channels respectively.
+			Disabled by default.
+
+	pata_legacy.ht6560b=	[HW,LIBATA]
+			Format: <int>
+			Set to 1, 2, or 3 for HT 6560B on the primary channel,
+			the secondary channel, or both channels respectively.
+			Disabled by default.
+
+	pata_legacy.iordy_mask=	[HW,LIBATA]
+			Format: <int>
+			IORDY enable mask.  Set individual bits to allow IORDY
+			for the respective channel.  Bit 0 is for the first
+			legacy channel handled by this driver, bit 1 is for
+			the second channel, and so on.  The sequence will often
+			correspond to the primary legacy channel, the secondary
+			legacy channel, and so on, but the handling of a PCI
+			bus and the use of other driver options may interfere
+			with the sequence.  By default IORDY is allowed across
+			all channels.
+
+	pata_legacy.opti82c46x=	[HW,LIBATA]
+			Format: <int>
+			Set to 1, 2, or 3 for Opti 82c611A on the primary
+			channel, the secondary channel, or both channels
+			respectively.  Disabled by default.
+
+	pata_legacy.opti82c611a=	[HW,LIBATA]
+			Format: <int>
+			Set to 1, 2, or 3 for Opti 82c465MV on the primary
+			channel, the secondary channel, or both channels
+			respectively.  Disabled by default.
+
+	pata_legacy.pio_mask=	[HW,LIBATA]
+			Format: <int>
+			PIO mode mask for autospeed devices.  Set individual
+			bits to allow the use of the respective PIO modes.
+			Bit 0 is for mode 0, bit 1 is for mode 1, and so on.
+			All modes allowed by default.
+
+	pata_legacy.probe_all=	[HW,LIBATA]
+			Format: <int>
+			Set to non-zero to probe tertiary and further ISA
+			port ranges on PCI systems.  Disabled by default.
+
+	pata_legacy.qdi=	[HW,LIBATA]
+			Format: <int>
+			Set to non-zero to probe QDI controllers.  By default
+			set to 1 if CONFIG_PATA_QDI_MODULE, 0 otherwise.
+
+	pata_legacy.winbond=	[HW,LIBATA]
+			Format: <int>
+			Set to non-zero to probe Winbond controllers.  Use
+			the standard I/O port (0x130) if 1, otherwise the
+			value given is the I/O port to use (typically 0x1b0).
+			By default set to 1 if CONFIG_PATA_WINBOND_VLB_MODULE,
+			0 otherwise.
+
 	pause_on_oops=
 			Halt all CPUs after the first oops has been printed for
 			the specified number of seconds.  This is to be used if
Index: linux-dolch/drivers/ata/pata_legacy.c
===================================================================
--- linux-dolch.orig/drivers/ata/pata_legacy.c
+++ linux-dolch/drivers/ata/pata_legacy.c
@@ -63,7 +63,62 @@
 
 static int all;
 module_param(all, int, 0444);
-MODULE_PARM_DESC(all, "Grab all legacy port devices, even if PCI(0=off, 1=on)");
+MODULE_PARM_DESC(all,
+		 "Set to probe unclaimed pri/sec ISA port ranges even if PCI");
+
+static int probe_all;
+module_param(probe_all, int, 0);
+MODULE_PARM_DESC(probe_all,
+		 "Set to probe tertiary+ ISA port ranges even if PCI");
+
+static int autospeed;
+module_param(autospeed, int, 0);
+MODULE_PARM_DESC(autospeed, "Chip present that snoops speed changes");
+
+static int pio_mask = ATA_PIO4;
+module_param(pio_mask, int, 0);
+MODULE_PARM_DESC(pio_mask, "PIO range for autospeed devices");
+
+static int iordy_mask = 0xFFFFFFFF;
+module_param(iordy_mask, int, 0);
+MODULE_PARM_DESC(iordy_mask, "Use IORDY if available");
+
+static int ht6560a;
+module_param(ht6560a, int, 0);
+MODULE_PARM_DESC(ht6560a, "HT 6560A on primary 1, second 2, both 3");
+
+static int ht6560b;
+module_param(ht6560b, int, 0);
+MODULE_PARM_DESC(ht6560b, "HT 6560B on primary 1, secondary 2, both 3");
+
+static int opti82c611a;
+module_param(opti82c611a, int, 0);
+MODULE_PARM_DESC(opti82c611a,
+		 "Opti 82c611A on primary 1, secondary 2, both 3");
+
+static int opti82c46x;
+module_param(opti82c46x, int, 0);
+MODULE_PARM_DESC(opti82c46x,
+		 "Opti 82c465MV on primary 1, secondary 2, both 3");
+
+#ifdef CONFIG_PATA_QDI_MODULE
+static int qdi = 1;
+#else
+static int qdi;
+#endif
+module_param(qdi, int, 0);
+MODULE_PARM_DESC(qdi, "Set to probe QDI controllers");
+
+#ifdef CONFIG_PATA_WINBOND_VLB_MODULE
+static int winbond = 1;
+#else
+static int winbond;
+#endif
+module_param(winbond, int, 0);
+MODULE_PARM_DESC(winbond,
+		 "Set to probe Winbond controllers, "
+		 "give I/O port if non standard");
+
 
 enum controller {
 	BIOS = 0,
@@ -117,30 +172,6 @@ static struct ata_host *legacy_host[NR_H
 static int nr_legacy_host;
 
 
-static int probe_all;		/* Set to check all ISA port ranges */
-static int ht6560a;		/* HT 6560A on primary 1, second 2, both 3 */
-static int ht6560b;		/* HT 6560A on primary 1, second 2, both 3 */
-static int opti82c611a;		/* Opti82c611A on primary 1, sec 2, both 3 */
-static int opti82c46x;		/* Opti 82c465MV present(pri/sec autodetect) */
-static int autospeed;		/* Chip present which snoops speed changes */
-static int pio_mask = ATA_PIO4;	/* PIO range for autospeed devices */
-static int iordy_mask = 0xFFFFFFFF;	/* Use iordy if available */
-
-/* Set to probe QDI controllers */
-#ifdef CONFIG_PATA_QDI_MODULE
-static int qdi = 1;
-#else
-static int qdi;
-#endif
-
-#ifdef CONFIG_PATA_WINBOND_VLB_MODULE
-static int winbond = 1;		/* Set to probe Winbond controllers,
-					give I/O port if non standard */
-#else
-static int winbond;		/* Set to probe Winbond controllers,
-					give I/O port if non standard */
-#endif
-
 /**
  *	legacy_probe_add	-	Add interface to probe list
  *	@port: Controller port
@@ -1250,16 +1281,5 @@ MODULE_VERSION(DRV_VERSION);
 MODULE_ALIAS("pata_qdi");
 MODULE_ALIAS("pata_winbond");
 
-module_param(probe_all, int, 0);
-module_param(autospeed, int, 0);
-module_param(ht6560a, int, 0);
-module_param(ht6560b, int, 0);
-module_param(opti82c611a, int, 0);
-module_param(opti82c46x, int, 0);
-module_param(qdi, int, 0);
-module_param(winbond, int, 0);
-module_param(pio_mask, int, 0);
-module_param(iordy_mask, int, 0);
-
 module_init(legacy_init);
 module_exit(legacy_exit);

Re: [PATCH 1/3] pata_legacy: Properly document module parameters

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/3] pata_platform: Document `pio_mask' module parameter

[Maciej W. Rozycki]

Add MODULE_PARM_DESC documentation and a kernel-parameters.txt entry.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
 Documentation/admin-guide/kernel-parameters.txt |    7 +++++++
 drivers/ata/pata_platform.c                     |    4 ++--
 2 files changed, 9 insertions(+), 2 deletions(-)

linux-pata-platform-parm-desc.diff
Index: linux-dolch/Documentation/admin-guide/kernel-parameters.txt
===================================================================
--- linux-dolch.orig/Documentation/admin-guide/kernel-parameters.txt
+++ linux-dolch/Documentation/admin-guide/kernel-parameters.txt
@@ -3640,6 +3640,13 @@
 			By default set to 1 if CONFIG_PATA_WINBOND_VLB_MODULE,
 			0 otherwise.
 
+	pata_platform.pio_mask=	[HW,LIBATA]
+			Format: <int>
+			Supported PIO mode mask.  Set individual bits to allow
+			the use of the respective PIO modes.  Bit 0 is for
+			mode 0, bit 1 is for mode 1, and so on.  Mode 0 only
+			allowed by default.
+
 	pause_on_oops=
 			Halt all CPUs after the first oops has been printed for
 			the specified number of seconds.  This is to be used if
Index: linux-dolch/drivers/ata/pata_platform.c
===================================================================
--- linux-dolch.orig/drivers/ata/pata_platform.c
+++ linux-dolch/drivers/ata/pata_platform.c
@@ -24,6 +24,8 @@
 #define DRV_VERSION "1.2"
 
 static int pio_mask = 1;
+module_param(pio_mask, int, 0);
+MODULE_PARM_DESC(pio_mask, "PIO modes supported, mode 0 only by default");
 
 /*
  * Provide our own set_mode() as we don't want to change anything that has
@@ -233,8 +235,6 @@ static struct platform_driver pata_platf
 
 module_platform_driver(pata_platform_driver);
 
-module_param(pio_mask, int, 0);
-
 MODULE_AUTHOR("Paul Mundt");
 MODULE_DESCRIPTION("low-level driver for platform device ATA");
 MODULE_LICENSE("GPL");

Re: [PATCH 2/3] pata_platform: Document `pio_mask' module parameter

[Christoph Hellwig]

On Sun, Mar 21, 2021 at 08:55:27PM +0100, Maciej W. Rozycki wrote:
> Add MODULE_PARM_DESC documentation and a kernel-parameters.txt entry.
> 
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/3] pata_legacy: Add `probe_mask' parameter like with ide-generic

[Maciej W. Rozycki]

Carry the `probe_mask' parameter over from ide-generic to pata_legacy so 
that there is a way to prevent random poking at ISA port I/O locations 
in attempt to discover adapter option cards with libata like with the 
old IDE driver.  By default all enabled locations are tried, however it 
may interfere with a different kind of hardware responding there.

For example with a plain (E)ISA system the driver tries all the six 
possible locations:

scsi host0: pata_legacy
ata1: PATA max PIO4 cmd 0x1f0 ctl 0x3f6 irq 14
ata1.00: ATA-4: ST310211A, 3.54, max UDMA/100
ata1.00: 19541088 sectors, multi 16: LBA
ata1.00: configured for PIO
scsi 0:0:0:0: Direct-Access     ATA      ST310211A        3.54 PQ: 0 ANSI: 5
scsi 0:0:0:0: Attached scsi generic sg0 type 0
sd 0:0:0:0: [sda] 19541088 512-byte logical blocks: (10.0 GB/9.32 GiB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
 sda: sda1 sda2 sda3
sd 0:0:0:0: [sda] Attached SCSI disk
scsi host1: pata_legacy
ata2: PATA max PIO4 cmd 0x170 ctl 0x376 irq 15
scsi host1: pata_legacy
ata3: PATA max PIO4 cmd 0x1e8 ctl 0x3ee irq 11
scsi host1: pata_legacy
ata4: PATA max PIO4 cmd 0x168 ctl 0x36e irq 10
scsi host1: pata_legacy
ata5: PATA max PIO4 cmd 0x1e0 ctl 0x3e6 irq 8
scsi host1: pata_legacy
ata6: PATA max PIO4 cmd 0x160 ctl 0x366 irq 12

however giving the kernel "pata_legacy.probe_mask=21" makes it try every 
other location only:

scsi host0: pata_legacy
ata1: PATA max PIO4 cmd 0x1f0 ctl 0x3f6 irq 14
ata1.00: ATA-4: ST310211A, 3.54, max UDMA/100
ata1.00: 19541088 sectors, multi 16: LBA
ata1.00: configured for PIO
scsi 0:0:0:0: Direct-Access     ATA      ST310211A        3.54 PQ: 0 ANSI: 5
scsi 0:0:0:0: Attached scsi generic sg0 type 0
sd 0:0:0:0: [sda] 19541088 512-byte logical blocks: (10.0 GB/9.32 GiB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
 sda: sda1 sda2 sda3
sd 0:0:0:0: [sda] Attached SCSI disk
scsi host1: pata_legacy
ata2: PATA max PIO4 cmd 0x1e8 ctl 0x3ee irq 11
scsi host1: pata_legacy
ata3: PATA max PIO4 cmd 0x1e0 ctl 0x3e6 irq 8

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
 Documentation/admin-guide/kernel-parameters.txt |   11 +++++++++++
 drivers/ata/pata_legacy.c                       |    6 ++++++
 2 files changed, 17 insertions(+)

linux-pata-legacy-probe-mask.diff
Index: linux-dolch/Documentation/admin-guide/kernel-parameters.txt
===================================================================
--- linux-dolch.orig/Documentation/admin-guide/kernel-parameters.txt
+++ linux-dolch/Documentation/admin-guide/kernel-parameters.txt
@@ -3627,6 +3627,17 @@
 			Set to non-zero to probe tertiary and further ISA
 			port ranges on PCI systems.  Disabled by default.
 
+	pata_legacy.probe_mask=	[HW,LIBATA]
+			Format: <int>
+			Probe mask for legacy ISA PATA ports.  Depending on
+			platform configuration and the use of other driver
+			options up to 6 legacy ports are supported: 0x1f0,
+			0x170, 0x1e8, 0x168, 0x1e0, 0x160, however probing
+			of individual ports can be disabled by setting the
+			corresponding bits in the mask to 1.  Bit 0 is for
+			the first port in the list above (0x1f0), and so on.
+			By default all supported ports are probed.
+
 	pata_legacy.qdi=	[HW,LIBATA]
 			Format: <int>
 			Set to non-zero to probe QDI controllers.  By default
Index: linux-dolch/drivers/ata/pata_legacy.c
===================================================================
--- linux-dolch.orig/drivers/ata/pata_legacy.c
+++ linux-dolch/drivers/ata/pata_legacy.c
@@ -71,6 +71,10 @@ module_param(probe_all, int, 0);
 MODULE_PARM_DESC(probe_all,
 		 "Set to probe tertiary+ ISA port ranges even if PCI");
 
+static int probe_mask = ~0;
+module_param(probe_mask, int, 0);
+MODULE_PARM_DESC(probe_mask, "Probe mask for legacy ISA PATA ports");
+
 static int autospeed;
 module_param(autospeed, int, 0);
 MODULE_PARM_DESC(autospeed, "Chip present that snoops speed changes");
@@ -199,6 +203,8 @@ static int legacy_probe_add(unsigned lon
 			free = lp;
 		/* Matching port, or the correct slot for ordering */
 		if (lp->port == port || legacy_port[i] == port) {
+			if (!(probe_mask & 1 << i))
+				return -1;
 			free = lp;
 			break;
 		}

Re: [PATCH 3/3] pata_legacy: Add `probe_mask' parameter like with ide-generic

[Christoph Hellwig]

The interface looks a little strange, but I guess deviating from the
old ide interface for something that has only such a use case in
retrocomputing probably doesn't make much sense.

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 0/3] ata: Module parameter clean-ups for pata_legacy and pata_platform

[Jens Axboe]

On 3/21/21 1:55 PM, Maciej W. Rozycki wrote:
> Hi,
> 
>  In the course of looking into Christoph's recent proposal to drop legacy 
> IDE drivers I have come across a number of issues with module parameters 
> of the pata_legacy and pata_platform drivers: errors in documentation 
> present in the comment form, missing user-visible documentation, and 
> unconditional poking at ISA I/O ports in pata_legacy that isn't there with 
> the old ide-generic driver (the lack of `probe_mask' parameter).
> 
>  Here's a small patch series that addresses these issues.  Overall I 
> find the design of the pata_legacy driver's options a bit messy, e.g. the 
> `all' vs the `probe_all' parameter, and the interpretation of masks where 
> bits correspond to probed PATA locations in a particular system (rather 
> than either all known or all existing), but it's been there long enough I 
> think we have to keep it, so I merely tried to describe the current 
> semantics.  See the individual change descriptions for details.
> 
>  The changes have been run-time verified with an EISA system and a single 
> ISA PATA adapter at the usual primary I/O location.  They have also been 
> verified (mainly for the correctness of MODULE_PARM_DESC use) with an 
> x86/PC build (for pata_legacy) and a MIPS/SWARM build (for pata_platform).

Applied, thanks.

-- 
Jens Axboe