[PATCH] carl9170: fix enum compare splat

[PATCH] carl9170: fix enum compare splat

[Christian Lamparter]

This patch fixes a noisy warning triggered by -Wenum-compare

|main.c:1390:31: warning: comparison between ‘enum nl80211_ac’
|	and ‘enum ar9170_txq’ [-Wenum-compare]
|  BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
|                               ^
| [...]

This is a little bit unfortunate, since the number of queues
(hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11
(much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or
less defined by the AR9170 hardware.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 drivers/net/wireless/ath/carl9170/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
index eaea08581cea..acc881bd7f9f 100644
--- a/drivers/net/wireless/ath/carl9170/main.c
+++ b/drivers/net/wireless/ath/carl9170/main.c
@@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
 	int ret;
 
 	BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ);
-	BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
+	BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ);
 	mutex_lock(&ar->mutex);
 	memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));
 	ret = carl9170_set_qos(ar);
-- 
2.20.1

[RFC PATCH v2] ath9k: add loader for AR92XX (and older) pci(e)

[Christian Lamparter]

Atheros cards with a AR92XX generation (and older) chip usually
store their pci(e) initialization vectors on an external eeprom chip.
However these chips technically don't need the eeprom chip attached,
the AR9280 Datasheet in section "6.1.2 DEVICE_ID" describes that
"... if the EEPROM content is not valid, a value of 0XFF1C returns
when read from the register". So the embedded devices like routers
and accesspoint usually have the pci(e) initialization vectors
stored on the system's FLASH, which is out of reach of the ath9k
chip.

Furthermore, Some devices (like the Cisco Meraki Z1 Cloud Managed
Teleworker Gateway) need to be able to initialize the PCIe wifi device.
Normally, this should be done as a pci quirk during the early stages of
booting linux. However, this isn't possible for devices which have the
init code for the Atheros chip stored on NAND in an UBI volume.
Hence, this module can be used to initialize the chip when the
user-space is ready to extract the init code.

Martin Blumenstingl prodived the following fixes:
owl-loader: add support for OWL emulation PCI devices
owl-loader: don't re-scan the bus when ath9k_pci_fixup failed
owl-loader: use dev_* instead of pr_* logging functions
owl-loader: auto-generate the eeprom filename as fallback
owl-loader: add a debug message when swapping the eeprom data
owl-loader: add missing newlines in log messages

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
v2: address Julian Calaby's comments:
    - make it a separate driver again (much like OpenWrt)
    - remove ar71xx leftovers (pdata)
---
 drivers/net/wireless/ath/ath9k/Kconfig        |  16 ++
 drivers/net/wireless/ath/ath9k/Makefile       |   2 +
 .../wireless/ath/ath9k/ath9k_pci_owl_loader.c | 215 ++++++++++++++++++
 3 files changed, 233 insertions(+)
 create mode 100644 drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c

diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig
index a1ef8769983a..d6a87698a44a 100644
--- a/drivers/net/wireless/ath/ath9k/Kconfig
+++ b/drivers/net/wireless/ath/ath9k/Kconfig
@@ -157,6 +157,22 @@ config ATH9K_PCOEM
 	depends on ATH9K
 	default y
 
+config ATH9K_PCI_NO_EEPROM
+	tristate "Atheros ath9k pci loader for EEPROM-less chips"
+	depends on ATH9K_PCI
+	default n
+	help
+	 This separate driver provides a loader in order to support the
+	 AR500X to AR92XX-generation of ath9k PCI(e) WiFi chips, which have
+	 their initialization data (which contains the real PCI Device ID
+	 that ath9k will need) stored together with the calibration data out
+	 of reach for the ath9k chip.
+
+	 These devices are usually various network appliances, routers or
+	 access Points and such.
+
+	 If unsure say N.
+
 config ATH9K_HTC
        tristate "Atheros HTC based wireless cards support"
        depends on USB && MAC80211
diff --git a/drivers/net/wireless/ath/ath9k/Makefile b/drivers/net/wireless/ath/ath9k/Makefile
index f71b2ad8275c..abd0f61370d9 100644
--- a/drivers/net/wireless/ath/ath9k/Makefile
+++ b/drivers/net/wireless/ath/ath9k/Makefile
@@ -77,3 +77,5 @@ ath9k_htc-y +=	htc_hst.o \
 ath9k_htc-$(CONFIG_ATH9K_HTC_DEBUGFS) += htc_drv_debug.o
 
 obj-$(CONFIG_ATH9K_HTC) += ath9k_htc.o
+
+obj-$(CONFIG_ATH9K_PCI_NO_EEPROM) += ath9k_pci_owl_loader.o
diff --git a/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c b/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c
new file mode 100644
index 000000000000..7ed495a9f1fe
--- /dev/null
+++ b/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Initialize Owl Emulation Devices
+ *
+ * Copyright (C) 2016 Christian Lamparter <chunkeey@gmail.com>
+ * Copyright (C) 2016 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+ *
+ * Some devices (like the Cisco Meraki Z1 Cloud Managed Teleworker Gateway)
+ * need to be able to initialize the PCIe wifi device. Normally, this is done
+ * during the early stages as a pci quirk.
+ * However, this isn't possible for devices which have the init code for the
+ * Atheros chip stored on UBI Volume on NAND. Hence, this module can be used to
+ * initialize the chip when the user-space is ready to extract the init code.
+ */
+#include <linux/module.h>
+#include <linux/version.h>
+#include <linux/completion.h>
+#include <linux/etherdevice.h>
+#include <linux/firmware.h>
+#include <linux/pci.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/ath9k_platform.h>
+
+struct owl_ctx {
+	struct completion eeprom_load;
+};
+
+#define EEPROM_FILENAME_LEN 100
+
+#define AR5416_EEPROM_MAGIC 0xa55a
+
+static int ath9k_pci_fixup(struct pci_dev *pdev, const u16 *cal_data,
+			   size_t cal_len)
+{
+	void __iomem *mem;
+	const void *cal_end = (void *)cal_data + cal_len;
+	const struct {
+		u16 reg;
+		u16 low_val;
+		u16 high_val;
+	} __packed * data;
+	u16 cmd;
+	u32 bar0;
+	bool swap_needed = false;
+
+	if (*cal_data != AR5416_EEPROM_MAGIC) {
+		if (*cal_data != swab16(AR5416_EEPROM_MAGIC)) {
+			dev_err(&pdev->dev, "invalid calibration data\n");
+			return -EINVAL;
+		}
+
+		dev_dbg(&pdev->dev, "calibration data needs swapping\n");
+		swap_needed = true;
+	}
+
+	dev_info(&pdev->dev, "fixup device configuration\n");
+
+	mem = pcim_iomap(pdev, 0, 0);
+	if (!mem) {
+		dev_err(&pdev->dev, "ioremap error\n");
+		return -EINVAL;
+	}
+
+	pci_read_config_dword(pdev, PCI_BASE_ADDRESS_0, &bar0);
+	pci_write_config_dword(pdev, PCI_BASE_ADDRESS_0,
+			       pci_resource_start(pdev, 0));
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	cmd |= PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY;
+	pci_write_config_word(pdev, PCI_COMMAND, cmd);
+
+	/* set pointer to first reg address */
+	for (data = (const void *)(cal_data + 3);
+	     (const void *)data <= cal_end && data->reg != (u16)~0;
+	     data++) {
+		u32 val;
+		u16 reg;
+
+		reg = data->reg;
+		val = data->low_val;
+		val |= ((u32)data->high_val) << 16;
+
+		if (swap_needed) {
+			reg = swab16(reg);
+			val = swahb32(val);
+		}
+
+		__raw_writel(val, mem + reg);
+		usleep_range(100, 120);
+	}
+
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	cmd &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);
+	pci_write_config_word(pdev, PCI_COMMAND, cmd);
+
+	pci_write_config_dword(pdev, PCI_BASE_ADDRESS_0, bar0);
+	pcim_iounmap(pdev, mem);
+
+	pci_disable_device(pdev);
+
+	return 0;
+}
+
+static void owl_fw_cb(const struct firmware *fw, void *context)
+{
+	struct pci_dev *pdev = (struct pci_dev *)context;
+	struct owl_ctx *ctx = (struct owl_ctx *)pci_get_drvdata(pdev);
+	struct pci_bus *bus;
+
+	complete(&ctx->eeprom_load);
+
+	if (!fw) {
+		dev_err(&pdev->dev, "no eeprom data received.\n");
+		goto release;
+	}
+
+	/* also note that we are doing *u16 operations on the file */
+	if (fw->size > 4096 || fw->size < 0x200 || (fw->size & 1) == 1) {
+		dev_err(&pdev->dev, "eeprom file has an invalid size.\n");
+		goto release;
+	}
+
+	if (ath9k_pci_fixup(pdev, (const u16 *)fw->data, fw->size))
+		goto release;
+
+	pci_lock_rescan_remove();
+	bus = pdev->bus;
+	pci_stop_and_remove_bus_device(pdev);
+	/* the device should come back with the proper
+	 * ProductId. But we have to initiate a rescan.
+	 */
+	pci_rescan_bus(bus);
+	pci_unlock_rescan_remove();
+
+release:
+	release_firmware(fw);
+}
+
+static const char *owl_get_eeprom_name(struct pci_dev *pdev)
+{
+	struct device *dev = &pdev->dev;
+	char *eeprom_name;
+
+	dev_dbg(dev, "using auto-generated eeprom filename\n");
+
+	eeprom_name = devm_kzalloc(dev, EEPROM_FILENAME_LEN, GFP_KERNEL);
+	if (!eeprom_name)
+		return NULL;
+
+	/* this should match the pattern used in ath9k/init.c */
+	scnprintf(eeprom_name, EEPROM_FILENAME_LEN, "ath9k-eeprom-pci-%s.bin",
+		  dev_name(dev));
+
+	return eeprom_name;
+}
+
+static int owl_probe(struct pci_dev *pdev,
+		     const struct pci_device_id *id)
+{
+	struct owl_ctx *ctx;
+	const char *eeprom_name;
+	int err = 0;
+
+	if (pcim_enable_device(pdev))
+		return -EIO;
+
+	pcim_pin_device(pdev);
+
+	eeprom_name = owl_get_eeprom_name(pdev);
+	if (!eeprom_name) {
+		dev_err(&pdev->dev, "no eeprom filename found.\n");
+		return -ENODEV;
+	}
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	init_completion(&ctx->eeprom_load);
+
+	pci_set_drvdata(pdev, ctx);
+	err = request_firmware_nowait(THIS_MODULE, true, eeprom_name,
+				      &pdev->dev, GFP_KERNEL, pdev, owl_fw_cb);
+	if (err)
+		dev_err(&pdev->dev, "failed to request caldata (%d).\n", err);
+
+	return err;
+}
+
+static void owl_remove(struct pci_dev *pdev)
+{
+	struct owl_ctx *ctx = pci_get_drvdata(pdev);
+
+	if (ctx) {
+		wait_for_completion(&ctx->eeprom_load);
+		pci_set_drvdata(pdev, NULL);
+	}
+}
+
+static const struct pci_device_id owl_pci_table[] = {
+	{ PCI_VDEVICE(ATHEROS, 0xff1c) }, /* PCIe */
+	{ PCI_VDEVICE(ATHEROS, 0xff1d) }, /* PCI */
+	{ },
+};
+MODULE_DEVICE_TABLE(pci, owl_pci_table);
+
+static struct pci_driver owl_driver = {
+	.name		= KBUILD_MODNAME,
+	.id_table	= owl_pci_table,
+	.probe		= owl_probe,
+	.remove		= owl_remove,
+};
+module_pci_driver(owl_driver);
+MODULE_AUTHOR("Christian Lamparter <chunkeey@gmail.com>");
+MODULE_DESCRIPTION("Initializes Atheros' Owl Emulation devices");
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

[PATCH v2] carl9170: fix misuse of device driver API

[Christian Lamparter]

This patch follows Alan Stern's recent patch:
"p54: Fix race between disconnect and firmware loading"

that overhauled carl9170 buggy firmware loading and driver
unbinding procedures.

Since the carl9170 code was adapted from p54 it uses the
same functions and is likely to have the same problem, but
it's just that the syzbot hasn't reproduce them (yet).

a summary from the changes (copied from the p54 patch):
 * Call usb_driver_release_interface() rather than
   device_release_driver().

 * Lock udev (the interface's parent) before unbinding the
   driver instead of locking udev->parent.

 * During the firmware loading process, take a reference
   to the USB interface instead of the USB device.

 * Don't take an unnecessary reference to the device during
   probe (and then don't drop it during disconnect).

and

 * Make sure to prevent use-after-free bugs by explicitly
   setting the driver context to NULL after signaling the
   completion.

Cc: <stable@vger.kernel.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
v2: Alan Stern's comments
  - fixed possible use-after-free
---
 drivers/net/wireless/ath/carl9170/usb.c | 39 +++++++++++--------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c
index e7c3f3b8457d..99f1897a775d 100644
--- a/drivers/net/wireless/ath/carl9170/usb.c
+++ b/drivers/net/wireless/ath/carl9170/usb.c
@@ -128,6 +128,8 @@ static const struct usb_device_id carl9170_usb_ids[] = {
 };
 MODULE_DEVICE_TABLE(usb, carl9170_usb_ids);
 
+static struct usb_driver carl9170_driver;
+
 static void carl9170_usb_submit_data_urb(struct ar9170 *ar)
 {
 	struct urb *urb;
@@ -966,32 +968,28 @@ static int carl9170_usb_init_device(struct ar9170 *ar)
 
 static void carl9170_usb_firmware_failed(struct ar9170 *ar)
 {
-	struct device *parent = ar->udev->dev.parent;
-	struct usb_device *udev;
-
-	/*
-	 * Store a copy of the usb_device pointer locally.
-	 * This is because device_release_driver initiates
-	 * carl9170_usb_disconnect, which in turn frees our
-	 * driver context (ar).
+	/* Store a copies of the usb_interface and usb_device pointer locally.
+	 * This is because release_driver initiates carl9170_usb_disconnect,
+	 * which in turn frees our driver context (ar).
 	 */
-	udev = ar->udev;
+	struct usb_interface *intf = ar->intf;
+	struct usb_device *udev = ar->udev;
 
 	complete(&ar->fw_load_wait);
+	/* at this point 'ar' could be already freed. Don't use it anymore */
+	ar = NULL;
 
 	/* unbind anything failed */
-	if (parent)
-		device_lock(parent);
-
-	device_release_driver(&udev->dev);
-	if (parent)
-		device_unlock(parent);
+	usb_lock_device(udev);
+	usb_driver_release_interface(&carl9170_driver, intf);
+	usb_unlock_device(udev);
 
-	usb_put_dev(udev);
+	usb_put_intf(intf);
 }
 
 static void carl9170_usb_firmware_finish(struct ar9170 *ar)
 {
+	struct usb_interface *intf = ar->intf;
 	int err;
 
 	err = carl9170_parse_firmware(ar);
@@ -1009,7 +1007,7 @@ static void carl9170_usb_firmware_finish(struct ar9170 *ar)
 		goto err_unrx;
 
 	complete(&ar->fw_load_wait);
-	usb_put_dev(ar->udev);
+	usb_put_intf(intf);
 	return;
 
 err_unrx:
@@ -1052,7 +1050,6 @@ static int carl9170_usb_probe(struct usb_interface *intf,
 		return PTR_ERR(ar);
 
 	udev = interface_to_usbdev(intf);
-	usb_get_dev(udev);
 	ar->udev = udev;
 	ar->intf = intf;
 	ar->features = id->driver_info;
@@ -1094,15 +1091,14 @@ static int carl9170_usb_probe(struct usb_interface *intf,
 	atomic_set(&ar->rx_anch_urbs, 0);
 	atomic_set(&ar->rx_pool_urbs, 0);
 
-	usb_get_dev(ar->udev);
+	usb_get_intf(intf);
 
 	carl9170_set_state(ar, CARL9170_STOPPED);
 
 	err = request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
 		&ar->udev->dev, GFP_KERNEL, ar, carl9170_usb_firmware_step2);
 	if (err) {
-		usb_put_dev(udev);
-		usb_put_dev(udev);
+		usb_put_intf(intf);
 		carl9170_free(ar);
 	}
 	return err;
@@ -1131,7 +1127,6 @@ static void carl9170_usb_disconnect(struct usb_interface *intf)
 
 	carl9170_release_firmware(ar);
 	carl9170_free(ar);
-	usb_put_dev(udev);
 }
 
 #ifdef CONFIG_PM
-- 
2.20.1

Re: [RFC PATCH v2] ath9k: add loader for AR92XX (and older) pci(e)

[Julian Calaby]

Hi Christian,

On Sun, Jun 9, 2019 at 12:49 AM Christian Lamparter <chunkeey@gmail.com> wrote:
>
> Atheros cards with a AR92XX generation (and older) chip usually
> store their pci(e) initialization vectors on an external eeprom chip.
> However these chips technically don't need the eeprom chip attached,
> the AR9280 Datasheet in section "6.1.2 DEVICE_ID" describes that
> "... if the EEPROM content is not valid, a value of 0XFF1C returns
> when read from the register". So the embedded devices like routers
> and accesspoint usually have the pci(e) initialization vectors
> stored on the system's FLASH, which is out of reach of the ath9k
> chip.
>
> Furthermore, Some devices (like the Cisco Meraki Z1 Cloud Managed
> Teleworker Gateway) need to be able to initialize the PCIe wifi device.
> Normally, this should be done as a pci quirk during the early stages of
> booting linux. However, this isn't possible for devices which have the
> init code for the Atheros chip stored on NAND in an UBI volume.
> Hence, this module can be used to initialize the chip when the
> user-space is ready to extract the init code.
>
> Martin Blumenstingl prodived the following fixes:
> owl-loader: add support for OWL emulation PCI devices
> owl-loader: don't re-scan the bus when ath9k_pci_fixup failed
> owl-loader: use dev_* instead of pr_* logging functions
> owl-loader: auto-generate the eeprom filename as fallback
> owl-loader: add a debug message when swapping the eeprom data
> owl-loader: add missing newlines in log messages
>
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> v2: address Julian Calaby's comments:
>     - make it a separate driver again (much like OpenWrt)
>     - remove ar71xx leftovers (pdata)

This looks sane to me. Thanks for splitting it out.

Reviewed-by: Julian Calaby <julian.calaby@gmail.com>

> ---
>  drivers/net/wireless/ath/ath9k/Kconfig        |  16 ++
>  drivers/net/wireless/ath/ath9k/Makefile       |   2 +
>  .../wireless/ath/ath9k/ath9k_pci_owl_loader.c | 215 ++++++++++++++++++
>  3 files changed, 233 insertions(+)
>  create mode 100644 drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c
>
> diff --git a/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c b/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c
> new file mode 100644
> index 000000000000..7ed495a9f1fe
> --- /dev/null
> +++ b/drivers/net/wireless/ath/ath9k/ath9k_pci_owl_loader.c
> @@ -0,0 +1,215 @@
> +module_pci_driver(owl_driver);
> +MODULE_AUTHOR("Christian Lamparter <chunkeey@gmail.com>");
> +MODULE_DESCRIPTION("Initializes Atheros' Owl Emulation devices");

Tiniest nit: something like "External EEPROM data loader for Atheros
AR500X to AR92XX" would make more sense to someone who isn't familiar
with the hardware.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH] carl9170: fix enum compare splat

[Kalle Valo]

Christian Lamparter <chunkeey@gmail.com> writes:

> This patch fixes a noisy warning triggered by -Wenum-compare
>
> |main.c:1390:31: warning: comparison between ‘enum nl80211_ac’
> |	and ‘enum ar9170_txq’ [-Wenum-compare]
> |  BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
> |                               ^
> | [...]
>
> This is a little bit unfortunate, since the number of queues
> (hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11
> (much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or
> less defined by the AR9170 hardware.

Is the warning enabled by default? TBH I'm not seeing how useful this
warning is for kernel development.

> --- a/drivers/net/wireless/ath/carl9170/main.c
> +++ b/drivers/net/wireless/ath/carl9170/main.c
> @@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
>  	int ret;
>  
>  	BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ);
> -	BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
> +	BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ);

IMHO this just makes the code worse. Does it make sense to workaround
(stupid) compiler warnings like this?

-- 
Kalle Valo

Re: [PATCH] carl9170: fix enum compare splat

[Christian Lamparter]

On Monday, June 10, 2019 9:06:30 AM CEST Kalle Valo wrote:
> Christian Lamparter <chunkeey@gmail.com> writes:
> 
> > This patch fixes a noisy warning triggered by -Wenum-compare
> >
> > |main.c:1390:31: warning: comparison between ‘enum nl80211_ac’
> > |	and ‘enum ar9170_txq’ [-Wenum-compare]
> > |  BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
> > |                               ^
> > | [...]
> >
> > This is a little bit unfortunate, since the number of queues
> > (hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11
> > (much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or
> > less defined by the AR9170 hardware.
> 
> Is the warning enabled by default? TBH I'm not seeing how useful this
> warning is for kernel development.

It is included in the "-Wall" (which is coming from "KBUILD_CFLAGS" 
in the main Makefile).

I tried debian's gcc starting from 4.6 to the lastest 8.3. They all
complain about it in various degrees.

https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Warning-Options.html#Warning-Options

> > --- a/drivers/net/wireless/ath/carl9170/main.c
> > +++ b/drivers/net/wireless/ath/carl9170/main.c
> > @@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
> >  	int ret;
> >  
> >  	BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ);
> > -	BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
> > +	BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ);
> 
> IMHO this just makes the code worse. Does it make sense to workaround
> (stupid) compiler warnings like this?
True. What's worse: This isn't really code. The BUILD_BUG_ON Macro is there
to guard but it's getting compiled away. I could also just drop it.

Re: [PATCH v2] carl9170: fix misuse of device driver API

[Alan Stern]

On Sat, 8 Jun 2019, Christian Lamparter wrote:

> This patch follows Alan Stern's recent patch:
> "p54: Fix race between disconnect and firmware loading"
> 
> that overhauled carl9170 buggy firmware loading and driver
> unbinding procedures.
> 
> Since the carl9170 code was adapted from p54 it uses the
> same functions and is likely to have the same problem, but
> it's just that the syzbot hasn't reproduce them (yet).
> 
> a summary from the changes (copied from the p54 patch):
>  * Call usb_driver_release_interface() rather than
>    device_release_driver().
> 
>  * Lock udev (the interface's parent) before unbinding the
>    driver instead of locking udev->parent.
> 
>  * During the firmware loading process, take a reference
>    to the USB interface instead of the USB device.
> 
>  * Don't take an unnecessary reference to the device during
>    probe (and then don't drop it during disconnect).
> 
> and
> 
>  * Make sure to prevent use-after-free bugs by explicitly
>    setting the driver context to NULL after signaling the
>    completion.
> 
> Cc: <stable@vger.kernel.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
> v2: Alan Stern's comments
>   - fixed possible use-after-free
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>


>  drivers/net/wireless/ath/carl9170/usb.c | 39 +++++++++++--------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c
> index e7c3f3b8457d..99f1897a775d 100644
> --- a/drivers/net/wireless/ath/carl9170/usb.c
> +++ b/drivers/net/wireless/ath/carl9170/usb.c
> @@ -128,6 +128,8 @@ static const struct usb_device_id carl9170_usb_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(usb, carl9170_usb_ids);
>  
> +static struct usb_driver carl9170_driver;
> +
>  static void carl9170_usb_submit_data_urb(struct ar9170 *ar)
>  {
>  	struct urb *urb;
> @@ -966,32 +968,28 @@ static int carl9170_usb_init_device(struct ar9170 *ar)
>  
>  static void carl9170_usb_firmware_failed(struct ar9170 *ar)
>  {
> -	struct device *parent = ar->udev->dev.parent;
> -	struct usb_device *udev;
> -
> -	/*
> -	 * Store a copy of the usb_device pointer locally.
> -	 * This is because device_release_driver initiates
> -	 * carl9170_usb_disconnect, which in turn frees our
> -	 * driver context (ar).
> +	/* Store a copies of the usb_interface and usb_device pointer locally.
> +	 * This is because release_driver initiates carl9170_usb_disconnect,
> +	 * which in turn frees our driver context (ar).
>  	 */
> -	udev = ar->udev;
> +	struct usb_interface *intf = ar->intf;
> +	struct usb_device *udev = ar->udev;
>  
>  	complete(&ar->fw_load_wait);
> +	/* at this point 'ar' could be already freed. Don't use it anymore */
> +	ar = NULL;
>  
>  	/* unbind anything failed */
> -	if (parent)
> -		device_lock(parent);
> -
> -	device_release_driver(&udev->dev);
> -	if (parent)
> -		device_unlock(parent);
> +	usb_lock_device(udev);
> +	usb_driver_release_interface(&carl9170_driver, intf);
> +	usb_unlock_device(udev);
>  
> -	usb_put_dev(udev);
> +	usb_put_intf(intf);
>  }
>  
>  static void carl9170_usb_firmware_finish(struct ar9170 *ar)
>  {
> +	struct usb_interface *intf = ar->intf;
>  	int err;
>  
>  	err = carl9170_parse_firmware(ar);
> @@ -1009,7 +1007,7 @@ static void carl9170_usb_firmware_finish(struct ar9170 *ar)
>  		goto err_unrx;
>  
>  	complete(&ar->fw_load_wait);
> -	usb_put_dev(ar->udev);
> +	usb_put_intf(intf);
>  	return;
>  
>  err_unrx:
> @@ -1052,7 +1050,6 @@ static int carl9170_usb_probe(struct usb_interface *intf,
>  		return PTR_ERR(ar);
>  
>  	udev = interface_to_usbdev(intf);
> -	usb_get_dev(udev);
>  	ar->udev = udev;
>  	ar->intf = intf;
>  	ar->features = id->driver_info;
> @@ -1094,15 +1091,14 @@ static int carl9170_usb_probe(struct usb_interface *intf,
>  	atomic_set(&ar->rx_anch_urbs, 0);
>  	atomic_set(&ar->rx_pool_urbs, 0);
>  
> -	usb_get_dev(ar->udev);
> +	usb_get_intf(intf);
>  
>  	carl9170_set_state(ar, CARL9170_STOPPED);
>  
>  	err = request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
>  		&ar->udev->dev, GFP_KERNEL, ar, carl9170_usb_firmware_step2);
>  	if (err) {
> -		usb_put_dev(udev);
> -		usb_put_dev(udev);
> +		usb_put_intf(intf);
>  		carl9170_free(ar);
>  	}
>  	return err;
> @@ -1131,7 +1127,6 @@ static void carl9170_usb_disconnect(struct usb_interface *intf)
>  
>  	carl9170_release_firmware(ar);
>  	carl9170_free(ar);
> -	usb_put_dev(udev);
>  }
>  
>  #ifdef CONFIG_PM
> 

Re: [PATCH] carl9170: fix enum compare splat

[Kalle Valo]

Christian Lamparter <chunkeey@gmail.com> writes:

> On Monday, June 10, 2019 9:06:30 AM CEST Kalle Valo wrote:
>> Christian Lamparter <chunkeey@gmail.com> writes:
>> 
>> > This patch fixes a noisy warning triggered by -Wenum-compare
>> >
>> > |main.c:1390:31: warning: comparison between ‘enum nl80211_ac’
>> > |	and ‘enum ar9170_txq’ [-Wenum-compare]
>> > |  BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
>> > |                               ^
>> > | [...]
>> >
>> > This is a little bit unfortunate, since the number of queues
>> > (hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11
>> > (much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or
>> > less defined by the AR9170 hardware.
>> 
>> Is the warning enabled by default? TBH I'm not seeing how useful this
>> warning is for kernel development.
>
> It is included in the "-Wall" (which is coming from "KBUILD_CFLAGS" 
> in the main Makefile).
>
> I tried debian's gcc starting from 4.6 to the lastest 8.3. They all
> complain about it in various degrees.
>
> https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Warning-Options.html#Warning-Options

Ok, odd that I haven't noticed this warning. Maybe I have been just
blind.

>> > --- a/drivers/net/wireless/ath/carl9170/main.c
>> > +++ b/drivers/net/wireless/ath/carl9170/main.c
>> > @@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
>> >  	int ret;
>> >  
>> >  	BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ);
>> > -	BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
>> > +	BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ);
>> 
>> IMHO this just makes the code worse. Does it make sense to workaround
>> (stupid) compiler warnings like this?
>
> True. What's worse: This isn't really code. The BUILD_BUG_ON Macro is there
> to guard but it's getting compiled away. I could also just drop it.

Either way is fine for me. If the GCC (by default) emits a warning for
this we need to silence that warning, so in that respect I guess we
should apply this. Unless better solutions come up, of course :)

-- 
Kalle Valo

Re: [PATCH] carl9170: fix enum compare splat

[Christian Lamparter]

On Tuesday, June 18, 2019 2:11:53 PM CEST Kalle Valo wrote:
> Christian Lamparter <chunkeey@gmail.com> writes:
> > On Monday, June 10, 2019 9:06:30 AM CEST Kalle Valo wrote:
> >> Christian Lamparter <chunkeey@gmail.com> writes:
> >> 
> >> > This patch fixes a noisy warning triggered by -Wenum-compare
> >> >
> >> > |main.c:1390:31: warning: comparison between ‘enum nl80211_ac’
> >> > |	and ‘enum ar9170_txq’ [-Wenum-compare]
> >> > |  BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
> >> > |                               ^
> >> > | [...]
> >> >
> >> > This is a little bit unfortunate, since the number of queues
> >> > (hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11
> >> > (much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or
> >> > less defined by the AR9170 hardware.
> >> 
> >> Is the warning enabled by default? TBH I'm not seeing how useful this
> >> warning is for kernel development.
> >
> > It is included in the "-Wall" (which is coming from "KBUILD_CFLAGS" 
> > in the main Makefile).
> >
> > I tried debian's gcc starting from 4.6 to the lastest 8.3. They all
> > complain about it in various degrees.
> >
> > https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Warning-Options.html#Warning-Options
> 
> Ok, odd that I haven't noticed this warning. Maybe I have been just
> blind.

Sorry. No, I messed up there. I made a patch back in the day during the spectre
mac80211 patches that I kept in my tree for the driver. But I now remember that
I never published those because of that exact "warning".
These lines are coming from there.
 
> >> > --- a/drivers/net/wireless/ath/carl9170/main.c
> >> > +++ b/drivers/net/wireless/ath/carl9170/main.c
> >> > @@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
> >> >  	int ret;
> >> >  
> >> >  	BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ);
> >> > -	BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
> >> > +	BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ);
> >> 
> >> IMHO this just makes the code worse. Does it make sense to workaround
> >> (stupid) compiler warnings like this?
> >
> > True. What's worse: This isn't really code. The BUILD_BUG_ON Macro is there
> > to guard but it's getting compiled away. I could also just drop it.
> 
> Either way is fine for me. If the GCC (by default) emits a warning for
> this we need to silence that warning, so in that respect I guess we
> should apply this. Unless better solutions come up, of course :)
Note: I dropped this patch from patchwork. So, there's nothing that
needs to be done now ;)

Well, except OT: Would you mind adding the QCA4019 boardfiles that
have accumulated over the past six-ish months?

<https://www.mail-archive.com/ath10k@lists.infradead.org/msg09966.html>

That list already had:
ath10k-firmware: QCA4019 hw1.0: Add ASUS Lyra specific BDFs
ath10k-firmware: QCA4019 hw1.0: Add Linksys EA6350v3
ath10k-firmware: QCA4019 hw1.0: Add Qxwlan E2600AC specific BDFs
ath10k-firmware: QCA4019 hw1.0: Update OpenMesh A62 specific BDFs
ath10k-firmware: QCA9888 hw2.0: Update OpenMesh A62 specific BDFs
ath10k-firmware: QCA4019 hw1.0: Update OpenMesh A42 specific BDFs
ath10k-firmware: QCA4019 hw1.0: Add ALFA Network AP120C-AC specific BDF
ath10k-firmware: QCA4019 hw1.0: Add AVM FRITZ!Box 7530 specific BDFs
ath10k-firmware: QCA4019 hw1.0: Update Linksys EA6350v3 specific BDFs

and now there's even more:

+ ath10k-firmware: QCA4019 hw1.0: Add Qxwlan E2600AC specific BDFs
 (Watch out, there are multiple versions! The first ones are dodgy
 with a bad crc) 
 This should be the right "one"
 <https://www.mail-archive.com/ath10k@lists.infradead.org/msg10007.html>

+ ath10k-firmware: QCA4019 hw1.0: Add AVM FRITZ!Repeater 3000 specific BDFs
 <https://www.mail-archive.com/ath10k@lists.infradead.org/msg10101.html>

+ ath10k-firmware: IPQ4018 hw1.0: Add EnGenius ENS620EXT specific BDFs
<http://lists.infradead.org/pipermail/ath10k/2019-March/013034.html>

+ ath10k-firmware: QCA9984 hw1.0: Update Netgear Orbi Pro SRK60 specific BDFs
  ath10k-firmware: QCA4019 hw1.0: Update Netgear Orbi Pro SRK60 specific BDFs

  One for QCA9984 and one for two QCA4019
<https://www.mail-archive.com/ath10k@lists.infradead.org/msg10170.html>
<https://www.mail-archive.com/ath10k@lists.infradead.org/msg10171.html>

+ ath10k-firmware: IPQ4019 hw1.0: Add BDF for Linksys EA8300 [1/2]
  ath10k-firmware: QCA9888 hw1.0: Add BDF for Linksys EA8300 [2/2]

<http://lists.infradead.org/pipermail/ath10k/2019-May/013403.html>
<http://lists.infradead.org/pipermail/ath10k/2019-May/013404.html>

Cheers,
Christian

Re: [PATCH] carl9170: fix enum compare splat

[Kalle Valo]

Christian Lamparter <chunkeey@gmail.com> writes:

> On Tuesday, June 18, 2019 2:11:53 PM CEST Kalle Valo wrote:
>> Christian Lamparter <chunkeey@gmail.com> writes:
>> > On Monday, June 10, 2019 9:06:30 AM CEST Kalle Valo wrote:
>> >> Christian Lamparter <chunkeey@gmail.com> writes:
>> >> 
>> >> > This patch fixes a noisy warning triggered by -Wenum-compare
>> >> >
>> >> > |main.c:1390:31: warning: comparison between ‘enum nl80211_ac’
>> >> > |	and ‘enum ar9170_txq’ [-Wenum-compare]
>> >> > |  BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
>> >> > |                               ^
>> >> > | [...]
>> >> >
>> >> > This is a little bit unfortunate, since the number of queues
>> >> > (hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11
>> >> > (much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or
>> >> > less defined by the AR9170 hardware.
>> >> 
>> >> Is the warning enabled by default? TBH I'm not seeing how useful this
>> >> warning is for kernel development.
>> >
>> > It is included in the "-Wall" (which is coming from "KBUILD_CFLAGS" 
>> > in the main Makefile).
>> >
>> > I tried debian's gcc starting from 4.6 to the lastest 8.3. They all
>> > complain about it in various degrees.
>> >
>> > https://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Warning-Options.html#Warning-Options
>> 
>> Ok, odd that I haven't noticed this warning. Maybe I have been just
>> blind.
>
> Sorry. No, I messed up there. I made a patch back in the day during the spectre
> mac80211 patches that I kept in my tree for the driver. But I now remember that
> I never published those because of that exact "warning".
> These lines are coming from there.
>  
>> >> > --- a/drivers/net/wireless/ath/carl9170/main.c
>> >> > +++ b/drivers/net/wireless/ath/carl9170/main.c
>> >> > @@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
>> >> >  	int ret;
>> >> >  
>> >> >  	BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ);
>> >> > -	BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
>> >> > +	BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ);
>> >> 
>> >> IMHO this just makes the code worse. Does it make sense to workaround
>> >> (stupid) compiler warnings like this?
>> >
>> > True. What's worse: This isn't really code. The BUILD_BUG_ON Macro is there
>> > to guard but it's getting compiled away. I could also just drop it.
>> 
>> Either way is fine for me. If the GCC (by default) emits a warning for
>> this we need to silence that warning, so in that respect I guess we
>> should apply this. Unless better solutions come up, of course :)
>
> Note: I dropped this patch from patchwork. So, there's nothing that
> needs to be done now ;)

Thanks, except I would prefer that I drop the patch myself :) In
numerous cases I have been wondering there a patch has disappeared and
only after a while I realise the submitter deleted the patch altogether
(which also destroys the conversation from patchwork etc). IMHO that's
really bad UX in patchwork, I should file a bug report about that.

So in general it's enough to say "Kalle, please drop the patch" and I'll
do the rest.

> Well, except OT: Would you mind adding the QCA4019 boardfiles that
> have accumulated over the past six-ish months?

Yeah, those are in my queue. But I'll first want to implement a script
so that I don't need to manually add all of them and I just haven't
found the time for that.

-- 
Kalle Valo

Re: [PATCH v2] carl9170: fix misuse of device driver API

[Kalle Valo]

Christian Lamparter <chunkeey@gmail.com> wrote:

> This patch follows Alan Stern's recent patch:
> "p54: Fix race between disconnect and firmware loading"
> 
> that overhauled carl9170 buggy firmware loading and driver
> unbinding procedures.
> 
> Since the carl9170 code was adapted from p54 it uses the
> same functions and is likely to have the same problem, but
> it's just that the syzbot hasn't reproduce them (yet).
> 
> a summary from the changes (copied from the p54 patch):
>  * Call usb_driver_release_interface() rather than
>    device_release_driver().
> 
>  * Lock udev (the interface's parent) before unbinding the
>    driver instead of locking udev->parent.
> 
>  * During the firmware loading process, take a reference
>    to the USB interface instead of the USB device.
> 
>  * Don't take an unnecessary reference to the device during
>    probe (and then don't drop it during disconnect).
> 
> and
> 
>  * Make sure to prevent use-after-free bugs by explicitly
>    setting the driver context to NULL after signaling the
>    completion.
> 
> Cc: <stable@vger.kernel.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

feb09b293327 carl9170: fix misuse of device driver API

-- 
https://patchwork.kernel.org/patch/10983223/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches