[PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable

[PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable

[Ondrej Zary]

Two bugfixes, one workaround and IDE command set registers support for fit3
driver. All needed for EXP Computer CD-865 drive with MC-1285B EPP cable to
work.

[PATCH 1/4] pata_parport: fix pata_parport_devchk

[Ondrej Zary]

There's a 'x' missing in 0x55 in pata_parport_devchk(), causing the
detection to always fail. Fix it.

Signed-off-by: Ondrej Zary <linux@zary.sk>
---
 drivers/ata/pata_parport/pata_parport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
index 1af64d435d3c..258d189f42e5 100644
--- a/drivers/ata/pata_parport/pata_parport.c
+++ b/drivers/ata/pata_parport/pata_parport.c
@@ -64,7 +64,7 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
 	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0xaa);
 	pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0x55);
 
-	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 055);
+	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0x55);
 	pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0xaa);
 
 	nsect = pi->proto->read_regr(pi, 0, ATA_REG_NSECT);
-- 
Ondrej Zary

[PATCH 2/4] pata_parport: implement set_devctl

[Ondrej Zary]

Add missing ops->sff_set_devctl implementation.

Signed-off-by: Ondrej Zary <linux@zary.sk>
---
 drivers/ata/pata_parport/pata_parport.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
index 258d189f42e5..cf87bbb52f1f 100644
--- a/drivers/ata/pata_parport/pata_parport.c
+++ b/drivers/ata/pata_parport/pata_parport.c
@@ -51,6 +51,13 @@ static void pata_parport_dev_select(struct ata_port *ap, unsigned int device)
 	ata_sff_pause(ap);
 }
 
+static void pata_parport_set_devctl(struct ata_port *ap, u8 ctl)
+{
+	struct pi_adapter *pi = ap->host->private_data;
+
+	pi->proto->write_regr(pi, 1, 6, ctl);
+}
+
 static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
 {
 	struct pi_adapter *pi = ap->host->private_data;
@@ -252,6 +259,7 @@ static struct ata_port_operations pata_parport_port_ops = {
 	.hardreset		= NULL,
 
 	.sff_dev_select		= pata_parport_dev_select,
+	.sff_set_devctl		= pata_parport_set_devctl,
 	.sff_check_status	= pata_parport_check_status,
 	.sff_check_altstatus	= pata_parport_check_altstatus,
 	.sff_tf_load		= pata_parport_tf_load,
-- 
Ondrej Zary

[PATCH 3/4] pata_parport: add custom version of wait_after_reset

[Ondrej Zary]

Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
bogus values when there's no master device present. This can cause
reset to fail, preventing the lone slave device (such as EXP Computer
CD-865) from working.

Add custom version of wait_after_reset that ignores master failure when
a slave device is present. The custom version is also needed because
the generic ata_sff_wait_after_reset uses direct port I/O for slave
device detection.

Signed-off-by: Ondrej Zary <linux@zary.sk>
---
 drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
index cf87bbb52f1f..b3db953e615a 100644
--- a/drivers/ata/pata_parport/pata_parport.c
+++ b/drivers/ata/pata_parport/pata_parport.c
@@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
 	return (nsect == 0x55) && (lbal == 0xaa);
 }
 
+static int pata_parport_wait_after_reset(struct ata_link *link,
+					 unsigned int devmask,
+					 unsigned long deadline)
+{
+	struct ata_port *ap = link->ap;
+	struct pi_adapter *pi = ap->host->private_data;
+	unsigned int dev0 = devmask & (1 << 0);
+	unsigned int dev1 = devmask & (1 << 1);
+	int rc, ret = 0;
+
+	ata_msleep(ap, ATA_WAIT_AFTER_RESET);
+
+	/* always check readiness of the master device */
+	rc = ata_sff_wait_ready(link, deadline);
+	/* some adapters return bogus values if master device is not present,
+	 * so don't abort now if a slave device is present
+	 */
+	if (rc) {
+		if (!dev1)
+			return rc;
+		ret = -ENODEV;
+	}
+
+	/* if device 1 was found in ata_devchk, wait for register
+	 * access briefly, then wait for BSY to clear.
+	 */
+	if (dev1) {
+		int i;
+
+		pata_parport_dev_select(ap, 1);
+
+		/* Wait for register access.  Some ATAPI devices fail
+		 * to set nsect/lbal after reset, so don't waste too
+		 * much time on it.  We're gonna wait for !BSY anyway.
+		 */
+		for (i = 0; i < 2; i++) {
+			u8 nsect, lbal;
+
+			nsect = pi->proto->read_regr(pi, 0, ATA_REG_NSECT);
+			lbal = pi->proto->read_regr(pi, 0, ATA_REG_LBAL);
+			if ((nsect == 1) && (lbal == 1))
+				break;
+			ata_msleep(ap, 50);	/* give drive a breather */
+		}
+
+		rc = ata_sff_wait_ready(link, deadline);
+		if (rc) {
+			if (rc != -ENODEV)
+				return rc;
+			ret = rc;
+		}
+	}
+
+	/* is all this really necessary? */
+	pata_parport_dev_select(ap, 0);
+	if (dev1)
+		pata_parport_dev_select(ap, 1);
+	if (dev0)
+		pata_parport_dev_select(ap, 0);
+
+	return ret;
+}
+
 static int pata_parport_bus_softreset(struct ata_port *ap, unsigned int devmask,
 				      unsigned long deadline)
 {
@@ -94,7 +157,7 @@ static int pata_parport_bus_softreset(struct ata_port *ap, unsigned int devmask,
 	ap->last_ctl = ap->ctl;
 
 	/* wait the port to become ready */
-	return ata_sff_wait_after_reset(&ap->link, devmask, deadline);
+	return pata_parport_wait_after_reset(&ap->link, devmask, deadline);
 }
 
 static int pata_parport_softreset(struct ata_link *link, unsigned int *classes,
-- 
Ondrej Zary

[PATCH 4/4] pata_parport-fit3: implement IDE command set registers

[Ondrej Zary]

fit3 protocol driver does not support accessing IDE control registers
(device control/altstatus). The DOS driver does not use these registers
either (as observed from DOSEMU trace). But the HW seems to be capable
of accessing these registers - I simply tried bit 3 and it works!

The control register is required to properly reset ATAPI devices or
they will be detected only once (after a power cycle).

Tested with EXP Computer CD-865 with MC-1285B EPP cable and
TransDisk 3000.

Signed-off-by: Ondrej Zary <linux@zary.sk>
---
 drivers/ata/pata_parport/fit3.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/pata_parport/fit3.c b/drivers/ata/pata_parport/fit3.c
index bad7aa920cdc..86b39966755b 100644
--- a/drivers/ata/pata_parport/fit3.c
+++ b/drivers/ata/pata_parport/fit3.c
@@ -9,11 +9,6 @@
  *
  * The TD-2000 and certain older devices use a different protocol.
  * Try the fit2 protocol module with them.
- *
- * NB:  The FIT adapters do not appear to support the control
- * registers.  So, we map ALT_STATUS to STATUS and NO-OP writes
- * to the device control register - this means that IDE reset
- * will not work on these devices.
  */
 
 #include <linux/module.h>
@@ -35,10 +30,11 @@
  * cont = 1 - access the IDE command set
  */
 
+static int cont_map[] = { 0x00, 0x08 };
+
 static void fit3_write_regr(struct pi_adapter *pi, int cont, int regr, int val)
 {
-	if (cont == 1)
-		return;
+	regr += cont_map[cont];
 
 	switch (pi->mode) {
 	case 0:
@@ -59,11 +55,7 @@ static int fit3_read_regr(struct pi_adapter *pi, int cont, int regr)
 {
 	int  a, b;
 
-	if (cont) {
-		if (regr != 6)
-			return 0xff;
-		regr = 7;
-	}
+	regr += cont_map[cont];
 
 	switch (pi->mode) {
 	case 0:
-- 
Ondrej Zary

Re: [PATCH 1/4] pata_parport: fix pata_parport_devchk

[Sergey Shtylyov]

Hello!

On 9/30/23 10:15 PM, Ondrej Zary wrote:

> There's a 'x' missing in 0x55 in pata_parport_devchk(), causing the
> detection to always fail. Fix it.
> 
> Signed-off-by: Ondrej Zary <linux@zary.sk>

   I think we need a Fixes: tag here...

> ---
>  drivers/ata/pata_parport/pata_parport.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index 1af64d435d3c..258d189f42e5 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c
> @@ -64,7 +64,7 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
>  	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0xaa);
>  	pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0x55);
>  
> -	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 055);
> +	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0x55);

   Oh, Gawd! How did this ever work?! :-/
   This bug seems to predate the Big PARIDE move...

>  	pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0xaa);
>  
>  	nsect = pi->proto->read_regr(pi, 0, ATA_REG_NSECT);
> 

MBR, Sergey

Re: [PATCH 1/4] pata_parport: fix pata_parport_devchk

[Sergey Shtylyov]

On 10/2/23 9:43 PM, Sergey Shtylyov wrote:
[...]

>> There's a 'x' missing in 0x55 in pata_parport_devchk(), causing the
>> detection to always fail. Fix it.
>>
>> Signed-off-by: Ondrej Zary <linux@zary.sk>
> 
>    I think we need a Fixes: tag here...

Fixes: 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")

[...]

MBR, Sergey

Re: [PATCH 2/4] pata_parport: implement set_devctl

[Sergey Shtylyov]

On 9/30/23 10:15 PM, Ondrej Zary wrote:

> Add missing ops->sff_set_devctl implementation.

   This is also a bug fix, right?

> Signed-off-by: Ondrej Zary <linux@zary.sk>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>
Fixes: 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")

[...]

MBR, Sergey

Re: [PATCH 4/4] pata_parport-fit3: implement IDE command set registers

[Sergey Shtylyov]

On 9/30/23 10:15 PM, Ondrej Zary wrote:

> fit3 protocol driver does not support accessing IDE control registers
> (device control/altstatus). The DOS driver does not use these registers
> either (as observed from DOSEMU trace). But the HW seems to be capable
> of accessing these registers - I simply tried bit 3 and it works!
> 
> The control register is required to properly reset ATAPI devices or
> they will be detected only once (after a power cycle).
> 
> Tested with EXP Computer CD-865 with MC-1285B EPP cable and
> TransDisk 3000.
> 
> Signed-off-by: Ondrej Zary <linux@zary.sk>
> ---
>  drivers/ata/pata_parport/fit3.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ata/pata_parport/fit3.c b/drivers/ata/pata_parport/fit3.c
> index bad7aa920cdc..86b39966755b 100644
> --- a/drivers/ata/pata_parport/fit3.c
> +++ b/drivers/ata/pata_parport/fit3.c
[...]
> @@ -35,10 +30,11 @@
>   * cont = 1 - access the IDE command set
>   */
>  
> +static int cont_map[] = { 0x00, 0x08 };

   const?

> +
>  static void fit3_write_regr(struct pi_adapter *pi, int cont, int regr, int val)
>  {
> -	if (cont == 1)
> -		return;
> +	regr += cont_map[cont];

   Perhaps regr += cont << 3 instead?

>  
>  	switch (pi->mode) {
>  	case 0:
> @@ -59,11 +55,7 @@ static int fit3_read_regr(struct pi_adapter *pi, int cont, int regr)
>  {
>  	int  a, b;
>  
> -	if (cont) {
> -		if (regr != 6)
> -			return 0xff;
> -		regr = 7;
> -	}
> +	regr += cont_map[cont];

   Likewise here?

[...]

MBR, Sergey

Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset

[Sergey Shtylyov]

On 9/30/23 10:15 PM, Ondrej Zary wrote:

> Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
> bogus values when there's no master device present. This can cause
> reset to fail, preventing the lone slave device (such as EXP Computer
> CD-865) from working.
> 
> Add custom version of wait_after_reset that ignores master failure when
> a slave device is present. The custom version is also needed because
> the generic ata_sff_wait_after_reset uses direct port I/O for slave
> device detection.
> 
> Signed-off-by: Ondrej Zary <linux@zary.sk>
> ---
>  drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index cf87bbb52f1f..b3db953e615a 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c
> @@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
>  	return (nsect == 0x55) && (lbal == 0xaa);
>  }
>  
> +static int pata_parport_wait_after_reset(struct ata_link *link,
> +					 unsigned int devmask,
> +					 unsigned long deadline)
> +{
> +	struct ata_port *ap = link->ap;
> +	struct pi_adapter *pi = ap->host->private_data;
> +	unsigned int dev0 = devmask & (1 << 0);
> +	unsigned int dev1 = devmask & (1 << 1);
> +	int rc, ret = 0;
> +
> +	ata_msleep(ap, ATA_WAIT_AFTER_RESET);
> +
> +	/* always check readiness of the master device */
> +	rc = ata_sff_wait_ready(link, deadline);
> +	/* some adapters return bogus values if master device is not present,

   The multiline comments should start with /* on its own line.
   Have you run scripts/checkpatch.pl on the patches?

> +	 * so don't abort now if a slave device is present
> +	 */
> +	if (rc) {
> +		if (!dev1)
> +			return rc;
> +		ret = -ENODEV;
> +	}
> +
> +	/* if device 1 was found in ata_devchk, wait for register

   Likewise here...

> +	 * access briefly, then wait for BSY to clear.
> +	 */
> +	if (dev1) {
> +		int i;
> +
> +		pata_parport_dev_select(ap, 1);
> +
> +		/* Wait for register access.  Some ATAPI devices fail

   And here...

[...]

MBR, Sergey

Re: [PATCH 0/4] pata_parport: fix EXP Computer CD-865 with MC-1285B EPP cable

[Damien Le Moal]

On 10/1/23 04:15, Ondrej Zary wrote:
> 
> Two bugfixes, one workaround and IDE command set registers support for fit3
> driver. All needed for EXP Computer CD-865 drive with MC-1285B EPP cable to
> work.
> 

Please use my email address listed by get_maintainer.pl: dlemoal@kernel.org

And these patches do not involve linux-block and Jens, so no need to send there
(that reduces noise on the block list).

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset

[Damien Le Moal]

On 10/1/23 04:15, Ondrej Zary wrote:
> Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
> bogus values when there's no master device present. This can cause
> reset to fail, preventing the lone slave device (such as EXP Computer
> CD-865) from working.
> 
> Add custom version of wait_after_reset that ignores master failure when
> a slave device is present. The custom version is also needed because
> the generic ata_sff_wait_after_reset uses direct port I/O for slave
> device detection.
> 
> Signed-off-by: Ondrej Zary <linux@zary.sk>
> ---
>  drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index cf87bbb52f1f..b3db953e615a 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c
> @@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
>  	return (nsect == 0x55) && (lbal == 0xaa);
>  }
>  
> +static int pata_parport_wait_after_reset(struct ata_link *link,
> +					 unsigned int devmask,
> +					 unsigned long deadline)
> +{
> +	struct ata_port *ap = link->ap;
> +	struct pi_adapter *pi = ap->host->private_data;
> +	unsigned int dev0 = devmask & (1 << 0);
> +	unsigned int dev1 = devmask & (1 << 1);
> +	int rc, ret = 0;
> +
> +	ata_msleep(ap, ATA_WAIT_AFTER_RESET);
> +
> +	/* always check readiness of the master device */
> +	rc = ata_sff_wait_ready(link, deadline);
> +	/* some adapters return bogus values if master device is not present,
> +	 * so don't abort now if a slave device is present
> +	 */

In addition to Sergey's comment, please move this comment inside the "if", or
even better, merge it with the otherwise not very useful "always check
readiness..." comment.

> +	if (rc) {
> +		if (!dev1)
> +			return rc;
> +		ret = -ENODEV;
> +	}
> +
> +	/* if device 1 was found in ata_devchk, wait for register
> +	 * access briefly, then wait for BSY to clear.
> +	 */
> +	if (dev1) {
> +		int i;
> +
> +		pata_parport_dev_select(ap, 1);
> +
> +		/* Wait for register access.  Some ATAPI devices fail
> +		 * to set nsect/lbal after reset, so don't waste too
> +		 * much time on it.  We're gonna wait for !BSY anyway.
> +		 */
> +		for (i = 0; i < 2; i++) {
> +			u8 nsect, lbal;
> +
> +			nsect = pi->proto->read_regr(pi, 0, ATA_REG_NSECT);
> +			lbal = pi->proto->read_regr(pi, 0, ATA_REG_LBAL);
> +			if ((nsect == 1) && (lbal == 1))
> +				break;
> +			ata_msleep(ap, 50);	/* give drive a breather */

Please move the comment on its own line above the sleep call.

> +		}
> +
> +		rc = ata_sff_wait_ready(link, deadline);
> +		if (rc) {
> +			if (rc != -ENODEV)
> +				return rc;
> +			ret = rc;
> +		}
> +	}
> +
> +	/* is all this really necessary? */

I don't know. It is your driver... So either drop this comment, or clearly
explain why this is done.

> +	pata_parport_dev_select(ap, 0);
> +	if (dev1)
> +		pata_parport_dev_select(ap, 1);
> +	if (dev0)
> +		pata_parport_dev_select(ap, 0);

Can you have dev1 && dev0 == true ? This seems like the second if should be an
"else if", but it is not clear what this is doing.

> +
> +	return ret;
> +}
> +
>  static int pata_parport_bus_softreset(struct ata_port *ap, unsigned int devmask,
>  				      unsigned long deadline)
>  {
> @@ -94,7 +157,7 @@ static int pata_parport_bus_softreset(struct ata_port *ap, unsigned int devmask,
>  	ap->last_ctl = ap->ctl;
>  
>  	/* wait the port to become ready */
> -	return ata_sff_wait_after_reset(&ap->link, devmask, deadline);
> +	return pata_parport_wait_after_reset(&ap->link, devmask, deadline);
>  }
>  
>  static int pata_parport_softreset(struct ata_link *link, unsigned int *classes,

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/4] pata_parport-fit3: implement IDE command set registers

[Damien Le Moal]

On 10/1/23 04:15, Ondrej Zary wrote:
> fit3 protocol driver does not support accessing IDE control registers
> (device control/altstatus). The DOS driver does not use these registers
> either (as observed from DOSEMU trace). But the HW seems to be capable
> of accessing these registers - I simply tried bit 3 and it works!
> 
> The control register is required to properly reset ATAPI devices or
> they will be detected only once (after a power cycle).
> 
> Tested with EXP Computer CD-865 with MC-1285B EPP cable and
> TransDisk 3000.
> 
> Signed-off-by: Ondrej Zary <linux@zary.sk>

For all patches of this series: please change the commit title to:

ata: pata_parport: xxx

> ---
>  drivers/ata/pata_parport/fit3.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ata/pata_parport/fit3.c b/drivers/ata/pata_parport/fit3.c
> index bad7aa920cdc..86b39966755b 100644
> --- a/drivers/ata/pata_parport/fit3.c
> +++ b/drivers/ata/pata_parport/fit3.c
> @@ -9,11 +9,6 @@
>   *
>   * The TD-2000 and certain older devices use a different protocol.
>   * Try the fit2 protocol module with them.
> - *
> - * NB:  The FIT adapters do not appear to support the control
> - * registers.  So, we map ALT_STATUS to STATUS and NO-OP writes
> - * to the device control register - this means that IDE reset
> - * will not work on these devices.
>   */
>  
>  #include <linux/module.h>
> @@ -35,10 +30,11 @@
>   * cont = 1 - access the IDE command set
>   */
>  
> +static int cont_map[] = { 0x00, 0x08 };
> +
>  static void fit3_write_regr(struct pi_adapter *pi, int cont, int regr, int val)
>  {
> -	if (cont == 1)
> -		return;
> +	regr += cont_map[cont];
>  
>  	switch (pi->mode) {
>  	case 0:
> @@ -59,11 +55,7 @@ static int fit3_read_regr(struct pi_adapter *pi, int cont, int regr)
>  {
>  	int  a, b;
>  
> -	if (cont) {
> -		if (regr != 6)
> -			return 0xff;
> -		regr = 7;
> -	}
> +	regr += cont_map[cont];
>  
>  	switch (pi->mode) {
>  	case 0:

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset

[Sergey Shtylyov]

On 10/3/23 3:55 AM, Damien Le Moal wrote:

[...]
>> Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
>> bogus values when there's no master device present. This can cause
>> reset to fail, preventing the lone slave device (such as EXP Computer
>> CD-865) from working.
>>
>> Add custom version of wait_after_reset that ignores master failure when
>> a slave device is present. The custom version is also needed because
>> the generic ata_sff_wait_after_reset uses direct port I/O for slave
>> device detection.
>>
>> Signed-off-by: Ondrej Zary <linux@zary.sk>
>> ---
>>  drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
>>  1 file changed, 64 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
>> index cf87bbb52f1f..b3db953e615a 100644
>> --- a/drivers/ata/pata_parport/pata_parport.c
>> +++ b/drivers/ata/pata_parport/pata_parport.c
>> @@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
>>  	return (nsect == 0x55) && (lbal == 0xaa);
>>  }
>>  
>> +static int pata_parport_wait_after_reset(struct ata_link *link,
>> +					 unsigned int devmask,
>> +					 unsigned long deadline)
>> +{
>> +	struct ata_port *ap = link->ap;
>> +	struct pi_adapter *pi = ap->host->private_data;
>> +	unsigned int dev0 = devmask & (1 << 0);
>> +	unsigned int dev1 = devmask & (1 << 1);
>> +	int rc, ret = 0;
>> +
>> +	ata_msleep(ap, ATA_WAIT_AFTER_RESET);
>> +
>> +	/* always check readiness of the master device */
>> +	rc = ata_sff_wait_ready(link, deadline);
>> +	/* some adapters return bogus values if master device is not present,
>> +	 * so don't abort now if a slave device is present
>> +	 */
> 
> In addition to Sergey's comment, please move this comment inside the "if", or
> even better, merge it with the otherwise not very useful "always check
> readiness..." comment.

   That comment was copied from ata_sff_wait_after_reset(), I think...

[...]
>> +	/* if device 1 was found in ata_devchk, wait for register
>> +	 * access briefly, then wait for BSY to clear.
>> +	 */
>> +	if (dev1) {
>> +		int i;
>> +
>> +		pata_parport_dev_select(ap, 1);
>> +
>> +		/* Wait for register access.  Some ATAPI devices fail
>> +		 * to set nsect/lbal after reset, so don't waste too
>> +		 * much time on it.  We're gonna wait for !BSY anyway.
>> +		 */
>> +		for (i = 0; i < 2; i++) {
>> +			u8 nsect, lbal;
>> +
>> +			nsect = pi->proto->read_regr(pi, 0, ATA_REG_NSECT);
>> +			lbal = pi->proto->read_regr(pi, 0, ATA_REG_LBAL);
>> +			if ((nsect == 1) && (lbal == 1))
>> +				break;
>> +			ata_msleep(ap, 50);	/* give drive a breather */
> 
> Please move the comment on its own line above the sleep call.

   Again, copied verbatim from ata_sff_wait_after_reset()...

>> +		}
>> +
>> +		rc = ata_sff_wait_ready(link, deadline);
>> +		if (rc) {
>> +			if (rc != -ENODEV)
>> +				return rc;
>> +			ret = rc;
>> +		}
>> +	}
>> +
>> +	/* is all this really necessary? */
> 
> I don't know. It is your driver... So either drop this comment, or clearly
> explain why this is done.

   And again, copied verbatim from ata_sff_wait_after_reset()...

>> +	pata_parport_dev_select(ap, 0);
>> +	if (dev1)
>> +		pata_parport_dev_select(ap, 1);
>> +	if (dev0)
>> +		pata_parport_dev_select(ap, 0);
> 
> Can you have dev1 && dev0 == true ? This seems like the second if should be an
> "else if", but it is not clear what this is doing.

   I guess this tries to leave the valid taskfile regs readable on a channel, instead
of just 0xff...

[...]

MBR, Sergey

Re: [PATCH 1/4] pata_parport: fix pata_parport_devchk

[Ondrej Zary]

On Monday 02 October 2023 20:43:09 Sergey Shtylyov wrote:
> Hello!
> 
> On 9/30/23 10:15 PM, Ondrej Zary wrote:
> 
> > There's a 'x' missing in 0x55 in pata_parport_devchk(), causing the
> > detection to always fail. Fix it.
> > 
> > Signed-off-by: Ondrej Zary <linux@zary.sk>
> 
>    I think we need a Fixes: tag here...
> 
> > ---
> >  drivers/ata/pata_parport/pata_parport.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> > index 1af64d435d3c..258d189f42e5 100644
> > --- a/drivers/ata/pata_parport/pata_parport.c
> > +++ b/drivers/ata/pata_parport/pata_parport.c
> > @@ -64,7 +64,7 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
> >  	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0xaa);
> >  	pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0x55);
> >  
> > -	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 055);
> > +	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0x55);
> 
>    Oh, Gawd! How did this ever work?! :-/
>    This bug seems to predate the Big PARIDE move...

This code was not present in PARIDE - it's my bug.
The function is a clone of ata_devchk() without direct port access.
It's called only from softreset so nobody notices the breakage until something goes wrong. The CD-865 drive needs a reset to start working.

-- 
Ondrej Zary

Re: [PATCH 4/4] pata_parport-fit3: implement IDE command set registers

[Ondrej Zary]

On Monday 02 October 2023 22:06:52 Sergey Shtylyov wrote:
> On 9/30/23 10:15 PM, Ondrej Zary wrote:
> 
> > fit3 protocol driver does not support accessing IDE control registers
> > (device control/altstatus). The DOS driver does not use these registers
> > either (as observed from DOSEMU trace). But the HW seems to be capable
> > of accessing these registers - I simply tried bit 3 and it works!
> > 
> > The control register is required to properly reset ATAPI devices or
> > they will be detected only once (after a power cycle).
> > 
> > Tested with EXP Computer CD-865 with MC-1285B EPP cable and
> > TransDisk 3000.
> > 
> > Signed-off-by: Ondrej Zary <linux@zary.sk>
> > ---
> >  drivers/ata/pata_parport/fit3.c | 16 ++++------------
> >  1 file changed, 4 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/ata/pata_parport/fit3.c b/drivers/ata/pata_parport/fit3.c
> > index bad7aa920cdc..86b39966755b 100644
> > --- a/drivers/ata/pata_parport/fit3.c
> > +++ b/drivers/ata/pata_parport/fit3.c
> [...]
> > @@ -35,10 +30,11 @@
> >   * cont = 1 - access the IDE command set
> >   */
> >  
> > +static int cont_map[] = { 0x00, 0x08 };
> 
>    const?

There's no 'const' in other protocol drivers. Maybe it should be added to all of them.

> > +
> >  static void fit3_write_regr(struct pi_adapter *pi, int cont, int regr, int val)
> >  {
> > -	if (cont == 1)
> > -		return;
> > +	regr += cont_map[cont];
> 
>    Perhaps regr += cont << 3 instead?

I matched the style used by other protocol drivers.

> >  
> >  	switch (pi->mode) {
> >  	case 0:
> > @@ -59,11 +55,7 @@ static int fit3_read_regr(struct pi_adapter *pi, int cont, int regr)
> >  {
> >  	int  a, b;
> >  
> > -	if (cont) {
> > -		if (regr != 6)
> > -			return 0xff;
> > -		regr = 7;
> > -	}
> > +	regr += cont_map[cont];
> 
>    Likewise here?
> 
> [...]
> 
> MBR, Sergey
> 


-- 
Ondrej Zary

Re: [PATCH 1/4] pata_parport: fix pata_parport_devchk

[Sergei Shtylyov]

On 10/3/23 8:07 PM, Ondrej Zary wrote:
[...]

>>> There's a 'x' missing in 0x55 in pata_parport_devchk(), causing the
>>> detection to always fail. Fix it.
>>>
>>> Signed-off-by: Ondrej Zary <linux@zary.sk>
>>
>>    I think we need a Fixes: tag here...
>>
>>> ---
>>>  drivers/ata/pata_parport/pata_parport.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
>>> index 1af64d435d3c..258d189f42e5 100644
>>> --- a/drivers/ata/pata_parport/pata_parport.c
>>> +++ b/drivers/ata/pata_parport/pata_parport.c
>>> @@ -64,7 +64,7 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
>>>  	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0xaa);
>>>  	pi->proto->write_regr(pi, 0, ATA_REG_LBAL, 0x55);
>>>  
>>> -	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 055);
>>> +	pi->proto->write_regr(pi, 0, ATA_REG_NSECT, 0x55);
>>
>>    Oh, Gawd! How did this ever work?! :-/
>>    This bug seems to predate the Big PARIDE move...
> 
> This code was not present in PARIDE - it's my bug.

   Yes, I finally figured -- hence the Fixes: tag I suggested later....

> The function is a clone of ata_devchk() without direct port access.

   The libata's taskfile methods suck big time -- I even used to have
the plans to clean this stuff up at some point...

> It's called only from softreset so nobody notices the breakage until something goes wrong. The CD-865 drive needs a reset to start working.

   I thought the SRST reset is used at the initial detection phase as well...

MBR, Sergey

Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset

[Ondrej Zary]

On Monday 02 October 2023 22:48:59 Sergey Shtylyov wrote:
> On 9/30/23 10:15 PM, Ondrej Zary wrote:
> 
> > Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
> > bogus values when there's no master device present. This can cause
> > reset to fail, preventing the lone slave device (such as EXP Computer
> > CD-865) from working.
> > 
> > Add custom version of wait_after_reset that ignores master failure when
> > a slave device is present. The custom version is also needed because
> > the generic ata_sff_wait_after_reset uses direct port I/O for slave
> > device detection.
> > 
> > Signed-off-by: Ondrej Zary <linux@zary.sk>
> > ---
> >  drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
> >  1 file changed, 64 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> > index cf87bbb52f1f..b3db953e615a 100644
> > --- a/drivers/ata/pata_parport/pata_parport.c
> > +++ b/drivers/ata/pata_parport/pata_parport.c
> > @@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
> >  	return (nsect == 0x55) && (lbal == 0xaa);
> >  }
> >  
> > +static int pata_parport_wait_after_reset(struct ata_link *link,
> > +					 unsigned int devmask,
> > +					 unsigned long deadline)
> > +{
> > +	struct ata_port *ap = link->ap;
> > +	struct pi_adapter *pi = ap->host->private_data;
> > +	unsigned int dev0 = devmask & (1 << 0);
> > +	unsigned int dev1 = devmask & (1 << 1);
> > +	int rc, ret = 0;
> > +
> > +	ata_msleep(ap, ATA_WAIT_AFTER_RESET);
> > +
> > +	/* always check readiness of the master device */
> > +	rc = ata_sff_wait_ready(link, deadline);
> > +	/* some adapters return bogus values if master device is not present,
> 
>    The multiline comments should start with /* on its own line.
>    Have you run scripts/checkpatch.pl on the patches?

Checkpatch doesn't complain.

> > +	 * so don't abort now if a slave device is present
> > +	 */
> > +	if (rc) {
> > +		if (!dev1)
> > +			return rc;
> > +		ret = -ENODEV;
> > +	}
> > +
> > +	/* if device 1 was found in ata_devchk, wait for register
> 
>    Likewise here...
> 
> > +	 * access briefly, then wait for BSY to clear.
> > +	 */
> > +	if (dev1) {
> > +		int i;
> > +
> > +		pata_parport_dev_select(ap, 1);
> > +
> > +		/* Wait for register access.  Some ATAPI devices fail
> 
>    And here...
> 
> [...]
> 
> MBR, Sergey
> 


-- 
Ondrej Zary

Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset

[Damien Le Moal]

On 10/4/23 01:55, Sergey Shtylyov wrote:
> On 10/3/23 3:55 AM, Damien Le Moal wrote:
> 
> [...]
>>> Some parallel adapters (e.g. EXP Computer MC-1285B EPP Cable) return
>>> bogus values when there's no master device present. This can cause
>>> reset to fail, preventing the lone slave device (such as EXP Computer
>>> CD-865) from working.
>>>
>>> Add custom version of wait_after_reset that ignores master failure when
>>> a slave device is present. The custom version is also needed because
>>> the generic ata_sff_wait_after_reset uses direct port I/O for slave
>>> device detection.
>>>
>>> Signed-off-by: Ondrej Zary <linux@zary.sk>
>>> ---
>>>  drivers/ata/pata_parport/pata_parport.c | 65 ++++++++++++++++++++++++-
>>>  1 file changed, 64 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
>>> index cf87bbb52f1f..b3db953e615a 100644
>>> --- a/drivers/ata/pata_parport/pata_parport.c
>>> +++ b/drivers/ata/pata_parport/pata_parport.c
>>> @@ -80,6 +80,69 @@ static bool pata_parport_devchk(struct ata_port *ap, unsigned int device)
>>>  	return (nsect == 0x55) && (lbal == 0xaa);
>>>  }
>>>  
>>> +static int pata_parport_wait_after_reset(struct ata_link *link,
>>> +					 unsigned int devmask,
>>> +					 unsigned long deadline)
>>> +{
>>> +	struct ata_port *ap = link->ap;
>>> +	struct pi_adapter *pi = ap->host->private_data;
>>> +	unsigned int dev0 = devmask & (1 << 0);
>>> +	unsigned int dev1 = devmask & (1 << 1);
>>> +	int rc, ret = 0;
>>> +
>>> +	ata_msleep(ap, ATA_WAIT_AFTER_RESET);
>>> +
>>> +	/* always check readiness of the master device */
>>> +	rc = ata_sff_wait_ready(link, deadline);
>>> +	/* some adapters return bogus values if master device is not present,
>>> +	 * so don't abort now if a slave device is present
>>> +	 */
>>
>> In addition to Sergey's comment, please move this comment inside the "if", or
>> even better, merge it with the otherwise not very useful "always check
>> readiness..." comment.
> 
>    That comment was copied from ata_sff_wait_after_reset(), I think...

Even if that is the case. let's not copy bad stuff as is but rather improve it.


-- 
Damien Le Moal
Western Digital Research