scsi: mptxsas: updates for ARM64

Current code gives up when 32 bit DMA is not supported. This problem has been observed on systems without any memory below 4 gig.

This patch tests 64 bit support before bailing out to find a working combination.

Quoted 7 lines · Sinan Kaya

Current code gives up when 32 bit DMA is not supported. This problem has been observed on systems without any memory below 4 gig.

This patch tests 64 bit support before bailing out to find a working combination.

That feels decidedly odd.

Why do you probe for 64bit if 32bit fails?

Typically it's the other way round, on the grounds that 64bit DMA should be preferred over 32bit.
Can you explain why it needs to be done the other way round here?

Quoted 8 lines · On Monday 09 November 2015 08:09:39 Hannes Reinecke wrote:

Quoted 7 lines · Sinan Kaya

Current code gives up when 32 bit DMA is not supported. This problem has been observed on systems without any memory below 4 gig.

This patch tests 64 bit support before bailing out to find a working combination.

Quoted 3 lines

That feels decidedly odd.

Why do you probe for 64bit if 32bit fails?

32-bit DMA mask on PCI cannot fail, we rely on that in all sorts of places. If the machine has all of its RAM visible above 4GB PCI bus addresses, it must use an IOMMU.

Quoted 4 lines

Typically it's the other way round, on the grounds that 64bit DMA should be preferred over 32bit.
Can you explain why it needs to be done the other way round here?

Something else is odd here, the driver already checks for dma_get_required_mask(), which will return the smallest mask that fits all of RAM. If the machine has any memory above 4GB, it already uses the 64-bit mask, and only falls back to the 32-bit mask if that fails or if all memory fits within the first 4GB.

So both the description and the patch are wrong. :(

Arnd

Quoted 13 lines · Arnd Bergmann

Quoted 11 lines · On Monday 09 November 2015 08:09:39 Hannes Reinecke wrote:

Quoted 7 lines · Sinan Kaya

Current code gives up when 32 bit DMA is not supported. This problem has been observed on systems without any memory below 4 gig.

This patch tests 64 bit support before bailing out to find a working combination.

That feels decidedly odd.

Why do you probe for 64bit if 32bit fails?

Quoted 3 lines

32-bit DMA mask on PCI cannot fail, we rely on that in all sorts of places. If the machine has all of its RAM visible above 4GB PCI bus addresses, it must use an IOMMU.

Can you be specific? PCIe does not have this limitation. It supports 32 bit and 64 bit TLPs.

I have not seen any limitation so far in the OS either.

Using IOMMU is fine but not required if the endpoint is a true 64 bit supporting endpoint. This endpoint supports 64bit too.

Quoted 11 lines

Quoted 3 lines

Typically it's the other way round, on the grounds that 64bit DMA should be preferred over 32bit.
Can you explain why it needs to be done the other way round here?

Something else is odd here, the driver already checks for dma_get_required_mask(), which will return the smallest mask that fits all of RAM. If the machine has any memory above 4GB, it already uses the 64-bit mask, and only falls back to the 32-bit mask if that fails or if all memory fits within the first 4GB.

I'll add some prints in the code to get to the bottom of it. I think the code is checking more than just the required mask and failing in one of the other conditions. At least that DMA comparison code was more than what I have ever seen.

Quoted 15 lines · On Monday 09 November 2015 09:07:36 Sinan Kaya wrote:

Quoted 13 lines · Arnd Bergmann

Quoted 11 lines · On Monday 09 November 2015 08:09:39 Hannes Reinecke wrote:

Quoted 7 lines · Sinan Kaya

Current code gives up when 32 bit DMA is not supported. This problem has been observed on systems without any memory below 4 gig.

This patch tests 64 bit support before bailing out to find a working combination.

That feels decidedly odd.

Why do you probe for 64bit if 32bit fails?

Quoted 8 lines

Quoted 3 lines

32-bit DMA mask on PCI cannot fail, we rely on that in all sorts of places. If the machine has all of its RAM visible above 4GB PCI bus addresses, it must use an IOMMU.

Can you be specific? PCIe does not have this limitation. It supports 32 bit and 64 bit TLPs.

I have not seen any limitation so far in the OS either.

See Documentation/DMA-API-HOWTO.txt

All PCI devices start out with a 32-bit DMA mask, and Linux assumes it can always fall back to a 32-bit mask if a smaller mask (needed for some devices that only support DMA on a subset of the 4GB) or larger mask (needed to address high memory but can fail when the PCI host does not support it) cannot be set.

Quoted 2 lines

Using IOMMU is fine but not required if the endpoint is a true 64 bit supporting endpoint. This endpoint supports 64bit too.

There are two aspects here:

a) setting a 32-bit mask should not have failed. Any device that actually needs 32-bit DMA will make the same call and the platform must guarantee that this works. If you have a broken platform that can't do this, complain to your board vendor so they wire up the RAM properly, with at least the first 2GB visible to low PCI bus addresses.

b) If the platform has any memory above 4GB and the supports high DMA, it should never have asked for the 32-bit mask before trying the 64-bit mask first. This is only a performance optimization, but the driver seems to do the right thing, so I assume there is something wrong with the platform code.

Quoted 12 lines

Quoted 11 lines

Quoted 3 lines

Typically it's the other way round, on the grounds that 64bit DMA should be preferred over 32bit.
Can you explain why it needs to be done the other way round here?

Something else is odd here, the driver already checks for dma_get_required_mask(), which will return the smallest mask that fits all of RAM. If the machine has any memory above 4GB, it already uses the 64-bit mask, and only falls back to the 32-bit mask if that fails or if all memory fits within the first 4GB.

Quoted 4 lines

I'll add some prints in the code to get to the bottom of it. I think the code is checking more than just the required mask and failing in one of the other conditions. At least that DMA comparison code was more than what I have ever seen.

Ok. That should take care of b) above, but we still have a bug with a) that will come back to bite you if you don't address it right.

Arnd

if (ioc->dma_mask)

     consistent_dma_mask = DMA_BIT_MASK(64);

else
consistent_dma_mask = DMA_BIT_MASK(32); <-- why here?

Quoted 2 lines · Arnd Bergmann

Quoted 1 line · On Monday 09 November 2015 09:07:36 Sinan Kaya wrote:

On 11/9/2015 3:59 AM, Arnd Bergmann wrote:

ioc->dma_mask is 0 and the driver is trying to use 32 bit even though 64 bit supported by the platform.

I think the proper fix is to pass the required_mask back to consistent_dma_mask rather than using ioc->dma_mask and guessing.

pci_set_consistent_dma_mask(pdev, required_mask)

My code was just a band aid for broken code.

No, as Timur found, the driver is correct and it intentionally sets the 32-bit mask, and that is guaranteed to work on all sane hardware. Don't change the driver but find a better platform for your workload, or talk to the people that are responsible for the platform and get them to fix it.

If the platform also doesn't have an IOMMU, you can probably work around it by setting up the dma-ranges property of the PCI host to map the low PCI addresses to the start of RAM. This will also require changes in the bootloader to set up the PCI outbound translation, and it will require implementing the DMA offset on ARM64, which I was hoping to avoid.

Quoted 5 lines · Arnd Bergmann

No, as Timur found, the driver is correct and it intentionally sets the 32-bit mask, and that is guaranteed to work on all sane hardware. Don't change the driver but find a better platform for your workload, or talk to the people that are responsible for the platform and get them to fix it.

Platform does have an IOMMU. No issues there. I am trying to clean out the patch pipe I have in order to get this card working with and without IOMMU.

Quoted 6 lines

If the platform also doesn't have an IOMMU, you can probably work around it by setting up the dma-ranges property of the PCI host to map the low PCI addresses to the start of RAM. This will also require changes in the bootloader to set up the PCI outbound translation, and it will require implementing the DMA offset on ARM64, which I was hoping to avoid.

From the email thread, it looks like this was introduced to support
some legacy card that has 64 bit addressing limitations and is being carried around ("rotted") since then.

I'm the second guy after the powerpc architecture complaining about the very same issue. Any red flags?

I can't change the address map for PCIe. SBSA requires all inbound PCIe addresses to be non-translated.

I'll just have to stick with IOMMU for this card.

What BenH was worried about here is that the driver sets different masks for streaming and coherent mappings, which is indeed a worry that could hit us on ARM as well, but I suppose we'll have to deal with that in platform code.

Setting both masks to 32-bit is something that a lot of drivers do, and without IOMMU enabled, you'd hit the same bug on all of them.

Quoted 5 lines · Arnd Bergmann

What BenH was worried about here is that the driver sets different masks for streaming and coherent mappings, which is indeed a worry that could hit us on ARM as well, but I suppose we'll have to deal with that in platform code.

Quoted 2 lines

Setting both masks to 32-bit is something that a lot of drivers do, and without IOMMU enabled, you'd hit the same bug on all of them.

Also note that I think that on PowerPC, the mask is set to 32 by default for all devices. I don't think we do that on ARM64. So on PowerPC, some drivers get away with not explicitly setting the mask.

If the mask is 64-bit by default on ARM64, that is a bug that we need to fix urgently. Can you verify this?

A lot of PCI devices can only do 32-bit DMA, and we have plenty of drivers that don't bother setting a mask at all because the 32-bit mask is the default on all other architectures.

Quoted 2 lines · Arnd Bergmann

If the mask is 64-bit by default on ARM64, that is a bug that we need to fix urgently. Can you verify this?

I think the mask is 0 by default, because there's no code in ARM64 that actually sets the mask.

Take a look at arch_setup_pdev_archdata() in
arch/powerpc/kernel/setup-common.c.

void arch_setup_pdev_archdata(struct platform_device *pdev)

{
         pdev->archdata.dma_mask = DMA_BIT_MASK(32);
         pdev->dev.dma_mask = &pdev->archdata.dma_mask;
         set_dma_ops(&pdev->dev, &dma_direct_ops);
}

I don't see anything equivalent in arch/arm64

Quoted 3 lines

A lot of PCI devices can only do 32-bit DMA, and we have plenty of drivers that don't bother setting a mask at all because the 32-bit mask is the default on all other architectures.

In our drivers for 32-bit devices, we have to explicitly set the DMA mask to 32-bits in order to get any DMA working.

Quoted 2 lines

In our drivers for 32-bit devices, we have to explicitly set the DMA mask to 32-bits in order to get any DMA working.

Do you mean PCI devices or platform devices?

Maybe the parent bus is lacking a dma-ranges property?

Quoted 4 lines · Arnd Bergmann

Quoted 2 lines

In our drivers for 32-bit devices, we have to explicitly set the DMA mask to 32-bits in order to get any DMA working.

Do you mean PCI devices or platform devices?

Platform.

Quoted 1 line

Maybe the parent bus is lacking a dma-ranges property?

All of this applies only on device-tree platforms. Sinan and I are working on an ACPI server platform. So we never call of_dma_configure(), and we don't have a dma-ranges property.

Quoted 7 lines · On Tuesday 10 November 2015 11:06:40 Sinan Kaya wrote:

Quoted 5 lines · Arnd Bergmann

No, as Timur found, the driver is correct and it intentionally sets the 32-bit mask, and that is guaranteed to work on all sane hardware. Don't change the driver but find a better platform for your workload, or talk to the people that are responsible for the platform and get them to fix it.

Quoted 3 lines

Platform does have an IOMMU. No issues there. I am trying to clean out the patch pipe I have in order to get this card working with and without IOMMU.

On PowerPC, I think we automatically enable the IOMMU whenever a DMA mask is set that doesn't cover all of the RAM. We could think about doing the same thing on ARM64 to make all devices work out of the box.

Quoted 7 lines

Quoted 6 lines

If the platform also doesn't have an IOMMU, you can probably work around it by setting up the dma-ranges property of the PCI host to map the low PCI addresses to the start of RAM. This will also require changes in the bootloader to set up the PCI outbound translation, and it will require implementing the DMA offset on ARM64, which I was hoping to avoid.

Quoted 6 lines

From the email thread, it looks like this was introduced to support
some legacy card that has 64 bit addressing limitations and is being carried around ("rotted") since then.

I'm the second guy after the powerpc architecture complaining about the very same issue. Any red flags?

What BenH was worried about here is that the driver sets different masks for streaming and coherent mappings, which is indeed a worry that could hit us on ARM as well, but I suppose we'll have to deal with that in platform code.

Setting both masks to 32-bit is something that a lot of drivers do, and without IOMMU enabled, you'd hit the same bug on all of them.

Quoted 2 lines

I can't change the address map for PCIe. SBSA requires all inbound PCIe addresses to be non-translated.

What about changing the memory map? I suspect there will be more problems for you in the future when all of your RAM is at high addresses. Is this something you could fix in the bootloader by moving the first 2GB to a different CPU physical address?

Quoted 2 lines

I'll just have to stick with IOMMU for this card.

Ok. But how do you currently decide whether to use the IOMMU or not?

Arnd

Quoted 14 lines · Arnd Bergmann

Quoted 7 lines · On Tuesday 10 November 2015 11:06:40 Sinan Kaya wrote:

On 11/10/2015 3:38 AM, Arnd Bergmann wrote:
From the email thread, it looks like this was introduced to support
some legacy card that has 64 bit addressing limitations and is being carried around ("rotted") since then.

I'm the second guy after the powerpc architecture complaining about the very same issue. Any red flags?

What BenH was worried about here is that the driver sets different masks for streaming and coherent mappings, which is indeed a worry that could hit us on ARM as well, but I suppose we'll have to deal with that in platform code.

Quoted 3 lines

Setting both masks to 32-bit is something that a lot of drivers do, and without IOMMU enabled, you'd hit the same bug on all of them.

Maybe, maybe not. This is the only card that I had problems with.

Quoted 15 lines · Sinan Kaya

Quoted 14 lines · Arnd Bergmann

Quoted 7 lines · On Tuesday 10 November 2015 11:06:40 Sinan Kaya wrote:

On 11/10/2015 3:38 AM, Arnd Bergmann wrote:
From the email thread, it looks like this was introduced to support
some legacy card that has 64 bit addressing limitations and is being carried around ("rotted") since then.

I'm the second guy after the powerpc architecture complaining about the very same issue. Any red flags?

What BenH was worried about here is that the driver sets different masks for streaming and coherent mappings, which is indeed a worry that could hit us on ARM as well, but I suppose we'll have to deal with that in platform code.

Quoted 5 lines

Quoted 3 lines

Setting both masks to 32-bit is something that a lot of drivers do, and without IOMMU enabled, you'd hit the same bug on all of them.

Maybe, maybe not. This is the only card that I had problems with.

Your characterisation of "some legacy card" isn't entirely correct. Just to clarify how this happens, most I/O cards today are intelligent offload engines which means they have some type of embedded CPU (it can even be a specially designed asic). This CPU is driven by firmware which is mostly (but not always) in the machine language of the CPU. DMA transfers are sometimes run by this CPU, but mostly handed off to a separate offload engine. When the board gets revised, it's often easier to update the offload engine to 64 bits and keep the CPU at 32 (or even
16) bits. This means that all the internal addresses in the firmware are 32 bit only. As I read the comments in the original thread, it looks like the mpt people tried to mitigate this by using segment registers for external addresses firmware uses ... that's why they say that they don't have to have all the addresses in DMA32 ... they just need the upper 32 bits to be constant so they can correctly program the segment register. Unfortunately, we have no way to parametrise this to the DMA allocation code.

You'll find the same thing with Adaptec SPI cards. Their route to 64 bits was via an initial 39 bit extension that had them layering the additional 7 bits into the unused lower region of the page descriptors for the firmware (keeping the actual pointers to DMA at 32 bits because they're always parametrised as address, offset, length and the address is always a 4k page).

Eventually, everything will rev to 64 bits and this problem will go away, but, as I suspect you know, it takes time for the embedded world to get to where everyone else already is.

As Arnd said, if you failed to allow for this in your platform, then oops, just don't use the card. I think this solution would be better than trying to get the driver to work out which cards can support 64 bit firmware descriptors and only failing on your platform for those that can't.

James

Quoted 50 lines · James Bottomley

Quoted 15 lines · Sinan Kaya

Quoted 14 lines · Arnd Bergmann

Quoted 7 lines · On Tuesday 10 November 2015 11:06:40 Sinan Kaya wrote:

On 11/10/2015 3:38 AM, Arnd Bergmann wrote:
From the email thread, it looks like this was introduced to support
some legacy card that has 64 bit addressing limitations and is being carried around ("rotted") since then.

I'm the second guy after the powerpc architecture complaining about the very same issue. Any red flags?

What BenH was worried about here is that the driver sets different masks for streaming and coherent mappings, which is indeed a worry that could hit us on ARM as well, but I suppose we'll have to deal with that in platform code.

Quoted 5 lines

Quoted 3 lines

Setting both masks to 32-bit is something that a lot of drivers do, and without IOMMU enabled, you'd hit the same bug on all of them.

Maybe, maybe not. This is the only card that I had problems with.

Your characterisation of "some legacy card" isn't entirely correct. Just to clarify how this happens, most I/O cards today are intelligent offload engines which means they have some type of embedded CPU (it can even be a specially designed asic). This CPU is driven by firmware which is mostly (but not always) in the machine language of the CPU. DMA transfers are sometimes run by this CPU, but mostly handed off to a separate offload engine. When the board gets revised, it's often easier to update the offload engine to 64 bits and keep the CPU at 32 (or even
16) bits. This means that all the internal addresses in the firmware are 32 bit only. As I read the comments in the original thread, it looks like the mpt people tried to mitigate this by using segment registers for external addresses firmware uses ... that's why they say that they don't have to have all the addresses in DMA32 ... they just need the upper 32 bits to be constant so they can correctly program the segment register. Unfortunately, we have no way to parametrise this to the DMA allocation code.

You'll find the same thing with Adaptec SPI cards. Their route to 64 bits was via an initial 39 bit extension that had them layering the additional 7 bits into the unused lower region of the page descriptors for the firmware (keeping the actual pointers to DMA at 32 bits because they're always parametrised as address, offset, length and the address is always a 4k page).

Eventually, everything will rev to 64 bits and this problem will go away, but, as I suspect you know, it takes time for the embedded world to get to where everyone else already is.

Quoted 9 lines

As Arnd said, if you failed to allow for this in your platform, then oops, just don't use the card. I think this solution would be better than trying to get the driver to work out which cards can support 64 bit firmware descriptors and only failing on your platform for those that can't.

James

James,
I was referring to this conversation here.

https://lkml.org/lkml/2015/2/20/31

"The aic79xx hardware problem was that the DMA engine could address the whole of memory (it had two address modes, a 39 bit one and a 64 bit one) but the script engine that runs the mailboxes only had a 32 bit activation register (the activating write points at the physical address of the script to begin executing)."

The fact that LSI SAS 92118i is working with 64 bit addresses suggests me that this problem is already solved. I have not hit any kind of regressions with 93xx and 92xx families under load in a true 64 bit environment. I am only mentioning this based on my testing exposure.

Another comment here from you.
https://lkml.org/lkml/2015/4/2/28

"Well, it was originally a hack for altix, because they had no regions below 4GB and had to specifically manufacture them. As you know, in Linux, if Intel doesn't need it, no-one cares and the implementation bitrots."

Maybe, it is time to fix the code for more recent (even decent) hardware?

The Issue, as stated by LSI is

Initially set the consistent DMA mask to 32 bit and then change it
to 64 bit mask after allocating RDPQ pools by calling the function
_base_change_consistent_dma_mask. This is to ensure that all the upper 32 bits of RDPQ entries's base address to be same.

If you set a 64 bit coherent mask before this point, you're benefiting from being lucky that all the upper 32 bits of the allocations are the same ... we can't code a driver to rely on luck. Particularly not when the failure mode looks like it would be silent and deadly.

Quoted 9 lines

Quoted 9 lines

Another comment here from you.
https://lkml.org/lkml/2015/4/2/28

"Well, it was originally a hack for altix, because they had no regions below 4GB and had to specifically manufacture them. As you know, in Linux, if Intel doesn't need it, no-one cares and the implementation bitrots."

Quoted 1 line

Quoted 1 line

Maybe, it is time to fix the code for more recent (even decent) hardware?

What do you mean "fix the code"? The code isn't broken, it's parametrising issues with particular hardware. There's no software work around (except allocating memory with the correct characteristics).

Quoted 9 lines · James Bottomley

The Issue, as stated by LSI is

Initially set the consistent DMA mask to 32 bit and then change it
to 64 bit mask after allocating RDPQ pools by calling the function
_base_change_consistent_dma_mask. This is to ensure that all the upper 32 bits of RDPQ entries's base address to be same.

Need somebody from mpt to confirm that this behavior is still valid for the recent cards besides altix.

OK, you don't seem to be understanding the problem: the Altix isn't a LSI card, it was a SGI platform.

It was the platform where we first
discovered the issue that a lot of storage cards didn't work because it by default had no memory below 4GB. The reason coherent masks were introduced was initially so the Altix could manufacture and manage a region of memory in the lower 4GB region and we would guarantee to make allocations from it so the storage cards would then work on that platform.

Quoted 2 lines · James Bottomley

OK, you don't seem to be understanding the problem: the Altix isn't a LSI card, it was a SGI platform.

Got it.

Quoted 7 lines

It was the platform where we first
discovered the issue that a lot of storage cards didn't work because it by default had no memory below 4GB. The reason coherent masks were introduced was initially so the Altix could manufacture and manage a region of memory in the lower 4GB region and we would guarantee to make allocations from it so the storage cards would then work on that platform.

I can't fix the issue if the card cannot do 64 bit DMA when IOMMU is not there. I need IOMMU enabled all the time for this card.

Quoted 12 lines · Arnd Bergmann

Quoted 10 lines · On Tuesday 10 November 2015 11:06:40 Sinan Kaya wrote:

Quoted 5 lines · Arnd Bergmann

No, as Timur found, the driver is correct and it intentionally sets the 32-bit mask, and that is guaranteed to work on all sane hardware. Don't change the driver but find a better platform for your workload, or talk to the people that are responsible for the platform and get them to fix it.

Platform does have an IOMMU. No issues there. I am trying to clean out the patch pipe I have in order to get this card working with and without IOMMU.

Quoted 4 lines

On PowerPC, I think we automatically enable the IOMMU whenever a DMA mask is set that doesn't cover all of the RAM. We could think about doing the same thing on ARM64 to make all devices work out of the box.

The ACPI IORT table declares whether you enable IOMMU for a particular device or not. The placement of IOMMU HW is system specific. The IORT table gives the IOMMU HW topology to the operating system.

Quoted 3 lines

The ACPI IORT table declares whether you enable IOMMU for a particular

Quoted 2 lines

device or not. The placement of IOMMU HW is system specific. The IORT table gives the IOMMU HW topology to the operating system.

This sounds odd. Clearly you need to specify the IOMMU settings for each possible PCI device independent of whether the OS actually uses the IOMMU or not.

In a lot of cases, we want to turn it off to get better performance when the driver has set a DMA mask that covers all of RAM, but you also want to enable the IOMMU for debugging purposes or for device assignment if you run virtual machines. The bootloader doesn't know how the device is going to be used, so it cannot define the policy here.

Quoted 3 lines · Arnd Bergmann

Quoted 3 lines

The ACPI IORT table declares whether you enable IOMMU for a particular

Quoted 2 lines

device or not. The placement of IOMMU HW is system specific. The IORT table gives the IOMMU HW topology to the operating system.

Quoted 3 lines

This sounds odd. Clearly you need to specify the IOMMU settings for each possible PCI device independent of whether the OS actually uses the IOMMU or not.

There are provisions to have DMA mask in the PCIe host bridge not at the PCIe device level inside IORT table. This setting is specific for each PCIe bus. It is not per PCIe device.

It is assumed that the endpoint device driver knows the hardware for PCIe devices. The driver can also query the supported DMA bits by this platform via DMA APIs and will request the correct DMA mask from the DMA subsystem (surprise!).

Quoted 5 lines

In a lot of cases, we want to turn it off to get better performance when the driver has set a DMA mask that covers all of RAM, but you also want to enable the IOMMU for debugging purposes or for device assignment if you run virtual machines. The bootloader doesn't know how the device is going to be used, so it cannot define the policy here.

I think we'll end up adding a virtualization option to the UEFI BIOS similar to how Intel platforms work. Based on this switch, we'll end up patching the ACPI table.

If I remove the IORT entry, then the device is in coherent mode with device accessing the full RAM range.

If I have the IORT table, the device is in IOMMU translation mode.

Details are in the IORT spec.

Quoted 7 lines · Sinan Kaya

Current code gives up when 32 bit DMA is not supported. This problem has been observed on systems without any memory below 4 gig.

This patch tests 64 bit support before bailing out to find a working combination.

That feels decidedly odd.

Why do you probe for 64bit if 32bit fails?
Typically it's the other way round, on the grounds that 64bit DMA should be preferred over 32bit.
Can you explain why it needs to be done the other way round here?

Cheers,

Hannes

The MULDIV macro has been designed for small numbers. Compiler emits an overflow warning on 64 bit systems. This patch uses 64 bit numbers in order to suppress warning.

Signed-off-by Sinan Kaya <[email protected]>

---
 drivers/scsi/sg.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

drivers/scsi/sg.c +1 -2

index 9d7b7db..112d8974 100644
@@ -51,6 +51,7 @@ static int sg_version_num = 30536;    /* 2 digits for each component */
 #include <linux/atomic.h>
 #include <linux/ratelimit.h>
 #include <linux/uio.h>
+#include <asm/div64.h>
 
 #include "scsi.h"
 #include <scsi/scsi_dbg.h>

@@ -85,12 +86,17 @@ static void sg_proc_cleanup(void);
  * Replacing muldiv(x) by muldiv(x)=((x % d) * m) / d + int(x / d) * m
  * calculates the same, but prevents the overflow when both m and d
  * are "small" numbers (like HZ and USER_HZ).
- * Of course an overflow is inavoidable if the result of muldiv doesn't fit
- * in 32 bits.

drivers/scsi/sg.c +2 -1

+}
 
-#define SG_DEFAULT_TIMEOUT MULDIV(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
+#define SG_DEFAULT_TIMEOUT mult_frac64(SG_DEFAULT_TIMEOUT_USER, HZ, USER_HZ)
 
 int sg_big_buff = SG_DEF_RESERVED_SIZE;

/* N.B. This variable is readable and writeable via

drivers/scsi/sg.c +3 -3

@@ -877,10 +883,10 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
                        return result;
                if (val < 0)
                        return -EIO;
-               if (val >= MULDIV (INT_MAX, USER_HZ, HZ))
-                   val = MULDIV (INT_MAX, USER_HZ, HZ);
+               if (val >= mult_frac64(INT_MAX, USER_HZ, HZ))
+                       val = mult_frac64(INT_MAX, USER_HZ, HZ);
 
 
                sfp->timeout_user = val;
-               sfp->timeout = MULDIV (val, HZ, USER_HZ);
+               sfp->timeout = mult_frac64(val, HZ, USER_HZ);
 

return 0;
case SG_GET_TIMEOUT: /* N.B. User receives timeout as return value */
--
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Parens are useless, noticed later, sorry.

Isn't mult_frac() enough here?

Btw, can you mention explicitly what is the warning you get (copy'n'paste of the line would be okay)?

I created this patch back in March with an older version of the compiler and older kernel (3.19). I'm no longer able to reproduce this with this compiler and linux-next.

Thread model: posix
gcc version 4.8.3 20140401 (prerelease) (crosstool-NG linaro-1.13.1-4.8-2014.04 - Linaro GCC 4.8-2014.04)

I'll drop this patch.

Quoted 5 lines · Sinan Kaya

I created this patch back in March with an older version of the compiler and older kernel (3.19). I'm no longer able to reproduce this with this compiler and linux-next.

Quoted 5 lines

Thread model: posix
gcc version 4.8.3 20140401 (prerelease) (crosstool-NG linaro-1.13.1-4.8-2014.04 - Linaro GCC 4.8-2014.04)

I'll drop this patch.

Are you sure the compiler handles the old macro correctly? Maybe it's just quiescing the error message, but it's still broken?

The code says it is using these macros for small integers only which can't overflow. I was trying to get rid of compiler warning and it seems to have disappeared.

Quoted 5 lines · Sinan Kaya

The code says it is using these macros for small integers only which can't overflow. I was trying to get rid of compiler warning and it seems to have disappeared.

I would double-check the assembly code, if I were you. I don't like it when warnings just go away like that.

Besides, we *should* be using do_div() for 64-bit division.

Quoted 4 lines · Sinan Kaya

The code says it is using these macros for small integers only which can't overflow. I was trying to get rid of compiler warning and it seems to have disappeared.

I would double-check the assembly code, if I were you. I don't like it when warnings just go away like that.

Besides, we *should* be using do_div() for 64-bit division.

The mpt2sas and mpt3sas drivers are spinning forever in their IRQ handlers if there are a lot of jobs queued up by the PCIe card. This handler is causing spikes for the rest of the system and sluggish behavior.

Marking all MSI interrupts as non-shared and moving the MSI interrupts to thread context. This relexes the rest of the system execution.

Quoted 9 lines · Sinan Kaya

The mpt2sas and mpt3sas drivers are spinning forever in their IRQ handlers if there are a lot of jobs queued up by the PCIe card. This handler is causing spikes for the rest of the system and sluggish behavior.

Marking all MSI interrupts as non-shared and moving the MSI interrupts to thread context. This relexes the rest of the system execution.

NACK.

If there is a scalability issue when handling interrupts it should be fixed in the driver directly.

Looking at the driver is should be possible to implement a worker thread handling the reply descriptor, and having the interrupt only to fetch the reply descriptor.

Cheers,

Hannes

Quoted 9 lines · Sinan Kaya

The mpt2sas and mpt3sas drivers are spinning forever in their IRQ handlers if there are a lot of jobs queued up by the PCIe card. This handler is causing spikes for the rest of the system and sluggish behavior.

Marking all MSI interrupts as non-shared and moving the MSI interrupts to thread context. This relexes the rest of the system execution.

NACK.

If there is a scalability issue when handling interrupts it should be fixed in the driver directly.

Looking at the driver is should be possible to implement a worker thread handling the reply descriptor, and having the interrupt only to fetch the reply descriptor.

Cheers,

Hannes

Quoted 10 lines · Hannes Reinecke

Quoted 9 lines · Sinan Kaya

The mpt2sas and mpt3sas drivers are spinning forever in their IRQ handlers if there are a lot of jobs queued up by the PCIe card. This handler is causing spikes for the rest of the system and sluggish behavior.

Marking all MSI interrupts as non-shared and moving the MSI interrupts to thread context. This relexes the rest of the system execution.

Quoted 9 lines

NACK.

If there is a scalability issue when handling interrupts it should be fixed in the driver directly.

Looking at the driver is should be possible to implement a worker thread handling the reply descriptor, and having the interrupt only to fetch the reply descriptor.

Can you go into the detail about which part of the _base_interrupt function needs to be executed in ISR context and which part can be queued up to worker thread?

I'm not familiar with the hardware or the code. That's why, I moved the entire ISR into the thread context.