* [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 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