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
