| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 4 Aug 2020 06:19:23 +0000
From: "Yuan, Perry" <Perry.Yuan@...l.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
CC: Sebastian Reichel <sre@...nel.org>,
Matthew Garrett <mjg59@...f.ucam.org>,
Pali Rohár <pali@...nel.org>,
Darren Hart <dvhart@...radead.org>,
Andy Shevchenko <andy@...radead.org>,
"Limonciello, Mario" <Mario.Limonciello@...l.com>,
Linux PM <linux-pm@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Platform Driver <platform-driver-x86@...r.kernel.org>
Subject: RE: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds
and charging mode switch.
> From: Andy Shevchenko <andy.shevchenko@...il.com>
> Sent: Wednesday, July 29, 2020 3:32 PM
> To: Yuan, Perry
> Cc: Sebastian Reichel; Matthew Garrett; Pali Rohár; Darren Hart; Andy
> Shevchenko; Limonciello, Mario; Linux PM; Linux Kernel Mailing List; Platform
> Driver
> Subject: Re: [PATCH] platform/x86:dell-laptop:Add battery charging thresholds
> and charging mode switch.
>
>
> [EXTERNAL EMAIL]
>
> On Wed, Jul 29, 2020 at 9:54 AM Perry Yuan <Perry.Yuan@...l.com> wrote:
> >
> > From: perry_yuan <perry_yuan@...l.com>
>
> Fix your name, please. Or do you have it spelled in the official documents like
> above?
>
> > The patch control battery charging thresholds when system is under
> > custom charging mode through smbios API and driver`s sys attributes.It
> > also set the percentage bounds for custom charge.
> > Start value must lie in the range [50, 95],End value must lie in the
> > range [55, 100],END must be at least (START + 5).
> >
> > The patch also add the battery charging modes switch support.User can
> > switch the battery charging mode through the new sysfs entry.
>
> The commit message needs rewording. I would recommend reading [1] and [2].
>
> [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> (section 2)
> [2]: https://chris.beams.io/posts/git-commit/
>
> > Primary battery charging modes valid choices are:
> > ['primarily_ac', 'adaptive', 'custom', 'standard', 'express']
>
> This and documentation are different (at least in terms of case).
>
> Looks like this is something that should be agreed upon on the format and gets
> supported by Power Supply framework in the first place,
>
> > Documentation/ABI/testing/sysfs-class-power | 23 ++
>
> In any case it should be a separate patch. It has nothing to do with Dell, on
> contrary it's a certain device which relies on generic attributes.
>
> ...
>
> > #include <linux/debugfs.h>
> > #include <linux/seq_file.h>
>
> > #include <acpi/video.h>
> > +#include <acpi/battery.h>
>
> Keep it ordered.
>
> > +#include <linux/string.h>
>
> And this is a generic one, should be in generic headers block.
>
> > #include "dell-rbtn.h"
> > #include "dell-smbios.h"
>
> ...
>
> > +static int dell_battery_get(int *start, int *end) {
> > + struct calling_interface_buffer buffer;
> > + struct calling_interface_token *token;
> > + int ret;
> > +
>
> > + if (start) {
> > + token =
> dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_START);
> > + if (!token)
> > + return -ENODEV;
> > + dell_fill_request(&buffer, token->location, 0, 0, 0);
> > + ret = dell_send_request(&buffer,
> > + CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> > + *start = buffer.output[1];
> > + }
>
> (1)
>
> > + if (end) {
> > + token = dell_smbios_find_token(BATTERY_CUSTOM_CHARGE_END);
> > + if (!token)
> > + return -ENODEV;
> > + dell_fill_request(&buffer, token->location, 0, 0, 0);
> > + ret = dell_send_request(&buffer,
> > + CLASS_TOKEN_READ, SELECT_TOKEN_STD);
> > + if (ret)
> > + return -EIO;
> > + *end = buffer.output[1];
> > + }
>
> (2)
>
> (1) and (2) look like twins. Care to provide a helper to simplify?
>
> > + return 0;
> > +}
>
> ...
>
> > + ret = dell_send_request(&buffer,
> > + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > + if (ret)
>
> > + return -EIO;
>
> Why shadowing error code?
>
> ...
>
> > + ret = dell_send_request(&buffer,
> > + CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
> > + if (ret)
> > + return -EIO;
>
> Ditto.
>
> > + return ret;
>
> And here perhaps simple 'return dell_send_request();'?
>
> ...
>
> > + switch (mode) {
> > + case BAT_STANDARD_MODE:
> > + token =
> > + dell_smbios_find_token(BAT_STANDARD_MODE_TOKEN);
>
> > + if (!token)
> > + return -ENODEV;
>
> > + break;
> > + case BAT_EXPRESS_MODE:
> > + token =
> > + dell_smbios_find_token(BAT_EXPRESS_MODE_TOKEN);
>
> > + if (!token)
> > + return -ENODEV;
>
> > + break;
> > + case BAT_PRIMARILY_AC_MODE:
> > + token =
> > + dell_smbios_find_token(BAT_PRIMARILY_AC_MODE_TOKEN);
>
> > + if (!token)
> > + return -ENODEV;
>
> > + break;
> > + case BAT_CUSTOM_MODE:
> > + token = dell_smbios_find_token(BAT_CUSTOM_MODE_TOKEN);
>
> > + if (!token)
> > + return -ENODEV;
>
> > + break;
> > + case BAT_ADAPTIVE_MODE:
> > + token =
> > + dell_smbios_find_token(BAT_ADAPTIVE_MODE_TOKEN);
>
> > + if (!token)
> > + return -ENODEV;
>
> Five times the same lines. Please deduplicate them. One is enough.
>
> > + break;
> > + default:
>
> > + pr_warn("unspported charging mode!\n");
>
> When you have a device use dev_*() to print messages.
>
> > + return -EINVAL;
> > + }
>
> ...
>
> > + ret = dell_send_request(&buffer, CLASS_TOKEN_WRITE,
> SELECT_TOKEN_STD);
> > + if (ret)
> > + return -EIO;
> > +
> > + return ret;
>
> return dell_send_request(...);
>
> ...
>
> > + ret = dell_send_request(&buffer, CLASS_TOKEN_READ,
> SELECT_TOKEN_STD);
> > + if (ret)
> > + return -EIO;
>
> Do not shadow error codes.
>
> > + if (ret == 0)
>
> What is the point?
>
> > + *mode = buffer.output[1];
>
> > + return ret;
>
> Any difference to return 0; ?
>
> ...
>
> > +static ssize_t charge_control_charging_mode_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + enum battery_charging_mode mode;
> > + char *s = buf;
> > +
> > + for (mode = BAT_STANDARD_MODE; mode < BAT_MAX_MODE; mode++)
> {
> > + if (battery_state[mode]) {
> > + if (mode == bat_chg_current)
> > + s += sprintf(s, "[%s] ", battery_state[mode]);
> > + else
> > + s += sprintf(s, "%s ",
> > + battery_state[mode]);
>
> You have to control buffer boundaries.
>
> > + }
> > + }
>
> > + if (s != buf)
>
> if (s == buf)
> return 0;
>
> > + /* convert the last space to a newline */
> > + *(s-1) = '\n';
> > + return (s - buf);
> > +}
>
> ...
>
> > +static ssize_t charge_control_charging_mode_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t size) {
> > + int err;
> > + enum battery_charging_mode mode;
> > + char *p;
> > + int len;
> > + const char *label;
>
> > + p = memchr(buf, '\n', size);
> > + len = p ? p - buf : size;
> > +
> > + for (mode = BAT_STANDARD_MODE; mode < BAT_MAX_MODE; mode++)
> {
> > + label = battery_state[mode];
> > + if (label && len == strlen(label) &&
> > + !strncmp(buf, label, len)) {
> > + bat_chg_current = mode;
> > + break;
> > + }
> > + }
>
> sysfs_match_string()
>
> > + if (mode > BAT_NONE_MODE && mode < BAT_MAX_MODE)
> > + err = battery_charging_mode_set(mode);
> > + else
> > + err = -EINVAL;
> > +
> > + return err ? err : size;
> > +}
>
> ...
>
> > +static ssize_t charge_control_thresholds_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t size) {
> > + int err, start, end;
>
> > + if (sscanf(buf, "%d %d", &start, &end) != 2)
> > + return -EINVAL;
>
> It's bad. The rule of thumb is one node per one property.
> On top of that sscanf() is not good, it doesn't check for overflow, use one of
> kstrto*().
>
> > + err = dell_battery_set(start, end);
> > + if (err)
> > + return err;
> > +
> > + return size;
> > +}
>
> ...
>
> > +static int dell_battery_add(struct power_supply *battery) {
> > + device_create_file(&battery->dev,
> > + &dev_attr_charge_control_start_threshold);
> > + device_create_file(&battery->dev,
> > + &dev_attr_charge_control_end_threshold);
> > + device_create_file(&battery->dev,
> > + &dev_attr_charge_control_charging_mode);
>
> Can it be an attribute group?
>
> > +}
>
> ...
>
> > +static struct acpi_battery_hook dell_battery_hook = {
> > + .add_battery = dell_battery_add,
> > + .remove_battery = dell_battery_remove,
> > + .name = "Dell Battery Extension"
>
> Missed comma.
>
> > +};
>
> ...
>
> > +static void dell_battery_setup(struct device *dev) {
> > + enum battery_charging_mode current_mode = BAT_NONE_MODE;
> > +
> > + battery_charging_mode_get(¤t_mode);
> > + if (current_mode) {
> > + bat_chg_current = current_mode;
>
> > + pr_debug("battery is present\n");
> > + } else {
> > + pr_debug("battery is not present\n");
> > + }
>
> Why not dev_dbg()? Why do we have these messages at all?
>
> > + battery_hook_register(&dell_battery_hook);
>
> > + device_create_file(dev, &dev_attr_charge_control_thresholds);
>
> Hmm...
>
> > +}
> > +
> > +static void dell_battery_exit(struct device *dev) {
> > + if (bat_chg_current != BAT_NONE_MODE) {
> > + battery_hook_unregister(&dell_battery_hook);
> > + device_remove_file(dev, &dev_attr_charge_control_thresholds);
> > + }
> > +}
>
> ...
>
> > +/*Battery Charging Modes */
>
> Indentation issues.
> Same for many other comments in this change.
>
>
> --
> With Best Regards,
> Andy Shevchenko
Andy .
Thanks a lot for your review, I am working on the V2 to drive your feedback.
Powered by blists - more mailing lists