Using a 64 bit constant generates "warning: integer constant is too
large for 'long' type" on 32 bit platforms. Instead use ~0l to get
the desired effect.
Detected by Andrew Morton who has confirmed that this patch
fixes the warning on i386/gcc-4.4.3, i386/gcc-4.4.0 and arm/gcc-4.4.4.
Signed-off-by: Sowmini Varadhan <[email protected]>
---
lib/iommu-common.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/lib/iommu-common.c b/lib/iommu-common.c
index df30632..fd1297d 100644
--- a/lib/iommu-common.c
+++ b/lib/iommu-common.c
@@ -119,7 +119,7 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
unsigned long align_mask = 0;
if (align_order > 0)
- align_mask = 0xffffffffffffffffl >> (64 - align_order);
+ align_mask = ~0l >> (64 - align_order);
/* Sanity check */
if (unlikely(npages == 0)) {
--
1.7.1
On Sun, Jul 19, 2015 at 02:20:14PM +0200, Sowmini Varadhan wrote:
>
> Using a 64 bit constant generates "warning: integer constant is too
> large for 'long' type" on 32 bit platforms. Instead use ~0l to get
> the desired effect.
>
> Detected by Andrew Morton who has confirmed that this patch
> fixes the warning on i386/gcc-4.4.3, i386/gcc-4.4.0 and arm/gcc-4.4.4.
>
> Signed-off-by: Sowmini Varadhan <[email protected]>
> ---
> lib/iommu-common.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/lib/iommu-common.c b/lib/iommu-common.c
> index df30632..fd1297d 100644
> --- a/lib/iommu-common.c
> +++ b/lib/iommu-common.c
> @@ -119,7 +119,7 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
> unsigned long align_mask = 0;
>
> if (align_order > 0)
> - align_mask = 0xffffffffffffffffl >> (64 - align_order);
> + align_mask = ~0l >> (64 - align_order);
>
Wonder if this just hides the real problem. Unless align_order
is very large, the resulting mask on 32 bit systems may be 0.
Is this really the idea ?
Guenter
On Sun, Jul 19 2015, Guenter Roeck <[email protected]> wrote:
> On Sun, Jul 19, 2015 at 02:20:14PM +0200, Sowmini Varadhan wrote:
>>
>> Using a 64 bit constant generates "warning: integer constant is too
>> large for 'long' type" on 32 bit platforms. Instead use ~0l to get
>> the desired effect.
>>
>> Detected by Andrew Morton who has confirmed that this patch
>> fixes the warning on i386/gcc-4.4.3, i386/gcc-4.4.0 and arm/gcc-4.4.4.
>>
>> Signed-off-by: Sowmini Varadhan <[email protected]>
>> ---
>> lib/iommu-common.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/lib/iommu-common.c b/lib/iommu-common.c
>> index df30632..fd1297d 100644
>> --- a/lib/iommu-common.c
>> +++ b/lib/iommu-common.c
>> @@ -119,7 +119,7 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
>> unsigned long align_mask = 0;
>>
>> if (align_order > 0)
>> - align_mask = 0xffffffffffffffffl >> (64 - align_order);
>> + align_mask = ~0l >> (64 - align_order);
>>
> Wonder if this just hides the real problem. Unless align_order
> is very large, the resulting mask on 32 bit systems may be 0.
> Is this really the idea ?
Probably not, but that's not what would happen on x86: the shift
only depends on the lower 5 or 6 bits - I don't know if other platforms
also has that behaviour. So for align_order == 2 and x86_32 we'd
effectively end up with a shift of 62 & 31 == 30 (though technically
undefined behaviour), and the desired mask of 0x3.
Wouldn't GENMASK(align_order-1, 0) work for all cases (assuming
align_order has a sane value)?
Rasmus
On (07/19/15 22:25), Rasmus Villemoes wrote:
>
> Wouldn't GENMASK(align_order-1, 0) work for all cases (assuming
> align_order has a sane value)?
Devices with limits on DMA masks are uncommon, so I'm personally
not an expert at all the variations in this space, but I was thinking
that this doing
align_mask = 0xffffffffffffffffull >> (64 - align_order);
would be the compact answer for both 32 and 64 bit cases here?
--Sowmini
On 07/19/2015 01:25 PM, Rasmus Villemoes wrote:
> On Sun, Jul 19 2015, Guenter Roeck <[email protected]> wrote:
>
>> On Sun, Jul 19, 2015 at 02:20:14PM +0200, Sowmini Varadhan wrote:
>>>
>>> Using a 64 bit constant generates "warning: integer constant is too
>>> large for 'long' type" on 32 bit platforms. Instead use ~0l to get
>>> the desired effect.
>>>
>>> Detected by Andrew Morton who has confirmed that this patch
>>> fixes the warning on i386/gcc-4.4.3, i386/gcc-4.4.0 and arm/gcc-4.4.4.
>>>
>>> Signed-off-by: Sowmini Varadhan <[email protected]>
>>> ---
>>> lib/iommu-common.c | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/lib/iommu-common.c b/lib/iommu-common.c
>>> index df30632..fd1297d 100644
>>> --- a/lib/iommu-common.c
>>> +++ b/lib/iommu-common.c
>>> @@ -119,7 +119,7 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
>>> unsigned long align_mask = 0;
>>>
>>> if (align_order > 0)
>>> - align_mask = 0xffffffffffffffffl >> (64 - align_order);
>>> + align_mask = ~0l >> (64 - align_order);
>>>
>> Wonder if this just hides the real problem. Unless align_order
>> is very large, the resulting mask on 32 bit systems may be 0.
>> Is this really the idea ?
>
> Probably not, but that's not what would happen on x86: the shift
> only depends on the lower 5 or 6 bits - I don't know if other platforms
> also has that behaviour. So for align_order == 2 and x86_32 we'd
> effectively end up with a shift of 62 & 31 == 30 (though technically
> undefined behaviour), and the desired mask of 0x3.
>
#include <stdio.h>
main(int argc, char **argv)
{
unsigned long am1, am2, am3;
int align_order = 2;
am1 = 0xffffffffffffffffl >> (64 - align_order);
am2 = ~0l >> (64 - align_order);
am3 = ~0ul >> (64 - align_order);
printf("0x%lx 0x%lx 0x%lx\n", am1, am2, am3);
}
yields an output of
0x3 0xffffffffffffffff 0x3
So either case ~0l appears to be wrong; it should be ~0ul.
I don't know if ~0ull makes a difference for some architectures.
Guenter
On (07/19/15 08:27), Guenter Roeck wrote:
> > - align_mask = 0xffffffffffffffffl >> (64 - align_order);
> > + align_mask = ~0l >> (64 - align_order);
> >
> Wonder if this just hides the real problem. Unless align_order
> is very large, the resulting mask on 32 bit systems may be 0.
> Is this really the idea ?
<subsequent example code deleted>
> So either case ~0l appears to be wrong; it should be ~0ul.
> I don't know if ~0ull makes a difference for some architectures.
I agree about the unsigned part. However, regarding the arch specific
twists..
I checked into this.. even though I have a test program on
x86_64 that "does the right thing" for both of
align_mask = ~0ul >> (64 - align_order);
align_mask = ~0ul >> (BITS_PER_LONG - align_order);
when I compiled with -m32 and without (I tried align_order == 1 and 31
for edge cases), I think there are some gcc/arch specific variations
possible based on undefined behavior, so that the second variant
is safer.
I'll send out a patch with that version soon.
--Sowmini
On Mon, Jul 20 2015, Guenter Roeck <[email protected]> wrote:
> So either case ~0l appears to be wrong; it should be ~0ul.
Yes, right-shifting -1 of any type is probably always wrong, as it will
always give -1 again. Probably one should add a smatch/sparse warning
for that.
> I don't know if ~0ull makes a difference for some architectures.
I highly doubt it. The result is truncated to unsigned long
anyway. Assuming align_order always has a value between 0 and
BITS_PER_LONG, GENMASK should be exactly what is wanted.
Rasmus
On (07/20/15 19:57), Rasmus Villemoes wrote:
> I highly doubt it. The result is truncated to unsigned long
> anyway. Assuming align_order always has a value between 0 and
> BITS_PER_LONG, GENMASK should be exactly what is wanted.
While GENMASK may do the job, the code is already quite obscure,
so I'm going to stick with the minimal delta to get this right,
namely
- align_mask = 0xffffffffffffffffl >> (64 - align_order);
+ align_mask = ~0ul >> (BITS_PER_LONG - align_order);
--Sowmini
On Mon, 20 Jul 2015 19:57:18 +0200 Rasmus Villemoes <[email protected]> wrote:
> On Mon, Jul 20 2015, Guenter Roeck <[email protected]> wrote:
>
> > So either case ~0l appears to be wrong; it should be ~0ul.
>
> Yes, right-shifting -1 of any type is probably always wrong, as it will
> always give -1 again.
Not for unsigned types.
The kernel uses "-1UL" and "-1ULL" quite a lot - it's a convenient way
of saying "all ones, regardless of size". Also, assigning plain old
"-1" to an unsigned variable will make that variable all-ones
regardless of size.
In this case I expect we could do
align_mask = -1UL >> (64 - align_order);
but I don't know about that 64. Maybe it should be BITS_PER_LONG?
On 07/20/2015 04:28 PM, Andrew Morton wrote:
> On Mon, 20 Jul 2015 19:57:18 +0200 Rasmus Villemoes <[email protected]> wrote:
>
>> On Mon, Jul 20 2015, Guenter Roeck <[email protected]> wrote:
>>
>>> So either case ~0l appears to be wrong; it should be ~0ul.
>>
>> Yes, right-shifting -1 of any type is probably always wrong, as it will
>> always give -1 again.
>
> Not for unsigned types.
>
> The kernel uses "-1UL" and "-1ULL" quite a lot - it's a convenient way
> of saying "all ones, regardless of size". Also, assigning plain old
> "-1" to an unsigned variable will make that variable all-ones
> regardless of size.
>
> In this case I expect we could do
>
> align_mask = -1UL >> (64 - align_order);
>
-1ul works, at least on x86 (32 and 64 bit).
> but I don't know about that 64. Maybe it should be BITS_PER_LONG?
>
I think that is going to be in the next version of the patch.
Guenter