linux-kernel - Re: [PATCH v1 08/23] ata: sata_dwc_460ex: don't call ata_sff_qc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1873082.m6E4AZmIX5@debian64>
Date:	Fri, 22 Apr 2016 13:44:15 +0200
From:	Christian Lamparter <chunkeey@...glemail.com>
To:	David Laight <David.Laight@...lab.com>
Cc:	'Andy Shevchenko' <andriy.shevchenko@...ux.intel.com>,
	Tejun Heo <tj@...nel.org>,
	"linux-ide@...r.kernel.org" <linux-ide@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Måns Rullgård <mans@...sr.com>,
	Julian Margetson <runaway@...dw.ms>
Subject: Re: [PATCH v1 08/23] ata: sata_dwc_460ex: don't call ata_sff_qc_issue() on DMA commands

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@...glemail.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > ---
> >  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>