Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1243954 > unrolled thread
| Started by | Punit Vara <punitvara@gmail.com> |
|---|---|
| First post | 2015-10-10 19:00 +0200 |
| Last post | 2015-10-11 19:30 +0200 |
| Articles | 11 — 6 participants |
Back to article view | Back to linux.kernel
[PATCH 0/4] Staging: rtl8712: Fix coding style warnings Punit Vara <punitvara@gmail.com> - 2015-10-10 19:00 +0200
[PATCH 3/4] Staging: rtl8712: Coding style warnings fix for block comments Punit Vara <punitvara@gmail.com> - 2015-10-10 19:00 +0200
[PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() Punit Vara <punitvara@gmail.com> - 2015-10-10 19:00 +0200
Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() Larry Finger <Larry.Finger@lwfinger.net> - 2015-10-10 19:40 +0200
Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() punit vara <punitvara@gmail.com> - 2015-10-10 21:50 +0200
Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() Julia Lawall <julia.lawall@lip6.fr> - 2015-10-10 22:00 +0200
Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() punit vara <punitvara@gmail.com> - 2015-10-10 22:30 +0200
Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() punit vara <punitvara@gmail.com> - 2015-10-10 22:30 +0200
Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() kbuild test robot <lkp@intel.com> - 2015-10-11 02:40 +0200
Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() punit vara <punitvara@gmail.com> - 2015-10-11 13:20 +0200
Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() Joe Perches <joe@perches.com> - 2015-10-11 19:30 +0200
| From | Punit Vara <punitvara@gmail.com> |
|---|---|
| Date | 2015-10-10 19:00 +0200 |
| Subject | [PATCH 0/4] Staging: rtl8712: Fix coding style warnings |
| Message-ID | <qi5F0-6Nw-11@gated-at.bofh.it> |
This set of patch fixes following warnings reported by checkpatch.pl:
-Prefer eth_broadcast_addr() over memset()
-Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()
-Block comments use * on subsequent lines
-Block comments use a trailing */ on a separate line
-Comparisons should place the constant on the right side of the test
Punit Vara (4):
Staging: rtl8712: Fix warning prefer eth_broadcast_addr() over
memset()
Staging: rtl8712: Use ether_addr_equal() over memcmp()
Staging: rtl8712: Coding style warnings fix for block comments
Staging: rtl8712: fix warning for placing constant on the right side
of test
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 47 ++++++++++++++-------------
1 file changed, 25 insertions(+), 22 deletions(-)
--
2.5.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Punit Vara <punitvara@gmail.com> |
|---|---|
| Date | 2015-10-10 19:00 +0200 |
| Subject | [PATCH 3/4] Staging: rtl8712: Coding style warnings fix for block comments |
| Message-ID | <qi5F1-6Nw-19@gated-at.bofh.it> |
| In reply to | #1243954 |
This patch is to the rtl871x_ioctl_linux.c file that fixes up following
warnings reported by checkpatch.pl :
-Block comments use * on subsequent lines
-Block comments use a trailing */ on a separate line
Signed-off-by: Punit Vara <punitvara@gmail.com>
---
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index 1c9092e..fc1028a 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -797,13 +797,13 @@ static int r871x_wx_set_pmkid(struct net_device *dev,
int intReturn = false;
/*
- There are the BSSID information in the bssid.sa_data array.
- If cmd is IW_PMKSA_FLUSH, it means the wpa_supplicant wants to clear
- all the PMKID information. If cmd is IW_PMKSA_ADD, it means the
- wpa_supplicant wants to add a PMKID/BSSID to driver.
- If cmd is IW_PMKSA_REMOVE, it means the wpa_supplicant wants to
- remove a PMKID/BSSID from driver.
-*/
+ * There are the BSSID information in the bssid.sa_data array.
+ * If cmd is IW_PMKSA_FLUSH, it means the wpa_supplicant wants to clear
+ * all the PMKID information. If cmd is IW_PMKSA_ADD, it means the
+ * wpa_supplicant wants to add a PMKID/BSSID to driver.
+ * If cmd is IW_PMKSA_REMOVE, it means the wpa_supplicant wants to
+ * remove a PMKID/BSSID from driver.
+ */
if (pPMK == NULL)
return -EINVAL;
memcpy(strIssueBssid, pPMK->bssid.sa_data, ETH_ALEN);
@@ -818,7 +818,8 @@ static int r871x_wx_set_pmkid(struct net_device *dev,
if (!memcmp(psecuritypriv->PMKIDList[j].Bssid,
strIssueBssid, ETH_ALEN)) {
/* BSSID is matched, the same AP => rewrite
- * with new PMKID. */
+ * with new PMKID.
+ */
netdev_info(dev, "r8712u: %s: BSSID exists in the PMKList.\n",
__func__);
memcpy(psecuritypriv->PMKIDList[j].PMKID,
@@ -850,7 +851,8 @@ static int r871x_wx_set_pmkid(struct net_device *dev,
if (!memcmp(psecuritypriv->PMKIDList[j].Bssid,
strIssueBssid, ETH_ALEN)) {
/* BSSID is matched, the same AP => Remove
- * this PMKID information and reset it. */
+ * this PMKID information and reset it.
+ */
eth_zero_addr(psecuritypriv->PMKIDList[j].Bssid);
psecuritypriv->PMKIDList[j].bUsed = false;
break;
@@ -1587,7 +1589,8 @@ static int r8711_wx_set_enc(struct net_device *dev,
} else {
wep.KeyLength = 0;
if (keyindex_provided == 1) { /* set key_id only, no given
- * KeyMaterial(erq->length==0).*/
+ * KeyMaterial(erq->length==0).
+ */
padapter->securitypriv.PrivacyKeyIndex = key;
switch (padapter->securitypriv.DefKeylen[key]) {
case 5:
@@ -2240,7 +2243,8 @@ static iw_handler r8711_handlers[] = {
r8711_wx_set_wap, /* SIOCSIWAP */
r8711_wx_get_wap, /* SIOCGIWAP */
r871x_wx_set_mlme, /* request MLME operation;
- * uses struct iw_mlme */
+ * uses struct iw_mlme
+ */
dummy, /* SIOCGIWAPLIST -- deprecated */
r8711_wx_set_scan, /* SIOCSIWSCAN */
r8711_wx_get_scan, /* SIOCGIWSCAN */
--
2.5.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Punit Vara <punitvara@gmail.com> |
|---|---|
| Date | 2015-10-10 19:00 +0200 |
| Subject | [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() |
| Message-ID | <qi5F1-6Nw-17@gated-at.bofh.it> |
| In reply to | #1243954 |
This patch is to the rtl871x_ioctl_linux.c file that fixes up following
warning reported by checkpatch.pl :
-Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()
bssid has datatype u8 and pnetwork->network.MacAddress has data type
unsigned char.
Signed-off-by: Punit Vara <punitvara@gmail.com>
---
drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index 2ba055d..1c9092e 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -2007,7 +2007,7 @@ static int r871x_get_ap_info(struct net_device *dev,
return -EINVAL;
}
netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
- if (!memcmp(bssid, pnetwork->network.MacAddress, ETH_ALEN)) {
+ if (!ether_addr_equal(bssid, pnetwork->network)) {
/* BSSID match, then check if supporting wpa/wpa2 */
pbuf = r8712_get_wpa_ie(&pnetwork->network.IEs[12],
&wpa_ielen, pnetwork->network.IELength-12);
--
2.5.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Larry Finger <Larry.Finger@lwfinger.net> |
|---|---|
| Date | 2015-10-10 19:40 +0200 |
| Subject | Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() |
| Message-ID | <qi6hH-7MP-15@gated-at.bofh.it> |
| In reply to | #1243956 |
On 10/10/2015 11:58 AM, Punit Vara wrote:
> This patch is to the rtl871x_ioctl_linux.c file that fixes up following
> warning reported by checkpatch.pl :
>
> -Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()
>
> bssid has datatype u8 and pnetwork->network.MacAddress has data type
> unsigned char.
>
> Signed-off-by: Punit Vara <punitvara@gmail.com>
> ---
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> index 2ba055d..1c9092e 100644
> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> @@ -2007,7 +2007,7 @@ static int r871x_get_ap_info(struct net_device *dev,
> return -EINVAL;
> }
> netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
> - if (!memcmp(bssid, pnetwork->network.MacAddress, ETH_ALEN)) {
> + if (!ether_addr_equal(bssid, pnetwork->network)) {
> /* BSSID match, then check if supporting wpa/wpa2 */
> pbuf = r8712_get_wpa_ie(&pnetwork->network.IEs[12],
> &wpa_ielen, pnetwork->network.IELength-12);
>
The types of the variables are not what is important - it is the alignment!
I suggest that you read the definition of ether_addr_equal() and consider what
happens if either of the two addresses is not aligned to u16!
You also have a logic error. Routine memcmp() returns zero when the two
arguments are equal. Thus !memcmp() will be true when they are equal. Routine
ether_addr_equal() returns true when the arguments are equal. Whenever !memcmp()
is correct and the arguments are aligned, then you need to replace it with
ether_addr_equal().
The checkpatch script is a good tool for locating things to be changed; however,
if the change is other than cosmetic, you *MUST THINK* about what is happening.
Yes, rtl8712 is ugly code, but it works. Please do not break it.
NACK.
Larry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | punit vara <punitvara@gmail.com> |
|---|---|
| Date | 2015-10-10 21:50 +0200 |
| Subject | Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() |
| Message-ID | <qi8jw-2fO-1@gated-at.bofh.it> |
| In reply to | #1243965 |
On Sat, Oct 10, 2015 at 11:04 PM, Larry Finger
<Larry.Finger@lwfinger.net> wrote:
> On 10/10/2015 11:58 AM, Punit Vara wrote:
>>
>> This patch is to the rtl871x_ioctl_linux.c file that fixes up following
>> warning reported by checkpatch.pl :
>>
>> -Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()
>>
>> bssid has datatype u8 and pnetwork->network.MacAddress has data type
>> unsigned char.
>>
>> Signed-off-by: Punit Vara <punitvara@gmail.com>
>> ---
>> drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> index 2ba055d..1c9092e 100644
>> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> @@ -2007,7 +2007,7 @@ static int r871x_get_ap_info(struct net_device *dev,
>> return -EINVAL;
>> }
>> netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
>> - if (!memcmp(bssid, pnetwork->network.MacAddress,
>> ETH_ALEN)) {
>> + if (!ether_addr_equal(bssid, pnetwork->network)) {
>> /* BSSID match, then check if supporting wpa/wpa2
>> */
>> pbuf =
>> r8712_get_wpa_ie(&pnetwork->network.IEs[12],
>> &wpa_ielen, pnetwork->network.IELength-12);
>>
>
> The types of the variables are not what is important - it is the alignment!
>
> I suggest that you read the definition of ether_addr_equal() and consider
> what happens if either of the two addresses is not aligned to u16!
>
> You also have a logic error. Routine memcmp() returns zero when the two
> arguments are equal. Thus !memcmp() will be true when they are equal.
> Routine ether_addr_equal() returns true when the arguments are equal.
> Whenever !memcmp() is correct and the arguments are aligned, then you need
> to replace it with ether_addr_equal().
>
> The checkpatch script is a good tool for locating things to be changed;
> however, if the change is other than cosmetic, you *MUST THINK* about what
> is happening. Yes, rtl8712 is ugly code, but it works. Please do not break
> it.
>
> NACK.
>
> Larry
>
@Larry I am thankful for your high quality feedback.
if both address are aligned to u16 then we should use
ether_addr_equal() if not then ether_addr_equal_unaligned().
Both return true if addr1 and addr2 matches. If I am understand
correctly bssid and MacAdress is not aligned to u16 .So right
correction would be
if(ether_addr_equal_unaligned(bssid, pnetwork->network))
right ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2015-10-10 22:00 +0200 |
| Subject | Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() |
| Message-ID | <qi8td-2ri-25@gated-at.bofh.it> |
| In reply to | #1243988 |
On Sun, 11 Oct 2015, punit vara wrote:
> On Sat, Oct 10, 2015 at 11:04 PM, Larry Finger
> <Larry.Finger@lwfinger.net> wrote:
> > On 10/10/2015 11:58 AM, Punit Vara wrote:
> >>
> >> This patch is to the rtl871x_ioctl_linux.c file that fixes up following
> >> warning reported by checkpatch.pl :
> >>
> >> -Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()
> >>
> >> bssid has datatype u8 and pnetwork->network.MacAddress has data type
> >> unsigned char.
> >>
> >> Signed-off-by: Punit Vara <punitvara@gmail.com>
> >> ---
> >> drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> index 2ba055d..1c9092e 100644
> >> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> @@ -2007,7 +2007,7 @@ static int r871x_get_ap_info(struct net_device *dev,
> >> return -EINVAL;
> >> }
> >> netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
> >> - if (!memcmp(bssid, pnetwork->network.MacAddress,
> >> ETH_ALEN)) {
> >> + if (!ether_addr_equal(bssid, pnetwork->network)) {
> >> /* BSSID match, then check if supporting wpa/wpa2
> >> */
> >> pbuf =
> >> r8712_get_wpa_ie(&pnetwork->network.IEs[12],
> >> &wpa_ielen, pnetwork->network.IELength-12);
> >>
> >
> > The types of the variables are not what is important - it is the alignment!
> >
> > I suggest that you read the definition of ether_addr_equal() and consider
> > what happens if either of the two addresses is not aligned to u16!
> >
> > You also have a logic error. Routine memcmp() returns zero when the two
> > arguments are equal. Thus !memcmp() will be true when they are equal.
> > Routine ether_addr_equal() returns true when the arguments are equal.
> > Whenever !memcmp() is correct and the arguments are aligned, then you need
> > to replace it with ether_addr_equal().
> >
> > The checkpatch script is a good tool for locating things to be changed;
> > however, if the change is other than cosmetic, you *MUST THINK* about what
> > is happening. Yes, rtl8712 is ugly code, but it works. Please do not break
> > it.
> >
> > NACK.
> >
> > Larry
> >
>
> @Larry I am thankful for your high quality feedback.
> if both address are aligned to u16 then we should use
> ether_addr_equal() if not then ether_addr_equal_unaligned().
>
> Both return true if addr1 and addr2 matches. If I am understand
> correctly bssid and MacAdress is not aligned to u16 .So right
> correction would be
> if(ether_addr_equal_unaligned(bssid, pnetwork->network))
>
> right ?
You can use pahole to check the alignment. Use git log to find other
patches that mention the use of this tool to see what information to
provide in the commit message.
julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | punit vara <punitvara@gmail.com> |
|---|---|
| Date | 2015-10-10 22:30 +0200 |
| Subject | Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() |
| Message-ID | <qi8We-3eU-5@gated-at.bofh.it> |
| In reply to | #1243995 |
On Sun, Oct 11, 2015 at 1:20 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Sun, 11 Oct 2015, punit vara wrote:
>
>> On Sat, Oct 10, 2015 at 11:04 PM, Larry Finger
>> <Larry.Finger@lwfinger.net> wrote:
>> > On 10/10/2015 11:58 AM, Punit Vara wrote:
>> >>
>> >> This patch is to the rtl871x_ioctl_linux.c file that fixes up following
>> >> warning reported by checkpatch.pl :
>> >>
>> >> -Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()
>> >>
>> >> bssid has datatype u8 and pnetwork->network.MacAddress has data type
>> >> unsigned char.
>> >>
>> >> Signed-off-by: Punit Vara <punitvara@gmail.com>
>> >> ---
>> >> drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> >> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> >> index 2ba055d..1c9092e 100644
>> >> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> >> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> >> @@ -2007,7 +2007,7 @@ static int r871x_get_ap_info(struct net_device *dev,
>> >> return -EINVAL;
>> >> }
>> >> netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
>> >> - if (!memcmp(bssid, pnetwork->network.MacAddress,
>> >> ETH_ALEN)) {
>> >> + if (!ether_addr_equal(bssid, pnetwork->network)) {
>> >> /* BSSID match, then check if supporting wpa/wpa2
>> >> */
>> >> pbuf =
>> >> r8712_get_wpa_ie(&pnetwork->network.IEs[12],
>> >> &wpa_ielen, pnetwork->network.IELength-12);
>> >>
>> >
>> > The types of the variables are not what is important - it is the alignment!
>> >
>> > I suggest that you read the definition of ether_addr_equal() and consider
>> > what happens if either of the two addresses is not aligned to u16!
>> >
>> > You also have a logic error. Routine memcmp() returns zero when the two
>> > arguments are equal. Thus !memcmp() will be true when they are equal.
>> > Routine ether_addr_equal() returns true when the arguments are equal.
>> > Whenever !memcmp() is correct and the arguments are aligned, then you need
>> > to replace it with ether_addr_equal().
>> >
>> > The checkpatch script is a good tool for locating things to be changed;
>> > however, if the change is other than cosmetic, you *MUST THINK* about what
>> > is happening. Yes, rtl8712 is ugly code, but it works. Please do not break
>> > it.
>> >
>> > NACK.
>> >
>> > Larry
>> >
>>
>> @Larry I am thankful for your high quality feedback.
>> if both address are aligned to u16 then we should use
>> ether_addr_equal() if not then ether_addr_equal_unaligned().
>>
>> Both return true if addr1 and addr2 matches. If I am understand
>> correctly bssid and MacAdress is not aligned to u16 .So right
>> correction would be
>> if(ether_addr_equal_unaligned(bssid, pnetwork->network))
>>
>> right ?
>
> You can use pahole to check the alignment. Use git log to find other
> patches that mention the use of this tool to see what information to
> provide in the commit message.
>
> julia
Thanks Julia I didn't know about that dwarf utility I will go through
it .sorry I have forgot to write MacAddress.
So here solution could be either
ether_addr_equal(bssid, pnetwork->network.MacAddress) or
ether_addr_equal_unaligned(bssid->network.MacAddress)
I will learn debugging with help of pahole and again send the patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | punit vara <punitvara@gmail.com> |
|---|---|
| Date | 2015-10-10 22:30 +0200 |
| Subject | Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() |
| Message-ID | <qi8We-3eU-9@gated-at.bofh.it> |
| In reply to | #1244006 |
On Sun, Oct 11, 2015 at 1:52 AM, punit vara <punitvara@gmail.com> wrote:
> On Sun, Oct 11, 2015 at 1:20 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>>
>>
>> On Sun, 11 Oct 2015, punit vara wrote:
>>
>>> On Sat, Oct 10, 2015 at 11:04 PM, Larry Finger
>>> <Larry.Finger@lwfinger.net> wrote:
>>> > On 10/10/2015 11:58 AM, Punit Vara wrote:
>>> >>
>>> >> This patch is to the rtl871x_ioctl_linux.c file that fixes up following
>>> >> warning reported by checkpatch.pl :
>>> >>
>>> >> -Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()
>>> >>
>>> >> bssid has datatype u8 and pnetwork->network.MacAddress has data type
>>> >> unsigned char.
>>> >>
>>> >> Signed-off-by: Punit Vara <punitvara@gmail.com>
>>> >> ---
>>> >> drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 2 +-
>>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>>> >> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>>> >> index 2ba055d..1c9092e 100644
>>> >> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>>> >> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>>> >> @@ -2007,7 +2007,7 @@ static int r871x_get_ap_info(struct net_device *dev,
>>> >> return -EINVAL;
>>> >> }
>>> >> netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
>>> >> - if (!memcmp(bssid, pnetwork->network.MacAddress,
>>> >> ETH_ALEN)) {
>>> >> + if (!ether_addr_equal(bssid, pnetwork->network)) {
>>> >> /* BSSID match, then check if supporting wpa/wpa2
>>> >> */
>>> >> pbuf =
>>> >> r8712_get_wpa_ie(&pnetwork->network.IEs[12],
>>> >> &wpa_ielen, pnetwork->network.IELength-12);
>>> >>
>>> >
>>> > The types of the variables are not what is important - it is the alignment!
>>> >
>>> > I suggest that you read the definition of ether_addr_equal() and consider
>>> > what happens if either of the two addresses is not aligned to u16!
>>> >
>>> > You also have a logic error. Routine memcmp() returns zero when the two
>>> > arguments are equal. Thus !memcmp() will be true when they are equal.
>>> > Routine ether_addr_equal() returns true when the arguments are equal.
>>> > Whenever !memcmp() is correct and the arguments are aligned, then you need
>>> > to replace it with ether_addr_equal().
>>> >
>>> > The checkpatch script is a good tool for locating things to be changed;
>>> > however, if the change is other than cosmetic, you *MUST THINK* about what
>>> > is happening. Yes, rtl8712 is ugly code, but it works. Please do not break
>>> > it.
>>> >
>>> > NACK.
>>> >
>>> > Larry
>>> >
>>>
>>> @Larry I am thankful for your high quality feedback.
>>> if both address are aligned to u16 then we should use
>>> ether_addr_equal() if not then ether_addr_equal_unaligned().
>>>
>>> Both return true if addr1 and addr2 matches. If I am understand
>>> correctly bssid and MacAdress is not aligned to u16 .So right
>>> correction would be
>>> if(ether_addr_equal_unaligned(bssid, pnetwork->network))
>>>
>>> right ?
>>
>> You can use pahole to check the alignment. Use git log to find other
>> patches that mention the use of this tool to see what information to
>> provide in the commit message.
>>
>> julia
>
> Thanks Julia I didn't know about that dwarf utility I will go through
> it .sorry I have forgot to write MacAddress.
>
> So here solution could be either
>
> ether_addr_equal(bssid, pnetwork->network.MacAddress) or
> ether_addr_equal_unaligned(bssid->network.MacAddress)
>
> I will learn debugging with help of pahole and again send the patch.
Thanks Julia I didn't know about that dwarves utility I will go through
it .sorry I have forgot to write MacAddress.
So here solution could be either
ether_addr_equal(bssid, pnetwork->network.MacAddress) or
ether_addr_equal_unaligned(bssid, pnetwork->network.MacAddress)
I will learn debugging with help of pahole and again send the patch.
Sorry for inconvenience previous mail send before adding because my
laptop was hanged.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | kbuild test robot <lkp@intel.com> |
|---|---|
| Date | 2015-10-11 02:40 +0200 |
| Subject | Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() |
| Message-ID | <qicQ9-oE-1@gated-at.bofh.it> |
| In reply to | #1243956 |
Hi Punit,
[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:227:15: sparse: cast to restricted __le16
>> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54: sparse: incorrect type in argument 2 (different base types)
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54: expected unsigned char const [usertype] *addr2
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54: got struct wlan_bssid_ex network
drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'r871x_get_ap_info':
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:32: error: incompatible type for argument 2 of 'ether_addr_equal'
if (!ether_addr_equal(bssid, pnetwork->network)) {
^
In file included from drivers/staging/rtl8712/osdep_service.h:39:0,
from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:32:
include/linux/etherdevice.h:310:20: note: expected 'const u8 * {aka const unsigned char *}' but argument is of type 'struct wlan_bssid_ex'
static inline bool ether_addr_equal(const u8 *addr1, const u8 *addr2)
^
vim +2010 drivers/staging/rtl8712/rtl871x_ioctl_linux.c
1994 return -EINVAL;
1995 spin_lock_irqsave(&(pmlmepriv->scanned_queue.lock), irqL);
1996 phead = &queue->queue;
1997 plist = phead->next;
1998 while (1) {
1999 if (end_of_queue_search(phead, plist) == true)
2000 break;
2001 pnetwork = LIST_CONTAINOR(plist, struct wlan_network, list);
2002 if (hwaddr_aton_i(data, bssid)) {
2003 netdev_info(dev, "r8712u: Invalid BSSID '%s'.\n",
2004 (u8 *)data);
2005 spin_unlock_irqrestore(&(pmlmepriv->scanned_queue.lock),
2006 irqL);
2007 return -EINVAL;
2008 }
2009 netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
> 2010 if (!ether_addr_equal(bssid, pnetwork->network)) {
2011 /* BSSID match, then check if supporting wpa/wpa2 */
2012 pbuf = r8712_get_wpa_ie(&pnetwork->network.IEs[12],
2013 &wpa_ielen, pnetwork->network.IELength-12);
2014 if (pbuf && (wpa_ielen > 0)) {
2015 pdata->flags = 1;
2016 break;
2017 }
2018 pbuf = r8712_get_wpa2_ie(&pnetwork->network.IEs[12],
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | punit vara <punitvara@gmail.com> |
|---|---|
| Date | 2015-10-11 13:20 +0200 |
| Subject | Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() |
| Message-ID | <qimPw-6GL-19@gated-at.bofh.it> |
| In reply to | #1244044 |
On Sun, Oct 11, 2015 at 6:07 AM, kbuild test robot <lkp@intel.com> wrote:
> Hi Punit,
>
> [auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]
>
> reproduce:
> # apt-get install sparse
> make ARCH=x86_64 allmodconfig
> make C=1 CF=-D__CHECK_ENDIAN__
>
>
> sparse warnings: (new ones prefixed by >>)
>
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:227:15: sparse: cast to restricted __le16
>>> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54: sparse: incorrect type in argument 2 (different base types)
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54: expected unsigned char const [usertype] *addr2
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:54: got struct wlan_bssid_ex network
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'r871x_get_ap_info':
> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:2010:32: error: incompatible type for argument 2 of 'ether_addr_equal'
> if (!ether_addr_equal(bssid, pnetwork->network)) {
> ^
> In file included from drivers/staging/rtl8712/osdep_service.h:39:0,
> from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:32:
> include/linux/etherdevice.h:310:20: note: expected 'const u8 * {aka const unsigned char *}' but argument is of type 'struct wlan_bssid_ex'
> static inline bool ether_addr_equal(const u8 *addr1, const u8 *addr2)
> ^
>
> vim +2010 drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>
> 1994 return -EINVAL;
> 1995 spin_lock_irqsave(&(pmlmepriv->scanned_queue.lock), irqL);
> 1996 phead = &queue->queue;
> 1997 plist = phead->next;
> 1998 while (1) {
> 1999 if (end_of_queue_search(phead, plist) == true)
> 2000 break;
> 2001 pnetwork = LIST_CONTAINOR(plist, struct wlan_network, list);
> 2002 if (hwaddr_aton_i(data, bssid)) {
> 2003 netdev_info(dev, "r8712u: Invalid BSSID '%s'.\n",
> 2004 (u8 *)data);
> 2005 spin_unlock_irqrestore(&(pmlmepriv->scanned_queue.lock),
> 2006 irqL);
> 2007 return -EINVAL;
> 2008 }
> 2009 netdev_info(dev, "r8712u: BSSID:%pM\n", bssid);
>> 2010 if (!ether_addr_equal(bssid, pnetwork->network)) {
> 2011 /* BSSID match, then check if supporting wpa/wpa2 */
> 2012 pbuf = r8712_get_wpa_ie(&pnetwork->network.IEs[12],
> 2013 &wpa_ielen, pnetwork->network.IELength-12);
> 2014 if (pbuf && (wpa_ielen > 0)) {
> 2015 pdata->flags = 1;
> 2016 break;
> 2017 }
> 2018 pbuf = r8712_get_wpa2_ie(&pnetwork->network.IEs[12],
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
That error is because I forgot MacAddress that I have already mention.
Julia I have used pahole as you suggested me
following are the two structure need to be consider for alignment.
struct iw_pmksa {
__u32 cmd; /* 0 4 */
struct sockaddr bssid; /* 4 16 */
__u8 pmkid[16]; /* 20 16 */
/* size: 36, cachelines: 1, members: 3 */
/* last cacheline: 36 bytes */
};
struct wlan_bssid_ex {
u32 Length; /* 0 4 */
unsigned char MacAddress[6]; /* 4 6 */
u8 Reserved[2]; /* 10 2 */
struct ndis_802_11_ssid Ssid; /* 12 36 */
u32 Privacy; /* 48 4 */
s32 Rssi; /* 52 4 */
enum NDIS_802_11_NETWORK_TYPE NetworkTypeInUse; /* 56 4 */
struct NDIS_802_11_CONFIGURATION Configuration; /* 60 32 */
/* --- cacheline 1 boundary (64 bytes) was 28 bytes ago --- */
enum NDIS_802_11_NETWORK_INFRASTRUCTURE InfrastructureMode; /* 92 4 */
u8 rates[16]; /* 96 16 */
u32 IELength; /* 112 4 */
u8 IEs[768]; /* 116 768 */
/* --- cacheline 13 boundary (832 bytes) was 52 bytes ago --- */
/* size: 884, cachelines: 14, members: 12 */
/* last cacheline: 52 bytes */
};
As I understood both are not aligned to u16 so
ether_addr_equal_unaligned() should be used.
Here ether_addr_equal_unaligned() return true if both address are equal.
so here correct answer would be
if(ether_addr_equal_unaligned(bssid, pnetwork->network.MacAddress)) ??
If I am getting wrong any feedback are welcome otherwise I will
resend patch with correct modification.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Joe Perches <joe@perches.com> |
|---|---|
| Date | 2015-10-11 19:30 +0200 |
| Subject | Re: [PATCH 2/4] Staging: rtl8712: Use ether_addr_equal() over memcmp() |
| Message-ID | <qisBA-6Ef-7@gated-at.bofh.it> |
| In reply to | #1244099 |
On Sun, 2015-10-11 at 16:42 +0530, punit vara wrote:
> following are the two structure need to be consider for alignment.
>
> struct iw_pmksa {
> __u32 cmd; /* 0 4 */
> struct sockaddr bssid; /* 4 16 */
> __u8 pmkid[16]; /* 20 16 */
wrong bssid, bssid here is on the stack
after a char *, so bssid is aligned on a
pointer boundary, either 4 or 8.
> /* size: 36, cachelines: 1, members: 3 */
> /* last cacheline: 36 bytes */
> };
>
> struct wlan_bssid_ex {
[]
> unsigned char MacAddress[6]; /* 4 6 */
> As I understood both are not aligned to u16 so
> ether_addr_equal_unaligned() should be used.
u16s are aligned when on any even address
So ether_addr_equal could be used.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web