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