Re: [PATCH 2/2] media: drxk_hard: check if parameter is not NULL
On Fri, Dec 7, 2018 at 5:08 AM Mauro Carvalho Chehab
wrote:
>
> There is a smatch warning:
> drivers/media/dvb-frontends/drxk_hard.c:
> drivers/media/dvb-frontends/drxk_hard.c:1478 scu_command() error: we
> previously assumed 'parameter' could be null (see line 1467)
>
> Telling that parameter might be NULL. Well, it can't, due to the
> way the driver works, but it doesn't hurt to add a check, in order
> to shut up smatch.
eh, yeah this function is kind of odd; the early return conditions are
a little tricky, but I agree that this check doesn't hurt to add.
Reviewed-by: Nick Desaulniers
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/dvb-frontends/drxk_hard.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/drxk_hard.c
> b/drivers/media/dvb-frontends/drxk_hard.c
> index 84ac3f73f8fe..8ea1e45be710 100644
> --- a/drivers/media/dvb-frontends/drxk_hard.c
> +++ b/drivers/media/dvb-frontends/drxk_hard.c
> @@ -1474,9 +1474,11 @@ static int scu_command(struct drxk_state *state,
>
> /* assume that the command register is ready
> since it is checked afterwards */
> - for (ii = parameter_len - 1; ii >= 0; ii -= 1) {
> - buffer[cnt++] = (parameter[ii] & 0xFF);
> - buffer[cnt++] = ((parameter[ii] >> 8) & 0xFF);
> + if (parameter) {
> + for (ii = parameter_len - 1; ii >= 0; ii -= 1) {
> + buffer[cnt++] = (parameter[ii] & 0xFF);
> + buffer[cnt++] = ((parameter[ii] >> 8) & 0xFF);
> + }
> }
> buffer[cnt++] = (cmd & 0xFF);
> buffer[cnt++] = ((cmd >> 8) & 0xFF);
> --
> 2.19.2
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH 2/3] media: drxj: get rid of uneeded casts
On Tue, Aug 7, 2018 at 5:26 AM Mauro Carvalho Chehab
wrote:
>
> Instead of doing casts, use %zd to print sizes, in order to make
> smatch happier:
> drivers/media/dvb-frontends/drx39xyj/drxj.c:11814 drx_ctrl_u_code()
> warn: argument 4 to %u specifier is cast from pointer
> drivers/media/dvb-frontends/drx39xyj/drxj.c:11845 drx_ctrl_u_code()
> warn: argument 3 to %u specifier is cast from pointer
> drivers/media/dvb-frontends/drx39xyj/drxj.c:11869 drx_ctrl_u_code()
> warn: argument 3 to %u specifier is cast from pointer
> drivers/media/dvb-frontends/drx39xyj/drxj.c:11878 drx_ctrl_u_code()
> warn: argument 3 to %u specifier is cast from pointer
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/dvb-frontends/drx39xyj/drxj.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/drx39xyj/drxj.c
> b/drivers/media/dvb-frontends/drx39xyj/drxj.c
> index 2948d12d7c14..9628d4067fe1 100644
> --- a/drivers/media/dvb-frontends/drx39xyj/drxj.c
> +++ b/drivers/media/dvb-frontends/drx39xyj/drxj.c
> @@ -11810,8 +11810,8 @@ static int drx_ctrl_u_code(struct drx_demod_instance
> *demod,
> block_hdr.CRC = be16_to_cpu(*(__be16 *)(mc_data));
> mc_data += sizeof(u16);
>
> - pr_debug("%u: addr %u, size %u, flags 0x%04x, CRC 0x%04x\n",
> - (unsigned)(mc_data - mc_data_init), block_hdr.addr,
> + pr_debug("%zd: addr %u, size %u, flags 0x%04x, CRC 0x%04x\n",
> + (mc_data - mc_data_init), block_hdr.addr,
> block_hdr.size, block_hdr.flags, block_hdr.CRC);
>
> /* Check block header on:
> @@ -11841,8 +11841,8 @@ static int drx_ctrl_u_code(struct drx_demod_instance
> *demod,
> mc_block_nr_bytes,
> mc_data, 0x)) {
> rc = -EIO;
> - pr_err("error writing firmware at pos %u\n",
> - (unsigned)(mc_data - mc_data_init));
> + pr_err("error writing firmware at pos %zd\n",
> + mc_data - mc_data_init);
> goto release;
> }
> break;
> @@ -11865,8 +11865,8 @@ static int drx_ctrl_u_code(struct drx_demod_instance
> *demod,
> (u16)bytes_to_comp,
> (u8 *)mc_data_buffer,
> 0x)) {
> - pr_err("error reading firmware at pos
> %u\n",
> - (unsigned)(mc_data -
> mc_data_init));
> + pr_err("error reading firmware at pos
> %zd\n",
> + mc_data - mc_data_init);
> return -EIO;
> }
>
> @@ -11874,8 +11874,8 @@ static int drx_ctrl_u_code(struct drx_demod_instance
> *demod,
> bytes_to_comp);
>
> if (result) {
> - pr_err("error verifying firmware at
> pos %u\n",
> - (unsigned)(mc_data -
> mc_data_init));
> + pr_err("error verifying firmware at
> pos %zd\n",
> + mc_data - mc_data_init);
> return -EIO;
> }
>
> --
> 2.17.1
>
>From Documentation/printk-formats.txt, it looks like %zd is for
ssize_t, which is what I would expect for pointer subtracting (well
maybe intptr_t, but ssize_t should be word-size-independent).
Reviewed-by: Nick Desaulniers
--
Thanks,
~Nick Desaulniers
Re: [PATCH] media: cleanup fall-through comments
On Tue, Aug 7, 2018 at 9:44 AM Greg KH wrote:
>
> On Tue, Aug 07, 2018 at 09:33:03AM -0700, Nick Desaulniers wrote:
> > On Tue, Aug 7, 2018 at 5:07 AM Mauro Carvalho Chehab
> > wrote:
> > >
> > > As Ian pointed out, adding a '-' to the fallthrough seems to meet
> > > the regex requirements at level 3 of the warning, at least when
> > > the comment fits into a single line.
> > >
> > > So, replace by a single line the comments that were broken into
> > > multiple lines just to make gcc -Wimplicit-fallthrough=3 happy.
> > >
> > > Suggested-by: Ian Arkver
> > > Signed-off-by: Mauro Carvalho Chehab
> > > ---
> > > drivers/media/dvb-frontends/drx39xyj/drxj.c | 3 +--
> > > drivers/media/dvb-frontends/drxd_hard.c | 6 ++
> > > drivers/media/dvb-frontends/drxk_hard.c | 18 ++
> > > drivers/staging/media/imx/imx-media-csi.c | 3 +--
> > > 4 files changed, 10 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/media/dvb-frontends/drx39xyj/drxj.c
> > > b/drivers/media/dvb-frontends/drx39xyj/drxj.c
> > > index 2ddb7d218ace..2948d12d7c14 100644
> > > --- a/drivers/media/dvb-frontends/drx39xyj/drxj.c
> > > +++ b/drivers/media/dvb-frontends/drx39xyj/drxj.c
> > > @@ -2841,8 +2841,7 @@ ctrl_set_cfg_mpeg_output(struct drx_demod_instance
> > > *demod, struct drx_cfg_mpeg_o
> > > /* coef = 188/204 */
> > > max_bit_rate =
> > > (ext_attr->curr_symbol_rate / 8) * nr_bits *
> > > 188;
> > > - /* pass through as b/c Annex A/c need following
> > > settings */
> > > - /* fall-through */
> > > + /* fall-through - as b/c Annex A/C need following
> > > settings */
> > > case DRX_STANDARD_ITU_B:
> > > rc = drxj_dap_write_reg16(dev_addr,
> > > FEC_OC_FCT_USAGE__A, FEC_OC_FCT_USAGE__PRE, 0);
> > > if (rc != 0) {
> > > diff --git a/drivers/media/dvb-frontends/drxd_hard.c
> > > b/drivers/media/dvb-frontends/drxd_hard.c
> > > index 11fc259e4383..684d428efb0d 100644
> > > --- a/drivers/media/dvb-frontends/drxd_hard.c
> > > +++ b/drivers/media/dvb-frontends/drxd_hard.c
> > > @@ -1970,8 +1970,7 @@ static int DRX_Start(struct drxd_state *state, s32
> > > off)
> > > switch (p->transmission_mode) {
> > > default:/* Not set, detect it automatically */
> > > operationMode |= SC_RA_RAM_OP_AUTO_MODE__M;
> > > - /* try first guess DRX_FFTMODE_8K */
> > > - /* fall through */
> > > + /* fall through - try first guess DRX_FFTMODE_8K
> > > */
> > > case TRANSMISSION_MODE_8K:
> > > transmissionParams |= SC_RA_RAM_OP_PARAM_MODE_8K;
> > > if (state->type_A) {
> > > @@ -2144,8 +2143,7 @@ static int DRX_Start(struct drxd_state *state, s32
> > > off)
> > > switch (p->modulation) {
> > > default:
> > > operationMode |= SC_RA_RAM_OP_AUTO_CONST__M;
> > > - /* try first guess DRX_CONSTELLATION_QAM64 */
> > > - /* fall through */
> > > + /* fall through - try first guess
> > > DRX_CONSTELLATION_QAM64 */
> > > case QAM_64:
> > > transmissionParams |=
> > > SC_RA_RAM_OP_PARAM_CONST_QAM64;
> > > if (state->type_A) {
> > > diff --git a/drivers/media/dvb-frontends/drxk_hard.c
> > > b/drivers/media/dvb-frontends/drxk_hard.c
> > > index ac10781d3550..f1886945a7bc 100644
> > > --- a/drivers/media/dvb-frontends/drxk_hard.c
> > > +++ b/drivers/media/dvb-frontends/drxk_hard.c
> > > @@ -3270,13 +3270,11 @@ static int dvbt_sc_command(struct drxk_state
> > > *state,
> > > case OFDM_SC_RA_RAM_CMD_SET_PREF_PARAM:
> > > case OFDM_SC_RA_RAM_CMD_PROGRAM_PARAM:
> > > status |= write16(state, OFDM_SC_RA_RAM_PARAM1__A,
> > > param1);
> > > - /* All commands using 1 parameters */
> > > - /* fall through */
Re: [PATCH] media: cleanup fall-through comments
gt; case TRANSMISSION_MODE_8K: > transmission_params |= OFDM_SC_RA_RAM_OP_PARAM_MODE_8K; > break; > @@ -3799,8 +3796,7 @@ static int set_dvbt(struct drxk_state *state, u16 > intermediate_freqk_hz, > default: > case GUARD_INTERVAL_AUTO: > operation_mode |= OFDM_SC_RA_RAM_OP_AUTO_GUARD__M; > - /* try first guess DRX_GUARD_1DIV4 */ > - /* fall through */ > + /* fall through - try first guess DRX_GUARD_1DIV4 */ > case GUARD_INTERVAL_1_4: > transmission_params |= OFDM_SC_RA_RAM_OP_PARAM_GUARD_4; > break; > @@ -3841,8 +3837,7 @@ static int set_dvbt(struct drxk_state *state, u16 > intermediate_freqk_hz, > case QAM_AUTO: > default: > operation_mode |= OFDM_SC_RA_RAM_OP_AUTO_CONST__M; > - /* try first guess DRX_CONSTELLATION_QAM64 */ > - /* fall through */ > + /* fall through - try first guess DRX_CONSTELLATION_QAM64 */ > case QAM_64: > transmission_params |= OFDM_SC_RA_RAM_OP_PARAM_CONST_QAM64; > break; > @@ -3885,8 +3880,7 @@ static int set_dvbt(struct drxk_state *state, u16 > intermediate_freqk_hz, > case FEC_AUTO: > default: > operation_mode |= OFDM_SC_RA_RAM_OP_AUTO_RATE__M; > - /* try first guess DRX_CODERATE_2DIV3 */ > - /* fall through */ > + /* fall through - try first guess DRX_CODERATE_2DIV3 */ > case FEC_2_3: > transmission_params |= OFDM_SC_RA_RAM_OP_PARAM_RATE_2_3; > break; > diff --git a/drivers/staging/media/imx/imx-media-csi.c > b/drivers/staging/media/imx/imx-media-csi.c > index b7ffd231c64b..cd2c291e1e94 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -460,8 +460,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) > passthrough_cycles = incc->cycles; > break; > } > - /* for non-passthrough RGB565 (CSI-2 bus) */ > - /* Falls through */ > + /* fallthrough - non-passthrough RGB565 (CSI-2 bus) */ > default: > burst_size = (image.pix.width & 0xf) ? 8 : 16; > passthrough_bits = 16; > -- > 2.17.1 > Can we use the compiler attribute: __attribute__((fallthrough)) rather than specifically formatted comments? The thing to be careful about with attributes is "what is the first version of the compiler that implements these?" https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7/ seems to indicate that the comment parsing was implemented in gcc-7 (and also shows the compiler attribute being used)? -- Thanks, ~Nick Desaulniers
[PATCH] [media] dvb-frontends: remove extraneous parens
Fixes 2 warnings from Clang about extra parentheses in a conditional,
that might have been meant as assignment.
Signed-off-by: Nick Desaulniers <ndesaulni...@google.com>
---
drivers/media/dvb-frontends/drx39xyj/drxj.c | 2 +-
drivers/media/dvb-frontends/drxk_hard.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/dvb-frontends/drx39xyj/drxj.c
b/drivers/media/dvb-frontends/drx39xyj/drxj.c
index 8cbd8cc21059..cf00a34ef0fc 100644
--- a/drivers/media/dvb-frontends/drx39xyj/drxj.c
+++ b/drivers/media/dvb-frontends/drx39xyj/drxj.c
@@ -11074,7 +11074,7 @@ ctrl_power_mode(struct drx_demod_instance *demod, enum
drx_power_mode *mode)
}
}
- if ((*mode == DRX_POWER_UP)) {
+ if (*mode == DRX_POWER_UP) {
/* Restore analog & pin configuration */
/* Initialize default AFE configuration for VSB */
diff --git a/drivers/media/dvb-frontends/drxk_hard.c
b/drivers/media/dvb-frontends/drxk_hard.c
index f59ac2e91c59..19cc84c69b3b 100644
--- a/drivers/media/dvb-frontends/drxk_hard.c
+++ b/drivers/media/dvb-frontends/drxk_hard.c
@@ -6062,7 +6062,7 @@ static int init_drxk(struct drxk_state *state)
u16 driver_version;
dprintk(1, "\n");
- if ((state->m_drxk_state == DRXK_UNINITIALIZED)) {
+ if (state->m_drxk_state == DRXK_UNINITIALIZED) {
drxk_i2c_lock(state);
status = power_up_device(state);
if (status < 0)
--
2.15.1.504.g5279b80103-goog
[PATCH] [media] dvb-frontends: remove self assignments
These were leftover from:
commit 469ffe083665 ("[media] tda18271c2dd: Remove the CHK_ERROR macro")
and
commit 58d5eaec9f87 ("[media] drxd: Don't use a macro for CHK_ERROR ...")
that programmatically removed the CHK_ERROR macro, which left behind a
few self assignments that Clang warns about. These instances aren't
errors.
Signed-off-by: Nick Desaulniers <ndesaulni...@google.com>
---
drivers/media/dvb-frontends/drxd_hard.c| 3 ---
drivers/media/dvb-frontends/tda18271c2dd.c | 1 -
2 files changed, 4 deletions(-)
diff --git a/drivers/media/dvb-frontends/drxd_hard.c
b/drivers/media/dvb-frontends/drxd_hard.c
index 0696bc62dcc9..ff18a0f7dc41 100644
--- a/drivers/media/dvb-frontends/drxd_hard.c
+++ b/drivers/media/dvb-frontends/drxd_hard.c
@@ -2140,7 +2140,6 @@ static int DRX_Start(struct drxd_state *state, s32 off)
}
break;
}
- status = status;
if (status < 0)
break;
@@ -2251,7 +2250,6 @@ static int DRX_Start(struct drxd_state *state, s32 off)
break;
}
- status = status;
if (status < 0)
break;
@@ -2318,7 +2316,6 @@ static int DRX_Start(struct drxd_state *state, s32 off)
}
break;
}
- status = status;
if (status < 0)
break;
diff --git a/drivers/media/dvb-frontends/tda18271c2dd.c
b/drivers/media/dvb-frontends/tda18271c2dd.c
index 2d2778be2d2f..45cd5ba0cf8a 100644
--- a/drivers/media/dvb-frontends/tda18271c2dd.c
+++ b/drivers/media/dvb-frontends/tda18271c2dd.c
@@ -674,7 +674,6 @@ static int PowerScan(struct tda_state *state,
Count = 20;
wait = true;
}
- status = status;
if (status < 0)
break;
if (CID_Gain >= CID_Target) {
--
2.15.1.504.g5279b80103-goog
