Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Subject: Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset
To:     Damien Le Moal <dlemoal@kernel.org>, Ondrej Zary <linux@zary.sk>,
        Sudip Mukherjee <sudipm.mukherjee@gmail.com>
CC:     Christoph Hellwig <hch@lst.de>, Tim Waugh <tim@cyberelk.net>,
        <linux-parport@lists.infradead.org>, <linux-ide@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20230930191511.24994-1-linux@zary.sk>
 <20230930191511.24994-4-linux@zary.sk>
 <d2fd9a3c-3efd-dbfe-7b2a-dc36989b8379@kernel.org>
From:   Sergey Shtylyov <s.shtylyov@omp.ru>
Organization: Open Mobile Platform
Message-ID: <14129ea5-1597-efce-e782-23a95f3c0bd9@omp.ru>
Date:   Tue, 3 Oct 2023 19:55:07 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <d2fd9a3c-3efd-dbfe-7b2a-dc36989b8379@kernel.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-KSE-Attachment-Filter-Triggered-Filters: Clean
X-KSE-BulkMessagesFiltering-Scan-Result: InTheLimit
X-Spam-Status: No, score=0.3 required=5.0 tests=BAYES_00,NICE_REPLY_A,
        RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Tue, 03 Oct 2023 09:55:34 -0700 (PDT)

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