linux-kernel - Re: [PATCH] test_power: Add CURRENT and CHARGE

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
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190625223537.6dlcooifqfs2rv2b@earth.universe>
Date:   Wed, 26 Jun 2019 00:35:37 +0200
From:   Sebastian Reichel <sebastian.reichel@...labora.com>
To:     Lecopzer Chen <lecopzer.chen@...iatek.com>
Cc:     linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        srv_heupstream@...iatek.com, yj.chiang@...iatek.com,
        linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] test_power: Add CURRENT and CHARGE_COUNTER properties

Hi,

On Tue, May 14, 2019 at 06:04:58PM +0800, Lecopzer Chen wrote:
> From: "Lecopzer Chen" <lecopzer.chen@...iatek.com>
> 
> Emulate battery current (variable) and battery CHARGE_COUNTER
> (same as battery_capacity) properties.
> 
> Signed-off-by: Lecopzer Chen <lecopzer.chen@...iatek.com>
> Cc: YJ Chiang <yj.chiang@...iatek.com>

Thanks for the patch and thanks for taking care of testing infrastructure.
I don't think it's a good idea to use CHARGE_CAPACITY/CHARGE_NOW also for
CHARGE_COUNTER. CHARGE_COUNTER may be negative, but capacity should not be
negative. Also the patch should be splitted (one for the current feature and
one for the charge counter feature). Last but not least I don't see any
conversion from mA to µA. The userspace sysfs ABI should always
return µA.

-- Sebastian

> ---
>  drivers/power/supply/test_power.c | 33 +++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
> index 57246cdbd042..9f85060c84de 100644
> --- a/drivers/power/supply/test_power.c
> +++ b/drivers/power/supply/test_power.c
> @@ -36,6 +36,7 @@ static int battery_present		= 1; /* true */
>  static int battery_technology		= POWER_SUPPLY_TECHNOLOGY_LION;
>  static int battery_capacity		= 50;
>  static int battery_voltage		= 3300;
> +static int battery_current		= 1600;
>  
>  static bool module_initialized;
>  
> @@ -101,6 +102,7 @@ static int test_power_get_battery_property(struct power_supply *psy,
>  		break;
>  	case POWER_SUPPLY_PROP_CAPACITY:
>  	case POWER_SUPPLY_PROP_CHARGE_NOW:
> +	case POWER_SUPPLY_PROP_CHARGE_COUNTER:
>  		val->intval = battery_capacity;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> @@ -117,6 +119,10 @@ static int test_power_get_battery_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>  		val->intval = battery_voltage;
>  		break;
> +	case POWER_SUPPLY_PROP_CURRENT_AVG:
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		val->intval = battery_current;
> +		break;
>  	default:
>  		pr_info("%s: some properties deliberately report errors.\n",
>  			__func__);
> @@ -138,6 +144,7 @@ static enum power_supply_property test_power_battery_props[] = {
>  	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
>  	POWER_SUPPLY_PROP_CHARGE_FULL,
>  	POWER_SUPPLY_PROP_CHARGE_NOW,
> +	POWER_SUPPLY_PROP_CHARGE_COUNTER,
>  	POWER_SUPPLY_PROP_CAPACITY,
>  	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
>  	POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG,
> @@ -147,6 +154,8 @@ static enum power_supply_property test_power_battery_props[] = {
>  	POWER_SUPPLY_PROP_SERIAL_NUMBER,
>  	POWER_SUPPLY_PROP_TEMP,
>  	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_AVG,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
>  };
>  
>  static char *test_power_ac_supplied_to[] = {
> @@ -450,6 +459,21 @@ static int param_set_battery_voltage(const char *key,
>  
>  #define param_get_battery_voltage param_get_int
>  
> +static int param_set_battery_current(const char *key,
> +					const struct kernel_param *kp)
> +{
> +	int tmp;
> +
> +	if (1 != sscanf(key, "%d", &tmp))
> +		return -EINVAL;
> +
> +	battery_current = tmp;
> +	signal_power_supply_changed(test_power_supplies[TEST_BATTERY]);
> +	return 0;
> +}
> +
> +#define param_get_battery_current param_get_int
> +
>  static const struct kernel_param_ops param_ops_ac_online = {
>  	.set = param_set_ac_online,
>  	.get = param_get_ac_online,
> @@ -490,6 +514,11 @@ static const struct kernel_param_ops param_ops_battery_voltage = {
>  	.get = param_get_battery_voltage,
>  };
>  
> +static const struct kernel_param_ops param_ops_battery_current = {
> +	.set = param_set_battery_current,
> +	.get = param_get_battery_current,
> +};
> +
>  #define param_check_ac_online(name, p) __param_check(name, p, void);
>  #define param_check_usb_online(name, p) __param_check(name, p, void);
>  #define param_check_battery_status(name, p) __param_check(name, p, void);
> @@ -498,6 +527,7 @@ static const struct kernel_param_ops param_ops_battery_voltage = {
>  #define param_check_battery_health(name, p) __param_check(name, p, void);
>  #define param_check_battery_capacity(name, p) __param_check(name, p, void);
>  #define param_check_battery_voltage(name, p) __param_check(name, p, void);
> +#define param_check_battery_current(name, p) __param_check(name, p, void);
>  
>  
>  module_param(ac_online, ac_online, 0644);
> @@ -528,6 +558,9 @@ MODULE_PARM_DESC(battery_capacity, "battery capacity (percentage)");
>  module_param(battery_voltage, battery_voltage, 0644);
>  MODULE_PARM_DESC(battery_voltage, "battery voltage (millivolts)");
>  
> +module_param(battery_current, battery_current, 0644);
> +MODULE_PARM_DESC(battery_current, "battery current (milliampere)");
> +
>  MODULE_DESCRIPTION("Power supply driver for testing");
>  MODULE_AUTHOR("Anton Vorontsov <cbouatmailru@...il.com>");
>  MODULE_LICENSE("GPL");
> -- 
> 2.18.0
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)