| Message ID | 1466704629-26084-1-git-send-email-luisbg@osg.samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote: > hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs > which have the following element: > u8 bssid[6]; [] > I am aware this patch gives a few checkpatch.pl warnings about lines being > over 80 characters. Fixing that would be a completely different issue, and > a lengthy one since the file has loads of them. > > Hopefully somebody else picks that up. Maybe I should send a hit to the > kernelnewbies mailing list :) Or not. really_long_identifiers™ makes using 80 columns silly. The hungarian could probably be converted though. A log of the memcpy and memcpy uses could probably be converted to ether_addr_<foo> too. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23/06/16 20:24, Joe Perches wrote: > On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote: >> hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs >> which have the following element: >> u8 bssid[6]; > [] >> I am aware this patch gives a few checkpatch.pl warnings about lines being >> over 80 characters. Fixing that would be a completely different issue, and >> a lengthy one since the file has loads of them. >> >> Hopefully somebody else picks that up. Maybe I should send a hit to the >> kernelnewbies mailing list :) > > Or not. > > really_long_identifiers™ makes using 80 columns silly. I agree. Not a priority, at all. > > The hungarian could probably be converted though. > I could look into this tomorrow. I noticed, for example these 3 in the same function: struct wid strWIDList[8]; u32 u32WidsCount = 0, dummyval = 0; u8 *pu8CurrByte = NULL; Not pretty and cleaning those should take little time. > A log of the memcpy and memcpy uses could probably be > converted to ether_addr_<foo> too. > Switching memcpy for ether_addr_copy and memcmp for ether_addr_equal. I could send a patch for this as well, but I would need to have somebody test it for me. Or maybe get this hardware for myself and do it properly. Do you approve of my original patch? Thanks for the review :) Luis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Joe, On Fri, Jun 24, 2016 at 5:24 AM, Joe Perches <joe@perches.com> wrote: > On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote: >> hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs >> which have the following element: >> u8 bssid[6]; > [] >> I am aware this patch gives a few checkpatch.pl warnings about lines being >> over 80 characters. Fixing that would be a completely different issue, and >> a lengthy one since the file has loads of them. >> >> Hopefully somebody else picks that up. Maybe I should send a hit to the >> kernelnewbies mailing list :) > > Or not. > > really_long_identifiers™ makes using 80 columns silly. > > The hungarian could probably be converted though. The main developers of this driver are slowly working through the driver's style issues, which is part of the reason why it's in staging. Thanks,
On 24/06/16 00:15, Julian Calaby wrote: > Hi Joe, > > On Fri, Jun 24, 2016 at 5:24 AM, Joe Perches <joe@perches.com> wrote: >> On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote: >>> hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs >>> which have the following element: >>> u8 bssid[6]; >> [] >>> I am aware this patch gives a few checkpatch.pl warnings about lines being >>> over 80 characters. Fixing that would be a completely different issue, and >>> a lengthy one since the file has loads of them. >>> >>> Hopefully somebody else picks that up. Maybe I should send a hit to the >>> kernelnewbies mailing list :) >> >> Or not. >> >> really_long_identifiers™ makes using 80 columns silly. >> >> The hungarian could probably be converted though. > > The main developers of this driver are slowly working through the > driver's style issues, which is part of the reason why it's in > staging. > > Thanks, > I understand Julian, All the maintainers listed in the MAINTAINERS file are in CC. I will wait for them to OK the suggestion of sending a patch fixing the Hungarian Notation. Didn't mean to step on your toes. I just wanted to help. Code in staging is cared for by a lot of people :) Luis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Luis, On Fri, Jun 24, 2016 at 9:50 AM, Luis de Bethencourt <luisbg@osg.samsung.com> wrote: > On 24/06/16 00:15, Julian Calaby wrote: >> Hi Joe, >> >> On Fri, Jun 24, 2016 at 5:24 AM, Joe Perches <joe@perches.com> wrote: >>> On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote: >>>> hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs >>>> which have the following element: >>>> u8 bssid[6]; >>> [] >>>> I am aware this patch gives a few checkpatch.pl warnings about lines being >>>> over 80 characters. Fixing that would be a completely different issue, and >>>> a lengthy one since the file has loads of them. >>>> >>>> Hopefully somebody else picks that up. Maybe I should send a hit to the >>>> kernelnewbies mailing list :) >>> >>> Or not. >>> >>> really_long_identifiers™ makes using 80 columns silly. >>> >>> The hungarian could probably be converted though. >> >> The main developers of this driver are slowly working through the >> driver's style issues, which is part of the reason why it's in >> staging. >> >> Thanks, >> > > I understand Julian, > > All the maintainers listed in the MAINTAINERS file are in CC. I will wait for > them to OK the suggestion of sending a patch fixing the Hungarian Notation. I was letting you know that this work is going to happen, not dissuading you from doing it. > Didn't mean to step on your toes. I just wanted to help. No toes were stepped on. As I said, this was not a "don't do that" message, this was an "it's going to happen eventually" message. > Code in staging is cared for by a lot of people :) Indeed it is. Thanks,
On 24/06/16 00:54, Julian Calaby wrote: > Hi Luis, > > On Fri, Jun 24, 2016 at 9:50 AM, Luis de Bethencourt > <luisbg@osg.samsung.com> wrote: >> On 24/06/16 00:15, Julian Calaby wrote: >>> Hi Joe, >>> >>> On Fri, Jun 24, 2016 at 5:24 AM, Joe Perches <joe@perches.com> wrote: >>>> On Thu, 2016-06-23 at 18:57 +0100, Luis de Bethencourt wrote: >>>>> hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs >>>>> which have the following element: >>>>> u8 bssid[6]; >>>> [] >>>>> I am aware this patch gives a few checkpatch.pl warnings about lines being >>>>> over 80 characters. Fixing that would be a completely different issue, and >>>>> a lengthy one since the file has loads of them. >>>>> >>>>> Hopefully somebody else picks that up. Maybe I should send a hit to the >>>>> kernelnewbies mailing list :) >>>> >>>> Or not. >>>> >>>> really_long_identifiers™ makes using 80 columns silly. >>>> >>>> The hungarian could probably be converted though. >>> >>> The main developers of this driver are slowly working through the >>> driver's style issues, which is part of the reason why it's in >>> staging. >>> >>> Thanks, >>> >> >> I understand Julian, >> >> All the maintainers listed in the MAINTAINERS file are in CC. I will wait for >> them to OK the suggestion of sending a patch fixing the Hungarian Notation. > > I was letting you know that this work is going to happen, not > dissuading you from doing it. > >> Didn't mean to step on your toes. I just wanted to help. > > No toes were stepped on. As I said, this was not a "don't do that" > message, this was an "it's going to happen eventually" message. > >> Code in staging is cared for by a lot of people :) > > Indeed it is. > > Thanks, > Gotcha. I will send the Hungarian Notation change tomorrow. Since it is some small help. I will let the memcpy/memcp to ether_addr_* change for the maintainers. I believe it will happen soon. Thanks for your input. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c index 9535842..7d5745a 100644 --- a/drivers/staging/wilc1000/host_interface.c +++ b/drivers/staging/wilc1000/host_interface.c @@ -1231,17 +1231,14 @@ static s32 Handle_RcvdNtwrkInfo(struct wilc_vif *vif, } for (i = 0; i < hif_drv->usr_scan_req.rcvd_ch_cnt; i++) { - if ((hif_drv->usr_scan_req.net_info[i].bssid) && - (pstrNetworkInfo->bssid)) { - if (memcmp(hif_drv->usr_scan_req.net_info[i].bssid, - pstrNetworkInfo->bssid, 6) == 0) { - if (pstrNetworkInfo->rssi <= hif_drv->usr_scan_req.net_info[i].rssi) { - goto done; - } else { - hif_drv->usr_scan_req.net_info[i].rssi = pstrNetworkInfo->rssi; - bNewNtwrkFound = false; - break; - } + if (memcmp(hif_drv->usr_scan_req.net_info[i].bssid, + pstrNetworkInfo->bssid, 6) == 0) { + if (pstrNetworkInfo->rssi <= hif_drv->usr_scan_req.net_info[i].rssi) { + goto done; + } else { + hif_drv->usr_scan_req.net_info[i].rssi = pstrNetworkInfo->rssi; + bNewNtwrkFound = false; + break; } } } @@ -1250,20 +1247,17 @@ static s32 Handle_RcvdNtwrkInfo(struct wilc_vif *vif, if (hif_drv->usr_scan_req.rcvd_ch_cnt < MAX_NUM_SCANNED_NETWORKS) { hif_drv->usr_scan_req.net_info[hif_drv->usr_scan_req.rcvd_ch_cnt].rssi = pstrNetworkInfo->rssi; - if (hif_drv->usr_scan_req.net_info[hif_drv->usr_scan_req.rcvd_ch_cnt].bssid && - pstrNetworkInfo->bssid) { - memcpy(hif_drv->usr_scan_req.net_info[hif_drv->usr_scan_req.rcvd_ch_cnt].bssid, - pstrNetworkInfo->bssid, 6); + memcpy(hif_drv->usr_scan_req.net_info[hif_drv->usr_scan_req.rcvd_ch_cnt].bssid, + pstrNetworkInfo->bssid, 6); - hif_drv->usr_scan_req.rcvd_ch_cnt++; + hif_drv->usr_scan_req.rcvd_ch_cnt++; - pstrNetworkInfo->new_network = true; - pJoinParams = host_int_ParseJoinBssParam(pstrNetworkInfo); + pstrNetworkInfo->new_network = true; + pJoinParams = host_int_ParseJoinBssParam(pstrNetworkInfo); - hif_drv->usr_scan_req.scan_result(SCAN_EVENT_NETWORK_FOUND, pstrNetworkInfo, - hif_drv->usr_scan_req.arg, - pJoinParams); - } + hif_drv->usr_scan_req.scan_result(SCAN_EVENT_NETWORK_FOUND, pstrNetworkInfo, + hif_drv->usr_scan_req.arg, + pJoinParams); } } else { pstrNetworkInfo->new_network = false; diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c index 51aff4f..3ddfa4a 100644 --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c @@ -625,8 +625,7 @@ static int scan(struct wiphy *wiphy, struct cfg80211_scan_request *request) for (i = 0; i < request->n_ssids; i++) { - if (request->ssids[i].ssid && - request->ssids[i].ssid_len != 0) { + if (request->ssids[i].ssid_len != 0) { strHiddenNetwork.net_info[i].ssid = kmalloc(request->ssids[i].ssid_len, GFP_KERNEL); memcpy(strHiddenNetwork.net_info[i].ssid, request->ssids[i].ssid, request->ssids[i].ssid_len); strHiddenNetwork.net_info[i].ssid_len = request->ssids[i].ssid_len;
hif_drv->usr_scan_req.net.net_info[i] contains found_net_info structs which have the following element: u8 bssid[6]; pstrNetworkInfo, of type network_info, also contains an u8 array named bssid. request->ssids is an array of cfg80211_ssid structs. Making ssid: u8 ssid[IEEE80211_MAX_SSID_LEN]; In these 3 cases the arrays are being checked against NULL, which can't happen. Removing the checks since they will always be true. Found with smatch: drivers/staging/wilc1000/host_interface.c:1234 Handle_RcvdNtwrkInfo() warn: this array is probably non-NULL. 'hif_drv->usr_scan_req.net_info[i].bssid' drivers/staging/wilc1000/host_interface.c:1235 Handle_RcvdNtwrkInfo() warn: this array is probably non-NULL. 'pstrNetworkInfo->bssid' drivers/staging/wilc1000/host_interface.c:1253 Handle_RcvdNtwrkInfo() warn: this array is probably non-NULL. 'hif_drv->usr_scan_req.net_info[hif_drv->usr_scan_req.rcvd_ch_cnt].bssid' drivers/staging/wilc1000/host_interface.c:1254 Handle_RcvdNtwrkInfo() warn: this array is probably non-NULL. 'pstrNetworkInfo->bssid' Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com> --- Hi, I am aware this patch gives a few checkpatch.pl warnings about lines being over 80 characters. Fixing that would be a completely different issue, and a lengthy one since the file has loads of them. Hopefully somebody else picks that up. Maybe I should send a hit to the kernelnewbies mailing list :) Thanks, Luis drivers/staging/wilc1000/host_interface.c | 38 ++++++++++------------- drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 3 +- 2 files changed, 17 insertions(+), 24 deletions(-)