Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
From:   Ondrej Zary <linux@zary.sk>
To:     Sergey Shtylyov <s.shtylyov@omp.ru>
Subject: Re: [PATCH 3/4] pata_parport: add custom version of wait_after_reset
Date:   Tue, 3 Oct 2023 19:20:29 +0200
User-Agent: KMail/1.9.10
Cc:     Damien Le Moal <damien.lemoal@opensource.wdc.com>,
        Sudip Mukherjee <sudipm.mukherjee@gmail.com>,
        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> <a164fc88-ef57-f957-25df-e149540830c0@omp.ru>
In-Reply-To: <a164fc88-ef57-f957-25df-e149540830c0@omp.ru>
MIME-Version: 1.0
Content-Type: Text/Plain;
  charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Message-Id: <202310031920.29288.linux@zary.sk>
Precedence: bulk

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