| Message ID | 1530284290-8555-1-git-send-email-chaitra.basappa@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Jun 29, 2018 at 5:58 PM, Chaitra P B <chaitra.basappa@broadcom.com> 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(...)
From: Andy Shevchenko <andy.shevchenko@gmail.com> Date: Fri, 29 Jun 2018 18:42:30 +0300 > On Fri, Jun 29, 2018 at 5:58 PM, Chaitra P B > <chaitra.basappa@broadcom.com> 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.
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
On Fri, Jun 29, 2018 at 7:06 PM, James Bottomley <James.Bottomley@hansenpartnership.com> 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().
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
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jun 29, 2018 at 7:06 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> 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
Hi, Any suggestion/update over my previous mail. I am using 4.13 kernel. Thanks, Sreekanth On Sat, Jun 30, 2018 at 12:34 AM, Sreekanth Reddy <sreekanth.reddy@broadcom.com> wrote: > 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 > <andy.shevchenko@gmail.com> wrote: >> On Fri, Jun 29, 2018 at 7:06 PM, James Bottomley >> <James.Bottomley@hansenpartnership.com> 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
From: Sreekanth Reddy <sreekanth.reddy@broadcom.com> Date: Tue, 3 Jul 2018 17:48:49 +0530 > Any suggestion/update over my previous mail. I am using 4.13 kernel. I think the issue is that if you are reading a 32-bit word and then interpreting it as a struct full of individual bytes, you have to order the bytes in the structure appropriately for the cpu endianness. Look at the 32-bit value read, it is identical in both the x86 and sparc cases.
On Tue, 2018-07-03 at 22:49 +0900, David Miller wrote: > From: Sreekanth Reddy <sreekanth.reddy@broadcom.com> > Date: Tue, 3 Jul 2018 17:48:49 +0530 > > > Any suggestion/update over my previous mail. I am using 4.13 > kernel. > > I think the issue is that if you are reading a 32-bit word and then > interpreting it as a struct full of individual bytes, you have to > order the bytes in the structure appropriately for the cpu > endianness. This is undoubtedly it. The point being if you read from a structure using readX, you have to read every element at its correct length for the endian swaps to work. You can't do a readq on 2 32 bit words and expect the endianness to be correct (you'll find they come out in the wrong order). I think you're using a shared (device and cpu) memory mapped structured data with a doorbell register, which is pretty identical to how the qla1280 does it. We went through several iterations of fixing that driver for big endian but finally settled on putting __le annotations on all the structures and doing cpu_to_leX() swaps as we wrote them (and obviously leX_to_cpu() swaps to read them), meaning the structure in memory is always correct for the device. Then we used a writeX to poke the doorbell and the device just picked up the correct information. The rule you want to be following is: memory mapped structure, you're responsible for annotation and swapping; readX/writeX to correctly sized data, the API will swap for you. So, can we just revert the original patch which is clearly now a regression and try to get this fixed in the merge window? I think the actual bug is simply you're missing __leX annotations on the shared memory mapped structure to fix sparse, but otherwise everything is working. James
On Tue, Jul 3, 2018 at 8:53 PM, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Tue, 2018-07-03 at 22:49 +0900, David Miller wrote: >> From: Sreekanth Reddy <sreekanth.reddy@broadcom.com> >> Date: Tue, 3 Jul 2018 17:48:49 +0530 >> >> > Any suggestion/update over my previous mail. I am using 4.13 >> kernel. >> >> I think the issue is that if you are reading a 32-bit word and then >> interpreting it as a struct full of individual bytes, you have to >> order the bytes in the structure appropriately for the cpu >> endianness. > > This is undoubtedly it. The point being if you read from a structure > using readX, you have to read every element at its correct length for > the endian swaps to work. You can't do a readq on 2 32 bit words and > expect the endianness to be correct (you'll find they come out in the > wrong order). > > I think you're using a shared (device and cpu) memory mapped structured > data with a doorbell register, [Sreekanth] Yes it is correct, we are using a shared memory mapped structured data with a doorbell register. In this particular handshake function, HBA will send only 16 bit data at a time. So driver has to read 16 bit data at a time in a loop until complete reply message is read. > which is pretty identical to how the > qla1280 does it. We went through several iterations of fixing that > driver for big endian but finally settled on putting __le annotations > on all the structures and doing cpu_to_leX() swaps as we wrote them > (and obviously leX_to_cpu() swaps to read them), meaning the structure > in memory is always correct for the device. Then we used a writeX to > poke the doorbell and the device just picked up the correct > information. > > The rule you want to be following is: memory mapped structure, you're > responsible for annotation and swapping; readX/writeX to correctly > sized data, the API will swap for you. > > So, can we just revert the original patch which is clearly now a > regression and try to get this fixed in the merge window? [Sreekanth] Yes we can revert the original patch. > I think the > actual bug is simply you're missing __leX annotations on the shared > memory mapped structure to fix sparse, but otherwise everything is > working. Actually our memory mapped structure variables are declared with __leX annotations as shown below, 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; and typedef u8 U8; typedef __le16 U16; typedef __le32 U32; typedef __le64 U64 __attribute__ ((aligned(4))); Also I tried replacing readl() API with readw()API (as HBA FW will send 16 bit data at a time) as shown below and still I see same issue, MPI2DefaultReply_t *default_reply = (MPI2DefaultReply_t *)reply; u16 reply1; reply1 = readw(&ioc->chip->Doorbell); reply[1] = reply1; 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); And I got below output where message length is wrong, when I execute above code on SPARC64 machine, LSI debug.. 0x311, 0x311 LSI MsgLength :3 Thanks, Sreekanth > > James >
From: Sreekanth Reddy <sreekanth.reddy@broadcom.com> Date: Wed, 4 Jul 2018 16:54:05 +0530 > > Also I tried replacing readl() API with readw()API (as HBA FW will > send 16 bit data at a time) as shown below and still I see same issue, > > MPI2DefaultReply_t *default_reply = (MPI2DefaultReply_t *)reply; > u16 reply1; > reply1 = readw(&ioc->chip->Doorbell); > reply[1] = reply1; > > 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); > > And I got below output where message length is wrong, when I execute > above code on SPARC64 machine, It's the ordering of the u8 objects in the structure that is the problem, on big endian they need to be swapped.
On Wed, Jul 4, 2018 at 5:52 PM, David Miller <davem@davemloft.net> wrote: > From: Sreekanth Reddy <sreekanth.reddy@broadcom.com> > Date: Wed, 4 Jul 2018 16:54:05 +0530 > >> >> Also I tried replacing readl() API with readw()API (as HBA FW will >> send 16 bit data at a time) as shown below and still I see same issue, >> >> MPI2DefaultReply_t *default_reply = (MPI2DefaultReply_t *)reply; >> u16 reply1; >> reply1 = readw(&ioc->chip->Doorbell); >> reply[1] = reply1; >> >> 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); >> >> And I got below output where message length is wrong, when I execute >> above code on SPARC64 machine, > > It's the ordering of the u8 objects in the structure that is the problem, > on big endian they need to be swapped. David, This is a shared structure between the host drivers and HBA device. HBA Firmware sends the information though this structures which are defined in the MPI headers. Now we can't change the order of these u8 objects. 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; Please let me know what I can do further. Thanks, Sreekanth
From: Sreekanth Reddy <sreekanth.reddy@broadcom.com> Date: Mon, 9 Jul 2018 17:12:51 +0530 > This is a shared structure between the host drivers and HBA device. > HBA Firmware sends the information though this structures which are > defined in the MPI headers. Now we can't change the order of these u8 > objects. > > typedef struct _MPI2_DEFAULT_REPLY { > U16 FunctionDependent1; /*0x00 */ > U8 MsgLength; /*0x02 */ > U8 Function; /*0x03 */ > U16 FunctionDependent2; /*0x04 */ When a big-endian cpu stores a 16-bit or 32-bit value into a piece of "cpu memory", which is what you are doing before interpreting the u8 values, you must define the u8 components in the endianness order they will be stored in. Alternatively, you must extract the 8-bit values by hand using shifts and masks in a local variable. The huge problem is that you are mixing and matching I/O memory accessors which do endianness swaps for you, and normal cpu loads and stores into a data structure in cpu memory, then reading smaller sized components back and expects this all to work out properly. In fact, if you want to keep the structure definition above as-is, you should swap the I/O memory read value _BACK_ to cpu endianness before storing it. And in fact that is what the code is doing now, which you broke by trying to "fix" it. Meanwhile, this discussion has been drawn out way way way too long, and the fact is that your change broke big endian instead of fixing any real problem whatsoever. You did not test your change on any real big endian system. Therefore, as requested from the very beginning, would you please revert this change which broke big endian and for which you do not have a satisfactory resolution for at this time. This is how regressions are handled, if a fix cannot be procured in a short amount of time, we revert. Thank you.
On Tue, Jul 10, 2018 at 2:38 AM, David Miller <davem@davemloft.net> wrote: > From: Sreekanth Reddy <sreekanth.reddy@broadcom.com> > Date: Mon, 9 Jul 2018 17:12:51 +0530 > >> This is a shared structure between the host drivers and HBA device. >> HBA Firmware sends the information though this structures which are >> defined in the MPI headers. Now we can't change the order of these u8 >> objects. >> >> typedef struct _MPI2_DEFAULT_REPLY { >> U16 FunctionDependent1; /*0x00 */ >> U8 MsgLength; /*0x02 */ >> U8 Function; /*0x03 */ >> U16 FunctionDependent2; /*0x04 */ > > When a big-endian cpu stores a 16-bit or 32-bit value into a piece of > "cpu memory", which is what you are doing before interpreting the u8 > values, you must define the u8 components in the endianness order > they will be stored in. > > Alternatively, you must extract the 8-bit values by hand using > shifts and masks in a local variable. > > The huge problem is that you are mixing and matching I/O memory > accessors which do endianness swaps for you, and normal cpu > loads and stores into a data structure in cpu memory, then > reading smaller sized components back and expects this all to > work out properly. > > In fact, if you want to keep the structure definition above > as-is, you should swap the I/O memory read value _BACK_ to > cpu endianness before storing it. > > And in fact that is what the code is doing now, which you broke by > trying to "fix" it. > > Meanwhile, this discussion has been drawn out way way way too long, > and the fact is that your change broke big endian instead of fixing > any real problem whatsoever. You did not test your change on any real > big endian system. > > Therefore, as requested from the very beginning, would you please > revert this change which broke big endian and for which you do not > have a satisfactory resolution for at this time. I am trying to revert patch commit cf6bf9710c (scsi: mpt3sas: Bug fix for big endian systems) but revert operation is not happening smoothly, it may be due to dependency with some other patch changes. Anyway in this patch we have reverted those hunks of cf6bf9710c patch which are problematic other hunks of cf6bf9710c patch are good. Can't this patch be considered? or shall I resend this patch with description(i.e. patch header) change. Thanks, Sreekanth > > This is how regressions are handled, if a fix cannot be procured in a > short amount of time, we revert. > > Thank you.
Sreekanth, > I am trying to revert patch commit cf6bf9710c (scsi: mpt3sas: Bug fix > for big endian systems) but revert operation is not happening > smoothly, it may be due to dependency with some other patch changes. > > Anyway in this patch we have reverted those hunks of cf6bf9710c patch > which are problematic other hunks of cf6bf9710c patch are good. Can't > this patch be considered? or shall I resend this patch with > description(i.e. patch header) change. Since this commit is buried a bit deep in the pile, please submit an incremental patch that addresses Dave's problem. Thanks!
> Since this commit is buried a bit deep in the pile, please submit an > incremental patch that addresses Dave's problem. I obviously missed the first email of this thread. Sigh.
On Wed, Jul 11, 2018 at 8:32 AM, Martin K. Petersen <martin.petersen@oracle.com> wrote: > >> Since this commit is buried a bit deep in the pile, please submit an >> incremental patch that addresses Dave's problem. This patch "Fix for regression caused due to cf6bf9710c" addresses the Dave's problem. > > I obviously missed the first email of this thread. Sigh. Whether I need to re-send this same patch? > > -- > Martin K. Petersen Oracle Linux Engineering
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); }
"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 <chaitra.basappa@broadcom.com> --- drivers/scsi/mpt3sas/mpt3sas_base.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)