On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote:
> On Wed, 24 Jul 2024 22:45:23 +0200
> Pali Rohár <pali@xxxxxxxxxx> wrote:
>
> > On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote:
> > > Hello, the driver change looks good. I have just few minor comments for
> > > this change below.
> > >
> > > Anyway, if there is somebody on the list with Dell laptop with 2 or 3
> > > batteries, see below, it would be nice to check how such laptop would
> > > behave with this patch. It does not seem that patch should cause
> > > regression but always it is better to do testing if it is possible.
> > >
> > > On Tuesday 23 July 2024 22:05:02 Andres Salomon wrote:
> [...]
> > And because CLASS_TOKEN_WRITE is being repeated, what about defining
> > something like this?
> >
> > static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value)
> > {
> > dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value);
> > }
> >
> > Just an idea. Do you think that it could be useful in second patch?
> >
>
> Let me implement the other changes first and then take a look.
Ok.
> > > > +
> > > > +static int dell_battery_set_custom_charge_start(int start)
> > > > +{
> > > > + struct calling_interface_buffer buffer;
> > > > + int end;
> > > > +
> > > > + if (start < CHARGE_START_MIN)
> > > > + start = CHARGE_START_MIN;
> > > > + else if (start > CHARGE_START_MAX)
> > > > + start = CHARGE_START_MAX;
> > > > +
> > > > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END);
> > > > + /* a failed read is okay */
> > >
> > > Why is failed read okay? It sounds strange if we ignore firmware errors.
> > > I think that if reading the custom charge value is failing we should not
> > > continue and trying to set/change custom charge value.
>
> Because the check itself is simply a sanity check to ensure that the
> start value is not larger than then end value. I thought that if it fails,
> it's not a big deal; the sanity check originally wasn't even there.
>
> However, you're right that if we're failing to communicate w/ SMBIOS,
> that likely indicates a bigger problem and we probably shouldn't
> continue.
>
>
>
> > >
> > > Also if the returned value is above 100 (%), should be continue?
>
> Right, I'd forgotten that we shouldn't trust a BIOS. :)
For example in driver dell-smm-hwmon.c we have to validate what BIOS
returns because sometimes it returns bogus value. This is not some
hypothetical condition, but real problem confirmed by more users.
So checking what Dell BIOS returns is really good idea.
>
> > >
> > > > + if (end < 0)
> > > > + end = CHARGE_END_MAX;
> > > > + if ((end - start) < CHARGE_MIN_DIFF)
> > >
> > > nit: I'm not sure what is the correct coding style for kernel drivers
> > > but I thought that parenthesis around (end - start) are not being
> > > written.
> > >
> > > > + start = end - CHARGE_MIN_DIFF;
> > > > +
> > > > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE,
> > > > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_START, start);
> > > > +}
> > > > +
> > > > +static int dell_battery_set_custom_charge_end(int end)
> > > > +{
> > > > + struct calling_interface_buffer buffer;
> > > > + int start;
> > > > +
> > > > + if (end < CHARGE_END_MIN)
> > > > + end = CHARGE_END_MIN;
> > > > + else if (end > CHARGE_END_MAX)
> > > > + end = CHARGE_END_MAX;
> > > > +
> > > > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START);
> > > > + /* a failed read is okay */
> > > > + if (start < 0)
> > > > + start = CHARGE_START_MIN;
> > > > + if ((end - start) < CHARGE_MIN_DIFF)
> > > > + end = start + CHARGE_MIN_DIFF;
> > > > +
> > > > + return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE,
> > > > + SELECT_TOKEN_STD, BAT_CUSTOM_CHARGE_END, end);
> > > > +}
> > > > +
> > > > +static ssize_t charge_type_show(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + char *buf)
> > > > +{
> > > > + ssize_t count = 0;
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> > > > + bool active;
> > > > +
> > > > + if (!(battery_supported_modes & BIT(i)))
> > > > + continue;
> > > > +
> > > > + active = dell_battery_mode_is_active(battery_modes[i].token);
> > > > + count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ",
> > > > + battery_modes[i].label);
> > > > + }
> > > > +
> > > > + /* convert the last space to a newline */
> > > > + if (count > 0)
> > > > + count--;
> > > > + count += sysfs_emit_at(buf, count, "\n");
> > > > +
> > > > + return count;
> > > > +}
> > > > +
> > > > +static ssize_t charge_type_store(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + const char *buf, size_t size)
> > > > +{
> > > > + bool matched = false;
> > > > + int err, i;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> > >
> > > nit: Personally I would put the "if (!(battery_supported_modes & BIT(i)))"
> > > check here with continue, to have same pattern in _show and _store
> > > functions. And also if we want to support battery mode which will have
> > > different tokens on different machines (see below for possibility).
> > >
>
> Certainly for possible future cases; yeah, it's smarter to handle modes with
> conflicting names.
>
>
> > > > + if (sysfs_streq(battery_modes[i].label, buf)) {
> > > > + matched = true;
> > > > + break;
> > > > + }
> > > > + }
> > > > + if (!matched || !(battery_supported_modes & BIT(i)))
> > > > + return -EINVAL;
> > > > +
> > > > + err = dell_battery_set_mode(battery_modes[i].token);
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > + return size;
> > > > +}
> > > > +
> > > > +static ssize_t charge_control_start_threshold_show(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + char *buf)
> > > > +{
> > > > + int start;
> > > > +
> > > > + start = dell_battery_read(BAT_CUSTOM_CHARGE_START);
> > > > + if (start < 0)
> > > > + return start;
> > >
> > > IIRC the value is in percentage. So we should also check that returned
> > > value is not above 100 (and return some error in case).
> > >
> > > > +
> > > > + return sysfs_emit(buf, "%d\n", start);
> > > > +}
> > > > +
> > > > +static ssize_t charge_control_start_threshold_store(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + const char *buf, size_t size)
> > > > +{
> > > > + int ret, start;
> > > > +
> > > > + ret = kstrtoint(buf, 10, &start);
> > > > + if (ret)
> > > > + return ret;
> > >
> > > I think that there should be some sanity validation. If format is
> > > percentage then we should not accept from userspace value outside of
> > > [0, 100] range.
> > >
>
> dell_battery_set_custom_charge_start() does validate that the userspace
> values are correct. It will actually round up or down rather than
> returning -EINVAL. So if userspace sends the "150" for start charge,
> it will round it down to 95% (CHARGE_START_MAX) and then round it down
> further to the end charge value minus 5% (CHARGE_MIN_DIFF). This behavior
> matches what's documented in Documentation/ABI/testing/sysfs-class-power
Rounding value to the nearest supported value is correct (e.g. 99% to 95%)
and documented in that file.
But it is really correct to accept value -1% or 1000000%?
Now I have just opened the first driver platform/x86/asus-wmi.c which
implements this interface and it returns -EINVAL for value < 0 || value > 100.
>
> > > > +
> > > > + ret = dell_battery_set_custom_charge_start(start);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + return size;
> > > > +}
> > > > +
> > > > +static ssize_t charge_control_end_threshold_show(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + char *buf)
> > > > +{
> > > > + int end;
> > > > +
> > > > + end = dell_battery_read(BAT_CUSTOM_CHARGE_END);
> > > > + if (end < 0)
> > > > + return end;
> > > > +
> > > > + return sysfs_emit(buf, "%d\n", end);
> > > > +}
> > > > +
> > > > +static ssize_t charge_control_end_threshold_store(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + const char *buf, size_t size)
> > > > +{
> > > > + int ret, end;
> > > > +
> > > > + ret = kstrtouint(buf, 10, &end);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = dell_battery_set_custom_charge_end(end);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + return size;
> > > > +}
> > > > +
> > > > +static DEVICE_ATTR_RW(charge_control_start_threshold);
> > > > +static DEVICE_ATTR_RW(charge_control_end_threshold);
> > > > +static DEVICE_ATTR_RW(charge_type);
> > > > +
> > > > +static struct attribute *dell_battery_attrs[] = {
> > > > + &dev_attr_charge_control_start_threshold.attr,
> > > > + &dev_attr_charge_control_end_threshold.attr,
> > > > + &dev_attr_charge_type.attr,
> > > > + NULL,
> > > > +};
> > > > +ATTRIBUTE_GROUPS(dell_battery);
> > > > +
> > > > +static int dell_battery_add(struct power_supply *battery,
> > > > + struct acpi_battery_hook *hook)
> > > > +{
> > > > + return device_add_groups(&battery->dev, dell_battery_groups);
> > > > +}
> > > > +
> > > > +static int dell_battery_remove(struct power_supply *battery,
> > > > + struct acpi_battery_hook *hook)
> > > > +{
> > > > + device_remove_groups(&battery->dev, dell_battery_groups);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static struct acpi_battery_hook dell_battery_hook = {
> > > > + .add_battery = dell_battery_add,
> > > > + .remove_battery = dell_battery_remove,
> > > > + .name = "Dell Primary Battery Extension",
> > > > +};
> > > > +
> > > > +static u32 __init battery_get_supported_modes(void)
> > > > +{
> > > > + u32 modes = 0;
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> > > > + if (dell_smbios_find_token(battery_modes[i].token))
> > > > + modes |= BIT(i);
> > > > + }
> > > > +
> > > > + return modes;
> > > > +}
> > > > +
> > > > +static void __init dell_battery_init(struct device *dev)
> > > > +{
> > > > + battery_supported_modes = battery_get_supported_modes();
> > > > +
> > > > + if (battery_supported_modes != 0)
> > > > + battery_hook_register(&dell_battery_hook);
> > >
> > > Anyway, how is this battery_hook_register() suppose to work on systems
> > > with multiple batteries? The provided API is only for the primary
> > > battery, but on older Dell laptop it was possible to connect up to
> > > 3 batteries. Would not this case some issues?
>
> This interface is _only_ for controlling charging of the primary battery.
> In theory, it should hopefully ignore any other batteries, which would
> need to have their settings changed in the BIOS or with a special tool or
> whatever.
That is OK. But where it is specified that the hook is being registered
only for the primary battery? Because from the usage it looks like that
the hook is applied either for all batteries present in the system or
for some one arbitrary chosen battery.
> However, I'm basing that assumption on the SMBIOS interface. These tokens
> are marked "Primary Battery". There are separate tokens marked "Battery
> Slice", which from my understanding was an older type of battery that
