* [PATCH 1/6] staging: rtl8188eu: remove unused function
@ 2015-07-16 11:28 Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 2/6] staging: rtl8188eu: remove redundant NULL check Sudip Mukherjee
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Sudip Mukherjee @ 2015-07-16 11:28 UTC (permalink / raw
To: Greg Kroah-Hartman; +Cc: linux-kernel, devel, Sudip Mukherjee
The inline function rtw_set_ips_deny() was only defined but was never
used.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/rtl8188eu/core/rtw_pwrctrl.c | 6 ------
drivers/staging/rtl8188eu/include/rtw_pwrctrl.h | 1 -
2 files changed, 7 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_pwrctrl.c b/drivers/staging/rtl8188eu/core/rtw_pwrctrl.c
index ec0a8a4..a2b54de 100644
--- a/drivers/staging/rtl8188eu/core/rtw_pwrctrl.c
+++ b/drivers/staging/rtl8188eu/core/rtw_pwrctrl.c
@@ -549,12 +549,6 @@ void rtw_init_pwrctrl_priv(struct adapter *padapter)
(unsigned long)padapter);
}
-inline void rtw_set_ips_deny(struct adapter *padapter, u32 ms)
-{
- struct pwrctrl_priv *pwrpriv = &padapter->pwrctrlpriv;
- pwrpriv->ips_deny_time = jiffies + msecs_to_jiffies(ms);
-}
-
/*
* rtw_pwr_wakeup - Wake the NIC up from: 1)IPS. 2)USB autosuspend
* @adapter: pointer to struct adapter structure
diff --git a/drivers/staging/rtl8188eu/include/rtw_pwrctrl.h b/drivers/staging/rtl8188eu/include/rtw_pwrctrl.h
index aa1fd87..a493d4c 100644
--- a/drivers/staging/rtl8188eu/include/rtw_pwrctrl.h
+++ b/drivers/staging/rtl8188eu/include/rtw_pwrctrl.h
@@ -257,7 +257,6 @@ s32 LPS_RF_ON_check(struct adapter *adapter, u32 delay_ms);
void LPS_Enter(struct adapter *adapter);
void LPS_Leave(struct adapter *adapter);
-void rtw_set_ips_deny(struct adapter *adapter, u32 ms);
int _rtw_pwr_wakeup(struct adapter *adapter, u32 ips_defer_ms,
const char *caller);
#define rtw_pwr_wakeup(adapter) \
--
1.8.1.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/6] staging: rtl8188eu: remove redundant NULL check
2015-07-16 11:28 [PATCH 1/6] staging: rtl8188eu: remove unused function Sudip Mukherjee
@ 2015-07-16 11:28 ` Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 3/6] staging: rtl8188eu: remove goto label Sudip Mukherjee
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Sudip Mukherjee @ 2015-07-16 11:28 UTC (permalink / raw
To: Greg Kroah-Hartman; +Cc: linux-kernel, devel, Sudip Mukherjee
The check for pstat and pdvobjpriv is not required here as we have
already checked for them before.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 2 +-
drivers/staging/rtl8188eu/os_dep/usb_intf.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index a0b8f66..ba8f9aa 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -3367,7 +3367,7 @@ static unsigned int OnAssocReq(struct adapter *padapter,
spin_unlock_bh(&pstapriv->asoc_list_lock);
/* now the station is qualified to join our BSS... */
- if (pstat && (pstat->state & WIFI_FW_ASSOC_SUCCESS) && (_STATS_SUCCESSFUL_ == status)) {
+ if ((pstat->state & WIFI_FW_ASSOC_SUCCESS) && (_STATS_SUCCESSFUL_ == status)) {
/* 1 bss_cap_update & sta_info_update */
bss_cap_update_on_sta_join(padapter, pstat);
sta_info_update(padapter, pstat);
diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index d0d4335..21744a5 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -123,7 +123,7 @@ static struct dvobj_priv *usb_dvobj_init(struct usb_interface *usb_intf)
status = _SUCCESS;
free_dvobj:
- if (status != _SUCCESS && pdvobjpriv) {
+ if (status != _SUCCESS) {
usb_set_intfdata(usb_intf, NULL);
kfree(pdvobjpriv);
pdvobjpriv = NULL;
--
1.8.1.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/6] staging: rtl8188eu: remove goto label
2015-07-16 11:28 [PATCH 1/6] staging: rtl8188eu: remove unused function Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 2/6] staging: rtl8188eu: remove redundant NULL check Sudip Mukherjee
@ 2015-07-16 11:28 ` Sudip Mukherjee
2015-07-17 11:03 ` Dan Carpenter
2015-07-16 11:28 ` [PATCH 4/6] staging: rtl8188eu: remove unneeded variable Sudip Mukherjee
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Sudip Mukherjee @ 2015-07-16 11:28 UTC (permalink / raw
To: Greg Kroah-Hartman; +Cc: linux-kernel, devel, Sudip Mukherjee
By checking for the success of kzalloc we were able to remove the goto
label thus making the code more readable.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/rtl8188eu/os_dep/usb_intf.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 21744a5..f68875b 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -115,14 +115,11 @@ static struct dvobj_priv *usb_dvobj_init(struct usb_interface *usb_intf)
mutex_init(&pdvobjpriv->usb_vendor_req_mutex);
pdvobjpriv->usb_vendor_req_buf = kzalloc(MAX_USB_IO_CTL_SIZE, GFP_KERNEL);
- if (!pdvobjpriv->usb_vendor_req_buf)
- goto free_dvobj;
-
- usb_get_dev(pusbd);
-
- status = _SUCCESS;
+ if (pdvobjpriv->usb_vendor_req_buf) {
+ usb_get_dev(pusbd);
+ status = _SUCCESS;
+ }
-free_dvobj:
if (status != _SUCCESS) {
usb_set_intfdata(usb_intf, NULL);
kfree(pdvobjpriv);
--
1.8.1.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/6] staging: rtl8188eu: remove unneeded variable
2015-07-16 11:28 [PATCH 1/6] staging: rtl8188eu: remove unused function Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 2/6] staging: rtl8188eu: remove redundant NULL check Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 3/6] staging: rtl8188eu: remove goto label Sudip Mukherjee
@ 2015-07-16 11:28 ` Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 5/6] staging: rtl8188eu: stop using DBG_88E Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 6/6] staging: rtl8188eu: remove unneeded ret Sudip Mukherjee
4 siblings, 0 replies; 14+ messages in thread
From: Sudip Mukherjee @ 2015-07-16 11:28 UTC (permalink / raw
To: Greg Kroah-Hartman; +Cc: linux-kernel, devel, Sudip Mukherjee
The default value of status was _FAIL, it was only changed if kzalloc
succeeds and the check for status is immediately following kzalloc. We
can have the failure code in the else part as the failure code will be
executed only if kzalloc fails.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/rtl8188eu/os_dep/usb_intf.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index f68875b..2d75c77 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -55,7 +55,6 @@ MODULE_DEVICE_TABLE(usb, rtw_usb_id_tbl);
static struct dvobj_priv *usb_dvobj_init(struct usb_interface *usb_intf)
{
int i;
- int status = _FAIL;
struct dvobj_priv *pdvobjpriv;
struct usb_host_config *phost_conf;
struct usb_config_descriptor *pconf_desc;
@@ -117,10 +116,7 @@ static struct dvobj_priv *usb_dvobj_init(struct usb_interface *usb_intf)
if (pdvobjpriv->usb_vendor_req_buf) {
usb_get_dev(pusbd);
- status = _SUCCESS;
- }
-
- if (status != _SUCCESS) {
+ } else {
usb_set_intfdata(usb_intf, NULL);
kfree(pdvobjpriv);
pdvobjpriv = NULL;
--
1.8.1.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/6] staging: rtl8188eu: stop using DBG_88E
2015-07-16 11:28 [PATCH 1/6] staging: rtl8188eu: remove unused function Sudip Mukherjee
` (2 preceding siblings ...)
2015-07-16 11:28 ` [PATCH 4/6] staging: rtl8188eu: remove unneeded variable Sudip Mukherjee
@ 2015-07-16 11:28 ` Sudip Mukherjee
2015-07-17 15:33 ` Jakub Sitnicki
2015-07-16 11:28 ` [PATCH 6/6] staging: rtl8188eu: remove unneeded ret Sudip Mukherjee
4 siblings, 1 reply; 14+ messages in thread
From: Sudip Mukherjee @ 2015-07-16 11:28 UTC (permalink / raw
To: Greg Kroah-Hartman; +Cc: linux-kernel, devel, Sudip Mukherjee
Stop using DBG_88E which is a custom macro for printing debugging
messages. Instead start using pr_debug and in the process define
pr_fmt.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/rtl8188eu/os_dep/usb_intf.c | 39 +++++++++++++++--------------
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 2d75c77..b245e9c 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -19,6 +19,7 @@
******************************************************************************/
#define _HCI_INTF_C_
+#define pr_fmt(fmt) "R8188EU: " fmt
#include <osdep_service.h>
#include <drv_types.h>
#include <recv_osdep.h>
@@ -143,7 +144,7 @@ static void usb_dvobj_deinit(struct usb_interface *usb_intf)
* on sitesurvey for the first time when
* device is up . Reset usb port for sitesurvey
* fail issue. */
- DBG_88E("usb attached..., try to reset usb device\n");
+ pr_debug("usb attached..., try to reset usb device\n");
usb_reset_device(interface_to_usbdev(usb_intf));
}
}
@@ -194,7 +195,7 @@ static void rtw_dev_unload(struct adapter *padapter)
RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("+rtw_dev_unload\n"));
if (padapter->bup) {
- DBG_88E("===> rtw_dev_unload\n");
+ pr_debug("===> rtw_dev_unload\n");
padapter->bDriverStopped = true;
if (padapter->xmitpriv.ack_tx)
rtw_ack_tx_done(&padapter->xmitpriv, RTW_SCTX_DONE_DRV_STOP);
@@ -217,7 +218,7 @@ static void rtw_dev_unload(struct adapter *padapter)
("r871x_dev_unload():padapter->bup == false\n"));
}
- DBG_88E("<=== rtw_dev_unload\n");
+ pr_debug("<=== rtw_dev_unload\n");
RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("-rtw_dev_unload\n"));
}
@@ -234,11 +235,11 @@ static int rtw_suspend(struct usb_interface *pusb_intf, pm_message_t message)
u32 start_time = jiffies;
- DBG_88E("==> %s (%s:%d)\n", __func__, current->comm, current->pid);
+ pr_debug("==> %s (%s:%d)\n", __func__, current->comm, current->pid);
if ((!padapter->bup) || (padapter->bDriverStopped) ||
(padapter->bSurpriseRemoved)) {
- DBG_88E("padapter->bup=%d bDriverStopped=%d bSurpriseRemoved = %d\n",
+ pr_debug("padapter->bup=%d bDriverStopped=%d bSurpriseRemoved = %d\n",
padapter->bup, padapter->bDriverStopped,
padapter->bSurpriseRemoved);
goto exit;
@@ -260,7 +261,7 @@ static int rtw_suspend(struct usb_interface *pusb_intf, pm_message_t message)
if (check_fwstate(pmlmepriv, WIFI_STATION_STATE) &&
check_fwstate(pmlmepriv, _FW_LINKED)) {
- DBG_88E("%s:%d %s(%pM), length:%d assoc_ssid.length:%d\n",
+ pr_debug("%s:%d %s(%pM), length:%d assoc_ssid.length:%d\n",
__func__, __LINE__,
pmlmepriv->cur_network.network.Ssid.Ssid,
pmlmepriv->cur_network.network.MacAddress,
@@ -286,7 +287,7 @@ static int rtw_suspend(struct usb_interface *pusb_intf, pm_message_t message)
rtw_indicate_disconnect(padapter);
exit:
- DBG_88E("<=== %s return %d.............. in %dms\n", __func__
+ pr_debug("<=== %s return %d.............. in %dms\n", __func__
, ret, rtw_get_passing_time_ms(start_time));
return ret;
@@ -299,7 +300,7 @@ static int rtw_resume_process(struct adapter *padapter)
int ret = -1;
u32 start_time = jiffies;
- DBG_88E("==> %s (%s:%d)\n", __func__, current->comm, current->pid);
+ pr_debug("==> %s (%s:%d)\n", __func__, current->comm, current->pid);
if (padapter) {
pnetdev = padapter->pnetdev;
@@ -312,7 +313,7 @@ static int rtw_resume_process(struct adapter *padapter)
rtw_reset_drv_sw(padapter);
pwrpriv->bkeepfwalive = false;
- DBG_88E("bkeepfwalive(%x)\n", pwrpriv->bkeepfwalive);
+ pr_debug("bkeepfwalive(%x)\n", pwrpriv->bkeepfwalive);
if (pm_netdev_open(pnetdev, true) != 0)
goto exit;
@@ -327,7 +328,7 @@ static int rtw_resume_process(struct adapter *padapter)
exit:
if (pwrpriv)
pwrpriv->bInSuspend = false;
- DBG_88E("<=== %s return %d.............. in %dms\n", __func__,
+ pr_debug("<=== %s return %d.............. in %dms\n", __func__,
ret, rtw_get_passing_time_ms(start_time));
@@ -400,8 +401,8 @@ static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
dvobj->pusbdev->do_remote_wakeup = 1;
pusb_intf->needs_remote_wakeup = 1;
device_init_wakeup(&pusb_intf->dev, 1);
- DBG_88E("\n padapter->pwrctrlpriv.bSupportRemoteWakeup~~~~~~\n");
- DBG_88E("\n padapter->pwrctrlpriv.bSupportRemoteWakeup~~~[%d]~~~\n",
+ pr_debug("\n padapter->pwrctrlpriv.bSupportRemoteWakeup~~~~~~\n");
+ pr_debug("\n padapter->pwrctrlpriv.bSupportRemoteWakeup~~~[%d]~~~\n",
device_may_wakeup(&pusb_intf->dev));
}
#endif
@@ -409,13 +410,13 @@ static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
/* 2012-07-11 Move here to prevent the 8723AS-VAU BT auto
* suspend influence */
if (usb_autopm_get_interface(pusb_intf) < 0)
- DBG_88E("can't get autopm:\n");
+ pr_debug("can't get autopm:\n");
/* alloc dev name after read efuse. */
rtw_init_netdev_name(pnetdev, padapter->registrypriv.ifname);
rtw_macaddr_cfg(padapter->eeprompriv.mac_addr);
memcpy(pnetdev->dev_addr, padapter->eeprompriv.mac_addr, ETH_ALEN);
- DBG_88E("MAC Address from pnetdev->dev_addr = %pM\n",
+ pr_debug("MAC Address from pnetdev->dev_addr = %pM\n",
pnetdev->dev_addr);
/* step 6. Tell the network stack we exist */
@@ -424,7 +425,7 @@ static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
goto free_hal_data;
}
- DBG_88E("bDriverStopped:%d, bSurpriseRemoved:%d, bup:%d, hw_init_completed:%d\n"
+ pr_debug("bDriverStopped:%d, bSurpriseRemoved:%d, bup:%d, hw_init_completed:%d\n"
, padapter->bDriverStopped
, padapter->bSurpriseRemoved
, padapter->bup
@@ -468,7 +469,7 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
rtw_cancel_all_timer(if1);
rtw_dev_unload(if1);
- DBG_88E("+r871xu_dev_remove, hw_init_completed=%d\n",
+ pr_debug("+r871xu_dev_remove, hw_init_completed=%d\n",
if1->hw_init_completed);
rtw_free_drv_sw(if1);
if (pnetdev)
@@ -493,7 +494,7 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
if1 = rtw_usb_if1_init(dvobj, pusb_intf, pdid);
if (if1 == NULL) {
- DBG_88E("rtw_init_primarystruct adapter Failed!\n");
+ pr_debug("rtw_init_primarystruct adapter Failed!\n");
goto free_dvobj;
}
@@ -518,7 +519,7 @@ static void rtw_dev_remove(struct usb_interface *pusb_intf)
struct adapter *padapter = dvobj->if1;
- DBG_88E("+rtw_dev_remove\n");
+ pr_debug("+rtw_dev_remove\n");
RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("+dev_remove()\n"));
if (!pusb_intf->unregistering)
@@ -534,7 +535,7 @@ static void rtw_dev_remove(struct usb_interface *pusb_intf)
usb_dvobj_deinit(pusb_intf);
RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("-dev_remove()\n"));
- DBG_88E("-r871xu_dev_remove, done\n");
+ pr_debug("-r871xu_dev_remove, done\n");
}
static struct usb_driver rtl8188e_usb_drv = {
--
1.8.1.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/6] staging: rtl8188eu: remove unneeded ret
2015-07-16 11:28 [PATCH 1/6] staging: rtl8188eu: remove unused function Sudip Mukherjee
` (3 preceding siblings ...)
2015-07-16 11:28 ` [PATCH 5/6] staging: rtl8188eu: stop using DBG_88E Sudip Mukherjee
@ 2015-07-16 11:28 ` Sudip Mukherjee
4 siblings, 0 replies; 14+ messages in thread
From: Sudip Mukherjee @ 2015-07-16 11:28 UTC (permalink / raw
To: Greg Kroah-Hartman; +Cc: linux-kernel, devel, Sudip Mukherjee
The variable ret was always 0. So remove the variable and always
return 0 from the function.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/rtl8188eu/os_dep/usb_intf.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index b245e9c..5490b29 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -230,11 +230,8 @@ static int rtw_suspend(struct usb_interface *pusb_intf, pm_message_t message)
struct net_device *pnetdev = padapter->pnetdev;
struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
struct pwrctrl_priv *pwrpriv = &padapter->pwrctrlpriv;
-
- int ret = 0;
u32 start_time = jiffies;
-
pr_debug("==> %s (%s:%d)\n", __func__, current->comm, current->pid);
if ((!padapter->bup) || (padapter->bDriverStopped) ||
@@ -287,10 +284,10 @@ static int rtw_suspend(struct usb_interface *pusb_intf, pm_message_t message)
rtw_indicate_disconnect(padapter);
exit:
- pr_debug("<=== %s return %d.............. in %dms\n", __func__
- , ret, rtw_get_passing_time_ms(start_time));
+ pr_debug("<=== %s .............. in %dms\n", __func__,
+ rtw_get_passing_time_ms(start_time));
- return ret;
+ return 0;
}
static int rtw_resume_process(struct adapter *padapter)
--
1.8.1.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] staging: rtl8188eu: remove goto label
2015-07-16 11:28 ` [PATCH 3/6] staging: rtl8188eu: remove goto label Sudip Mukherjee
@ 2015-07-17 11:03 ` Dan Carpenter
2015-07-17 11:25 ` Sudip Mukherjee
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2015-07-17 11:03 UTC (permalink / raw
To: Sudip Mukherjee; +Cc: Greg Kroah-Hartman, devel, linux-kernel
On Thu, Jul 16, 2015 at 04:58:09PM +0530, Sudip Mukherjee wrote:
> By checking for the success of kzalloc we were able to remove the goto
> label thus making the code more readable.
>
No... You've just changed error handling to success handling and added
some new indent levels and made a tangled spaghetti exit path even more
tangled. Spoderman wants to know, "Why u do dis?"
It should look like:
diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index d0d4335..7617a22 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -120,16 +120,13 @@ static struct dvobj_priv *usb_dvobj_init(struct usb_interface *usb_intf)
usb_get_dev(pusbd);
- status = _SUCCESS;
+ return pdvobjpriv;
free_dvobj:
- if (status != _SUCCESS && pdvobjpriv) {
- usb_set_intfdata(usb_intf, NULL);
- kfree(pdvobjpriv);
- pdvobjpriv = NULL;
- }
-exit:
- return pdvobjpriv;
+ usb_set_intfdata(usb_intf, NULL);
+ kfree(pdvobjpriv);
+
+ return NULL;
}
static void usb_dvobj_deinit(struct usb_interface *usb_intf)
See? No indenting, no if statements. Just one statement after another.
regards,
dan carpenter
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] staging: rtl8188eu: remove goto label
2015-07-17 11:03 ` Dan Carpenter
@ 2015-07-17 11:25 ` Sudip Mukherjee
2015-07-17 11:47 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Sudip Mukherjee @ 2015-07-17 11:25 UTC (permalink / raw
To: Dan Carpenter; +Cc: Greg Kroah-Hartman, devel, linux-kernel
On Fri, Jul 17, 2015 at 02:03:48PM +0300, Dan Carpenter wrote:
> On Thu, Jul 16, 2015 at 04:58:09PM +0530, Sudip Mukherjee wrote:
> > By checking for the success of kzalloc we were able to remove the goto
> > label thus making the code more readable.
> >
>
> No... You've just changed error handling to success handling and added
> some new indent levels and made a tangled spaghetti exit path even more
> tangled. Spoderman wants to know, "Why u do dis?"
I had to go for google to lookup Spoderman or Spiderman. :)
At the end of the series, isn't the code better looking and simpler
than the original code?
regards
sudip
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] staging: rtl8188eu: remove goto label
2015-07-17 11:25 ` Sudip Mukherjee
@ 2015-07-17 11:47 ` Dan Carpenter
2015-07-17 12:03 ` Sudip Mukherjee
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2015-07-17 11:47 UTC (permalink / raw
To: Sudip Mukherjee; +Cc: devel, Greg Kroah-Hartman, linux-kernel
On Fri, Jul 17, 2015 at 04:55:12PM +0530, Sudip Mukherjee wrote:
> On Fri, Jul 17, 2015 at 02:03:48PM +0300, Dan Carpenter wrote:
> > On Thu, Jul 16, 2015 at 04:58:09PM +0530, Sudip Mukherjee wrote:
> > > By checking for the success of kzalloc we were able to remove the goto
> > > label thus making the code more readable.
> > >
> >
> > No... You've just changed error handling to success handling and added
> > some new indent levels and made a tangled spaghetti exit path even more
> > tangled. Spoderman wants to know, "Why u do dis?"
> I had to go for google to lookup Spoderman or Spiderman. :)
> At the end of the series, isn't the code better looking and simpler
> than the original code?
Yes, but that's not a very high bar to clear.
What I'm trying to explain is the beauty of writing one statement after
the other without indenting. The simplest kind of code looks like this
without error handling.
int some_function(void)
{
do_thing_one();
do_thing_two();
do_thing_three();
return 0;
}
When you add canonical error handling it looks like this:
int some_function(void)
{
do_thing_one();
if (failure)
return -ERROR;
do_thing_two();
if (failure)
goto undo_one;
do_thing_three();
if (failure)
goto undo_two;
return 0;
undo_two:
undo_two_thing();
undo_one:
undo_one_thing();
return -ERROR;
}
But with success handling it look like this:
int some_function(void)
{
do_thing_one();
if (success) {
do_thing_two();
if (success) {
do_thing_three();
if (success)
return 0;
}
}
return -ERROR;
}
Fewer lines, but terrible code. In this patch we only make the last
condition success handling but it's still messier than need be.
if (fail)
if (fail)
if (fail)
if (success)
else
The canonical method is more consistent and simplest to read.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] staging: rtl8188eu: remove goto label
2015-07-17 11:47 ` Dan Carpenter
@ 2015-07-17 12:03 ` Sudip Mukherjee
0 siblings, 0 replies; 14+ messages in thread
From: Sudip Mukherjee @ 2015-07-17 12:03 UTC (permalink / raw
To: Dan Carpenter; +Cc: devel, Greg Kroah-Hartman, linux-kernel
On Fri, Jul 17, 2015 at 02:47:55PM +0300, Dan Carpenter wrote:
> On Fri, Jul 17, 2015 at 04:55:12PM +0530, Sudip Mukherjee wrote:
> > On Fri, Jul 17, 2015 at 02:03:48PM +0300, Dan Carpenter wrote:
> > > On Thu, Jul 16, 2015 at 04:58:09PM +0530, Sudip Mukherjee wrote:
> > > > By checking for the success of kzalloc we were able to remove the goto
> > > > label thus making the code more readable.
> > > >
> > >
> > > No... You've just changed error handling to success handling and added
> > > some new indent levels and made a tangled spaghetti exit path even more
> > > tangled. Spoderman wants to know, "Why u do dis?"
> > I had to go for google to lookup Spoderman or Spiderman. :)
> > At the end of the series, isn't the code better looking and simpler
> > than the original code?
>
> Yes, but that's not a very high bar to clear.
>
> What I'm trying to explain is the beauty of writing one statement after
> the other without indenting.
understood.
I was planning another series for that file so i will include one for
this also. But before that I am going to send in the patch for sm7xxfb
to move out of staging. Please check that.
regards
sudip
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/6] staging: rtl8188eu: stop using DBG_88E
2015-07-16 11:28 ` [PATCH 5/6] staging: rtl8188eu: stop using DBG_88E Sudip Mukherjee
@ 2015-07-17 15:33 ` Jakub Sitnicki
2015-07-18 4:46 ` Sudip Mukherjee
0 siblings, 1 reply; 14+ messages in thread
From: Jakub Sitnicki @ 2015-07-17 15:33 UTC (permalink / raw
To: Sudip Mukherjee; +Cc: Greg Kroah-Hartman, linux-kernel, devel
On Thu, Jul 16, 2015 at 01:28 PM CEST, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> Stop using DBG_88E which is a custom macro for printing debugging
> messages. Instead start using pr_debug and in the process define
> pr_fmt.
In the end, don't we want to use netdev_dbg() everywhere where we work
with a struct net_device? And use dev_dbg() everywhere where we work
with a struct device (or a struct usb_interface)?
At least that's how I understand commit 8f26b8376faa ("checkpatch:
update suggested printk conversions") description:
Direct conversion of printk(KERN_<LEVEL>... to pr_<level> isn't the
preferred conversion when a struct net_device or struct device is
available.
Do you think it is worth going straight for netdev_dbg()/dev_dbg() to
avoid redoing it later?
>
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> drivers/staging/rtl8188eu/os_dep/usb_intf.c | 39 +++++++++++++++--------------
> 1 file changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
> index 2d75c77..b245e9c 100644
> --- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
> @@ -19,6 +19,7 @@
> ******************************************************************************/
> #define _HCI_INTF_C_
>
> +#define pr_fmt(fmt) "R8188EU: " fmt
> #include <osdep_service.h>
> #include <drv_types.h>
> #include <recv_osdep.h>
If we're going to stay with pr_debug(), using KBUILD_MODNAME seems to be
the convention among drivers when defining pr_fmt():
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Cheers,
Jakub
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/6] staging: rtl8188eu: stop using DBG_88E
2015-07-17 15:33 ` Jakub Sitnicki
@ 2015-07-18 4:46 ` Sudip Mukherjee
2015-07-18 18:03 ` Jakub Sitnicki
0 siblings, 1 reply; 14+ messages in thread
From: Sudip Mukherjee @ 2015-07-18 4:46 UTC (permalink / raw
To: Jakub Sitnicki; +Cc: Greg Kroah-Hartman, linux-kernel, devel
On Fri, Jul 17, 2015 at 05:33:55PM +0200, Jakub Sitnicki wrote:
> On Thu, Jul 16, 2015 at 01:28 PM CEST, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> > Stop using DBG_88E which is a custom macro for printing debugging
> > messages. Instead start using pr_debug and in the process define
> > pr_fmt.
>
> In the end, don't we want to use netdev_dbg() everywhere where we work
> with a struct net_device? And use dev_dbg() everywhere where we work
> with a struct device (or a struct usb_interface)?
Looks like in some places we can get net_device from usb_interface.
struct dvobj_priv *dvobj = usb_get_intfdata(pusb_intf);
struct adapter *padapter = dvobj->if1;
struct net_device *pnetdev = padapter->pnetdev;
>
> At least that's how I understand commit 8f26b8376faa ("checkpatch:
> update suggested printk conversions") description:
>
> Direct conversion of printk(KERN_<LEVEL>... to pr_<level> isn't the
> preferred conversion when a struct net_device or struct device is
> available.
>
> Do you think it is worth going straight for netdev_dbg()/dev_dbg() to
> avoid redoing it later?
At the end it should be netdev_* or dev_* and if both are not available
then pr_*. Here my main intention was to remove the custom defined
macro. And while doing this it is easier to use a script to reduce the
chances of error. Now that the custom macro is out of the way we can
concentrate on converting it to netdev_* or dev_*.
>
> >
<snip>
> >
> > +#define pr_fmt(fmt) "R8188EU: " fmt
> > #include <osdep_service.h>
> > #include <drv_types.h>
> > #include <recv_osdep.h>
>
> If we're going to stay with pr_debug(), using KBUILD_MODNAME seems to be
> the convention among drivers when defining pr_fmt():
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
yes, KBUILD_MODNAME is the usual convention but you will also find many
places where that has not been used. Here I have used R8188EU to keep it
same for all the messages that will be printed from the other files of
this driver else it might be confusing for the user.
regards
sudip
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/6] staging: rtl8188eu: stop using DBG_88E
2015-07-18 4:46 ` Sudip Mukherjee
@ 2015-07-18 18:03 ` Jakub Sitnicki
2015-07-20 5:29 ` Sudip Mukherjee
0 siblings, 1 reply; 14+ messages in thread
From: Jakub Sitnicki @ 2015-07-18 18:03 UTC (permalink / raw
To: Sudip Mukherjee; +Cc: Greg Kroah-Hartman, linux-kernel, devel
On Sat, Jul 18, 2015 at 06:46 AM CEST, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> On Fri, Jul 17, 2015 at 05:33:55PM +0200, Jakub Sitnicki wrote:
>> On Thu, Jul 16, 2015 at 01:28 PM CEST, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
>> > Stop using DBG_88E which is a custom macro for printing debugging
>> > messages. Instead start using pr_debug and in the process define
>> > pr_fmt.
>>
>> In the end, don't we want to use netdev_dbg() everywhere where we work
>> with a struct net_device? And use dev_dbg() everywhere where we work
>> with a struct device (or a struct usb_interface)?
> Looks like in some places we can get net_device from usb_interface.
>
> struct dvobj_priv *dvobj = usb_get_intfdata(pusb_intf);
> struct adapter *padapter = dvobj->if1;
> struct net_device *pnetdev = padapter->pnetdev;
You're right providing that the net_device has been successfully
allocated and hasn't been deallocated yet. There are at least a couple
of pr_debug() calls that couldn't be converted to netdev_dbg(), one in
rtw_drv_init() and another in rtw_dev_remove(), because the net_device
is not available, if I'm not wrong.
>> At least that's how I understand commit 8f26b8376faa ("checkpatch:
>> update suggested printk conversions") description:
>>
>> Direct conversion of printk(KERN_<LEVEL>... to pr_<level> isn't the
>> preferred conversion when a struct net_device or struct device is
>> available.
>>
>> Do you think it is worth going straight for netdev_dbg()/dev_dbg() to
>> avoid redoing it later?
> At the end it should be netdev_* or dev_* and if both are not available
> then pr_*. Here my main intention was to remove the custom defined
> macro. And while doing this it is easier to use a script to reduce the
> chances of error. Now that the custom macro is out of the way we can
> concentrate on converting it to netdev_* or dev_*.
>>
>> >
> <snip>
>> >
>> > +#define pr_fmt(fmt) "R8188EU: " fmt
>> > #include <osdep_service.h>
>> > #include <drv_types.h>
>> > #include <recv_osdep.h>
>>
>> If we're going to stay with pr_debug(), using KBUILD_MODNAME seems to be
>> the convention among drivers when defining pr_fmt():
>>
>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> yes, KBUILD_MODNAME is the usual convention but you will also find many
> places where that has not been used. Here I have used R8188EU to keep it
> same for all the messages that will be printed from the other files of
> this driver else it might be confusing for the user.
I see your point. If consistent log prefix is the goal do you think it
would make sense to change it to:
#define pr_fmt(fmt) DRIVER_PREFIX ": " fmt
... so the prefix is not defined in two places?
Cheers,
Jakub
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/6] staging: rtl8188eu: stop using DBG_88E
2015-07-18 18:03 ` Jakub Sitnicki
@ 2015-07-20 5:29 ` Sudip Mukherjee
0 siblings, 0 replies; 14+ messages in thread
From: Sudip Mukherjee @ 2015-07-20 5:29 UTC (permalink / raw
To: Jakub Sitnicki; +Cc: Greg Kroah-Hartman, linux-kernel, devel
On Sat, Jul 18, 2015 at 08:03:27PM +0200, Jakub Sitnicki wrote:
> On Sat, Jul 18, 2015 at 06:46 AM CEST, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> > On Fri, Jul 17, 2015 at 05:33:55PM +0200, Jakub Sitnicki wrote:
> >> On Thu, Jul 16, 2015 at 01:28 PM CEST, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> >> > Stop using DBG_88E which is a custom macro for printing debugging
> >> > messages. Instead start using pr_debug and in the process define
> >> > pr_fmt.
> >>
> >> In the end, don't we want to use netdev_dbg() everywhere where we work
> >> with a struct net_device? And use dev_dbg() everywhere where we work
> >> with a struct device (or a struct usb_interface)?
> > Looks like in some places we can get net_device from usb_interface.
> >
> > struct dvobj_priv *dvobj = usb_get_intfdata(pusb_intf);
> > struct adapter *padapter = dvobj->if1;
> > struct net_device *pnetdev = padapter->pnetdev;
>
> You're right providing that the net_device has been successfully
> allocated and hasn't been deallocated yet. There are at least a couple
> of pr_debug() calls that couldn't be converted to netdev_dbg(), one in
> rtw_drv_init() and another in rtw_dev_remove(), because the net_device
> is not available, if I'm not wrong.
yes. That is why I said in some places.
>
> >> At least that's how I understand commit 8f26b8376faa ("checkpatch:
> >> update suggested printk conversions") description:
> >>
> >> Direct conversion of printk(KERN_<LEVEL>... to pr_<level> isn't the
> >> preferred conversion when a struct net_device or struct device is
> >> available.
> >>
> >> Do you think it is worth going straight for netdev_dbg()/dev_dbg() to
> >> avoid redoing it later?
> > At the end it should be netdev_* or dev_* and if both are not available
> > then pr_*. Here my main intention was to remove the custom defined
> > macro. And while doing this it is easier to use a script to reduce the
> > chances of error. Now that the custom macro is out of the way we can
> > concentrate on converting it to netdev_* or dev_*.
> >>
> >> >
> > <snip>
> >> >
> >> > +#define pr_fmt(fmt) "R8188EU: " fmt
> >> > #include <osdep_service.h>
> >> > #include <drv_types.h>
> >> > #include <recv_osdep.h>
> >>
> >> If we're going to stay with pr_debug(), using KBUILD_MODNAME seems to be
> >> the convention among drivers when defining pr_fmt():
> >>
> >> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > yes, KBUILD_MODNAME is the usual convention but you will also find many
> > places where that has not been used. Here I have used R8188EU to keep it
> > same for all the messages that will be printed from the other files of
> > this driver else it might be confusing for the user.
>
> I see your point. If consistent log prefix is the goal do you think it
> would make sense to change it to:
>
> #define pr_fmt(fmt) DRIVER_PREFIX ": " fmt
>
> ... so the prefix is not defined in two places?
Well, then we need to define DRIVER_PREFIX in some header file and by
norm all local header files should be included at the end. And pr_fmt
should be at the top so we will not have DRIVER_PREFIX at that point.
regards
sudip
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-07-20 5:30 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-16 11:28 [PATCH 1/6] staging: rtl8188eu: remove unused function Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 2/6] staging: rtl8188eu: remove redundant NULL check Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 3/6] staging: rtl8188eu: remove goto label Sudip Mukherjee
2015-07-17 11:03 ` Dan Carpenter
2015-07-17 11:25 ` Sudip Mukherjee
2015-07-17 11:47 ` Dan Carpenter
2015-07-17 12:03 ` Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 4/6] staging: rtl8188eu: remove unneeded variable Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 5/6] staging: rtl8188eu: stop using DBG_88E Sudip Mukherjee
2015-07-17 15:33 ` Jakub Sitnicki
2015-07-18 4:46 ` Sudip Mukherjee
2015-07-18 18:03 ` Jakub Sitnicki
2015-07-20 5:29 ` Sudip Mukherjee
2015-07-16 11:28 ` [PATCH 6/6] staging: rtl8188eu: remove unneeded ret Sudip Mukherjee
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.