NACK: [PATCH] [media] rainshadow-cec: ensure exit_loop is initialized
On 19/05/17 18:39, Colin King wrote: > From: Colin Ian King > > exit_loop is not being initialized, so it contains garbage. Ensure it is > initialized to false. > > Detected by CoverityScan, CID#1436409 ("Uninitialzed scalar variable") > > Fixes: ea6a69defd3311 ("[media] rainshadow-cec: avoid -Wmaybe-uninitialized > warning") > Signed-off-by: Colin Ian King > --- > drivers/gpu/drm/vc4/vc4_v3d.c | 2 +- > drivers/media/usb/rainshadow-cec/rainshadow-cec.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c > index c53afec34586..c42210203f6e 100644 > --- a/drivers/gpu/drm/vc4/vc4_v3d.c > +++ b/drivers/gpu/drm/vc4/vc4_v3d.c > @@ -218,7 +218,7 @@ int vc4_v3d_get_bin_slot(struct vc4_dev *vc4) > * overall CMA pool before they make scenes complicated enough to run > * out of bin space. > */ > -int > +static int > vc4_allocate_bin_bo(struct drm_device *drm) > { > struct vc4_dev *vc4 = to_vc4_dev(drm); > diff --git a/drivers/media/usb/rainshadow-cec/rainshadow-cec.c > b/drivers/media/usb/rainshadow-cec/rainshadow-cec.c > index 8d3ca2c8b20f..ad468efc4399 100644 > --- a/drivers/media/usb/rainshadow-cec/rainshadow-cec.c > +++ b/drivers/media/usb/rainshadow-cec/rainshadow-cec.c > @@ -119,7 +119,7 @@ static void rain_irq_work_handler(struct work_struct > *work) > > while (true) { > unsigned long flags; > - bool exit_loop; > + bool exit_loop = false; > char data; > > spin_lock_irqsave(&rain->buf_lock, flags); > Sorry, got another fix included into that. I'll re-submit
Re: [PATCH] staging: fbtft: make const array gamma_par_mask static
On 11/07/17 18:30, Greg Kroah-Hartman wrote: > On Tue, Jul 11, 2017 at 06:20:02PM +0100, Colin King wrote: >> From: Colin Ian King >> >> Don't populate array gamma_par_mask on the stack but instead make it >> static. Makes the object code smaller by 148 bytes: >> >> Before: >>text data bss dec hex filename >>2993 1104 040971001 >> drivers/staging/fbtft/fb_st7789v.o >> >> After: >>text data bss dec hex filename >>2757 1192 0 3949 f6d >> drivers/staging/fbtft/fb_st7789v.o >> >> Signed-off-by: Colin Ian King >> --- >> drivers/media/usb/gspca/xirlink_cit.c | 2 +- >> drivers/staging/fbtft/fb_st7789v.c| 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) > > Your subject doesn't match the patch :( Got distracted by the Trump Jnr tweet. Will resend. > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Re: [PATCH] dvb_frontend: initialize variable s with FE_NONE instead of 0
On 21/07/17 19:05, Shuah Khan wrote: > On 07/21/2017 10:01 AM, Colin King wrote: >> From: Colin Ian King >> >> In a previous commit, we added FE_NONE as an unknown fe_status. >> Initialize variable s to FE_NONE instead of the more opaque value 0. >> >> Signed-off-by: Colin Ian King > > The change looks good to me. > Reviewed-by: Shuah Khan > > I think this patch should be part of a patch series that includes > the uAPI Documentation change, uAPI change, and the following > patch: > > [PATCH][V2] dvb_frontend: ensure that inital front end status initialized > > based on Mauro's review comments. Anyway, I will leave it to Mauro to > decide how he wants the patches split and if he is okay with uAPI change. > OK, I'm off on vacation very shortly, so I won't be able to respond for any further resubmissions for a couple of weeks. > thanks, > -- Shuah > >> --- >> drivers/media/dvb-core/dvb_frontend.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/media/dvb-core/dvb_frontend.c >> b/drivers/media/dvb-core/dvb_frontend.c >> index 18cc3bbc699c..114994ca0929 100644 >> --- a/drivers/media/dvb-core/dvb_frontend.c >> +++ b/drivers/media/dvb-core/dvb_frontend.c >> @@ -460,7 +460,7 @@ static int dvb_frontend_swzigzag_autotune(struct >> dvb_frontend *fe, int check_wra >> >> static void dvb_frontend_swzigzag(struct dvb_frontend *fe) >> { >> -enum fe_status s = 0; >> +enum fe_status s = FE_NONE; >> int retval = 0; >> struct dvb_frontend_private *fepriv = fe->frontend_priv; >> struct dtv_frontend_properties *c = &fe->dtv_property_cache, tmp; >> >
Re: [PATCH] staging: davinci: drop pointless static qualifier in vpfe_resizer_init()
On 11/03/2019 14:07, Dan Carpenter wrote: > On Mon, Mar 11, 2019 at 10:14:05PM +0800, Mao Wenan wrote: >> There is no need to have the 'T *v' variable static >> since new value always be assigned before use it. >> >> Signed-off-by: Mao Wenan >> --- >> drivers/staging/media/davinci_vpfe/dm365_resizer.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/staging/media/davinci_vpfe/dm365_resizer.c >> b/drivers/staging/media/davinci_vpfe/dm365_resizer.c >> index 6098f43ac51b..a2a672d4615d 100644 >> --- a/drivers/staging/media/davinci_vpfe/dm365_resizer.c >> +++ b/drivers/staging/media/davinci_vpfe/dm365_resizer.c >> @@ -1881,7 +1881,7 @@ int vpfe_resizer_init(struct vpfe_resizer_device >> *vpfe_rsz, >> struct v4l2_subdev *sd = &vpfe_rsz->crop_resizer.subdev; >> struct media_pad *pads = &vpfe_rsz->crop_resizer.pads[0]; >> struct media_entity *me = &sd->entity; >> -static resource_size_t res_len; >> +resource_size_t res_len; > ^ > Could you remove the extra space character also, please. shouldn't checkpatch find these? > >> struct resource *res; >> int ret; > > regards, > dan carpenter > >
Re: [PATCH][next] media: staging: intel-ipu3: fix unsigned comparison with < 0
ping? On 22/12/2018 11:49, Colin King wrote: > From: Colin Ian King > > The comparison css->pipes[pipe].bindex < 0 is always false because > bindex is an unsigned int. Fix this by using a signed integer for > the comparison. > > Detected by CoverityScan, CID#1476023 ("Unsigned compared against 0") > > Fixes: f5f2e4273518 ("media: staging/intel-ipu3: Add css pipeline > programming") > Signed-off-by: Colin Ian King > --- > drivers/staging/media/ipu3/ipu3-css.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/ipu3/ipu3-css.c > b/drivers/staging/media/ipu3/ipu3-css.c > index 44c55639389a..b9354d2bb692 100644 > --- a/drivers/staging/media/ipu3/ipu3-css.c > +++ b/drivers/staging/media/ipu3/ipu3-css.c > @@ -1751,7 +1751,7 @@ int ipu3_css_fmt_try(struct ipu3_css *css, > &q[IPU3_CSS_QUEUE_OUT].fmt.mpix; > struct v4l2_pix_format_mplane *const vf = > &q[IPU3_CSS_QUEUE_VF].fmt.mpix; > - int i, s; > + int i, s, ret; > > /* Adjust all formats, get statistics buffer sizes and formats */ > for (i = 0; i < IPU3_CSS_QUEUES; i++) { > @@ -1826,12 +1826,12 @@ int ipu3_css_fmt_try(struct ipu3_css *css, > s = (bds->height - gdc->height) / 2 - FILTER_SIZE; > env->height = s < MIN_ENVELOPE ? MIN_ENVELOPE : s; > > - css->pipes[pipe].bindex = > - ipu3_css_find_binary(css, pipe, q, r); > - if (css->pipes[pipe].bindex < 0) { > + ret = ipu3_css_find_binary(css, pipe, q, r); > + if (ret < 0) { > dev_err(css->dev, "failed to find suitable binary\n"); > return -EINVAL; > } > + css->pipes[pipe].bindex = ret; > > dev_dbg(css->dev, "Binary index %d for pipe %d found.", > css->pipes[pipe].bindex, pipe); >
Re: [PATCH][media-next] media: ddbridge: avoid out-of-bounds write on array demod_in_use
On 08/05/18 11:38, Daniel Scheller wrote: > Hi Colin, > > Am Tue, 8 May 2018 00:08:42 +0100 > schrieb Colin King : > >> From: Colin Ian King >> >> In function stop there is a check to see if state->demod is a stopped >> value of 0xff, however, later on, array demod_in_use is indexed with >> this value causing an out-of-bounds write error. Avoid this by only >> writing to array demod_in_use if state->demod is not set to the stopped >> sentinal value for this specific corner case. Also, replace the magic >> value 0xff with DEMOD_STOPPED to make code more readable. >> >> Detected by CoverityScan, CID#1468550 ("Out-of-bounds write") >> >> Fixes: daeeb1319e6f ("media: ddbridge: initial support for MCI-based MaxSX8 >> cards") >> Signed-off-by: Colin Ian King >> --- >> drivers/media/pci/ddbridge/ddbridge-mci.c | 11 +++ >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/pci/ddbridge/ddbridge-mci.c >> b/drivers/media/pci/ddbridge/ddbridge-mci.c >> index a85ff3e6b919..1f5ed53c8d35 100644 >> --- a/drivers/media/pci/ddbridge/ddbridge-mci.c >> +++ b/drivers/media/pci/ddbridge/ddbridge-mci.c >> @@ -20,6 +20,8 @@ >> #include "ddbridge-io.h" >> #include "ddbridge-mci.h" >> >> +#define DEMOD_STOPPED (0xff) >> + >> static LIST_HEAD(mci_list); >> >> static const u32 MCLK = (155000 / 12); >> @@ -193,7 +195,7 @@ static int stop(struct dvb_frontend *fe) >> u32 input = state->tuner; >> >> memset(&cmd, 0, sizeof(cmd)); >> -if (state->demod != 0xff) { >> +if (state->demod != DEMOD_STOPPED) { >> cmd.command = MCI_CMD_STOP; >> cmd.demod = state->demod; >> mci_cmd(state, &cmd, NULL); >> @@ -209,10 +211,11 @@ static int stop(struct dvb_frontend *fe) >> state->base->tuner_use_count[input]--; >> if (!state->base->tuner_use_count[input]) >> mci_set_tuner(fe, input, 0); >> -state->base->demod_in_use[state->demod] = 0; >> +if (state->demod != DEMOD_STOPPED) >> +state->base->demod_in_use[state->demod] = 0; >> state->base->used_ldpc_bitrate[state->nr] = 0; >> -state->demod = 0xff; >> -state->base->assigned_demod[state->nr] = 0xff; >> +state->demod = DEMOD_STOPPED; >> +state->base->assigned_demod[state->nr] = DEMOD_STOPPED; >> state->base->iq_mode = 0; >> mutex_unlock(&state->base->tuner_lock); >> state->started = 0; > > Thanks for the patch, or - better - pointing this out. While it's > unlikely this will ever be an issue, I'm fine with changing the code > like that, but I'd prefer to change it a bit differently (ie. > DEMOD_STOPPED should be DEMOD_UNUSED, and I'd add defines for max. > tuners and use/compare against them). Sounds like a good idea. > > I'll send out a different patch that will cover the potential > coverityscan problem throughout the end of the week. Great. Thanks! > > Best regards, > Daniel Scheller >
Re: [PATCH] [media] gp8psk: fix spelling mistake: "firmare" -> "firmware"
On 29/12/16 21:23, VDR User wrote: >> - err("firmare chunk size bigger than 64 bytes."); >> + err("firmware chunk size bigger than 64 bytes."); > > Yup. > >> -"HW don't support CMAC encrypiton, use software >> CMAC encrypiton\n"); >> +"HW don't support CMAC encryption, use software >> CMAC encryption\n"); > > Should be: "HW doesn't support CMAC encryption, use software CMAC > encryption\n"); > Very true, I was so focused on the spelling I overlooked the grammar. I'll re-send with that fixed. Colin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue
On 09/03/17 11:49, walter harms wrote: > > > Am 09.03.2017 11:57, schrieb Hans Verkuil: >> Hi Songjun, >> >> On 08/03/17 03:25, Wu, Songjun wrote: >>> Hi Colin, >>> >>> Thank you for your comment. >>> It is a bug, will be fixed in the next patch. >> >> Do you mean that you will provide a new patch for this? Is there anything >> wrong with this patch? It seems reasonable to me. >> >> Regards, >> >> Hans >> > > > > perhaps he will make it a bit more readable, like: > > *hist_count += i * (*hist_entry++); > > *hist_count += hist_entry[i]*i; As long as it gets fixed somehow, then I'm happy. Colin > > > re, > wh >>> >>> On 3/7/2017 22:30, Colin King wrote: >>>> From: Colin Ian King >>>> >>>> The are only HIST_ENTRIES worth of entries in hist_entry however the >>>> for-loop is iterating one too many times leasing to a read access off >>>> the end off the array ctrls->hist_entry. Fix this by iterating by >>>> the correct number of times. >>>> >>>> Detected by CoverityScan, CID#1415279 ("Out-of-bounds read") >>>> >>>> Signed-off-by: Colin Ian King >>>> --- >>>> drivers/media/platform/atmel/atmel-isc.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/media/platform/atmel/atmel-isc.c >>>> b/drivers/media/platform/atmel/atmel-isc.c >>>> index b380a7d..7dacf8c 100644 >>>> --- a/drivers/media/platform/atmel/atmel-isc.c >>>> +++ b/drivers/media/platform/atmel/atmel-isc.c >>>> @@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc) >>>> regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES); >>>> >>>> *hist_count = 0; >>>> -for (i = 0; i <= HIST_ENTRIES; i++) >>>> +for (i = 0; i < HIST_ENTRIES; i++) >>>> *hist_count += i * (*hist_entry++); >>>> } >>>> >>>> >> > > > > >> -- >> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >>
Re: [PATCH] [media] include/media: fix missing | operator when setting cfg
On 18/04/18 16:23, Sylwester Nawrocki wrote: > On 04/18/2018 05:20 PM, Sylwester Nawrocki wrote: >> On 04/18/2018 05:06 PM, Colin King wrote: >>> From: Colin Ian King >>> >>> The value from a readl is being masked with ITE_REG_CIOCAN_MASK however >>> this is not being used and cfg is being re-assigned. I believe the >>> assignment operator should actually be instead the |= operator. >>> >>> Detected by CoverityScan, CID#1467987 ("Unused value") >>> >>> Signed-off-by: Colin Ian King >> Thanks for the patch. >> >> Acked-by: Sylwester Nawrocki > > I forgot to mention that the subject should rather looks something > like: > > "exynos4-is: fimc-lite: : fix missing | operator when setting cfg" > Oops, shall I re-send?
Re: [PATCH] [media] bdisp: remove redundant assignment to pix
On 29/10/17 13:30, Julia Lawall wrote: > > > On Sun, 29 Oct 2017, Colin King wrote: > >> From: Colin Ian King >> >> Pointer pix is being initialized to a value and a little later >> being assigned the same value again. Remove the redundant second >> duplicate assignment. Cleans up the clang warning: >> >> drivers/media/platform/sti/bdisp/bdisp-v4l2.c:726:26: warning: Value >> stored to 'pix' during its initialization is never read >> >> Signed-off-by: Colin Ian King >> --- >> drivers/media/platform/sti/bdisp/bdisp-v4l2.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c >> b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c >> index 939da6da7644..14e99aeae140 100644 >> --- a/drivers/media/platform/sti/bdisp/bdisp-v4l2.c >> +++ b/drivers/media/platform/sti/bdisp/bdisp-v4l2.c >> @@ -731,7 +731,6 @@ static int bdisp_g_fmt(struct file *file, void *fh, >> struct v4l2_format *f) >> return PTR_ERR(frame); >> } >> >> -pix = &f->fmt.pix; > > Why not keep this one and drop the first one? Maybe it would be nice to > keep all the initializations related to pix together? Good point. Will send a V2. > > julia > >> pix->width = frame->width; >> pix->height = frame->height; >> pix->pixelformat = frame->fmt->pixelformat; >> -- >> 2.14.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >>
Re: [PATCH] [media] mb86a20s: remove redundant check if val is less than zero
On 15/07/16 16:20, Mauro Carvalho Chehab wrote: > Em Tue, 12 Jul 2016 10:30:51 +0100 > Colin King escreveu: > >> From: Colin Ian King >> >> The result of mb86a20s_readreg(state, 0x0a) & 0xf is always in the range >> 0x00 to 0x0f and can never be negative, so remove the redundant check >> of the result being less than zero. >> >> Signed-off-by: Colin Ian King >> --- >> drivers/media/dvb-frontends/mb86a20s.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/media/dvb-frontends/mb86a20s.c >> b/drivers/media/dvb-frontends/mb86a20s.c >> index fb88ddd..0205846 100644 >> --- a/drivers/media/dvb-frontends/mb86a20s.c >> +++ b/drivers/media/dvb-frontends/mb86a20s.c >> @@ -302,8 +302,6 @@ static int mb86a20s_read_status(struct dvb_frontend *fe, >> enum fe_status *status) >> *status = 0; >> >> val = mb86a20s_readreg(state, 0x0a) & 0xf; >> -if (val < 0) >> -return val; > > Actually, mb86a20s_readreg() can return a negative value. Oops, yep, clearly my code is stupid. I'll send a fix soon. > > Please change the above logic to first check for the value returned > from mb86a20s_readreg() and then apply the bitmask. > > Thanks, > Mauro > >> >> if (val >= 2) >> *status |= FE_HAS_SIGNAL; > > > > Thanks, > Mauro > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html