Re: [PATCH v1] scsi: mpt3sas: Use lo_hi_writeq() helper

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, 2018-02-14 at 11:40 -0800, James Bottomley wrote:
> On Wed, 2018-02-14 at 20:10 +0200, Andy Shevchenko wrote:
> > Since we have a writeq() for 32-bit architectures as provided by IO
> > non-atomic helpers, there is no need to open code it.
> > 
> > Moreover sparse complains about this:
> > 
> > drivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: expected unsigned long
> > long val
> > drivers/scsi/mpt3sas/mpt3sas_base.c:2975:16: got restricted __le64
> > <noident>
> > 
> > Fixing this by replacing custom writeq() with one provided by
> > io-64-nonatomic-lo-hi.h header.
> > 

> > -#if defined(writeq) && defined(CONFIG_64BIT)
> > -static inline void
> > -_base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t
> > *writeq_lock)
> > -{
> > -	writeq(cpu_to_le64(b), addr);
> > -}
> > -#else
> >  static inline void
> >  _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t
> > *writeq_lock)
> >  {
> > @@ -2985,11 +2979,9 @@ _base_writeq(__u64 b, volatile void __iomem
> > *addr, spinlock_t *writeq_lock)
> >  	__u64 data_out = cpu_to_le64(b);
> >  
> >  	spin_lock_irqsave(writeq_lock, flags);
> > -	writel((u32)(data_out), addr);
> > -	writel((u32)(data_out >> 32), (addr + 4));
> > +	writeq(data_out, addr);
> >  	spin_unlock_irqrestore(writeq_lock, flags);
> >  }
> > -#endif
> 
> This would rather defeat the purpose of the original code, I think.

James, TBH, I don't see any value of that lock. What it's protecting
from? simultaneous thread doing writeq()? But this is pointless if at
the same time you will have writel() to the device.

For my opinion perhaps complete removal of the custom writeq() and
replacing it by just writeq() with enabled non-atomic helpers will do
the job.

The code is very old, and I have no idea why it's done this (strange)
way.

>  The coding seems to indicate that if the platform can do a writeq
> atomically (i.e. it's 64 bit and has the primitive) then it should and
> not take the hit of using a lock.  Otherwise the 64 bit value is
> updated by two 32 bit writes protected by a lock to ensure we don't
> get
> interleaving of the write.
> 
> So I think you can replace the double writel with a lo_hi_writeq but
> you have to lose the lock if the platform supports the atomic 64 bit
> write for performance reasons.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy