mwifiex: avoid deleting uninitialized timer during USB cleanup
diff mbox series

Message ID 1560354873-17182-1-git-send-email-gbhat@marvell.com
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series
  • mwifiex: avoid deleting uninitialized timer during USB cleanup
Related show

Commit Message

Ganapathi Bhat June 12, 2019, 3:54 p.m. UTC
Driver calls del_timer_sync(hold_timer), in unregister_dev(), but
there exists is a case when the timer is yet to be initialized. A
restructure of init and cleanup is needed to synchronize timer
creation and delee. Make use of init_if() / cleanup_if() handlers
to get this done.

Reported-by: syzbot+373e6719b49912399d21@syzkaller.appspotmail.com
Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
---
 drivers/net/wireless/marvell/mwifiex/usb.c | 32 +++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

Comments

Brian Norris June 14, 2019, 1:16 a.m. UTC | #1
Hi Ganapathi,

This looks kinda wrong, but I'm not totally sure, as I'm not very familiar with
your USB driver.

On Wed, Jun 12, 2019 at 09:24:33PM +0530, Ganapathi Bhat wrote:
> Driver calls del_timer_sync(hold_timer), in unregister_dev(), but
> there exists is a case when the timer is yet to be initialized. A
> restructure of init and cleanup is needed to synchronize timer
> creation and delee. Make use of init_if() / cleanup_if() handlers

s/delee/delete/

> to get this done.
> 
> Reported-by: syzbot+373e6719b49912399d21@syzkaller.appspotmail.com
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/usb.c | 32 +++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
> index c2365ee..939f1e9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -1348,6 +1348,8 @@ static void mwifiex_usb_cleanup_tx_aggr(struct mwifiex_adapter *adapter)
>  
>  	for (idx = 0; idx < MWIFIEX_TX_DATA_PORT; idx++) {
>  		port = &card->port[idx];
> +		if (!port->tx_data_ep)
> +			continue;

It's not clear to me what this is about. Are you sure you're not just cleaning
stuff up in the wrong order?

>  		if (adapter->bus_aggr.enable)
>  			while ((skb_tmp =
>  				skb_dequeue(&port->tx_aggr.aggr_list)))

...

> @@ -1584,7 +1580,29 @@ static void mwifiex_usb_submit_rem_rx_urbs(struct mwifiex_adapter *adapter)
>  	return 0;
>  }
>  
> +static int mwifiex_init_usb(struct mwifiex_adapter *adapter)
> +{
> +	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
> +	int ret = 0;
> +
> +	if (card->usb_boot_state == USB8XXX_FW_DNLD)
> +		return 0;

This looks wrong. You don't want to skip your basic initialization just because
firmware isn't loaded yet. In fact, init_if() always gets called before FW
init, so haven't you basically stubbed out this function most of the time?

I guess the question is: is this step supposed to go before, or after firmware
initilization? Based on that answer, we can make an appropriate patch.

(The original code does this after FW initialization, and now you're only sort
of moving it before.)

> +
> +	ret = mwifiex_usb_rx_init(adapter);
> +	if (!ret)
> +		ret = mwifiex_usb_tx_init(adapter);
> +
> +	return ret;
> +}

Brian

Patch
diff mbox series

diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index c2365ee..939f1e9 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -1348,6 +1348,8 @@  static void mwifiex_usb_cleanup_tx_aggr(struct mwifiex_adapter *adapter)
 
 	for (idx = 0; idx < MWIFIEX_TX_DATA_PORT; idx++) {
 		port = &card->port[idx];
+		if (!port->tx_data_ep)
+			continue;
 		if (adapter->bus_aggr.enable)
 			while ((skb_tmp =
 				skb_dequeue(&port->tx_aggr.aggr_list)))
@@ -1365,8 +1367,6 @@  static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 
 	mwifiex_usb_free(card);
 
-	mwifiex_usb_cleanup_tx_aggr(adapter);
-
 	card->adapter = NULL;
 }
 
@@ -1510,7 +1510,7 @@  static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter,
 static int mwifiex_usb_dnld_fw(struct mwifiex_adapter *adapter,
 			struct mwifiex_fw_image *fw)
 {
-	int ret;
+	int ret = 0;
 	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
 
 	if (card->usb_boot_state == USB8XXX_FW_DNLD) {
@@ -1523,10 +1523,6 @@  static int mwifiex_usb_dnld_fw(struct mwifiex_adapter *adapter,
 			return -1;
 	}
 
-	ret = mwifiex_usb_rx_init(adapter);
-	if (!ret)
-		ret = mwifiex_usb_tx_init(adapter);
-
 	return ret;
 }
 
@@ -1584,7 +1580,29 @@  static void mwifiex_usb_submit_rem_rx_urbs(struct mwifiex_adapter *adapter)
 	return 0;
 }
 
+static int mwifiex_init_usb(struct mwifiex_adapter *adapter)
+{
+	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
+	int ret = 0;
+
+	if (card->usb_boot_state == USB8XXX_FW_DNLD)
+		return 0;
+
+	ret = mwifiex_usb_rx_init(adapter);
+	if (!ret)
+		ret = mwifiex_usb_tx_init(adapter);
+
+	return ret;
+}
+
+static void mwifiex_cleanup_usb(struct mwifiex_adapter *adapter)
+{
+	mwifiex_usb_cleanup_tx_aggr(adapter);
+}
+
 static struct mwifiex_if_ops usb_ops = {
+	.init_if =		mwifiex_init_usb,
+	.cleanup_if =		mwifiex_cleanup_usb,
 	.register_dev =		mwifiex_register_dev,
 	.unregister_dev =	mwifiex_unregister_dev,
 	.wakeup =		mwifiex_pm_wakeup_card,