Hi Guenter,
>> @@ -217,14 +235,13 @@ static unsigned int fans_handled;
>> /* Indicates which temperature sensors set to use. */
>> static unsigned int applesmc_temperature_set;
>>
>> -static DEFINE_MUTEX(applesmc_lock);
>> -
>> /*
>> * Last index written to key_at_index sysfs file, and value to use for all other
>> * key_at_index_* sysfs files.
>> */
>> static unsigned int key_at_index;
>>
>> +
>
> unnecessary blank line
Ironically, I even tried to get rid out it two times. ;-)
>> +static int applesmc_get_entry_by_index(int index, struct applesmc_entry *entry)
>> +{
>
> One thing I don't understand about the whole caching scheme - why don't you just
> return (and use) a pointer to the cached entry ? Seems to me that would be much simpler.
> If you want to return error types, you could use ERR_PTR, PTR_ERR, and IS_ERR.
Yes, the return types are important. I can change this, it will reduce the patch
by some lines.
>> + struct applesmc_entry *cache = &smcreg.entry[index];
>> + __be32 be;
>> + int ret;
>> +
>> + if (cache->valid) {
>> + memcpy(entry, cache, sizeof(*entry));
>> + return 0;
>> }
>> - key[4] = 0;
>>
>> - return 0;
>> + mutex_lock(&smcreg.mutex);
Note to self: it is possible to arrive here with a valid cache. If pointers are
used, exit to avoid incoherent reads.
>> +
>> + be = cpu_to_be32(index);
>> + ret = read_smc(APPLESMC_GET_KEY_BY_INDEX_CMD, (u8 *)&be, cache->key, 4);
>> + if (ret)
>> + goto out;
>> + ret = read_smc(APPLESMC_GET_KEY_TYPE_CMD, cache->key, &cache->len, 6);
>
> This one is a bit odd. cache->len is an u8. You are reading 6 bytes into it.
> I assume this is supposed to fill both cache->len and cache->type in a single operation.
True.
> Not sure if this is a good idea. Seems to depend a lot on the assumption that
> fields are consecutive. Might be safer to read the data into a temporary
> 6 byte buffer and copy it into ->len and ->type afterwards.
Right, there could be alignment problems here. Thanks.
> If that is not acceptable, please add a comment describing what you are doing here
> and why.
It was just about the extra stack space. There will be a natural place for the
buffer once the pointer are used.
>> + if (ret)
>> + goto out;
>> +
>> + cache->type[4] = 0;
>
> Why read 6 bytes above if you overwrite the last byte anyway ?
Backwards compatibility reasons. The register type names have been used as four
byte strings since the beginning. I do not know is the last value codes
something else - I can make that clearer by reserving a byte in the struct instead.
>
>> + cache->valid = 1;
Continued note to self: a switch here is not handled properly.
>> + memcpy(entry, cache, sizeof(*entry));
>> +
>> +out:
>> + mutex_unlock(&smcreg.mutex);
>> + return ret;
>> }
>> +/*
>> + * applesmc_init_smcreg_try - Try to initialize register cache. Idempotent.
>> + */
>> +static int applesmc_init_smcreg_try(void)
>> +{
>> + struct applesmc_registers *s = &smcreg;
>> + int ret;
>> +
>> + if (s->init_complete)
>> + return 0;
>> +
>> + mutex_init(&s->mutex);
>> +
>> + ret = read_register_count(&s->key_count);
>> + if (ret)
>> + return ret;
>> +
>> + if (!s->entry)
>> + s->entry = kcalloc(s->key_count, sizeof(*s->entry), GFP_KERNEL);
>> + if (!s->entry)
>> + return -ENOMEM;
>> +
>> + s->init_complete = true;
>> +
>> + pr_info("key=%d\n", s->key_count);
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * applesmc_init_smcreg - Initialize register cache.
>> + *
>> + * Retries until initialization is successful, or the operation times out.
>> + *
>> + */
>> +static int applesmc_init_smcreg(void)
>> +{
>> + int ms, ret;
>> +
>> + for (ms = 0; ms < INIT_TIMEOUT_MSECS; ms += INIT_WAIT_MSECS) {
>> + ret = applesmc_init_smcreg_try();
>> + if (!ret)
>> + return 0;
>> + pr_warn("slow init, retrying\n");
>> + msleep(INIT_WAIT_MSECS);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void applesmc_destroy_smcreg(void)
>> +{
>> + kfree(smcreg.entry);
>> + smcreg.entry = NULL;
>
> Do you also have to reset init_complete ?
Yes!
Thanks,
Henrik
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
