RFC: A workaround for BCM43XX devices with no on-board SPROM

RFC: A workaround for BCM43XX devices with no on-board SPROM

[Larry Finger]

Michael,

I'm switching this discussion from the kernel Bugzilla to the lists.

As you know, but I'm restating for anyone that has not read our previous
discussions, the b43 driver needs to be changed to handle some of the newer
devices do not have an on-board SPROM. It would be trivial to incorporate the
data except for the need to have a unique, reproducible MAC.

I am proposing to solve this problem using the following steps:

(1) Modify b43-fwcutter to take data from an existing SPROM, modify the MAC by
replacing the last 3 octets with random numbers, and write the resulting file to
/lib/firmware/b43. Fortunately, all affected devices seem to have Revision 8
SPROMS, which makes preparation easier. All such devices will need to use the
calibration parameters of the device from which the prototype SPROM was copied,
but that should not be a serious problem. I have chosen to implement this in
fwcutter rather than ssb_sprom because the ordinary user will not have access to
ssb_sprom; however, they do have a version of fwcutter supplied by the distro.
Unconditionally writing an additional small file to the firmware directory when
extracting firmware is small overhead and it will be transparent to the user of
whatever mechanism the distro uses. The routines needed to calculate the CRC,
etc. have been copied into fwcutter from ssb_sprom. A version of this code is
already running.

(2) Use the steps in http://bcm-v4.sipsolutions.net/802.11/IsSpromAvailable to
determine if the device has an SPROM. If not, then use the kernel's firmware
loading mechanism to get the contents of the file prepared in step 1. This file
has an 8-bit CRC, thus the validity of the file can be tested even though the
test is not very robust.

It it reasonable to keep the vendor portion of the MAC and only replace the
"serial number", or would it be better to randomize all 6 octants?

Is there a better way to load a file into the kernel?

Thanks,

Larry

Re: RFC: A workaround for BCM43XX devices with no on-board SPROM

[Michael Buesch]

On Thursday 18 March 2010 18:46:35 Larry Finger wrote:
> (1) Modify b43-fwcutter to take data from an existing SPROM,

Why not extend the ssb-sprom tool? I don't think this has anything to do with
firmware, except that we (ab)use the firmware loading mechanism of the kernel
for loading the blob into the kernel.

> I have chosen to implement this in
> fwcutter rather than ssb_sprom because the ordinary user will not have access to
> ssb_sprom;

Huh? ssb-sprom is GPL software. I have no problem relicensing it under BSD or
even something more liberal. I don't see a problem for "ordinary users" here.

> however, they do have a version of fwcutter supplied by the distro.

Well, but that version won't do anything on the SPROM, too.

> It it reasonable to keep the vendor portion of the MAC and only replace the
> "serial number", or would it be better to randomize all 6 octants?

I think it doesn't really matter.

-- 
Greetings, Michael.

Re: RFC: A workaround for BCM43XX devices with no on-board SPROM

[John W. Linville]

On Thu, Mar 18, 2010 at 08:31:24PM +0100, Michael Buesch wrote:
> On Thursday 18 March 2010 18:46:35 Larry Finger wrote:

> > It it reasonable to keep the vendor portion of the MAC and only replace the
> > "serial number", or would it be better to randomize all 6 octants?
> 
> I think it doesn't really matter.

It might be a good idea to set the LAA bit...?

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

Re: RFC: A workaround for BCM43XX devices with no on-board SPROM

[Johannes Berg]

On Thu, 2010-03-18 at 16:20 -0400, John W. Linville wrote:
> On Thu, Mar 18, 2010 at 08:31:24PM +0100, Michael Buesch wrote:
> > On Thursday 18 March 2010 18:46:35 Larry Finger wrote:
> 
> > > It it reasonable to keep the vendor portion of the MAC and only replace the
> > > "serial number", or would it be better to randomize all 6 octants?
> > 
> > I think it doesn't really matter.
> 
> It might be a good idea to set the LAA bit...?

And clear the mcast bit :)

johannes

Re: RFC: A workaround for BCM43XX devices with no on-board SPROM

[Nicolas de Pesloüan]

Larry Finger wrote :
> Michael,
> 
> I'm switching this discussion from the kernel Bugzilla to the lists.
> 
> As you know, but I'm restating for anyone that has not read our previous
> discussions, the b43 driver needs to be changed to handle some of the newer
> devices do not have an on-board SPROM. It would be trivial to incorporate the
> data except for the need to have a unique, reproducible MAC.
> 
> I am proposing to solve this problem using the following steps:
> 
> (1) Modify b43-fwcutter to take data from an existing SPROM, modify the MAC by
> replacing the last 3 octets with random numbers, and write the resulting file to
> /lib/firmware/b43. Fortunately, all affected devices seem to have Revision 8
> SPROMS, which makes preparation easier. All such devices will need to use the
> calibration parameters of the device from which the prototype SPROM was copied,
> but that should not be a serious problem. I have chosen to implement this in
> fwcutter rather than ssb_sprom because the ordinary user will not have access to
> ssb_sprom; however, they do have a version of fwcutter supplied by the distro.
> Unconditionally writing an additional small file to the firmware directory when
> extracting firmware is small overhead and it will be transparent to the user of
> whatever mechanism the distro uses. The routines needed to calculate the CRC,
> etc. have been copied into fwcutter from ssb_sprom. A version of this code is
> already running.
> 
> (2) Use the steps in http://bcm-v4.sipsolutions.net/802.11/IsSpromAvailable to
> determine if the device has an SPROM. If not, then use the kernel's firmware
> loading mechanism to get the contents of the file prepared in step 1. This file
> has an 8-bit CRC, thus the validity of the file can be tested even though the
> test is not very robust.
> 
> It it reasonable to keep the vendor portion of the MAC and only replace the
> "serial number", or would it be better to randomize all 6 octants?

We know that there exist a risk (arguably low) for two devices in the same LAN to have the same 
random part of the MAC. The risk might be higher if, for any reason, the random number generator 
lack good entropy at the time fwcutter is run.

Also, keeping the same MAC prefix (one from Broadcom) will lead to a risk of having the same MAC for 
a randomly generated device and for a legitimate MAC from Broadcom.

To reduce the risk, we can chose two different ways :

1/ Using a "private" MAC (having bit 0x02 of the lowest byte of the MAC set to 1). This will provide 
a random area of 46 bits, far better than 24 bits if we keep the Broadcom prefix.
2/ Registering a public MAC prefix for this usage.

Anyway, I think we should add a duplicate MAC detection system, that would at least issue a warning 
if the NIC lacking an SPROM (and so knowing the MAC address was randomized) receive a packet with 
its MAC as the source MAC.

Of course, in some network topologies, several NIC may share the same MAC, leading to some of them 
receiving packets with their own MAC as source MAC. A bonding configuration is one such situation. 
For this reason, it might be also desirable to have the ability to report to an upper layer that the 
MAC is possibly "unsafe" (subject to a risk of duplicate MAC) and probably not suitable to become 
the shared MAC for a bonding configuration. That way, at the time such configuration are setup, the 
tool used to setup the configuration would have the ability to report the situation to the user.

Also, in order to keep the same MAC if one run fwcutter again, we should provide fwcutter with an 
option to decide whether we want to keep the previously randomized MAC or whether fwcutter should 
provide a new one (in particular if the current one hit a duplicate MAC).

	Nicolas.

> 
> Is there a better way to load a file into the kernel?
> 
> Thanks,
> 
> Larry
> _______________________________________________
> Bcm43xx-dev mailing list
> Bcm43xx-dev@lists.berlios.de
> https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
> 

Re: RFC: A workaround for BCM43XX devices with no on-board SPROM

[Johannes Berg]

On Thu, 2010-03-18 at 12:46 -0500, Larry Finger wrote:
> Michael,
> 
> I'm switching this discussion from the kernel Bugzilla to the lists.
> 
> As you know, but I'm restating for anyone that has not read our previous
> discussions, the b43 driver needs to be changed to handle some of the newer
> devices do not have an on-board SPROM. It would be trivial to incorporate the
> data except for the need to have a unique, reproducible MAC.

Where does the data usually come from in these devices?

johannes

Re: RFC: A workaround for BCM43XX devices with no on-board SPROM

[Larry Finger]

On 03/18/2010 03:53 PM, Johannes Berg wrote:
> On Thu, 2010-03-18 at 12:46 -0500, Larry Finger wrote:
>> Michael,
>>
>> I'm switching this discussion from the kernel Bugzilla to the lists.
>>
>> As you know, but I'm restating for anyone that has not read our previous
>> discussions, the b43 driver needs to be changed to handle some of the newer
>> devices do not have an on-board SPROM. It would be trivial to incorporate the
>> data except for the need to have a unique, reproducible MAC.
> 
> Where does the data usually come from in these devices?

It comes from the SPROM, which is missing in the devices in question. Broadcrap
wanted to save a few pennies.

Larry

Re: RFC: A workaround for BCM43XX devices with no on-board SPROM

[Johannes Berg]

On Thu, 2010-03-18 at 16:10 -0500, Larry Finger wrote:
> On 03/18/2010 03:53 PM, Johannes Berg wrote:
> > On Thu, 2010-03-18 at 12:46 -0500, Larry Finger wrote:
> >> Michael,
> >>
> >> I'm switching this discussion from the kernel Bugzilla to the lists.
> >>
> >> As you know, but I'm restating for anyone that has not read our previous
> >> discussions, the b43 driver needs to be changed to handle some of the newer
> >> devices do not have an on-board SPROM. It would be trivial to incorporate the
> >> data except for the need to have a unique, reproducible MAC.
> > 
> > Where does the data usually come from in these devices?
> 
> It comes from the SPROM, which is missing in the devices in question. Broadcrap
> wanted to save a few pennies.

Right, but they have to support getting the data somehow on for example
windows even if there's no sprom. Do we know where it comes from then?

johannes

Re: RFC: A workaround for BCM43XX devices with no on-board SPROM

[Larry Finger]

On 03/18/2010 02:31 PM, Michael Buesch wrote:
> On Thursday 18 March 2010 18:46:35 Larry Finger wrote:
>> (1) Modify b43-fwcutter to take data from an existing SPROM,
> 
> Why not extend the ssb-sprom tool? I don't think this has anything to do with
> firmware, except that we (ab)use the firmware loading mechanism of the kernel
> for loading the blob into the kernel.

It has nothing to do with firmware, but the existing fwcutter has all the parts
to generate files in the firmware directory, which is a good place to put this
virtual SPROM.
> 
>> I have chosen to implement this in
>> fwcutter rather than ssb_sprom because the ordinary user will not have access to
>> ssb_sprom;
> 
> Huh? ssb-sprom is GPL software. I have no problem relicensing it under BSD or
> even something more liberal. I don't see a problem for "ordinary users" here.

It has nothing to do with the license. My distro, openSUSE, packages fwcutter
along with a script that uses wget to download the Broadcom drivers and extract
firmware for both b43 and b43legacy. The average user only has to execute that
script. Of course, the package could include both fwcutter and ssb_sprom
programs, but that would make a bigger change to the openSUSE package than just
a patch to fwcutter. I suspect that other distros use similar packages.

> Well, but that version won't do anything on the SPROM, too.

Yes, but if fwcutter were modified, it could write the virtual SPROM file.

Larry

Re: RFC: A workaround for BCM43XX devices with no on-board SPROM

[Larry Finger]

On 03/18/2010 04:20 PM, Johannes Berg wrote:
> On Thu, 2010-03-18 at 16:10 -0500, Larry Finger wrote:
>> On 03/18/2010 03:53 PM, Johannes Berg wrote:
>>> On Thu, 2010-03-18 at 12:46 -0500, Larry Finger wrote:
>>>> Michael,
>>>>
>>>> I'm switching this discussion from the kernel Bugzilla to the lists.
>>>>
>>>> As you know, but I'm restating for anyone that has not read our previous
>>>> discussions, the b43 driver needs to be changed to handle some of the newer
>>>> devices do not have an on-board SPROM. It would be trivial to incorporate the
>>>> data except for the need to have a unique, reproducible MAC.
>>>
>>> Where does the data usually come from in these devices?
>>
>> It comes from the SPROM, which is missing in the devices in question. Broadcrap
>> wanted to save a few pennies.
> 
> Right, but they have to support getting the data somehow on for example
> windows even if there's no sprom. Do we know where it comes from then?

In the Linux driver and likely in the Windows driver, the SPROM data is read
from the SPROM and encoded into a set of tagged strings. After that, the actual
SPROM is ignored. I have not completed the RE on this area, but it looks as if
they have a set of "canned" data that is copied into the area. How they handle a
MAC is not yet understood.

Larry

Re: RFC: A workaround for BCM43XX devices with no on-board SPROM

[Michael Buesch]

On Thursday 18 March 2010 22:38:17 Larry Finger wrote:
> On 03/18/2010 02:31 PM, Michael Buesch wrote:
> > On Thursday 18 March 2010 18:46:35 Larry Finger wrote:
> >> (1) Modify b43-fwcutter to take data from an existing SPROM,
> > 
> > Why not extend the ssb-sprom tool? I don't think this has anything to do with
> > firmware, except that we (ab)use the firmware loading mechanism of the kernel
> > for loading the blob into the kernel.
> 
> It has nothing to do with firmware, but the existing fwcutter has all the parts
> to generate files in the firmware directory,

Everything needed to "generate a file in the firmware directory" are the open()
write() and close() syscalls.

> > 
> >> I have chosen to implement this in
> >> fwcutter rather than ssb_sprom because the ordinary user will not have access to
> >> ssb_sprom;
> > 
> > Huh? ssb-sprom is GPL software. I have no problem relicensing it under BSD or
> > even something more liberal. I don't see a problem for "ordinary users" here.
> 
> It has nothing to do with the license. My distro, openSUSE, packages fwcutter
> along with a script that uses wget to download the Broadcom drivers and extract
> firmware for both b43 and b43legacy. The average user only has to execute that
> script. Of course, the package could include both fwcutter and ssb_sprom
> programs, but that would make a bigger change to the openSUSE package than just
> a patch to fwcutter. I suspect that other distros use similar packages.
> 
> > Well, but that version won't do anything on the SPROM, too.
> 
> Yes, but if fwcutter were modified, it could write the virtual SPROM file.

I think it really is abuse of fwcutter.
What if you don't want any proprietary firmware at all, but still want an SPROM image?
What about distros that do _not_ automatically use fwcutter to put proprietary fw in place
for legal reasons? (Which most likely is the majority of distributions).

Why create yet another dependency on fwcutter. I thought the long term plan was to get rid
of proprietary firmware and fwcutter?

Is it really such a big deal for a distribution to include yet another tiny opensource
package? If that really is a problem for a distribution, they should just completely
stop doing their distro.

-- 
Greetings, Michael.

Re: RFC: A workaround for BCM43XX devices with no on-board SPROM

[Kalle Valo]

Larry Finger <Larry.Finger@lwfinger.net> writes:

> It comes from the SPROM, which is missing in the devices in
> question. Broadcrap wanted to save a few pennies.

It's just only about the cost, it's also about the size. Most probably
in the future there will be even more designs like this. In the
embedded side there has been such devices for few years now.

-- 
Kalle Valo

[PATCH] ssb: do not read SPROM if it does not exist

[John W. Linville]

Attempting to read registers that don't exist on the SSB bus can cause
hangs on some boxes.  At least some b43 devices are 'in the wild' that
don't have SPROMs at all.  When the SSB bus support loads, it attempts
to read these (non-existant) SPROMs and causes hard hangs on the box --
no console output, etc.

This patch adds some intelligence to determine whether or not the SPROM
is present before attempting to read it.  This avoids those hard hangs
on those devices with no SPROM attached to their SSB bus.  The
SSB-attached devices (e.g. b43, et al.) won't work, but at least the box
will survive to test further patches. :-)

Signed-off-by: John W. Linville <linville@tuxdriver.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Michael Buesch <mb@bu3sch.de>
Cc: stable@kernel.org
---
 drivers/ssb/pci.c                         |    3 +++
 drivers/ssb/scan.c                        |    4 ++++
 drivers/ssb/sprom.c                       |   22 ++++++++++++++++++++++
 include/linux/ssb/ssb.h                   |    3 +++
 include/linux/ssb/ssb_driver_chipcommon.h |   15 +++++++++++++++
 5 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
index 9e50896..2f7b16d 100644
--- a/drivers/ssb/pci.c
+++ b/drivers/ssb/pci.c
@@ -620,6 +620,9 @@ static int ssb_pci_sprom_get(struct ssb_bus *bus,
 	int err = -ENOMEM;
 	u16 *buf;
 
+	if (!ssb_is_sprom_available(bus))
+		return -ENODEV;
+
 	buf = kcalloc(SSB_SPROMSIZE_WORDS_R123, sizeof(u16), GFP_KERNEL);
 	if (!buf)
 		goto out;
diff --git a/drivers/ssb/scan.c b/drivers/ssb/scan.c
index 0d6c028..3a8d0b9 100644
--- a/drivers/ssb/scan.c
+++ b/drivers/ssb/scan.c
@@ -306,6 +306,10 @@ int ssb_bus_scan(struct ssb_bus *bus,
 		}
 		tmp = scan_read32(bus, 0, SSB_CHIPCO_CAP);
 		bus->chipco.capabilities = tmp;
+		if (bus->chip_rev >= 11) {
+			tmp = scan_read32(bus, 0, SSB_CHIPCO_CHIPSTAT);
+			bus->chipco.status = tmp;
+		}
 	} else {
 		if (bus->bustype == SSB_BUSTYPE_PCI) {
 			bus->chip_id = pcidev_to_chipid(bus->host_pci);
diff --git a/drivers/ssb/sprom.c b/drivers/ssb/sprom.c
index d0e6762..4d44de4 100644
--- a/drivers/ssb/sprom.c
+++ b/drivers/ssb/sprom.c
@@ -175,3 +175,25 @@ const struct ssb_sprom *ssb_get_fallback_sprom(void)
 {
 	return fallback_sprom;
 }
+
+bool ssb_is_sprom_available(struct ssb_bus *bus)
+{
+	/* status register only exists on chip_rev >= 11 */
+	if (bus->chip_rev < 11)
+		return true;
+
+	switch (bus->chip_id) {
+	case 0x4312:
+		return SSB_CHIPCO_CHST_4312_SPROM_PRESENT(bus->chipco.status);
+	case 0x4322:
+		return SSB_CHIPCO_CHST_4322_SPROM_PRESENT(bus->chipco.status);
+	case 0x4325:
+		return SSB_CHIPCO_CHST_4325_SPROM_PRESENT(bus->chipco.status);
+	default:
+		break;
+	}
+	if (bus->chip_rev >= 31)
+		return bus->chipco.capabilities & SSB_CHIPCO_CAP_SPROM;
+
+	return true;
+}
diff --git a/include/linux/ssb/ssb.h b/include/linux/ssb/ssb.h
index 24f9885..3b4da23 100644
--- a/include/linux/ssb/ssb.h
+++ b/include/linux/ssb/ssb.h
@@ -394,6 +394,9 @@ extern int ssb_bus_sdiobus_register(struct ssb_bus *bus,
 
 extern void ssb_bus_unregister(struct ssb_bus *bus);
 
+/* Does the device have an SPROM? */
+extern bool ssb_is_sprom_available(struct ssb_bus *bus);
+
 /* Set a fallback SPROM.
  * See kdoc at the function definition for complete documentation. */
 extern int ssb_arch_set_fallback_sprom(const struct ssb_sprom *sprom);
diff --git a/include/linux/ssb/ssb_driver_chipcommon.h b/include/linux/ssb/ssb_driver_chipcommon.h
index 4e27acf..4e5726d 100644
--- a/include/linux/ssb/ssb_driver_chipcommon.h
+++ b/include/linux/ssb/ssb_driver_chipcommon.h
@@ -30,6 +30,7 @@
 #define   SSB_CHIPCO_CAP_UARTCLK_INT	0x00000008	/* UARTs are driven by internal divided clock */
 #define  SSB_CHIPCO_CAP_UARTGPIO	0x00000020	/* UARTs on GPIO 15-12 */
 #define  SSB_CHIPCO_CAP_EXTBUS		0x000000C0	/* External buses present */
+#define  SSB_CHIPCO_CAP_SPROM		0x40000000	/* SPROM present */
 #define  SSB_CHIPCO_CAP_FLASHT		0x00000700	/* Flash Type */
 #define   SSB_CHIPCO_FLASHT_NONE	0x00000000	/* No flash */
 #define   SSB_CHIPCO_FLASHT_STSER	0x00000100	/* ST serial flash */
@@ -385,6 +386,7 @@
 
 
 /** Chip specific Chip-Status register contents. */
+#define SSB_CHIPCO_CHST_4322_SPROM_EXISTS	0x00000040 /* SPROM present */
 #define SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL	0x00000003
 #define SSB_CHIPCO_CHST_4325_DEFCIS_SEL		0 /* OTP is powered up, use def. CIS, no SPROM */
 #define SSB_CHIPCO_CHST_4325_SPROM_SEL		1 /* OTP is powered up, SPROM is present */
@@ -398,6 +400,18 @@
 #define SSB_CHIPCO_CHST_4325_RCAL_VALUE_SHIFT	4
 #define SSB_CHIPCO_CHST_4325_PMUTOP_2B 		0x00000200 /* 1 for 2b, 0 for to 2a */
 
+/** Macros to determine SPROM presence based on Chip-Status register. */
+#define SSB_CHIPCO_CHST_4312_SPROM_PRESENT(status) \
+	((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_OTP_SEL)
+#define SSB_CHIPCO_CHST_4322_SPROM_PRESENT(status) \
+	(status & SSB_CHIPCO_CHST_4322_SPROM_EXISTS)
+#define SSB_CHIPCO_CHST_4325_SPROM_PRESENT(status) \
+	(((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_DEFCIS_SEL) && \
+	 ((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_OTP_SEL))
+
 
 
 /** Clockcontrol masks and values **/
@@ -564,6 +578,7 @@ struct ssb_chipcommon_pmu {
 struct ssb_chipcommon {
 	struct ssb_device *dev;
 	u32 capabilities;
+	u32 status;
 	/* Fast Powerup Delay constant */
 	u16 fast_pwrup_delay;
 	struct ssb_chipcommon_pmu pmu;
-- 
1.6.2.5

Re: [PATCH] ssb: do not read SPROM if it does not exist

[Michael Buesch]

On Friday 19 March 2010 20:08:07 John W. Linville wrote:
> +	switch (bus->chip_id) {
> +	case 0x4312:
> +		return SSB_CHIPCO_CHST_4312_SPROM_PRESENT(bus->chipco.status);

Not sure why we want to hide the logic in defines. But I don't care much, either.

> +	case 0x4322:
> +		return SSB_CHIPCO_CHST_4322_SPROM_PRESENT(bus->chipco.status);
> +	case 0x4325:
> +		return SSB_CHIPCO_CHST_4325_SPROM_PRESENT(bus->chipco.status);
> +	default:
> +		break;
> +	}
> +	if (bus->chip_rev >= 31)

This check is wrong.
You need to check the chipcommon core revision. Not the chip revision.

> +		return bus->chipco.capabilities & SSB_CHIPCO_CAP_SPROM;
> +
> +	return true;
> +}
> diff --git a/include/linux/ssb/ssb.h b/include/linux/ssb/ssb.h
> index 24f9885..3b4da23 100644
> --- a/include/linux/ssb/ssb.h
> +++ b/include/linux/ssb/ssb.h
> @@ -394,6 +394,9 @@ extern int ssb_bus_sdiobus_register(struct ssb_bus *bus,
>  
>  extern void ssb_bus_unregister(struct ssb_bus *bus);
>  
> +/* Does the device have an SPROM? */
> +extern bool ssb_is_sprom_available(struct ssb_bus *bus);
> +
>  /* Set a fallback SPROM.
>   * See kdoc at the function definition for complete documentation. */
>  extern int ssb_arch_set_fallback_sprom(const struct ssb_sprom *sprom);
> diff --git a/include/linux/ssb/ssb_driver_chipcommon.h b/include/linux/ssb/ssb_driver_chipcommon.h
> index 4e27acf..4e5726d 100644
> --- a/include/linux/ssb/ssb_driver_chipcommon.h
> +++ b/include/linux/ssb/ssb_driver_chipcommon.h
> @@ -30,6 +30,7 @@
>  #define   SSB_CHIPCO_CAP_UARTCLK_INT	0x00000008	/* UARTs are driven by internal divided clock */
>  #define  SSB_CHIPCO_CAP_UARTGPIO	0x00000020	/* UARTs on GPIO 15-12 */
>  #define  SSB_CHIPCO_CAP_EXTBUS		0x000000C0	/* External buses present */
> +#define  SSB_CHIPCO_CAP_SPROM		0x40000000	/* SPROM present */
>  #define  SSB_CHIPCO_CAP_FLASHT		0x00000700	/* Flash Type */

Probably keep ordering of capabilities correct.

>  #define   SSB_CHIPCO_FLASHT_NONE	0x00000000	/* No flash */
>  #define   SSB_CHIPCO_FLASHT_STSER	0x00000100	/* ST serial flash */
> @@ -385,6 +386,7 @@

-- 
Greetings, Michael.

Re: [PATCH] ssb: do not read SPROM if it does not exist

[Michael Buesch]

On Friday 19 March 2010 20:41:47 Michael Buesch wrote:
> > +	if (bus->chip_rev >= 31)
> 
> This check is wrong.
> You need to check the chipcommon core revision. Not the chip revision.

Actually. All three checks in the patch are wrong. Not just in this place.

-- 
Greetings, Michael.

Re: [PATCH] ssb: do not read SPROM if it does not exist

[John W. Linville]

On Fri, Mar 19, 2010 at 08:41:47PM +0100, Michael Buesch wrote:
> On Friday 19 March 2010 20:08:07 John W. Linville wrote:
> > +	switch (bus->chip_id) {
> > +	case 0x4312:
> > +		return SSB_CHIPCO_CHST_4312_SPROM_PRESENT(bus->chipco.status);
> 
> Not sure why we want to hide the logic in defines. But I don't care much, either.

To me it just seems clearer than a bunch of long and ugly bit
operations that differ in the details but are logically accomplishing
the same thing.
 
> > +	case 0x4322:
> > +		return SSB_CHIPCO_CHST_4322_SPROM_PRESENT(bus->chipco.status);
> > +	case 0x4325:
> > +		return SSB_CHIPCO_CHST_4325_SPROM_PRESENT(bus->chipco.status);
> > +	default:
> > +		break;
> > +	}
> > +	if (bus->chip_rev >= 31)
> 
> This check is wrong.
> You need to check the chipcommon core revision. Not the chip revision.

I'm sorry, I had trouble figuring-out what you meant (since chip_rev
comes from a chipcommon register).  I think you mean this:

-       if (bus->chip_rev >= 31)
+       if (bus->chipco.dev->id.revision >= 31)

Is that right?

John

> > +		return bus->chipco.capabilities & SSB_CHIPCO_CAP_SPROM;
> > +
> > +	return true;
> > +}
> > diff --git a/include/linux/ssb/ssb.h b/include/linux/ssb/ssb.h
> > index 24f9885..3b4da23 100644
> > --- a/include/linux/ssb/ssb.h
> > +++ b/include/linux/ssb/ssb.h
> > @@ -394,6 +394,9 @@ extern int ssb_bus_sdiobus_register(struct ssb_bus *bus,
> >  
> >  extern void ssb_bus_unregister(struct ssb_bus *bus);
> >  
> > +/* Does the device have an SPROM? */
> > +extern bool ssb_is_sprom_available(struct ssb_bus *bus);
> > +
> >  /* Set a fallback SPROM.
> >   * See kdoc at the function definition for complete documentation. */
> >  extern int ssb_arch_set_fallback_sprom(const struct ssb_sprom *sprom);
> > diff --git a/include/linux/ssb/ssb_driver_chipcommon.h b/include/linux/ssb/ssb_driver_chipcommon.h
> > index 4e27acf..4e5726d 100644
> > --- a/include/linux/ssb/ssb_driver_chipcommon.h
> > +++ b/include/linux/ssb/ssb_driver_chipcommon.h
> > @@ -30,6 +30,7 @@
> >  #define   SSB_CHIPCO_CAP_UARTCLK_INT	0x00000008	/* UARTs are driven by internal divided clock */
> >  #define  SSB_CHIPCO_CAP_UARTGPIO	0x00000020	/* UARTs on GPIO 15-12 */
> >  #define  SSB_CHIPCO_CAP_EXTBUS		0x000000C0	/* External buses present */
> > +#define  SSB_CHIPCO_CAP_SPROM		0x40000000	/* SPROM present */
> >  #define  SSB_CHIPCO_CAP_FLASHT		0x00000700	/* Flash Type */
> 
> Probably keep ordering of capabilities correct.

Oops, sorry...

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

Re: [PATCH] ssb: do not read SPROM if it does not exist

[Michael Buesch]

On Friday 19 March 2010 21:21:21 John W. Linville wrote:
> -       if (bus->chip_rev >= 31)
> +       if (bus->chipco.dev->id.revision >= 31)
> 
> Is that right?

Yeah, that's OK. Same goes for the other revision tests.

-- 
Greetings, Michael.

Re: [PATCH] ssb: do not read SPROM if it does not exist

[John W. Linville]

On Fri, Mar 19, 2010 at 09:30:50PM +0100, Michael Buesch wrote:
> On Friday 19 March 2010 21:21:21 John W. Linville wrote:
> > -       if (bus->chip_rev >= 31)
> > +       if (bus->chipco.dev->id.revision >= 31)
> > 
> > Is that right?
> 
> Yeah, that's OK. Same goes for the other revision tests.

Cool, thanks!

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

[PATCH v2] ssb: do not read SPROM if it does not exist

[John W. Linville]

Attempting to read registers that don't exist on the SSB bus can cause
hangs on some boxes.  At least some b43 devices are 'in the wild' that
don't have SPROMs at all.  When the SSB bus support loads, it attempts
to read these (non-existant) SPROMs and causes hard hangs on the box --
no console output, etc.

This patch adds some intelligence to determine whether or not the SPROM
is present before attempting to read it.  This avoids those hard hangs
on those devices with no SPROM attached to their SSB bus.  The
SSB-attached devices (e.g. b43, et al.) won't work, but at least the box
will survive to test further patches. :-)

Signed-off-by: John W. Linville <linville@tuxdriver.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Michael Buesch <mb@bu3sch.de>
Cc: stable@kernel.org
---
Version 2, check the correct place for ChipCommon core revision... :-)

 drivers/ssb/pci.c                         |    3 +++
 drivers/ssb/scan.c                        |    4 ++++
 drivers/ssb/sprom.c                       |   22 ++++++++++++++++++++++
 include/linux/ssb/ssb.h                   |    3 +++
 include/linux/ssb/ssb_driver_chipcommon.h |   15 +++++++++++++++
 5 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
index 9e50896..2f7b16d 100644
--- a/drivers/ssb/pci.c
+++ b/drivers/ssb/pci.c
@@ -620,6 +620,9 @@ static int ssb_pci_sprom_get(struct ssb_bus *bus,
 	int err = -ENOMEM;
 	u16 *buf;
 
+	if (!ssb_is_sprom_available(bus))
+		return -ENODEV;
+
 	buf = kcalloc(SSB_SPROMSIZE_WORDS_R123, sizeof(u16), GFP_KERNEL);
 	if (!buf)
 		goto out;
diff --git a/drivers/ssb/scan.c b/drivers/ssb/scan.c
index 0d6c028..c4944b9 100644
--- a/drivers/ssb/scan.c
+++ b/drivers/ssb/scan.c
@@ -306,6 +306,10 @@ int ssb_bus_scan(struct ssb_bus *bus,
 		}
 		tmp = scan_read32(bus, 0, SSB_CHIPCO_CAP);
 		bus->chipco.capabilities = tmp;
+		if (bus->chipco.dev->id.revision >= 11)
+			tmp = scan_read32(bus, 0, SSB_CHIPCO_CHIPSTAT);
+			bus->chipco.status = tmp;
+		}
 	} else {
 		if (bus->bustype == SSB_BUSTYPE_PCI) {
 			bus->chip_id = pcidev_to_chipid(bus->host_pci);
diff --git a/drivers/ssb/sprom.c b/drivers/ssb/sprom.c
index d0e6762..55eb9b0 100644
--- a/drivers/ssb/sprom.c
+++ b/drivers/ssb/sprom.c
@@ -175,3 +175,25 @@ const struct ssb_sprom *ssb_get_fallback_sprom(void)
 {
 	return fallback_sprom;
 }
+
+bool ssb_is_sprom_available(struct ssb_bus *bus)
+{
+	/* status register only exists on chipcomon rev >= 11 */
+	if (bus->chipco.dev->id.revision < 11)
+		return true;
+
+	switch (bus->chip_id) {
+	case 0x4312:
+		return SSB_CHIPCO_CHST_4312_SPROM_PRESENT(bus->chipco.status);
+	case 0x4322:
+		return SSB_CHIPCO_CHST_4322_SPROM_PRESENT(bus->chipco.status);
+	case 0x4325:
+		return SSB_CHIPCO_CHST_4325_SPROM_PRESENT(bus->chipco.status);
+	default:
+		break;
+	}
+	if (bus->chipco.dev->id.revision >= 31)
+		return bus->chipco.capabilities & SSB_CHIPCO_CAP_SPROM;
+
+	return true;
+}
diff --git a/include/linux/ssb/ssb.h b/include/linux/ssb/ssb.h
index 24f9885..3b4da23 100644
--- a/include/linux/ssb/ssb.h
+++ b/include/linux/ssb/ssb.h
@@ -394,6 +394,9 @@ extern int ssb_bus_sdiobus_register(struct ssb_bus *bus,
 
 extern void ssb_bus_unregister(struct ssb_bus *bus);
 
+/* Does the device have an SPROM? */
+extern bool ssb_is_sprom_available(struct ssb_bus *bus);
+
 /* Set a fallback SPROM.
  * See kdoc at the function definition for complete documentation. */
 extern int ssb_arch_set_fallback_sprom(const struct ssb_sprom *sprom);
diff --git a/include/linux/ssb/ssb_driver_chipcommon.h b/include/linux/ssb/ssb_driver_chipcommon.h
index 4e27acf..2cdf249 100644
--- a/include/linux/ssb/ssb_driver_chipcommon.h
+++ b/include/linux/ssb/ssb_driver_chipcommon.h
@@ -53,6 +53,7 @@
 #define  SSB_CHIPCO_CAP_64BIT		0x08000000	/* 64-bit Backplane */
 #define  SSB_CHIPCO_CAP_PMU		0x10000000	/* PMU available (rev >= 20) */
 #define  SSB_CHIPCO_CAP_ECI		0x20000000	/* ECI available (rev >= 20) */
+#define  SSB_CHIPCO_CAP_SPROM		0x40000000	/* SPROM present */
 #define SSB_CHIPCO_CORECTL		0x0008
 #define  SSB_CHIPCO_CORECTL_UARTCLK0	0x00000001	/* Drive UART with internal clock */
 #define	 SSB_CHIPCO_CORECTL_SE		0x00000002	/* sync clk out enable (corerev >= 3) */
@@ -385,6 +386,7 @@
 
 
 /** Chip specific Chip-Status register contents. */
+#define SSB_CHIPCO_CHST_4322_SPROM_EXISTS	0x00000040 /* SPROM present */
 #define SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL	0x00000003
 #define SSB_CHIPCO_CHST_4325_DEFCIS_SEL		0 /* OTP is powered up, use def. CIS, no SPROM */
 #define SSB_CHIPCO_CHST_4325_SPROM_SEL		1 /* OTP is powered up, SPROM is present */
@@ -398,6 +400,18 @@
 #define SSB_CHIPCO_CHST_4325_RCAL_VALUE_SHIFT	4
 #define SSB_CHIPCO_CHST_4325_PMUTOP_2B 		0x00000200 /* 1 for 2b, 0 for to 2a */
 
+/** Macros to determine SPROM presence based on Chip-Status register. */
+#define SSB_CHIPCO_CHST_4312_SPROM_PRESENT(status) \
+	((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_OTP_SEL)
+#define SSB_CHIPCO_CHST_4322_SPROM_PRESENT(status) \
+	(status & SSB_CHIPCO_CHST_4322_SPROM_EXISTS)
+#define SSB_CHIPCO_CHST_4325_SPROM_PRESENT(status) \
+	(((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_DEFCIS_SEL) && \
+	 ((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_OTP_SEL))
+
 
 
 /** Clockcontrol masks and values **/
@@ -564,6 +578,7 @@ struct ssb_chipcommon_pmu {
 struct ssb_chipcommon {
 	struct ssb_device *dev;
 	u32 capabilities;
+	u32 status;
 	/* Fast Powerup Delay constant */
 	u16 fast_pwrup_delay;
 	struct ssb_chipcommon_pmu pmu;
-- 
1.6.2.5

[PATCH v3] ssb: do not read SPROM if it does not exist

[John W. Linville]

Attempting to read registers that don't exist on the SSB bus can cause
hangs on some boxes.  At least some b43 devices are 'in the wild' that
don't have SPROMs at all.  When the SSB bus support loads, it attempts
to read these (non-existant) SPROMs and causes hard hangs on the box --
no console output, etc.

This patch adds some intelligence to determine whether or not the SPROM
is present before attempting to read it.  This avoids those hard hangs
on those devices with no SPROM attached to their SSB bus.  The
SSB-attached devices (e.g. b43, et al.) won't work, but at least the box
will survive to test further patches. :-)

Signed-off-by: John W. Linville <linville@tuxdriver.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Michael Buesch <mb@bu3sch.de>
Cc: stable@kernel.org
---
Version 3, add missing semi-colon... :-(
Version 2, check the correct place for ChipCommon core revision... :-)

 drivers/ssb/pci.c                         |    3 +++
 drivers/ssb/scan.c                        |    4 ++++
 drivers/ssb/sprom.c                       |   22 ++++++++++++++++++++++
 include/linux/ssb/ssb.h                   |    3 +++
 include/linux/ssb/ssb_driver_chipcommon.h |   15 +++++++++++++++
 5 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
index 9e50896..2f7b16d 100644
--- a/drivers/ssb/pci.c
+++ b/drivers/ssb/pci.c
@@ -620,6 +620,9 @@ static int ssb_pci_sprom_get(struct ssb_bus *bus,
 	int err = -ENOMEM;
 	u16 *buf;
 
+	if (!ssb_is_sprom_available(bus))
+		return -ENODEV;
+
 	buf = kcalloc(SSB_SPROMSIZE_WORDS_R123, sizeof(u16), GFP_KERNEL);
 	if (!buf)
 		goto out;
diff --git a/drivers/ssb/scan.c b/drivers/ssb/scan.c
index 0d6c028..6d51895 100644
--- a/drivers/ssb/scan.c
+++ b/drivers/ssb/scan.c
@@ -306,6 +306,10 @@ int ssb_bus_scan(struct ssb_bus *bus,
 		}
 		tmp = scan_read32(bus, 0, SSB_CHIPCO_CAP);
 		bus->chipco.capabilities = tmp;
+		if (bus->chipco.dev->id.revision >= 11) {
+			tmp = scan_read32(bus, 0, SSB_CHIPCO_CHIPSTAT);
+			bus->chipco.status = tmp;
+		}
 	} else {
 		if (bus->bustype == SSB_BUSTYPE_PCI) {
 			bus->chip_id = pcidev_to_chipid(bus->host_pci);
diff --git a/drivers/ssb/sprom.c b/drivers/ssb/sprom.c
index d0e6762..55eb9b0 100644
--- a/drivers/ssb/sprom.c
+++ b/drivers/ssb/sprom.c
@@ -175,3 +175,25 @@ const struct ssb_sprom *ssb_get_fallback_sprom(void)
 {
 	return fallback_sprom;
 }
+
+bool ssb_is_sprom_available(struct ssb_bus *bus)
+{
+	/* status register only exists on chipcomon rev >= 11 */
+	if (bus->chipco.dev->id.revision < 11)
+		return true;
+
+	switch (bus->chip_id) {
+	case 0x4312:
+		return SSB_CHIPCO_CHST_4312_SPROM_PRESENT(bus->chipco.status);
+	case 0x4322:
+		return SSB_CHIPCO_CHST_4322_SPROM_PRESENT(bus->chipco.status);
+	case 0x4325:
+		return SSB_CHIPCO_CHST_4325_SPROM_PRESENT(bus->chipco.status);
+	default:
+		break;
+	}
+	if (bus->chipco.dev->id.revision >= 31)
+		return bus->chipco.capabilities & SSB_CHIPCO_CAP_SPROM;
+
+	return true;
+}
diff --git a/include/linux/ssb/ssb.h b/include/linux/ssb/ssb.h
index 24f9885..3b4da23 100644
--- a/include/linux/ssb/ssb.h
+++ b/include/linux/ssb/ssb.h
@@ -394,6 +394,9 @@ extern int ssb_bus_sdiobus_register(struct ssb_bus *bus,
 
 extern void ssb_bus_unregister(struct ssb_bus *bus);
 
+/* Does the device have an SPROM? */
+extern bool ssb_is_sprom_available(struct ssb_bus *bus);
+
 /* Set a fallback SPROM.
  * See kdoc at the function definition for complete documentation. */
 extern int ssb_arch_set_fallback_sprom(const struct ssb_sprom *sprom);
diff --git a/include/linux/ssb/ssb_driver_chipcommon.h b/include/linux/ssb/ssb_driver_chipcommon.h
index 4e27acf..2cdf249 100644
--- a/include/linux/ssb/ssb_driver_chipcommon.h
+++ b/include/linux/ssb/ssb_driver_chipcommon.h
@@ -53,6 +53,7 @@
 #define  SSB_CHIPCO_CAP_64BIT		0x08000000	/* 64-bit Backplane */
 #define  SSB_CHIPCO_CAP_PMU		0x10000000	/* PMU available (rev >= 20) */
 #define  SSB_CHIPCO_CAP_ECI		0x20000000	/* ECI available (rev >= 20) */
+#define  SSB_CHIPCO_CAP_SPROM		0x40000000	/* SPROM present */
 #define SSB_CHIPCO_CORECTL		0x0008
 #define  SSB_CHIPCO_CORECTL_UARTCLK0	0x00000001	/* Drive UART with internal clock */
 #define	 SSB_CHIPCO_CORECTL_SE		0x00000002	/* sync clk out enable (corerev >= 3) */
@@ -385,6 +386,7 @@
 
 
 /** Chip specific Chip-Status register contents. */
+#define SSB_CHIPCO_CHST_4322_SPROM_EXISTS	0x00000040 /* SPROM present */
 #define SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL	0x00000003
 #define SSB_CHIPCO_CHST_4325_DEFCIS_SEL		0 /* OTP is powered up, use def. CIS, no SPROM */
 #define SSB_CHIPCO_CHST_4325_SPROM_SEL		1 /* OTP is powered up, SPROM is present */
@@ -398,6 +400,18 @@
 #define SSB_CHIPCO_CHST_4325_RCAL_VALUE_SHIFT	4
 #define SSB_CHIPCO_CHST_4325_PMUTOP_2B 		0x00000200 /* 1 for 2b, 0 for to 2a */
 
+/** Macros to determine SPROM presence based on Chip-Status register. */
+#define SSB_CHIPCO_CHST_4312_SPROM_PRESENT(status) \
+	((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_OTP_SEL)
+#define SSB_CHIPCO_CHST_4322_SPROM_PRESENT(status) \
+	(status & SSB_CHIPCO_CHST_4322_SPROM_EXISTS)
+#define SSB_CHIPCO_CHST_4325_SPROM_PRESENT(status) \
+	(((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_DEFCIS_SEL) && \
+	 ((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_OTP_SEL))
+
 
 
 /** Clockcontrol masks and values **/
@@ -564,6 +578,7 @@ struct ssb_chipcommon_pmu {
 struct ssb_chipcommon {
 	struct ssb_device *dev;
 	u32 capabilities;
+	u32 status;
 	/* Fast Powerup Delay constant */
 	u16 fast_pwrup_delay;
 	struct ssb_chipcommon_pmu pmu;
-- 
1.6.2.5

Re: [PATCH v3] ssb: do not read SPROM if it does not exist

[Michael Buesch]

On Friday 19 March 2010 21:41:49 John W. Linville wrote:
> Attempting to read registers that don't exist on the SSB bus can cause
> hangs on some boxes.  At least some b43 devices are 'in the wild' that
> don't have SPROMs at all.  When the SSB bus support loads, it attempts
> to read these (non-existant) SPROMs and causes hard hangs on the box --
> no console output, etc.
> 
> This patch adds some intelligence to determine whether or not the SPROM
> is present before attempting to read it.  This avoids those hard hangs
> on those devices with no SPROM attached to their SSB bus.  The
> SSB-attached devices (e.g. b43, et al.) won't work, but at least the box
> will survive to test further patches. :-)
> 
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> Cc: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Michael Buesch <mb@bu3sch.de>
> Cc: stable@kernel.org
> ---
> Version 3, add missing semi-colon... :-(
> Version 2, check the correct place for ChipCommon core revision... :-)
> 
>  drivers/ssb/pci.c                         |    3 +++
>  drivers/ssb/scan.c                        |    4 ++++
>  drivers/ssb/sprom.c                       |   22 ++++++++++++++++++++++
>  include/linux/ssb/ssb.h                   |    3 +++
>  include/linux/ssb/ssb_driver_chipcommon.h |   15 +++++++++++++++
>  5 files changed, 47 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
> index 9e50896..2f7b16d 100644
> --- a/drivers/ssb/pci.c
> +++ b/drivers/ssb/pci.c
> @@ -620,6 +620,9 @@ static int ssb_pci_sprom_get(struct ssb_bus *bus,
>  	int err = -ENOMEM;
>  	u16 *buf;
>  
> +	if (!ssb_is_sprom_available(bus))
> +		return -ENODEV;
> +
>  	buf = kcalloc(SSB_SPROMSIZE_WORDS_R123, sizeof(u16), GFP_KERNEL);
>  	if (!buf)
>  		goto out;
> diff --git a/drivers/ssb/scan.c b/drivers/ssb/scan.c
> index 0d6c028..6d51895 100644
> --- a/drivers/ssb/scan.c
> +++ b/drivers/ssb/scan.c
> @@ -306,6 +306,10 @@ int ssb_bus_scan(struct ssb_bus *bus,
>  		}
>  		tmp = scan_read32(bus, 0, SSB_CHIPCO_CAP);
>  		bus->chipco.capabilities = tmp;
> +		if (bus->chipco.dev->id.revision >= 11) {
> +			tmp = scan_read32(bus, 0, SSB_CHIPCO_CHIPSTAT);
> +			bus->chipco.status = tmp;
> +		}

Hm, Ok. There's another issue here. We're that early in the scan that
id.xxxx is not assigned, yet. I think chipco.dev might even be NULL here, so
it'd crash.
This gets a little bit ugly. The revisions are read later in the scan function.
And as you can see there the actual read is pretty ugly, too.

What if we do not read the status _that_ early? We're really very very
early here. If you move the read into the chipcommon driver init, it will be much
easier.

-- 
Greetings, Michael.

Re: [PATCH v3] ssb: do not read SPROM if it does not exist

[John W. Linville]

On Fri, Mar 19, 2010 at 10:12:55PM +0100, Michael Buesch wrote:
> On Friday 19 March 2010 21:41:49 John W. Linville wrote:

> > diff --git a/drivers/ssb/scan.c b/drivers/ssb/scan.c
> > index 0d6c028..6d51895 100644
> > --- a/drivers/ssb/scan.c
> > +++ b/drivers/ssb/scan.c
> > @@ -306,6 +306,10 @@ int ssb_bus_scan(struct ssb_bus *bus,
> >  		}
> >  		tmp = scan_read32(bus, 0, SSB_CHIPCO_CAP);
> >  		bus->chipco.capabilities = tmp;
> > +		if (bus->chipco.dev->id.revision >= 11) {
> > +			tmp = scan_read32(bus, 0, SSB_CHIPCO_CHIPSTAT);
> > +			bus->chipco.status = tmp;
> > +		}
> 
> Hm, Ok. There's another issue here. We're that early in the scan that
> id.xxxx is not assigned, yet. I think chipco.dev might even be NULL here, so
> it'd crash.
> This gets a little bit ugly. The revisions are read later in the scan function.
> And as you can see there the actual read is pretty ugly, too.
> 
> What if we do not read the status _that_ early? We're really very very
> early here. If you move the read into the chipcommon driver init, it will be much
> easier.

Yeah, that makes sense.  Patch to follow...

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

[PATCH v4] ssb: do not read SPROM if it does not exist

[John W. Linville]

Attempting to read registers that don't exist on the SSB bus can cause
hangs on some boxes.  At least some b43 devices are 'in the wild' that
don't have SPROMs at all.  When the SSB bus support loads, it attempts
to read these (non-existant) SPROMs and causes hard hangs on the box --
no console output, etc.

This patch adds some intelligence to determine whether or not the SPROM
is present before attempting to read it.  This avoids those hard hangs
on those devices with no SPROM attached to their SSB bus.  The
SSB-attached devices (e.g. b43, et al.) won't work, but at least the box
will survive to test further patches. :-)

Signed-off-by: John W. Linville <linville@tuxdriver.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Michael Buesch <mb@bu3sch.de>
Cc: stable@kernel.org
---
Version 4, move read of ChipCommon status register to ssb_chipcommon_init
Version 3, add missing semi-colon... :-(
Version 2, check the correct place for ChipCommon core revision... :-)

 drivers/ssb/driver_chipcommon.c           |    3 +++
 drivers/ssb/pci.c                         |    3 +++
 drivers/ssb/sprom.c                       |   22 ++++++++++++++++++++++
 include/linux/ssb/ssb.h                   |    3 +++
 include/linux/ssb/ssb_driver_chipcommon.h |   15 +++++++++++++++
 5 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/drivers/ssb/driver_chipcommon.c b/drivers/ssb/driver_chipcommon.c
index 9681536..6cf288d 100644
--- a/drivers/ssb/driver_chipcommon.c
+++ b/drivers/ssb/driver_chipcommon.c
@@ -233,6 +233,9 @@ void ssb_chipcommon_init(struct ssb_chipcommon *cc)
 {
 	if (!cc->dev)
 		return; /* We don't have a ChipCommon */
+	if (cc->dev->id.revision >= 11) {
+		cc->status = chipco_read32(cc, SSB_CHIPCO_CHIPSTAT);
+	}
 	ssb_pmu_init(cc);
 	chipco_powercontrol_init(cc);
 	ssb_chipco_set_clockmode(cc, SSB_CLKMODE_FAST);
diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
index 9e50896..2f7b16d 100644
--- a/drivers/ssb/pci.c
+++ b/drivers/ssb/pci.c
@@ -620,6 +620,9 @@ static int ssb_pci_sprom_get(struct ssb_bus *bus,
 	int err = -ENOMEM;
 	u16 *buf;
 
+	if (!ssb_is_sprom_available(bus))
+		return -ENODEV;
+
 	buf = kcalloc(SSB_SPROMSIZE_WORDS_R123, sizeof(u16), GFP_KERNEL);
 	if (!buf)
 		goto out;
diff --git a/drivers/ssb/sprom.c b/drivers/ssb/sprom.c
index d0e6762..55eb9b0 100644
--- a/drivers/ssb/sprom.c
+++ b/drivers/ssb/sprom.c
@@ -175,3 +175,25 @@ const struct ssb_sprom *ssb_get_fallback_sprom(void)
 {
 	return fallback_sprom;
 }
+
+bool ssb_is_sprom_available(struct ssb_bus *bus)
+{
+	/* status register only exists on chipcomon rev >= 11 */
+	if (bus->chipco.dev->id.revision < 11)
+		return true;
+
+	switch (bus->chip_id) {
+	case 0x4312:
+		return SSB_CHIPCO_CHST_4312_SPROM_PRESENT(bus->chipco.status);
+	case 0x4322:
+		return SSB_CHIPCO_CHST_4322_SPROM_PRESENT(bus->chipco.status);
+	case 0x4325:
+		return SSB_CHIPCO_CHST_4325_SPROM_PRESENT(bus->chipco.status);
+	default:
+		break;
+	}
+	if (bus->chipco.dev->id.revision >= 31)
+		return bus->chipco.capabilities & SSB_CHIPCO_CAP_SPROM;
+
+	return true;
+}
diff --git a/include/linux/ssb/ssb.h b/include/linux/ssb/ssb.h
index 24f9885..3b4da23 100644
--- a/include/linux/ssb/ssb.h
+++ b/include/linux/ssb/ssb.h
@@ -394,6 +394,9 @@ extern int ssb_bus_sdiobus_register(struct ssb_bus *bus,
 
 extern void ssb_bus_unregister(struct ssb_bus *bus);
 
+/* Does the device have an SPROM? */
+extern bool ssb_is_sprom_available(struct ssb_bus *bus);
+
 /* Set a fallback SPROM.
  * See kdoc at the function definition for complete documentation. */
 extern int ssb_arch_set_fallback_sprom(const struct ssb_sprom *sprom);
diff --git a/include/linux/ssb/ssb_driver_chipcommon.h b/include/linux/ssb/ssb_driver_chipcommon.h
index 4e27acf..2cdf249 100644
--- a/include/linux/ssb/ssb_driver_chipcommon.h
+++ b/include/linux/ssb/ssb_driver_chipcommon.h
@@ -53,6 +53,7 @@
 #define  SSB_CHIPCO_CAP_64BIT		0x08000000	/* 64-bit Backplane */
 #define  SSB_CHIPCO_CAP_PMU		0x10000000	/* PMU available (rev >= 20) */
 #define  SSB_CHIPCO_CAP_ECI		0x20000000	/* ECI available (rev >= 20) */
+#define  SSB_CHIPCO_CAP_SPROM		0x40000000	/* SPROM present */
 #define SSB_CHIPCO_CORECTL		0x0008
 #define  SSB_CHIPCO_CORECTL_UARTCLK0	0x00000001	/* Drive UART with internal clock */
 #define	 SSB_CHIPCO_CORECTL_SE		0x00000002	/* sync clk out enable (corerev >= 3) */
@@ -385,6 +386,7 @@
 
 
 /** Chip specific Chip-Status register contents. */
+#define SSB_CHIPCO_CHST_4322_SPROM_EXISTS	0x00000040 /* SPROM present */
 #define SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL	0x00000003
 #define SSB_CHIPCO_CHST_4325_DEFCIS_SEL		0 /* OTP is powered up, use def. CIS, no SPROM */
 #define SSB_CHIPCO_CHST_4325_SPROM_SEL		1 /* OTP is powered up, SPROM is present */
@@ -398,6 +400,18 @@
 #define SSB_CHIPCO_CHST_4325_RCAL_VALUE_SHIFT	4
 #define SSB_CHIPCO_CHST_4325_PMUTOP_2B 		0x00000200 /* 1 for 2b, 0 for to 2a */
 
+/** Macros to determine SPROM presence based on Chip-Status register. */
+#define SSB_CHIPCO_CHST_4312_SPROM_PRESENT(status) \
+	((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_OTP_SEL)
+#define SSB_CHIPCO_CHST_4322_SPROM_PRESENT(status) \
+	(status & SSB_CHIPCO_CHST_4322_SPROM_EXISTS)
+#define SSB_CHIPCO_CHST_4325_SPROM_PRESENT(status) \
+	(((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_DEFCIS_SEL) && \
+	 ((status & SSB_CHIPCO_CHST_4325_SPROM_OTP_SEL) != \
+		SSB_CHIPCO_CHST_4325_OTP_SEL))
+
 
 
 /** Clockcontrol masks and values **/
@@ -564,6 +578,7 @@ struct ssb_chipcommon_pmu {
 struct ssb_chipcommon {
 	struct ssb_device *dev;
 	u32 capabilities;
+	u32 status;
 	/* Fast Powerup Delay constant */
 	u16 fast_pwrup_delay;
 	struct ssb_chipcommon_pmu pmu;
-- 
1.6.2.5