Re: [PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch
Hi All,
Here is the issue which we are observing when driver don't use
le16_to_cpu() in below code snippet on Sparc64 machine when driver is
reading 2 bytes of data which is posted by HBA firmware,
u32 reply1;
reply1 = readl(&ioc->chip->Doorbell);
reply[1] = (reply1 & MPI2_DOORBELL_DATA_MASK);
printk("LSI debug.. 0x%x, 0x%x, 0x%x \n", reply1, reply[1]);
writel(0, &ioc->chip->HostInterruptStatus);
printk("LSI MsgLength :%d\n", default_reply->MsgLength);
When I execute above code I got below output on Sparc64 machine,
LSI debug.. 0x1c000311, 0x311
LSI MsgLength :3
When I execute same code in x86 machine then I got below output,
LSI debug.. 0x1c000311, 0x311
LSI MsgLength :17
Correct message (Here I am referring IOCFacts message) Length is 17
words. But on Sparc64 machine we got message length as 3 words which
is wrong.
Here is data structure of default reply message,
typedef struct _MPI2_DEFAULT_REPLY {
U16 FunctionDependent1; /*0x00 */
U8 MsgLength; /*0x02 */
U8 Function;/*0x03 */
U16 FunctionDependent2; /*0x04 */
U8 FunctionDependent3; /*0x06 */
U8 MsgFlags;/*0x07 */
U8 VP_ID; /*0x08 */
U8 VF_ID; /*0x09 */
U16 Reserved1; /*0x0A */
U16 FunctionDependent5; /*0x0C */
U16 IOCStatus; /*0x0E */
U32 IOCLogInfo; /*0x10 */
} MPI2_DEFAULT_REPLY, *PTR_MPI2_DEFAULT_REPLY,
MPI2DefaultReply_t, *pMPI2DefaultReply_t;
Until host reads correct number of reply words, IOC won't clear
Doorbel Used bit and hence we see below error message while loading
the driver and IOC initialization fails.
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_get_ioc_facts
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_wait_for_iocstate
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: doorbell is in use (line=5241)
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_get_ioc_facts:
handshake failed (r=-14)
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: mpt3sas_base_free_resources
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_make_ioc_ready
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: mpt3sas_base_unmap_resources
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: _base_release_memory_pools
Jun 28 02:21:57 localhost kernel: mpt4sas_cm0: failure at
/home/chaitra/mpt3sas_with_sparse_patch/mpt3sas_scsih.c:10776/_scsih_probe()
Thanks,
Sreekanth
On Sat, Jun 30, 2018 at 12:06 AM, Andy Shevchenko
wrote:
> On Fri, Jun 29, 2018 at 7:06 PM, James Bottomley
> wrote:
>> On Fri, 2018-06-29 at 10:58 -0400, Chaitra P B wrote:
>>> "scsi: mpt3sas: Bug fix for big endian systems"
>>>
>>> Above patch with commit id "cf6bf9710cabba1fe94a4349f4eb8db623c77ebc"
>>> was posted to fix sparse warnings. While posting this patch it was
>>> assumed that readl() & writel() APIs internally calls le32_to_cpu() &
>>> cpu_to_le32() APIs respectively. Looks like it is not true for all
>>> architecture
>>
>> Just a minute, it damn well should be. The definition of readl/writel
>> is barriers and little endian (you can see this in asm-generic/io.h).
>>
>> Which architecture is getting this wrong? Because it sounds like
>> that's what we need to fix rather than doing something like this in all
>> drivers.
>>
>> Sparc (and parisc) definitely do the little endian thing, so if this
>> code is what it takes to get them working again, it looks like you're
>> double swapping somewhere. I really think cf6bf9710c needs to be
>> reverted and you should look again at your sparse warnings. Since the
>> driver is using the non-raw readX/writeX it should be cpu endian
>> internally which cf6bf9710c upsets.
>
> And we definitely won't see the constructions like
> writeq(cpu_to_le64()) in the code, because it's weird.
> If I get it correctly it's equivalent to __raw_writeq().
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch
On Fri, Jun 29, 2018 at 7:06 PM, James Bottomley wrote: > On Fri, 2018-06-29 at 10:58 -0400, Chaitra P B wrote: >> "scsi: mpt3sas: Bug fix for big endian systems" >> >> Above patch with commit id "cf6bf9710cabba1fe94a4349f4eb8db623c77ebc" >> was posted to fix sparse warnings. While posting this patch it was >> assumed that readl() & writel() APIs internally calls le32_to_cpu() & >> cpu_to_le32() APIs respectively. Looks like it is not true for all >> architecture > > Just a minute, it damn well should be. The definition of readl/writel > is barriers and little endian (you can see this in asm-generic/io.h). > > Which architecture is getting this wrong? Because it sounds like > that's what we need to fix rather than doing something like this in all > drivers. > > Sparc (and parisc) definitely do the little endian thing, so if this > code is what it takes to get them working again, it looks like you're > double swapping somewhere. I really think cf6bf9710c needs to be > reverted and you should look again at your sparse warnings. Since the > driver is using the non-raw readX/writeX it should be cpu endian > internally which cf6bf9710c upsets. And we definitely won't see the constructions like writeq(cpu_to_le64()) in the code, because it's weird. If I get it correctly it's equivalent to __raw_writeq(). -- With Best Regards, Andy Shevchenko
Re: [PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch
On Fri, 2018-06-29 at 10:58 -0400, Chaitra P B wrote: > "scsi: mpt3sas: Bug fix for big endian systems" > > Above patch with commit id "cf6bf9710cabba1fe94a4349f4eb8db623c77ebc" > was posted to fix sparse warnings. While posting this patch it was > assumed that readl() & writel() APIs internally calls le32_to_cpu() & > cpu_to_le32() APIs respectively. Looks like it is not true for all > architecture Just a minute, it damn well should be. The definition of readl/writel is barriers and little endian (you can see this in asm-generic/io.h). Which architecture is getting this wrong? Because it sounds like that's what we need to fix rather than doing something like this in all drivers. Sparc (and parisc) definitely do the little endian thing, so if this code is what it takes to get them working again, it looks like you're double swapping somewhere. I really think cf6bf9710c needs to be reverted and you should look again at your sparse warnings. Since the driver is using the non-raw readX/writeX it should be cpu endian internally which cf6bf9710c upsets. James
Re: [PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch
From: Andy Shevchenko
Date: Fri, 29 Jun 2018 18:42:30 +0300
> On Fri, Jun 29, 2018 at 5:58 PM, Chaitra P B
> wrote:
>> "scsi: mpt3sas: Bug fix for big endian systems"
>>
>> Above patch with commit id "cf6bf9710cabba1fe94a4349f4eb8db623c77ebc" was
>> posted to fix sparse warnings. While posting this patch it was assumed that
>> readl() & writel() APIs internally calls le32_to_cpu() & cpu_to_le32() APIs
>> respectively.
>
>> Looks like it is not true for all architecture
Sparc does. It uses endianness translating stores and loads for the MMIO
accesses.
For example, readl() does:
__asm__ __volatile__("lduwa\t[%1] %2, %0\t/* pci_readl */"
: "=r" (ret)
: "r" (addr), "i" (ASI_PHYS_BYPASS_EC_E_L)
: "memory");
That "_L" at the end of the ASI_* value means "little-endian".
So if your understanding and basis for this bug fix is that sparc64
does not endian translate, it is a false one.
>> this patch is reverting back only those hunks which removed le32_to_cpu()
>> API call while using readl() API & cpu_to_le32() API call while using
>> writel() API.
>
> Can't you move to raw variants at the same time to be more clear with
> intentions?
> It would work on all architectures in the same way and won't trigger
> sparse warnings.
If you move to the raw variants you lose the memory barriers, it doesn't
just change the endianness of the access.
Re: [PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch
On Fri, Jun 29, 2018 at 5:58 PM, Chaitra P B wrote: > "scsi: mpt3sas: Bug fix for big endian systems" > > Above patch with commit id "cf6bf9710cabba1fe94a4349f4eb8db623c77ebc" was > posted to fix sparse warnings. While posting this patch it was assumed that > readl() & writel() APIs internally calls le32_to_cpu() & cpu_to_le32() APIs > respectively. > Looks like it is not true for all architecture Oh, what a mess. Looking at code comments I could imagine why it's done so... Sad. > and hence > this patch is reverting back only those hunks which removed le32_to_cpu() > API call while using readl() API & cpu_to_le32() API call while using > writel() API. Can't you move to raw variants at the same time to be more clear with intentions? It would work on all architectures in the same way and won't trigger sparse warnings. As an example: > - writeq(b, addr); > + writeq(cpu_to_le64(b), addr); /* Not all architectures has a writeq() equivalent to the below (sparc64) */ __raw_writeq(...) -- With Best Regards, Andy Shevchenko
[PATCH] mpt3sas: Fix for regression caused due to cf6bf9710c patch
"scsi: mpt3sas: Bug fix for big endian systems"
Above patch with commit id "cf6bf9710cabba1fe94a4349f4eb8db623c77ebc" was
posted to fix sparse warnings. While posting this patch it was assumed that
readl() & writel() APIs internally calls le32_to_cpu() & cpu_to_le32() APIs
respectively. Looks like it is not true for all architecture and hence
this patch is reverting back only those hunks which removed le32_to_cpu()
API call while using readl() API & cpu_to_le32() API call while using
writel() API.
Signed-off-by: Chaitra P B
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index dc41bd3..17b6b0a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -3321,7 +3321,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);
@@ -3344,7 +3344,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
@@ -5215,7 +5215,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;
}
@@ -5236,7 +5236,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))) {
@@ -5245,7 +5245,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);
@@ -5259,7 +5259,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);
}
--
1.8.3.1
Re: [PATCH] sd_zbc: Remove an assignment from sd_zbc_setup_report_cmnd()
Looks good, Reviewed-by: Johannes Thumshirn -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
