Re: [PATCH] mpt3sas: Add an i/o barrier

[Martin K. Petersen]

Tomas,

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

Applied to 4.18/scsi-fixes. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

RE: [PATCH] mpt3sas: Add an i/o barrier

[Chaitra Basappa]

Hi,
Please consider this patch as

Acked-by: Chaitra P B 

Thanks,
 Chaitra

-Original Message-
From: Tomas Henzl [mailto:the...@redhat.com]
Sent: Thursday, May 24, 2018 9:19 PM
To: James Bottomley; linux-scsi@vger.kernel.org
Cc: chaitra.basa...@broadcom.com; sreekanth.re...@broadcom.com;
sathya.prak...@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 
>>>> ---
>>>>  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
>

Re: [PATCH] mpt3sas: Add an i/o barrier

[Tomas Henzl]

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

Re: [PATCH] mpt3sas: Add an i/o barrier

[James Bottomley]

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

Re: [PATCH] mpt3sas: Add an i/o barrier

[Tomas Henzl]

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

Re: [PATCH] mpt3sas: Add an i/o barrier

[James Bottomley]

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

[PATCH] mpt3sas: 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 
---
 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);
 }
 
-- 
2.14.3