* [PATCH 1/3] hwmon: (applesmc) Allow format checking
@ 2015-02-12 14:15 Rasmus Villemoes
2015-02-12 14:15 ` [PATCH 2/3] hwmon: (coretemp) " Rasmus Villemoes
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Rasmus Villemoes @ 2015-02-12 14:15 UTC (permalink / raw)
To: Henrik Rydberg, Jean Delvare, Guenter Roeck
Cc: Rasmus Villemoes, lm-sensors, linux-kernel
Currently gcc and other tools can't check the format strings. It's
easy to fix by letting fan_speed_fmt simply hold what is different
between the strings (and renaming it appropriately). While at it, we
can also eliminate some wasted space and an extra level of indirection
by making it an array of char[4] instead of char*.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
drivers/hwmon/applesmc.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 0af63da6b603..0c950e1b03f3 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -84,12 +84,12 @@
#define TEMP_SENSOR_TYPE "sp78"
/* List of keys used to read/write fan speeds */
-static const char *const fan_speed_fmt[] = {
- "F%dAc", /* actual speed */
- "F%dMn", /* minimum speed (rw) */
- "F%dMx", /* maximum speed */
- "F%dSf", /* safe speed - not all models */
- "F%dTg", /* target speed (manual: rw) */
+static const char fan_speed_suffix[][4] = {
+ "Ac", /* actual speed */
+ "Mn", /* minimum speed (rw) */
+ "Mx", /* maximum speed */
+ "Sf", /* safe speed - not all models */
+ "Tg", /* target speed (manual: rw) */
};
#define INIT_TIMEOUT_MSECS 5000 /* wait up to 5s for device init ... */
@@ -811,7 +811,7 @@ static ssize_t applesmc_show_fan_speed(struct device *dev,
char newkey[5];
u8 buffer[2];
- sprintf(newkey, fan_speed_fmt[to_option(attr)], to_index(attr));
+ sprintf(newkey, "F%d%s", to_index(attr), fan_speed_suffix[to_option(attr)]);
ret = applesmc_read_key(newkey, buffer, 2);
speed = ((buffer[0] << 8 | buffer[1]) >> 2);
@@ -834,7 +834,7 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
if (kstrtoul(sysfsbuf, 10, &speed) < 0 || speed >= 0x4000)
return -EINVAL; /* Bigger than a 14-bit value */
- sprintf(newkey, fan_speed_fmt[to_option(attr)], to_index(attr));
+ sprintf(newkey, "F%d%s", to_index(attr), fan_speed_suffix[to_option(attr)]);
buffer[0] = (speed >> 6) & 0xff;
buffer[1] = (speed << 2) & 0xff;
--
2.1.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] hwmon: (coretemp) Allow format checking
2015-02-12 14:15 [PATCH 1/3] hwmon: (applesmc) Allow format checking Rasmus Villemoes
@ 2015-02-12 14:15 ` Rasmus Villemoes
2015-02-13 5:08 ` Guenter Roeck
2015-02-12 14:15 ` [PATCH 3/3] hwmon: (ibmpex) Allow format string checking Rasmus Villemoes
2015-02-13 5:13 ` [PATCH 1/3] hwmon: (applesmc) Allow format checking Guenter Roeck
2 siblings, 1 reply; 6+ messages in thread
From: Rasmus Villemoes @ 2015-02-12 14:15 UTC (permalink / raw)
To: Fenghua Yu, Jean Delvare, Guenter Roeck
Cc: Rasmus Villemoes, lm-sensors, linux-kernel
By extracting the only part that differs we can allow static checking
of the format string, and possibly save a little .rodata.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
drivers/hwmon/coretemp.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index 5b7fec824f10..0e873bca98a4 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -397,14 +397,13 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev,
struct device_attribute *devattr, char *buf) = {
show_label, show_crit_alarm, show_temp, show_tjmax,
show_ttarget };
- static const char *const names[TOTAL_ATTRS] = {
- "temp%d_label", "temp%d_crit_alarm",
- "temp%d_input", "temp%d_crit",
- "temp%d_max" };
+ static const char *const suffixes[TOTAL_ATTRS] = {
+ "label", "crit_alarm", "input", "crit", "max"
+ };
for (i = 0; i < tdata->attr_size; i++) {
- snprintf(tdata->attr_name[i], CORETEMP_NAME_LENGTH, names[i],
- attr_no);
+ snprintf(tdata->attr_name[i], CORETEMP_NAME_LENGTH,
+ "temp%d_%s", attr_no, suffixes[i]);
sysfs_attr_init(&tdata->sd_attrs[i].dev_attr.attr);
tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i];
tdata->sd_attrs[i].dev_attr.attr.mode = S_IRUGO;
--
2.1.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] hwmon: (ibmpex) Allow format string checking
2015-02-12 14:15 [PATCH 1/3] hwmon: (applesmc) Allow format checking Rasmus Villemoes
2015-02-12 14:15 ` [PATCH 2/3] hwmon: (coretemp) " Rasmus Villemoes
@ 2015-02-12 14:15 ` Rasmus Villemoes
2015-02-13 5:09 ` Guenter Roeck
2015-02-13 5:13 ` [PATCH 1/3] hwmon: (applesmc) Allow format checking Guenter Roeck
2 siblings, 1 reply; 6+ messages in thread
From: Rasmus Villemoes @ 2015-02-12 14:15 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck; +Cc: Rasmus Villemoes, lm-sensors, linux-kernel
The only difference between the three power_sensor_name_templates is
whether there is a suffix of "", "_lowest" or "_highest". We might as
well pull those into an array and use a literal format string,
allowing gcc to do type checking of the arguments to
sprintf. Incidentially, the same three suffixes are used in the
temp_sensor_name_templates case, so we end up eliminating one static
array.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
drivers/hwmon/ibmpex.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/hwmon/ibmpex.c b/drivers/hwmon/ibmpex.c
index 030e7ff589be..2eb672299a31 100644
--- a/drivers/hwmon/ibmpex.c
+++ b/drivers/hwmon/ibmpex.c
@@ -56,15 +56,10 @@ static u8 const temp_sensor_sig[] = {0x74, 0x65, 0x6D};
static u8 const watt_sensor_sig[] = {0x41, 0x43};
#define PEX_NUM_SENSOR_FUNCS 3
-static char const * const power_sensor_name_templates[] = {
- "%s%d_average",
- "%s%d_average_lowest",
- "%s%d_average_highest"
-};
-static char const * const temp_sensor_name_templates[] = {
- "%s%d_input",
- "%s%d_input_lowest",
- "%s%d_input_highest"
+static const char * const sensor_name_suffixes[] = {
+ "",
+ "_lowest",
+ "_highest"
};
static void ibmpex_msg_handler(struct ipmi_recv_msg *msg, void *user_msg_data);
@@ -355,9 +350,9 @@ static int create_sensor(struct ibmpex_bmc_data *data, int type,
return -ENOMEM;
if (type == TEMP_SENSOR)
- sprintf(n, temp_sensor_name_templates[func], "temp", counter);
+ sprintf(n, "temp%d_input%s", counter, sensor_name_suffixes[func]);
else if (type == POWER_SENSOR)
- sprintf(n, power_sensor_name_templates[func], "power", counter);
+ sprintf(n, "power%d_average%s", counter, sensor_name_suffixes[func]);
sysfs_attr_init(&data->sensors[sensor].attr[func].dev_attr.attr);
data->sensors[sensor].attr[func].dev_attr.attr.name = n;
--
2.1.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] hwmon: (coretemp) Allow format checking
2015-02-12 14:15 ` [PATCH 2/3] hwmon: (coretemp) " Rasmus Villemoes
@ 2015-02-13 5:08 ` Guenter Roeck
0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2015-02-13 5:08 UTC (permalink / raw)
To: Rasmus Villemoes, Fenghua Yu, Jean Delvare; +Cc: lm-sensors, linux-kernel
On 02/12/2015 06:15 AM, Rasmus Villemoes wrote:
> By extracting the only part that differs we can allow static checking
> of the format string, and possibly save a little .rodata.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Applied to -next after fixing multi-line alignment.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] hwmon: (ibmpex) Allow format string checking
2015-02-12 14:15 ` [PATCH 3/3] hwmon: (ibmpex) Allow format string checking Rasmus Villemoes
@ 2015-02-13 5:09 ` Guenter Roeck
0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2015-02-13 5:09 UTC (permalink / raw)
To: Rasmus Villemoes, Jean Delvare; +Cc: lm-sensors, linux-kernel
On 02/12/2015 06:15 AM, Rasmus Villemoes wrote:
> The only difference between the three power_sensor_name_templates is
> whether there is a suffix of "", "_lowest" or "_highest". We might as
> well pull those into an array and use a literal format string,
> allowing gcc to do type checking of the arguments to
> sprintf. Incidentially, the same three suffixes are used in the
> temp_sensor_name_templates case, so we end up eliminating one static
> array.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Applied to -next after fixing 'line over 80 characters' checkpatch warning.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] hwmon: (applesmc) Allow format checking
2015-02-12 14:15 [PATCH 1/3] hwmon: (applesmc) Allow format checking Rasmus Villemoes
2015-02-12 14:15 ` [PATCH 2/3] hwmon: (coretemp) " Rasmus Villemoes
2015-02-12 14:15 ` [PATCH 3/3] hwmon: (ibmpex) Allow format string checking Rasmus Villemoes
@ 2015-02-13 5:13 ` Guenter Roeck
2 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2015-02-13 5:13 UTC (permalink / raw)
To: Rasmus Villemoes, Henrik Rydberg, Jean Delvare; +Cc: lm-sensors, linux-kernel
On 02/12/2015 06:15 AM, Rasmus Villemoes wrote:
> Currently gcc and other tools can't check the format strings. It's
> easy to fix by letting fan_speed_fmt simply hold what is different
> between the strings (and renaming it appropriately). While at it, we
> can also eliminate some wasted space and an extra level of indirection
> by making it an array of char[4] instead of char*.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Saving a few bytes with the added cost of harder to understand code.
Not really sure if that is worth it. I'll need to see a Tested-by:
for this patch.
Also, please fix the checkpatch warnings.
Thanks,
Guenter
> ---
> drivers/hwmon/applesmc.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 0af63da6b603..0c950e1b03f3 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -84,12 +84,12 @@
> #define TEMP_SENSOR_TYPE "sp78"
>
> /* List of keys used to read/write fan speeds */
> -static const char *const fan_speed_fmt[] = {
> - "F%dAc", /* actual speed */
> - "F%dMn", /* minimum speed (rw) */
> - "F%dMx", /* maximum speed */
> - "F%dSf", /* safe speed - not all models */
> - "F%dTg", /* target speed (manual: rw) */
> +static const char fan_speed_suffix[][4] = {
> + "Ac", /* actual speed */
> + "Mn", /* minimum speed (rw) */
> + "Mx", /* maximum speed */
> + "Sf", /* safe speed - not all models */
> + "Tg", /* target speed (manual: rw) */
> };
>
> #define INIT_TIMEOUT_MSECS 5000 /* wait up to 5s for device init ... */
> @@ -811,7 +811,7 @@ static ssize_t applesmc_show_fan_speed(struct device *dev,
> char newkey[5];
> u8 buffer[2];
>
> - sprintf(newkey, fan_speed_fmt[to_option(attr)], to_index(attr));
> + sprintf(newkey, "F%d%s", to_index(attr), fan_speed_suffix[to_option(attr)]);
>
> ret = applesmc_read_key(newkey, buffer, 2);
> speed = ((buffer[0] << 8 | buffer[1]) >> 2);
> @@ -834,7 +834,7 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
> if (kstrtoul(sysfsbuf, 10, &speed) < 0 || speed >= 0x4000)
> return -EINVAL; /* Bigger than a 14-bit value */
>
> - sprintf(newkey, fan_speed_fmt[to_option(attr)], to_index(attr));
> + sprintf(newkey, "F%d%s", to_index(attr), fan_speed_suffix[to_option(attr)]);
>
> buffer[0] = (speed >> 6) & 0xff;
> buffer[1] = (speed << 2) & 0xff;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-02-13 5:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-12 14:15 [PATCH 1/3] hwmon: (applesmc) Allow format checking Rasmus Villemoes
2015-02-12 14:15 ` [PATCH 2/3] hwmon: (coretemp) " Rasmus Villemoes
2015-02-13 5:08 ` Guenter Roeck
2015-02-12 14:15 ` [PATCH 3/3] hwmon: (ibmpex) Allow format string checking Rasmus Villemoes
2015-02-13 5:09 ` Guenter Roeck
2015-02-13 5:13 ` [PATCH 1/3] hwmon: (applesmc) Allow format checking Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox