[PATCH AUTOSEL 5.4 18/52] Revert "ath6kl: return error code in ath6kl_wmi_set_roam_lrssi_cmd()"

[PATCH AUTOSEL 5.4 18/52] Revert "ath6kl: return error code in ath6kl_wmi_set_roam_lrssi_cmd()"

[Sasha Levin]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[ Upstream commit efba106f89fc6848726716c101f4c84e88720a9c ]

This reverts commit fc6a6521556c8250e356ddc6a3f2391aa62dc976.

Because of recent interactions with developers from @umn.edu, all
commits from them have been recently re-reviewed to ensure if they were
correct or not.

Upon review, this commit was found to be incorrect for the reasons
below, so it must be reverted.  It will be fixed up "correctly" in a
later kernel change.

The change being reverted does NOTHING as the caller to this function
does not even look at the return value of the call.  So the "claim" that
this fixed an an issue is not true.  It will be fixed up properly in a
future patch by propagating the error up the stack correctly.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Kalle Valo <kvalo@codeaurora.org>
Link: https://lore.kernel.org/r/20210503115736.2104747-43-gregkh@linuxfoundation.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/net/wireless/ath/ath6kl/wmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index c610fe21c85c..d27b4088b874 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -776,8 +776,10 @@ int ath6kl_wmi_set_roam_lrssi_cmd(struct wmi *wmi, u8 lrssi)
 	cmd->info.params.roam_rssi_floor = DEF_LRSSI_ROAM_FLOOR;
 	cmd->roam_ctrl = WMI_SET_LRSSI_SCAN_PARAMS;
 
-	return ath6kl_wmi_cmd_send(wmi, 0, skb, WMI_SET_ROAM_CTRL_CMDID,
+	ath6kl_wmi_cmd_send(wmi, 0, skb, WMI_SET_ROAM_CTRL_CMDID,
 			    NO_SYNC_WMIFLAG);
+
+	return 0;
 }
 
 int ath6kl_wmi_force_roam_cmd(struct wmi *wmi, const u8 *bssid)
-- 
2.30.2

[PATCH AUTOSEL 5.4 19/52] ath6kl: return error code in ath6kl_wmi_set_roam_lrssi_cmd()

[Sasha Levin]

From: Anirudh Rayabharam <mail@anirudhrb.com>

[ Upstream commit 54433367840b46a1555c8ed36c4c0cfc5dbf1358 ]

Propagate error code from failure of ath6kl_wmi_cmd_send() to the
caller.

Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Link: https://lore.kernel.org/r/20210503115736.2104747-44-gregkh@linuxfoundation.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/net/wireless/ath/ath6kl/debug.c | 5 ++++-
 drivers/net/wireless/ath/ath6kl/wmi.c   | 4 +---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/debug.c b/drivers/net/wireless/ath/ath6kl/debug.c
index 54337d60f288..085a134069f7 100644
--- a/drivers/net/wireless/ath/ath6kl/debug.c
+++ b/drivers/net/wireless/ath/ath6kl/debug.c
@@ -1027,14 +1027,17 @@ static ssize_t ath6kl_lrssi_roam_write(struct file *file,
 {
 	struct ath6kl *ar = file->private_data;
 	unsigned long lrssi_roam_threshold;
+	int ret;
 
 	if (kstrtoul_from_user(user_buf, count, 0, &lrssi_roam_threshold))
 		return -EINVAL;
 
 	ar->lrssi_roam_threshold = lrssi_roam_threshold;
 
-	ath6kl_wmi_set_roam_lrssi_cmd(ar->wmi, ar->lrssi_roam_threshold);
+	ret = ath6kl_wmi_set_roam_lrssi_cmd(ar->wmi, ar->lrssi_roam_threshold);
 
+	if (ret)
+		return ret;
 	return count;
 }
 
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index d27b4088b874..c610fe21c85c 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -776,10 +776,8 @@ int ath6kl_wmi_set_roam_lrssi_cmd(struct wmi *wmi, u8 lrssi)
 	cmd->info.params.roam_rssi_floor = DEF_LRSSI_ROAM_FLOOR;
 	cmd->roam_ctrl = WMI_SET_LRSSI_SCAN_PARAMS;
 
-	ath6kl_wmi_cmd_send(wmi, 0, skb, WMI_SET_ROAM_CTRL_CMDID,
+	return ath6kl_wmi_cmd_send(wmi, 0, skb, WMI_SET_ROAM_CTRL_CMDID,
 			    NO_SYNC_WMIFLAG);
-
-	return 0;
 }
 
 int ath6kl_wmi_force_roam_cmd(struct wmi *wmi, const u8 *bssid)
-- 
2.30.2

[PATCH AUTOSEL 5.4 24/52] Revert "libertas: add checks for the return value of sysfs_create_group"

[Sasha Levin]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[ Upstream commit 46651077765c80a0d6f87f3469129a72e49ce91b ]

This reverts commit 434256833d8eb988cb7f3b8a41699e2fe48d9332.

Because of recent interactions with developers from @umn.edu, all
commits from them have been recently re-reviewed to ensure if they were
correct or not.

Upon review, this commit was found to be incorrect for the reasons
below, so it must be reverted.  It will be fixed up "correctly" in a
later kernel change.

The original commit was incorrect, the error needs to be propagated back
to the caller AND if the second group call fails, the first needs to be
removed.  There are much better ways to solve this, the driver should
NOT be calling sysfs_create_group() on its own as it is racing userspace
and loosing.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Kalle Valo <kvalo@codeaurora.org>
Link: https://lore.kernel.org/r/20210503115736.2104747-53-gregkh@linuxfoundation.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/net/wireless/marvell/libertas/mesh.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/mesh.c b/drivers/net/wireless/marvell/libertas/mesh.c
index 2747c957d18c..a21c86d446fa 100644
--- a/drivers/net/wireless/marvell/libertas/mesh.c
+++ b/drivers/net/wireless/marvell/libertas/mesh.c
@@ -805,12 +805,7 @@ static void lbs_persist_config_init(struct net_device *dev)
 {
 	int ret;
 	ret = sysfs_create_group(&(dev->dev.kobj), &boot_opts_group);
-	if (ret)
-		pr_err("failed to create boot_opts_group.\n");
-
 	ret = sysfs_create_group(&(dev->dev.kobj), &mesh_ie_group);
-	if (ret)
-		pr_err("failed to create mesh_ie_group.\n");
 }
 
 static void lbs_persist_config_remove(struct net_device *dev)
-- 
2.30.2

[PATCH AUTOSEL 5.4 25/52] libertas: register sysfs groups properly

[Sasha Levin]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[ Upstream commit 7e79b38fe9a403b065ac5915465f620a8fb3de84 ]

The libertas driver was trying to register sysfs groups "by hand" which
causes them to be created _after_ the device is initialized and
announced to userspace, which causes races and can prevent userspace
tools from seeing the sysfs files correctly.

Fix this up by using the built-in sysfs_groups pointers in struct
net_device which were created for this very reason, fixing the race
condition, and properly allowing for any error that might have occured
to be handled properly.

Cc: Kalle Valo <kvalo@codeaurora.org>
Link: https://lore.kernel.org/r/20210503115736.2104747-54-gregkh@linuxfoundation.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/net/wireless/marvell/libertas/mesh.c | 28 +++-----------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/mesh.c b/drivers/net/wireless/marvell/libertas/mesh.c
index a21c86d446fa..050fd403110e 100644
--- a/drivers/net/wireless/marvell/libertas/mesh.c
+++ b/drivers/net/wireless/marvell/libertas/mesh.c
@@ -801,19 +801,6 @@ static const struct attribute_group mesh_ie_group = {
 	.attrs = mesh_ie_attrs,
 };
 
-static void lbs_persist_config_init(struct net_device *dev)
-{
-	int ret;
-	ret = sysfs_create_group(&(dev->dev.kobj), &boot_opts_group);
-	ret = sysfs_create_group(&(dev->dev.kobj), &mesh_ie_group);
-}
-
-static void lbs_persist_config_remove(struct net_device *dev)
-{
-	sysfs_remove_group(&(dev->dev.kobj), &boot_opts_group);
-	sysfs_remove_group(&(dev->dev.kobj), &mesh_ie_group);
-}
-
 
 /***************************************************************************
  * Initializing and starting, stopping mesh
@@ -1014,6 +1001,10 @@ static int lbs_add_mesh(struct lbs_private *priv)
 	SET_NETDEV_DEV(priv->mesh_dev, priv->dev->dev.parent);
 
 	mesh_dev->flags |= IFF_BROADCAST | IFF_MULTICAST;
+	mesh_dev->sysfs_groups[0] = &lbs_mesh_attr_group;
+	mesh_dev->sysfs_groups[1] = &boot_opts_group;
+	mesh_dev->sysfs_groups[2] = &mesh_ie_group;
+
 	/* Register virtual mesh interface */
 	ret = register_netdev(mesh_dev);
 	if (ret) {
@@ -1021,19 +1012,10 @@ static int lbs_add_mesh(struct lbs_private *priv)
 		goto err_free_netdev;
 	}
 
-	ret = sysfs_create_group(&(mesh_dev->dev.kobj), &lbs_mesh_attr_group);
-	if (ret)
-		goto err_unregister;
-
-	lbs_persist_config_init(mesh_dev);
-
 	/* Everything successful */
 	ret = 0;
 	goto done;
 
-err_unregister:
-	unregister_netdev(mesh_dev);
-
 err_free_netdev:
 	free_netdev(mesh_dev);
 
@@ -1054,8 +1036,6 @@ void lbs_remove_mesh(struct lbs_private *priv)
 
 	netif_stop_queue(mesh_dev);
 	netif_carrier_off(mesh_dev);
-	sysfs_remove_group(&(mesh_dev->dev.kobj), &lbs_mesh_attr_group);
-	lbs_persist_config_remove(mesh_dev);
 	unregister_netdev(mesh_dev);
 	priv->mesh_dev = NULL;
 	kfree(mesh_dev->ieee80211_ptr);
-- 
2.30.2

[PATCH AUTOSEL 5.4 38/52] Revert "brcmfmac: add a check for the status of usb_register"

[Sasha Levin]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[ Upstream commit 30a350947692f794796f563029d29764497f2887 ]

This reverts commit 42daad3343be4a4e1ee03e30a5f5cc731dadfef5.

Because of recent interactions with developers from @umn.edu, all
commits from them have been recently re-reviewed to ensure if they were
correct or not.

Upon review, this commit was found to be incorrect for the reasons
below, so it must be reverted.  It will be fixed up "correctly" in a
later kernel change.

The original commit here did nothing to actually help if usb_register()
failed, so it gives a "false sense of security" when there is none.  The
correct solution is to correctly unwind from this error.

Cc: Kangjie Lu <kjlu@umn.edu>
Cc: Kalle Valo <kvalo@codeaurora.org>
Link: https://lore.kernel.org/r/20210503115736.2104747-69-gregkh@linuxfoundation.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 575ed19e9195..6f41d28930e4 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1560,10 +1560,6 @@ void brcmf_usb_exit(void)
 
 void brcmf_usb_register(void)
 {
-	int ret;
-
 	brcmf_dbg(USB, "Enter\n");
-	ret = usb_register(&brcmf_usbdrvr);
-	if (ret)
-		brcmf_err("usb_register failed %d\n", ret);
+	usb_register(&brcmf_usbdrvr);
 }
-- 
2.30.2

[PATCH AUTOSEL 5.4 39/52] brcmfmac: properly check for bus register errors

[Sasha Levin]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]

The brcmfmac driver ignores any errors on initialization with the
different busses by deferring the initialization to a workqueue and
ignoring all possible errors that might happen.  Fix up all of this by
only allowing the module to load if all bus registering worked properly.

Cc: Kalle Valo <kvalo@codeaurora.org>
Link: https://lore.kernel.org/r/20210503115736.2104747-70-gregkh@linuxfoundation.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  8 +---
 .../broadcom/brcm80211/brcmfmac/bus.h         | 19 ++++++++-
 .../broadcom/brcm80211/brcmfmac/core.c        | 42 ++++++++-----------
 .../broadcom/brcm80211/brcmfmac/pcie.c        |  9 +---
 .../broadcom/brcm80211/brcmfmac/pcie.h        |  5 ---
 .../broadcom/brcm80211/brcmfmac/usb.c         |  4 +-
 6 files changed, 41 insertions(+), 46 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index fc12598b2dd3..c492d2d2db1d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -1168,13 +1168,9 @@ static struct sdio_driver brcmf_sdmmc_driver = {
 	},
 };
 
-void brcmf_sdio_register(void)
+int brcmf_sdio_register(void)
 {
-	int ret;
-
-	ret = sdio_register_driver(&brcmf_sdmmc_driver);
-	if (ret)
-		brcmf_err("sdio_register_driver failed: %d\n", ret);
+	return sdio_register_driver(&brcmf_sdmmc_driver);
 }
 
 void brcmf_sdio_exit(void)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
index 623c0168da79..8b27494a5d3d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
@@ -274,11 +274,26 @@ void brcmf_bus_add_txhdrlen(struct device *dev, uint len);
 
 #ifdef CONFIG_BRCMFMAC_SDIO
 void brcmf_sdio_exit(void);
-void brcmf_sdio_register(void);
+int brcmf_sdio_register(void);
+#else
+static inline void brcmf_sdio_exit(void) { }
+static inline int brcmf_sdio_register(void) { return 0; }
 #endif
+
 #ifdef CONFIG_BRCMFMAC_USB
 void brcmf_usb_exit(void);
-void brcmf_usb_register(void);
+int brcmf_usb_register(void);
+#else
+static inline void brcmf_usb_exit(void) { }
+static inline int brcmf_usb_register(void) { return 0; }
+#endif
+
+#ifdef CONFIG_BRCMFMAC_PCIE
+void brcmf_pcie_exit(void);
+int brcmf_pcie_register(void);
+#else
+static inline void brcmf_pcie_exit(void) { }
+static inline int brcmf_pcie_register(void) { return 0; }
 #endif
 
 #endif /* BRCMFMAC_BUS_H */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index e9bb8dbdc9aa..edb79e9665dc 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -1438,40 +1438,34 @@ void brcmf_bus_change_state(struct brcmf_bus *bus, enum brcmf_bus_state state)
 	}
 }
 
-static void brcmf_driver_register(struct work_struct *work)
-{
-#ifdef CONFIG_BRCMFMAC_SDIO
-	brcmf_sdio_register();
-#endif
-#ifdef CONFIG_BRCMFMAC_USB
-	brcmf_usb_register();
-#endif
-#ifdef CONFIG_BRCMFMAC_PCIE
-	brcmf_pcie_register();
-#endif
-}
-static DECLARE_WORK(brcmf_driver_work, brcmf_driver_register);
-
 int __init brcmf_core_init(void)
 {
-	if (!schedule_work(&brcmf_driver_work))
-		return -EBUSY;
+	int err;
+
+	err = brcmf_sdio_register();
+	if (err)
+		return err;
+
+	err = brcmf_usb_register();
+	if (err)
+		goto error_usb_register;
 
+	err = brcmf_pcie_register();
+	if (err)
+		goto error_pcie_register;
 	return 0;
+
+error_pcie_register:
+	brcmf_usb_exit();
+error_usb_register:
+	brcmf_sdio_exit();
+	return err;
 }
 
 void __exit brcmf_core_exit(void)
 {
-	cancel_work_sync(&brcmf_driver_work);
-
-#ifdef CONFIG_BRCMFMAC_SDIO
 	brcmf_sdio_exit();
-#endif
-#ifdef CONFIG_BRCMFMAC_USB
 	brcmf_usb_exit();
-#endif
-#ifdef CONFIG_BRCMFMAC_PCIE
 	brcmf_pcie_exit();
-#endif
 }
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index cb68f54a9c56..bda042138e96 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -2137,15 +2137,10 @@ static struct pci_driver brcmf_pciedrvr = {
 };
 
 
-void brcmf_pcie_register(void)
+int brcmf_pcie_register(void)
 {
-	int err;
-
 	brcmf_dbg(PCIE, "Enter\n");
-	err = pci_register_driver(&brcmf_pciedrvr);
-	if (err)
-		brcmf_err(NULL, "PCIE driver registration failed, err=%d\n",
-			  err);
+	return pci_register_driver(&brcmf_pciedrvr);
 }
 
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h
index d026401d2001..8e6c227e8315 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.h
@@ -11,9 +11,4 @@ struct brcmf_pciedev {
 	struct brcmf_pciedev_info *devinfo;
 };
 
-
-void brcmf_pcie_exit(void);
-void brcmf_pcie_register(void);
-
-
 #endif /* BRCMFMAC_PCIE_H */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index 6f41d28930e4..3b897f040371 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1558,8 +1558,8 @@ void brcmf_usb_exit(void)
 	usb_deregister(&brcmf_usbdrvr);
 }
 
-void brcmf_usb_register(void)
+int brcmf_usb_register(void)
 {
 	brcmf_dbg(USB, "Enter\n");
-	usb_register(&brcmf_usbdrvr);
+	return usb_register(&brcmf_usbdrvr);
 }
-- 
2.30.2

Re: [PATCH AUTOSEL 5.4 39/52] brcmfmac: properly check for bus register errors

[Arend van Spriel]

On 5/24/2021 4:48 PM, Sasha Levin wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
> 
> The brcmfmac driver ignores any errors on initialization with the
> different busses by deferring the initialization to a workqueue and
> ignoring all possible errors that might happen.  Fix up all of this by
> only allowing the module to load if all bus registering worked properly.

Hi Greg,

Saw this one flying by for stable kernel. Actually the first time I saw 
this patch, because I don't follow LKML as much as linux-wireless. The 
patch is fine, but wanted to give some context on the workqueue 
approach. It was there for historic reasons. Back then we had the UMH to 
provide firmware loading and because we request firmware during driver 
probe we could cause kernel boot to show significant delay when driver 
was built-in. Hence the workqueue which allowed kernel boot to proceed 
and driver probe was running in another thread context. These days we 
have direct firmware loading from the kernel and brcmfmac uses the 
asynchronous firmware loading API so there is indeed no longer a need 
for the workqueue.

Just for my understanding could you explain the motivation behind this 
change. In the preceding revert patch I saw this remark:

"""
The original commit here did nothing to actually help if usb_register()
failed, so it gives a "false sense of security" when there is none.  The
correct solution is to correctly unwind from this error.
"""

Does this mean the patch is addressing some security issue. Before your 
patch the module would remain loaded despite a bus register failure. I 
guess there is a story behind this that I am curious about.

Regards,
Arend

> Cc: Kalle Valo <kvalo@codeaurora.org>
> Link: https://lore.kernel.org/r/20210503115736.2104747-70-gregkh@linuxfoundation.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>   .../broadcom/brcm80211/brcmfmac/bcmsdh.c      |  8 +---
>   .../broadcom/brcm80211/brcmfmac/bus.h         | 19 ++++++++-
>   .../broadcom/brcm80211/brcmfmac/core.c        | 42 ++++++++-----------
>   .../broadcom/brcm80211/brcmfmac/pcie.c        |  9 +---
>   .../broadcom/brcm80211/brcmfmac/pcie.h        |  5 ---
>   .../broadcom/brcm80211/brcmfmac/usb.c         |  4 +-
>   6 files changed, 41 insertions(+), 46 deletions(-)

Re: [PATCH AUTOSEL 5.4 39/52] brcmfmac: properly check for bus register errors

[Greg Kroah-Hartman]

On Tue, May 25, 2021 at 08:38:34AM +0200, Arend van Spriel wrote:
> On 5/24/2021 4:48 PM, Sasha Levin wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
> > 
> > The brcmfmac driver ignores any errors on initialization with the
> > different busses by deferring the initialization to a workqueue and
> > ignoring all possible errors that might happen.  Fix up all of this by
> > only allowing the module to load if all bus registering worked properly.
> 
> Hi Greg,
> 
> Saw this one flying by for stable kernel. Actually the first time I saw this
> patch, because I don't follow LKML as much as linux-wireless. The patch is
> fine, but wanted to give some context on the workqueue approach. It was
> there for historic reasons. Back then we had the UMH to provide firmware
> loading and because we request firmware during driver probe we could cause
> kernel boot to show significant delay when driver was built-in. Hence the
> workqueue which allowed kernel boot to proceed and driver probe was running
> in another thread context. These days we have direct firmware loading from
> the kernel and brcmfmac uses the asynchronous firmware loading API so there
> is indeed no longer a need for the workqueue.
> 
> Just for my understanding could you explain the motivation behind this
> change. In the preceding revert patch I saw this remark:
> 
> """
> The original commit here did nothing to actually help if usb_register()
> failed, so it gives a "false sense of security" when there is none.  The
> correct solution is to correctly unwind from this error.
> """
> 
> Does this mean the patch is addressing some security issue. Before your
> patch the module would remain loaded despite a bus register failure. I guess
> there is a story behind this that I am curious about.

The module would remain loaded, yes, but nothing would work, and so no
one would have any idea that something went wrong.  The original commit
was wrong, it did not actually solve anything.

This commit properly propagates any error that happens back to the user,
like any other module being loaded.

thanks,

greg k-h

Re: [PATCH AUTOSEL 5.4 39/52] brcmfmac: properly check for bus register errors

[Arend Van Spriel]

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



On 5/25/2021 8:43 AM, Greg Kroah-Hartman wrote:
> On Tue, May 25, 2021 at 08:38:34AM +0200, Arend van Spriel wrote:
>> On 5/24/2021 4:48 PM, Sasha Levin wrote:
>>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>
>>> [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
>>>
>>> The brcmfmac driver ignores any errors on initialization with the
>>> different busses by deferring the initialization to a workqueue and
>>> ignoring all possible errors that might happen.  Fix up all of this by
>>> only allowing the module to load if all bus registering worked properly.
>>
>> Hi Greg,
>>
>> Saw this one flying by for stable kernel. Actually the first time I saw this
>> patch, because I don't follow LKML as much as linux-wireless. The patch is
>> fine, but wanted to give some context on the workqueue approach. It was
>> there for historic reasons. Back then we had the UMH to provide firmware
>> loading and because we request firmware during driver probe we could cause
>> kernel boot to show significant delay when driver was built-in. Hence the
>> workqueue which allowed kernel boot to proceed and driver probe was running
>> in another thread context. These days we have direct firmware loading from
>> the kernel and brcmfmac uses the asynchronous firmware loading API so there
>> is indeed no longer a need for the workqueue.
>>
>> Just for my understanding could you explain the motivation behind this
>> change. In the preceding revert patch I saw this remark:
>>
>> """
>> The original commit here did nothing to actually help if usb_register()
>> failed, so it gives a "false sense of security" when there is none.  The
>> correct solution is to correctly unwind from this error.
>> """
>>
>> Does this mean the patch is addressing some security issue. Before your
>> patch the module would remain loaded despite a bus register failure. I guess
>> there is a story behind this that I am curious about.
> 
> The module would remain loaded, yes, but nothing would work, and so no
> one would have any idea that something went wrong.  The original commit
> was wrong, it did not actually solve anything.

Agree.

> This commit properly propagates any error that happens back to the user,
> like any other module being loaded.

I understand, but this might cause a regression for the user. For 
instance if the usb_register() fails, but the other driver registrations 
succeed and the user has a wireless PCIe device. Before this change the 
user would have a functioning wifi device, but with this change it does not?

Regards,
Arend

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH AUTOSEL 5.4 39/52] brcmfmac: properly check for bus register errors

[Greg Kroah-Hartman]

On Tue, May 25, 2021 at 09:04:59AM +0200, Arend Van Spriel wrote:
> -- 
> This electronic communication and the information and any files transmitted
> with it, or attached to it, are confidential and are intended solely for the
> use of the individual or entity to whom it is addressed and may contain
> information that is confidential, legally privileged, protected by privacy
> laws, or otherwise restricted from disclosure to anyone else. If you are not
> the intended recipient or the person responsible for delivering the e-mail
> to the intended recipient, you are hereby notified that any use, copying,
> distributing, dissemination, forwarding, printing, or copying of this e-mail
> is strictly prohibited. If you received this e-mail in error, please return
> the e-mail to the sender, delete it from your computer, and destroy any
> printed copy of it.

Now deleted.

Re: [PATCH AUTOSEL 5.4 39/52] brcmfmac: properly check for bus register errors

[Arend van Spriel]

Resending without disclaimer

On 5/25/2021 9:04 AM, Arend Van Spriel wrote:
> 
> 
> On 5/25/2021 8:43 AM, Greg Kroah-Hartman wrote:
>> On Tue, May 25, 2021 at 08:38:34AM +0200, Arend van Spriel wrote:
>>> On 5/24/2021 4:48 PM, Sasha Levin wrote:
>>>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>
>>>> [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
>>>>
>>>> The brcmfmac driver ignores any errors on initialization with the
>>>> different busses by deferring the initialization to a workqueue and
>>>> ignoring all possible errors that might happen.  Fix up all of this by
>>>> only allowing the module to load if all bus registering worked 
>>>> properly.
>>>
>>> Hi Greg,
>>>
>>> Saw this one flying by for stable kernel. Actually the first time I 
>>> saw this
>>> patch, because I don't follow LKML as much as linux-wireless. The 
>>> patch is
>>> fine, but wanted to give some context on the workqueue approach. It was
>>> there for historic reasons. Back then we had the UMH to provide firmware
>>> loading and because we request firmware during driver probe we could 
>>> cause
>>> kernel boot to show significant delay when driver was built-in. Hence 
>>> the
>>> workqueue which allowed kernel boot to proceed and driver probe was 
>>> running
>>> in another thread context. These days we have direct firmware loading 
>>> from
>>> the kernel and brcmfmac uses the asynchronous firmware loading API so 
>>> there
>>> is indeed no longer a need for the workqueue.
>>>
>>> Just for my understanding could you explain the motivation behind this
>>> change. In the preceding revert patch I saw this remark:
>>>
>>> """
>>> The original commit here did nothing to actually help if usb_register()
>>> failed, so it gives a "false sense of security" when there is none.  The
>>> correct solution is to correctly unwind from this error.
>>> """
>>>
>>> Does this mean the patch is addressing some security issue. Before your
>>> patch the module would remain loaded despite a bus register failure. 
>>> I guess
>>> there is a story behind this that I am curious about.
>>
>> The module would remain loaded, yes, but nothing would work, and so no
>> one would have any idea that something went wrong.  The original commit
>> was wrong, it did not actually solve anything.
> 
> Agree.
> 
>> This commit properly propagates any error that happens back to the user,
>> like any other module being loaded.
> 
> I understand, but this might cause a regression for the user. For 
> instance if the usb_register() fails, but the other driver registrations 
> succeed and the user has a wireless PCIe device. Before this change the 
> user would have a functioning wifi device, but with this change it does 
> not?
> 
> Regards,
> Arend

Sorry for that. Corporate email can be a drag.

Regards,
Arend

Re: [PATCH AUTOSEL 5.4 39/52] brcmfmac: properly check for bus register errors

[Greg Kroah-Hartman]

On Tue, May 25, 2021 at 09:23:41AM +0200, Arend van Spriel wrote:
> Resending without disclaimer
> 
> On 5/25/2021 9:04 AM, Arend Van Spriel wrote:
> > 
> > 
> > On 5/25/2021 8:43 AM, Greg Kroah-Hartman wrote:
> > > On Tue, May 25, 2021 at 08:38:34AM +0200, Arend van Spriel wrote:
> > > > On 5/24/2021 4:48 PM, Sasha Levin wrote:
> > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > 
> > > > > [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
> > > > > 
> > > > > The brcmfmac driver ignores any errors on initialization with the
> > > > > different busses by deferring the initialization to a workqueue and
> > > > > ignoring all possible errors that might happen.  Fix up all of this by
> > > > > only allowing the module to load if all bus registering
> > > > > worked properly.
> > > > 
> > > > Hi Greg,
> > > > 
> > > > Saw this one flying by for stable kernel. Actually the first
> > > > time I saw this
> > > > patch, because I don't follow LKML as much as linux-wireless.
> > > > The patch is
> > > > fine, but wanted to give some context on the workqueue approach. It was
> > > > there for historic reasons. Back then we had the UMH to provide firmware
> > > > loading and because we request firmware during driver probe we
> > > > could cause
> > > > kernel boot to show significant delay when driver was built-in.
> > > > Hence the
> > > > workqueue which allowed kernel boot to proceed and driver probe
> > > > was running
> > > > in another thread context. These days we have direct firmware
> > > > loading from
> > > > the kernel and brcmfmac uses the asynchronous firmware loading
> > > > API so there
> > > > is indeed no longer a need for the workqueue.
> > > > 
> > > > Just for my understanding could you explain the motivation behind this
> > > > change. In the preceding revert patch I saw this remark:
> > > > 
> > > > """
> > > > The original commit here did nothing to actually help if usb_register()
> > > > failed, so it gives a "false sense of security" when there is none.  The
> > > > correct solution is to correctly unwind from this error.
> > > > """
> > > > 
> > > > Does this mean the patch is addressing some security issue. Before your
> > > > patch the module would remain loaded despite a bus register
> > > > failure. I guess
> > > > there is a story behind this that I am curious about.
> > > 
> > > The module would remain loaded, yes, but nothing would work, and so no
> > > one would have any idea that something went wrong.  The original commit
> > > was wrong, it did not actually solve anything.
> > 
> > Agree.
> > 
> > > This commit properly propagates any error that happens back to the user,
> > > like any other module being loaded.
> > 
> > I understand, but this might cause a regression for the user. For
> > instance if the usb_register() fails, but the other driver registrations
> > succeed and the user has a wireless PCIe device. Before this change the
> > user would have a functioning wifi device, but with this change it does
> > not?

If registering one of those other busses fails, you have major system
problems that need to be resolved and lots of other things will also
break.

You shouldn't just "eat error messages" and ignore them, as that's what
is what was happening here, you could have had errors and never knew it.

thanks,

greg k-h

Re: [PATCH AUTOSEL 5.4 39/52] brcmfmac: properly check for bus register errors

[Arend van Spriel]

On 5/25/2021 9:26 AM, Greg Kroah-Hartman wrote:
> On Tue, May 25, 2021 at 09:23:41AM +0200, Arend van Spriel wrote:
>> Resending without disclaimer
>>
>> On 5/25/2021 9:04 AM, Arend Van Spriel wrote:
>>>
>>>
>>> On 5/25/2021 8:43 AM, Greg Kroah-Hartman wrote:
>>>> On Tue, May 25, 2021 at 08:38:34AM +0200, Arend van Spriel wrote:
>>>>> On 5/24/2021 4:48 PM, Sasha Levin wrote:
>>>>>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>>>
>>>>>> [ Upstream commit 419b4a142a7ece36cebcd434f8ce2af59ef94b85 ]
>>>>>>
>>>>>> The brcmfmac driver ignores any errors on initialization with the
>>>>>> different busses by deferring the initialization to a workqueue and
>>>>>> ignoring all possible errors that might happen.  Fix up all of this by
>>>>>> only allowing the module to load if all bus registering
>>>>>> worked properly.
>>>>>
>>>>> Hi Greg,
>>>>>
>>>>> Saw this one flying by for stable kernel. Actually the first
>>>>> time I saw this
>>>>> patch, because I don't follow LKML as much as linux-wireless.
>>>>> The patch is
>>>>> fine, but wanted to give some context on the workqueue approach. It was
>>>>> there for historic reasons. Back then we had the UMH to provide firmware
>>>>> loading and because we request firmware during driver probe we
>>>>> could cause
>>>>> kernel boot to show significant delay when driver was built-in.
>>>>> Hence the
>>>>> workqueue which allowed kernel boot to proceed and driver probe
>>>>> was running
>>>>> in another thread context. These days we have direct firmware
>>>>> loading from
>>>>> the kernel and brcmfmac uses the asynchronous firmware loading
>>>>> API so there
>>>>> is indeed no longer a need for the workqueue.
>>>>>
>>>>> Just for my understanding could you explain the motivation behind this
>>>>> change. In the preceding revert patch I saw this remark:
>>>>>
>>>>> """
>>>>> The original commit here did nothing to actually help if usb_register()
>>>>> failed, so it gives a "false sense of security" when there is none.  The
>>>>> correct solution is to correctly unwind from this error.
>>>>> """
>>>>>
>>>>> Does this mean the patch is addressing some security issue. Before your
>>>>> patch the module would remain loaded despite a bus register
>>>>> failure. I guess
>>>>> there is a story behind this that I am curious about.
>>>>
>>>> The module would remain loaded, yes, but nothing would work, and so no
>>>> one would have any idea that something went wrong.  The original commit
>>>> was wrong, it did not actually solve anything.
>>>
>>> Agree.
>>>
>>>> This commit properly propagates any error that happens back to the user,
>>>> like any other module being loaded.
>>>
>>> I understand, but this might cause a regression for the user. For
>>> instance if the usb_register() fails, but the other driver registrations
>>> succeed and the user has a wireless PCIe device. Before this change the
>>> user would have a functioning wifi device, but with this change it does
>>> not?
> 
> If registering one of those other busses fails, you have major system
> problems that need to be resolved and lots of other things will also
> break.

Right.

> You shouldn't just "eat error messages" and ignore them, as that's what
> is what was happening here, you could have had errors and never knew it.

As said earlier I agree with the patch. I just thought I might learn 
something today, because there was more to it than I could find in the 
commit message. :-p

Regards,
Arend