The b43 driver generates the following sparse warnings:
CHECK drivers/net/wireless/b43/phy_g.c
drivers/net/wireless/b43/phy_g.c:974:35: warning: cast truncates bits from constant value (ffff7fff becomes 7fff)
CHECK drivers/net/wireless/b43/wa.c
drivers/net/wireless/b43/wa.c:385:53: warning: cast truncates bits from constant value (ffff00ff becomes ff)
drivers/net/wireless/b43/wa.c:403:48: warning: cast truncates bits from constant value (ffff00ff becomes ff)
drivers/net/wireless/b43/wa.c:405:48: warning: cast truncates bits from constant value (ffff00ff becomes ff)
drivers/net/wireless/b43/wa.c:415:50: warning: cast truncates bits from constant value (ffff0fff becomes fff)
Signed-off-by: Larry Finger <[email protected]>
---
John,
There is no hurry for this material.
Larry
---
Index: wireless-testing/drivers/net/wireless/b43/phy_g.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/b43/phy_g.c
+++ wireless-testing/drivers/net/wireless/b43/phy_g.c
@@ -971,7 +971,7 @@ b43_radio_interference_mitigation_enable
b43_phy_maskset(dev, 0x04A2, 0xFFF0, 0x000B);
if (phy->rev >= 3) {
- b43_phy_mask(dev, 0x048A, (u16)~0x8000);
+ b43_phy_mask(dev, 0x048A, (u16)(~0x8000 & 0xFFFF));
b43_phy_maskset(dev, 0x0415, 0x8000, 0x36D8);
b43_phy_maskset(dev, 0x0416, 0x8000, 0x36D8);
b43_phy_maskset(dev, 0x0417, 0xFE00, 0x016D);
Index: wireless-testing/drivers/net/wireless/b43/wa.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/b43/wa.c
+++ wireless-testing/drivers/net/wireless/b43/wa.c
@@ -382,7 +382,8 @@ static void b43_wa_altagc(struct b43_wld
b43_ofdmtab_write16(dev, B43_OFDMTAB_AGC1, 3, 25);
}
- b43_phy_maskset(dev, B43_PHY_CCKSHIFTBITS_WA, (u16)~0xFF00, 0x5700);
+ b43_phy_maskset(dev, B43_PHY_CCKSHIFTBITS_WA, (u16)(~0xFF00 & 0xFFFF),
+ 0x5700);
b43_phy_maskset(dev, B43_PHY_OFDM(0x1A), ~0x007F, 0x000F);
b43_phy_maskset(dev, B43_PHY_OFDM(0x1A), ~0x3F80, 0x2B80);
b43_phy_maskset(dev, B43_PHY_ANTWRSETT, 0xF0FF, 0x0300);
@@ -400,9 +401,9 @@ static void b43_wa_altagc(struct b43_wld
b43_phy_maskset(dev, B43_PHY_OFDM(0x89), ~0x00FF, 0x0020);
b43_phy_maskset(dev, B43_PHY_OFDM(0x89), ~0x3F00, 0x0200);
b43_phy_maskset(dev, B43_PHY_OFDM(0x82), ~0x00FF, 0x002E);
- b43_phy_maskset(dev, B43_PHY_OFDM(0x96), (u16)~0xFF00, 0x1A00);
+ b43_phy_maskset(dev, B43_PHY_OFDM(0x96), (u16)(~0xFF00 & 0xFFFF), 0x1A00);
b43_phy_maskset(dev, B43_PHY_OFDM(0x81), ~0x00FF, 0x0028);
- b43_phy_maskset(dev, B43_PHY_OFDM(0x81), (u16)~0xFF00, 0x2C00);
+ b43_phy_maskset(dev, B43_PHY_OFDM(0x81), (u16)(~0xFF00 & 0xFFFF), 0x2C00);
if (phy->rev == 1) {
b43_phy_write(dev, B43_PHY_PEAK_COUNT, 0x092B);
b43_phy_maskset(dev, B43_PHY_OFDM(0x1B), ~0x001E, 0x0002);
@@ -412,7 +413,8 @@ static void b43_wa_altagc(struct b43_wld
b43_phy_maskset(dev, B43_PHY_LPFGAINCTL, ~0x000F, 0x0004);
if (phy->rev >= 6) {
b43_phy_write(dev, B43_PHY_OFDM(0x22), 0x287A);
- b43_phy_maskset(dev, B43_PHY_LPFGAINCTL, (u16)~0xF000, 0x3000);
+ b43_phy_maskset(dev, B43_PHY_LPFGAINCTL, (u16)(~0xF000 &
+ 0xFFFF), 0x3000);
}
}
b43_phy_maskset(dev, B43_PHY_DIVSRCHIDX, 0x8080, 0x7874);
On Fri, 2009-08-14 at 22:15 +0200, Michael Buesch wrote:
> > - b43_phy_mask(dev, 0x048A, (u16)~0x8000);
> > + b43_phy_mask(dev, 0x048A, (u16)(~0x8000 & 0xFFFF));
>
> Uh come on...
> The u16 cast already is stupid as hell, but this is becoming braindead.
> The code is perfectly fine. Sparse should instead provide an option to disable
> this fragile check.
There are cases where we want to know that a constant was truncated. In
fact, in most cases it's a useful warning.
In this case, we intuitively expect that it's OK to cast a result of a
bitwise operation to its original type, as if it was never promoted.
But sparse will need to have a special exception for this case.
I would just use 0x7fff here.
--
Regards,
Pavel Roskin
On Monday 17 August 2009 22:30:41 Pavel Roskin wrote:
> On Sat, 2009-08-15 at 12:04 +0200, Michael Buesch wrote:
>
> > I still do not understand why it does complain about an _explicit_ truncation.
> > That's simply stupid. If I program an explicit truncation I _do_ mean to truncate the value.
>
> Actually, it's a bug in sparse. Sparse acts inconsistently.
>
> This causes a warning:
>
> void mask(unsigned short mask);
> static void test(void)
> {
> mask((unsigned short)0xffff0000);
> }
>
> test.c:4:30: warning: cast truncates bits from constant value (ffff0000
> becomes 0)
>
> But this is OK:
>
> void mask(unsigned short mask);
> static void test(void)
> {
> mask((unsigned short)0xfffff000);
> }
>
> Moreover, this is OK, even though the cast changes the value of the
> constant:
>
> void mask(unsigned short mask);
> static void test(void)
> {
> mask((unsigned short)0xfffff000U);
> }
>
> I suggest that we take no action until sparse is fixed. I'm going to
> report the issue to the sparse mailing list now.
>
Cool, thanks a lot for tracking this down. :)
--
Greetings, Michael.
On Fri, Aug 14, 2009 at 11:00 PM, Michael Buesch<[email protected]> wrote:
> On Friday 14 August 2009 22:52:13 Pavel Roskin wrote:
>> On Fri, 2009-08-14 at 22:15 +0200, Michael Buesch wrote:
>>
>> > > - ? ? ? ? ? ? ? ? b43_phy_mask(dev, 0x048A, (u16)~0x8000);
>> > > + ? ? ? ? ? ? ? ? b43_phy_mask(dev, 0x048A, (u16)(~0x8000 & 0xFFFF));
>> >
>
>> I would just use 0x7fff here.
>
> That does not work if 0x8000 is a #defined bit.
What about ~((u16)0x8000)? (Or maybe ~(u16)0x8000 is enough, without
the extra parentheses.)
>
> --
> Greetings, Michael.
> _______________________________________________
> Bcm43xx-dev mailing list
> [email protected]
> https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
>
--
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)
On Friday 14 August 2009 22:52:13 Pavel Roskin wrote:
> On Fri, 2009-08-14 at 22:15 +0200, Michael Buesch wrote:
>
> > > - b43_phy_mask(dev, 0x048A, (u16)~0x8000);
> > > + b43_phy_mask(dev, 0x048A, (u16)(~0x8000 & 0xFFFF));
> >
> I would just use 0x7fff here.
That does not work if 0x8000 is a #defined bit.
--
Greetings, Michael.
On Fri, 2009-08-14 at 17:29 -0400, Pavel Roskin wrote:
> On Fri, 2009-08-14 at 23:00 +0200, Michael Buesch wrote:
> > On Friday 14 August 2009 22:52:13 Pavel Roskin wrote:
> > > On Fri, 2009-08-14 at 22:15 +0200, Michael Buesch wrote:
> > >
> > > > > - b43_phy_mask(dev, 0x048A, (u16)~0x8000);
> > > > > + b43_phy_mask(dev, 0x048A, (u16)(~0x8000 & 0xFFFF));
> > > >
> >
> > > I would just use 0x7fff here.
> >
> > That does not work if 0x8000 is a #defined bit.
>
> One approach would be to use a macro and tell sparse to ignore it
>
> #define NEGATE(x) (__force typeof(x))(~x)
Scratch that. It has no change to work for constants unless we hardcode
the size, e.g. by having NEGATE16, NEGATE32 etc.
The best I could do is:
#define NEGATE16(x) (0xFFFF & ~x)
b43_phy_mask(dev, 0x048A, NEGATE16(0x8000));
> Another approach would be to have b43_phy_mask() and similar functions
> accept int and do the bit cutting in one place.
I tend to think that it would be the best approach.
> It should also be possible to have separate functions e.g.
> b43_phy_unmask() that would do the negation, so that the caller won't
> need to do it.
This could make the code hard to read.
--
Regards,
Pavel Roskin
On Friday 14 August 2009 00:15:07 Larry Finger wrote:
> The b43 driver generates the following sparse warnings:
>
> CHECK drivers/net/wireless/b43/phy_g.c
> drivers/net/wireless/b43/phy_g.c:974:35: warning: cast truncates bits from constant value (ffff7fff becomes 7fff)
> CHECK drivers/net/wireless/b43/wa.c
> drivers/net/wireless/b43/wa.c:385:53: warning: cast truncates bits from constant value (ffff00ff becomes ff)
> drivers/net/wireless/b43/wa.c:403:48: warning: cast truncates bits from constant value (ffff00ff becomes ff)
> drivers/net/wireless/b43/wa.c:405:48: warning: cast truncates bits from constant value (ffff00ff becomes ff)
> drivers/net/wireless/b43/wa.c:415:50: warning: cast truncates bits from constant value (ffff0fff becomes fff)
>
> Signed-off-by: Larry Finger <[email protected]>
> ---
>
> John,
>
> There is no hurry for this material.
>
> Larry
> ---
>
> Index: wireless-testing/drivers/net/wireless/b43/phy_g.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/b43/phy_g.c
> +++ wireless-testing/drivers/net/wireless/b43/phy_g.c
> @@ -971,7 +971,7 @@ b43_radio_interference_mitigation_enable
> b43_phy_maskset(dev, 0x04A2, 0xFFF0, 0x000B);
>
> if (phy->rev >= 3) {
> - b43_phy_mask(dev, 0x048A, (u16)~0x8000);
> + b43_phy_mask(dev, 0x048A, (u16)(~0x8000 & 0xFFFF));
Uh come on...
The u16 cast already is stupid as hell, but this is becoming braindead.
The code is perfectly fine. Sparse should instead provide an option to disable
this fragile check.
--
Greetings, Michael.
Michael Buesch wrote:
> On Friday 14 August 2009 00:15:07 Larry Finger wrote:
>> The b43 driver generates the following sparse warnings:
>>
>> CHECK drivers/net/wireless/b43/phy_g.c
>> drivers/net/wireless/b43/phy_g.c:974:35: warning: cast truncates bits from constant value (ffff7fff becomes 7fff)
>> CHECK drivers/net/wireless/b43/wa.c
>> drivers/net/wireless/b43/wa.c:385:53: warning: cast truncates bits from constant value (ffff00ff becomes ff)
>> drivers/net/wireless/b43/wa.c:403:48: warning: cast truncates bits from constant value (ffff00ff becomes ff)
>> drivers/net/wireless/b43/wa.c:405:48: warning: cast truncates bits from constant value (ffff00ff becomes ff)
>> drivers/net/wireless/b43/wa.c:415:50: warning: cast truncates bits from constant value (ffff0fff becomes fff)
>>
>> Signed-off-by: Larry Finger <[email protected]>
>> ---
>>
>> John,
>>
>> There is no hurry for this material.
>>
>> Larry
>> ---
>>
>> Index: wireless-testing/drivers/net/wireless/b43/phy_g.c
>> ===================================================================
>> --- wireless-testing.orig/drivers/net/wireless/b43/phy_g.c
>> +++ wireless-testing/drivers/net/wireless/b43/phy_g.c
>> @@ -971,7 +971,7 @@ b43_radio_interference_mitigation_enable
>> b43_phy_maskset(dev, 0x04A2, 0xFFF0, 0x000B);
>>
>> if (phy->rev >= 3) {
>> - b43_phy_mask(dev, 0x048A, (u16)~0x8000);
>> + b43_phy_mask(dev, 0x048A, (u16)(~0x8000 & 0xFFFF));
>
> Uh come on...
> The u16 cast already is stupid as hell, but this is becoming braindead.
> The code is perfectly fine. Sparse should instead provide an option to disable
> this fragile check.
John,
Just drop this patch. The code is OK as is.
Larry
On Sat, 2009-08-15 at 12:04 +0200, Michael Buesch wrote:
> I still do not understand why it does complain about an _explicit_ truncation.
> That's simply stupid. If I program an explicit truncation I _do_ mean to truncate the value.
Actually, it's a bug in sparse. Sparse acts inconsistently.
This causes a warning:
void mask(unsigned short mask);
static void test(void)
{
mask((unsigned short)0xffff0000);
}
test.c:4:30: warning: cast truncates bits from constant value (ffff0000
becomes 0)
But this is OK:
void mask(unsigned short mask);
static void test(void)
{
mask((unsigned short)0xfffff000);
}
Moreover, this is OK, even though the cast changes the value of the
constant:
void mask(unsigned short mask);
static void test(void)
{
mask((unsigned short)0xfffff000U);
}
I suggest that we take no action until sparse is fixed. I'm going to
report the issue to the sparse mailing list now.
--
Regards,
Pavel Roskin
On Fri, 2009-08-14 at 23:00 +0200, Michael Buesch wrote:
> On Friday 14 August 2009 22:52:13 Pavel Roskin wrote:
> > On Fri, 2009-08-14 at 22:15 +0200, Michael Buesch wrote:
> >
> > > > - b43_phy_mask(dev, 0x048A, (u16)~0x8000);
> > > > + b43_phy_mask(dev, 0x048A, (u16)(~0x8000 & 0xFFFF));
> > >
>
> > I would just use 0x7fff here.
>
> That does not work if 0x8000 is a #defined bit.
One approach would be to use a macro and tell sparse to ignore it
#define NEGATE(x) (__force typeof(x))(~x)
Another approach would be to have b43_phy_mask() and similar functions
accept int and do the bit cutting in one place.
It should also be possible to have separate functions e.g.
b43_phy_unmask() that would do the negation, so that the caller won't
need to do it.
--
Regards,
Pavel Roskin
On Fri, 2009-08-14 at 23:04 +0200, G?bor Stefanik wrote:
> On Fri, Aug 14, 2009 at 11:00 PM, Michael Buesch<[email protected]> wrote:
> > On Friday 14 August 2009 22:52:13 Pavel Roskin wrote:
> >> On Fri, 2009-08-14 at 22:15 +0200, Michael Buesch wrote:
> >>
> >> > > - b43_phy_mask(dev, 0x048A, (u16)~0x8000);
> >> > > + b43_phy_mask(dev, 0x048A, (u16)(~0x8000 & 0xFFFF));
> >> >
> >
> >> I would just use 0x7fff here.
> >
> > That does not work if 0x8000 is a #defined bit.
>
> What about ~((u16)0x8000)?
phy_g.c:974: warning: large integer implicitly truncated to unsigned
type
> (Or maybe ~(u16)0x8000 is enough, without
> the extra parentheses.)
Same thing. Sparse complains whether the cast is explicit or implicit.
--
Regards,
Pavel Roskin
On Friday 14 August 2009 23:46:54 Pavel Roskin wrote:
> On Fri, 2009-08-14 at 17:29 -0400, Pavel Roskin wrote:
> > On Fri, 2009-08-14 at 23:00 +0200, Michael Buesch wrote:
> > > On Friday 14 August 2009 22:52:13 Pavel Roskin wrote:
> > > > On Fri, 2009-08-14 at 22:15 +0200, Michael Buesch wrote:
> > > >
> > > > > > - b43_phy_mask(dev, 0x048A, (u16)~0x8000);
> > > > > > + b43_phy_mask(dev, 0x048A, (u16)(~0x8000 & 0xFFFF));
> > > > >
> > >
> > > > I would just use 0x7fff here.
> > >
> > > That does not work if 0x8000 is a #defined bit.
> >
> > One approach would be to use a macro and tell sparse to ignore it
> >
> > #define NEGATE(x) (__force typeof(x))(~x)
>
> Scratch that. It has no change to work for constants unless we hardcode
> the size, e.g. by having NEGATE16, NEGATE32 etc.
>
> The best I could do is:
>
> #define NEGATE16(x) (0xFFFF & ~x)
> b43_phy_mask(dev, 0x048A, NEGATE16(0x8000));
I think the real question is whether this does really prevent any bugs or whether
it just introduces new possibilities for bugs.
void my_func(u32 x);
my_func(NEGATE16(0x8000));
--
Greetings, Michael.
On Friday 14 August 2009 23:35:29 Pavel Roskin wrote:
> On Fri, 2009-08-14 at 23:04 +0200, G?bor Stefanik wrote:
> > On Fri, Aug 14, 2009 at 11:00 PM, Michael Buesch<[email protected]> wrote:
> > > On Friday 14 August 2009 22:52:13 Pavel Roskin wrote:
> > >> On Fri, 2009-08-14 at 22:15 +0200, Michael Buesch wrote:
> > >>
> > >> > > - b43_phy_mask(dev, 0x048A, (u16)~0x8000);
> > >> > > + b43_phy_mask(dev, 0x048A, (u16)(~0x8000 & 0xFFFF));
> > >> >
> > >
> > >> I would just use 0x7fff here.
> > >
> > > That does not work if 0x8000 is a #defined bit.
> >
> > What about ~((u16)0x8000)?
>
> phy_g.c:974: warning: large integer implicitly truncated to unsigned
> type
>
> > (Or maybe ~(u16)0x8000 is enough, without
> > the extra parentheses.)
>
> Same thing. Sparse complains whether the cast is explicit or implicit.
>
I still do not understand why it does complain about an _explicit_ truncation.
That's simply stupid. If I program an explicit truncation I _do_ mean to truncate the value.
--
Greetings, Michael.