Re: [PATCH v1 08/23] ata: sata_dwc_460ex: don't call ata_sff_qc_issue() on DMA commands

[Christian Lamparter]

On Friday, April 22, 2016 11:32:00 AM David Laight wrote:
> From: Andy Shevchenko
> > Sent: 21 April 2016 19:15
> > ata_sff_qc_issue() can't handle DMA commands and thus we have to avoid it for
> > them. Do call ata_bmdma_qc_issue() instead for this case.
> > 
> > Suggested-by: Christian Lamparter <chunkeey@xxxxxxxxxxxxxx>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/ata/sata_dwc_460ex.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> > index 038e5fb..845c35d 100644
> > --- a/drivers/ata/sata_dwc_460ex.c
> > +++ b/drivers/ata/sata_dwc_460ex.c
> > @@ -1061,10 +1061,12 @@ static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
> >  			__func__, tag, qc->ap->link.sactive, sactive);
> > 
> >  		ap->ops->sff_tf_load(ap, &qc->tf);
> > -		sata_dwc_exec_command_by_tag(ap, &qc->tf, qc->tag,
> > +		sata_dwc_exec_command_by_tag(ap, &qc->tf, tag,
> >  					     SATA_DWC_CMD_ISSUED_PEND);
> > +	} else if (ata_is_dma(qc->tf.protocol)) {
> > +		return ata_bmdma_qc_issue(qc);
> >  	} else {
> > -		ata_sff_qc_issue(qc);
> > +		return ata_sff_qc_issue(qc);
> >  	}
> >  	return 0;
> >  }
> 
> I'd nuke those 'else if', they make it very hard to read.
> I Think the code is:
> 
> 		sata_dwc_exec_command_by_tag(...);
> 		return 0;
> 	}
> 
> 	if (ata_is_dma(qc->tf.protocol))
> 		return ata_bmdma_qc_issue(qc);
> 
> 	return ata_sff_qc_issue(qc);
> }

What about something like this instead? ata_bmdma_qc_issue already calls
ata_sff_qc_issue, if it's not a dma transfere anyway [0].
---
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 6a61184..67cce54 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1096,12 +1096,9 @@ static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
 		ap->ops->sff_tf_load(ap, &qc->tf);
 		sata_dwc_exec_command_by_tag(ap, &qc->tf, tag,
 					     SATA_DWC_CMD_ISSUED_PEND);
-	} else if (ata_is_dma(qc->tf.protocol)) {
+		return 0;
+	} else
 		return ata_bmdma_qc_issue(qc);
-	} else {
-		return ata_sff_qc_issue(qc);
-	}
-	return 0;
 }
 
 static void sata_dwc_error_handler(struct ata_port *ap)
---

[0] <http://lxr.free-electrons.com/source/drivers/ata/libata-sff.c#L2787>