Re: [PATCH] staging: ks7010: clean up code
On Fri, 2017-03-03 at 23:24-0800, Joe Perches wrote: > > More stuff the changelog doesn't show :( > > On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote: > > This fixes type warnings generated by sparse, replaces instances of > > ntohs() with be16_to_cpu() and removes unused fields in structs. I’ve posted a v2 of the patch, reducing the scope so as to not inadvertently break something.
Re: [PATCH] staging: ks7010: clean up code
On Fri, 2017-03-03 at 23:24-0800, Joe Perches wrote: > > More stuff the changelog doesn't show :( > > On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote: > > This fixes type warnings generated by sparse, replaces instances of > > ntohs() with be16_to_cpu() and removes unused fields in structs. I’ve posted a v2 of the patch, reducing the scope so as to not inadvertently break something.
Re: [PATCH] staging: ks7010: clean up code
On Sat, 2017-03-04 at 08:49 +0200, Ernestas Kulik wrote: > On Fri, 2017-03-03 at 22:37-0800, Joe Perches wrote: > > > > struct hostif_hdr.event is declared at uint16_t > > and not as __le16 so I believe this is incorrect > > and actually introduces a sparse error. > > Sure, but I change that in the patch. :) More stuff the changelog doesn't show :( On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote: > This fixes type warnings generated by sparse, replaces instances of > ntohs() with be16_to_cpu() and removes unused fields in structs.
Re: [PATCH] staging: ks7010: clean up code
On Sat, 2017-03-04 at 08:49 +0200, Ernestas Kulik wrote: > On Fri, 2017-03-03 at 22:37-0800, Joe Perches wrote: > > > > struct hostif_hdr.event is declared at uint16_t > > and not as __le16 so I believe this is incorrect > > and actually introduces a sparse error. > > Sure, but I change that in the patch. :) More stuff the changelog doesn't show :( On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote: > This fixes type warnings generated by sparse, replaces instances of > ntohs() with be16_to_cpu() and removes unused fields in structs.
Re: [PATCH] staging: ks7010: clean up code
On Fri, 2017-03-03 at 22:37-0800, Joe Perches wrote: > > struct hostif_hdr.event is declared at uint16_t > and not as __le16 so I believe this is incorrect > and actually introduces a sparse error. Sure, but I change that in the patch. :) On Sat, 2017-03-04 at 08:28 +0200, Ernestas Kulik wrote: > > struct hostif_hdr { >- uint16_t size; >- uint16_t event; >+ __le16 size; >+ __le16 event; > } __packed;
Re: [PATCH] staging: ks7010: clean up code
On Fri, 2017-03-03 at 22:37-0800, Joe Perches wrote: > > struct hostif_hdr.event is declared at uint16_t > and not as __le16 so I believe this is incorrect > and actually introduces a sparse error. Sure, but I change that in the patch. :) On Sat, 2017-03-04 at 08:28 +0200, Ernestas Kulik wrote: > > struct hostif_hdr { >- uint16_t size; >- uint16_t event; >+ __le16 size; >+ __le16 event; > } __packed;
Re: [PATCH] staging: ks7010: clean up code
On Sat, 2017-03-04 at 08:28 +0200, Ernestas Kulik wrote: > On Fri, 2017-03-03 at 17:23-0800, Joe Perches wrote: > > > On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote: > > > > > This fixes type warnings generated by sparse, replaces instances of > > > ntohs() with be16_to_cpu() and removes unused fields in structs. > > > > There's no real need to convert ntohs to be16_to_cpu > > I noticed a patch that was merged that replaces calls to these with the > more “generic” be16_to_cpu(), but I can revert that. > > This kind of conversion also happened in SeaBIOS, but it’s not exactly > the same thing as here. > > > > diff --git a/drivers/staging/ks7010/ks7010_sdio.c > > > b/drivers/staging/ks7010/ks7010_sdio.c > > > > [] > > > > > > @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private > > > *priv, unsigned char *buffer, > > > hdr = (struct hostif_hdr *)buffer; > > > > > > DPRINTK(4, "size=%d\n", hdr->size); > > > - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) { > > > + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ || > > > + HIF_REQ_MAX < le16_to_cpu(hdr->event)) { > > > > > > > This isn't mentioned and doesn't match the commit message > > It is and it does, but the commit message is a bit vague. I’ll try to > expand it. Not really. struct hostif_hdr.event is declared at uint16_t and not as __le16 so I believe this is incorrect and actually introduces a sparse error.
Re: [PATCH] staging: ks7010: clean up code
On Sat, 2017-03-04 at 08:28 +0200, Ernestas Kulik wrote: > On Fri, 2017-03-03 at 17:23-0800, Joe Perches wrote: > > > On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote: > > > > > This fixes type warnings generated by sparse, replaces instances of > > > ntohs() with be16_to_cpu() and removes unused fields in structs. > > > > There's no real need to convert ntohs to be16_to_cpu > > I noticed a patch that was merged that replaces calls to these with the > more “generic” be16_to_cpu(), but I can revert that. > > This kind of conversion also happened in SeaBIOS, but it’s not exactly > the same thing as here. > > > > diff --git a/drivers/staging/ks7010/ks7010_sdio.c > > > b/drivers/staging/ks7010/ks7010_sdio.c > > > > [] > > > > > > @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private > > > *priv, unsigned char *buffer, > > > hdr = (struct hostif_hdr *)buffer; > > > > > > DPRINTK(4, "size=%d\n", hdr->size); > > > - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) { > > > + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ || > > > + HIF_REQ_MAX < le16_to_cpu(hdr->event)) { > > > > > > > This isn't mentioned and doesn't match the commit message > > It is and it does, but the commit message is a bit vague. I’ll try to > expand it. Not really. struct hostif_hdr.event is declared at uint16_t and not as __le16 so I believe this is incorrect and actually introduces a sparse error.
Re: [PATCH] staging: ks7010: clean up code
On Fri, 2017-03-03 at 17:23-0800, Joe Perches wrote: > On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote: > > > This fixes type warnings generated by sparse, replaces instances of > > ntohs() with be16_to_cpu() and removes unused fields in structs. > > There's no real need to convert ntohs to be16_to_cpu I noticed a patch that was merged that replaces calls to these with the more “generic” be16_to_cpu(), but I can revert that. This kind of conversion also happened in SeaBIOS, but it’s not exactly the same thing as here. > > diff --git a/drivers/staging/ks7010/ks7010_sdio.c > > b/drivers/staging/ks7010/ks7010_sdio.c > [] > > > > @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private > > *priv, unsigned char *buffer, > > hdr = (struct hostif_hdr *)buffer; > > > > DPRINTK(4, "size=%d\n", hdr->size); > > - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) { > > + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ || > > + HIF_REQ_MAX < le16_to_cpu(hdr->event)) { > > > > This isn't mentioned and doesn't match the commit message It is and it does, but the commit message is a bit vague. I’ll try to expand it.
Re: [PATCH] staging: ks7010: clean up code
On Fri, 2017-03-03 at 17:23-0800, Joe Perches wrote: > On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote: > > > This fixes type warnings generated by sparse, replaces instances of > > ntohs() with be16_to_cpu() and removes unused fields in structs. > > There's no real need to convert ntohs to be16_to_cpu I noticed a patch that was merged that replaces calls to these with the more “generic” be16_to_cpu(), but I can revert that. This kind of conversion also happened in SeaBIOS, but it’s not exactly the same thing as here. > > diff --git a/drivers/staging/ks7010/ks7010_sdio.c > > b/drivers/staging/ks7010/ks7010_sdio.c > [] > > > > @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private > > *priv, unsigned char *buffer, > > hdr = (struct hostif_hdr *)buffer; > > > > DPRINTK(4, "size=%d\n", hdr->size); > > - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) { > > + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ || > > + HIF_REQ_MAX < le16_to_cpu(hdr->event)) { > > > > This isn't mentioned and doesn't match the commit message It is and it does, but the commit message is a bit vague. I’ll try to expand it.
Re: [PATCH] staging: ks7010: clean up code
On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote: > This fixes type warnings generated by sparse, replaces instances of > ntohs() with be16_to_cpu() and removes unused fields in structs. There's no real need to convert ntohs to be16_to_cpu > diff --git a/drivers/staging/ks7010/ks7010_sdio.c > b/drivers/staging/ks7010/ks7010_sdio.c [] > > @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private *priv, > unsigned char *buffer, > hdr = (struct hostif_hdr *)buffer; > > DPRINTK(4, "size=%d\n", hdr->size); > - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) { > + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ || > + HIF_REQ_MAX < le16_to_cpu(hdr->event)) { > This isn't mentioned and doesn't match the commit message
Re: [PATCH] staging: ks7010: clean up code
On Fri, 2017-03-03 at 22:58 +0200, Ernestas Kulik wrote: > This fixes type warnings generated by sparse, replaces instances of > ntohs() with be16_to_cpu() and removes unused fields in structs. There's no real need to convert ntohs to be16_to_cpu > diff --git a/drivers/staging/ks7010/ks7010_sdio.c > b/drivers/staging/ks7010/ks7010_sdio.c [] > > @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private *priv, > unsigned char *buffer, > hdr = (struct hostif_hdr *)buffer; > > DPRINTK(4, "size=%d\n", hdr->size); > - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) { > + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ || > + HIF_REQ_MAX < le16_to_cpu(hdr->event)) { > This isn't mentioned and doesn't match the commit message
[PATCH] staging: ks7010: clean up code
This fixes type warnings generated by sparse, replaces instances of ntohs() with be16_to_cpu() and removes unused fields in structs. Signed-off-by: Ernestas Kulik--- drivers/staging/ks7010/ks7010_sdio.c | 12 ++-- drivers/staging/ks7010/ks_hostif.c | 30 + drivers/staging/ks7010/ks_hostif.h | 124 +-- drivers/staging/ks7010/ks_wlan.h | 2 +- 4 files changed, 85 insertions(+), 83 deletions(-) diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c index 6f9f746a3a61..8e644ff8eca8 100644 --- a/drivers/staging/ks7010/ks7010_sdio.c +++ b/drivers/staging/ks7010/ks7010_sdio.c @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private *priv, unsigned char *buffer, hdr = (struct hostif_hdr *)buffer; DPRINTK(4, "size=%d\n", hdr->size); - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) { + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ || + HIF_REQ_MAX < le16_to_cpu(hdr->event)) { DPRINTK(1, "unknown event=%04X\n", hdr->event); return 0; } @@ -361,13 +362,14 @@ int ks_wlan_hw_tx(struct ks_wlan_private *priv, void *p, unsigned long size, hdr = (struct hostif_hdr *)p; - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) { + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ || + HIF_REQ_MAX < le16_to_cpu(hdr->event)) { DPRINTK(1, "unknown event=%04X\n", hdr->event); return 0; } /* add event to hostt buffer */ - priv->hostt.buff[priv->hostt.qtail] = hdr->event; + priv->hostt.buff[priv->hostt.qtail] = le16_to_cpu(hdr->event); priv->hostt.qtail = (priv->hostt.qtail + 1) % SME_EVENT_BUFF_SIZE; DPRINTK(4, "event=%04X\n", hdr->event); @@ -406,7 +408,7 @@ static void ks_wlan_hw_rx(void *dev, uint16_t size) struct rx_device_buffer *rx_buffer; struct hostif_hdr *hdr; unsigned char read_status; - unsigned short event = 0; + __le16 event = 0; DPRINTK(4, "\n"); @@ -459,7 +461,7 @@ static void ks_wlan_hw_rx(void *dev, uint16_t size) DPRINTK(4, "READ_STATUS=%02X\n", read_status); if (atomic_read(>psstatus.confirm_wait)) { - if (IS_HIF_CONF(event)) { + if (IS_HIF_CONF(le16_to_cpu(event))) { DPRINTK(4, "IS_HIF_CONF true !!\n"); atomic_dec(>psstatus.confirm_wait); } diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c index da7c42ef05f5..4ad4a0c72ca8 100644 --- a/drivers/staging/ks7010/ks_hostif.c +++ b/drivers/staging/ks7010/ks_hostif.c @@ -339,7 +339,7 @@ void hostif_data_indication(struct ks_wlan_private *priv) get_WORD(priv); /* Reserve Area */ eth_hdr = (struct ether_hdr *)(priv->rxp); - eth_proto = ntohs(eth_hdr->h_proto); + eth_proto = be16_to_cpu((__force __be16)eth_hdr->h_proto); DPRINTK(3, "ether protocol = %04X\n", eth_proto); /* source address check */ @@ -1200,7 +1200,7 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *packet) /* for WPA */ eth_hdr = (struct ether_hdr *)>data[0]; - eth_proto = ntohs(eth_hdr->h_proto); + eth_proto = be16_to_cpu((__force __be16)eth_hdr->h_proto); /* for MIC FAILURE REPORT check */ if (eth_proto == ETHER_PROTOCOL_TYPE_EAP @@ -1208,7 +1208,7 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *packet) aa1x_hdr = (struct ieee802_1x_hdr *)(eth_hdr + 1); if (aa1x_hdr->type == IEEE802_1X_TYPE_EAPOL_KEY) { eap_key = (struct wpa_eapol_key *)(aa1x_hdr + 1); - keyinfo = ntohs(eap_key->key_info); + keyinfo = be16_to_cpu((__force __be16)eap_key->key_info); } } @@ -1867,7 +1867,7 @@ void hostif_receive(struct ks_wlan_private *priv, unsigned char *p, static void hostif_sme_set_wep(struct ks_wlan_private *priv, int type) { - uint32_t val; + __le32 val; switch (type) { case SME_WEP_INDEX_REQUEST: @@ -1916,13 +1916,13 @@ void hostif_sme_set_wep(struct ks_wlan_private *priv, int type) } struct wpa_suite_t { - unsigned short size; + __le16 size; unsigned char suite[4][CIPHER_ID_LEN]; } __packed; struct rsn_mode_t { - uint32_t rsn_mode; - uint16_t rsn_capability; + __le32 rsn_mode; + __le16 rsn_capability; } __packed; static @@ -1930,7 +1930,7 @@ void hostif_sme_set_rsn(struct ks_wlan_private *priv, int type) { struct wpa_suite_t wpa_suite; struct rsn_mode_t rsn_mode; - uint32_t val; + __le32 val; memset(_suite, 0, sizeof(wpa_suite)); @@ -1982,7 +1982,8 @@ void hostif_sme_set_rsn(struct ks_wlan_private
[PATCH] staging: ks7010: clean up code
This fixes type warnings generated by sparse, replaces instances of ntohs() with be16_to_cpu() and removes unused fields in structs. Signed-off-by: Ernestas Kulik --- drivers/staging/ks7010/ks7010_sdio.c | 12 ++-- drivers/staging/ks7010/ks_hostif.c | 30 + drivers/staging/ks7010/ks_hostif.h | 124 +-- drivers/staging/ks7010/ks_wlan.h | 2 +- 4 files changed, 85 insertions(+), 83 deletions(-) diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c index 6f9f746a3a61..8e644ff8eca8 100644 --- a/drivers/staging/ks7010/ks7010_sdio.c +++ b/drivers/staging/ks7010/ks7010_sdio.c @@ -297,7 +297,8 @@ static int write_to_device(struct ks_wlan_private *priv, unsigned char *buffer, hdr = (struct hostif_hdr *)buffer; DPRINTK(4, "size=%d\n", hdr->size); - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) { + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ || + HIF_REQ_MAX < le16_to_cpu(hdr->event)) { DPRINTK(1, "unknown event=%04X\n", hdr->event); return 0; } @@ -361,13 +362,14 @@ int ks_wlan_hw_tx(struct ks_wlan_private *priv, void *p, unsigned long size, hdr = (struct hostif_hdr *)p; - if (hdr->event < HIF_DATA_REQ || HIF_REQ_MAX < hdr->event) { + if (le16_to_cpu(hdr->event) < HIF_DATA_REQ || + HIF_REQ_MAX < le16_to_cpu(hdr->event)) { DPRINTK(1, "unknown event=%04X\n", hdr->event); return 0; } /* add event to hostt buffer */ - priv->hostt.buff[priv->hostt.qtail] = hdr->event; + priv->hostt.buff[priv->hostt.qtail] = le16_to_cpu(hdr->event); priv->hostt.qtail = (priv->hostt.qtail + 1) % SME_EVENT_BUFF_SIZE; DPRINTK(4, "event=%04X\n", hdr->event); @@ -406,7 +408,7 @@ static void ks_wlan_hw_rx(void *dev, uint16_t size) struct rx_device_buffer *rx_buffer; struct hostif_hdr *hdr; unsigned char read_status; - unsigned short event = 0; + __le16 event = 0; DPRINTK(4, "\n"); @@ -459,7 +461,7 @@ static void ks_wlan_hw_rx(void *dev, uint16_t size) DPRINTK(4, "READ_STATUS=%02X\n", read_status); if (atomic_read(>psstatus.confirm_wait)) { - if (IS_HIF_CONF(event)) { + if (IS_HIF_CONF(le16_to_cpu(event))) { DPRINTK(4, "IS_HIF_CONF true !!\n"); atomic_dec(>psstatus.confirm_wait); } diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c index da7c42ef05f5..4ad4a0c72ca8 100644 --- a/drivers/staging/ks7010/ks_hostif.c +++ b/drivers/staging/ks7010/ks_hostif.c @@ -339,7 +339,7 @@ void hostif_data_indication(struct ks_wlan_private *priv) get_WORD(priv); /* Reserve Area */ eth_hdr = (struct ether_hdr *)(priv->rxp); - eth_proto = ntohs(eth_hdr->h_proto); + eth_proto = be16_to_cpu((__force __be16)eth_hdr->h_proto); DPRINTK(3, "ether protocol = %04X\n", eth_proto); /* source address check */ @@ -1200,7 +1200,7 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *packet) /* for WPA */ eth_hdr = (struct ether_hdr *)>data[0]; - eth_proto = ntohs(eth_hdr->h_proto); + eth_proto = be16_to_cpu((__force __be16)eth_hdr->h_proto); /* for MIC FAILURE REPORT check */ if (eth_proto == ETHER_PROTOCOL_TYPE_EAP @@ -1208,7 +1208,7 @@ int hostif_data_request(struct ks_wlan_private *priv, struct sk_buff *packet) aa1x_hdr = (struct ieee802_1x_hdr *)(eth_hdr + 1); if (aa1x_hdr->type == IEEE802_1X_TYPE_EAPOL_KEY) { eap_key = (struct wpa_eapol_key *)(aa1x_hdr + 1); - keyinfo = ntohs(eap_key->key_info); + keyinfo = be16_to_cpu((__force __be16)eap_key->key_info); } } @@ -1867,7 +1867,7 @@ void hostif_receive(struct ks_wlan_private *priv, unsigned char *p, static void hostif_sme_set_wep(struct ks_wlan_private *priv, int type) { - uint32_t val; + __le32 val; switch (type) { case SME_WEP_INDEX_REQUEST: @@ -1916,13 +1916,13 @@ void hostif_sme_set_wep(struct ks_wlan_private *priv, int type) } struct wpa_suite_t { - unsigned short size; + __le16 size; unsigned char suite[4][CIPHER_ID_LEN]; } __packed; struct rsn_mode_t { - uint32_t rsn_mode; - uint16_t rsn_capability; + __le32 rsn_mode; + __le16 rsn_capability; } __packed; static @@ -1930,7 +1930,7 @@ void hostif_sme_set_rsn(struct ks_wlan_private *priv, int type) { struct wpa_suite_t wpa_suite; struct rsn_mode_t rsn_mode; - uint32_t val; + __le32 val; memset(_suite, 0, sizeof(wpa_suite)); @@ -1982,7 +1982,8 @@ void hostif_sme_set_rsn(struct ks_wlan_private *priv, int type)