On 12/09/2016 01:20 PM, Benjamin Herrenschmidt wrote: > On Wed, 2016-12-07 at 17:31 -0600, Tyrel Datwyler wrote: >> The first byte of each CRQ entry is used to indicate whether an entry is >> a valid response or free for the VIOS to use. After processing a >> response the driver sets the valid byte to zero to indicate the entry is >> now free to be reused. Add a memory barrier after this write to ensure >> no other stores are reordered when updating the valid byte. > > Which "other stores" specifically ? This smells fishy without that > precision. It's important to always understand what exactly barriers > order with.
So, this patch initially came about while chasing a data integrity issue based on the observation that we were already doing this same write barrier in the virtual fibre channel driver. However, the more I stare at it I agree it does seem fishy and I can't see any other stores that we need to order with here. In terms of the 16byte CRQ entries we only ever write to the first byte as a sort of doorbell to tell the VIOS that we have processed the data and the entry is free to be re-used. The remainder of the CRQ is only ever read from. The wmb() in the ibmvfc driver was part of a commit from Brian that also involved a necessary rmb() that prevented stale data from load re-ordering by ensuring that a read from the first byte completed and contained a valid value prior to doing any other reads of the CRQ entry. I'd have to defer to Brian as to whether he remembers a legitimate reason for the wmb(). -Tyrel > > Cheers, > Ben. > >> Signed-off-by: Tyrel Datwyler <tyr...@linux.vnet.ibm.com> >> --- >> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c >> b/drivers/scsi/ibmvscsi/ibmvscsi.c >> index d9534ee..2f5b07e 100644 >> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c >> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c >> @@ -232,6 +232,7 @@ static void ibmvscsi_task(void *data) >>> while ((crq = crq_queue_next_crq(&hostdata->queue)) != NULL) { >>> ibmvscsi_handle_crq(crq, hostdata); >>> crq->valid = VIOSRP_CRQ_FREE; >>> + wmb(); >>> } >> >>> vio_enable_interrupts(vdev); >> @@ -240,6 +241,7 @@ static void ibmvscsi_task(void *data) >>> vio_disable_interrupts(vdev); >>> ibmvscsi_handle_crq(crq, hostdata); >>> crq->valid = VIOSRP_CRQ_FREE; >>> + wmb(); >>> } else { >>> done = 1; >>> } > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >