Re: [PATCH] I/O space write barrier

Re: [PATCH] I/O space write barrier

[Jeremy Higdon]

On Thu, Sep 30, 2004 at 11:21:39PM +0200, Guennadi Liakhovetski wrote:
> 
> A pretty obvious note: instead of repeating this nice but pretty lengthy 
> comment 3 times in the same file, wouldn't it be better to write at 
> further locations something like
> 
> 	/* Enforce IO-ordering. See comment in <function> for details. */
> 
> Also helps if you later have to modify the comment, or move it, or add 
> more mmiowb()s, or do some other modifications.
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski

Suggestion applied.  Now that the mmiowb is in the mm patch, this
patch to qla1280 should be ready for inclusion therein also.

signed-off-by: Jeremy Higdon <jeremy@sgi.com>


--- linux-2.6.9-rc4.orig/drivers/scsi/qla1280.c	2004-10-15 17:21:21.000000000 -0700
+++ linux-2.6.9-rc4/drivers/scsi/qla1280.c	2004-10-15 17:23:08.000000000 -0700
@@ -3409,7 +3409,8 @@
 	sp->flags |= SRB_SENT;
 	ha->actthreads++;
 	WRT_REG_WORD(&reg->mailbox4, ha->req_ring_index);
-	(void) RD_REG_WORD(&reg->mailbox4); /* PCI posted write flush */
+	/* Enforce mmio write ordering; see comment in qla1280_isp_cmd(). */
+	mmiowb();
 
  out:
 	if (status)
@@ -3677,7 +3678,8 @@
 	sp->flags |= SRB_SENT;
 	ha->actthreads++;
 	WRT_REG_WORD(&reg->mailbox4, ha->req_ring_index);
-	(void) RD_REG_WORD(&reg->mailbox4); /* PCI posted write flush */
+	/* Enforce mmio write ordering; see comment in qla1280_isp_cmd(). */
+	mmiowb();
 
 out:
 	if (status)
@@ -3787,9 +3789,21 @@
 	} else
 		ha->request_ring_ptr++;
 
-	/* Set chip new ring index. */
+	/*
+	 * Update request index to mailbox4 (Request Queue In).
+	 * The mmiowb() ensures that this write is ordered with writes by other
+	 * CPUs.  Without the mmiowb(), it is possible for the following:
+	 *    CPUA posts write of index 5 to mailbox4
+	 *    CPUA releases host lock
+	 *    CPUB acquires host lock
+	 *    CPUB posts write of index 6 to mailbox4
+	 *    On PCI bus, order reverses and write of 6 posts, then index 5,
+	 *       causing chip to issue full queue of stale commands
+	 * The mmiowb() prevents future writes from crossing the barrier.
+	 * See Documentation/DocBook/deviceiobook.tmpl for more information.
+	 */
 	WRT_REG_WORD(&reg->mailbox4, ha->req_ring_index);
-	(void) RD_REG_WORD(&reg->mailbox4); /* PCI posted write flush */
+	mmiowb();
 
 	LEAVE("qla1280_isp_cmd");
 }

Re: [PATCH] I/O space write barrier

[Matthew Wilcox]

On Fri, Oct 15, 2004 at 05:38:09PM -0700, Jeremy Higdon wrote:
> -	(void) RD_REG_WORD(&reg->mailbox4); /* PCI posted write flush */
> +	/* Enforce mmio write ordering; see comment in qla1280_isp_cmd(). */
> +	mmiowb();

I really don't think we want a comment by every mmiowb() explaining what
it does.  We needed one by the write flush because it had two potential
meanings, and we didn't want people overoptimising it away.  But mmiowb()
is clear and unambiguous.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] I/O space write barrier

[Jeremy Higdon]

On Sat, Oct 16, 2004 at 04:20:44AM +0100, Matthew Wilcox wrote:
> On Fri, Oct 15, 2004 at 05:38:09PM -0700, Jeremy Higdon wrote:
> > -	(void) RD_REG_WORD(&reg->mailbox4); /* PCI posted write flush */
> > +	/* Enforce mmio write ordering; see comment in qla1280_isp_cmd(). */
> > +	mmiowb();
> 
> I really don't think we want a comment by every mmiowb() explaining what
> it does.  We needed one by the write flush because it had two potential
> meanings, and we didn't want people overoptimising it away.  But mmiowb()
> is clear and unambiguous.

This seems to be a case of not being able to please everyone.
James Bottomley asked for documentation on the usage of mmiowb().
Guennadi asked for one copy of the documentation and the references
to that in other places.

I don't really care that much, but this is the third version of this
patch, where the only difference is comments.  If it's all right, let
this go in, and you can submit patches to change the comments later.

I believe James' idea was that qla1280 would be the "example" driver.

jeremy