Hello everybody,
While looking into Coverity ID 1227035 I ran into the following piece
of code at drivers/media/dvb-frontends/af9013.c:595:
595 /* program CFOE coefficients */
596 if (c->bandwidth_hz != state->bandwidth_hz) {
597 for (i = 0; i < ARRAY_SIZE(coeff_lut); i++) {
598 if (coeff_lut[i].clock == state->config.clock &&
599 coeff_lut[i].bandwidth_hz ==
c->bandwidth_hz) {
600 break;
601 }
602 }
603
604 /* Return an error if can't find bandwidth or the
right clock */
605 if (i == ARRAY_SIZE(coeff_lut))
606 return -EINVAL;
608 ret = af9013_wr_regs(state, 0xae00, coeff_lut[i].val,
609 sizeof(coeff_lut[i].val));
610 }
611
612 /* program frequency control */
613 if (c->bandwidth_hz != state->bandwidth_hz || state->first_tune) {
614 /* get used IF frequency */
615 if (fe->ops.tuner_ops.get_if_frequency)
616 fe->ops.tuner_ops.get_if_frequency(fe,
&if_frequency);
617 else
618 if_frequency = state->config.if_frequency;
619
620 dev_dbg(&state->i2c->dev, "%s: if_frequency=%d\n",
621 __func__, if_frequency);
622
623 sampling_freq = if_frequency;
624
625 while (sampling_freq > (state->config.clock / 2))
626 sampling_freq -= state->config.clock;
627
628 if (sampling_freq < 0) {
629 sampling_freq *= -1;
630 spec_inv = state->config.spec_inv;
631 } else {
632 spec_inv = !state->config.spec_inv;
633 }
634
635 freq_cw = af9013_div(state, sampling_freq,
state->config.clock,
636 23);
637
638 if (spec_inv)
639 freq_cw = 0x800000 - freq_cw;
640
641 buf[0] = (freq_cw >> 0) & 0xff;
642 buf[1] = (freq_cw >> 8) & 0xff;
643 buf[2] = (freq_cw >> 16) & 0x7f;
644
645 freq_cw = 0x800000 - freq_cw;
646
647 buf[3] = (freq_cw >> 0) & 0xff;
648 buf[4] = (freq_cw >> 8) & 0xff;
649 buf[5] = (freq_cw >> 16) & 0x7f;
650
651 ret = af9013_wr_regs(state, 0xd140, buf, 3);
652 if (ret)
653 goto err;
654
655 ret = af9013_wr_regs(state, 0x9be7, buf, 6);
656 if (ret)
657 goto err;
658 }
659
660 /* clear TPS lock flag */
661 ret = af9013_wr_reg_bits(state, 0xd330, 3, 1, 1);
662 if (ret)
663 goto err;
664
665 /* clear MPEG2 lock flag */
666 ret = af9013_wr_reg_bits(state, 0xd507, 6, 1, 0);
667 if (ret)
668 goto err;
669
670 /* empty channel function */
671 ret = af9013_wr_reg_bits(state, 0x9bfe, 0, 1, 0);
672 if (ret)
673 goto err;
674
675 /* empty DVB-T channel function */
676 ret = af9013_wr_reg_bits(state, 0x9bc2, 0, 1, 0);
677 if (ret)
678 goto err;
679
The issue here is that the value stored in variable _ret_ at line 608,
is not being evaluated as it happens at line 662, 667, 672 and 677.
Then after looking into function af9013_wr_regs(), I noticed that this
function always returns zero, no matter what, as you can see below:
121static int af9013_wr_regs(struct af9013_state *priv, u16 reg, const
u8 *val,
122 int len)
123{
124 int ret, i;
125 u8 mbox = (0 << 7)|(0 << 6)|(1 << 1)|(1 << 0);
126
127 if ((priv->config.ts_mode == AF9013_TS_USB) &&
128 ((reg & 0xff00) != 0xff00) && ((reg & 0xff00) != 0xae00)) {
129 mbox |= ((len - 1) << 2);
130 ret = af9013_wr_regs_i2c(priv, mbox, reg, val, len);
131 } else {
132 for (i = 0; i < len; i++) {
133 ret = af9013_wr_regs_i2c(priv, mbox, reg+i,
val+i, 1);
134 if (ret)
135 goto err;
136 }
137 }
138
139err:
140 return 0;
141}
So I am wondering if such function is really intended to ignore the
return value of af9013_wr_regs_i2c()?
If that is the case maybe it should be refactored as follows:
--- a/drivers/media/dvb-frontends/af9013.c
+++ b/drivers/media/dvb-frontends/af9013.c
@@ -118,26 +118,21 @@ static int af9013_rd_regs_i2c(struct
af9013_state *priv, u8 mbox, u16 reg,
}
/* write multiple registers */
-static int af9013_wr_regs(struct af9013_state *priv, u16 reg, const u8 *val,
+static void af9013_wr_regs(struct af9013_state *priv, u16 reg, const u8 *val,
int len)
{
- int ret, i;
+ int i;
u8 mbox = (0 << 7)|(0 << 6)|(1 << 1)|(1 << 0);
if ((priv->config.ts_mode == AF9013_TS_USB) &&
((reg & 0xff00) != 0xff00) && ((reg & 0xff00) != 0xae00)) {
mbox |= ((len - 1) << 2);
- ret = af9013_wr_regs_i2c(priv, mbox, reg, val, len);
+ af9013_wr_regs_i2c(priv, mbox, reg, val, len);
} else {
for (i = 0; i < len; i++) {
- ret = af9013_wr_regs_i2c(priv, mbox, reg+i, val+i, 1);
- if (ret)
- goto err;
+ af9013_wr_regs_i2c(priv, mbox, reg+i, val+i, 1);
}
}
-
-err:
- return 0;
}
and of course, all of its callers also should be refactored
accordinly. Otherwise, the value contained in variable _ret_ should
be properly returned.
I can do the refactoring but first it would be great to hear your
opinions on this.
I'd really appreciate any comment on this.
Thank you!
--
Gustavo A. R. Silva
Gustavo A. R. Silva kirjoitti 2017-06-09 00:51:
> Hello everybody,
>
> While looking into Coverity ID 1227035 I ran into the following piece
> of code at drivers/media/dvb-frontends/af9013.c:595:
> The issue here is that the value stored in variable _ret_ at line 608,
> is not being evaluated as it happens at line 662, 667, 672 and 677.
> Then after looking into function af9013_wr_regs(), I noticed that this
> function always returns zero, no matter what, as you can see below:
>
> 121static int af9013_wr_regs(struct af9013_state *priv, u16 reg, const
> u8 *val,
> 122 int len)
> 123{
> 124 int ret, i;
> 125 u8 mbox = (0 << 7)|(0 << 6)|(1 << 1)|(1 << 0);
> 126
> 127 if ((priv->config.ts_mode == AF9013_TS_USB) &&
> 128 ((reg & 0xff00) != 0xff00) && ((reg & 0xff00) !=
> 0xae00)) {
> 129 mbox |= ((len - 1) << 2);
> 130 ret = af9013_wr_regs_i2c(priv, mbox, reg, val, len);
> 131 } else {
> 132 for (i = 0; i < len; i++) {
> 133 ret = af9013_wr_regs_i2c(priv, mbox, reg+i,
> val+i, 1);
> 134 if (ret)
> 135 goto err;
> 136 }
> 137 }
> 138
> 139err:
> 140 return 0;
> 141}
That function should return error code on error case, not zero always.
regards
Antti
--
http://palosaari.fi/