[PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[Gabor Juhos]

An ioremap call is allowed to fail, however
the return value of that is not checked in
the 'rt2800pci_read_eeprom_soc' function.

The patch adds the missing check, and makes
the function return an int value. The patch
also converts the 'rt2800_read_eeprom' and
'rt2800_ops.read_eeprom' functions to return
an int value, so the error value can be
propagated up to the 'rt2800_validate_eeprom'
function.

Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
---
 drivers/net/wireless/rt2x00/rt2800lib.c |    5 ++++-
 drivers/net/wireless/rt2x00/rt2800lib.h |    6 +++---
 drivers/net/wireless/rt2x00/rt2800pci.c |   17 ++++++++++++-----
 drivers/net/wireless/rt2x00/rt2800usb.c |    4 +++-
 4 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
index 197b446..52b2978 100644
--- a/drivers/net/wireless/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/rt2x00/rt2800lib.c
@@ -4635,11 +4635,14 @@ static int rt2800_validate_eeprom(struct rt2x00_dev *rt2x00dev)
 	u16 word;
 	u8 *mac;
 	u8 default_lna_gain;
+	int retval;
 
 	/*
 	 * Read the EEPROM.
 	 */
-	rt2800_read_eeprom(rt2x00dev);
+	retval = rt2800_read_eeprom(rt2x00dev);
+	if (retval)
+		return retval;
 
 	/*
 	 * Start validation of the data that has been read.
diff --git a/drivers/net/wireless/rt2x00/rt2800lib.h b/drivers/net/wireless/rt2x00/rt2800lib.h
index a128cea..8252c67 100644
--- a/drivers/net/wireless/rt2x00/rt2800lib.h
+++ b/drivers/net/wireless/rt2x00/rt2800lib.h
@@ -43,7 +43,7 @@ struct rt2800_ops {
 			    const unsigned int offset,
 			    const struct rt2x00_field32 field, u32 *reg);
 
-	void (*read_eeprom)(struct rt2x00_dev *rt2x00dev);
+	int (*read_eeprom)(struct rt2x00_dev *rt2x00dev);
 	bool (*hwcrypt_disabled)(struct rt2x00_dev *rt2x00dev);
 
 	int (*drv_write_firmware)(struct rt2x00_dev *rt2x00dev,
@@ -117,11 +117,11 @@ static inline int rt2800_regbusy_read(struct rt2x00_dev *rt2x00dev,
 	return rt2800ops->regbusy_read(rt2x00dev, offset, field, reg);
 }
 
-static inline void rt2800_read_eeprom(struct rt2x00_dev *rt2x00dev)
+static inline int rt2800_read_eeprom(struct rt2x00_dev *rt2x00dev)
 {
 	const struct rt2800_ops *rt2800ops = rt2x00dev->ops->drv;
 
-	rt2800ops->read_eeprom(rt2x00dev);
+	return rt2800ops->read_eeprom(rt2x00dev);
 }
 
 static inline bool rt2800_hwcrypt_disabled(struct rt2x00_dev *rt2x00dev)
diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
index 9224d87..5fc16dd 100644
--- a/drivers/net/wireless/rt2x00/rt2800pci.c
+++ b/drivers/net/wireless/rt2x00/rt2800pci.c
@@ -90,17 +90,21 @@ static void rt2800pci_mcu_status(struct rt2x00_dev *rt2x00dev, const u8 token)
 }
 
 #if defined(CONFIG_RALINK_RT288X) || defined(CONFIG_RALINK_RT305X)
-static void rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
+static int rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
 {
 	void __iomem *base_addr = ioremap(0x1F040000, EEPROM_SIZE);
 
+	if (!base_addr)
+		return -ENOMEM;
+
 	memcpy_fromio(rt2x00dev->eeprom, base_addr, EEPROM_SIZE);
 
 	iounmap(base_addr);
 }
 #else
-static inline void rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
+static inline int rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
 {
+	return -ENOMEM;
 }
 #endif /* CONFIG_RALINK_RT288X || CONFIG_RALINK_RT305X */
 
@@ -970,14 +974,17 @@ static irqreturn_t rt2800pci_interrupt(int irq, void *dev_instance)
 /*
  * Device probe functions.
  */
-static void rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
+static int rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
 {
 	if (rt2x00_is_soc(rt2x00dev))
-		rt2800pci_read_eeprom_soc(rt2x00dev);
-	else if (rt2800pci_efuse_detect(rt2x00dev))
+		return rt2800pci_read_eeprom_soc(rt2x00dev);
+
+	if (rt2800pci_efuse_detect(rt2x00dev))
 		rt2800pci_read_eeprom_efuse(rt2x00dev);
 	else
 		rt2800pci_read_eeprom_pci(rt2x00dev);
+
+	return 0;
 }
 
 static const struct ieee80211_ops rt2800pci_mac80211_ops = {
diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index 5c149b5..48de5c9 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -735,13 +735,15 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
 /*
  * Device probe functions.
  */
-static void rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
+static int rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
 {
 	if (rt2800_efuse_detect(rt2x00dev))
 		rt2800_read_eeprom_efuse(rt2x00dev);
 	else
 		rt2x00usb_eeprom_read(rt2x00dev, rt2x00dev->eeprom,
 				      EEPROM_SIZE);
+
+	return 0;
 }
 
 static int rt2800usb_probe_hw(struct rt2x00_dev *rt2x00dev)
-- 
1.7.10

[PATCH 2/2] rt2x00: rt2800pci: allow to load EEPROM data via firmware API

[Gabor Juhos]

Currently the driver fetches the EEPROM data
from a fixed memory location for SoC devices
for SoC devices with a built-in wireless MAC.

The usability of this approach is quite
limited, because it is only suitable if the
location of the EEPROM data is mapped into
the memory. This condition is true on embedded
boards equipped which are using a parallel NOR
flash, but it is not true for boards equipped
with SPI or NAND flashes. The fixed location
also does not work in all cases, because the
offset of the EEPROM data varies between
different boards.

Additionally, various embedded boards are using
a PCI/PCIe chip soldered directly onto the PCB.
Such boards usually does not have a separate
EEPROM chip for the PCI/PCIe devices, the data
of the EEPROM is stored in the main flash
instead.

The patch makes it possible to load the EEPROM
data via firmware API. This new method works
regardless of the type of the flash, and it is
usable with built-in and with PCI/PCIe devices.

Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
---
 drivers/net/wireless/rt2x00/rt2800pci.c |   77 +++++++++++++++++++++++++------
 include/linux/platform_data/rt2800pci.h |   19 ++++++++
 2 files changed, 81 insertions(+), 15 deletions(-)
 create mode 100644 include/linux/platform_data/rt2800pci.h

diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
index 5fc16dd..a63a359 100644
--- a/drivers/net/wireless/rt2x00/rt2800pci.c
+++ b/drivers/net/wireless/rt2x00/rt2800pci.c
@@ -39,6 +39,8 @@
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/eeprom_93cx6.h>
+#include <linux/firmware.h>
+#include <linux/platform_data/rt2800pci.h>
 
 #include "rt2x00.h"
 #include "rt2x00pci.h"
@@ -47,6 +49,11 @@
 #include "rt2800.h"
 #include "rt2800pci.h"
 
+struct rt2800pci_eeprom_ctx {
+	struct completion complete;
+	const struct firmware *blob;
+};
+
 /*
  * Allow hardware encryption to be disabled.
  */
@@ -89,24 +96,49 @@ static void rt2800pci_mcu_status(struct rt2x00_dev *rt2x00dev, const u8 token)
 	rt2x00pci_register_write(rt2x00dev, H2M_MAILBOX_CID, ~0);
 }
 
-#if defined(CONFIG_RALINK_RT288X) || defined(CONFIG_RALINK_RT305X)
-static int rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
+static void rt2800pci_eeprom_request_cb(const struct firmware *blob,
+					void *ctx)
 {
-	void __iomem *base_addr = ioremap(0x1F040000, EEPROM_SIZE);
-
-	if (!base_addr)
-		return -ENOMEM;
+	struct rt2800pci_eeprom_ctx *ec = ctx;
 
-	memcpy_fromio(rt2x00dev->eeprom, base_addr, EEPROM_SIZE);
-
-	iounmap(base_addr);
+	ec->blob = blob;
+	complete(&ec->complete);
 }
-#else
-static inline int rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
+
+static int rt2800pci_read_eeprom_file(struct rt2x00_dev *rt2x00dev,
+				      const char *name)
 {
-	return -ENOMEM;
+
+	struct rt2800pci_eeprom_ctx ec = {};
+	int retval;
+
+	init_completion(&ec.complete);
+	retval = request_firmware_nowait(THIS_MODULE, 1, name,
+					 rt2x00dev->dev, GFP_KERNEL, &ec,
+					 rt2800pci_eeprom_request_cb);
+	if (retval < 0) {
+		ERROR(rt2x00dev, "EEPROM request failed\n");
+		return retval;
+	}
+
+	wait_for_completion(&ec.complete);
+	if (!ec.blob) {
+		ERROR(rt2x00dev, "Unable to load EEPROM file %s\n", name);
+		return -EINVAL;
+	}
+
+	if (ec.blob->size < EEPROM_SIZE) {
+		ERROR(rt2x00dev, "invalid EEPROM file %s\n", name);
+		retval = -EINVAL;
+		goto release_eeprom;
+	}
+
+	memcpy(rt2x00dev->eeprom, ec.blob->data, EEPROM_SIZE);
+	retval = 0;
+
+release_eeprom:
+	return retval;
 }
-#endif /* CONFIG_RALINK_RT288X || CONFIG_RALINK_RT305X */
 
 #ifdef CONFIG_PCI
 static void rt2800pci_eepromregister_read(struct eeprom_93cx6 *eeprom)
@@ -976,8 +1008,17 @@ static irqreturn_t rt2800pci_interrupt(int irq, void *dev_instance)
  */
 static int rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
 {
-	if (rt2x00_is_soc(rt2x00dev))
-		return rt2800pci_read_eeprom_soc(rt2x00dev);
+	struct rt2800pci_platform_data *pdata;
+
+	pdata = rt2x00dev->dev->platform_data;
+	if (pdata && pdata->eeprom_name) {
+		/* If the eeprom_name is provided via
+		 * platform_data, load the EEPROM content
+		 * from that file.
+		 */
+		return rt2800pci_read_eeprom_file(rt2x00dev,
+						  pdata->eeprom_name);
+	}
 
 	if (rt2800pci_efuse_detect(rt2x00dev))
 		rt2800pci_read_eeprom_efuse(rt2x00dev);
@@ -1173,6 +1214,12 @@ MODULE_LICENSE("GPL");
 #if defined(CONFIG_RALINK_RT288X) || defined(CONFIG_RALINK_RT305X)
 static int rt2800soc_probe(struct platform_device *pdev)
 {
+	struct rt2800pci_platform_data *pdata;
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata || !pdata->eeprom_name)
+		return -EINVAL;
+
 	return rt2x00soc_probe(pdev, &rt2800pci_ops);
 }
 
diff --git a/include/linux/platform_data/rt2800pci.h b/include/linux/platform_data/rt2800pci.h
new file mode 100644
index 0000000..1caced9
--- /dev/null
+++ b/include/linux/platform_data/rt2800pci.h
@@ -0,0 +1,19 @@
+/*
+ * Platform data definition for the rt2800pci driver
+ *
+ * Copyright (C) 2011-2012 Gabor Juhos <juhosg@openwrt.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ */
+
+#ifndef _RT2800PCI_PLATFORM_H
+#define _RT2800PCI_PLATFORM_H
+
+struct rt2800pci_platform_data {
+	const char *eeprom_name;
+};
+
+#endif /* _RT2800PCI_PLATFORM_H */
-- 
1.7.10

Re: [rt2x00-users] [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[Stanislaw Gruszka]

On Tue, Dec 18, 2012 at 05:22:22PM +0100, Gabor Juhos wrote:
> An ioremap call is allowed to fail, however
> the return value of that is not checked in
> the 'rt2800pci_read_eeprom_soc' function.
> 
> The patch adds the missing check, and makes
> the function return an int value. The patch
> also converts the 'rt2800_read_eeprom' and
> 'rt2800_ops.read_eeprom' functions to return
> an int value, so the error value can be
> propagated up to the 'rt2800_validate_eeprom'
> function.
> 
> Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
Reviewed-by: Stanislaw Gruszka <stf_xl@wp.pl>

Re: [rt2x00-users] [PATCH 2/2] rt2x00: rt2800pci: allow to load EEPROM data via firmware API

[Stanislaw Gruszka]

On Tue, Dec 18, 2012 at 05:22:23PM +0100, Gabor Juhos wrote:
> Currently the driver fetches the EEPROM data
> from a fixed memory location for SoC devices
> for SoC devices with a built-in wireless MAC.
> 
> The usability of this approach is quite
> limited, because it is only suitable if the
> location of the EEPROM data is mapped into
> the memory. This condition is true on embedded
> boards equipped which are using a parallel NOR
> flash, but it is not true for boards equipped
> with SPI or NAND flashes. The fixed location
> also does not work in all cases, because the
> offset of the EEPROM data varies between
> different boards.
> 
> Additionally, various embedded boards are using
> a PCI/PCIe chip soldered directly onto the PCB.
> Such boards usually does not have a separate
> EEPROM chip for the PCI/PCIe devices, the data
> of the EEPROM is stored in the main flash
> instead.
> 
> The patch makes it possible to load the EEPROM
> data via firmware API. This new method works
> regardless of the type of the flash, and it is
> usable with built-in and with PCI/PCIe devices.
> 
> Signed-off-by: Gabor Juhos <juhosg@openwrt.org>

I understand this patch will not broke NOR boards, which use
ioremap approach currently?

> +	init_completion(&ec.complete);
> +	retval = request_firmware_nowait(THIS_MODULE, 1, name,
> +					 rt2x00dev->dev, GFP_KERNEL, &ec,
> +					 rt2800pci_eeprom_request_cb);
> +	if (retval < 0) {
> +		ERROR(rt2x00dev, "EEPROM request failed\n");
> +		return retval;
> +	}
> +
> +	wait_for_completion(&ec.complete);
Since we use completion here, why we can not just use normal synchronous
version of request_firmware? I heard of request_firmware drawbacks, so
this approach can be correct. Just want to know if we do not complicate
things not necessarily here.

> +		goto release_eeprom;
> +	}
> +
> +	memcpy(rt2x00dev->eeprom, ec.blob->data, EEPROM_SIZE);
> +	retval = 0;
> +
> +release_eeprom:
We do not free memory - I guess we should do relase_firmware(ec.blob)?

Thanks
Stanislaw

Re: [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[Julian Calaby]

Hi Gabor,

One minor question:

On Wed, Dec 19, 2012 at 3:22 AM, Gabor Juhos <juhosg@openwrt.org> wrote:
> An ioremap call is allowed to fail, however
> the return value of that is not checked in
> the 'rt2800pci_read_eeprom_soc' function.
>
> The patch adds the missing check, and makes
> the function return an int value. The patch
> also converts the 'rt2800_read_eeprom' and
> 'rt2800_ops.read_eeprom' functions to return
> an int value, so the error value can be
> propagated up to the 'rt2800_validate_eeprom'
> function.
>
> Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
> ---
>  drivers/net/wireless/rt2x00/rt2800lib.c |    5 ++++-
>  drivers/net/wireless/rt2x00/rt2800lib.h |    6 +++---
>  drivers/net/wireless/rt2x00/rt2800pci.c |   17 ++++++++++++-----
>  drivers/net/wireless/rt2x00/rt2800usb.c |    4 +++-
>  4 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
> index 9224d87..5fc16dd 100644
> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
> @@ -970,14 +974,17 @@ static irqreturn_t rt2800pci_interrupt(int irq, void *dev_instance)
>  /*
>   * Device probe functions.
>   */
> -static void rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
> +static int rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
>  {
>         if (rt2x00_is_soc(rt2x00dev))
> -               rt2800pci_read_eeprom_soc(rt2x00dev);
> -       else if (rt2800pci_efuse_detect(rt2x00dev))
> +               return rt2800pci_read_eeprom_soc(rt2x00dev);
> +
> +       if (rt2800pci_efuse_detect(rt2x00dev))
>                 rt2800pci_read_eeprom_efuse(rt2x00dev);
>         else
>                 rt2800pci_read_eeprom_pci(rt2x00dev);

Would it make any sense to have rt2800pci_read_eeprom_efuse() and
rt2800pci_read_eeprom_pci() return a value, simply for consistency
with rt2800pci_read_eeprom_soc()?

> +
> +       return 0;
>  }
>
>  static const struct ieee80211_ops rt2800pci_mac80211_ops = {
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index 5c149b5..48de5c9 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -735,13 +735,15 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>  /*
>   * Device probe functions.
>   */
> -static void rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
> +static int rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
>  {
>         if (rt2800_efuse_detect(rt2x00dev))
>                 rt2800_read_eeprom_efuse(rt2x00dev);
>         else
>                 rt2x00usb_eeprom_read(rt2x00dev, rt2x00dev->eeprom,
>                                       EEPROM_SIZE);

Same here.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Re: [rt2x00-users] [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[Gabor Juhos]

2012.12.18. 22:57 keltezéssel, Stanislaw Gruszka írta:
> On Tue, Dec 18, 2012 at 05:22:22PM +0100, Gabor Juhos wrote:
>> An ioremap call is allowed to fail, however
>> the return value of that is not checked in
>> the 'rt2800pci_read_eeprom_soc' function.
>>
>> The patch adds the missing check, and makes
>> the function return an int value. The patch
>> also converts the 'rt2800_read_eeprom' and
>> 'rt2800_ops.read_eeprom' functions to return
>> an int value, so the error value can be
>> propagated up to the 'rt2800_validate_eeprom'
>> function.
>>
>> Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
> Reviewed-by: Stanislaw Gruszka <stf_xl@wp.pl>

Thanks!

-Gabor

Re: [rt2x00-users] [PATCH 2/2] rt2x00: rt2800pci: allow to load EEPROM data via firmware API

[Gabor Juhos]

2012.12.18. 23:22 keltezéssel, Stanislaw Gruszka írta:
> On Tue, Dec 18, 2012 at 05:22:23PM +0100, Gabor Juhos wrote:
>> Currently the driver fetches the EEPROM data
>> from a fixed memory location for SoC devices
>> for SoC devices with a built-in wireless MAC.
>>
>> The usability of this approach is quite
>> limited, because it is only suitable if the
>> location of the EEPROM data is mapped into
>> the memory. This condition is true on embedded
>> boards equipped which are using a parallel NOR
>> flash, but it is not true for boards equipped
>> with SPI or NAND flashes. The fixed location
>> also does not work in all cases, because the
>> offset of the EEPROM data varies between
>> different boards.
>>
>> Additionally, various embedded boards are using
>> a PCI/PCIe chip soldered directly onto the PCB.
>> Such boards usually does not have a separate
>> EEPROM chip for the PCI/PCIe devices, the data
>> of the EEPROM is stored in the main flash
>> instead.
>>
>> The patch makes it possible to load the EEPROM
>> data via firmware API. This new method works
>> regardless of the type of the flash, and it is
>> usable with built-in and with PCI/PCIe devices.
>>
>> Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
> 
> I understand this patch will not broke NOR boards, which use
> ioremap approach currently?

The change will break those obviously, so those boards must be converted to use
the new method. I have added sanity check into the 'rt2800soc_probe' function
which ensures that the users of such boards will be informed about that. FWIW,
that approach is used by out-of-tree boards only.

>> +	init_completion(&ec.complete);
>> +	retval = request_firmware_nowait(THIS_MODULE, 1, name,
>> +					 rt2x00dev->dev, GFP_KERNEL, &ec,
>> +					 rt2800pci_eeprom_request_cb);
>> +	if (retval < 0) {
>> +		ERROR(rt2x00dev, "EEPROM request failed\n");
>> +		return retval;
>> +	}
>> +
>> +	wait_for_completion(&ec.complete);
> Since we use completion here, why we can not just use normal synchronous
> version of request_firmware? I heard of request_firmware drawbacks, so
> this approach can be correct. Just want to know if we do not complicate
> things not necessarily here.

If the driver is built into the kernel, then the synchronous version would fail
because user-space is not up during probe time.

The initial version of the patch used the synchronous version, but Gertjan had
concerns about that:

http://rt2x00.serialmonkey.com/pipermail/users_rt2x00.serialmonkey.com/2012-December/005526.html

>> +		goto release_eeprom;
>> +	}
>> +
>> +	memcpy(rt2x00dev->eeprom, ec.blob->data, EEPROM_SIZE);
>> +	retval = 0;
>> +
>> +release_eeprom:
> We do not free memory - I guess we should do relase_firmware(ec.blob)?

Yes. I'm sure that I have added that call once, but it seems lost in the rebase
process. Will send and updated version.

Thank you for the comments!

-Gabor

Re: [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[Gabor Juhos]

2012.12.19. 0:58 keltezéssel, Julian Calaby írta:
> Hi Gabor,
> 
> One minor question:
> 
> On Wed, Dec 19, 2012 at 3:22 AM, Gabor Juhos <juhosg@openwrt.org> wrote:
>> An ioremap call is allowed to fail, however
>> the return value of that is not checked in
>> the 'rt2800pci_read_eeprom_soc' function.
>>
>> The patch adds the missing check, and makes
>> the function return an int value. The patch
>> also converts the 'rt2800_read_eeprom' and
>> 'rt2800_ops.read_eeprom' functions to return
>> an int value, so the error value can be
>> propagated up to the 'rt2800_validate_eeprom'
>> function.
>>
>> Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
>> ---
>>  drivers/net/wireless/rt2x00/rt2800lib.c |    5 ++++-
>>  drivers/net/wireless/rt2x00/rt2800lib.h |    6 +++---
>>  drivers/net/wireless/rt2x00/rt2800pci.c |   17 ++++++++++++-----
>>  drivers/net/wireless/rt2x00/rt2800usb.c |    4 +++-
>>  4 files changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
>> index 9224d87..5fc16dd 100644
>> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
>> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
>> @@ -970,14 +974,17 @@ static irqreturn_t rt2800pci_interrupt(int irq, void *dev_instance)
>>  /*
>>   * Device probe functions.
>>   */
>> -static void rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
>> +static int rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
>>  {
>>         if (rt2x00_is_soc(rt2x00dev))
>> -               rt2800pci_read_eeprom_soc(rt2x00dev);
>> -       else if (rt2800pci_efuse_detect(rt2x00dev))
>> +               return rt2800pci_read_eeprom_soc(rt2x00dev);
>> +
>> +       if (rt2800pci_efuse_detect(rt2x00dev))
>>                 rt2800pci_read_eeprom_efuse(rt2x00dev);
>>         else
>>                 rt2800pci_read_eeprom_pci(rt2x00dev);
> 
> Would it make any sense to have rt2800pci_read_eeprom_efuse() and
> rt2800pci_read_eeprom_pci() return a value, simply for consistency
> with rt2800pci_read_eeprom_soc()?

I wanted to keep the change as minimal as possible, but this would make sense.

> 
>> +
>> +       return 0;
>>  }
>>
>>  static const struct ieee80211_ops rt2800pci_mac80211_ops = {
>> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
>> index 5c149b5..48de5c9 100644
>> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
>> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
>> @@ -735,13 +735,15 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>>  /*
>>   * Device probe functions.
>>   */
>> -static void rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
>> +static int rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
>>  {
>>         if (rt2800_efuse_detect(rt2x00dev))
>>                 rt2800_read_eeprom_efuse(rt2x00dev);
>>         else
>>                 rt2x00usb_eeprom_read(rt2x00dev, rt2x00dev->eeprom,
>>                                       EEPROM_SIZE);
> 
> Same here.

Yes, especially that the 'rt2x00usb_eeprom_read' function returns an int value
already.

Thank you for the review!

-Gabor

Re: [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[Jones Desougi]

Hi Gabor,

One thing:

On 12/18/2012 05:22 PM, Gabor Juhos wrote:
> An ioremap call is allowed to fail, however
> the return value of that is not checked in
> the 'rt2800pci_read_eeprom_soc' function.
> 
> The patch adds the missing check, and makes
> the function return an int value. The patch
> also converts the 'rt2800_read_eeprom' and
> 'rt2800_ops.read_eeprom' functions to return
> an int value, so the error value can be
> propagated up to the 'rt2800_validate_eeprom'
> function.
> 
> Signed-off-by: Gabor Juhos <juhosg@openwrt.org>
> ---
>  drivers/net/wireless/rt2x00/rt2800lib.c |    5 ++++-
>  drivers/net/wireless/rt2x00/rt2800lib.h |    6 +++---
>  drivers/net/wireless/rt2x00/rt2800pci.c |   17 ++++++++++++-----
>  drivers/net/wireless/rt2x00/rt2800usb.c |    4 +++-
>  4 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
> index 197b446..52b2978 100644
> --- a/drivers/net/wireless/rt2x00/rt2800lib.c
> +++ b/drivers/net/wireless/rt2x00/rt2800lib.c
> @@ -4635,11 +4635,14 @@ static int rt2800_validate_eeprom(struct rt2x00_dev *rt2x00dev)
>  	u16 word;
>  	u8 *mac;
>  	u8 default_lna_gain;
> +	int retval;
>  
>  	/*
>  	 * Read the EEPROM.
>  	 */
> -	rt2800_read_eeprom(rt2x00dev);
> +	retval = rt2800_read_eeprom(rt2x00dev);
> +	if (retval)
> +		return retval;
>  
>  	/*
>  	 * Start validation of the data that has been read.
> diff --git a/drivers/net/wireless/rt2x00/rt2800lib.h b/drivers/net/wireless/rt2x00/rt2800lib.h
> index a128cea..8252c67 100644
> --- a/drivers/net/wireless/rt2x00/rt2800lib.h
> +++ b/drivers/net/wireless/rt2x00/rt2800lib.h
> @@ -43,7 +43,7 @@ struct rt2800_ops {
>  			    const unsigned int offset,
>  			    const struct rt2x00_field32 field, u32 *reg);
>  
> -	void (*read_eeprom)(struct rt2x00_dev *rt2x00dev);
> +	int (*read_eeprom)(struct rt2x00_dev *rt2x00dev);
>  	bool (*hwcrypt_disabled)(struct rt2x00_dev *rt2x00dev);
>  
>  	int (*drv_write_firmware)(struct rt2x00_dev *rt2x00dev,
> @@ -117,11 +117,11 @@ static inline int rt2800_regbusy_read(struct rt2x00_dev *rt2x00dev,
>  	return rt2800ops->regbusy_read(rt2x00dev, offset, field, reg);
>  }
>  
> -static inline void rt2800_read_eeprom(struct rt2x00_dev *rt2x00dev)
> +static inline int rt2800_read_eeprom(struct rt2x00_dev *rt2x00dev)
>  {
>  	const struct rt2800_ops *rt2800ops = rt2x00dev->ops->drv;
>  
> -	rt2800ops->read_eeprom(rt2x00dev);
> +	return rt2800ops->read_eeprom(rt2x00dev);
>  }
>  
>  static inline bool rt2800_hwcrypt_disabled(struct rt2x00_dev *rt2x00dev)
> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.c b/drivers/net/wireless/rt2x00/rt2800pci.c
> index 9224d87..5fc16dd 100644
> --- a/drivers/net/wireless/rt2x00/rt2800pci.c
> +++ b/drivers/net/wireless/rt2x00/rt2800pci.c
> @@ -90,17 +90,21 @@ static void rt2800pci_mcu_status(struct rt2x00_dev *rt2x00dev, const u8 token)
>  }
>  
>  #if defined(CONFIG_RALINK_RT288X) || defined(CONFIG_RALINK_RT305X)
> -static void rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
> +static int rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
>  {
>  	void __iomem *base_addr = ioremap(0x1F040000, EEPROM_SIZE);
>  
> +	if (!base_addr)
> +		return -ENOMEM;
> +
>  	memcpy_fromio(rt2x00dev->eeprom, base_addr, EEPROM_SIZE);
>  
>  	iounmap(base_addr);
>  }

Missing return at the end, since it should now return an int.
Even if this is gone with the second patch.

>  #else
> -static inline void rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
> +static inline int rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
>  {
> +	return -ENOMEM;
>  }
>  #endif /* CONFIG_RALINK_RT288X || CONFIG_RALINK_RT305X */
>  
> @@ -970,14 +974,17 @@ static irqreturn_t rt2800pci_interrupt(int irq, void *dev_instance)
>  /*
>   * Device probe functions.
>   */
> -static void rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
> +static int rt2800pci_read_eeprom(struct rt2x00_dev *rt2x00dev)
>  {
>  	if (rt2x00_is_soc(rt2x00dev))
> -		rt2800pci_read_eeprom_soc(rt2x00dev);
> -	else if (rt2800pci_efuse_detect(rt2x00dev))
> +		return rt2800pci_read_eeprom_soc(rt2x00dev);
> +
> +	if (rt2800pci_efuse_detect(rt2x00dev))
>  		rt2800pci_read_eeprom_efuse(rt2x00dev);
>  	else
>  		rt2800pci_read_eeprom_pci(rt2x00dev);
> +
> +	return 0;
>  }
>  
>  static const struct ieee80211_ops rt2800pci_mac80211_ops = {
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index 5c149b5..48de5c9 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -735,13 +735,15 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry,
>  /*
>   * Device probe functions.
>   */
> -static void rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
> +static int rt2800usb_read_eeprom(struct rt2x00_dev *rt2x00dev)
>  {
>  	if (rt2800_efuse_detect(rt2x00dev))
>  		rt2800_read_eeprom_efuse(rt2x00dev);
>  	else
>  		rt2x00usb_eeprom_read(rt2x00dev, rt2x00dev->eeprom,
>  				      EEPROM_SIZE);
> +
> +	return 0;
>  }
>  
>  static int rt2800usb_probe_hw(struct rt2x00_dev *rt2x00dev)
> 


Regards,
   /Jones

Re: [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[Gabor Juhos]

2012.12.19. 12:05 keltezéssel, Jones Desougi írta:
> Hi Gabor,
> 
> One thing:
> 
> On 12/18/2012 05:22 PM, Gabor Juhos wrote:
>> An ioremap call is allowed to fail, however
>> the return value of that is not checked in
>> the 'rt2800pci_read_eeprom_soc' function.
>>
>> The patch adds the missing check, and makes
>> the function return an int value. The patch
>> also converts the 'rt2800_read_eeprom' and
>> 'rt2800_ops.read_eeprom' functions to return
>> an int value, so the error value can be
>> propagated up to the 'rt2800_validate_eeprom'
>> function.
>>
>> Signed-off-by: Gabor Juhos <juhosg@openwrt.org>

>> +static int rt2800pci_read_eeprom_soc(struct rt2x00_dev *rt2x00dev)
>>  {
>>  	void __iomem *base_addr = ioremap(0x1F040000, EEPROM_SIZE);
>>  
>> +	if (!base_addr)
>> +		return -ENOMEM;
>> +
>>  	memcpy_fromio(rt2x00dev->eeprom, base_addr, EEPROM_SIZE);
>>  
>>  	iounmap(base_addr);
>>  }
> 
> Missing return at the end, since it should now return an int.

You are right.

> Even if this is gone with the second patch.

I will post an updated version of the patch-set anyway and will fix this.

Thank you for the review!

-Gabor

Re: [rt2x00-users] [PATCH 2/2] rt2x00: rt2800pci: allow to load EEPROM data via firmware API

[Kalle Valo]

Gabor Juhos <juhosg@openwrt.org> writes:

>> Since we use completion here, why we can not just use normal synchronous
>> version of request_firmware? I heard of request_firmware drawbacks, so
>> this approach can be correct. Just want to know if we do not complicate
>> things not necessarily here.
>
> If the driver is built into the kernel, then the synchronous version
> would fail because user-space is not up during probe time.

Didn't udev/systemd guys finally fix their software? At least I recall
seeing such claims on g+.

-- 
Kalle Valo

Re: [rt2x00-users] [PATCH 2/2] rt2x00: rt2800pci: allow to load EEPROM data via firmware API

[Stanislaw Gruszka]

On Thu, Dec 20, 2012 at 10:58:31AM +0200, Kalle Valo wrote:
> Gabor Juhos <juhosg@openwrt.org> writes:
> 
> >> Since we use completion here, why we can not just use normal synchronous
> >> version of request_firmware? I heard of request_firmware drawbacks, so
> >> this approach can be correct. Just want to know if we do not complicate
> >> things not necessarily here.
> >
> > If the driver is built into the kernel, then the synchronous version
> > would fail because user-space is not up during probe time.
> 
> Didn't udev/systemd guys finally fix their software? At least I recall
> seeing such claims on g+.

Udev broke and later fixed request_firmware when driver is compiled as
module. Here we avoid problem when driver is compiled into kernel
(problem which was there from very beginning) - no userspace available
so request_firmware fail. But since everyone are using modules, nobody
cares.

Openwrt must use modules too, since it use compat-wireless, right? So
I'm not sure if it worth to use request_firmware_nowait. Especially
like that with completion, which block ->probe. As long kernel does
not probes devices in parallel, we block whole boot, and deadlock since
probe wait for userspace.

Gabor, did you test your patches ?

Stanislaw

Re: [rt2x00-users] [PATCH 2/2] rt2x00: rt2800pci: allow to load EEPROM data via firmware API

[Gabor Juhos]

2012.12.20. 12:06 keltezéssel, Stanislaw Gruszka írta:
> On Thu, Dec 20, 2012 at 10:58:31AM +0200, Kalle Valo wrote:
>> Gabor Juhos <juhosg@openwrt.org> writes:
>>
>>>> Since we use completion here, why we can not just use normal synchronous
>>>> version of request_firmware? I heard of request_firmware drawbacks, so
>>>> this approach can be correct. Just want to know if we do not complicate
>>>> things not necessarily here.
>>>
>>> If the driver is built into the kernel, then the synchronous version
>>> would fail because user-space is not up during probe time.
>>
>> Didn't udev/systemd guys finally fix their software? At least I recall
>> seeing such claims on g+.
> 
> Udev broke and later fixed request_firmware when driver is compiled as
> module. Here we avoid problem when driver is compiled into kernel
> (problem which was there from very beginning) - no userspace available
> so request_firmware fail. But since everyone are using modules, nobody
> cares.
> 
> Openwrt must use modules too, since it use compat-wireless, right? 

Yes, we are using modules.

> So I'm not sure if it worth to use request_firmware_nowait. Especially like
> that with completion, which block ->probe. As long kernel does not probes
> devices in parallel, we block whole boot, and deadlock since probe wait for
> userspace.
>
> Gabor, did you test your patches ?

I have tested the patches, but only agains compat-wireless. I did not try the
rt2x00 driver built into the kernel. Although we are using 3.6.11 currently, but
the deadlock should happen on that as well. I will try it.

-Gabor

Re: [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[John W. Linville]

On Wed, Dec 19, 2012 at 12:59:26PM +0100, Gabor Juhos wrote:

> I will post an updated version of the patch-set anyway and will fix this.

Is this still coming?

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[Gabor Juhos]

2013.01.07. 21:02 keltezéssel, John W. Linville írta:
> On Wed, Dec 19, 2012 at 12:59:26PM +0100, Gabor Juhos wrote:
> 
>> I will post an updated version of the patch-set anyway and will fix this.
> 
> Is this still coming?

I have posted a modified version of 1/2 as a single patch:

https://patchwork.kernel.org/patch/1918781/

2/2 should be dropped for now.

-Gabor

Re: [PATCH 1/2] rt2x00: rt2800pci: verify ioremap return value

[John W. Linville]

On Mon, Jan 07, 2013 at 09:37:36PM +0100, Gabor Juhos wrote:
> 2013.01.07. 21:02 keltezéssel, John W. Linville írta:
> > On Wed, Dec 19, 2012 at 12:59:26PM +0100, Gabor Juhos wrote:
> > 
> >> I will post an updated version of the patch-set anyway and will fix this.
> > 
> > Is this still coming?
> 
> I have posted a modified version of 1/2 as a single patch:
> 
> https://patchwork.kernel.org/patch/1918781/

Yup, found it later.
 
> 2/2 should be dropped for now.

OK

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.