RE: megaraid_sas: add an i/o barrier

[Kashyap Desai <kashyap.desai@avagotech.com>]

> -----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