Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 13 Jun 2019 18:16:50 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     Ganapathi Bhat <gbhat@marvell.com>
Cc:     linux-wireless@vger.kernel.org, Cathy Luo <cluo@marvell.com>,
        Zhiyuan Yang <yangzy@marvell.com>,
        James Cao <jcao@marvell.com>,
        Rakesh Parmar <rakeshp@marvell.com>,
        Dmitry Vyukov <dvyukov@google.com>
Subject: Re: [PATCH] mwifiex: avoid deleting uninitialized timer during USB
 cleanup
Message-ID: <20190614011648.GA121099@google.com>
References: <1560354873-17182-1-git-send-email-gbhat@marvell.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1560354873-17182-1-git-send-email-gbhat@marvell.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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