linux-staging.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/6] staging: rtl8188eu: remove unused hal_data_8188e members
@ 2021-06-12 18:00 Martin Kaiser
  2021-06-12 18:00 ` [PATCH 2/6] staging: rtl8188eu: fix usb_submit_urb error handling Martin Kaiser
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Martin Kaiser @ 2021-06-12 18:00 UTC (permalink / raw)
  To: Larry Finger, Greg Kroah-Hartman
  Cc: linux-staging, kernel-janitors, linux-kernel, Martin Kaiser

Remove some members of struct hal_data_8188e which are not used
in this driver.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/rtl8188eu/include/rtl8188e_hal.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/staging/rtl8188eu/include/rtl8188e_hal.h b/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
index 0c4c23be1dd5..b2d85dac3ddd 100644
--- a/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
+++ b/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
@@ -233,7 +233,6 @@ struct hal_data_8188e {
 	u8	PwrGroupHT20[RF_PATH_MAX][CHANNEL_MAX_NUMBER];
 	u8	PwrGroupHT40[RF_PATH_MAX][CHANNEL_MAX_NUMBER];
 
-	u8	LegacyHTTxPowerDiff;/*  Legacy to HT rate power diff */
 	/*  The current Tx Power Level */
 	u8	CurrentCckTxPwrIdx;
 	u8	CurrentOfdm24GTxPwrIdx;
@@ -242,12 +241,8 @@ struct hal_data_8188e {
 
 	/*  Read/write are allow for following hardware information variables */
 	u8	framesync;
-	u32	framesyncC34;
-	u8	framesyncMonitor;
-	u8	DefaultInitialGain[4];
 	u8	pwrGroupCnt;
 	u32	MCSTxPowerLevelOriginalOffset[MAX_PG_GROUP][16];
-	u32	CCKTxPowerLevelOriginalOffset;
 
 	u8	CrystalCap;
 
@@ -284,9 +279,6 @@ struct hal_data_8188e {
 					*  beacon in TxQ.
 					*/
 
-	/*  2010/08/09 MH Add CU power down mode. */
-	bool		pwrdown;
-
 	/*  Add for dual MAC  0--Mac0 1--Mac1 */
 	u32	interfaceIndex;
 
@@ -309,7 +301,6 @@ struct hal_data_8188e {
 	u8	UsbTxAggMode;
 	u8	UsbTxAggDescNum;
 	u16	HwRxPageSize;		/*  Hardware setting */
-	u32	MaxUsbRxAggBlock;
 
 	enum usb_rx_agg_mode UsbRxAggMode;
 	u8	UsbRxAggBlockCount;	/*  USB Block count. Block size is
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/6] staging: rtl8188eu: fix usb_submit_urb error handling
  2021-06-12 18:00 [PATCH 1/6] staging: rtl8188eu: remove unused hal_data_8188e members Martin Kaiser
@ 2021-06-12 18:00 ` Martin Kaiser
  2021-06-14 14:46   ` Greg Kroah-Hartman
  2021-06-12 18:00 ` [PATCH 3/6] staging: rtl8188eu: remove a write-only struct member Martin Kaiser
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Martin Kaiser @ 2021-06-12 18:00 UTC (permalink / raw)
  To: Larry Finger, Greg Kroah-Hartman
  Cc: linux-staging, kernel-janitors, linux-kernel, Martin Kaiser,
	Dan Carpenter

-EPERM should be handled like any other error.

Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
index ec07b2017fb7..0ceb05f3884f 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
@@ -366,7 +366,6 @@ u32 usb_read_port(struct adapter *adapter, u32 addr, struct recv_buf *precvbuf)
 	struct usb_device *pusbd = pdvobj->pusbdev;
 	int err;
 	unsigned int pipe;
-	u32 ret = _SUCCESS;
 
 	if (adapter->bDriverStopped || adapter->bSurpriseRemoved ||
 	    adapter->pwrctrlpriv.pnp_bstop_trx) {
@@ -403,10 +402,10 @@ u32 usb_read_port(struct adapter *adapter, u32 addr, struct recv_buf *precvbuf)
 			  precvbuf);/* context is precvbuf */
 
 	err = usb_submit_urb(purb, GFP_ATOMIC);
-	if ((err) && (err != (-EPERM)))
-		ret = _FAIL;
+	if (err)
+		return _FAIL;
 
-	return ret;
+	return _SUCCESS;
 }
 
 void rtw_hal_inirp_deinit(struct adapter *padapter)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/6] staging: rtl8188eu: remove a write-only struct member
  2021-06-12 18:00 [PATCH 1/6] staging: rtl8188eu: remove unused hal_data_8188e members Martin Kaiser
  2021-06-12 18:00 ` [PATCH 2/6] staging: rtl8188eu: fix usb_submit_urb error handling Martin Kaiser
@ 2021-06-12 18:00 ` Martin Kaiser
  2021-06-12 18:00 ` [PATCH 4/6] staging: rtl8188eu: remove a write-only power-index members Martin Kaiser
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Martin Kaiser @ 2021-06-12 18:00 UTC (permalink / raw)
  To: Larry Finger, Greg Kroah-Hartman
  Cc: linux-staging, kernel-janitors, linux-kernel, Martin Kaiser

FwRsvdPageStartOffset in struct hal_data_8188e is never read. Remove it.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/rtl8188eu/hal/rtl8188e_cmd.c     | 1 -
 drivers/staging/rtl8188eu/include/rtl8188e_hal.h | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/drivers/staging/rtl8188eu/hal/rtl8188e_cmd.c b/drivers/staging/rtl8188eu/hal/rtl8188e_cmd.c
index 34d2c62765f0..d6c83e4fb105 100644
--- a/drivers/staging/rtl8188eu/hal/rtl8188e_cmd.c
+++ b/drivers/staging/rtl8188eu/hal/rtl8188e_cmd.c
@@ -466,7 +466,6 @@ static void SetFwRsvdPagePkt(struct adapter *adapt, bool bDLFinished)
 	if (PageNeed == 1)
 		PageNeed += 1;
 	PageNum += PageNeed;
-	adapt->HalData->FwRsvdPageStartOffset = PageNum;
 
 	BufIndex += PageNeed * 128;
 
diff --git a/drivers/staging/rtl8188eu/include/rtl8188e_hal.h b/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
index b2d85dac3ddd..7b8ca0ea3008 100644
--- a/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
+++ b/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
@@ -275,9 +275,6 @@ struct hal_data_8188e {
 
 	u8	bDumpRxPkt;/* for debug */
 	u8	bDumpTxPkt;/* for debug */
-	u8	FwRsvdPageStartOffset; /* Reserve page start offset except
-					*  beacon in TxQ.
-					*/
 
 	/*  Add for dual MAC  0--Mac0 1--Mac1 */
 	u32	interfaceIndex;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/6] staging: rtl8188eu: remove a write-only power-index members
  2021-06-12 18:00 [PATCH 1/6] staging: rtl8188eu: remove unused hal_data_8188e members Martin Kaiser
  2021-06-12 18:00 ` [PATCH 2/6] staging: rtl8188eu: fix usb_submit_urb error handling Martin Kaiser
  2021-06-12 18:00 ` [PATCH 3/6] staging: rtl8188eu: remove a write-only struct member Martin Kaiser
@ 2021-06-12 18:00 ` Martin Kaiser
  2021-06-12 18:00 ` [PATCH 5/6] staging: rtl8188eu: remove another write-only member Martin Kaiser
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Martin Kaiser @ 2021-06-12 18:00 UTC (permalink / raw)
  To: Larry Finger, Greg Kroah-Hartman
  Cc: linux-staging, kernel-janitors, linux-kernel, Martin Kaiser

Remove power index members of struct hal_data_8188e that are written to
but never read.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/rtl8188eu/hal/phy.c              | 15 ---------------
 drivers/staging/rtl8188eu/include/rtl8188e_hal.h |  6 ------
 2 files changed, 21 deletions(-)

diff --git a/drivers/staging/rtl8188eu/hal/phy.c b/drivers/staging/rtl8188eu/hal/phy.c
index 5d9ad09ced70..256f87b9d630 100644
--- a/drivers/staging/rtl8188eu/hal/phy.c
+++ b/drivers/staging/rtl8188eu/hal/phy.c
@@ -162,18 +162,6 @@ static void get_tx_power_index(struct adapter *adapt, u8 channel, u8 *cck_pwr,
 	}
 }
 
-static void phy_power_index_check(struct adapter *adapt, u8 channel,
-				  u8 *cck_pwr, u8 *ofdm_pwr, u8 *bw20_pwr,
-				  u8 *bw40_pwr)
-{
-	struct hal_data_8188e *hal_data = adapt->HalData;
-
-	hal_data->CurrentCckTxPwrIdx = cck_pwr[0];
-	hal_data->CurrentOfdm24GTxPwrIdx = ofdm_pwr[0];
-	hal_data->CurrentBW2024GTxPwrIdx = bw20_pwr[0];
-	hal_data->CurrentBW4024GTxPwrIdx = bw40_pwr[0];
-}
-
 void phy_set_tx_power_level(struct adapter *adapt, u8 channel)
 {
 	u8 cck_pwr[MAX_TX_COUNT] = {0};
@@ -184,9 +172,6 @@ void phy_set_tx_power_level(struct adapter *adapt, u8 channel)
 	get_tx_power_index(adapt, channel, &cck_pwr[0], &ofdm_pwr[0],
 			   &bw20_pwr[0], &bw40_pwr[0]);
 
-	phy_power_index_check(adapt, channel, &cck_pwr[0], &ofdm_pwr[0],
-			      &bw20_pwr[0], &bw40_pwr[0]);
-
 	rtl88eu_phy_rf6052_set_cck_txpower(adapt, &cck_pwr[0]);
 	rtl88eu_phy_rf6052_set_ofdm_txpower(adapt, &ofdm_pwr[0], &bw20_pwr[0],
 					    &bw40_pwr[0], channel);
diff --git a/drivers/staging/rtl8188eu/include/rtl8188e_hal.h b/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
index 7b8ca0ea3008..20049bdf39f5 100644
--- a/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
+++ b/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
@@ -233,12 +233,6 @@ struct hal_data_8188e {
 	u8	PwrGroupHT20[RF_PATH_MAX][CHANNEL_MAX_NUMBER];
 	u8	PwrGroupHT40[RF_PATH_MAX][CHANNEL_MAX_NUMBER];
 
-	/*  The current Tx Power Level */
-	u8	CurrentCckTxPwrIdx;
-	u8	CurrentOfdm24GTxPwrIdx;
-	u8	CurrentBW2024GTxPwrIdx;
-	u8	CurrentBW4024GTxPwrIdx;
-
 	/*  Read/write are allow for following hardware information variables */
 	u8	framesync;
 	u8	pwrGroupCnt;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 5/6] staging: rtl8188eu: remove another write-only member
  2021-06-12 18:00 [PATCH 1/6] staging: rtl8188eu: remove unused hal_data_8188e members Martin Kaiser
                   ` (2 preceding siblings ...)
  2021-06-12 18:00 ` [PATCH 4/6] staging: rtl8188eu: remove a write-only power-index members Martin Kaiser
@ 2021-06-12 18:00 ` Martin Kaiser
  2021-06-12 18:00 ` [PATCH 6/6] staging: rtl8188eu: remove RT_TRACE and DBG_88E prints from usb_intf.c Martin Kaiser
  2021-06-20 17:40 ` [PATCH v2] staging: rtl8188eu: fix usb_submit_urb error handling Martin Kaiser
  5 siblings, 0 replies; 11+ messages in thread
From: Martin Kaiser @ 2021-06-12 18:00 UTC (permalink / raw)
  To: Larry Finger, Greg Kroah-Hartman
  Cc: linux-staging, kernel-janitors, linux-kernel, Martin Kaiser

bAPKThermalMeterIgnore in struct hal_data_8188e is never read and can
be removed.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/rtl8188eu/hal/rtl8188e_hal_init.c | 1 -
 drivers/staging/rtl8188eu/include/rtl8188e_hal.h  | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/staging/rtl8188eu/hal/rtl8188e_hal_init.c b/drivers/staging/rtl8188eu/hal/rtl8188e_hal_init.c
index 95b27b4df705..0b46af1755ed 100644
--- a/drivers/staging/rtl8188eu/hal/rtl8188e_hal_init.c
+++ b/drivers/staging/rtl8188eu/hal/rtl8188e_hal_init.c
@@ -567,7 +567,6 @@ void Hal_ReadThermalMeter_88E(struct adapter *Adapter, u8 *PROMContent, bool Aut
 		pHalData->EEPROMThermalMeter = EEPROM_Default_ThermalMeter_88E;
 
 	if (pHalData->EEPROMThermalMeter == 0xff || AutoloadFail) {
-		pHalData->bAPKThermalMeterIgnore = true;
 		pHalData->EEPROMThermalMeter = EEPROM_Default_ThermalMeter_88E;
 	}
 	DBG_88E("ThermalMeter = 0x%x\n", pHalData->EEPROMThermalMeter);
diff --git a/drivers/staging/rtl8188eu/include/rtl8188e_hal.h b/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
index 20049bdf39f5..0bfab5990d27 100644
--- a/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
+++ b/drivers/staging/rtl8188eu/include/rtl8188e_hal.h
@@ -208,7 +208,6 @@ struct hal_data_8188e {
 
 	u8	bTXPowerDataReadFromEEPORM;
 	u8	EEPROMThermalMeter;
-	u8	bAPKThermalMeterIgnore;
 
 	bool	EepromOrEfuse;
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 6/6] staging: rtl8188eu: remove RT_TRACE and DBG_88E prints from usb_intf.c
  2021-06-12 18:00 [PATCH 1/6] staging: rtl8188eu: remove unused hal_data_8188e members Martin Kaiser
                   ` (3 preceding siblings ...)
  2021-06-12 18:00 ` [PATCH 5/6] staging: rtl8188eu: remove another write-only member Martin Kaiser
@ 2021-06-12 18:00 ` Martin Kaiser
  2021-06-14 11:34   ` Dan Carpenter
  2021-06-20 17:40 ` [PATCH v2] staging: rtl8188eu: fix usb_submit_urb error handling Martin Kaiser
  5 siblings, 1 reply; 11+ messages in thread
From: Martin Kaiser @ 2021-06-12 18:00 UTC (permalink / raw)
  To: Larry Finger, Greg Kroah-Hartman
  Cc: linux-staging, kernel-janitors, linux-kernel, Martin Kaiser

These prints are disabled by default.

Replace the print after dev_alloc_name with proper error handling.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/rtl8188eu/os_dep/usb_intf.c | 30 +++------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/usb_intf.c b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 3a970d67aa8c..b03d6aa629ad 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
@@ -143,16 +143,6 @@ static void usb_dvobj_deinit(struct usb_interface *usb_intf)
 
 void usb_intf_stop(struct adapter *padapter)
 {
-	RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("+%s\n", __func__));
-
-	/* disable_hw_interrupt */
-	if (!padapter->bSurpriseRemoved) {
-		/* device still exists, so driver can do i/o operation */
-		/* TODO: */
-		RT_TRACE(_module_hci_intfs_c_, _drv_err_,
-			 ("SurpriseRemoved == false\n"));
-	}
-
 	/* cancel in irp */
 	rtw_hal_inirp_deinit(padapter);
 
@@ -160,14 +150,10 @@ void usb_intf_stop(struct adapter *padapter)
 	usb_write_port_cancel(padapter);
 
 	/* todo:cancel other irps */
-
-	RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("-%s\n", __func__));
 }
 
 static void rtw_dev_unload(struct adapter *padapter)
 {
-	RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("+%s\n", __func__));
-
 	if (padapter->bup) {
 		pr_debug("===> %s\n", __func__);
 		padapter->bDriverStopped = true;
@@ -186,14 +172,9 @@ static void rtw_dev_unload(struct adapter *padapter)
 		}
 
 		padapter->bup = false;
-	} else {
-		RT_TRACE(_module_hci_intfs_c_, _drv_err_,
-			 ("r871x_dev_unload():padapter->bup == false\n"));
 	}
 
 	pr_debug("<=== %s\n", __func__);
-
-	RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("-%s\n", __func__));
 }
 
 static int rtw_suspend(struct usb_interface *pusb_intf, pm_message_t message)
@@ -351,7 +332,6 @@ static int rtw_usb_if1_init(struct usb_interface *pusb_intf)
 
 	padapter->HalData = kzalloc(sizeof(struct hal_data_8188e), GFP_KERNEL);
 	if (!padapter->HalData) {
-		DBG_88E("Failed to allocate memory for HAL data\n");
 		err = -ENOMEM;
 		goto free_adapter;
 	}
@@ -367,8 +347,6 @@ static int rtw_usb_if1_init(struct usb_interface *pusb_intf)
 
 	/* step 5. */
 	if (rtw_init_drv_sw(padapter) == _FAIL) {
-		RT_TRACE(_module_hci_intfs_c_, _drv_err_,
-			 ("Initialize driver software resource Failed!\n"));
 		err = -ENOMEM;
 		goto free_hal_data;
 	}
@@ -388,8 +366,9 @@ static int rtw_usb_if1_init(struct usb_interface *pusb_intf)
 		pr_debug("can't get autopm:\n");
 
 	/*  alloc dev name after read efuse. */
-	if (dev_alloc_name(pnetdev, padapter->registrypriv.ifname) < 0)
-		RT_TRACE(_module_os_intfs_c_, _drv_err_, ("dev_alloc_name, fail!\n"));
+	err = dev_alloc_name(pnetdev, padapter->registrypriv.ifname)
+	if (err < 0)
+		goto free_hal_data;
 
 	netif_carrier_off(pnetdev);
 
@@ -401,7 +380,6 @@ static int rtw_usb_if1_init(struct usb_interface *pusb_intf)
 	/* step 6. Tell the network stack we exist */
 	err = register_netdev(pnetdev);
 	if (err) {
-		RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("register_netdev() failed\n"));
 		goto free_hal_data;
 	}
 
@@ -478,7 +456,6 @@ static void rtw_dev_remove(struct usb_interface *pusb_intf)
 	struct adapter *padapter = dvobj->if1;
 
 	pr_debug("+%s\n", __func__);
-	RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("+dev_remove()\n"));
 
 	if (!pusb_intf->unregistering)
 		padapter->bSurpriseRemoved = true;
@@ -492,7 +469,6 @@ 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"));
 	pr_debug("-r871xu_dev_remove, done\n");
 }
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 6/6] staging: rtl8188eu: remove RT_TRACE and DBG_88E prints from usb_intf.c
  2021-06-12 18:00 ` [PATCH 6/6] staging: rtl8188eu: remove RT_TRACE and DBG_88E prints from usb_intf.c Martin Kaiser
@ 2021-06-14 11:34   ` Dan Carpenter
  2021-06-14 14:58     ` Martin Kaiser
  0 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2021-06-14 11:34 UTC (permalink / raw)
  To: Martin Kaiser
  Cc: Larry Finger, Greg Kroah-Hartman, linux-staging, kernel-janitors,
	linux-kernel

On Sat, Jun 12, 2021 at 08:00:19PM +0200, Martin Kaiser wrote:
> These prints are disabled by default.
> 

Not, just by default.  There is literally no way to enable them.

> Replace the print after dev_alloc_name with proper error handling.
> 

Ugh...  :(  This part really needs to be done first and in a separate
patch.  You can delete the RT_TRACE() from that one call site since it's
on the same line but the subject of the patch needs to say something
like "check for allocation failure".  It can't be "remove RT_TRACE and
DBG_88E prints".

The first five of these patch look good though.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/6] staging: rtl8188eu: fix usb_submit_urb error handling
  2021-06-12 18:00 ` [PATCH 2/6] staging: rtl8188eu: fix usb_submit_urb error handling Martin Kaiser
@ 2021-06-14 14:46   ` Greg Kroah-Hartman
  2021-06-15  6:47     ` Dan Carpenter
  0 siblings, 1 reply; 11+ messages in thread
From: Greg Kroah-Hartman @ 2021-06-14 14:46 UTC (permalink / raw)
  To: Martin Kaiser
  Cc: Larry Finger, linux-staging, kernel-janitors, linux-kernel,
	Dan Carpenter

On Sat, Jun 12, 2021 at 08:00:15PM +0200, Martin Kaiser wrote:
> -EPERM should be handled like any other error.

Why?  This is not "any other error" for the usb core, right?

> 
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
> index ec07b2017fb7..0ceb05f3884f 100644
> --- a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
> +++ b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
> @@ -366,7 +366,6 @@ u32 usb_read_port(struct adapter *adapter, u32 addr, struct recv_buf *precvbuf)
>  	struct usb_device *pusbd = pdvobj->pusbdev;
>  	int err;
>  	unsigned int pipe;
> -	u32 ret = _SUCCESS;
>  
>  	if (adapter->bDriverStopped || adapter->bSurpriseRemoved ||
>  	    adapter->pwrctrlpriv.pnp_bstop_trx) {
> @@ -403,10 +402,10 @@ u32 usb_read_port(struct adapter *adapter, u32 addr, struct recv_buf *precvbuf)
>  			  precvbuf);/* context is precvbuf */
>  
>  	err = usb_submit_urb(purb, GFP_ATOMIC);
> -	if ((err) && (err != (-EPERM)))
> -		ret = _FAIL;

if -EPERM returns from this function, someone set the "reject" bit on
the urb.

Can this driver do that?  Where did this check originally come from, as
it feels like this was added for a good reason.

If this patch is "correct", I need a better changelog text explaining
why it is so :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 6/6] staging: rtl8188eu: remove RT_TRACE and DBG_88E prints from usb_intf.c
  2021-06-14 11:34   ` Dan Carpenter
@ 2021-06-14 14:58     ` Martin Kaiser
  0 siblings, 0 replies; 11+ messages in thread
From: Martin Kaiser @ 2021-06-14 14:58 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Larry Finger, Greg Kroah-Hartman, linux-staging, kernel-janitors,
	linux-kernel

Thus wrote Dan Carpenter (dan.carpenter@oracle.com):

> On Sat, Jun 12, 2021 at 08:00:19PM +0200, Martin Kaiser wrote:
> > These prints are disabled by default.


> Not, just by default.  There is literally no way to enable them.

> > Replace the print after dev_alloc_name with proper error handling.


> Ugh...  :(  This part really needs to be done first and in a separate
> patch.  You can delete the RT_TRACE() from that one call site since it's
> on the same line but the subject of the patch needs to say something
> like "check for allocation failure".  It can't be "remove RT_TRACE and
> DBG_88E prints".

ok, understood. I'll split this in two (and fix it, I forgot a
semicolon).

Thanks,

   Martin

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/6] staging: rtl8188eu: fix usb_submit_urb error handling
  2021-06-14 14:46   ` Greg Kroah-Hartman
@ 2021-06-15  6:47     ` Dan Carpenter
  0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2021-06-15  6:47 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Martin Kaiser, Larry Finger, linux-staging, kernel-janitors,
	linux-kernel

On Mon, Jun 14, 2021 at 04:46:41PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Jun 12, 2021 at 08:00:15PM +0200, Martin Kaiser wrote:
> > -EPERM should be handled like any other error.
> 
> Why?  This is not "any other error" for the usb core, right?
> 

Yeah.  It's a fair point that this commit message doesn't say why to do
it or explain the implications.

> > 
> > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> > ---
> >  drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
> > index ec07b2017fb7..0ceb05f3884f 100644
> > --- a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
> > +++ b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
> > @@ -366,7 +366,6 @@ u32 usb_read_port(struct adapter *adapter, u32 addr, struct recv_buf *precvbuf)
> >  	struct usb_device *pusbd = pdvobj->pusbdev;
> >  	int err;
> >  	unsigned int pipe;
> > -	u32 ret = _SUCCESS;
> >  
> >  	if (adapter->bDriverStopped || adapter->bSurpriseRemoved ||
> >  	    adapter->pwrctrlpriv.pnp_bstop_trx) {
> > @@ -403,10 +402,10 @@ u32 usb_read_port(struct adapter *adapter, u32 addr, struct recv_buf *precvbuf)
> >  			  precvbuf);/* context is precvbuf */
> >  
> >  	err = usb_submit_urb(purb, GFP_ATOMIC);
> > -	if ((err) && (err != (-EPERM)))
> > -		ret = _FAIL;
> 
> if -EPERM returns from this function, someone set the "reject" bit on
> the urb.
> 
> Can this driver do that?  Where did this check originally come from, as
> it feels like this was added for a good reason.
> 

Yeah.  It can cancel urbs in rtw_hal_inirp_deinit().  That function used
to have a better name, "usb_read_port_cancel" and in retrospect the
original name was probably better.

I think the reason for that -EPERM was treated differently was because
originally there were some error messages printed if usb_submit_urb()
failed.  (They were't actually printed because this code is buggy).  The
authors probably didn't want to print the error messages but
accidentally made it return success as well.

There is only one caller that checks the return and it only affects the
behavior if we race against open.  Can that even happen?  I'm pretty
sure that returning a failure is the correct behavior but I'm going to
leave it to Martin to check for absolutely sure.  :P

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] staging: rtl8188eu: fix usb_submit_urb error handling
  2021-06-12 18:00 [PATCH 1/6] staging: rtl8188eu: remove unused hal_data_8188e members Martin Kaiser
                   ` (4 preceding siblings ...)
  2021-06-12 18:00 ` [PATCH 6/6] staging: rtl8188eu: remove RT_TRACE and DBG_88E prints from usb_intf.c Martin Kaiser
@ 2021-06-20 17:40 ` Martin Kaiser
  5 siblings, 0 replies; 11+ messages in thread
From: Martin Kaiser @ 2021-06-20 17:40 UTC (permalink / raw)
  To: Larry Finger, Greg Kroah-Hartman
  Cc: linux-staging, kernel-janitors, linux-kernel, Martin Kaiser,
	Dan Carpenter

usb_read_port prepares a bulk in urb and calls usb_submit_urb to pass the
usb to the usb core. It seems wrong that usb_read_port returns success to
its caller if usb_submit_urb failed with -EPERM.

According to drivers/usb/core/urb.c, usb_submit_urb returns -EPERM when
an urb is resubmitted after being cancelled by usb_kill_urb etc.

The only caller who checks the return value of usb_read_port is
rtw_hal_inirp_init. This function submits the bulk in urbs for the first
time after the netdevice is opened. We'll not receive -EPERM from
usb_submit_urb in this case. The urbs are resubmitted by
read_port_complete, which does not check for errors from usb_read_port.

This driver may kill pending bulk in urbs when the netdevice is closed when
the driver is unloaded or the system goes to sleep. I don't think that this
will interrupt an ongoing netdev open.

Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
v2:
 - improved the commit message based on explanations from Greg and Dan

 drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
index ec07b2017fb7..0ceb05f3884f 100644
--- a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
+++ b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
@@ -366,7 +366,6 @@ u32 usb_read_port(struct adapter *adapter, u32 addr, struct recv_buf *precvbuf)
 	struct usb_device *pusbd = pdvobj->pusbdev;
 	int err;
 	unsigned int pipe;
-	u32 ret = _SUCCESS;
 
 	if (adapter->bDriverStopped || adapter->bSurpriseRemoved ||
 	    adapter->pwrctrlpriv.pnp_bstop_trx) {
@@ -403,10 +402,10 @@ u32 usb_read_port(struct adapter *adapter, u32 addr, struct recv_buf *precvbuf)
 			  precvbuf);/* context is precvbuf */
 
 	err = usb_submit_urb(purb, GFP_ATOMIC);
-	if ((err) && (err != (-EPERM)))
-		ret = _FAIL;
+	if (err)
+		return _FAIL;
 
-	return ret;
+	return _SUCCESS;
 }
 
 void rtw_hal_inirp_deinit(struct adapter *padapter)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-06-20 17:41 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-12 18:00 [PATCH 1/6] staging: rtl8188eu: remove unused hal_data_8188e members Martin Kaiser
2021-06-12 18:00 ` [PATCH 2/6] staging: rtl8188eu: fix usb_submit_urb error handling Martin Kaiser
2021-06-14 14:46   ` Greg Kroah-Hartman
2021-06-15  6:47     ` Dan Carpenter
2021-06-12 18:00 ` [PATCH 3/6] staging: rtl8188eu: remove a write-only struct member Martin Kaiser
2021-06-12 18:00 ` [PATCH 4/6] staging: rtl8188eu: remove a write-only power-index members Martin Kaiser
2021-06-12 18:00 ` [PATCH 5/6] staging: rtl8188eu: remove another write-only member Martin Kaiser
2021-06-12 18:00 ` [PATCH 6/6] staging: rtl8188eu: remove RT_TRACE and DBG_88E prints from usb_intf.c Martin Kaiser
2021-06-14 11:34   ` Dan Carpenter
2021-06-14 14:58     ` Martin Kaiser
2021-06-20 17:40 ` [PATCH v2] staging: rtl8188eu: fix usb_submit_urb error handling Martin Kaiser

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).