[PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

[PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

[Quytelda Kahja]

The case statement in get_ap_information() should not use literal integers
to parse information element IDs when these values are provided by name
in 'enum ieee80211_eid' in the header 'linux/ieee80211.h'.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
Reviewed-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/staging/ks7010/ks_hostif.c | 31 +++++++++++++++----------------
 drivers/staging/ks7010/ks_hostif.h |  1 +
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
index 74a08417bd0b..67cf32433023 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -251,9 +251,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
 	offset = 0;
 
 	while (bsize > offset) {
-		/* DPRINTK(4, "Element ID=%d\n",*bp); */
-		switch (*bp) {
-		case 0:	/* ssid */
+		switch (*bp) { /* Information Element ID */
+		case WLAN_EID_SSID:
 			if (*(bp + 1) <= SSID_MAX_SIZE) {
 				ap->ssid.size = *(bp + 1);
 			} else {
@@ -263,8 +262,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
 			}
 			memcpy(ap->ssid.body, bp + 2, ap->ssid.size);
 			break;
-		case 1:	/* rate */
-		case 50:	/* ext rate */
+		case WLAN_EID_SUPP_RATES:
+		case WLAN_EID_EXT_SUPP_RATES:
 			if ((*(bp + 1) + ap->rate_set.size) <=
 			    RATE_SET_MAX_SIZE) {
 				memcpy(&ap->rate_set.body[ap->rate_set.size],
@@ -280,9 +279,9 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
 				    (RATE_SET_MAX_SIZE - ap->rate_set.size);
 			}
 			break;
-		case 3:	/* DS parameter */
+		case WLAN_EID_DS_PARAMS:
 			break;
-		case 48:	/* RSN(WPA2) */
+		case WLAN_EID_RSN:
 			ap->rsn_ie.id = *bp;
 			if (*(bp + 1) <= RSN_IE_BODY_MAX) {
 				ap->rsn_ie.size = *(bp + 1);
@@ -293,8 +292,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
 			}
 			memcpy(ap->rsn_ie.body, bp + 2, ap->rsn_ie.size);
 			break;
-		case 221:	/* WPA */
-			if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) {	/* WPA OUI check */
+		case WLAN_EID_VENDOR_SPECIFIC: /* WPA */
+			if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) { /* WPA OUI check */
 				ap->wpa_ie.id = *bp;
 				if (*(bp + 1) <= RSN_IE_BODY_MAX) {
 					ap->wpa_ie.size = *(bp + 1);
@@ -309,18 +308,18 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
 			}
 			break;
 
-		case 2:	/* FH parameter */
-		case 4:	/* CF parameter */
-		case 5:	/* TIM */
-		case 6:	/* IBSS parameter */
-		case 7:	/* Country */
-		case 42:	/* ERP information */
-		case 47:	/* Reserve ID 47 Broadcom AP */
+		case WLAN_EID_FH_PARAMS:
+		case WLAN_EID_CF_PARAMS:
+		case WLAN_EID_TIM:
+		case WLAN_EID_IBSS_PARAMS:
+		case WLAN_EID_COUNTRY:
+		case WLAN_EID_ERP_INFO:
 			break;
 		default:
 			DPRINTK(4, "unknown Element ID=%d\n", *bp);
 			break;
 		}
+
 		offset += 2;	/* id & size field */
 		offset += *(bp + 1);	/* +size offset */
 		bp += (*(bp + 1) + 2);	/* pointer update */
diff --git a/drivers/staging/ks7010/ks_hostif.h b/drivers/staging/ks7010/ks_hostif.h
index 5bae8d468e23..9ac317e4b507 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -13,6 +13,7 @@
 #define _KS_HOSTIF_H_
 
 #include <linux/compiler.h>
+#include <linux/ieee80211.h>
 
 /*
  * HOST-MAC I/F events
-- 
2.16.2

[PATCH 2/5] staging: ks7010: Replace SSID_MAX_SIZE with IEEE80211_MAX_SSID_LEN.

[Quytelda Kahja]

SSID_MAX_SIZE is a constant defined locally in ks_hostif.h, but it should
be replaced with IEEE80211_MAX_SSID_LEN from the kernel's 802.11 header,
of which it is just a copy.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
Reviewed-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/staging/ks7010/ks_hostif.c | 4 ++--
 drivers/staging/ks7010/ks_hostif.h | 3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
index 67cf32433023..59f7c4e422d3 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -253,12 +253,12 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
 	while (bsize > offset) {
 		switch (*bp) { /* Information Element ID */
 		case WLAN_EID_SSID:
-			if (*(bp + 1) <= SSID_MAX_SIZE) {
+			if (*(bp + 1) <= IEEE80211_MAX_SSID_LEN) {
 				ap->ssid.size = *(bp + 1);
 			} else {
 				DPRINTK(1, "size over :: ssid size=%d\n",
 					*(bp + 1));
-				ap->ssid.size = SSID_MAX_SIZE;
+				ap->ssid.size = IEEE80211_MAX_SSID_LEN;
 			}
 			memcpy(ap->ssid.body, bp + 2, ap->ssid.size);
 			break;
diff --git a/drivers/staging/ks7010/ks_hostif.h b/drivers/staging/ks7010/ks_hostif.h
index 9ac317e4b507..b46aa94c0d48 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -225,10 +225,9 @@ struct hostif_start_confirm_t {
 	__le16 result_code;
 } __packed;
 
-#define SSID_MAX_SIZE 32
 struct ssid_t {
 	u8 size;
-	u8 body[SSID_MAX_SIZE];
+	u8 body[IEEE80211_MAX_SSID_LEN];
 	u8 ssid_pad;
 } __packed;
 
-- 
2.16.2

[PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Quytelda Kahja]

The code that generates a WLAN capability mask is repeated in five
functions.  This change refactors that code into a new function, which is
called now in each of those functions.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
Reviewed-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/staging/ks7010/ks_hostif.c | 88 ++++++++++----------------------------
 1 file changed, 23 insertions(+), 65 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
index 59f7c4e422d3..4d11627a3d72 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -1381,11 +1381,28 @@ void hostif_start_request(struct ks_wlan_private *priv, unsigned char mode)
 	priv->scan_ind_count = 0;
 }
 
+static __le16 ks_wlan_cap(struct ks_wlan_private *priv)
+{
+	u16 capability = 0x0000;
+
+	if (priv->reg.preamble == SHORT_PREAMBLE) {
+		capability |= BSS_CAP_SHORT_PREAMBLE;
+	}
+
+	capability &= ~(BSS_CAP_PBCC);	/* pbcc not support */
+
+	if (priv->reg.phy_type != D_11B_ONLY_MODE) {
+		capability |= BSS_CAP_SHORT_SLOT_TIME;
+		capability &= ~(BSS_CAP_DSSS_OFDM);
+	}
+
+	return cpu_to_le16((uint16_t)capability);
+}
+
 static
 void hostif_ps_adhoc_set_request(struct ks_wlan_private *priv)
 {
 	struct hostif_ps_adhoc_set_request_t *pp;
-	u16 capability;
 
 	DPRINTK(3, "\n");
 
@@ -1398,21 +1415,10 @@ void hostif_ps_adhoc_set_request(struct ks_wlan_private *priv)
 	pp->scan_type = cpu_to_le16((uint16_t)(priv->reg.scan_type));
 	pp->channel = cpu_to_le16((uint16_t)(priv->reg.channel));
 	pp->rate_set.size = priv->reg.rate_set.size;
+	pp->capability = ks_wlan_cap(priv);
 	memcpy(&pp->rate_set.body[0], &priv->reg.rate_set.body[0],
 	       priv->reg.rate_set.size);
 
-	capability = 0x0000;
-	if (priv->reg.preamble == SHORT_PREAMBLE) {
-		/* short preamble */
-		capability |= BSS_CAP_SHORT_PREAMBLE;
-	}
-	capability &= ~(BSS_CAP_PBCC);	/* pbcc not support */
-	if (priv->reg.phy_type != D_11B_ONLY_MODE) {
-		capability |= BSS_CAP_SHORT_SLOT_TIME;	/* ShortSlotTime support */
-		capability &= ~(BSS_CAP_DSSS_OFDM);	/* DSSS OFDM */
-	}
-	pp->capability = cpu_to_le16((uint16_t)capability);
-
 	/* send to device request */
 	ps_confirm_wait_inc(priv);
 	ks_wlan_hw_tx(priv, pp, hif_align_size(sizeof(*pp)), NULL, NULL);
@@ -1422,7 +1428,6 @@ static
 void hostif_infrastructure_set_request(struct ks_wlan_private *priv)
 {
 	struct hostif_infrastructure_set_request_t *pp;
-	u16 capability;
 
 	DPRINTK(3, "ssid.size=%d\n", priv->reg.ssid.size);
 
@@ -1439,18 +1444,7 @@ void hostif_infrastructure_set_request(struct ks_wlan_private *priv)
 	       priv->reg.rate_set.size);
 	pp->ssid.size = priv->reg.ssid.size;
 	memcpy(&pp->ssid.body[0], &priv->reg.ssid.body[0], priv->reg.ssid.size);
-
-	capability = 0x0000;
-	if (priv->reg.preamble == SHORT_PREAMBLE) {
-		/* short preamble */
-		capability |= BSS_CAP_SHORT_PREAMBLE;
-	}
-	capability &= ~(BSS_CAP_PBCC);	/* pbcc not support */
-	if (priv->reg.phy_type != D_11B_ONLY_MODE) {
-		capability |= BSS_CAP_SHORT_SLOT_TIME;	/* ShortSlotTime support */
-		capability &= ~(BSS_CAP_DSSS_OFDM);	/* DSSS OFDM not support */
-	}
-	pp->capability = cpu_to_le16((uint16_t)capability);
+	pp->capability = ks_wlan_cap(priv);
 	pp->beacon_lost_count =
 	    cpu_to_le16((uint16_t)(priv->reg.beacon_lost_count));
 	pp->auth_type = cpu_to_le16((uint16_t)(priv->reg.authenticate_type));
@@ -1483,7 +1477,6 @@ void hostif_infrastructure_set_request(struct ks_wlan_private *priv)
 static void hostif_infrastructure_set2_request(struct ks_wlan_private *priv)
 {
 	struct hostif_infrastructure_set2_request_t *pp;
-	u16 capability;
 
 	DPRINTK(2, "ssid.size=%d\n", priv->reg.ssid.size);
 
@@ -1500,18 +1493,7 @@ static void hostif_infrastructure_set2_request(struct ks_wlan_private *priv)
 	       priv->reg.rate_set.size);
 	pp->ssid.size = priv->reg.ssid.size;
 	memcpy(&pp->ssid.body[0], &priv->reg.ssid.body[0], priv->reg.ssid.size);
-
-	capability = 0x0000;
-	if (priv->reg.preamble == SHORT_PREAMBLE) {
-		/* short preamble */
-		capability |= BSS_CAP_SHORT_PREAMBLE;
-	}
-	capability &= ~(BSS_CAP_PBCC);	/* pbcc not support */
-	if (priv->reg.phy_type != D_11B_ONLY_MODE) {
-		capability |= BSS_CAP_SHORT_SLOT_TIME;	/* ShortSlotTime support */
-		capability &= ~(BSS_CAP_DSSS_OFDM);	/* DSSS OFDM not support */
-	}
-	pp->capability = cpu_to_le16((uint16_t)capability);
+	pp->capability = ks_wlan_cap(priv);
 	pp->beacon_lost_count =
 	    cpu_to_le16((uint16_t)(priv->reg.beacon_lost_count));
 	pp->auth_type = cpu_to_le16((uint16_t)(priv->reg.authenticate_type));
@@ -1547,7 +1529,6 @@ static
 void hostif_adhoc_set_request(struct ks_wlan_private *priv)
 {
 	struct hostif_adhoc_set_request_t *pp;
-	u16 capability;
 
 	DPRINTK(3, "\n");
 
@@ -1564,18 +1545,7 @@ void hostif_adhoc_set_request(struct ks_wlan_private *priv)
 	       priv->reg.rate_set.size);
 	pp->ssid.size = priv->reg.ssid.size;
 	memcpy(&pp->ssid.body[0], &priv->reg.ssid.body[0], priv->reg.ssid.size);
-
-	capability = 0x0000;
-	if (priv->reg.preamble == SHORT_PREAMBLE) {
-		/* short preamble */
-		capability |= BSS_CAP_SHORT_PREAMBLE;
-	}
-	capability &= ~(BSS_CAP_PBCC);	/* pbcc not support */
-	if (priv->reg.phy_type != D_11B_ONLY_MODE) {
-		capability |= BSS_CAP_SHORT_SLOT_TIME;	/* ShortSlotTime support */
-		capability &= ~(BSS_CAP_DSSS_OFDM);	/* DSSS OFDM not support */
-	}
-	pp->capability = cpu_to_le16((uint16_t)capability);
+	pp->capability = ks_wlan_cap(priv);
 
 	/* send to device request */
 	ps_confirm_wait_inc(priv);
@@ -1586,7 +1556,6 @@ static
 void hostif_adhoc_set2_request(struct ks_wlan_private *priv)
 {
 	struct hostif_adhoc_set2_request_t *pp;
-	u16 capability;
 
 	DPRINTK(3, "\n");
 
@@ -1602,18 +1571,7 @@ void hostif_adhoc_set2_request(struct ks_wlan_private *priv)
 	       priv->reg.rate_set.size);
 	pp->ssid.size = priv->reg.ssid.size;
 	memcpy(&pp->ssid.body[0], &priv->reg.ssid.body[0], priv->reg.ssid.size);
-
-	capability = 0x0000;
-	if (priv->reg.preamble == SHORT_PREAMBLE) {
-		/* short preamble */
-		capability |= BSS_CAP_SHORT_PREAMBLE;
-	}
-	capability &= ~(BSS_CAP_PBCC);	/* pbcc not support */
-	if (priv->reg.phy_type != D_11B_ONLY_MODE) {
-		capability |= BSS_CAP_SHORT_SLOT_TIME;	/* ShortSlotTime support */
-		capability &= ~(BSS_CAP_DSSS_OFDM);	/* DSSS OFDM not support */
-	}
-	pp->capability = cpu_to_le16((uint16_t)capability);
+	pp->capability = ks_wlan_cap(priv);
 
 	pp->channel_list.body[0] = priv->reg.channel;
 	pp->channel_list.size = 1;
-- 
2.16.2

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Wed, Feb 28, 2018 at 09:19:09PM -0800, Quytelda Kahja wrote:
> The code that generates a WLAN capability mask is repeated in five
> functions.  This change refactors that code into a new function, which is
> called now in each of those functions.

Perhaps in future something like:

Code to generate the WLAN capability mask is duplicated five times

Add helper function to generate WLAN capability mask, refactor code to
use newly defined function.


See Documentation/process/submitting-patches.rst section 2 for more info.

	2) Describe your changes

Hope this helps,
Tobin.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Dan Carpenter]

On Thu, Mar 01, 2018 at 05:37:21PM +1100, Tobin C. Harding wrote:
> On Wed, Feb 28, 2018 at 09:19:09PM -0800, Quytelda Kahja wrote:
> > The code that generates a WLAN capability mask is repeated in five
> > functions.  This change refactors that code into a new function, which is
> > called now in each of those functions.
> 
> Perhaps in future something like:
> 
> Code to generate the WLAN capability mask is duplicated five times
> 
> Add helper function to generate WLAN capability mask, refactor code to
> use newly defined function.
> 

I honestly don't see the difference between that and what Quytelda
wrote?  I understood the original changelog just fine.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Thu, Mar 01, 2018 at 02:15:00PM +0300, Dan Carpenter wrote:
> On Thu, Mar 01, 2018 at 05:37:21PM +1100, Tobin C. Harding wrote:
> > On Wed, Feb 28, 2018 at 09:19:09PM -0800, Quytelda Kahja wrote:
> > > The code that generates a WLAN capability mask is repeated in five
> > > functions.  This change refactors that code into a new function, which is
> > > called now in each of those functions.
> > 
> > Perhaps in future something like:
> > 
> > Code to generate the WLAN capability mask is duplicated five times
> > 
> > Add helper function to generate WLAN capability mask, refactor code to
> > use newly defined function.
> > 
> 
> I honestly don't see the difference between that and what Quytelda
> wrote?  I understood the original changelog just fine.
> 
> regards,
> dan carpenter

I had a feeling that the sentiment of the suggestion I was trying to get
at didn't come across, thanks for pointing it out.  I was intending to
suggest not using sentences like this

    	> This change refactors that code into a new function, which is
	> called now in each of those functions.

And instead use, as suggested in submitting-patches.rst, imperative mood

	Refactor code into new function ...

FTR I find the English bits of kernel dev (and programming in general)
the most difficult even though English is my first language.  I would
like to write it better.


Hope this helps,
Tobin.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Quytelda Kahja]

Tobin,
I understand your point, and I've read submitting-patches.rst.  I made
that wording choice because I was looking at some older commits that
were worded like that.  I'm fairly new to the kernel workflow, so I
was just trying to emulate something established, and it sounded less
stilted than my imperative attempts.  However, I will word my change
logs in the imperative as best I can in the future.

Thank you,
Quytelda Kahja

On Thu, Mar 1, 2018 at 12:54 PM, Tobin C. Harding <me@tobin.cc> wrote:
> On Thu, Mar 01, 2018 at 02:15:00PM +0300, Dan Carpenter wrote:
>> On Thu, Mar 01, 2018 at 05:37:21PM +1100, Tobin C. Harding wrote:
>> > On Wed, Feb 28, 2018 at 09:19:09PM -0800, Quytelda Kahja wrote:
>> > > The code that generates a WLAN capability mask is repeated in five
>> > > functions.  This change refactors that code into a new function, which is
>> > > called now in each of those functions.
>> >
>> > Perhaps in future something like:
>> >
>> > Code to generate the WLAN capability mask is duplicated five times
>> >
>> > Add helper function to generate WLAN capability mask, refactor code to
>> > use newly defined function.
>> >
>>
>> I honestly don't see the difference between that and what Quytelda
>> wrote?  I understood the original changelog just fine.
>>
>> regards,
>> dan carpenter
>
> I had a feeling that the sentiment of the suggestion I was trying to get
> at didn't come across, thanks for pointing it out.  I was intending to
> suggest not using sentences like this
>
>         > This change refactors that code into a new function, which is
>         > called now in each of those functions.
>
> And instead use, as suggested in submitting-patches.rst, imperative mood
>
>         Refactor code into new function ...
>
> FTR I find the English bits of kernel dev (and programming in general)
> the most difficult even though English is my first language.  I would
> like to write it better.
>
>
> Hope this helps,
> Tobin.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Thu, Mar 01, 2018 at 05:28:26PM -0800, Quytelda Kahja wrote:
> Tobin,
> I understand your point, and I've read submitting-patches.rst.  I made
> that wording choice because I was looking at some older commits that
> were worded like that.  I'm fairly new to the kernel workflow, so I
> was just trying to emulate something established, and it sounded less
> stilted than my imperative attempts.  However, I will word my change
> logs in the imperative as best I can in the future.

If you master the art of writing commit logs let me know - I'm chasing
that elusive goal myself.

Good luck,
Tobin.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Dan Carpenter]

And no top posting on email threads...  So don't you forget it.  :P

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Dan Carpenter]

There are so many rules for kernel developers to deal with.  Is it worse
to go over the 80 character limit or align the parameters properly?  Is
it OK to start the subject with a lower case letter?  I get in trouble
for using the wrong prefix but I'm the first person to patch a driver so
how on earth am I supposed to read into your @#$R@#$@#$@#$@ mind to see
what prefix you are going to want?

The imperitive thing is a good suggestion for people to think about but
it's not something that you should suggest to other people out of the
blue for no reason.  I've seen people do it for OPW and I'm like, "Eh...
I guess in that case maybe the intern will have to deal with some super
anal maintainer so they should know all the rules".  But generally,
unless the person asked you to tutor them about every single unpleasant
thing/maintainer they might have to deal then just let it be.  Otherwise
you risk becoming that unpleasant thing they have to deal with.

Don't lose track of the bigger picture which is "Can you understand the
changelog?"  There is no such thing as a perfect patch.  This patch has
a style violation which I overlooked because it just doesn't matter.

It's discouraging to be nagged at.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: ks7010: Factor out repeated code into function 'ks_wlan_cap()'.

[Tobin C. Harding]

On Fri, Mar 02, 2018 at 12:05:03PM +0300, Dan Carpenter wrote:
> There are so many rules for kernel developers to deal with.  Is it worse
> to go over the 80 character limit or align the parameters properly?  Is
> it OK to start the subject with a lower case letter?  I get in trouble
> for using the wrong prefix but I'm the first person to patch a driver so
> how on earth am I supposed to read into your @#$R@#$@#$@#$@ mind to see
> what prefix you are going to want?
> 
> The imperitive thing is a good suggestion for people to think about but
> it's not something that you should suggest to other people out of the
> blue for no reason.  I've seen people do it for OPW and I'm like, "Eh...
> I guess in that case maybe the intern will have to deal with some super
> anal maintainer so they should know all the rules".  But generally,
> unless the person asked you to tutor them about every single unpleasant
> thing/maintainer they might have to deal then just let it be.  Otherwise
> you risk becoming that unpleasant thing they have to deal with.
> 
> Don't lose track of the bigger picture which is "Can you understand the
> changelog?"  There is no such thing as a perfect patch.  This patch has
> a style violation which I overlooked because it just doesn't matter.
> 
> It's discouraging to be nagged at.
> 
> regards,
> dan carpenter

No worries Dan, thanks for pointing this out.  I actually don't enjoy
reviewing staging patches.  I guessed no one really likes reviewing them
but I feel that we should all give back a little and since I don't have
all that much to give I figured reviewing staging patches was a good way
to do something that others probably don't like doing.  I'd like to be
able to do the reviews in a way that is encouraging to new devs but like
most I only have my own likes/dislikes to go on.  I liked having every
detail of my first X patches picked apart because it meant I was
confident then to go on and try to do patches outside of staging -
others may vary and I definitely agree with you that no one likes to be
nagged so it's a fine line.  If you have any suggestions for me as to
how to be more useful in reviewing staging patches please do share
them.  So far I only have one clue as to how to go about it and that was
this from Greg KH 'Just be polite'.  There certainly is a lot to learn
getting started with kernel work but for me anyways staging is doing
really well at smoothing the process and that is mainly thanks to Greg
and yourself in doing the reviews.  If I'm totally wrong and you two
really love doing it then I will happily bow out and not touch another
staging patch (please say so if this is the case).  If I can help to
carry the load then I'm happy to do so because you two helped me a lot.
Please continue to pick me up if I do wrongs - I'm super happy when you
do because then I've got a chance of doing it right next time :)


thanks,
Tobin.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/5] staging: ks7010: Replace local capability constants with kernel constants.

[Quytelda Kahja]

This driver defined constants BSS_CAP_* to represent WLAN capability
codes; however, these constants are already defined in the header
'linux/ieee80211.h' as WLAN_CAPABILITY_*.  This change removes the locally
defined constants and substitutes the kernel's constants.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
Reviewed-by: Tobin C. Harding <me@tobin.cc>

---
 drivers/staging/ks7010/ks_hostif.c   |  8 ++++----
 drivers/staging/ks7010/ks_hostif.h   | 10 ----------
 drivers/staging/ks7010/ks_wlan_net.c |  6 +++---
 3 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
index 4d11627a3d72..f425975fbcbc 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -1386,14 +1386,14 @@ static __le16 ks_wlan_cap(struct ks_wlan_private *priv)
 	u16 capability = 0x0000;
 
 	if (priv->reg.preamble == SHORT_PREAMBLE) {
-		capability |= BSS_CAP_SHORT_PREAMBLE;
+		capability |= WLAN_CAPABILITY_SHORT_PREAMBLE;
 	}
 
-	capability &= ~(BSS_CAP_PBCC);	/* pbcc not support */
+	capability &= ~(WLAN_CAPABILITY_PBCC);	/* pbcc not support */
 
 	if (priv->reg.phy_type != D_11B_ONLY_MODE) {
-		capability |= BSS_CAP_SHORT_SLOT_TIME;
-		capability &= ~(BSS_CAP_DSSS_OFDM);
+		capability |= WLAN_CAPABILITY_SHORT_SLOT_TIME;
+		capability &= ~(WLAN_CAPABILITY_DSSS_OFDM);
 	}
 
 	return cpu_to_le16((uint16_t)capability);
diff --git a/drivers/staging/ks7010/ks_hostif.h b/drivers/staging/ks7010/ks_hostif.h
index b46aa94c0d48..8f08e1e58991 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -284,16 +284,6 @@ struct ap_info_t {
 	u8 pad0;	/* +09 */
 	__le16 beacon_period;	/* +10 */
 	__le16 capability;	/* +12 */
-#define BSS_CAP_ESS             BIT(0)
-#define BSS_CAP_IBSS            BIT(1)
-#define BSS_CAP_CF_POLABLE      BIT(2)
-#define BSS_CAP_CF_POLL_REQ     BIT(3)
-#define BSS_CAP_PRIVACY         BIT(4)
-#define BSS_CAP_SHORT_PREAMBLE  BIT(5)
-#define BSS_CAP_PBCC            BIT(6)
-#define BSS_CAP_CHANNEL_AGILITY BIT(7)
-#define BSS_CAP_SHORT_SLOT_TIME BIT(10)
-#define BSS_CAP_DSSS_OFDM       BIT(13)
 	u8 frame_type;	/* +14 */
 	u8 ch_info;	/* +15 */
 #define FRAME_TYPE_BEACON	0x80
diff --git a/drivers/staging/ks7010/ks_wlan_net.c b/drivers/staging/ks7010/ks_wlan_net.c
index e48c55769c94..91acf87ab8dd 100644
--- a/drivers/staging/ks7010/ks_wlan_net.c
+++ b/drivers/staging/ks7010/ks_wlan_net.c
@@ -1366,8 +1366,8 @@ static inline char *ks_wlan_translate_scan(struct net_device *dev,
 	/* Add mode */
 	iwe.cmd = SIOCGIWMODE;
 	capabilities = ap->capability;
-	if (capabilities & (BSS_CAP_ESS | BSS_CAP_IBSS)) {
-		if (capabilities & BSS_CAP_ESS)
+	if (capabilities & (WLAN_CAPABILITY_ESS | WLAN_CAPABILITY_IBSS)) {
+		if (capabilities & WLAN_CAPABILITY_ESS)
 			iwe.u.mode = IW_MODE_INFRA;
 		else
 			iwe.u.mode = IW_MODE_ADHOC;
@@ -1396,7 +1396,7 @@ static inline char *ks_wlan_translate_scan(struct net_device *dev,
 
 	/* Add encryption capability */
 	iwe.cmd = SIOCGIWENCODE;
-	if (capabilities & BSS_CAP_PRIVACY)
+	if (capabilities & WLAN_CAPABILITY_PRIVACY)
 		iwe.u.data.flags = IW_ENCODE_ENABLED | IW_ENCODE_NOKEY;
 	else
 		iwe.u.data.flags = IW_ENCODE_DISABLED;
-- 
2.16.2

[PATCH 5/5] staging: ks7010: Replace local frame type constants with kernel constants.

[Quytelda Kahja]

This driver defined constants FRAME_TYPE_* to represent frame control
field codes; however, these constants are already defined in the header
'linux/ieee80211.h' as  IEEE80211_STYPE_*.  This change removes the locally
defined constants and substitutes the kernel's constants.

Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
Reviewed-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/staging/ks7010/ks_hostif.c | 2 +-
 drivers/staging/ks7010/ks_hostif.h | 6 ------
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
index f425975fbcbc..7935ba56bb1d 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -847,7 +847,7 @@ void hostif_scan_indication(struct ks_wlan_private *priv)
 				   priv->aplist.ap[i].bssid, ETH_ALEN) != 0)
 				continue;
 
-			if (ap_info->frame_type == FRAME_TYPE_PROBE_RESP)
+			if (ap_info->frame_type == IEEE80211_STYPE_PROBE_RESP)
 				get_ap_information(priv, ap_info,
 						   &priv->aplist.ap[i]);
 			return;
diff --git a/drivers/staging/ks7010/ks_hostif.h b/drivers/staging/ks7010/ks_hostif.h
index 8f08e1e58991..166d83e4885c 100644
--- a/drivers/staging/ks7010/ks_hostif.h
+++ b/drivers/staging/ks7010/ks_hostif.h
@@ -286,8 +286,6 @@ struct ap_info_t {
 	__le16 capability;	/* +12 */
 	u8 frame_type;	/* +14 */
 	u8 ch_info;	/* +15 */
-#define FRAME_TYPE_BEACON	0x80
-#define FRAME_TYPE_PROBE_RESP	0x50
 	__le16 body_size;	/* +16 */
 	u8 body[1024];	/* +18 */
 	/* +1032 */
@@ -465,8 +463,6 @@ struct last_associate_t {
 
 struct association_request_t {
 	u8 type;
-#define FRAME_TYPE_ASSOC_REQ	0x00
-#define FRAME_TYPE_REASSOC_REQ	0x20
 	u8 pad;
 	__le16 capability;
 	__le16 listen_interval;
@@ -476,8 +472,6 @@ struct association_request_t {
 
 struct association_response_t {
 	u8 type;
-#define FRAME_TYPE_ASSOC_RESP	0x10
-#define FRAME_TYPE_REASSOC_RESP	0x30
 	u8 pad;
 	__le16 capability;
 	__le16 status;
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

[Tobin C. Harding]

On Wed, Feb 28, 2018 at 09:19:07PM -0800, Quytelda Kahja wrote:
> The case statement in get_ap_information() should not use literal integers
> to parse information element IDs when these values are provided by name
> in 'enum ieee80211_eid' in the header 'linux/ieee80211.h'.

Nice.  Magic number removal, I like it.

> Signed-off-by: Quytelda Kahja <quytelda@tamalin.org>
> ---
>  drivers/staging/ks7010/ks_hostif.c | 31 +++++++++++++++----------------
>  drivers/staging/ks7010/ks_hostif.h |  1 +
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/ks7010/ks_hostif.c b/drivers/staging/ks7010/ks_hostif.c
> index 74a08417bd0b..67cf32433023 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -251,9 +251,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
>  	offset = 0;
>  
>  	while (bsize > offset) {
> -		/* DPRINTK(4, "Element ID=%d\n",*bp); */

nit: Probably shouldn't remove debugging output in the same patch as
doing magic number removal.  (super nit-picky though.)

> -		switch (*bp) {
> -		case 0:	/* ssid */
> +		switch (*bp) { /* Information Element ID */
> +		case WLAN_EID_SSID:
>  			if (*(bp + 1) <= SSID_MAX_SIZE) {
>  				ap->ssid.size = *(bp + 1);
>  			} else {
> @@ -263,8 +262,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
>  			}
>  			memcpy(ap->ssid.body, bp + 2, ap->ssid.size);
>  			break;
> -		case 1:	/* rate */
> -		case 50:	/* ext rate */
> +		case WLAN_EID_SUPP_RATES:
> +		case WLAN_EID_EXT_SUPP_RATES:
>  			if ((*(bp + 1) + ap->rate_set.size) <=
>  			    RATE_SET_MAX_SIZE) {
>  				memcpy(&ap->rate_set.body[ap->rate_set.size],
> @@ -280,9 +279,9 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
>  				    (RATE_SET_MAX_SIZE - ap->rate_set.size);
>  			}
>  			break;
> -		case 3:	/* DS parameter */
> +		case WLAN_EID_DS_PARAMS:
>  			break;
> -		case 48:	/* RSN(WPA2) */
> +		case WLAN_EID_RSN:
>  			ap->rsn_ie.id = *bp;
>  			if (*(bp + 1) <= RSN_IE_BODY_MAX) {
>  				ap->rsn_ie.size = *(bp + 1);
> @@ -293,8 +292,8 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
>  			}
>  			memcpy(ap->rsn_ie.body, bp + 2, ap->rsn_ie.size);
>  			break;
> -		case 221:	/* WPA */
> -			if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) {	/* WPA OUI check */
> +		case WLAN_EID_VENDOR_SPECIFIC: /* WPA */
> +			if (memcmp(bp + 2, "\x00\x50\xf2\x01", 4) == 0) { /* WPA OUI check */

Shouldn't have this line (unnecessary white space change)

>  				ap->wpa_ie.id = *bp;
>  				if (*(bp + 1) <= RSN_IE_BODY_MAX) {
>  					ap->wpa_ie.size = *(bp + 1);
> @@ -309,18 +308,18 @@ int get_ap_information(struct ks_wlan_private *priv, struct ap_info_t *ap_info,
>  			}
>  			break;
>  
> -		case 2:	/* FH parameter */
> -		case 4:	/* CF parameter */
> -		case 5:	/* TIM */
> -		case 6:	/* IBSS parameter */
> -		case 7:	/* Country */
> -		case 42:	/* ERP information */
> -		case 47:	/* Reserve ID 47 Broadcom AP */
> +		case WLAN_EID_FH_PARAMS:
> +		case WLAN_EID_CF_PARAMS:
> +		case WLAN_EID_TIM:
> +		case WLAN_EID_IBSS_PARAMS:
> +		case WLAN_EID_COUNTRY:
> +		case WLAN_EID_ERP_INFO:
>  			break;
>  		default:
>  			DPRINTK(4, "unknown Element ID=%d\n", *bp);
>  			break;
>  		}
> +

FTR try not to include unrelated whitespace changes.  I see you changed
the case statement but the whitespace is after it and not really
related.  Again quite nit-picky but I'm guessing you are in staging to
learn so might as well learn it now than when you are patching the core
kernel :)


Good work,
Tobin.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/5] staging: ks7010: Use constants from ieee80211_eid instead of literal ints.

[Tobin C. Harding]

On Wed, Feb 28, 2018 at 09:19:07PM -0800, Quytelda Kahja wrote:
...

I would normally respond to the cover letter but here goes.

Reviewed-by: Tobin C. Harding <me@tobin.cc>

thanks,
Tobin.