* Re: A patch to test_power module
2013-02-03 3:39 ` A patch to test_power module Anton Vorontsov
@ 2013-02-03 11:17 ` Andrey Gelman
2013-03-19 2:31 ` Anton Vorontsov
0 siblings, 1 reply; 3+ messages in thread
From: Andrey Gelman @ 2013-02-03 11:17 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: dwmw2, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5849 bytes --]
On 02/03/2013 05:39 AM, Anton Vorontsov wrote:
> On Sun, Jan 27, 2013 at 10:45:43PM +0200, Andrey Gelman wrote:
>> Hello !
>> This patch fixes 2 issues in test_power module:
>> 1. the mapping of ac_online parameter to its string value "on / off"
>> was swapped.
>> 2. more important ...
>> You could not insmod test_power with module parameters, without
>> causing kernel crash.
>> The reason is that in test_power, setting module parameter value has
>> side effect - power_supply_changed event.
>> As the module parameters are processed prior to calling module_init,
>> power_supply_changed event is generated on behalf module that is not
>> yet initialized, causing some NULL pointer dereference.
>>
>> My solution is to test whether the module has been initialized,
>> prior to calling power_supply_changed.
>
> Hi!
>
> The patch looks good, but it is missing Signed-off-by tag. Plus, you
> should also Cc: linux-kernel@vger.kernel.org on submissions (I've added it
> now).
>
> Plus, please don't use html in emails. :)
>
> Thanks a lot!
>
> Anton
>
>> From ccd5b3e916477a3c2a51bbf10a3be8e01f3c85e4 Mon Sep 17 00:00:00 2001
>> From: Andrey Gelman <andreyg@compulab.co.il>
>> Date: Sun, 27 Jan 2013 22:16:33 +0200
>> Subject: [PATCH] test_power: fix a bug in setting module parameter values
>>
>> When the kernel loads a module, module params are processed
>> prior to calling module_init. As a result, setting module param
>> value should not have side effects, at least as long as the
>> module has not been initialized.
>> ---
>> drivers/power/test_power.c | 31 +++++++++++++++++++++----------
>> 1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/power/test_power.c b/drivers/power/test_power.c
>> index b99a452..753a74f 100644
>> --- a/drivers/power/test_power.c
>> +++ b/drivers/power/test_power.c
>> @@ -30,6 +30,8 @@ static int battery_technology = POWER_SUPPLY_TECHNOLOGY_LION;
>> static int battery_capacity = 50;
>> static int battery_voltage = 3300;
>>
>> +static bool module_initialized = false;
>> +
>> static int test_power_get_ac_property(struct power_supply *psy,
>> enum power_supply_property psp,
>> union power_supply_propval *val)
>> @@ -185,6 +187,7 @@ static int __init test_power_init(void)
>> }
>> }
>>
>> + module_initialized = true;
>> return 0;
>> failed:
>> while (--i >= 0)
>> @@ -209,6 +212,8 @@ static void __exit test_power_exit(void)
>>
>> for (i = 0; i < ARRAY_SIZE(test_power_supplies); i++)
>> power_supply_unregister(&test_power_supplies[i]);
>> +
>> + module_initialized = false;
>> }
>> module_exit(test_power_exit);
>>
>> @@ -221,8 +226,8 @@ struct battery_property_map {
>> };
>>
>> static struct battery_property_map map_ac_online[] = {
>> - { 0, "on" },
>> - { 1, "off" },
>> + { 0, "off" },
>> + { 1, "on" },
>> { -1, NULL },
>> };
>>
>> @@ -295,10 +300,16 @@ static const char *map_get_key(struct battery_property_map *map, int value,
>> return def_key;
>> }
>>
>> +static inline void signal_power_supply_changed(struct power_supply *psy)
>> +{
>> + if (module_initialized)
>> + power_supply_changed(psy);
>> +}
>> +
>> static int param_set_ac_online(const char *key, const struct kernel_param *kp)
>> {
>> ac_online = map_get_value(map_ac_online, key, ac_online);
>> - power_supply_changed(&test_power_supplies[0]);
>> + signal_power_supply_changed(&test_power_supplies[0]);
>> return 0;
>> }
>>
>> @@ -311,7 +322,7 @@ static int param_get_ac_online(char *buffer, const struct kernel_param *kp)
>> static int param_set_usb_online(const char *key, const struct kernel_param *kp)
>> {
>> usb_online = map_get_value(map_ac_online, key, usb_online);
>> - power_supply_changed(&test_power_supplies[2]);
>> + signal_power_supply_changed(&test_power_supplies[2]);
>> return 0;
>> }
>>
>> @@ -325,7 +336,7 @@ static int param_set_battery_status(const char *key,
>> const struct kernel_param *kp)
>> {
>> battery_status = map_get_value(map_status, key, battery_status);
>> - power_supply_changed(&test_power_supplies[1]);
>> + signal_power_supply_changed(&test_power_supplies[1]);
>> return 0;
>> }
>>
>> @@ -339,7 +350,7 @@ static int param_set_battery_health(const char *key,
>> const struct kernel_param *kp)
>> {
>> battery_health = map_get_value(map_health, key, battery_health);
>> - power_supply_changed(&test_power_supplies[1]);
>> + signal_power_supply_changed(&test_power_supplies[1]);
>> return 0;
>> }
>>
>> @@ -353,7 +364,7 @@ static int param_set_battery_present(const char *key,
>> const struct kernel_param *kp)
>> {
>> battery_present = map_get_value(map_present, key, battery_present);
>> - power_supply_changed(&test_power_supplies[0]);
>> + signal_power_supply_changed(&test_power_supplies[0]);
>> return 0;
>> }
>>
>> @@ -369,7 +380,7 @@ static int param_set_battery_technology(const char *key,
>> {
>> battery_technology = map_get_value(map_technology, key,
>> battery_technology);
>> - power_supply_changed(&test_power_supplies[1]);
>> + signal_power_supply_changed(&test_power_supplies[1]);
>> return 0;
>> }
>>
>> @@ -390,7 +401,7 @@ static int param_set_battery_capacity(const char *key,
>> return -EINVAL;
>>
>> battery_capacity = tmp;
>> - power_supply_changed(&test_power_supplies[1]);
>> + signal_power_supply_changed(&test_power_supplies[1]);
>> return 0;
>> }
>>
>> @@ -405,7 +416,7 @@ static int param_set_battery_voltage(const char *key,
>> return -EINVAL;
>>
>> battery_voltage = tmp;
>> - power_supply_changed(&test_power_supplies[1]);
>> + signal_power_supply_changed(&test_power_supplies[1]);
>> return 0;
>> }
>>
>> --
>> 1.7.9.5
Fixed ... I think.
Thank you,
Andrey
[-- Attachment #2: test_power.patch --]
[-- Type: text/x-patch, Size: 4309 bytes --]
>From cc873c2cddd8c7457ad807d71ebada42754638dd Mon Sep 17 00:00:00 2001
From: Andrey Gelman <andreyg@compulab.co.il>
Date: Sun, 27 Jan 2013 22:16:33 +0200
Subject: [PATCH] test_power: fix a bug in setting module parameter values
When the kernel loads a module, module params are processed
prior to calling module_init. As a result, setting module param
value should not have side effects, at least as long as the
module has not been initialized.
Signed-off-by: Andrey Gelman <andreyg@compulab.co.il>
---
drivers/power/test_power.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/drivers/power/test_power.c b/drivers/power/test_power.c
index b99a452..753a74f 100644
--- a/drivers/power/test_power.c
+++ b/drivers/power/test_power.c
@@ -30,6 +30,8 @@ static int battery_technology = POWER_SUPPLY_TECHNOLOGY_LION;
static int battery_capacity = 50;
static int battery_voltage = 3300;
+static bool module_initialized = false;
+
static int test_power_get_ac_property(struct power_supply *psy,
enum power_supply_property psp,
union power_supply_propval *val)
@@ -185,6 +187,7 @@ static int __init test_power_init(void)
}
}
+ module_initialized = true;
return 0;
failed:
while (--i >= 0)
@@ -209,6 +212,8 @@ static void __exit test_power_exit(void)
for (i = 0; i < ARRAY_SIZE(test_power_supplies); i++)
power_supply_unregister(&test_power_supplies[i]);
+
+ module_initialized = false;
}
module_exit(test_power_exit);
@@ -221,8 +226,8 @@ struct battery_property_map {
};
static struct battery_property_map map_ac_online[] = {
- { 0, "on" },
- { 1, "off" },
+ { 0, "off" },
+ { 1, "on" },
{ -1, NULL },
};
@@ -295,10 +300,16 @@ static const char *map_get_key(struct battery_property_map *map, int value,
return def_key;
}
+static inline void signal_power_supply_changed(struct power_supply *psy)
+{
+ if (module_initialized)
+ power_supply_changed(psy);
+}
+
static int param_set_ac_online(const char *key, const struct kernel_param *kp)
{
ac_online = map_get_value(map_ac_online, key, ac_online);
- power_supply_changed(&test_power_supplies[0]);
+ signal_power_supply_changed(&test_power_supplies[0]);
return 0;
}
@@ -311,7 +322,7 @@ static int param_get_ac_online(char *buffer, const struct kernel_param *kp)
static int param_set_usb_online(const char *key, const struct kernel_param *kp)
{
usb_online = map_get_value(map_ac_online, key, usb_online);
- power_supply_changed(&test_power_supplies[2]);
+ signal_power_supply_changed(&test_power_supplies[2]);
return 0;
}
@@ -325,7 +336,7 @@ static int param_set_battery_status(const char *key,
const struct kernel_param *kp)
{
battery_status = map_get_value(map_status, key, battery_status);
- power_supply_changed(&test_power_supplies[1]);
+ signal_power_supply_changed(&test_power_supplies[1]);
return 0;
}
@@ -339,7 +350,7 @@ static int param_set_battery_health(const char *key,
const struct kernel_param *kp)
{
battery_health = map_get_value(map_health, key, battery_health);
- power_supply_changed(&test_power_supplies[1]);
+ signal_power_supply_changed(&test_power_supplies[1]);
return 0;
}
@@ -353,7 +364,7 @@ static int param_set_battery_present(const char *key,
const struct kernel_param *kp)
{
battery_present = map_get_value(map_present, key, battery_present);
- power_supply_changed(&test_power_supplies[0]);
+ signal_power_supply_changed(&test_power_supplies[0]);
return 0;
}
@@ -369,7 +380,7 @@ static int param_set_battery_technology(const char *key,
{
battery_technology = map_get_value(map_technology, key,
battery_technology);
- power_supply_changed(&test_power_supplies[1]);
+ signal_power_supply_changed(&test_power_supplies[1]);
return 0;
}
@@ -390,7 +401,7 @@ static int param_set_battery_capacity(const char *key,
return -EINVAL;
battery_capacity = tmp;
- power_supply_changed(&test_power_supplies[1]);
+ signal_power_supply_changed(&test_power_supplies[1]);
return 0;
}
@@ -405,7 +416,7 @@ static int param_set_battery_voltage(const char *key,
return -EINVAL;
battery_voltage = tmp;
- power_supply_changed(&test_power_supplies[1]);
+ signal_power_supply_changed(&test_power_supplies[1]);
return 0;
}
--
1.7.9.5
^ permalink raw reply [flat|nested] 3+ messages in thread