linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH]wireless:ath9k Fix ath_print in xmit.c
@ 2010-05-26 17:14 Justin P. Mattock
  2010-05-26 17:14 ` [PATCH]wireless:ath9k Disable leds for Apple products Justin P. Mattock
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Justin P. Mattock @ 2010-05-26 17:14 UTC (permalink / raw)
  To: linux-wireless; +Cc: linville, mcgrof, Justin P. Mattock

ath_print in xmit.c should say "Reseting hardware"
instead of Reaseting HAL!(since HAL is being fazed out).
dmesg shows:
[ 8660.899624] ath: Failed to stop TX DMA in 100 msec after killing last frame
[ 8660.899676] ath: Unable to stop TxDMA. Reset HAL!

 
 Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
 drivers/net/wireless/ath/ath9k/xmit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 3db1917..2a0558e 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1198,7 +1198,7 @@ void ath_drain_all_txq(struct ath_softc *sc, bool retry_tx)
 		int r;
 
 		ath_print(common, ATH_DBG_FATAL,
-			  "Unable to stop TxDMA. Reset HAL!\n");
+			  "Unable to stop TxDMA. Reseting hardware!\n");
 
 		spin_lock_bh(&sc->sc_resetlock);
 		r = ath9k_hw_reset(ah, sc->sc_ah->curchan, false);
-- 
1.6.5.GIT


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH]wireless:ath9k Disable leds for Apple products.
  2010-05-26 17:14 [PATCH]wireless:ath9k Fix ath_print in xmit.c Justin P. Mattock
@ 2010-05-26 17:14 ` Justin P. Mattock
  2010-05-26 17:18   ` John W. Linville
  2010-05-26 17:19   ` Luis R. Rodriguez
  2010-05-26 17:16 ` [PATCH]wireless:ath9k Fix ath_print in xmit.c Luis R. Rodriguez
  2010-05-26 17:28 ` Arnd Hannemann
  2 siblings, 2 replies; 12+ messages in thread
From: Justin P. Mattock @ 2010-05-26 17:14 UTC (permalink / raw)
  To: linux-wireless; +Cc: linville, mcgrof, Justin P. Mattock

Disable the leds on ath9k for Apple products, since
there is no leds on any of there machines(or non that I can find!!).

 Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

---
 drivers/net/wireless/ath/ath9k/gpio.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/gpio.c b/drivers/net/wireless/ath/ath9k/gpio.c
index 0ee75e7..c21e74f 100644
--- a/drivers/net/wireless/ath/ath9k/gpio.c
+++ b/drivers/net/wireless/ath/ath9k/gpio.c
@@ -15,6 +15,7 @@
  */
 
 #include "ath9k.h"
+#include <linux/dmi.h>
 
 /********************************/
 /*	 LED functions		*/
@@ -127,11 +128,30 @@ void ath_deinit_leds(struct ath_softc *sc)
 	ath9k_hw_set_gpio(sc->sc_ah, sc->sc_ah->led_pin, 1);
 }
 
+static struct dmi_system_id __initdata dmi_system_table[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_BIOS_VENDOR, "Apple Computer, Inc.")
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_BIOS_VENDOR, "Apple Inc.")
+		},
+	},
+	{}
+};
+
 void ath_init_leds(struct ath_softc *sc)
 {
 	char *trigger;
 	int ret;
 
+	/* Apple has no leds lights for their wireless.  */
+	if (dmi_check_system(dmi_system_table) > 0)
+		return -ENODEV;
+	else
+
 	if (AR_SREV_9287(sc->sc_ah))
 		sc->sc_ah->led_pin = ATH_LED_PIN_9287;
 	else
-- 
1.6.5.GIT


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH]wireless:ath9k Fix ath_print in xmit.c
  2010-05-26 17:14 [PATCH]wireless:ath9k Fix ath_print in xmit.c Justin P. Mattock
  2010-05-26 17:14 ` [PATCH]wireless:ath9k Disable leds for Apple products Justin P. Mattock
@ 2010-05-26 17:16 ` Luis R. Rodriguez
  2010-05-26 17:28 ` Arnd Hannemann
  2 siblings, 0 replies; 12+ messages in thread
From: Luis R. Rodriguez @ 2010-05-26 17:16 UTC (permalink / raw)
  To: Justin P. Mattock; +Cc: linux-wireless, linville

Subject should just be

ath9k: Fix ath_print in xmit for hardware reset

On Wed, May 26, 2010 at 10:14 AM, Justin P. Mattock
<justinmattock@gmail.com> wrote:
> ath_print in xmit.c should say "Reseting hardware"
> instead of Reaseting HAL!(since HAL is being fazed out).
> dmesg shows:
> [ 8660.899624] ath: Failed to stop TX DMA in 100 msec after killing last frame
> [ 8660.899676] ath: Unable to stop TxDMA. Reset HAL!
>
>
>  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>

Looks good. You want to address patches To john, and cc the rest.

> ---
>  drivers/net/wireless/ath/ath9k/xmit.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index 3db1917..2a0558e 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -1198,7 +1198,7 @@ void ath_drain_all_txq(struct ath_softc *sc, bool retry_tx)
>                int r;
>
>                ath_print(common, ATH_DBG_FATAL,
> -                         "Unable to stop TxDMA. Reset HAL!\n");
> +                         "Unable to stop TxDMA. Reseting hardware!\n");
>
>                spin_lock_bh(&sc->sc_resetlock);
>                r = ath9k_hw_reset(ah, sc->sc_ah->curchan, false);
> --
> 1.6.5.GIT
>
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH]wireless:ath9k Disable leds for Apple products.
  2010-05-26 17:14 ` [PATCH]wireless:ath9k Disable leds for Apple products Justin P. Mattock
@ 2010-05-26 17:18   ` John W. Linville
  2010-05-26 17:38     ` Justin P. Mattock
  2010-05-26 17:19   ` Luis R. Rodriguez
  1 sibling, 1 reply; 12+ messages in thread
From: John W. Linville @ 2010-05-26 17:18 UTC (permalink / raw)
  To: Justin P. Mattock; +Cc: linux-wireless, mcgrof

On Wed, May 26, 2010 at 10:14:16AM -0700, Justin P. Mattock wrote:
> Disable the leds on ath9k for Apple products, since
> there is no leds on any of there machines(or non that I can find!!).
> 
>  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
> 

>  void ath_init_leds(struct ath_softc *sc)
>  {
>  	char *trigger;
>  	int ret;
>  
> +	/* Apple has no leds lights for their wireless.  */
> +	if (dmi_check_system(dmi_system_table) > 0)
> +		return -ENODEV;
> +	else
> +
>  	if (AR_SREV_9287(sc->sc_ah))
>  		sc->sc_ah->led_pin = ATH_LED_PIN_9287;
>  	else

Surely we don't need the 'else'.

Does enabling the LEDs on these systems cause any problems?

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH]wireless:ath9k Disable leds for Apple products.
  2010-05-26 17:14 ` [PATCH]wireless:ath9k Disable leds for Apple products Justin P. Mattock
  2010-05-26 17:18   ` John W. Linville
@ 2010-05-26 17:19   ` Luis R. Rodriguez
  1 sibling, 0 replies; 12+ messages in thread
From: Luis R. Rodriguez @ 2010-05-26 17:19 UTC (permalink / raw)
  To: Justin P. Mattock, David Quan, Vinod Nagarajan; +Cc: linux-wireless, linville

Note: This e-mail is on a public mailing list.

David, Vinod, is it true, no Apple devices have LEDs?

On Wed, May 26, 2010 at 10:14 AM, Justin P. Mattock
<justinmattock@gmail.com> wrote:
> Disable the leds on ath9k for Apple products, since
> there is no leds on any of there machines(or non that I can find!!).
>
>  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
>
> ---
>  drivers/net/wireless/ath/ath9k/gpio.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/gpio.c b/drivers/net/wireless/ath/ath9k/gpio.c
> index 0ee75e7..c21e74f 100644
> --- a/drivers/net/wireless/ath/ath9k/gpio.c
> +++ b/drivers/net/wireless/ath/ath9k/gpio.c
> @@ -15,6 +15,7 @@
>  */
>
>  #include "ath9k.h"
> +#include <linux/dmi.h>
>
>  /********************************/
>  /*      LED functions          */
> @@ -127,11 +128,30 @@ void ath_deinit_leds(struct ath_softc *sc)
>        ath9k_hw_set_gpio(sc->sc_ah, sc->sc_ah->led_pin, 1);
>  }
>
> +static struct dmi_system_id __initdata dmi_system_table[] = {
> +       {
> +               .matches = {
> +                       DMI_MATCH(DMI_BIOS_VENDOR, "Apple Computer, Inc.")
> +               },
> +       },
> +       {
> +               .matches = {
> +                       DMI_MATCH(DMI_BIOS_VENDOR, "Apple Inc.")
> +               },
> +       },
> +       {}
> +};
> +
>  void ath_init_leds(struct ath_softc *sc)
>  {
>        char *trigger;
>        int ret;
>
> +       /* Apple has no leds lights for their wireless.  */
> +       if (dmi_check_system(dmi_system_table) > 0)
> +               return -ENODEV;
> +       else
> +
>        if (AR_SREV_9287(sc->sc_ah))
>                sc->sc_ah->led_pin = ATH_LED_PIN_9287;
>        else
> --
> 1.6.5.GIT
>
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH]wireless:ath9k Fix ath_print in xmit.c
  2010-05-26 17:14 [PATCH]wireless:ath9k Fix ath_print in xmit.c Justin P. Mattock
  2010-05-26 17:14 ` [PATCH]wireless:ath9k Disable leds for Apple products Justin P. Mattock
  2010-05-26 17:16 ` [PATCH]wireless:ath9k Fix ath_print in xmit.c Luis R. Rodriguez
@ 2010-05-26 17:28 ` Arnd Hannemann
  2010-05-26 17:49   ` Justin P. Mattock
  2 siblings, 1 reply; 12+ messages in thread
From: Arnd Hannemann @ 2010-05-26 17:28 UTC (permalink / raw)
  To: Justin P. Mattock; +Cc: linux-wireless, linville, mcgrof

Am 26.05.2010 19:14, schrieb Justin P. Mattock:
> ath_print in xmit.c should say "Reseting hardware"
> instead of Reaseting HAL!(since HAL is being fazed out).
> dmesg shows:
> [ 8660.899624] ath: Failed to stop TX DMA in 100 msec after killing last frame
> [ 8660.899676] ath: Unable to stop TxDMA. Reset HAL!
>
>  
>  Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
>
> ---
>  drivers/net/wireless/ath/ath9k/xmit.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index 3db1917..2a0558e 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -1198,7 +1198,7 @@ void ath_drain_all_txq(struct ath_softc *sc, bool retry_tx)
>  		int r;
>  
>  		ath_print(common, ATH_DBG_FATAL,
> -			  "Unable to stop TxDMA. Reset HAL!\n");
> +			  "Unable to stop TxDMA. Reseting hardware!\n");
>   
1.) Should'nt it read "Resetting"
2.) Why "common" ? Seems it is ath9k specific so message should read
"ath9k: Failed to stop TX DMA"
3.) While we are at it write "TX DMA" instead of "TxDMA" just to be
consistent...
>  
>  		spin_lock_bh(&sc->sc_resetlock);
>  		r = ath9k_hw_reset(ah, sc->sc_ah->curchan, false);
>   

Best regards,
Arnd


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH]wireless:ath9k Disable leds for Apple products.
  2010-05-26 17:18   ` John W. Linville
@ 2010-05-26 17:38     ` Justin P. Mattock
  2010-05-26 21:05       ` Luis R. Rodriguez
  0 siblings, 1 reply; 12+ messages in thread
From: Justin P. Mattock @ 2010-05-26 17:38 UTC (permalink / raw)
  To: John W. Linville; +Cc: linux-wireless, mcgrof

On 05/26/2010 10:18 AM, John W. Linville wrote:
> On Wed, May 26, 2010 at 10:14:16AM -0700, Justin P. Mattock wrote:
>    
>> Disable the leds on ath9k for Apple products, since
>> there is no leds on any of there machines(or non that I can find!!).
>>
>>   Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>
>>      
>    
>>   void ath_init_leds(struct ath_softc *sc)
>>   {
>>   	char *trigger;
>>   	int ret;
>>
>> +	/* Apple has no leds lights for their wireless.  */
>> +	if (dmi_check_system(dmi_system_table)>  0)
>> +		return -ENODEV;
>> +	else
>> +
>>   	if (AR_SREV_9287(sc->sc_ah))
>>   		sc->sc_ah->led_pin = ATH_LED_PIN_9287;
>>   	else
>>      
> Surely we don't need the 'else'.
>
> Does enabling the LEDs on these systems cause any problems?
>
> John
>    
I picked up the idea, from this patch:
http://kerneltrap.org/mailarchive/linux-kernel/2010/1/20/4530535
while looking into a bug for ath9k(thinking maybe leds
are causing something, which want  the case)

so Id have to say I don't think the leds cause issue's,
if anything just a wasted symlink, call, or whatever
in /sys/class/leds/*

Justin P. mattock


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH]wireless:ath9k Fix ath_print in xmit.c
  2010-05-26 17:28 ` Arnd Hannemann
@ 2010-05-26 17:49   ` Justin P. Mattock
  0 siblings, 0 replies; 12+ messages in thread
From: Justin P. Mattock @ 2010-05-26 17:49 UTC (permalink / raw)
  To: Arnd Hannemann; +Cc: linux-wireless, linville, mcgrof

On 05/26/2010 10:28 AM, Arnd Hannemann wrote:
> Am 26.05.2010 19:14, schrieb Justin P. Mattock:
>    
>> ath_print in xmit.c should say "Reseting hardware"
>> instead of Reaseting HAL!(since HAL is being fazed out).
>> dmesg shows:
>> [ 8660.899624] ath: Failed to stop TX DMA in 100 msec after killing last frame
>> [ 8660.899676] ath: Unable to stop TxDMA. Reset HAL!
>>
>>
>>   Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>
>> ---
>>   drivers/net/wireless/ath/ath9k/xmit.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
>> index 3db1917..2a0558e 100644
>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>> @@ -1198,7 +1198,7 @@ void ath_drain_all_txq(struct ath_softc *sc, bool retry_tx)
>>   		int r;
>>
>>   		ath_print(common, ATH_DBG_FATAL,
>> -			  "Unable to stop TxDMA. Reset HAL!\n");
>> +			  "Unable to stop TxDMA. Reseting hardware!\n");
>>
>>      
> 1.) Should'nt it read "Resetting"
> 2.) Why "common" ? Seems it is ath9k specific so message should read
> "ath9k: Failed to stop TX DMA"
> 3.) While we are at it write "TX DMA" instead of "TxDMA" just to be
> consistent...
>    

pretty bad!! I cant spell.
>>
>>   		spin_lock_bh(&sc->sc_resetlock);
>>   		r = ath9k_hw_reset(ah, sc->sc_ah->curchan, false);
>>
>>      
> Best regards,
> Arnd
>
>
>    
o.k. re-submitted, with the above fixes, if good
let me know..(a bit confused with the signed off
stuff(if I need to re-du to add the correct signed-off's
let me know))

Justin P. mattock

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH]wireless:ath9k Disable leds for Apple products.
  2010-05-26 17:38     ` Justin P. Mattock
@ 2010-05-26 21:05       ` Luis R. Rodriguez
  2010-05-26 21:34         ` Justin P. Mattock
  0 siblings, 1 reply; 12+ messages in thread
From: Luis R. Rodriguez @ 2010-05-26 21:05 UTC (permalink / raw)
  To: Justin P. Mattock; +Cc: John W. Linville, linux-wireless

On Wed, May 26, 2010 at 10:38 AM, Justin P. Mattock
<justinmattock@gmail.com> wrote:
> On 05/26/2010 10:18 AM, John W. Linville wrote:
>>
>> On Wed, May 26, 2010 at 10:14:16AM -0700, Justin P. Mattock wrote:
>>
>>>
>>> Disable the leds on ath9k for Apple products, since
>>> there is no leds on any of there machines(or non that I can find!!).
>>>
>>>  Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>

Our team has confirmed Apple devices do not have LEDs so a change like
this is OK but this patch is wrong anyway. More details below.

>>>
>>>  void ath_init_leds(struct ath_softc *sc)
>>>  {
>>>        char *trigger;
>>>        int ret;
>>>
>>> +       /* Apple has no leds lights for their wireless.  */
>>> +       if (dmi_check_system(dmi_system_table)>  0)
>>> +               return -ENODEV;
>>> +       else
>>> +
>>>        if (AR_SREV_9287(sc->sc_ah))
>>>                sc->sc_ah->led_pin = ATH_LED_PIN_9287;
>>>        else
>>>
>>
>> Surely we don't need the 'else'.
>>
>> Does enabling the LEDs on these systems cause any problems?
>>
>> John
>>
>
> I picked up the idea, from this patch:
> http://kerneltrap.org/mailarchive/linux-kernel/2010/1/20/4530535
> while looking into a bug for ath9k(thinking maybe leds
> are causing something, which want  the case)
>
> so Id have to say I don't think the leds cause issue's,
> if anything just a wasted symlink, call, or whatever
> in /sys/class/leds/*

The patch is wrong anyway, ath_init_leds() is void and yet you return
a value. If this is really needed I'd also prefer if you define a bool
or something and use a ah->ignore_leds and set this to true if the DMI
stuff checks out for Apple up hw init. Then check for ignore_leds
prior to calling ath_init_leds() and also avoid it if set. Also check
for ingore_leds upon device cleanup and ignore the
acancel_delayed_work_sync(&sc->ath_led_blink_work) and
ath_deinit_leds() and skip that.

But given that it is not needed it seems a lot of work for something
that is a non-issue.

  Luis

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH]wireless:ath9k Disable leds for Apple products.
  2010-05-26 21:05       ` Luis R. Rodriguez
@ 2010-05-26 21:34         ` Justin P. Mattock
  2010-05-26 21:42           ` Luis R. Rodriguez
  0 siblings, 1 reply; 12+ messages in thread
From: Justin P. Mattock @ 2010-05-26 21:34 UTC (permalink / raw)
  To: Luis R. Rodriguez; +Cc: John W. Linville, linux-wireless

On 05/26/2010 02:05 PM, Luis R. Rodriguez wrote:
> On Wed, May 26, 2010 at 10:38 AM, Justin P. Mattock
> <justinmattock@gmail.com>  wrote:
>    
>> On 05/26/2010 10:18 AM, John W. Linville wrote:
>>      
>>> On Wed, May 26, 2010 at 10:14:16AM -0700, Justin P. Mattock wrote:
>>>
>>>        
>>>> Disable the leds on ath9k for Apple products, since
>>>> there is no leds on any of there machines(or non that I can find!!).
>>>>
>>>>   Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>>>          
> Our team has confirmed Apple devices do not have LEDs so a change like
> this is OK but this patch is wrong anyway. More details below.
>
>    
>>>>   void ath_init_leds(struct ath_softc *sc)
>>>>   {
>>>>         char *trigger;
>>>>         int ret;
>>>>
>>>> +       /* Apple has no leds lights for their wireless.  */
>>>> +       if (dmi_check_system(dmi_system_table)>    0)
>>>> +               return -ENODEV;
>>>> +       else
>>>> +
>>>>         if (AR_SREV_9287(sc->sc_ah))
>>>>                 sc->sc_ah->led_pin = ATH_LED_PIN_9287;
>>>>         else
>>>>
>>>>          
>>> Surely we don't need the 'else'.
>>>
>>> Does enabling the LEDs on these systems cause any problems?
>>>
>>> John
>>>
>>>        
>> I picked up the idea, from this patch:
>> http://kerneltrap.org/mailarchive/linux-kernel/2010/1/20/4530535
>> while looking into a bug for ath9k(thinking maybe leds
>> are causing something, which want  the case)
>>
>> so Id have to say I don't think the leds cause issue's,
>> if anything just a wasted symlink, call, or whatever
>> in /sys/class/leds/*
>>      
> The patch is wrong anyway, ath_init_leds() is void and yet you return
> a value. If this is really needed I'd also prefer if you define a bool
> or something and use a ah->ignore_leds and set this to true if the DMI
> stuff checks out for Apple up hw init. Then check for ignore_leds
> prior to calling ath_init_leds() and also avoid it if set. Also check
> for ingore_leds upon device cleanup and ignore the
> acancel_delayed_work_sync(&sc->ath_led_blink_work) and
> ath_deinit_leds() and skip that.
>
> But given that it is not needed it seems a lot of work for something
> that is a non-issue.
>
>    Luis
>
>    
tough to say!! I was looking into a bug a few months back, and
thought hmm.. maybe something is being called with the leds
but the leds are not there causing some thing to crap out..

so out of curiosity I remembered the dmi disable patch, and
semi somewhat pieced it together(just to see the results of disabling 
the leds).
unfortunately still hit the strange thing with wpa_supplicant and 
ath9k(the disconnect thing).

as for the above procedure(I can give that a try.. but it might
take a while...)

Justin P. Mattock



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH]wireless:ath9k Disable leds for Apple products.
  2010-05-26 21:34         ` Justin P. Mattock
@ 2010-05-26 21:42           ` Luis R. Rodriguez
  2010-05-26 22:28             ` Justin P. Mattock
  0 siblings, 1 reply; 12+ messages in thread
From: Luis R. Rodriguez @ 2010-05-26 21:42 UTC (permalink / raw)
  To: Justin P. Mattock; +Cc: John W. Linville, linux-wireless

On Wed, May 26, 2010 at 2:34 PM, Justin P. Mattock
<justinmattock@gmail.com> wrote:
> On 05/26/2010 02:05 PM, Luis R. Rodriguez wrote:
>>
>> On Wed, May 26, 2010 at 10:38 AM, Justin P. Mattock
>> <justinmattock@gmail.com>  wrote:
>>
>>>
>>> On 05/26/2010 10:18 AM, John W. Linville wrote:
>>>
>>>>
>>>> On Wed, May 26, 2010 at 10:14:16AM -0700, Justin P. Mattock wrote:
>>>>
>>>>
>>>>>
>>>>> Disable the leds on ath9k for Apple products, since
>>>>> there is no leds on any of there machines(or non that I can find!!).
>>>>>
>>>>>  Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>>>>
>>
>> Our team has confirmed Apple devices do not have LEDs so a change like
>> this is OK but this patch is wrong anyway. More details below.
>>
>>
>>>>>
>>>>>  void ath_init_leds(struct ath_softc *sc)
>>>>>  {
>>>>>        char *trigger;
>>>>>        int ret;
>>>>>
>>>>> +       /* Apple has no leds lights for their wireless.  */
>>>>> +       if (dmi_check_system(dmi_system_table)>    0)
>>>>> +               return -ENODEV;
>>>>> +       else
>>>>> +
>>>>>        if (AR_SREV_9287(sc->sc_ah))
>>>>>                sc->sc_ah->led_pin = ATH_LED_PIN_9287;
>>>>>        else
>>>>>
>>>>>
>>>>
>>>> Surely we don't need the 'else'.
>>>>
>>>> Does enabling the LEDs on these systems cause any problems?
>>>>
>>>> John
>>>>
>>>>
>>>
>>> I picked up the idea, from this patch:
>>> http://kerneltrap.org/mailarchive/linux-kernel/2010/1/20/4530535
>>> while looking into a bug for ath9k(thinking maybe leds
>>> are causing something, which want  the case)
>>>
>>> so Id have to say I don't think the leds cause issue's,
>>> if anything just a wasted symlink, call, or whatever
>>> in /sys/class/leds/*
>>>
>>
>> The patch is wrong anyway, ath_init_leds() is void and yet you return
>> a value. If this is really needed I'd also prefer if you define a bool
>> or something and use a ah->ignore_leds and set this to true if the DMI
>> stuff checks out for Apple up hw init. Then check for ignore_leds
>> prior to calling ath_init_leds() and also avoid it if set. Also check
>> for ingore_leds upon device cleanup and ignore the
>> acancel_delayed_work_sync(&sc->ath_led_blink_work) and
>> ath_deinit_leds() and skip that.
>>
>> But given that it is not needed it seems a lot of work for something
>> that is a non-issue.
>>
>>   Luis
>>
>>
>
> tough to say!! I was looking into a bug a few months back, and
> thought hmm.. maybe something is being called with the leds
> but the leds are not there causing some thing to crap out..
>
> so out of curiosity I remembered the dmi disable patch, and
> semi somewhat pieced it together(just to see the results of disabling the
> leds).
> unfortunately still hit the strange thing with wpa_supplicant and ath9k(the
> disconnect thing).
>
> as for the above procedure(I can give that a try.. but it might
> take a while...)

OK thanks for the details, best just ignore it unless you can prove it
fixes an issue.

 Luis

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH]wireless:ath9k Disable leds for Apple products.
  2010-05-26 21:42           ` Luis R. Rodriguez
@ 2010-05-26 22:28             ` Justin P. Mattock
  0 siblings, 0 replies; 12+ messages in thread
From: Justin P. Mattock @ 2010-05-26 22:28 UTC (permalink / raw)
  To: Luis R. Rodriguez; +Cc: John W. Linville, linux-wireless

On 05/26/2010 02:42 PM, Luis R. Rodriguez wrote:
> On Wed, May 26, 2010 at 2:34 PM, Justin P. Mattock
> <justinmattock@gmail.com>  wrote:
>    
>> On 05/26/2010 02:05 PM, Luis R. Rodriguez wrote:
>>      
>>> On Wed, May 26, 2010 at 10:38 AM, Justin P. Mattock
>>> <justinmattock@gmail.com>    wrote:
>>>
>>>        
>>>> On 05/26/2010 10:18 AM, John W. Linville wrote:
>>>>
>>>>          
>>>>> On Wed, May 26, 2010 at 10:14:16AM -0700, Justin P. Mattock wrote:
>>>>>
>>>>>
>>>>>            
>>>>>> Disable the leds on ath9k for Apple products, since
>>>>>> there is no leds on any of there machines(or non that I can find!!).
>>>>>>
>>>>>>   Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>>>>>>
>>>>>>              
>>> Our team has confirmed Apple devices do not have LEDs so a change like
>>> this is OK but this patch is wrong anyway. More details below.
>>>
>>>
>>>        
>>>>>>   void ath_init_leds(struct ath_softc *sc)
>>>>>>   {
>>>>>>         char *trigger;
>>>>>>         int ret;
>>>>>>
>>>>>> +       /* Apple has no leds lights for their wireless.  */
>>>>>> +       if (dmi_check_system(dmi_system_table)>      0)
>>>>>> +               return -ENODEV;
>>>>>> +       else
>>>>>> +
>>>>>>         if (AR_SREV_9287(sc->sc_ah))
>>>>>>                 sc->sc_ah->led_pin = ATH_LED_PIN_9287;
>>>>>>         else
>>>>>>
>>>>>>
>>>>>>              
>>>>> Surely we don't need the 'else'.
>>>>>
>>>>> Does enabling the LEDs on these systems cause any problems?
>>>>>
>>>>> John
>>>>>
>>>>>
>>>>>            
>>>> I picked up the idea, from this patch:
>>>> http://kerneltrap.org/mailarchive/linux-kernel/2010/1/20/4530535
>>>> while looking into a bug for ath9k(thinking maybe leds
>>>> are causing something, which want  the case)
>>>>
>>>> so Id have to say I don't think the leds cause issue's,
>>>> if anything just a wasted symlink, call, or whatever
>>>> in /sys/class/leds/*
>>>>
>>>>          
>>> The patch is wrong anyway, ath_init_leds() is void and yet you return
>>> a value. If this is really needed I'd also prefer if you define a bool
>>> or something and use a ah->ignore_leds and set this to true if the DMI
>>> stuff checks out for Apple up hw init. Then check for ignore_leds
>>> prior to calling ath_init_leds() and also avoid it if set. Also check
>>> for ingore_leds upon device cleanup and ignore the
>>> acancel_delayed_work_sync(&sc->ath_led_blink_work) and
>>> ath_deinit_leds() and skip that.
>>>
>>> But given that it is not needed it seems a lot of work for something
>>> that is a non-issue.
>>>
>>>    Luis
>>>
>>>
>>>        
>> tough to say!! I was looking into a bug a few months back, and
>> thought hmm.. maybe something is being called with the leds
>> but the leds are not there causing some thing to crap out..
>>
>> so out of curiosity I remembered the dmi disable patch, and
>> semi somewhat pieced it together(just to see the results of disabling the
>> leds).
>> unfortunately still hit the strange thing with wpa_supplicant and ath9k(the
>> disconnect thing).
>>
>> as for the above procedure(I can give that a try.. but it might
>> take a while...)
>>      
> OK thanks for the details, best just ignore it unless you can prove it
> fixes an issue.
>
>   Luis
>
>    
at this point the only thing I can think of(if anything),
would maybe a wakeup with powertop or something in
the area of power management(but haven't looked into it).

In any case Thanks for having a look at the ath_print
earlier..

cheers,

Justin P. Mattock

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2010-05-26 22:28 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-26 17:14 [PATCH]wireless:ath9k Fix ath_print in xmit.c Justin P. Mattock
2010-05-26 17:14 ` [PATCH]wireless:ath9k Disable leds for Apple products Justin P. Mattock
2010-05-26 17:18   ` John W. Linville
2010-05-26 17:38     ` Justin P. Mattock
2010-05-26 21:05       ` Luis R. Rodriguez
2010-05-26 21:34         ` Justin P. Mattock
2010-05-26 21:42           ` Luis R. Rodriguez
2010-05-26 22:28             ` Justin P. Mattock
2010-05-26 17:19   ` Luis R. Rodriguez
2010-05-26 17:16 ` [PATCH]wireless:ath9k Fix ath_print in xmit.c Luis R. Rodriguez
2010-05-26 17:28 ` Arnd Hannemann
2010-05-26 17:49   ` Justin P. Mattock

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).