| Message ID | 20180524151246.2429-1-thenzl@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, 2018-05-24 at 17:12 +0200, Tomas Henzl wrote: > A barrier should be added to ensure proper ordering of memory mapped > writes. > > Signed-off-by: Tomas Henzl <thenzl@redhat.com> > --- > drivers/scsi/mpt3sas/mpt3sas_base.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c > b/drivers/scsi/mpt3sas/mpt3sas_base.c > index bf04fa90f..569392d0d 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -3348,6 +3348,7 @@ _base_mpi_ep_writeq(__u64 b, volatile void > __iomem *addr, > spin_lock_irqsave(writeq_lock, flags); > writel((u32)(data_out), addr); > writel((u32)(data_out >> 32), (addr + 4)); > + mmiowb(); > spin_unlock_irqrestore(writeq_lock, flags); > } I thought, assuming mpt3sas has this right, that this construction is only used on 32 bit platforms that don't have a writeq instruction? I don't believe there's any overlap with the NUMA systems that need io and memory domain synchronization, so either this problem is purely theoretical or mpt3sas doesn't have the use of writeq correct and if the latter case it should be fixed correctly. James
On 05/24/2018 05:19 PM, James Bottomley wrote: > On Thu, 2018-05-24 at 17:12 +0200, Tomas Henzl wrote: >> A barrier should be added to ensure proper ordering of memory mapped >> writes. >> >> Signed-off-by: Tomas Henzl <thenzl@redhat.com> >> --- >> drivers/scsi/mpt3sas/mpt3sas_base.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c >> b/drivers/scsi/mpt3sas/mpt3sas_base.c >> index bf04fa90f..569392d0d 100644 >> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c >> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c >> @@ -3348,6 +3348,7 @@ _base_mpi_ep_writeq(__u64 b, volatile void >> __iomem *addr, >> spin_lock_irqsave(writeq_lock, flags); >> writel((u32)(data_out), addr); >> writel((u32)(data_out >> 32), (addr + 4)); >> + mmiowb(); >> spin_unlock_irqrestore(writeq_lock, flags); >> } > I thought, assuming mpt3sas has this right, that this construction is > only used on 32 bit platforms that don't have a writeq instruction? I > don't believe there's any overlap with the NUMA systems that need io > and memory domain synchronization, so either this problem is purely > theoretical or mpt3sas doesn't have the use of writeq correct and if > the latter case it should be fixed correctly. The _base_mpi_ep_writeq is used regardless to 32/64 bit arch for example in _base_put_smid_mpi_ep_scsi_io, mpt3sas_base_put_smid_hi_priority and so on. > > James >
On Thu, 2018-05-24 at 17:31 +0200, Tomas Henzl wrote: > On 05/24/2018 05:19 PM, James Bottomley wrote: > > On Thu, 2018-05-24 at 17:12 +0200, Tomas Henzl wrote: > > > A barrier should be added to ensure proper ordering of memory > > > mapped > > > writes. > > > > > > Signed-off-by: Tomas Henzl <thenzl@redhat.com> > > > --- > > > drivers/scsi/mpt3sas/mpt3sas_base.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c > > > b/drivers/scsi/mpt3sas/mpt3sas_base.c > > > index bf04fa90f..569392d0d 100644 > > > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > > > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > > > @@ -3348,6 +3348,7 @@ _base_mpi_ep_writeq(__u64 b, volatile void > > > __iomem *addr, > > > spin_lock_irqsave(writeq_lock, flags); > > > writel((u32)(data_out), addr); > > > writel((u32)(data_out >> 32), (addr + 4)); > > > + mmiowb(); > > > spin_unlock_irqrestore(writeq_lock, flags); > > > } > > > > I thought, assuming mpt3sas has this right, that this construction > > is only used on 32 bit platforms that don't have a writeq > > instruction? I don't believe there's any overlap with the NUMA > > systems that need io and memory domain synchronization, so either > > this problem is purely theoretical or mpt3sas doesn't have the use > > of writeq correct and if the latter case it should be fixed > > correctly. > > The _base_mpi_ep_writeq is used regardless to 32/64 bit arch > for example in _base_put_smid_mpi_ep_scsi_io, > mpt3sas_base_put_smid_hi_priority > and so on. So it's the latter ... but my point is that that should be fixed rather than adding barriers to what should be a corner case work around. James
On 05/24/2018 05:33 PM, James Bottomley wrote: > On Thu, 2018-05-24 at 17:31 +0200, Tomas Henzl wrote: >> On 05/24/2018 05:19 PM, James Bottomley wrote: >>> On Thu, 2018-05-24 at 17:12 +0200, Tomas Henzl wrote: >>>> A barrier should be added to ensure proper ordering of memory >>>> mapped >>>> writes. >>>> >>>> Signed-off-by: Tomas Henzl <thenzl@redhat.com> >>>> --- >>>> drivers/scsi/mpt3sas/mpt3sas_base.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c >>>> b/drivers/scsi/mpt3sas/mpt3sas_base.c >>>> index bf04fa90f..569392d0d 100644 >>>> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c >>>> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c >>>> @@ -3348,6 +3348,7 @@ _base_mpi_ep_writeq(__u64 b, volatile void >>>> __iomem *addr, >>>> spin_lock_irqsave(writeq_lock, flags); >>>> writel((u32)(data_out), addr); >>>> writel((u32)(data_out >> 32), (addr + 4)); >>>> + mmiowb(); >>>> spin_unlock_irqrestore(writeq_lock, flags); >>>> } >>> I thought, assuming mpt3sas has this right, that this construction >>> is only used on 32 bit platforms that don't have a writeq >>> instruction? I don't believe there's any overlap with the NUMA >>> systems that need io and memory domain synchronization, so either >>> this problem is purely theoretical or mpt3sas doesn't have the use >>> of writeq correct and if the latter case it should be fixed >>> correctly. >> The _base_mpi_ep_writeq is used regardless to 32/64 bit arch >> for example in _base_put_smid_mpi_ep_scsi_io, >> mpt3sas_base_put_smid_hi_priority >> and so on. > So it's the latter ... but my point is that that should be fixed rather > than adding barriers to what should be a corner case work around. I think that the hw is for some reason not able to handle a 64 write, the patch in e5747439366c1079257083f231f5dd9a84bf0fd7 "scsi: mpt3sas: Introduce function to clone mpi request" states that it is intentional. So adding the write barrier still makes sense for me. > > James >
Hi, Please consider this patch as Acked-by: Chaitra P B <chaitra.basappa@broadcom.com> Thanks, Chaitra -----Original Message----- From: Tomas Henzl [mailto:thenzl@redhat.com] Sent: Thursday, May 24, 2018 9:19 PM To: James Bottomley; linux-scsi@vger.kernel.org Cc: chaitra.basappa@broadcom.com; sreekanth.reddy@broadcom.com; Sathya.Prakash@broadcom.com Subject: Re: [PATCH] mpt3sas: Add an i/o barrier On 05/24/2018 05:33 PM, James Bottomley wrote: > On Thu, 2018-05-24 at 17:31 +0200, Tomas Henzl wrote: >> On 05/24/2018 05:19 PM, James Bottomley wrote: >>> On Thu, 2018-05-24 at 17:12 +0200, Tomas Henzl wrote: >>>> A barrier should be added to ensure proper ordering of memory >>>> mapped writes. >>>> >>>> Signed-off-by: Tomas Henzl <thenzl@redhat.com> >>>> --- >>>> drivers/scsi/mpt3sas/mpt3sas_base.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c >>>> b/drivers/scsi/mpt3sas/mpt3sas_base.c >>>> index bf04fa90f..569392d0d 100644 >>>> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c >>>> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c >>>> @@ -3348,6 +3348,7 @@ _base_mpi_ep_writeq(__u64 b, volatile void >>>> __iomem *addr, >>>> spin_lock_irqsave(writeq_lock, flags); >>>> writel((u32)(data_out), addr); >>>> writel((u32)(data_out >> 32), (addr + 4)); >>>> + mmiowb(); >>>> spin_unlock_irqrestore(writeq_lock, flags); >>>> } >>> I thought, assuming mpt3sas has this right, that this construction >>> is only used on 32 bit platforms that don't have a writeq >>> instruction? I don't believe there's any overlap with the NUMA >>> systems that need io and memory domain synchronization, so either >>> this problem is purely theoretical or mpt3sas doesn't have the use >>> of writeq correct and if the latter case it should be fixed >>> correctly. >> The _base_mpi_ep_writeq is used regardless to 32/64 bit arch for >> example in _base_put_smid_mpi_ep_scsi_io, >> mpt3sas_base_put_smid_hi_priority and so on. > So it's the latter ... but my point is that that should be fixed > rather than adding barriers to what should be a corner case work around. I think that the hw is for some reason not able to handle a 64 write, the patch in e5747439366c1079257083f231f5dd9a84bf0fd7 "scsi: mpt3sas: Introduce function to clone mpi request" states that it is intentional. So adding the write barrier still makes sense for me. > > James >
Tomas, > A barrier should be added to ensure proper ordering of memory mapped > writes. Applied to 4.18/scsi-fixes. Thanks!
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c index bf04fa90f..569392d0d 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_base.c +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c @@ -3348,6 +3348,7 @@ _base_mpi_ep_writeq(__u64 b, volatile void __iomem *addr, spin_lock_irqsave(writeq_lock, flags); writel((u32)(data_out), addr); writel((u32)(data_out >> 32), (addr + 4)); + mmiowb(); spin_unlock_irqrestore(writeq_lock, flags); }
A barrier should be added to ensure proper ordering of memory mapped writes. Signed-off-by: Tomas Henzl <thenzl@redhat.com> --- drivers/scsi/mpt3sas/mpt3sas_base.c | 1 + 1 file changed, 1 insertion(+)