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    [W:0.029 / U:0.192 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site