lkml.org 
[lkml]   [2010]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 04/11] hwmon: applesmc: Introduce a register lookup table (rev2)

    >>
    >> mutex_destroy() is defined as a nop, so I guess the question is whether anything
    >> could be holding the lock when entering a second init. There are no sysfs files
    >> created at that point, so I would say no. The mutex could be put back with a
    >> static initializer, if this is not satisfactory. The real reason to move it to
    >> the smcreg struct was to force a rename of the mutex itself.
    >>
    >
    > Alternatively, you could move the mutex initialization to the beginning
    > of applesmc_init_smcreg() and make it
    > mutex_init(&smcreg.mutex);


    Looking at this again, it seems there are two other problems as well. Firstly,
    the cache memory is not freed after probe failure, my apologies. Secondly,
    execution continues after a probe failure, and the initialization is retried. I
    would like to push the latter problem to some other occasion, since the whole
    platform logic should be rewritten for the new interface, anyways.

    >>
    >> With the empirical failure rate, it is extremely unlikely to get more than a
    >> couple of failures in a row - information which in itself could be very useful.
    >
    > You would have alternative options, though, with less noise. For
    > example, something along the line of
    >
    > for (...) {
    > ...
    > if (!ret) {
    > if (ms)
    > pr_info("smcreg initialization took %d ms\n", ms);
    > return 0;
    > }
    > ...
    > }
    > pr_err("smcreg initialization failed\n");


    Looks nice, have applied, but without the last line; the probe failure report
    should be enough to deduce this.

    >>
    >> Changing the place of the mutex will ripple through all patches, so I will
    >> resend from this one onwards. I suppose you have more comments on the following
    >> patches?
    >
    > Maybe it won't be that bad if you initialize it as I suggested above.


    I tried several types of changes, and they all had some effect on later patches.
    The patch below comprise the resulting changes to patch 4. Hope you like. In
    addition, patch 5 and 7 needed one line of wiggling. I am resending all three.

    @@ -217,7 +217,9 @@ static struct applesmc_registers {
    unsigned int key_count; /* number of SMC registers */
    bool init_complete; /* true when fully initialized */
    struct applesmc_entry *cache; /* cached key entries */
    -} smcreg;
    +} smcreg = {
    + .mutex = __MUTEX_INITIALIZER(smcreg.mutex),
    +};

    static const int debug;
    static struct platform_device *pdev;
    @@ -581,8 +583,6 @@ static int applesmc_init_smcreg_try(void)
    if (s->init_complete)
    return 0;

    - mutex_init(&s->mutex);
    -
    ret = read_register_count(&s->key_count);
    if (ret)
    return ret;
    @@ -611,19 +611,25 @@ static int applesmc_init_smcreg(void)

    for (ms = 0; ms < INIT_TIMEOUT_MSECS; ms += INIT_WAIT_MSECS) {
    ret = applesmc_init_smcreg_try();
    - if (!ret)
    + if (!ret) {
    + if (ms)
    + pr_info("smcreg initialization took %d ms\n", ms);
    return 0;
    - pr_warn("slow init, retrying\n");
    + }
    msleep(INIT_WAIT_MSECS);
    }

    + kfree(smcreg.cache);
    + smcreg.cache = NULL;
    +
    return ret;
    }

    static void applesmc_destroy_smcreg(void)
    {
    kfree(smcreg.cache);
    - memset(&smcreg, 0, sizeof(smcreg));
    + smcreg.cache = NULL;
    + smcreg.init_complete = false;
    }

    /* Device model stuff */
    Thanks,
    Henrik


    \
     
     \ /
      Last update: 2010-11-10 11:59    [W:0.028 / U:59.868 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site