| Message ID | 1532511725-7726-1-git-send-email-sreekanth.reddy@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mpt3sas: Swap I/O memory read value back to cpu endianness | expand |
On Wed, Jul 25, 2018 at 12:42 PM, Sreekanth Reddy <sreekanth.reddy@broadcom.com> wrote: > Swap the I/O memory read value back to cpu endianness before storing it > in a data structures which are defined in the MPI headers where > u8 components are not defined in the endianness order. > > In this area from day one mpt3sas driver is using le32_to_cpu() & > cpu_to_le32() APIs. But in the patch cf6bf9710c > (mpt3sas: Bug fix for big endian systems) we have removed these APIs > before reading I/O memory which we should haven't done it. So > in this patch I am correcting it by adding these APIs back > before accessing I/O memory. > - __u64 data_out = b; > + __u64 data_out = cpu_to_le64(b); > > spin_lock_irqsave(writeq_lock, flags); > writel((u32)(data_out), addr); Wouldn't be the same as __raw_writel(data_out >> 0, addr); __raw_writel(data_out >> 32, addr + 4); mmiowb(); ? > _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock) > { > - writeq(b, addr); > + writeq(cpu_to_le64(b), addr); Similar here __raw_writeq(b, addr); mmiowb(); > - writel((u32)(request[i]), &ioc->chip->Doorbell); > + writel(cpu_to_le32(request[i]), &ioc->chip->Doorbell); This kind of endianess play (including above) should make sparse unhappy. Did you run it with C=1 CF=-D__CHECK_ENDIAN__ ?
On Wed, Jul 25, 2018 at 8:32 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Jul 25, 2018 at 12:42 PM, Sreekanth Reddy > <sreekanth.reddy@broadcom.com> wrote: >> Swap the I/O memory read value back to cpu endianness before storing it >> in a data structures which are defined in the MPI headers where >> u8 components are not defined in the endianness order. >> >> In this area from day one mpt3sas driver is using le32_to_cpu() & >> cpu_to_le32() APIs. But in the patch cf6bf9710c >> (mpt3sas: Bug fix for big endian systems) we have removed these APIs >> before reading I/O memory which we should haven't done it. So >> in this patch I am correcting it by adding these APIs back >> before accessing I/O memory. > >> - __u64 data_out = b; >> + __u64 data_out = cpu_to_le64(b); >> >> spin_lock_irqsave(writeq_lock, flags); >> writel((u32)(data_out), addr); > > Wouldn't be the same as > > __raw_writel(data_out >> 0, addr); > __raw_writel(data_out >> 32, addr + 4); > mmiowb(); > > ? Yes, I can replace the writel() APIs with __raw_writel() with mmiowb() memory barrier. Hoping this doesn't create any other side effects. I will post new patch with this change tomorrow after doing some testing in this area. > >> _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock) >> { >> - writeq(b, addr); >> + writeq(cpu_to_le64(b), addr); > > Similar here > __raw_writeq(b, addr); > mmiowb(); > > >> - writel((u32)(request[i]), &ioc->chip->Doorbell); >> + writel(cpu_to_le32(request[i]), &ioc->chip->Doorbell); > > This kind of endianess play (including above) should make sparse unhappy. > > Did you run it with > > C=1 CF=-D__CHECK_ENDIAN__ > > ? Yes I run it on 4.18 kernel and I don't see any error or warning for these lines. Thanks, Sreekanth > > -- > With Best Regards, > Andy Shevchenko
On Thu, Jul 26, 2018 at 2:25 PM, Sreekanth Reddy <sreekanth.reddy@broadcom.com> wrote: > On Wed, Jul 25, 2018 at 8:32 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Wed, Jul 25, 2018 at 12:42 PM, Sreekanth Reddy >> <sreekanth.reddy@broadcom.com> wrote: >>> Swap the I/O memory read value back to cpu endianness before storing it >>> in a data structures which are defined in the MPI headers where >>> u8 components are not defined in the endianness order. >>> >>> In this area from day one mpt3sas driver is using le32_to_cpu() & >>> cpu_to_le32() APIs. But in the patch cf6bf9710c >>> (mpt3sas: Bug fix for big endian systems) we have removed these APIs >>> before reading I/O memory which we should haven't done it. So >>> in this patch I am correcting it by adding these APIs back >>> before accessing I/O memory. >> >>> - __u64 data_out = b; >>> + __u64 data_out = cpu_to_le64(b); >>> >>> spin_lock_irqsave(writeq_lock, flags); >>> writel((u32)(data_out), addr); >> >> Wouldn't be the same as >> >> __raw_writel(data_out >> 0, addr); >> __raw_writel(data_out >> 32, addr + 4); >> mmiowb(); >> >> ? > > Yes, I can replace the writel() APIs with __raw_writel() with mmiowb() > memory barrier. Hoping this doesn't create any other side effects. > > I will post new patch with this change tomorrow after doing some > testing in this area. Thanks! > >> >>> _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock) >>> { >>> - writeq(b, addr); >>> + writeq(cpu_to_le64(b), addr); >> >> Similar here >> __raw_writeq(b, addr); >> mmiowb(); >> >> >>> - writel((u32)(request[i]), &ioc->chip->Doorbell); >>> + writel(cpu_to_le32(request[i]), &ioc->chip->Doorbell); >> >> This kind of endianess play (including above) should make sparse unhappy. >> >> Did you run it with >> >> C=1 CF=-D__CHECK_ENDIAN__ >> >> ? > > Yes I run it on 4.18 kernel and I don't see any error or warning for > these lines. If you try on x86 I'm pretty sure you get some warnings, especially taken into consideration [1]. [1]: commit 6469a0ee0a06b2ea1f5afbb1d5a3feed017d4c7a (tip/x86-asm-for-linus) Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: Tue May 15 14:52:11 2018 +0300 x86/io: Define readq()/writeq() to use 64-bit type
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c index 94359d8..c7738d1 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_base.c +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c @@ -3350,7 +3350,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, spinlock_t *writeq_lock) { unsigned long flags; - __u64 data_out = b; + __u64 data_out = cpu_to_le64(b); spin_lock_irqsave(writeq_lock, flags); writel((u32)(data_out), addr); @@ -3374,7 +3374,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, static inline void _base_writeq(__u64 b, volatile void __iomem *addr, spinlock_t *writeq_lock) { - writeq(b, addr); + writeq(cpu_to_le64(b), addr); } #else static inline void @@ -5275,7 +5275,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, /* send message 32-bits at a time */ for (i = 0, failed = 0; i < request_bytes/4 && !failed; i++) { - writel((u32)(request[i]), &ioc->chip->Doorbell); + writel(cpu_to_le32(request[i]), &ioc->chip->Doorbell); if ((_base_wait_for_doorbell_ack(ioc, 5))) failed = 1; } @@ -5296,7 +5296,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, } /* read the first two 16-bits, it gives the total length of the reply */ - reply[0] = (u16)(readl(&ioc->chip->Doorbell) + reply[0] = le16_to_cpu(readl(&ioc->chip->Doorbell) & MPI2_DOORBELL_DATA_MASK); writel(0, &ioc->chip->HostInterruptStatus); if ((_base_wait_for_doorbell_int(ioc, 5))) { @@ -5305,7 +5305,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, ioc->name, __LINE__); return -EFAULT; } - reply[1] = (u16)(readl(&ioc->chip->Doorbell) + reply[1] = le16_to_cpu(readl(&ioc->chip->Doorbell) & MPI2_DOORBELL_DATA_MASK); writel(0, &ioc->chip->HostInterruptStatus); @@ -5319,7 +5319,7 @@ void mpt3sas_base_clear_st(struct MPT3SAS_ADAPTER *ioc, if (i >= reply_bytes/2) /* overflow case */ readl(&ioc->chip->Doorbell); else - reply[i] = (u16)(readl(&ioc->chip->Doorbell) + reply[i] = le16_to_cpu(readl(&ioc->chip->Doorbell) & MPI2_DOORBELL_DATA_MASK); writel(0, &ioc->chip->HostInterruptStatus); }
Swap the I/O memory read value back to cpu endianness before storing it in a data structures which are defined in the MPI headers where u8 components are not defined in the endianness order. In this area from day one mpt3sas driver is using le32_to_cpu() & cpu_to_le32() APIs. But in the patch cf6bf9710c (mpt3sas: Bug fix for big endian systems) we have removed these APIs before reading I/O memory which we should haven't done it. So in this patch I am correcting it by adding these APIs back before accessing I/O memory. Signed-off-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com> --- drivers/scsi/mpt3sas/mpt3sas_base.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)