[RFC/RFT] p54: Fix sparse warnings

[RFC/RFT] p54: Fix sparse warnings

[Larry Finger]

When this patch is ready, I will use the commit message below.

Question: Does anyone use either p54pci or p54usb on a big-endian machine?

Thanks,

Larry

=====================================================================

The command

make C=2 CF="-D__CHECK_ENDIAN__" drivers/net/wireless/p54/

generates the following warnings:

.../p54common.c:152:38: warning: incorrect type in argument 1 (different base types)
.../p54common.c:152:38:    expected restricted __be32 const [usertype] *p
.../p54common.c:152:38:    got unsigned int *<noident>
.../p54common.c:184:15: warning: restricted __le32 degrades to integer
.../p54common.c:185:29: warning: cast to restricted __le16
.../p54common.c:309:11: warning: symbol 'p54_rf_chips' was not declared.
		        Should it be static?
.../p54common.c:313:5: warning: symbol 'p54_parse_eeprom' was not declared.
		       Should it be static?
.../p54common.c:620:43: warning: incorrect type in argument 3 (different base types)
.../p54common.c:620:43:    expected unsigned long [unsigned] [usertype] len
.../p54common.c:620:43:    got restricted __le16 [usertype] len
.../p54common.c:780:41: warning: restricted __le16 degrades to integer
.../p54common.c:781:32: warning: restricted __le16 degrades to integer
.../p54common.c:1250:28: warning: incorrect type in argument 2 (different base types)
.../p54common.c:1250:28:    expected unsigned short [unsigned] [usertype] filter_type
.../p54common.c:1250:28:    got restricted __le16 [usertype] filter_type
.../p54common.c:1252:28: warning: incorrect type in argument 2 (different base types)
.../p54common.c:1252:28:    expected unsigned short [unsigned] [usertype] filter_type
.../p54common.c:1252:28:    got restricted __le16 [usertype] filter_type
.../p54common.c:1257:42: warning: incorrect type in argument 2 (different base types)
.../p54common.c:1257:42:    expected unsigned short [unsigned] [usertype] filter_type
.../p54common.c:1257:42:    got restricted __le16
.../p54common.c:1260:42: warning: incorrect type in argument 2 (different base types)
.../p54common.c:1260:42:    expected unsigned short [unsigned] [usertype] filter_type
.../p54common.c:1260:42:    got restricted __le16
.../p54usb.c:228:10: warning: restricted __le32 degrades to integer
.../p54usb.c:228:23: warning: restricted __le32 degrades to integer
.../p54usb.c:228:7: warning: incorrect type in assignment (different base types)
.../p54usb.c:228:7:    expected restricted __le32 [assigned] [usertype] chk
.../p54usb.c:228:7:    got unsigned int
.../p54usb.c:221:8: warning: symbol 'p54u_lm87_chksum' was not declared.
		    Should it be static?

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

Index: wireless-testing/drivers/net/wireless/p54/p54usb.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/p54/p54usb.c
+++ wireless-testing/drivers/net/wireless/p54/p54usb.c
@@ -218,17 +218,17 @@ static void p54u_tx_3887(struct ieee8021
 	usb_submit_urb(data_urb, GFP_ATOMIC);
 }
 
-__le32 p54u_lm87_chksum(const u32 *data, size_t length)
+static __le32 p54u_lm87_chksum(const u32 *data, size_t length)
 {
-	__le32 chk = 0;
+	u32 chk = 0;
 
 	length >>= 2;
 	while (length--) {
-		chk ^= cpu_to_le32(*data++);
+		chk ^= *data++;
 		chk = (chk >> 5) ^ (chk << 3);
 	}
 
-	return chk;
+	return cpu_to_le32(chk);
 }
 
 static void p54u_tx_lm87(struct ieee80211_hw *dev,
Index: wireless-testing/drivers/net/wireless/p54/p54common.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/p54/p54common.c
+++ wireless-testing/drivers/net/wireless/p54/p54common.c
@@ -149,7 +149,7 @@ int p54_parse_firmware(struct ieee80211_
 		u32 code = le32_to_cpu(bootrec->code);
 		switch (code) {
 		case BR_CODE_COMPONENT_ID:
-			priv->fw_interface = be32_to_cpup(bootrec->data);
+			priv->fw_interface = be32_to_cpup((const __be32 *)bootrec->data);
 			switch (priv->fw_interface) {
 			case FW_FMAC:
 				printk(KERN_INFO "p54: FreeMAC firmware\n");
@@ -181,9 +181,8 @@ int p54_parse_firmware(struct ieee80211_
 			priv->rx_end = le32_to_cpu(desc->rx_end) - 0x3500;
 			priv->headroom = desc->headroom;
 			priv->tailroom = desc->tailroom;
-			if (bootrec->len == 11)
-				priv->rx_mtu = (size_t) le16_to_cpu(
-					(__le16)bootrec->data[10]);
+			if (le32_to_cpu(bootrec->len) == 11)
+				priv->rx_mtu = le16_to_cpu(bootrec->data16[20]);
 			else
 				priv->rx_mtu = (size_t)
 					0x620 - priv->tx_hdr_len;
@@ -306,11 +305,11 @@ static int p54_convert_rev1(struct ieee8
 	return 0;
 }
 
-const char* p54_rf_chips[] = { "NULL", "Indigo?", "Duette",
+static const char *p54_rf_chips[] = { "NULL", "Indigo?", "Duette",
                               "Frisbee", "Xbow", "Longbow" };
 static int p54_init_xbow_synth(struct ieee80211_hw *dev);
 
-int p54_parse_eeprom(struct ieee80211_hw *dev, void *eeprom, int len)
+static int p54_parse_eeprom(struct ieee80211_hw *dev, void *eeprom, int len)
 {
 	struct p54_common *priv = dev->priv;
 	struct eeprom_pda_wrap *wrap = NULL;
@@ -617,7 +616,7 @@ static void p54_rx_eeprom_readback(struc
 	if (!priv->eeprom)
 		return ;
 
-	memcpy(priv->eeprom, eeprom->data, eeprom->len);
+	memcpy(priv->eeprom, eeprom->data, le16_to_cpu(eeprom->len));
 
 	complete(&priv->eeprom_comp);
 }
@@ -777,8 +776,9 @@ int p54_read_eeprom(struct ieee80211_hw
 		hdr->len = cpu_to_le16(blocksize + sizeof(*eeprom_hdr));
 		eeprom_hdr->offset = cpu_to_le16(offset);
 		eeprom_hdr->len = cpu_to_le16(blocksize);
-		p54_assign_address(dev, NULL, hdr, hdr->len + sizeof(*hdr));
-	        priv->tx(dev, hdr, hdr->len + sizeof(*hdr), 0);
+		p54_assign_address(dev, NULL, hdr, le16_to_cpu(hdr->len) +
+				   sizeof(*hdr));
+		priv->tx(dev, hdr, le16_to_cpu(hdr->len) + sizeof(*hdr), 0);
 
 		if (!wait_for_completion_interruptible_timeout(&priv->eeprom_comp, HZ)) {
 			printk(KERN_ERR "%s: device does not respond!\n",
@@ -1247,18 +1247,20 @@ static void p54_configure_filter(struct
 
 	if (changed_flags & FIF_BCN_PRBRESP_PROMISC) {
 		if (*total_flags & FIF_BCN_PRBRESP_PROMISC)
-			p54_set_filter(dev, priv->filter_type, NULL);
+			p54_set_filter(dev, le16_to_cpu(priv->filter_type),
+				 NULL);
 		else
-			p54_set_filter(dev, priv->filter_type, priv->bssid);
+			p54_set_filter(dev, le16_to_cpu(priv->filter_type),
+				 priv->bssid);
 	}
 
 	if (changed_flags & FIF_PROMISC_IN_BSS) {
 		if (*total_flags & FIF_PROMISC_IN_BSS)
-			p54_set_filter(dev, priv->filter_type |
-				cpu_to_le16(0x8), NULL);
+			p54_set_filter(dev, le16_to_cpu(priv->filter_type) |
+				0x8, NULL);
 		else
-			p54_set_filter(dev, priv->filter_type &
-				~cpu_to_le16(0x8), priv->bssid);
+			p54_set_filter(dev, le16_to_cpu(priv->filter_type) &
+				~0x8, priv->bssid);
 	}
 }
 
Index: wireless-testing/drivers/net/wireless/p54/p54common.h
===================================================================
--- wireless-testing.orig/drivers/net/wireless/p54/p54common.h
+++ wireless-testing/drivers/net/wireless/p54/p54common.h
@@ -18,7 +18,13 @@
 struct bootrec {
 	__le32 code;
 	__le32 len;
-	u32 data[0];
+	/* Most references to the data section that follows are for u32
+	 * quantities; however, one is for an le16 quantity. The union
+	 * below avoids a cast and makes the usage clearer. */
+	union {
+		u32 data[0];
+		__le16 data16[0];
+	};
 } __attribute__((packed));
 
 struct bootrec_exp_if {

Re: [RFC/RFT] p54: Fix sparse warnings

[Michael Buesch]

On Friday 12 September 2008 20:16:03 Larry Finger wrote:
>  struct bootrec {
>  	__le32 code;
>  	__le32 len;
> -	u32 data[0];
> +	/* Most references to the data section that follows are for u32
> +	 * quantities; however, one is for an le16 quantity. The union
> +	 * below avoids a cast and makes the usage clearer. */
> +	union {
> +		u32 data[0];
> +		__le16 data16[0];
> +	};
>  } __attribute__((packed));

I suggest you change
	u32 data[0];
to
	__le32 data_le32[0];
and also add
	__be32 data_be32[0];
to the union. This avoids another few casts.
(Maybe you don't even need the __le32 variant. I didn't check all the code).

-- 
Greetings Michael.

Re: [RFC/RFT] p54: Fix sparse warnings

[Michael Buesch]

On Friday 12 September 2008 20:36:03 Michael Buesch wrote:
> On Friday 12 September 2008 20:16:03 Larry Finger wrote:
> >  struct bootrec {
> >  	__le32 code;
> >  	__le32 len;
> > -	u32 data[0];
> > +	/* Most references to the data section that follows are for u32
> > +	 * quantities; however, one is for an le16 quantity. The union
> > +	 * below avoids a cast and makes the usage clearer. */
> > +	union {
> > +		u32 data[0];
> > +		__le16 data16[0];
> > +	};
> >  } __attribute__((packed));
> 
> I suggest you change
> 	u32 data[0];
> to
> 	__le32 data_le32[0];
> and also add
> 	__be32 data_be32[0];
> to the union. This avoids another few casts.
> (Maybe you don't even need the __le32 variant. I didn't check all the code).

Ah and I think you should also add packed attribute to the union
to tell the compiler to avoid any padding.

-- 
Greetings Michael.

Re: [RFC/RFT] p54: Fix sparse warnings

[Larry Finger]

Michael Buesch wrote:
> On Friday 12 September 2008 20:16:03 Larry Finger wrote:
>>  struct bootrec {
>>  	__le32 code;
>>  	__le32 len;
>> -	u32 data[0];
>> +	/* Most references to the data section that follows are for u32
>> +	 * quantities; however, one is for an le16 quantity. The union
>> +	 * below avoids a cast and makes the usage clearer. */
>> +	union {
>> +		u32 data[0];
>> +		__le16 data16[0];
>> +	};
>>  } __attribute__((packed));
> 
> I suggest you change
> 	u32 data[0];
> to
> 	__le32 data_le32[0];
> and also add
> 	__be32 data_be32[0];
> to the union. This avoids another few casts.
> (Maybe you don't even need the __le32 variant. I didn't check all the code).

No, it won't avoid any casts. The program uses the data area 7 times 
in native-cpu order, once as be32, and once in little-endian order 
(the one that was fixed by the union).

I will add the packed attribute as suggested in your other email.

Thanks for the review,

Larry

Re: [RFC/RFT] p54: Fix sparse warnings

[Michael Buesch]

On Friday 12 September 2008 20:52:52 Larry Finger wrote:
> Michael Buesch wrote:
> > On Friday 12 September 2008 20:16:03 Larry Finger wrote:
> >>  struct bootrec {
> >>  	__le32 code;
> >>  	__le32 len;
> >> -	u32 data[0];
> >> +	/* Most references to the data section that follows are for u32
> >> +	 * quantities; however, one is for an le16 quantity. The union
> >> +	 * below avoids a cast and makes the usage clearer. */
> >> +	union {
> >> +		u32 data[0];
> >> +		__le16 data16[0];
> >> +	};
> >>  } __attribute__((packed));
> > 
> > I suggest you change
> > 	u32 data[0];
> > to
> > 	__le32 data_le32[0];
> > and also add
> > 	__be32 data_be32[0];
> > to the union. This avoids another few casts.
> > (Maybe you don't even need the __le32 variant. I didn't check all the code).
> 
> No, it won't avoid any casts. The program uses the data area 7 times 
> in native-cpu order, once as be32, and once in little-endian order 

Is the native use correct? Smells fishy.

-- 
Greetings Michael.

Re: [RFC/RFT] p54: Fix sparse warnings

[Larry Finger]

Michael Buesch wrote:
> On Friday 12 September 2008 20:52:52 Larry Finger wrote:
>> No, it won't avoid any casts. The program uses the data area 7 times 
>> in native-cpu order, once as be32, and once in little-endian order 
> 
> Is the native use correct? Smells fishy.

It does to me as well. I just got a copy of Intersil's data sheet for
the MAC and everything looks little endian, but until we get a tester
with a big-endian machine, we won't know. So far, no volunteers have
come forward.

Larry

Re: [RFC/RFT] p54: Fix sparse warnings

[Pavel Roskin]

On Sat, 2008-09-13 at 10:40 -0500, Larry Finger wrote:
> Michael Buesch wrote:
> > On Friday 12 September 2008 20:52:52 Larry Finger wrote:
> >> No, it won't avoid any casts. The program uses the data area 7 times 
> >> in native-cpu order, once as be32, and once in little-endian order 
> > 
> > Is the native use correct? Smells fishy.
> 
> It does to me as well.

I think we should assume fixed byte order unless the hardware is
programmed differently for big-endian hosts (e.g. Atheros chipsets can
byte-swap some registers).  If the driver is working on little-endian
systems, we should assume little-endian data unless testing proves us to
be wrong.

>  I just got a copy of Intersil's data sheet for
> the MAC and everything looks little endian, but until we get a tester
> with a big-endian machine, we won't know. So far, no volunteers have
> come forward.

I have a PowerMac and a Prism54 card, so I can test.

-- 
Regards,
Pavel Roskin

Re: [RFC/RFT] p54: Fix sparse warnings

[Larry Finger]

Pavel Roskin wrote:
> 
> I have a PowerMac and a Prism54 card, so I can test.
> 

I gather you have not yet tested. I'm pretty sure that it does not
work without the patch I posted, but wonder if it does with it.

Thanks,

Larry