[PATCH] ucode debug status via sysfs for wireless-2.6

[PATCH] ucode debug status via sysfs for wireless-2.6

[Larry Finger]

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

John,

Please apply this patch by Martin Langer to wireless-2.6. It must follow the patch to "Add firmware 
version printout to wireless-2.6 (bcm43xx-softmac)". As originally submitted, the patch was 
appropriate for an obsolete version of bcm43xx-softmac, but I have updated and tested.

Thanks,

Larry

This patch prints out the ucode debug status to sysfs. So, users can
watch the microcode status of their hardware.

Signed-off-by: Martin Langer <martin-langer@gmx.de>
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>

=========================================

diff --git a/drivers/net/wireless/bcm43xx/bcm43xx.h b/drivers/net/wireless/bcm43xx/bcm43xx.h
index 62fd7e2..7c2163e 100644
Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx.h
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx.h
+++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx.h
@@ -311,6 +311,7 @@
  #define BCM43xx_UCODE_PATCHLEVEL        0x0002
  #define BCM43xx_UCODE_DATE              0x0004
  #define BCM43xx_UCODE_TIME              0x0006
+#define BCM43xx_UCODE_STATUS            0x0040

  /* MicrocodeFlagsBitfield (addr + lo-word values?)*/
  #define BCM43xx_UCODEFLAGS_OFFSET	0x005E
Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
+++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
@@ -373,6 +373,56 @@ static DEVICE_ATTR(phymode, 0644,
  		   bcm43xx_attr_phymode_show,
  		   bcm43xx_attr_phymode_store);

+static ssize_t bcm43xx_attr_microcode_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct bcm43xx_private *bcm = dev_to_bcm(dev);
+	ssize_t count = 0;
+	u16 status;
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	mutex_lock(&(bcm)->mutex);
+	status = bcm43xx_shm_read16(bcm, BCM43xx_SHM_SHARED,
+				    BCM43xx_UCODE_STATUS);
+
+	switch (status) {
+	case 0x0000:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (invalid)\n",
+				 status);
+		break;
+	case 0x0001:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (init)\n",
+				 status);
+		break;
+	case 0x0002:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (active)\n",
+				 status);
+		break;
+	case 0x0003:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (suspended)\n",
+				 status);
+		break;
+	case 0x0004:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (asleep)\n",
+				 status);
+		break;
+	default:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (unknown)\n",
+				 status);
+		break;
+	}
+	mutex_unlock(&(bcm)->mutex);
+
+	return count;
+}
+
+static DEVICE_ATTR(microcodestatus, 0444,
+		   bcm43xx_attr_microcode_show,
+		   NULL);
+
  int bcm43xx_sysfs_register(struct bcm43xx_private *bcm)
  {
  	struct device *dev = &bcm->pci_dev->dev;
@@ -392,9 +442,14 @@ int bcm43xx_sysfs_register(struct bcm43x
  	err = device_create_file(dev, &dev_attr_phymode);
  	if (err)
  		goto err_remove_shortpreamble;
+	err = device_create_file(dev, &dev_attr_microcodestatus);
+	if (err)
+		goto err_remove_phymode;

  out:
  	return err;
+err_remove_phymode:
+	device_remove_file(dev, &dev_attr_phymode);
  err_remove_shortpreamble:
  	device_remove_file(dev, &dev_attr_shortpreamble);
  err_remove_interfmode:
@@ -408,6 +463,7 @@ void bcm43xx_sysfs_unregister(struct bcm
  {
  	struct device *dev = &bcm->pci_dev->dev;

+	device_remove_file(dev, &dev_attr_microcodestatus);
  	device_remove_file(dev, &dev_attr_phymode);
  	device_remove_file(dev, &dev_attr_shortpreamble);
  	device_remove_file(dev, &dev_attr_interference);



===========

[-- Attachment #2: patch_ucode_debug --]
[-- Type: text/plain, Size: 3632 bytes --]

diff --git a/drivers/net/wireless/bcm43xx/bcm43xx.h b/drivers/net/wireless/bcm43xx/bcm43xx.h
index 62fd7e2..7c2163e 100644
Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx.h
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx.h
+++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx.h
@@ -311,6 +311,7 @@
 #define BCM43xx_UCODE_PATCHLEVEL        0x0002
 #define BCM43xx_UCODE_DATE              0x0004
 #define BCM43xx_UCODE_TIME              0x0006
+#define BCM43xx_UCODE_STATUS            0x0040
 
 /* MicrocodeFlagsBitfield (addr + lo-word values?)*/
 #define BCM43xx_UCODEFLAGS_OFFSET	0x005E
Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
+++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
@@ -373,6 +373,56 @@ static DEVICE_ATTR(phymode, 0644,
 		   bcm43xx_attr_phymode_show,
 		   bcm43xx_attr_phymode_store);
 
+static ssize_t bcm43xx_attr_microcode_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct bcm43xx_private *bcm = dev_to_bcm(dev);
+	ssize_t count = 0;
+	u16 status;
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	mutex_lock(&(bcm)->mutex);
+	status = bcm43xx_shm_read16(bcm, BCM43xx_SHM_SHARED,
+				    BCM43xx_UCODE_STATUS);
+
+	switch (status) {
+	case 0x0000:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (invalid)\n",
+				 status);
+		break;
+	case 0x0001:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (init)\n",
+				 status);
+		break;
+	case 0x0002:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (active)\n",
+				 status);
+		break;
+	case 0x0003:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (suspended)\n",
+				 status);
+		break;
+	case 0x0004:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (asleep)\n",
+				 status);
+		break;
+	default:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (unknown)\n",
+				 status);
+		break;
+	}
+	mutex_unlock(&(bcm)->mutex);
+
+	return count;
+}
+
+static DEVICE_ATTR(microcodestatus, 0444,
+		   bcm43xx_attr_microcode_show,
+		   NULL);
+
 int bcm43xx_sysfs_register(struct bcm43xx_private *bcm)
 {
 	struct device *dev = &bcm->pci_dev->dev;
@@ -392,9 +442,14 @@ int bcm43xx_sysfs_register(struct bcm43x
 	err = device_create_file(dev, &dev_attr_phymode);
 	if (err)
 		goto err_remove_shortpreamble;
+	err = device_create_file(dev, &dev_attr_microcodestatus);
+	if (err)
+		goto err_remove_phymode;
 
 out:
 	return err;
+err_remove_phymode:
+	device_remove_file(dev, &dev_attr_phymode);
 err_remove_shortpreamble:
 	device_remove_file(dev, &dev_attr_shortpreamble);
 err_remove_interfmode:
@@ -408,6 +463,7 @@ void bcm43xx_sysfs_unregister(struct bcm
 {
 	struct device *dev = &bcm->pci_dev->dev;
 
+	device_remove_file(dev, &dev_attr_microcodestatus);
 	device_remove_file(dev, &dev_attr_phymode);
 	device_remove_file(dev, &dev_attr_shortpreamble);
 	device_remove_file(dev, &dev_attr_interference);
Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.c
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_main.c
+++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_main.c
@@ -3141,7 +3141,7 @@ static void do_periodic_work(struct bcm4
 		bcm43xx_periodic_every15sec(bcm);
 	bcm->periodic_state = state + 1;
 
-	schedule_delayed_work(&bcm->periodic_work, HZ * 15);
+	schedule_delayed_work(&bcm->periodic_work, HZ * 3);
 }
 
 /* Estimate a "Badness" value based on the periodic work

Re: [PATCH] ucode debug status via sysfs for wireless-2.6

[Michael Buesch]

On Thursday 07 September 2006 03:34, Larry Finger wrote:
> John,
> 
> Please apply this patch by Martin Langer to wireless-2.6. It must follow the patch to "Add firmware 
> version printout to wireless-2.6 (bcm43xx-softmac)". As originally submitted, the patch was 
> appropriate for an obsolete version of bcm43xx-softmac, but I have updated and tested.
> 
> Thanks,
> 
> Larry
> 
> This patch prints out the ucode debug status to sysfs. So, users can
> watch the microcode status of their hardware.
> 
> Signed-off-by: Martin Langer <martin-langer-Mmb7MZpHnFY@public.gmane.org>
> Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
> 
> =========================================
> 
> diff --git a/drivers/net/wireless/bcm43xx/bcm43xx.h b/drivers/net/wireless/bcm43xx/bcm43xx.h
> index 62fd7e2..7c2163e 100644
> Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx.h
> ===================================================================
> --- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx.h
> +++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx.h
> @@ -311,6 +311,7 @@
>   #define BCM43xx_UCODE_PATCHLEVEL        0x0002
>   #define BCM43xx_UCODE_DATE              0x0004
>   #define BCM43xx_UCODE_TIME              0x0006
> +#define BCM43xx_UCODE_STATUS            0x0040
> 
>   /* MicrocodeFlagsBitfield (addr + lo-word values?)*/
>   #define BCM43xx_UCODEFLAGS_OFFSET	0x005E
> Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
> ===================================================================
> --- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
> +++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
> @@ -373,6 +373,56 @@ static DEVICE_ATTR(phymode, 0644,
>   		   bcm43xx_attr_phymode_show,
>   		   bcm43xx_attr_phymode_store);
> 
> +static ssize_t bcm43xx_attr_microcode_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct bcm43xx_private *bcm = dev_to_bcm(dev);
> +	ssize_t count = 0;
> +	u16 status;
> +
> +	if (!capable(CAP_NET_ADMIN))
> +		return -EPERM;
> +

you want to take the spinlock lock here, too.

> +	mutex_lock(&(bcm)->mutex);
> +	status = bcm43xx_shm_read16(bcm, BCM43xx_SHM_SHARED,
> +				    BCM43xx_UCODE_STATUS);

can unlock both here.

> +	switch (status) {
> +	case 0x0000:
> +		count = snprintf(buf, PAGE_SIZE, "0x%.4x (invalid)\n",
> +				 status);
> +		break;
> +	case 0x0001:
> +		count = snprintf(buf, PAGE_SIZE, "0x%.4x (init)\n",
> +				 status);
> +		break;
> +	case 0x0002:
> +		count = snprintf(buf, PAGE_SIZE, "0x%.4x (active)\n",
> +				 status);
> +		break;
> +	case 0x0003:
> +		count = snprintf(buf, PAGE_SIZE, "0x%.4x (suspended)\n",
> +				 status);
> +		break;
> +	case 0x0004:
> +		count = snprintf(buf, PAGE_SIZE, "0x%.4x (asleep)\n",
> +				 status);
> +		break;
> +	default:
> +		count = snprintf(buf, PAGE_SIZE, "0x%.4x (unknown)\n",
> +				 status);
> +		break;
> +	}
> +	mutex_unlock(&(bcm)->mutex);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(microcodestatus, 0444,
> +		   bcm43xx_attr_microcode_show,
> +		   NULL);
> +
>   int bcm43xx_sysfs_register(struct bcm43xx_private *bcm)
>   {
>   	struct device *dev = &bcm->pci_dev->dev;
> @@ -392,9 +442,14 @@ int bcm43xx_sysfs_register(struct bcm43x
>   	err = device_create_file(dev, &dev_attr_phymode);
>   	if (err)
>   		goto err_remove_shortpreamble;
> +	err = device_create_file(dev, &dev_attr_microcodestatus);
> +	if (err)
> +		goto err_remove_phymode;
> 
>   out:
>   	return err;
> +err_remove_phymode:
> +	device_remove_file(dev, &dev_attr_phymode);
>   err_remove_shortpreamble:
>   	device_remove_file(dev, &dev_attr_shortpreamble);
>   err_remove_interfmode:
> @@ -408,6 +463,7 @@ void bcm43xx_sysfs_unregister(struct bcm
>   {
>   	struct device *dev = &bcm->pci_dev->dev;
> 
> +	device_remove_file(dev, &dev_attr_microcodestatus);
>   	device_remove_file(dev, &dev_attr_phymode);
>   	device_remove_file(dev, &dev_attr_shortpreamble);
>   	device_remove_file(dev, &dev_attr_interference);
> 
> 
> 
> ===========
> 

-- 
Greetings Michael.

Re: [PATCH] ucode debug status via sysfs for wireless-2.6

[Larry Finger]

Michael Buesch wrote:
> On Thursday 07 September 2006 03:34, Larry Finger wrote:
>> +		return -EPERM;
>> +
> 
> you want to take the spinlock lock here, too.

Obviously, I copied the wrong model. Is it correct that one should take both locks if your code will 
touch the hardware, but the mutex lock only is sufficient if your code just accesses data 
structures? This seems to be the pattern in the other bcm43xx_attr_xxxx_show routines.

Larry

[PATCH] Try 2: ucode debug status via sysfs for wireless-2.6

[Larry Finger]

John,

Please apply this patch by Martin Langer to wireless-2.6. It must follow the patch to "Add firmware
version printout to wireless-2.6 (bcm43xx-softmac)". Martins's original version was appropriate for 
an obsolete version of bcm43xx-softmac, but I have updated and tested. This version incorporates the 
locking revision comments of Michael Buesch.

Thanks,

Larry

This patch prints out the ucode debug status to sysfs. So, users can
watch the microcode status of their hardware.

Signed-off-by: Martin Langer <martin-langer-Mmb7MZpHnFY@public.gmane.org>
Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>



===========

diff --git a/drivers/net/wireless/bcm43xx/bcm43xx.h b/drivers/net/wireless/bcm43xx/bcm43xx.h
index 62fd7e2..7c2163e 100644
Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx.h
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx.h
+++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx.h
@@ -311,6 +311,7 @@
  #define BCM43xx_UCODE_PATCHLEVEL        0x0002
  #define BCM43xx_UCODE_DATE              0x0004
  #define BCM43xx_UCODE_TIME              0x0006
+#define BCM43xx_UCODE_STATUS            0x0040

  /* MicrocodeFlagsBitfield (addr + lo-word values?)*/
  #define BCM43xx_UCODEFLAGS_OFFSET	0x005E
Index: wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
+++ wireless-2.6/drivers/net/wireless/bcm43xx/bcm43xx_sysfs.c
@@ -373,6 +373,59 @@ static DEVICE_ATTR(phymode, 0644,
  		   bcm43xx_attr_phymode_show,
  		   bcm43xx_attr_phymode_store);

+static ssize_t bcm43xx_attr_microcode_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	unsigned long flags;
+	struct bcm43xx_private *bcm = dev_to_bcm(dev);
+	ssize_t count = 0;
+	u16 status;
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	mutex_lock(&(bcm)->mutex);
+	spin_lock_irqsave(&bcm->irq_lock, flags);
+	status = bcm43xx_shm_read16(bcm, BCM43xx_SHM_SHARED,
+				    BCM43xx_UCODE_STATUS);
+
+	spin_unlock_irqrestore(&bcm->irq_lock, flags);
+	mutex_unlock(&(bcm)->mutex);
+	switch (status) {
+	case 0x0000:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (invalid)\n",
+				 status);
+		break;
+	case 0x0001:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (init)\n",
+				 status);
+		break;
+	case 0x0002:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (active)\n",
+				 status);
+		break;
+	case 0x0003:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (suspended)\n",
+				 status);
+		break;
+	case 0x0004:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (asleep)\n",
+				 status);
+		break;
+	default:
+		count = snprintf(buf, PAGE_SIZE, "0x%.4x (unknown)\n",
+				 status);
+		break;
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR(microcodestatus, 0444,
+		   bcm43xx_attr_microcode_show,
+		   NULL);
+
  int bcm43xx_sysfs_register(struct bcm43xx_private *bcm)
  {
  	struct device *dev = &bcm->pci_dev->dev;
@@ -392,9 +445,14 @@ int bcm43xx_sysfs_register(struct bcm43x
  	err = device_create_file(dev, &dev_attr_phymode);
  	if (err)
  		goto err_remove_shortpreamble;
+	err = device_create_file(dev, &dev_attr_microcodestatus);
+	if (err)
+		goto err_remove_phymode;

  out:
  	return err;
+err_remove_phymode:
+	device_remove_file(dev, &dev_attr_phymode);
  err_remove_shortpreamble:
  	device_remove_file(dev, &dev_attr_shortpreamble);
  err_remove_interfmode:
@@ -408,6 +466,7 @@ void bcm43xx_sysfs_unregister(struct bcm
  {
  	struct device *dev = &bcm->pci_dev->dev;

+	device_remove_file(dev, &dev_attr_microcodestatus);
  	device_remove_file(dev, &dev_attr_phymode);
  	device_remove_file(dev, &dev_attr_shortpreamble);
  	device_remove_file(dev, &dev_attr_interference);

Re: [PATCH] ucode debug status via sysfs for wireless-2.6

[Michael Buesch]

On Thursday 07 September 2006 15:21, Larry Finger wrote:
> Michael Buesch wrote:
> > On Thursday 07 September 2006 03:34, Larry Finger wrote:
> >> +		return -EPERM;
> >> +
> > 
> > you want to take the spinlock lock here, too.
> 
> Obviously, I copied the wrong model. Is it correct that one should take both locks if your code will 
> touch the hardware, but the mutex lock only is sufficient if your code just accesses data 
> structures? This seems to be the pattern in the other bcm43xx_attr_xxxx_show routines.

In general, no.
But, for some sysfs attrs it is sufficient to only take
the mutex, because:
* We don't access hardware.
* We don't modify this data in a spinlock-only critical section.

Yes, I know that having two locks does not really fit the
"lock data, not code" model. But it's well defined in bcm43xx,
so I think we can live with it. (and we must live with it,
if we want to have preemptible periodic work. And we _want_).
It's defined by the following rules:

1) Always take both, mutex and lock.
2) There are only two places where we can't take the
   mutex, but only the spinlock. IRQ and TX paths.

(Yes, I know that there are still exceptions to 2.
At least in dscape. The softmac port should be OK.
These are bugs, I am aware of them and will fix it)

So these two rules lead to the following rule:

* Code where we only take the mutex can race against the
TX and IRQ paths.
Now we come back to the sysfs problem above. If the data, we
access in this sysfs code, is not touched in either TX or IRQ path
we don't need to take the spinlock. Yes, it's a little bit black
magic. So if you aren't sure, always take both locks.

-- 
Greetings Michael.

Re: [PATCH] ucode debug status via sysfs for wireless-2.6

[Larry Finger]

Michael Buesch wrote:
> 
> In general, no.
> But, for some sysfs attrs it is sufficient to only take
> the mutex, because:
> * We don't access hardware.
> * We don't modify this data in a spinlock-only critical section.
> 
> Yes, I know that having two locks does not really fit the
> "lock data, not code" model. But it's well defined in bcm43xx,
> so I think we can live with it. (and we must live with it,
> if we want to have preemptible periodic work. And we _want_).
> It's defined by the following rules:
> 
> 1) Always take both, mutex and lock.
> 2) There are only two places where we can't take the
>    mutex, but only the spinlock. IRQ and TX paths.
> 
> (Yes, I know that there are still exceptions to 2.
> At least in dscape. The softmac port should be OK.
> These are bugs, I am aware of them and will fix it)
> 
> So these two rules lead to the following rule:
> 
> * Code where we only take the mutex can race against the
> TX and IRQ paths.
> Now we come back to the sysfs problem above. If the data, we
> access in this sysfs code, is not touched in either TX or IRQ path
> we don't need to take the spinlock. Yes, it's a little bit black
> magic. So if you aren't sure, always take both locks.

Thanks for the clarification.

Larry