* [bug report] hwmon: (nct6775) Add support for weighted fan control
@ 2018-09-04 12:40 Dan Carpenter
2018-09-04 14:27 ` Guenter Roeck
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2018-09-04 12:40 UTC (permalink / raw)
To: linux; +Cc: linux-hwmon
Hello Guenter Roeck,
The patch bbd8decd4123: "hwmon: (nct6775) Add support for weighted
fan control" from Dec 4, 2012, leads to the following static checker
warning:
drivers/hwmon/nct6775.c:1562 nct6775_update_pwm()
warn: dead code because of 'j == 1' and 'j < (56 / 8 + 0)'
drivers/hwmon/nct6775.c
1503 static void nct6775_update_pwm(struct device *dev)
1504 {
1505 struct nct6775_data *data = dev_get_drvdata(dev);
1506 int i, j;
1507 int fanmodecfg, reg;
1508 bool duty_is_dc;
1509
1510 for (i = 0; i < data->pwm_num; i++) {
1511 if (!(data->has_pwm & BIT(i)))
1512 continue;
1513
1514 duty_is_dc = data->REG_PWM_MODE[i] &&
1515 (nct6775_read_value(data, data->REG_PWM_MODE[i])
1516 & data->PWM_MODE_MASK[i]);
1517 data->pwm_mode[i] = !duty_is_dc;
1518
1519 fanmodecfg = nct6775_read_value(data, data->REG_FAN_MODE[i]);
1520 for (j = 0; j < ARRAY_SIZE(data->REG_PWM); j++) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
j is set to ARRAY_SIZE(data->REG_PWM) which is 7 at the end of this
loop.
1521 if (data->REG_PWM[j] && data->REG_PWM[j][i]) {
1522 data->pwm[j][i]
1523 = nct6775_read_value(data,
1524 data->REG_PWM[j][i]);
1525 }
1526 }
1527
1528 data->pwm_enable[i] = reg_to_pwm_enable(data->pwm[0][i],
1529 (fanmodecfg >> 4) & 7);
1530
1531 if (!data->temp_tolerance[0][i] ||
1532 data->pwm_enable[i] != speed_cruise)
1533 data->temp_tolerance[0][i] = fanmodecfg & 0x0f;
1534 if (!data->target_speed_tolerance[i] ||
1535 data->pwm_enable[i] == speed_cruise) {
1536 u8 t = fanmodecfg & 0x0f;
1537
1538 if (data->REG_TOLERANCE_H) {
1539 t |= (nct6775_read_value(data,
1540 data->REG_TOLERANCE_H[i]) & 0x70) >> 1;
1541 }
1542 data->target_speed_tolerance[i] = t;
1543 }
1544
1545 data->temp_tolerance[1][i] =
1546 nct6775_read_value(data,
1547 data->REG_CRITICAL_TEMP_TOLERANCE[i]);
1548
1549 reg = nct6775_read_value(data, data->REG_TEMP_SEL[i]);
1550 data->pwm_temp_sel[i] = reg & 0x1f;
1551 /* If fan can stop, report floor as 0 */
1552 if (reg & 0x80)
1553 data->pwm[2][i] = 0;
1554
1555 if (!data->REG_WEIGHT_TEMP_SEL[i])
1556 continue;
1557
1558 reg = nct6775_read_value(data, data->REG_WEIGHT_TEMP_SEL[i]);
1559 data->pwm_weight_temp_sel[i] = reg & 0x1f;
1560 /* If weight is disabled, report weight source as 0 */
1561 if (j == 1 && !(reg & 0x80))
^^^^^^
So it can't be 1 here. Did you intend to say "i == 1"?
1562 data->pwm_weight_temp_sel[i] = 0;
1563
1564 /* Weight temp data */
1565 for (j = 0; j < ARRAY_SIZE(data->weight_temp); j++) {
1566 data->weight_temp[j][i]
1567 = nct6775_read_value(data,
1568 data->REG_WEIGHT_TEMP[j][i]);
1569 }
1570 }
1571 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [bug report] hwmon: (nct6775) Add support for weighted fan control
2018-09-04 12:40 [bug report] hwmon: (nct6775) Add support for weighted fan control Dan Carpenter
@ 2018-09-04 14:27 ` Guenter Roeck
2018-09-05 7:46 ` Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Guenter Roeck @ 2018-09-04 14:27 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-hwmon
Hi Dan,
On 09/04/2018 05:40 AM, Dan Carpenter wrote:
> Hello Guenter Roeck,
>
> The patch bbd8decd4123: "hwmon: (nct6775) Add support for weighted
> fan control" from Dec 4, 2012, leads to the following static checker
> warning:
>
> drivers/hwmon/nct6775.c:1562 nct6775_update_pwm()
> warn: dead code because of 'j == 1' and 'j < (56 / 8 + 0)'
>
> drivers/hwmon/nct6775.c
> 1503 static void nct6775_update_pwm(struct device *dev)
> 1504 {
> 1505 struct nct6775_data *data = dev_get_drvdata(dev);
> 1506 int i, j;
> 1507 int fanmodecfg, reg;
> 1508 bool duty_is_dc;
> 1509
> 1510 for (i = 0; i < data->pwm_num; i++) {
> 1511 if (!(data->has_pwm & BIT(i)))
> 1512 continue;
> 1513
> 1514 duty_is_dc = data->REG_PWM_MODE[i] &&
> 1515 (nct6775_read_value(data, data->REG_PWM_MODE[i])
> 1516 & data->PWM_MODE_MASK[i]);
> 1517 data->pwm_mode[i] = !duty_is_dc;
> 1518
> 1519 fanmodecfg = nct6775_read_value(data, data->REG_FAN_MODE[i]);
> 1520 for (j = 0; j < ARRAY_SIZE(data->REG_PWM); j++) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> j is set to ARRAY_SIZE(data->REG_PWM) which is 7 at the end of this
> loop.
>
> 1521 if (data->REG_PWM[j] && data->REG_PWM[j][i]) {
> 1522 data->pwm[j][i]
> 1523 = nct6775_read_value(data,
> 1524 data->REG_PWM[j][i]);
> 1525 }
> 1526 }
> 1527
> 1528 data->pwm_enable[i] = reg_to_pwm_enable(data->pwm[0][i],
> 1529 (fanmodecfg >> 4) & 7);
> 1530
> 1531 if (!data->temp_tolerance[0][i] ||
> 1532 data->pwm_enable[i] != speed_cruise)
> 1533 data->temp_tolerance[0][i] = fanmodecfg & 0x0f;
> 1534 if (!data->target_speed_tolerance[i] ||
> 1535 data->pwm_enable[i] == speed_cruise) {
> 1536 u8 t = fanmodecfg & 0x0f;
> 1537
> 1538 if (data->REG_TOLERANCE_H) {
> 1539 t |= (nct6775_read_value(data,
> 1540 data->REG_TOLERANCE_H[i]) & 0x70) >> 1;
> 1541 }
> 1542 data->target_speed_tolerance[i] = t;
> 1543 }
> 1544
> 1545 data->temp_tolerance[1][i] =
> 1546 nct6775_read_value(data,
> 1547 data->REG_CRITICAL_TEMP_TOLERANCE[i]);
> 1548
> 1549 reg = nct6775_read_value(data, data->REG_TEMP_SEL[i]);
> 1550 data->pwm_temp_sel[i] = reg & 0x1f;
> 1551 /* If fan can stop, report floor as 0 */
> 1552 if (reg & 0x80)
> 1553 data->pwm[2][i] = 0;
> 1554
> 1555 if (!data->REG_WEIGHT_TEMP_SEL[i])
> 1556 continue;
> 1557
> 1558 reg = nct6775_read_value(data, data->REG_WEIGHT_TEMP_SEL[i]);
> 1559 data->pwm_weight_temp_sel[i] = reg & 0x1f;
> 1560 /* If weight is disabled, report weight source as 0 */
> 1561 if (j == 1 && !(reg & 0x80))
> ^^^^^^
> So it can't be 1 here. Did you intend to say "i == 1"?
>
Nice catch. No, the condition does not depend on an index and should simply be
if (!(reg & 0x80))
No idea what if anything I was thinking when I wrote the code.
It would be great if you can send a patch.
Thanks,
Guenter
> 1562 data->pwm_weight_temp_sel[i] = 0;
> 1563
> 1564 /* Weight temp data */
> 1565 for (j = 0; j < ARRAY_SIZE(data->weight_temp); j++) {
> 1566 data->weight_temp[j][i]
> 1567 = nct6775_read_value(data,
> 1568 data->REG_WEIGHT_TEMP[j][i]);
> 1569 }
> 1570 }
> 1571 }
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] hwmon: (nct6775) Set weight source to zero correctly
2018-09-04 14:27 ` Guenter Roeck
@ 2018-09-05 7:46 ` Dan Carpenter
2018-09-05 7:46 ` Dan Carpenter
2018-09-05 7:57 ` [bug report] hwmon: (nct6775) Add support for weighted fan control Dan Carpenter
2 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2018-09-05 7:46 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Jean Delvare, linux-hwmon, kernel-janitors
This is dead code because j can never be 1 at this point. We had
intended to just test if the bit was clear.
Fixes: bbd8decd4123 ("hwmon: (nct6775) Add support for weighted fan control")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 944f5b63aecd..139781ae830b 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -1558,7 +1558,7 @@ static void nct6775_update_pwm(struct device *dev)
reg = nct6775_read_value(data, data->REG_WEIGHT_TEMP_SEL[i]);
data->pwm_weight_temp_sel[i] = reg & 0x1f;
/* If weight is disabled, report weight source as 0 */
- if (j == 1 && !(reg & 0x80))
+ if (!(reg & 0x80))
data->pwm_weight_temp_sel[i] = 0;
/* Weight temp data */
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/2] hwmon: (nct6775) Set weight source to zero correctly
@ 2018-09-05 7:46 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2018-09-05 7:46 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Jean Delvare, linux-hwmon, kernel-janitors
This is dead code because j can never be 1 at this point. We had
intended to just test if the bit was clear.
Fixes: bbd8decd4123 ("hwmon: (nct6775) Add support for weighted fan control")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 944f5b63aecd..139781ae830b 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -1558,7 +1558,7 @@ static void nct6775_update_pwm(struct device *dev)
reg = nct6775_read_value(data, data->REG_WEIGHT_TEMP_SEL[i]);
data->pwm_weight_temp_sel[i] = reg & 0x1f;
/* If weight is disabled, report weight source as 0 */
- if (j = 1 && !(reg & 0x80))
+ if (!(reg & 0x80))
data->pwm_weight_temp_sel[i] = 0;
/* Weight temp data */
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] hwmon: (nct6775) Clean up a condition
2018-09-04 14:27 ` Guenter Roeck
@ 2018-09-05 7:46 ` Dan Carpenter
2018-09-05 7:46 ` Dan Carpenter
2018-09-05 7:57 ` [bug report] hwmon: (nct6775) Add support for weighted fan control Dan Carpenter
2 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2018-09-05 7:46 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Jean Delvare, linux-hwmon, kernel-janitors
I removed the "dsw_en &&" chunk of the condition because we know that
"dsw_en" is set.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 139781ae830b..719effe53ceb 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -3533,8 +3533,7 @@ nct6775_check_fan_inputs(struct nct6775_data *data)
if (!fan6pin)
fan6pin = (regval_2a & BIT(4)) &&
- (!dsw_en ||
- (dsw_en && (regval_ed & BIT(4))));
+ (!dsw_en || (regval_ed & BIT(4)));
if (!pwm6pin)
pwm6pin = (regval_2a & BIT(3)) &&
(regval_ed & BIT(2));
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] hwmon: (nct6775) Clean up a condition
@ 2018-09-05 7:46 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2018-09-05 7:46 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Jean Delvare, linux-hwmon, kernel-janitors
I removed the "dsw_en &&" chunk of the condition because we know that
"dsw_en" is set.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 139781ae830b..719effe53ceb 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -3533,8 +3533,7 @@ nct6775_check_fan_inputs(struct nct6775_data *data)
if (!fan6pin)
fan6pin = (regval_2a & BIT(4)) &&
- (!dsw_en ||
- (dsw_en && (regval_ed & BIT(4))));
+ (!dsw_en || (regval_ed & BIT(4)));
if (!pwm6pin)
pwm6pin = (regval_2a & BIT(3)) &&
(regval_ed & BIT(2));
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [bug report] hwmon: (nct6775) Add support for weighted fan control
2018-09-04 14:27 ` Guenter Roeck
2018-09-05 7:46 ` Dan Carpenter
2018-09-05 7:46 ` Dan Carpenter
@ 2018-09-05 7:57 ` Dan Carpenter
2018-09-05 22:05 ` Guenter Roeck
2 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2018-09-05 7:57 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon
Done. Btw, I saw another that looks legit.
drivers/hwmon/nct6775.c:1687 nct6775_update_device()
error: buffer overflow 'data->FAN_PULSE_SHIFT' 6 <= 6
drivers/hwmon/nct6775.c
1667 data->in[i][2] = nct6775_read_value(data,
1668 data->REG_IN_MINMAX[1][i]);
1669 }
1670
1671 /* Measured fan speeds and limits */
1672 for (i = 0; i < ARRAY_SIZE(data->rpm); i++) {
^^^^^^^^^^^^^^^^^^^^^^^^
This is a 7 element array.
1673 u16 reg;
1674
1675 if (!(data->has_fan & BIT(i)))
^^^^^^^^^^^^^^^^^^^^^^
I probably wouldn't have reported this originally because Smatch is bad
at these checks. (I just haven't gotten around to it yet). But we do
have fan7pin so it looks like BIT(6) can be set.
1676 continue;
1677
1678 reg = nct6775_read_value(data, data->REG_FAN[i]);
1679 data->rpm[i] = data->fan_from_reg(reg,
1680 data->fan_div[i]);
1681
1682 if (data->has_fan_min & BIT(i))
1683 data->fan_min[i] = nct6775_read_value(data,
1684 data->REG_FAN_MIN[i]);
1685 data->fan_pulses[i] =
1686 (nct6775_read_value(data, data->REG_FAN_PULSES[i])
1687 >> data->FAN_PULSE_SHIFT[i]) & 0x03;
^^^^^^^^^^^^^^^^^^^^^^^^
FAN_PULSE_SHIFT is either 5 or 6 elements.
1688
1689 nct6775_select_fan_div(dev, data, i, reg);
1690 }
1691
1692 nct6775_update_pwm(dev);
1693 nct6775_update_pwm_limits(dev);
1694
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] hwmon: (nct6775) Set weight source to zero correctly
2018-09-05 7:46 ` Dan Carpenter
@ 2018-09-05 21:48 ` Guenter Roeck
-1 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2018-09-05 21:48 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Jean Delvare, linux-hwmon, kernel-janitors
On Wed, Sep 05, 2018 at 10:46:27AM +0300, Dan Carpenter wrote:
> This is dead code because j can never be 1 at this point. We had
> intended to just test if the bit was clear.
>
> Fixes: bbd8decd4123 ("hwmon: (nct6775) Add support for weighted fan control")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied.
Thanks,
Guenter
>
> diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
> index 944f5b63aecd..139781ae830b 100644
> --- a/drivers/hwmon/nct6775.c
> +++ b/drivers/hwmon/nct6775.c
> @@ -1558,7 +1558,7 @@ static void nct6775_update_pwm(struct device *dev)
> reg = nct6775_read_value(data, data->REG_WEIGHT_TEMP_SEL[i]);
> data->pwm_weight_temp_sel[i] = reg & 0x1f;
> /* If weight is disabled, report weight source as 0 */
> - if (j == 1 && !(reg & 0x80))
> + if (!(reg & 0x80))
> data->pwm_weight_temp_sel[i] = 0;
>
> /* Weight temp data */
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] hwmon: (nct6775) Set weight source to zero correctly
@ 2018-09-05 21:48 ` Guenter Roeck
0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2018-09-05 21:48 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Jean Delvare, linux-hwmon, kernel-janitors
On Wed, Sep 05, 2018 at 10:46:27AM +0300, Dan Carpenter wrote:
> This is dead code because j can never be 1 at this point. We had
> intended to just test if the bit was clear.
>
> Fixes: bbd8decd4123 ("hwmon: (nct6775) Add support for weighted fan control")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied.
Thanks,
Guenter
>
> diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
> index 944f5b63aecd..139781ae830b 100644
> --- a/drivers/hwmon/nct6775.c
> +++ b/drivers/hwmon/nct6775.c
> @@ -1558,7 +1558,7 @@ static void nct6775_update_pwm(struct device *dev)
> reg = nct6775_read_value(data, data->REG_WEIGHT_TEMP_SEL[i]);
> data->pwm_weight_temp_sel[i] = reg & 0x1f;
> /* If weight is disabled, report weight source as 0 */
> - if (j = 1 && !(reg & 0x80))
> + if (!(reg & 0x80))
> data->pwm_weight_temp_sel[i] = 0;
>
> /* Weight temp data */
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] hwmon: (nct6775) Clean up a condition
2018-09-05 7:46 ` Dan Carpenter
@ 2018-09-05 21:49 ` Guenter Roeck
-1 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2018-09-05 21:49 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Jean Delvare, linux-hwmon, kernel-janitors
On Wed, Sep 05, 2018 at 10:46:55AM +0300, Dan Carpenter wrote:
> I removed the "dsw_en &&" chunk of the condition because we know that
> "dsw_en" is set.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied to hwmon-next.
Thanks,
Guenter
>
> diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
> index 139781ae830b..719effe53ceb 100644
> --- a/drivers/hwmon/nct6775.c
> +++ b/drivers/hwmon/nct6775.c
> @@ -3533,8 +3533,7 @@ nct6775_check_fan_inputs(struct nct6775_data *data)
>
> if (!fan6pin)
> fan6pin = (regval_2a & BIT(4)) &&
> - (!dsw_en ||
> - (dsw_en && (regval_ed & BIT(4))));
> + (!dsw_en || (regval_ed & BIT(4)));
> if (!pwm6pin)
> pwm6pin = (regval_2a & BIT(3)) &&
> (regval_ed & BIT(2));
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] hwmon: (nct6775) Clean up a condition
@ 2018-09-05 21:49 ` Guenter Roeck
0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2018-09-05 21:49 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Jean Delvare, linux-hwmon, kernel-janitors
On Wed, Sep 05, 2018 at 10:46:55AM +0300, Dan Carpenter wrote:
> I removed the "dsw_en &&" chunk of the condition because we know that
> "dsw_en" is set.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied to hwmon-next.
Thanks,
Guenter
>
> diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
> index 139781ae830b..719effe53ceb 100644
> --- a/drivers/hwmon/nct6775.c
> +++ b/drivers/hwmon/nct6775.c
> @@ -3533,8 +3533,7 @@ nct6775_check_fan_inputs(struct nct6775_data *data)
>
> if (!fan6pin)
> fan6pin = (regval_2a & BIT(4)) &&
> - (!dsw_en ||
> - (dsw_en && (regval_ed & BIT(4))));
> + (!dsw_en || (regval_ed & BIT(4)));
> if (!pwm6pin)
> pwm6pin = (regval_2a & BIT(3)) &&
> (regval_ed & BIT(2));
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [bug report] hwmon: (nct6775) Add support for weighted fan control
2018-09-05 7:57 ` [bug report] hwmon: (nct6775) Add support for weighted fan control Dan Carpenter
@ 2018-09-05 22:05 ` Guenter Roeck
2018-09-06 6:50 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2018-09-05 22:05 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-hwmon
Hi Dan,
On 09/05/2018 12:57 AM, Dan Carpenter wrote:
> Done. Btw, I saw another that looks legit.
>
Thanks ...
> drivers/hwmon/nct6775.c:1687 nct6775_update_device()
> error: buffer overflow 'data->FAN_PULSE_SHIFT' 6 <= 6
>
> drivers/hwmon/nct6775.c
> 1667 data->in[i][2] = nct6775_read_value(data,
> 1668 data->REG_IN_MINMAX[1][i]);
> 1669 }
> 1670
> 1671 /* Measured fan speeds and limits */
> 1672 for (i = 0; i < ARRAY_SIZE(data->rpm); i++) {
> ^^^^^^^^^^^^^^^^^^^^^^^^
> This is a 7 element array.
>
> 1673 u16 reg;
> 1674
> 1675 if (!(data->has_fan & BIT(i)))
> ^^^^^^^^^^^^^^^^^^^^^^
>
> I probably wouldn't have reported this originally because Smatch is bad
> at these checks. (I just haven't gotten around to it yet). But we do
> have fan7pin so it looks like BIT(6) can be set.
>
> 1676 continue;
> 1677
> 1678 reg = nct6775_read_value(data, data->REG_FAN[i]);
> 1679 data->rpm[i] = data->fan_from_reg(reg,
> 1680 data->fan_div[i]);
> 1681
> 1682 if (data->has_fan_min & BIT(i))
> 1683 data->fan_min[i] = nct6775_read_value(data,
> 1684 data->REG_FAN_MIN[i]);
> 1685 data->fan_pulses[i] =
> 1686 (nct6775_read_value(data, data->REG_FAN_PULSES[i])
> 1687 >> data->FAN_PULSE_SHIFT[i]) & 0x03;
> ^^^^^^^^^^^^^^^^^^^^^^^^
> FAN_PULSE_SHIFT is either 5 or 6 elements.
>
Array size of xxx_REG_FAN_PULSES[] and xxx_FAN_PULSE_SHIFT[] should probably
be NUM_FAN to be on the safe side. There is a slight secondary problem, though:
data->REG_FAN_PULSES[i] should not be read if it is 0. It doesn't matter much -
it results in an unnecessary read from register 0 - but it is still undesirable.
Care to send another patch ?
Thanks,
Guenter
> 1688
> 1689 nct6775_select_fan_div(dev, data, i, reg);
> 1690 }
> 1691
> 1692 nct6775_update_pwm(dev);
> 1693 nct6775_update_pwm_limits(dev);
> 1694
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [bug report] hwmon: (nct6775) Add support for weighted fan control
2018-09-05 22:05 ` Guenter Roeck
@ 2018-09-06 6:50 ` Dan Carpenter
2018-09-06 13:13 ` Guenter Roeck
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2018-09-06 6:50 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon
On Wed, Sep 05, 2018 at 03:05:00PM -0700, Guenter Roeck wrote:
> Hi Dan,
>
> On 09/05/2018 12:57 AM, Dan Carpenter wrote:
> > Done. Btw, I saw another that looks legit.
> >
> Thanks ...
>
> > drivers/hwmon/nct6775.c:1687 nct6775_update_device()
> > error: buffer overflow 'data->FAN_PULSE_SHIFT' 6 <= 6
> >
> > drivers/hwmon/nct6775.c
> > 1667 data->in[i][2] = nct6775_read_value(data,
> > 1668 data->REG_IN_MINMAX[1][i]);
> > 1669 }
> > 1670
> > 1671 /* Measured fan speeds and limits */
> > 1672 for (i = 0; i < ARRAY_SIZE(data->rpm); i++) {
> > ^^^^^^^^^^^^^^^^^^^^^^^^
> > This is a 7 element array.
> >
> > 1673 u16 reg;
> > 1674
> > 1675 if (!(data->has_fan & BIT(i)))
> > ^^^^^^^^^^^^^^^^^^^^^^
> >
> > I probably wouldn't have reported this originally because Smatch is bad
> > at these checks. (I just haven't gotten around to it yet). But we do
> > have fan7pin so it looks like BIT(6) can be set.
> >
> > 1676 continue;
> > 1677
> > 1678 reg = nct6775_read_value(data, data->REG_FAN[i]);
> > 1679 data->rpm[i] = data->fan_from_reg(reg,
> > 1680 data->fan_div[i]);
> > 1681
> > 1682 if (data->has_fan_min & BIT(i))
> > 1683 data->fan_min[i] = nct6775_read_value(data,
> > 1684 data->REG_FAN_MIN[i]);
> > 1685 data->fan_pulses[i] =
> > 1686 (nct6775_read_value(data, data->REG_FAN_PULSES[i])
> > 1687 >> data->FAN_PULSE_SHIFT[i]) & 0x03;
> > ^^^^^^^^^^^^^^^^^^^^^^^^
> > FAN_PULSE_SHIFT is either 5 or 6 elements.
> >
>
> Array size of xxx_REG_FAN_PULSES[] and xxx_FAN_PULSE_SHIFT[] should probably
> be NUM_FAN to be on the safe side. There is a slight secondary problem, though:
> data->REG_FAN_PULSES[i] should not be read if it is 0. It doesn't matter much -
> it results in an unnecessary read from register 0 - but it is still undesirable.
>
> Care to send another patch ?
Can you do that one and give me a Reported-by tag? It seems slightly
more involved and I am dumb.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [bug report] hwmon: (nct6775) Add support for weighted fan control
2018-09-06 6:50 ` Dan Carpenter
@ 2018-09-06 13:13 ` Guenter Roeck
0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2018-09-06 13:13 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-hwmon
On 09/05/2018 11:50 PM, Dan Carpenter wrote:
> On Wed, Sep 05, 2018 at 03:05:00PM -0700, Guenter Roeck wrote:
>> Hi Dan,
>>
>> On 09/05/2018 12:57 AM, Dan Carpenter wrote:
>>> Done. Btw, I saw another that looks legit.
>>>
>> Thanks ...
>>
>>> drivers/hwmon/nct6775.c:1687 nct6775_update_device()
>>> error: buffer overflow 'data->FAN_PULSE_SHIFT' 6 <= 6
>>>
>>> drivers/hwmon/nct6775.c
>>> 1667 data->in[i][2] = nct6775_read_value(data,
>>> 1668 data->REG_IN_MINMAX[1][i]);
>>> 1669 }
>>> 1670
>>> 1671 /* Measured fan speeds and limits */
>>> 1672 for (i = 0; i < ARRAY_SIZE(data->rpm); i++) {
>>> ^^^^^^^^^^^^^^^^^^^^^^^^
>>> This is a 7 element array.
>>>
>>> 1673 u16 reg;
>>> 1674
>>> 1675 if (!(data->has_fan & BIT(i)))
>>> ^^^^^^^^^^^^^^^^^^^^^^
>>>
>>> I probably wouldn't have reported this originally because Smatch is bad
>>> at these checks. (I just haven't gotten around to it yet). But we do
>>> have fan7pin so it looks like BIT(6) can be set.
>>>
>>> 1676 continue;
>>> 1677
>>> 1678 reg = nct6775_read_value(data, data->REG_FAN[i]);
>>> 1679 data->rpm[i] = data->fan_from_reg(reg,
>>> 1680 data->fan_div[i]);
>>> 1681
>>> 1682 if (data->has_fan_min & BIT(i))
>>> 1683 data->fan_min[i] = nct6775_read_value(data,
>>> 1684 data->REG_FAN_MIN[i]);
>>> 1685 data->fan_pulses[i] =
>>> 1686 (nct6775_read_value(data, data->REG_FAN_PULSES[i])
>>> 1687 >> data->FAN_PULSE_SHIFT[i]) & 0x03;
>>> ^^^^^^^^^^^^^^^^^^^^^^^^
>>> FAN_PULSE_SHIFT is either 5 or 6 elements.
>>>
>>
>> Array size of xxx_REG_FAN_PULSES[] and xxx_FAN_PULSE_SHIFT[] should probably
>> be NUM_FAN to be on the safe side. There is a slight secondary problem, though:
>> data->REG_FAN_PULSES[i] should not be read if it is 0. It doesn't matter much -
>> it results in an unnecessary read from register 0 - but it is still undesirable.
>>
>> Care to send another patch ?
>
> Can you do that one and give me a Reported-by tag? It seems slightly
> more involved and I am dumb.
>
Sure, NP.
Guenter
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-09-06 17:48 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-04 12:40 [bug report] hwmon: (nct6775) Add support for weighted fan control Dan Carpenter
2018-09-04 14:27 ` Guenter Roeck
2018-09-05 7:46 ` [PATCH 1/2] hwmon: (nct6775) Set weight source to zero correctly Dan Carpenter
2018-09-05 7:46 ` Dan Carpenter
2018-09-05 21:48 ` Guenter Roeck
2018-09-05 21:48 ` Guenter Roeck
2018-09-05 7:46 ` [PATCH 2/2] hwmon: (nct6775) Clean up a condition Dan Carpenter
2018-09-05 7:46 ` Dan Carpenter
2018-09-05 21:49 ` Guenter Roeck
2018-09-05 21:49 ` Guenter Roeck
2018-09-05 7:57 ` [bug report] hwmon: (nct6775) Add support for weighted fan control Dan Carpenter
2018-09-05 22:05 ` Guenter Roeck
2018-09-06 6:50 ` Dan Carpenter
2018-09-06 13:13 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.