megaraid_sas: add an i/o barrier

megaraid_sas: add an i/o barrier

[Tomas Henzl]

A barrier should be added to ensure proper
ordering of memory mapped writes.

Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index d9d0029fb1..98a848bdfd 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -204,6 +204,7 @@ megasas_fire_cmd_fusion(struct megasas_instance *instance,
 		&instance->reg_set->inbound_low_queue_port);
 	writel(le32_to_cpu(req_desc->u.high),
 		&instance->reg_set->inbound_high_queue_port);
+	mmiowb();
 	spin_unlock_irqrestore(&instance->hba_lock, flags);
 #endif
 }
-- 
2.4.3

RE: megaraid_sas: add an i/o barrier

[Kashyap Desai]

> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Friday, January 29, 2016 11:05 PM
> To: 'linux-scsi@vger.kernel.org'
> Cc: Sumit.Saxena@avagotech.com; Desai, Kashyap; Uday Lingala
> Subject: megaraid_sas: add an i/o barrier
>
> A barrier should be added to ensure proper ordering of memory mapped
> writes.
>
> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
> ---
>  drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index d9d0029fb1..98a848bdfd 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -204,6 +204,7 @@ megasas_fire_cmd_fusion(struct
> megasas_instance *instance,
>  		&instance->reg_set->inbound_low_queue_port);
>  	writel(le32_to_cpu(req_desc->u.high),
>  		&instance->reg_set->inbound_high_queue_port);
> +	mmiowb();
>  	spin_unlock_irqrestore(&instance->hba_lock, flags);  #endif  }

Tomas-

We may need similar changes around below Functions as well, because there is
no associated readX or mmiowb() call.
megasas_fire_cmd_xscale()
megasas_fire_cmd_ppc()
megasas_fire_cmd_skinny()
megasas_fire_cmd_gen2()

Also,  wrireq() routine in same function megasas_fire_cmd_fusion() need i/o
barrier.

> --
> 2.4.3

Re: megaraid_sas: add an i/o barrier

[Tomas Henzl]

On 1.2.2016 05:45, Kashyap Desai wrote:
>> -----Original Message-----
>> From: Tomas Henzl [mailto:thenzl@redhat.com]
>> Sent: Friday, January 29, 2016 11:05 PM
>> To: 'linux-scsi@vger.kernel.org'
>> Cc: Sumit.Saxena@avagotech.com; Desai, Kashyap; Uday Lingala
>> Subject: megaraid_sas: add an i/o barrier
>>
>> A barrier should be added to ensure proper ordering of memory mapped
>> writes.
>>
>> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
>> ---
>>  drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> index d9d0029fb1..98a848bdfd 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> @@ -204,6 +204,7 @@ megasas_fire_cmd_fusion(struct
>> megasas_instance *instance,
>>  		&instance->reg_set->inbound_low_queue_port);
>>  	writel(le32_to_cpu(req_desc->u.high),
>>  		&instance->reg_set->inbound_high_queue_port);
>> +	mmiowb();
>>  	spin_unlock_irqrestore(&instance->hba_lock, flags);  #endif  }
> Tomas-
>
> We may need similar changes around below Functions as well, because there is
> no associated readX or mmiowb() call.
> megasas_fire_cmd_xscale()
> megasas_fire_cmd_ppc()
> megasas_fire_cmd_skinny()
> megasas_fire_cmd_gen2()
>
> Also,  wrireq() routine in same function megasas_fire_cmd_fusion() need i/o
> barrier.

I don't think so (with the exception of megasas_fire_cmd_skinny - I missed this one).
When two threads try to use a fire_cmd there is no protection of certain ordering,
that had to be done in a caller of fire_cmd (for example in megasas_build_and_issue_cmd_fusion)
and it seems to me that there is nothing like that. Likely is, that this - a strict ordering 
of commands - is not needed.
The protection which I'm adding is needed when a command consist of a sequence of more
than one write, see memory-barriers.txt.

(It looks to me that 
megasas_fire_cmd_ -xscale -ppc -skiny -gen2 do not need the hba_lock unless there is another
i/o sequence protected with the same lock (note also that there is
no such lock in megasas_fire_cmd_fusion).)


I'll add the mmiowb to megasas_fire_cmd_skinny and send a new patch - agreed?

--tms

>
>> --
>> 2.4.3
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: megaraid_sas: add an i/o barrier

[Kashyap Desai]

> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Monday, February 01, 2016 6:23 PM
> To: Kashyap Desai; linux-scsi@vger.kernel.org
> Cc: Sumit Saxena; Uday Lingala
> Subject: Re: megaraid_sas: add an i/o barrier
>
> On 1.2.2016 05:45, Kashyap Desai wrote:
> >> -----Original Message-----
> >> From: Tomas Henzl [mailto:thenzl@redhat.com]
> >> Sent: Friday, January 29, 2016 11:05 PM
> >> To: 'linux-scsi@vger.kernel.org'
> >> Cc: Sumit.Saxena@avagotech.com; Desai, Kashyap; Uday Lingala
> >> Subject: megaraid_sas: add an i/o barrier
> >>
> >> A barrier should be added to ensure proper ordering of memory mapped
> >> writes.
> >>
> >> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
> >> ---
> >>  drivers/scsi/megaraid/megaraid_sas_fusion.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> >> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> >> index d9d0029fb1..98a848bdfd 100644
> >> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> >> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> >> @@ -204,6 +204,7 @@ megasas_fire_cmd_fusion(struct
> megasas_instance
> >> *instance,
> >>  		&instance->reg_set->inbound_low_queue_port);
> >>  	writel(le32_to_cpu(req_desc->u.high),
> >>  		&instance->reg_set->inbound_high_queue_port);
> >> +	mmiowb();
> >>  	spin_unlock_irqrestore(&instance->hba_lock, flags);  #endif  }
> > Tomas-
> >
> > We may need similar changes around below Functions as well, because
> > there is no associated readX or mmiowb() call.
> > megasas_fire_cmd_xscale()
> > megasas_fire_cmd_ppc()
> > megasas_fire_cmd_skinny()
> > megasas_fire_cmd_gen2()
> >
> > Also,  wrireq() routine in same function megasas_fire_cmd_fusion()
> > need i/o barrier.
>
> I don't think so (with the exception of megasas_fire_cmd_skinny - I missed
> this one).
> When two threads try to use a fire_cmd there is no protection of certain
> ordering, that had to be done in a caller of fire_cmd (for example in
> megasas_build_and_issue_cmd_fusion)
> and it seems to me that there is nothing like that. Likely is, that this -
> a
> strict ordering of commands - is not needed.
> The protection which I'm adding is needed when a command consist of a
> sequence of more than one write, see memory-barriers.txt.
>
> (It looks to me that
> megasas_fire_cmd_ -xscale -ppc -skiny -gen2 do not need the hba_lock
> unless there is another i/o sequence protected with the same lock (note
> also that there is no such lock in megasas_fire_cmd_fusion).)
Agree, we don't need hba lock in older fire_cmd wrapper. We updated fusion
code because it was active shipment and product life cycle was active for
fusion adapter, so code changes was tested by Avago. We did not clean code
in MFI based controller since it is only in critical support cycle.

>
>
> I'll add the mmiowb to megasas_fire_cmd_skinny and send a new patch -
> agreed?

Got your word. You are right.  Yes additional changes in
megasas_fire_cmd_skinny along with current patch will be good to go.

>
> --tms
>
> >
> >> --
> >> 2.4.3
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html