[PATCH 1/2] iwmc3200wifi: Fix sparse warnings

[PATCH 1/2] iwmc3200wifi: Fix sparse warnings

[Zhu Yi]

From: Samuel Ortiz <sameo@linux.intel.com>

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Zhu Yi <yi.zhu@intel.com>
---
 drivers/net/wireless/iwmc3200wifi/rx.c    |    3 ++-
 drivers/net/wireless/iwmc3200wifi/trace.h |    4 ++--
 drivers/net/wireless/iwmc3200wifi/tx.c    |    4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/iwmc3200wifi/rx.c b/drivers/net/wireless/iwmc3200wifi/rx.c
index ad53987..e1184de 100644
--- a/drivers/net/wireless/iwmc3200wifi/rx.c
+++ b/drivers/net/wireless/iwmc3200wifi/rx.c
@@ -431,7 +431,8 @@ static int iwm_ntf_rx_ticket(struct iwm_priv *iwm, u8 *buf,
 				return PTR_ERR(ticket_node);
 
 			IWM_DBG_RX(iwm, DBG, "TICKET %s(%d)\n",
-				   ticket->action ==  IWM_RX_TICKET_RELEASE ?
+				   __le16_to_cpu(ticket->action) ==
+							IWM_RX_TICKET_RELEASE ?
 				   "RELEASE" : "DROP",
 				   ticket->id);
 			spin_lock(&iwm->ticket_lock);
diff --git a/drivers/net/wireless/iwmc3200wifi/trace.h b/drivers/net/wireless/iwmc3200wifi/trace.h
index 320e54f..abb4805 100644
--- a/drivers/net/wireless/iwmc3200wifi/trace.h
+++ b/drivers/net/wireless/iwmc3200wifi/trace.h
@@ -76,7 +76,7 @@ TRACE_EVENT(iwm_tx_wifi_cmd,
 		IWM_ASSIGN;
 		__entry->opcode = hdr->sw_hdr.cmd.cmd;
 		__entry->lmac = 0;
-		__entry->seq = hdr->sw_hdr.cmd.seq_num;
+		__entry->seq = __le16_to_cpu(hdr->sw_hdr.cmd.seq_num);
 		__entry->resp = GET_VAL8(hdr->sw_hdr.cmd.flags, UMAC_DEV_CMD_FLAGS_RESP_REQ);
 		__entry->color = GET_VAL32(hdr->sw_hdr.meta_data, UMAC_FW_CMD_TX_STA_COLOR);
 		__entry->eot = GET_VAL32(hdr->hw_hdr.cmd, UMAC_HDI_OUT_CMD_EOT);
@@ -123,7 +123,7 @@ TRACE_EVENT(iwm_tx_packets,
 		__entry->ra_tid = GET_VAL32(hdr->hw_hdr.meta_data, UMAC_HDI_OUT_RATID);
 		__entry->credit_group = GET_VAL32(hdr->hw_hdr.meta_data, UMAC_HDI_OUT_CREDIT_GRP);
 		__entry->color = GET_VAL32(hdr->sw_hdr.meta_data, UMAC_FW_CMD_TX_STA_COLOR);
-		__entry->seq = hdr->sw_hdr.cmd.seq_num;
+		__entry->seq = __le16_to_cpu(hdr->sw_hdr.cmd.seq_num);
 		__entry->npkt = 1;
 		__entry->bytes = len;
 
diff --git a/drivers/net/wireless/iwmc3200wifi/tx.c b/drivers/net/wireless/iwmc3200wifi/tx.c
index 9537cdb..3216621 100644
--- a/drivers/net/wireless/iwmc3200wifi/tx.c
+++ b/drivers/net/wireless/iwmc3200wifi/tx.c
@@ -302,8 +302,8 @@ void iwm_tx_credit_init_pools(struct iwm_priv *iwm,
 
 #define IWM_UDMA_HDR_LEN	sizeof(struct iwm_umac_wifi_out_hdr)
 
-static int iwm_tx_build_packet(struct iwm_priv *iwm, struct sk_buff *skb,
-			       int pool_id, u8 *buf)
+static __le16 iwm_tx_build_packet(struct iwm_priv *iwm, struct sk_buff *skb,
+				  int pool_id, u8 *buf)
 {
 	struct iwm_umac_wifi_out_hdr *hdr = (struct iwm_umac_wifi_out_hdr *)buf;
 	struct iwm_udma_wifi_cmd udma_cmd;
-- 
1.6.3.3

[PATCH 2/2] iwmc3200wifi: check sparse endianness annotations

[Zhu Yi]

From: Samuel Ortiz <sameo@linux.intel.com>

Add -D__CHECK_ENDIAN__ to driver ccflags so that sparse will
always check endianness by default.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Zhu Yi <yi.zhu@intel.com>
---
 drivers/net/wireless/iwmc3200wifi/Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/iwmc3200wifi/Makefile b/drivers/net/wireless/iwmc3200wifi/Makefile
index aeed5cd..cdc7e07 100644
--- a/drivers/net/wireless/iwmc3200wifi/Makefile
+++ b/drivers/net/wireless/iwmc3200wifi/Makefile
@@ -6,3 +6,5 @@ iwmc3200wifi-$(CONFIG_IWM_DEBUG) += debugfs.o
 iwmc3200wifi-$(CONFIG_IWM_TRACING) += trace.o
 
 CFLAGS_trace.o := -I$(src)
+
+ccflags-y += -D__CHECK_ENDIAN__
-- 
1.6.3.3

Re: [PATCH 2/2] iwmc3200wifi: check sparse endianness annotations

[Pavel Roskin]

On Fri, 2010-04-16 at 09:28 +0800, Zhu Yi wrote:
> From: Samuel Ortiz <sameo@linux.intel.com>
> 
> Add -D__CHECK_ENDIAN__ to driver ccflags so that sparse will
> always check endianness by default.

I believe it's wrong.  I would be fine with enabling __CHECK_ENDIAN__
for the whole kernel by default (perhaps with an option to disable it).
But there is no reason why some particular drivers need that check more
than the rest of the kernel.

-- 
Regards,
Pavel Roskin

Re: [PATCH 2/2] iwmc3200wifi: check sparse endianness annotations

[Johannes Berg]

On Fri, 2010-04-16 at 16:55 -0400, Pavel Roskin wrote:
> On Fri, 2010-04-16 at 09:28 +0800, Zhu Yi wrote:
> > From: Samuel Ortiz <sameo@linux.intel.com>
> > 
> > Add -D__CHECK_ENDIAN__ to driver ccflags so that sparse will
> > always check endianness by default.
> 
> I believe it's wrong.  I would be fine with enabling __CHECK_ENDIAN__
> for the whole kernel by default (perhaps with an option to disable it).
> But there is no reason why some particular drivers need that check more
> than the rest of the kernel.

We've done it on other drivers -- we can't do it for all of the kernel
because that would drown people in warnings, but for those drivers that
_should_ be clean it ought to be fine to add it by default.

johannes

Re: [PATCH 2/2] iwmc3200wifi: check sparse endianness annotations

[Pavel Roskin]

On Fri, 2010-04-16 at 22:57 +0200, Johannes Berg wrote:

> We've done it on other drivers -- we can't do it for all of the kernel
> because that would drown people in warnings, but for those drivers that
> _should_ be clean it ought to be fine to add it by default.

Oh, I see, there are several precedents.

That said, there are not many warnings added by __CHECK_ENDIAN__.  In my
x86_64 configuration, sparse produces 2316 lines of output without
__CHECK_ENDIAN__ and 3526 lines with __CHECK_ENDIAN__, a 52% increase.
That's hardly "drowning".

Also, the warnings about endianess are perhaps the most useful of all
sparse warnings.

-- 
Regards,
Pavel Roskin

Re: [PATCH 2/2] iwmc3200wifi: check sparse endianness annotations

[Johannes Berg]

On Fri, 2010-04-16 at 17:34 -0400, Pavel Roskin wrote:
> On Fri, 2010-04-16 at 22:57 +0200, Johannes Berg wrote:
> 
> > We've done it on other drivers -- we can't do it for all of the kernel
> > because that would drown people in warnings, but for those drivers that
> > _should_ be clean it ought to be fine to add it by default.
> 
> Oh, I see, there are several precedents.
> 
> That said, there are not many warnings added by __CHECK_ENDIAN__.  In my
> x86_64 configuration, sparse produces 2316 lines of output without
> __CHECK_ENDIAN__ and 3526 lines with __CHECK_ENDIAN__, a 52% increase.
> That's hardly "drowning".

Other people feel differently by that, evidenced by the "lack of
enthusiasm" for adding endian checking by default.

> Also, the warnings about endianess are perhaps the most useful of all
> sparse warnings.

I agree, so I want them whenever I run sparse on code I own.

However, I'm simply not willing to fight for making endian checking
default kernel-wide to achieve that goal. You may disagree, and you're
welcome to pick that fight. Until then, please let us do what gets our
job done.

johannes