Re: [PATCH v3] libata: support the ata host which implements a queue depth less than 32

[Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>]

On Friday, July 11, 2014 06:28:33 PM Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Friday, July 11, 2014 02:10:26 PM Kevin Hao wrote:
> > The sata on fsl mpc8315e is broken after the commit 8a4aeec8d2d6
> > ("libata/ahci: accommodate tag ordered controllers"). The reason is
> > that the ata controller on this SoC only implement a queue depth of
> > 16. When issuing the commands in tag order, all the commands in tag
> > 16 ~ 31 are mapped to tag 0 unconditionally and then causes the sata
> > malfunction. It makes no senses to use a 32 queue in software while
> > the hardware has less queue depth. So consider the queue depth
> > implemented by the hardware when requesting a command tag.
> > 
> > Fixes: 8a4aeec8d2d6 ("libata/ahci: accommodate tag ordered controllers")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > ---
> > v3: Use ap->scsi_host->can_queue for the queue depth implemented by hardware.
> >     Patch 2 in v2 is also dropped due to this change.
> > 
> > v2: Remove the changes for the ata tag helper functions
> > 
> > Hi Tejun,
> > 
> > I didn't get explicit objection for the codes at http://marc.info/?l=linux-ide&m=140478830920334&w=2
> > So I assume that you are OK wit it. The code in this patch is the same as that,
> > just add the commit log.
> > 
> >  drivers/ata/libata-core.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index 8f3043165048..4792fea79acf 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -4728,14 +4728,17 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
> >  static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
> >  {
> >  	struct ata_queued_cmd *qc = NULL;
> > -	unsigned int i, tag;
> > +	unsigned int i, tag, max_queue;
> > +
> > +	max_queue = ap->scsi_host->can_queue;
> > +	WARN_ON_ONCE(max_queue > ATA_MAX_QUEUE);
> 
> Why not handle this properly by just doing what ata_dev_config_ncq() does and
> using
> 
> 	min(ap->scsi_host->can_queue, ATA_MAX_QUEUE - 1);

actually this should be

	min(ap->scsi_host->can_queue, ATA_MAX_QUEUE);

in this function

> for obtaining max_queue?
>  
> >  	/* no command while frozen */
> >  	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
> >  		return NULL;
> >  
> > -	for (i = 0; i < ATA_MAX_QUEUE; i++) {
> > -		tag = (i + ap->last_tag + 1) % ATA_MAX_QUEUE;
> > +	for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) {
> > +		tag = tag < max_queue ? tag : 0;
> >  
> >  		/* the last tag is reserved for internal command. */
> >  		if (tag == ATA_TAG_INTERNAL)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics