[PATCH 2/2] ssb: Save sprom image for dump of device at alternate location

[PATCH 2/2] ssb: Save sprom image for dump of device at alternate location

[Larry Finger]

Some recent Broadcom devices in netbooks have an SPROM that is located
at 0x0800, not the normal location of 0x1000. Initial reading of the
SPROM has been solved in a previous commit; however, dumping to a console
no longer works. This difficulty is fixed by saving the SPROM image
from the initial read, and only freeing that memory at module unload.

Uploading a new SPROM image is not supported for these devices.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

John,

This is 2.6.38 material.

Larry
---
 
Index: wireless-testing/drivers/ssb/pci.c
===================================================================
--- wireless-testing.orig/drivers/ssb/pci.c
+++ wireless-testing/drivers/ssb/pci.c
@@ -709,7 +709,7 @@ static int ssb_pci_sprom_get(struct ssb_
 			if (fallback) {
 				memcpy(sprom, fallback, sizeof(*sprom));
 				err = 0;
-				goto out_free;
+				goto out;
 			}
 			ssb_printk(KERN_WARNING PFX "WARNING: Invalid"
 				   " SPROM CRC (corrupt SPROM)\n");
@@ -763,8 +763,8 @@ static int ssb_pci_sprom_get(struct ssb_
 	}
 	err = sprom_extract(bus, sprom, buf, bus->sprom_size);
 
-out_free:
-	kfree(buf);
+out:
+	bus->sprom_data = buf;
 	return err;
 }
 
@@ -1013,6 +1013,7 @@ void ssb_pci_exit(struct ssb_bus *bus)
 	if (bus->bustype != SSB_BUSTYPE_PCI)
 		return;
 
+	kfree(bus->sprom_data);
 	pdev = bus->host_pci;
 	device_remove_file(&pdev->dev, &dev_attr_ssb_sprom);
 }
Index: wireless-testing/drivers/ssb/sprom.c
===================================================================
--- wireless-testing.orig/drivers/ssb/sprom.c
+++ wireless-testing/drivers/ssb/sprom.c
@@ -72,24 +72,29 @@ ssize_t ssb_attr_sprom_show(struct ssb_b
 	ssize_t count = 0;
 	size_t sprom_size_words = bus->sprom_size;
 
-	sprom = kcalloc(sprom_size_words, sizeof(u16), GFP_KERNEL);
-	if (!sprom)
-		goto out;
-
-	/* Use interruptible locking, as the SPROM write might
-	 * be holding the lock for several seconds. So allow userspace
-	 * to cancel operation. */
-	err = -ERESTARTSYS;
-	if (mutex_lock_interruptible(&bus->sprom_mutex))
-		goto out_kfree;
-	err = sprom_read(bus, sprom);
-	mutex_unlock(&bus->sprom_mutex);
-
+	if (bus->sprom_data) {
+		sprom = bus->sprom_data;
+		err = 0;
+	} else {
+		sprom = kcalloc(sprom_size_words, sizeof(u16), GFP_KERNEL);
+		if (!sprom)
+			goto out;
+
+		/* Use interruptible locking, as the SPROM write might
+		 * be holding the lock for several seconds. So allow userspace
+		 * to cancel operation. */
+		err = -ERESTARTSYS;
+		if (mutex_lock_interruptible(&bus->sprom_mutex))
+			goto out_kfree;
+		err = sprom_read(bus, sprom);
+		mutex_unlock(&bus->sprom_mutex);
+	}
 	if (!err)
 		count = sprom2hex(sprom, buf, PAGE_SIZE, sprom_size_words);
 
 out_kfree:
-	kfree(sprom);
+	if (!bus->sprom_data)
+		kfree(sprom);
 out:
 	return err ? err : count;
 }
@@ -105,6 +110,8 @@ ssize_t ssb_attr_sprom_store(struct ssb_
 	size_t sprom_size_words = bus->sprom_size;
 	struct ssb_freeze_context freeze;
 
+	if (bus->sprom_offset < SSB_SPROM_BASE1)
+		return -EINVAL;
 	sprom = kcalloc(bus->sprom_size, sizeof(u16), GFP_KERNEL);
 	if (!sprom)
 		goto out;
Index: wireless-testing/include/linux/ssb/ssb.h
===================================================================
--- wireless-testing.orig/include/linux/ssb/ssb.h
+++ wireless-testing/include/linux/ssb/ssb.h
@@ -311,6 +311,7 @@ struct ssb_bus {
 	u16 chip_rev;
 	u16 sprom_offset;
 	u16 sprom_size;		/* number of words in sprom */
+	u16 *sprom_data;	/* saved sprom raw data */
 	u8 chip_package;
 
 	/* List of devices (cores) on the backplane. */

[PATCH 2/2] ssb: Save sprom image for dump of device at alternate location

[Michael Büsch]

On Sat, 2010-12-11 at 21:35 -0600, Larry Finger wrote: 
> Some recent Broadcom devices in netbooks have an SPROM that is located
> at 0x0800, not the normal location of 0x1000. Initial reading of the
> SPROM has been solved in a previous commit; however, dumping to a console
> no longer works. This difficulty is fixed by saving the SPROM image
> from the initial read, and only freeing that memory at module unload.

Ah wait. Now I get what you were talking about.
Yes, those devices map the SPROM into the wireless core window.
So, yes, the wireless core has to be mapped for the readout to work,
of course. I don't know what other prerequisites have to be met to
read the data. Maybe IHR region has to be disabled.
I don't know how writing works on these devices.

-- 
Greetings Michael.

[PATCH 2/2] ssb: Save sprom image for dump of device at alternate location

[Michael Büsch]

On Sat, 2010-12-11 at 21:35 -0600, Larry Finger wrote: 
> @@ -1013,6 +1013,7 @@ void ssb_pci_exit(struct ssb_bus *bus)
>  	if (bus->bustype != SSB_BUSTYPE_PCI)
>  		return;
>  
> +	kfree(bus->sprom_data);
>  	pdev = bus->host_pci;
>  	device_remove_file(&pdev->dev, &dev_attr_ssb_sprom);
>  }

Need to put kfree after removing the sprom file. Otherwise there's
a race condition with userspace.

> Index: wireless-testing/drivers/ssb/sprom.c
> ===================================================================
> --- wireless-testing.orig/drivers/ssb/sprom.c
> +++ wireless-testing/drivers/ssb/sprom.c
> @@ -72,24 +72,29 @@ ssize_t ssb_attr_sprom_show(struct ssb_b
>  	ssize_t count = 0;
>  	size_t sprom_size_words = bus->sprom_size;
>  
> -	sprom = kcalloc(sprom_size_words, sizeof(u16), GFP_KERNEL);
> -	if (!sprom)
> -		goto out;
> -
> -	/* Use interruptible locking, as the SPROM write might
> -	 * be holding the lock for several seconds. So allow userspace
> -	 * to cancel operation. */
> -	err = -ERESTARTSYS;
> -	if (mutex_lock_interruptible(&bus->sprom_mutex))
> -		goto out_kfree;
> -	err = sprom_read(bus, sprom);
> -	mutex_unlock(&bus->sprom_mutex);
> -
> +	if (bus->sprom_data) {
> +		sprom = bus->sprom_data;
> +		err = 0;
> +	} else {

This branch is dead now, or do I miss something?

> +		sprom = kcalloc(sprom_size_words, sizeof(u16), GFP_KERNEL);
> +		if (!sprom)
> +			goto out;
> +
> +		/* Use interruptible locking, as the SPROM write might
> +		 * be holding the lock for several seconds. So allow userspace
> +		 * to cancel operation. */
> +		err = -ERESTARTSYS;
> +		if (mutex_lock_interruptible(&bus->sprom_mutex))
> +			goto out_kfree;
> +		err = sprom_read(bus, sprom);
> +		mutex_unlock(&bus->sprom_mutex);
> +	}
>  	if (!err)
>  		count = sprom2hex(sprom, buf, PAGE_SIZE, sprom_size_words);
>  
>  out_kfree:
> -	kfree(sprom);
> +	if (!bus->sprom_data)
> +		kfree(sprom);
>  out:
>  	return err ? err : count;
>  }

I agree that caching the SPROM probably is easier than reading it from
the wireless core at sysfs read time. There are a few concurrency issues
that are hard to solve with the current ssb-pci MMIO access code.
Most notably is that the SPROM read would race with wireless interrupts.

-- 
Greetings Michael.

[PATCH 2/2] ssb: Save sprom image for dump of device at alternate location

[Larry Finger]

On 12/12/2010 03:03 AM, Michael B?sch wrote:
> On Sat, 2010-12-11 at 21:35 -0600, Larry Finger wrote: 
>> @@ -1013,6 +1013,7 @@ void ssb_pci_exit(struct ssb_bus *bus)
>>  	if (bus->bustype != SSB_BUSTYPE_PCI)
>>  		return;
>>  
>> +	kfree(bus->sprom_data);
>>  	pdev = bus->host_pci;
>>  	device_remove_file(&pdev->dev, &dev_attr_ssb_sprom);
>>  }
> 
> Need to put kfree after removing the sprom file. Otherwise there's
> a race condition with userspace.

Will do. Thanks.

>> Index: wireless-testing/drivers/ssb/sprom.c
>> ===================================================================
>> --- wireless-testing.orig/drivers/ssb/sprom.c
>> +++ wireless-testing/drivers/ssb/sprom.c
>> @@ -72,24 +72,29 @@ ssize_t ssb_attr_sprom_show(struct ssb_b
>>  	ssize_t count = 0;
>>  	size_t sprom_size_words = bus->sprom_size;
>>  
>> -	sprom = kcalloc(sprom_size_words, sizeof(u16), GFP_KERNEL);
>> -	if (!sprom)
>> -		goto out;
>> -
>> -	/* Use interruptible locking, as the SPROM write might
>> -	 * be holding the lock for several seconds. So allow userspace
>> -	 * to cancel operation. */
>> -	err = -ERESTARTSYS;
>> -	if (mutex_lock_interruptible(&bus->sprom_mutex))
>> -		goto out_kfree;
>> -	err = sprom_read(bus, sprom);
>> -	mutex_unlock(&bus->sprom_mutex);
>> -
>> +	if (bus->sprom_data) {
>> +		sprom = bus->sprom_data;
>> +		err = 0;
>> +	} else {
> 
> This branch is dead now, or do I miss something?

I kept it for those devices with SPROMs that are not PCI. If there are none,
then it can de removed.

>> +		sprom = kcalloc(sprom_size_words, sizeof(u16), GFP_KERNEL);
>> +		if (!sprom)
>> +			goto out;
>> +
>> +		/* Use interruptible locking, as the SPROM write might
>> +		 * be holding the lock for several seconds. So allow userspace
>> +		 * to cancel operation. */
>> +		err = -ERESTARTSYS;
>> +		if (mutex_lock_interruptible(&bus->sprom_mutex))
>> +			goto out_kfree;
>> +		err = sprom_read(bus, sprom);
>> +		mutex_unlock(&bus->sprom_mutex);
>> +	}
>>  	if (!err)
>>  		count = sprom2hex(sprom, buf, PAGE_SIZE, sprom_size_words);
>>  
>>  out_kfree:
>> -	kfree(sprom);
>> +	if (!bus->sprom_data)
>> +		kfree(sprom);
>>  out:
>>  	return err ? err : count;
>>  }
> 
> I agree that caching the SPROM probably is easier than reading it from
> the wireless core at sysfs read time. There are a few concurrency issues
> that are hard to solve with the current ssb-pci MMIO access code.
> Most notably is that the SPROM read would race with wireless interrupts.

As it was easy and adds very little to the memory footprint, it seems the way to go.

Larry

[PATCH 2/2] ssb: Save sprom image for dump of device at alternate location

[Larry Finger]

On 12/12/2010 02:45 AM, Michael B?sch wrote:
> On Sat, 2010-12-11 at 21:35 -0600, Larry Finger wrote: 
>> Some recent Broadcom devices in netbooks have an SPROM that is located
>> at 0x0800, not the normal location of 0x1000. Initial reading of the
>> SPROM has been solved in a previous commit; however, dumping to a console
>> no longer works. This difficulty is fixed by saving the SPROM image
>> from the initial read, and only freeing that memory at module unload.
> 
> Ah wait. Now I get what you were talking about.
> Yes, those devices map the SPROM into the wireless core window.
> So, yes, the wireless core has to be mapped for the readout to work,
> of course. I don't know what other prerequisites have to be met to
> read the data. Maybe IHR region has to be disabled.
> I don't know how writing works on these devices.

I do not think it is worthwhile to bother to find out how to rewrite the SPROM.
As long as we can read it in case of questions regarding content, that should be
enough. In V2, I will return -ENOTSUP rather than -EINVAL as that is more
appropriate.

Larry

[PATCH 2/2] ssb: Save sprom image for dump of device at alternate location

[Michael Büsch]

On Sun, 2010-12-12 at 09:28 -0600, Larry Finger wrote: 
> >> +	if (bus->sprom_data) {
> >> +		sprom = bus->sprom_data;
> >> +		err = 0;
> >> +	} else {
> > 
> > This branch is dead now, or do I miss something?
> 
> I kept it for those devices with SPROMs that are not PCI. If there are none,
> then it can de removed.

Ah ok. Seems it would still be needed for pcmcia. At least with the
current implementation.

-- 
Greetings Michael.

[PATCH 2/2] ssb: Save sprom image for dump of device at alternate location

[Michael Büsch]

On Sun, 2010-12-12 at 09:34 -0600, Larry Finger wrote: 
> On 12/12/2010 02:45 AM, Michael B?sch wrote:
> > On Sat, 2010-12-11 at 21:35 -0600, Larry Finger wrote: 
> >> Some recent Broadcom devices in netbooks have an SPROM that is located
> >> at 0x0800, not the normal location of 0x1000. Initial reading of the
> >> SPROM has been solved in a previous commit; however, dumping to a console
> >> no longer works. This difficulty is fixed by saving the SPROM image
> >> from the initial read, and only freeing that memory at module unload.
> > 
> > Ah wait. Now I get what you were talking about.
> > Yes, those devices map the SPROM into the wireless core window.
> > So, yes, the wireless core has to be mapped for the readout to work,
> > of course. I don't know what other prerequisites have to be met to
> > read the data. Maybe IHR region has to be disabled.
> > I don't know how writing works on these devices.
> 
> I do not think it is worthwhile to bother to find out how to rewrite the SPROM.
> As long as we can read it in case of questions regarding content, that should be
> enough. In V2, I will return -ENOTSUP rather than -EINVAL as that is more
> appropriate.

So, do we actually make sure the wireless core is mapped while reading
SPROM from offset 0x800? I guess not and it just works by accident,
because the core is still mapped from a previous operation.

-- 
Greetings Michael.

[PATCH 2/2] ssb: Save sprom image for dump of device at alternate location

[Larry Finger]

On 12/12/2010 09:38 AM, Michael B?sch wrote:
> 
> So, do we actually make sure the wireless core is mapped while reading
> SPROM from offset 0x800? I guess not and it just works by accident,
> because the core is still mapped from a previous operation.

I have core switching debug enabled on that box. When the SPROM is read, the
ChipCommon core is mapped, and no other core has yet been selected.

Larry

[PATCH 2/2] ssb: Save sprom image for dump of device at alternate location

[Michael Büsch]

On Sun, 2010-12-12 at 10:05 -0600, Larry Finger wrote: 
> On 12/12/2010 09:38 AM, Michael B?sch wrote:
> > 
> > So, do we actually make sure the wireless core is mapped while reading
> > SPROM from offset 0x800? I guess not and it just works by accident,
> > because the core is still mapped from a previous operation.
> 
> I have core switching debug enabled on that box. When the SPROM is read, the
> ChipCommon core is mapped, and no other core has yet been selected.

Well, ok. Whatever core it is, it's coincidence that it is mapped.
I'd rather prefer an explicit mapping of the chipcommon before
the area is accessed. The chipcommon MMIO functions could probably
be used. That would be easiest. (16bit function does not exist, yet,
but is trivial to add).

-- 
Greetings Michael.