All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lorenzo Bianconi <lorenzo@kernel.org>
To: Helmut Grohne <helmut@subdivi.de>, Felix Fietkau <nbd@nbd.name>,
	Ryder Lee <ryder.lee@mediatek.com>,
	linux-wireless@vger.kernel.org,
	Stuart Hayhurst <stuart.a.hayhurst@gmail.com>,
	Shayne Chen <shayne.chen@mediatek.com>,
	Sean Wang <sean.wang@mediatek.com>,
	Deren Wu <deren.wu@mediatek.com>,
	netdev@vger.kernel.org, 1029116@bugs.debian.org
Subject: Re: [PATCH] wifi: mt76: mt7921: correctly handle removal in the absence of firmware
Date: Wed, 22 Feb 2023 12:18:36 +0100	[thread overview]
Message-ID: <Y/X6DPaxF0TMaUBh@lore-desk> (raw)
In-Reply-To: <Y/Ss5LYSYG2M7jSq@alf.mars>

[-- Attachment #1: Type: text/plain, Size: 6190 bytes --]

> Trying to probe a mt7921e pci card without firmware results in a
> successful probe where ieee80211_register_hw hasn't been called. When
> removing the driver, ieee802111_unregister_hw is called unconditionally
> leading to a kernel NULL pointer dereference among other things.
> 
> As with other drivers that delay registration after probe, we track the
> registration state in a flag variable and conidtionalize deregistration.
> 
> Link: https://bugs.debian.org/1029116
> Link: https://bugs.kali.org/view.php?id=8140
> Reported-by: Stuart Hayhurst <stuart.a.hayhurst@gmail.com>
> Fixes: 1c71e03afe4b ("mt76: mt7921: move mt7921_init_hw in a dedicated work")
> Signed-off-by: Helmut Grohne <helmut@freexian.com>
> Cc: stable@vger.kernel.org
> Sponsored-by: Freexian and Offensive Security

Hi Helmut,

Thx for working on this. I would say it is a more general issue since we have
dbdc support in mt7915/mt7996/mt7915 devices. I think we can move the check in
mac80211.c, what do you think? Something like (please note this patch is not
tested):

diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c
index b117e4467c87..34abf70f44af 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -539,6 +539,7 @@ int mt76_register_phy(struct mt76_phy *phy, bool vht,
 	if (ret)
 		return ret;
 
+	set_bit(MT76_STATE_REGISTERED, &phy->state);
 	phy->dev->phys[phy->band_idx] = phy;
 
 	return 0;
@@ -549,6 +550,9 @@ void mt76_unregister_phy(struct mt76_phy *phy)
 {
 	struct mt76_dev *dev = phy->dev;
 
+	if (!test_bit(MT76_STATE_REGISTERED, &phy->state))
+		return;
+
 	if (IS_ENABLED(CONFIG_MT76_LEDS))
 		mt76_led_cleanup(phy);
 	mt76_tx_status_check(dev, true);
@@ -719,6 +723,7 @@ int mt76_register_device(struct mt76_dev *dev, bool vht,
 		return ret;
 
 	WARN_ON(mt76_worker_setup(hw, &dev->tx_worker, NULL, "tx"));
+	set_bit(MT76_STATE_REGISTERED, &phy->state);
 	sched_set_fifo_low(dev->tx_worker.task);
 
 	return 0;
@@ -729,6 +734,9 @@ void mt76_unregister_device(struct mt76_dev *dev)
 {
 	struct ieee80211_hw *hw = dev->hw;
 
+	if (!test_bit(MT76_STATE_REGISTERED, &dev->phy.state))
+		return;
+
 	if (IS_ENABLED(CONFIG_MT76_LEDS))
 		mt76_led_cleanup(&dev->phy);
 	mt76_tx_status_check(dev, true);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 4ffb6be63571..38c311760d15 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -402,6 +402,7 @@ struct mt76_tx_cb {
 
 enum {
 	MT76_STATE_INITIALIZED,
+	MT76_STATE_REGISTERED,
 	MT76_STATE_RUNNING,
 	MT76_STATE_MCU_RUNNING,
 	MT76_SCANNING,

Regards,
Lorenzo

> ---
>  drivers/net/wireless/mediatek/mt76/mt7921/init.c   | 1 +
>  drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h | 1 +
>  drivers/net/wireless/mediatek/mt76/mt7921/pci.c    | 3 ++-
>  drivers/net/wireless/mediatek/mt76/mt7921/sdio.c   | 3 ++-
>  drivers/net/wireless/mediatek/mt76/mt7921/usb.c    | 3 ++-
>  5 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/init.c b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> index 542dfd425129..d5438212d5ff 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/init.c
> @@ -315,6 +315,7 @@ static void mt7921_init_work(struct work_struct *work)
>  		dev_err(dev->mt76.dev, "register device failed\n");
>  		return;
>  	}
> +	dev->hw_registered = true;
>  
>  	ret = mt7921_init_debugfs(dev);
>  	if (ret) {
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h b/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h
> index 15d6b7fe1c6c..e3b5d8ebf243 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/mt7921.h
> @@ -288,6 +288,7 @@ struct mt7921_dev {
>  	bool hw_full_reset:1;
>  	bool hw_init_done:1;
>  	bool fw_assert:1;
> +	bool hw_registered:1;
>  
>  	struct list_head sta_poll_list;
>  	spinlock_t sta_poll_lock;
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci.c b/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
> index cb72ded37256..1841eb7345dc 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci.c
> @@ -110,7 +110,8 @@ static void mt7921e_unregister_device(struct mt7921_dev *dev)
>  	struct mt76_connac_pm *pm = &dev->pm;
>  
>  	cancel_work_sync(&dev->init_work);
> -	mt76_unregister_device(&dev->mt76);
> +	if (dev->hw_registered)
> +		mt76_unregister_device(&dev->mt76);
>  	mt76_for_each_q_rx(&dev->mt76, i)
>  		napi_disable(&dev->mt76.napi[i]);
>  	cancel_delayed_work_sync(&pm->ps_work);
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> index 8ce4252b8ae7..23a9dd3c6450 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/sdio.c
> @@ -43,7 +43,8 @@ static void mt7921s_unregister_device(struct mt7921_dev *dev)
>  	struct mt76_connac_pm *pm = &dev->pm;
>  
>  	cancel_work_sync(&dev->init_work);
> -	mt76_unregister_device(&dev->mt76);
> +	if (dev->hw_registered)
> +		mt76_unregister_device(&dev->mt76);
>  	cancel_delayed_work_sync(&pm->ps_work);
>  	cancel_work_sync(&pm->wake_work);
>  
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/usb.c b/drivers/net/wireless/mediatek/mt76/mt7921/usb.c
> index 5321d20dcdcb..e55e1b50f760 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/usb.c
> @@ -301,7 +301,8 @@ static void mt7921u_disconnect(struct usb_interface *usb_intf)
>  	if (!test_bit(MT76_STATE_INITIALIZED, &dev->mphy.state))
>  		return;
>  
> -	mt76_unregister_device(&dev->mt76);
> +	if (dev->hw_registered)
> +		mt76_unregister_device(&dev->mt76);
>  	mt7921u_cleanup(dev);
>  
>  	usb_set_intfdata(usb_intf, NULL);
> -- 
> 2.39.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

  reply	other threads:[~2023-02-22 11:18 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-21 11:37 [PATCH] wifi: mt76: mt7921: correctly handle removal in the absence of firmware Helmut Grohne
2023-02-22 11:18 ` Lorenzo Bianconi [this message]
2023-02-22 14:42   ` Helmut Grohne

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y/X6DPaxF0TMaUBh@lore-desk \
    --to=lorenzo@kernel.org \
    --cc=1029116@bugs.debian.org \
    --cc=deren.wu@mediatek.com \
    --cc=helmut@subdivi.de \
    --cc=linux-wireless@vger.kernel.org \
    --cc=nbd@nbd.name \
    --cc=netdev@vger.kernel.org \
    --cc=ryder.lee@mediatek.com \
    --cc=sean.wang@mediatek.com \
    --cc=shayne.chen@mediatek.com \
    --cc=stuart.a.hayhurst@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.