Re: [PATCH] mwifiex: avoid deleting uninitialized timer during USB cleanup

[Brian Norris <briannorris@chromium.org>]

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