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@xxxxxxx>
> ---
> 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
