2.6.27-rc9 + patches to enable WPA from wireless-testing (picked manually):
+ orinoco-add-essid-specific-sca
+ orinoco-update-scan-translatio
+ orinoco-specify-all-three-para
+ orinoco-move-export_symbol-dec
+ orinoco-add-function-to-execut
+ orinoco-move-firmware-download
+ orinoco-make-firmware-download
+ orinoco-extend-hermes_dld-rout
+ orinoco-invoke-firmware-downlo
+ orinoco-fix-transmit-for-agere
+ orinoco-address-checkpatch-typ
+ orinoco-use-extended-agere-sca
+ orinoco-don-t-use-boolean-para
+ orinoco-split-wevent-work-thre
+ orinoco-use-a-macro-to-define-
+ orinoco-add-we-18-ioctls-for-w
+ orinoco-send-association-event
+ orinoco-process-bulk-of-receiv
+ orinoco-add-mic-on-tx-and-chec
+ orinoco-fix-compile-warnings
Firmware taken from orinoco-devel list and placed in /lib/firmware:
4bc30fed682d83ce0bbeb32af6d218a3 /lib/firmware/agere_sta_fw.bin
After loading orinoco and "inserting" adapter I get either BUG with
endless loop (attached) or kernel panic on NULL pointer dereference
(was not able capture). Adapter works fine with wlags49 driver.
{pts/1}% pccardctl ident
Socket 0:
product info: "TOSHIBA", "Wireless LAN Card", "Version 01.01", ""
manfid: 0x0156, 0x0002
function: 6 (network)
Socket 1:
no product info available
Socket 2:
no product info available
Any idea? Anything else in wireless-testing that is missing in vanilla?
On Thu, Oct 09, 2008 at 05:22:55PM +0400, Andrey Borzenkov wrote:
> After loading orinoco and "inserting" adapter I get either BUG with
> endless loop (attached) or kernel panic on NULL pointer dereference
> (was not able capture). Adapter works fine with wlags49 driver.
It looks like you've fallen off the bottom of the kernel stack. Do you
have 4k stacks enabled in your config?
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Thursday 09 October 2008, Matthew Wilcox wrote:
>
> On Thu, Oct 09, 2008 at 05:22:55PM +0400, Andrey Borzenkov wrote:
> > After loading orinoco and "inserting" adapter I get either BUG with
> > endless loop (attached) or kernel panic on NULL pointer dereference
> > (was not able capture). Adapter works fine with wlags49 driver.
>
> It looks like you've fallen off the bottom of the kernel stack. Do you
> have 4k stacks enabled in your config?
>
You are right. Using 8K stacks load and runs fine. Hmm ... not nice
from it :p
On Thu, 2008-10-09 at 19:59 +0400, Andrey Borzenkov wrote:
> On Thursday 09 October 2008, Matthew Wilcox wrote:
> >
> > On Thu, Oct 09, 2008 at 05:22:55PM +0400, Andrey Borzenkov wrote:
> > > After loading orinoco and "inserting" adapter I get either BUG with
> > > endless loop (attached) or kernel panic on NULL pointer dereference
> > > (was not able capture). Adapter works fine with wlags49 driver.
> >
> > It looks like you've fallen off the bottom of the kernel stack. Do you
> > have 4k stacks enabled in your config?
> >
>
> You are right. Using 8K stacks load and runs fine. Hmm ... not nice
> from it :p
== driver bug; the driver should not require large stacks and this
should get fixed.
Dan
On Thursday 09 October 2008, Dan Williams wrote:
>
> On Thu, 2008-10-09 at 19:59 +0400, Andrey Borzenkov wrote:
> > On Thursday 09 October 2008, Matthew Wilcox wrote:
> > >
> > > On Thu, Oct 09, 2008 at 05:22:55PM +0400, Andrey Borzenkov wrote:
> > > > After loading orinoco and "inserting" adapter I get either BUG with
> > > > endless loop (attached) or kernel panic on NULL pointer dereference
> > > > (was not able capture). Adapter works fine with wlags49 driver.
> > >
> > > It looks like you've fallen off the bottom of the kernel stack. Do you
> > > have 4k stacks enabled in your config?
> > >
> >
> > You are right. Using 8K stacks load and runs fine. Hmm ... not nice
> > from it :p
>
> == driver bug; the driver should not require large stacks and this
> should get fixed.
>
I'll check for obvious places, still ...
/* Download either STA or AP firmware into the card. */
static int
orinoco_dl_firmware(struct orinoco_private *priv,
const struct fw_info *fw,
int ap)
{
/* Plug Data Area (PDA) */
__le16 pda[512] = { 0 };
allocating 1K on stack is not nice; but what has eaten 3K more?
Is there any instrumentation to analyze stack usage?
On Thursday 09 October 2008, Dan Williams wrote:
>
> On Thu, 2008-10-09 at 19:59 +0400, Andrey Borzenkov wrote:
> > On Thursday 09 October 2008, Matthew Wilcox wrote:
> > >
> > > On Thu, Oct 09, 2008 at 05:22:55PM +0400, Andrey Borzenkov wrote:
> > > > After loading orinoco and "inserting" adapter I get either BUG with
> > > > endless loop (attached) or kernel panic on NULL pointer dereference
> > > > (was not able capture). Adapter works fine with wlags49 driver.
> > >
> > > It looks like you've fallen off the bottom of the kernel stack. Do you
> > > have 4k stacks enabled in your config?
> > >
> >
> > You are right. Using 8K stacks load and runs fine. Hmm ... not nice
> > from it :p
>
> == driver bug; the driver should not require large stacks and this
> should get fixed.
>
The attached patch fixes 4K stack for me. I have not tested spectrum case.
-andrey
On Thu, 2008-10-09 at 23:21 +0400, Andrey Borzenkov wrote:
> On Thursday 09 October 2008, Dan Williams wrote:
> >
> > On Thu, 2008-10-09 at 19:59 +0400, Andrey Borzenkov wrote:
> > > On Thursday 09 October 2008, Matthew Wilcox wrote:
> > > >
> > > > On Thu, Oct 09, 2008 at 05:22:55PM +0400, Andrey Borzenkov wrote:
> > > > > After loading orinoco and "inserting" adapter I get either BUG with
> > > > > endless loop (attached) or kernel panic on NULL pointer dereference
> > > > > (was not able capture). Adapter works fine with wlags49 driver.
> > > >
> > > > It looks like you've fallen off the bottom of the kernel stack. Do you
> > > > have 4k stacks enabled in your config?
> > > >
> > >
> > > You are right. Using 8K stacks load and runs fine. Hmm ... not nice
> > > from it :p
> >
> > == driver bug; the driver should not require large stacks and this
> > should get fixed.
> >
>
> The attached patch fixes 4K stack for me. I have not tested spectrum case.
Looks pretty good, although if nobody can test the Symbol case (all I
have is Intersil firmware) maybe you should not use priv->pda_size but
#define SYMBOL_PDA_SIZE 256 and use that for the hermes_read_pda()
length just to ensure the patched code is functionally the same as
before the patch.
Dan
Andrey Borzenkov wrote:
> The attached patch fixes 4K stack for me. I have not tested spectrum case.
Thanks for identifying the problem. The Agere case looks good - a few suggestions for the Symbol case though:
> diff --git a/drivers/net/wireless/orinoco.c b/drivers/net/wireless/orinoco.c
> --- a/drivers/net/wireless/orinoco.c
> +++ b/drivers/net/wireless/orinoco.c
> @@ -549,12 +556,16 @@ symbol_dl_image(struct orinoco_private *priv, const struct fw_info *fw,
> int secondary)
> {
> hermes_t *hw = &priv->hw;
> - int ret;
> + int ret = 0;
> const unsigned char *ptr;
> const unsigned char *first_block;
>
> /* Plug Data Area (PDA) */
> - __le16 pda[256];
> + __le16 *pda;
Please initialise pda to NULL here...
> + pda = kzalloc(fw->pda_size, GFP_KERNEL);
> + if (!pda)
> + return -ENOMEM;
And move the allocation to
> /* Binary block begins after the 0x1A marker */
> ptr = image;
> @@ -563,22 +574,22 @@ symbol_dl_image(struct orinoco_private *priv, const struct fw_info *fw,
>
> /* Read the PDA from EEPROM */
> if (secondary) {
... here.
> - ret = hermes_read_pda(hw, pda, fw->pda_addr, sizeof(pda), 1);
> + ret = hermes_read_pda(hw, pda, fw->pda_addr, fw->pda_size, 1);
> if (ret)
> - return ret;
> + goto free;
> }
>
> /* Stop the firmware, so that it can be safely rewritten */
> if (priv->stop_fw) {
> ret = priv->stop_fw(priv, 1);
> if (ret)
> - return ret;
> + goto free;
> }
>
> /* Program the adapter with new firmware */
> ret = hermes_program(hw, first_block, end);
> if (ret)
> - return ret;
> + goto free;
>
> /* Write the PDA to the adapter */
> if (secondary) {
> @@ -586,28 +597,31 @@ symbol_dl_image(struct orinoco_private *priv, const struct fw_info *fw,
> ptr = first_block + len;
> ret = hermes_apply_pda(hw, ptr, pda);
> if (ret)
> - return ret;
> + goto free;
Then kfree(pda) here. We're done with it now.
> }
>
> /* Run the firmware */
> if (priv->stop_fw) {
> ret = priv->stop_fw(priv, 0);
> if (ret)
> - return ret;
> + goto free;
So this isn't needed.
> }
>
> /* Reset hermes chip and make sure it responds */
> ret = hermes_init(hw);
>
> /* hermes_reset() should return 0 with the secondary firmware */
> - if (secondary && ret != 0)
> - return -ENODEV;
> + if (secondary && ret != 0) {
> + ret = -ENODEV;
> + goto free;
> + }
nor this.
>
> /* And this should work with any firmware */
> if (!hermes_present(hw))
> - return -ENODEV;
> + ret = -ENODEV;
>
> - return 0;
or these.
> +free:
But you absolutely have to kfree(pda) here. I would prefer the label be something like 'abort' (what we are doing), rather than 'free' - but there's merit in keeping with what you have called in in orinoco_dl_firmware which already has an 'abort' label.
> + return ret;
> }
The net effect of the above suggestion is that we won't allocate memory when programming the primary firmware (which is just before we do the secondary).
Dan Williams wrote:
> maybe you should not use priv->pda_size but
> #define SYMBOL_PDA_SIZE 256 and use that for the hermes_read_pda()
> length just to ensure the patched code is functionally the same as
> before the patch.
Using fw->pda_size should be fine. The value comes from a const static, set to 0x100 for Symbol.
Regards,
Dave.
On Friday 10 October 2008, Dave wrote:
> Thanks for identifying the problem. The Agere case looks good - a few suggestions for the Symbol case though:
Updated patch attached
>
> But you absolutely have to kfree(pda) here.
Yes, I forgot; sorry.
> Dan Williams wrote:
> > maybe you should not use priv->pda_size but
> > #define SYMBOL_PDA_SIZE 256 and use that for the hermes_read_pda()
> > length just to ensure the patched code is functionally the same as
> > before the patch.
>
> Using fw->pda_size should be fine. The value comes from a const static, set to 0x100 for Symbol.
>
Actually it allocated 512 byte before. Should it probably be set to 0x200?
Andrey Borzenkov wrote:
> On Friday 10 October 2008, Dave wrote:
>> Thanks for identifying the problem. The Agere case looks good - a few suggestions for the Symbol case though:
>
> Updated patch attached
>
>>
>> But you absolutely have to kfree(pda) here.
>
> Yes, I forgot; sorry.
>
>> Dan Williams wrote:
>>> maybe you should not use priv->pda_size but
>>> #define SYMBOL_PDA_SIZE 256 and use that for the hermes_read_pda()
>>> length just to ensure the patched code is functionally the same as
>>> before the patch.
>> Using fw->pda_size should be fine. The value comes from a const static, set to 0x100 for Symbol.
>>
>
> Actually it allocated 512 byte before. Should it probably be set to 0x200?
You're right. pda_size is supposed to be the maximum size of the PDA (in bytes) that we expect with the firmware. It should be set to 0x200.
I've scanned the rest of the patch and it looks good.
Dave.
On Friday 10 October 2008, Dave wrote:
>
> You're right. pda_size is supposed to be the maximum size of the PDA (in bytes) that we expect with the firmware. It should be set to 0x200.
>
Do you want me to send a patch?
Andrey Borzenkov wrote:
> On Friday 10 October 2008, Dave wrote:
>> You're right. pda_size is supposed to be the maximum size of the PDA (in bytes) that we expect with the firmware. It should be set to 0x200.
>>
>
> Do you want me to send a patch?
Could you fix it as part of the patch you submitted?
Regards,
Dave.
On Friday 10 October 2008, Dave wrote:
>
> Andrey Borzenkov wrote:
> > On Friday 10 October 2008, Dave wrote:
> >> You're right. pda_size is supposed to be the maximum size of the PDA (in bytes) that we expect with the firmware. It should be set to 0x200.
> >>
> >
> > Do you want me to send a patch?
>
> Could you fix it as part of the patch you submitted?
>
Attached. As other entries use decimal I thought it is more logical
to use 512 as well.
Andrey Borzenkov wrote:
> On Friday 10 October 2008, Dave wrote:
>> Andrey Borzenkov wrote:
>>> On Friday 10 October 2008, Dave wrote:
>>>> You're right. pda_size is supposed to be the maximum size of the PDA (in bytes) that we expect with the firmware. It should be set to 0x200.
>>>>
>>> Do you want me to send a patch?
>> Could you fix it as part of the patch you submitted?
>>
>
> Attached. As other entries use decimal I thought it is more logical
> to use 512 as well.
ACK.
Hi,
On Fri, Oct 10, 2008 at 09:26:30PM +0400, Andrey Borzenkov wrote:
> On Friday 10 October 2008, Dave wrote:
> >
> > Andrey Borzenkov wrote:
> > > On Friday 10 October 2008, Dave wrote:
> > >> You're right. pda_size is supposed to be the maximum size of the PDA (in bytes) that we expect with the firmware. It should be set to 0x200.
> > >>
> > >
> > > Do you want me to send a patch?
> >
> > Could you fix it as part of the patch you submitted?
> >
>
> Attached. As other entries use decimal I thought it is more logical
> to use 512 as well.
> Subject: [PATCH] orinoco: reduce stack usage in firmware download path
> From: Andrey Borzenkov < [email protected]>
>
> orinoco_dl_firmware and symbol_dl_mage allocate large local
> variables (1K); at least orinoco fails with panic or hung
> kernel if 4K stacks is enabled.
>
> Allocate large buffers dynamically at run time.
>
> Tested-By: Andrey Borzenkov <[email protected]> for Agere case
>
> Signed-off-by: Andrey Borzenkov < [email protected]>
Acked-by: Dominik Brodowski <[email protected]>
I assmue this is to be merged via wireless and net, and not via pcmcia?
Best,
Dominik