| Message ID | 1560354873-17182-1-git-send-email-gbhat@marvell.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | mwifiex: avoid deleting uninitialized timer during USB cleanup | expand |
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
Hi Brian, Sorry for delayed response; > > @@ -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? OK; > > > 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 think, we are not skipping the basic initialization: original flow: PID_1: mwifiex_usb_probe >>> "do driver initialization" >>> "download the firmware" PID_2: mwifiex_usb_probe >>> "do driver initialization" >>> "do interface initialization" >>> "do firmware initialization" new flow: PID_1: mwifiex_usb_probe >>> "do driver initialization" >>> "download the firmware" PID_2: mwifiex_usb_probe >>> "do interface initialization" >>> "do driver initialization" >>> "do firmware initialization" > > 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. It was before firmware initialization, in the existing driver( and the new change also follows this); > > (The original code does this after FW initialization, and now you're only sort > of moving it before.) I don't think so; as per the above flow, things which we moved to init_if() were previously done in mwifiex_usb_dnld_fw(), just before it return(for PID_2); FW initialization (mwifiex_init_fw()) happens after that; Regards, Ganapathi
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,
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(-)