lkml.org 
[lkml]   [2008]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: rc9 + orinoco WPA patchset: BUG: scheduling while atomic loading firmware with PCMCIA adapter
From
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.


\
 
 \ /
  Last update: 2008-10-10 00:49    [from the cache]
©2003-2011 Jasper Spaans