Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1384485 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v1 08/23] ata: sata_dwc_460ex: don't call ata_sff_qc_issue() on DMA commands Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-04-21 20:20 +0200
    RE: [PATCH v1 08/23] ata: sata_dwc_460ex: don't call  ata_sff_qc_issue() on DMA commands David Laight <David.Laight@ACULAB.COM> - 2016-04-22 13:40 +0200
      Re: [PATCH v1 08/23] ata: sata_dwc_460ex: don't call ata_sff_qc_issue() on DMA commands Christian Lamparter <chunkeey@googlemail.com> - 2016-04-22 13:50 +0200

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

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@googlemail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.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;
 }
-- 
2.8.0.rc3

[toc] | [next] | [standalone]

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

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@googlemail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.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);
}

	David

[toc] | [prev] | [next] | [standalone]

#1385099

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@googlemail.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.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>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web