On Saturday 20 July 2024 05:24:19 Andres Salomon wrote: > Thanks for the quick feedback! Responses below. > > On Sat, 20 Jul 2024 10:40:19 +0200 > Pali Rohár <pali@xxxxxxxxxx> wrote: > > > Hello, > > > > I looked at your patch. I wrote some comments below. The main issue is > > how to correctly interpret read token values. > > > [...] > > > > > dell_send_request() returns negative value on error. As the read value > > seems to be always non-negative number, you can change API of the > > dell_battery_read_req() function to have read value in the return value > > (instead of in *val pointer). E.g. > > > > static int dell_battery_read_req(const int type) > > { > > ... > > err = dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD); > > if (err) > > return err; > > > > return buffer.output[1]; > > } > > > > Good call, I'll change that. > > > > > + > > > +static int dell_battery_write_req(const int type, int val) > > > +{ > > > + struct calling_interface_buffer buffer; > > > + struct calling_interface_token *token; > > > + > > > + token = dell_smbios_find_token(type); > > > + if (!token) > > > + return -ENODEV; > > > + > > > + dell_fill_request(&buffer, token->location, val, 0, 0); > > > + return dell_send_request(&buffer, > > > + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD); > > > +} > > > + > > > +/* The rules: the minimum start charging value is 50%. The maximum > > > + * start charging value is 95%. The minimum end charging value is > > > + * 55%. The maximum end charging value is 100%. And finally, there > > > + * has to be at least a 5% difference between start & end values. > > > + */ > > > +#define CHARGE_START_MIN 50 > > > +#define CHARGE_START_MAX 95 > > > +#define CHARGE_END_MIN 55 > > > +#define CHARGE_END_MAX 100 > > > +#define CHARGE_MIN_DIFF 5 > > > + > > > +static int dell_battery_custom_set(const int type, int val) > > > +{ > > > + if (type == BAT_CUSTOM_CHARGE_START) { > > > + int end = CHARGE_END_MAX; > > > + > > > + if (val < CHARGE_START_MIN) > > > + val = CHARGE_START_MIN; > > > + else if (val > CHARGE_START_MAX) > > > + val = CHARGE_START_MAX; > > > + > > > + dell_battery_read_req(BAT_CUSTOM_CHARGE_END, &end); > > > > Missing check for failure of dell_battery_read_req. > > This is intentional; it's just a sanity check, we don't need to bail > if we hit a failure. I'll change the code to make that explicit > though, as it's not currently clear. > > > > > > > > + if ((end - val) < CHARGE_MIN_DIFF) > > > + val = end - CHARGE_MIN_DIFF; > > > + } else if (type == BAT_CUSTOM_CHARGE_END) { > > > + int start = CHARGE_START_MIN; > > > + > > > + if (val < CHARGE_END_MIN) > > > + val = CHARGE_END_MIN; > > > + else if (val > CHARGE_END_MAX) > > > + val = CHARGE_END_MAX; > > > + > > > + dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); > > > > Missing check for failure of dell_battery_read_req. > > > > Ditto. > > > > > + if ((val - start) < CHARGE_MIN_DIFF) > > > + val = start + CHARGE_MIN_DIFF; > > > + } > > > + > > > + return dell_battery_write_req(type, val); > > > +} > > > + > > > +static int battery_charging_mode_set(enum battery_charging_mode mode) > > > +{ > > > + int err; > > > + > > > + switch (mode) { > > > + case DELL_BAT_MODE_STANDARD: > > > + err = dell_battery_write_req(BAT_STANDARD_MODE_TOKEN, mode); > > > + break; > > > + case DELL_BAT_MODE_EXPRESS: > > > + err = dell_battery_write_req(BAT_EXPRESS_MODE_TOKEN, mode); > > > + break; > > > + case DELL_BAT_MODE_PRIMARILY_AC: > > > + err = dell_battery_write_req(BAT_PRI_AC_MODE_TOKEN, mode); > > > + break; > > > + case DELL_BAT_MODE_ADAPTIVE: > > > + err = dell_battery_write_req(BAT_ADAPTIVE_MODE_TOKEN, mode); > > > + break; > > > + case DELL_BAT_MODE_CUSTOM: > > > + err = dell_battery_write_req(BAT_CUSTOM_MODE_TOKEN, mode); > > > + break; > > > + default: > > > + err = -EINVAL; > > > + } > > > + > > > + return err; > > > +} > > > > You can make whole function smaller by avoiding err variable: > > > > static int battery_charging_mode_set(enum battery_charging_mode mode) > > { > > switch (mode) { > > case DELL_BAT_MODE_STANDARD: > > return dell_battery_write_req(BAT_STANDARD_MODE_TOKEN, mode); > > case DELL_BAT_MODE_EXPRESS: > > return dell_battery_write_req(BAT_EXPRESS_MODE_TOKEN, mode); > > case DELL_BAT_MODE_PRIMARILY_AC: > > return dell_battery_write_req(BAT_PRI_AC_MODE_TOKEN, mode); > > case DELL_BAT_MODE_ADAPTIVE: > > return dell_battery_write_req(BAT_ADAPTIVE_MODE_TOKEN, mode); > > case DELL_BAT_MODE_CUSTOM: > > return dell_battery_write_req(BAT_CUSTOM_MODE_TOKEN, mode); > > default: > > return -EINVAL; > > } > > } > > > > Okay, I'll change it. > > > > > + > > > +static ssize_t charge_type_show(struct device *dev, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + enum battery_charging_mode mode; > > > + ssize_t count = 0; > > > + > > > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > > > + if (battery_state[mode]) { > > > + count += sysfs_emit_at(buf, count, > > > + mode == bat_chg_current ? "[%s] " : "%s ", > > > + battery_state[mode]); > > > + } > > > + } > > > + > > > + /* convert the last space to a newline */ > > > + count--; > > > + count += sysfs_emit_at(buf, count, "\n"); > > > > Here is missing protection in the case when number of valid modes is > > zero, so count is 0 and buf was untouched. > > > > This will never be zero (based on the hardcoded value of DELL_BAT_MODE_MAX), Now I see. You are iterating over members of constant array battery_state[]. I overlooked it and I thought that this iteration over mode values. What about writing the for- conditions to be visible that you are iterating over the array? E.g. size_t i; ... for (i = 0; i < ARRAY_SIZE(battery_state); i++) { if (!battery_state[i]) continue; count += sysfs_emit_at(buf, count, i == bat_chg_current ? "[%s] " : "%s ", battery_state[i]); } ... This has an advantage that you do not depend on DELL_BAT_MODE_MAX value, compiler will calculate upper bound automatically. Or another way. You can define array of tokens, like it is done for keyboard backlight. See how the array kbd_tokens[] is used. With this approach you can completely get rid of the DELL_BAT_MODE_MAX. > but perhaps a static_assert or BUILD_BUG_ON to verify that the number of > modes > 0? I think it is not needed. > > > > + > > > + return count; > > > +} > > > + > > > +static ssize_t charge_type_store(struct device *dev, > > > + struct device_attribute *attr, > > > + const char *buf, size_t size) > > > +{ > > > + enum battery_charging_mode mode; > > > + const char *label; > > > + int ret = -EINVAL; > > > + > > > + for (mode = DELL_BAT_MODE_STANDARD; mode < DELL_BAT_MODE_MAX; mode++) { > > > + label = battery_state[mode]; > > > + if (label && sysfs_streq(label, buf)) > > > + break; > > > + } > > > + > > > + if (mode > DELL_BAT_MODE_NONE && mode < DELL_BAT_MODE_MAX) { > > > + ret = battery_charging_mode_set(mode); > > > + if (!ret) { > > > + bat_chg_current = mode; > > > + ret = size; > > > + } > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +static ssize_t charge_control_start_threshold_show(struct device *dev, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + int ret, start; > > > + > > > + ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); > > > + if (!ret) > > > + ret = sysfs_emit(buf, "%d\n", start); > > > + > > > + return ret; > > > +} > > > > This function and also following 3 functions have unusual error > > handling. Normally error handling is done by early return, as: > > > > ret = func1(); > > if (ret) > > return ret; > > > > ret = func2(); > > if (ret) > > return ret; > > > > return 0; > > > > You can change it something like: > > > > { > > int ret, start; > > > > ret = dell_battery_read_req(BAT_CUSTOM_CHARGE_START, &start); > > if (ret) > > return ret; > > > > return sysfs_emit(buf, "%d\n", start); > > } > > > > Okay. > > > > > +static ssize_t charge_control_start_threshold_store(struct device *dev, > > > + struct device_attribute *attr, > > > + const char *buf, size_t size) > > > +{ > [...] > > > > + > > > +static void __init dell_battery_init(struct device *dev) > > > +{ > > > + enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE; > > > + > > > + dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) ¤t_mode); > > > + if (current_mode != DELL_BAT_MODE_NONE) { > > > > I quite do not understand how is this code suppose to work. > > > > Why is there mix of custom kernel enum battery_charging_mode and return > > value from Dell's API? > > This is from the original patch from Dell; tbh, I'm not sure. It does > work, though. That is, current_mode ends up holding the correct value > based on what was previously set, even if the charging mode is set from > the BIOS. > > I just scanned through the libsmbios code to see what it's doing, and > it appears to loop through every charging mode to check if its active. > I'm not really sure that makes much more sense, so I'll try some more > tests. Keyboard backlight code (kbd_get_first_active_token_bit) is doing also this type scan. If I remember correctly, for every keyboard backlight token we just know the boolean value - if the token is set or not. It would really nice to see what (raw) value is returned by the dell_battery_read_req(token) function for every battery token and for every initial state. Because it is really suspicious if dell_battery_read_req() would return value of the enum battery_charging_mode (as this is kernel enum). Also another important thing. In past it was possible to buy from Dell special batteries with long warranty (3+ years). I'm not sure if these batteries are still available for end-user customers. With this type of battery, it was not possible to change charging mode to ExpressCharge (bios option was fade-out). I do not have such battery anymore, but I would expect that the firmware disabled BAT_EXPRESS_MODE_TOKEN as mark it as unavailable. I think that we should scan list of available tokens, like it is done for keyboard backlight in kbd_init_tokens(). And export via sysfs only those battery modes for which there is available token. > > > > > My feeling is that dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN) checks > > if the token BAT_CUSTOM_MODE_TOKEN is set or not. > > > > Could you please check what is stored in every BAT_*_MODE_TOKEN token at > > this init stage? > > > > I think it should work similarly, like keyboard backlight tokens as > > implemented in functions: kbd_set_token_bit, kbd_get_token_bit, > > kbd_get_first_active_token_bit. > > > > > + bat_chg_current = current_mode; > > > + battery_hook_register(&dell_battery_hook); > > > + } > > > +} > > > + > [...] > > > > #define GLOBAL_MUTE_ENABLE 0x058C > > > #define GLOBAL_MUTE_DISABLE 0x058D > > > > > > +enum battery_charging_mode { > > > + DELL_BAT_MODE_NONE = 0, > > > + DELL_BAT_MODE_STANDARD, > > > + DELL_BAT_MODE_EXPRESS, > > > + DELL_BAT_MODE_PRIMARILY_AC, > > > + DELL_BAT_MODE_ADAPTIVE, > > > + DELL_BAT_MODE_CUSTOM, > > > + DELL_BAT_MODE_MAX, > > > +}; > > > + > > > > I think that this is just an internal driver enum, not Dell API. So this > > enum should be in the dell-laptop.c file. > > > > Agreed, I'll change it. > > > > > -- > I'm available for contract & employment work, see: > https://spindle.queued.net/~dilinger/resume-tech.pdf