Re: Staging: Media: Lirc - Fix possible ERR_PTR() dereferencing.
On Tue, Sep 20, 2016 at 12:21:21PM +0530, Shailendra Verma wrote: > This is of course wrong to call kfree() if memdup_user() fails, > no memory was allocated and the error in the error-valued pointer > should be returned. > > Reviewed-by: Ravikant Sharma > Signed-off-by: Shailendra Verma Calling kfree(NULL) is fine so there is no bug in the original code. Also this patch creates a new locking bug. regards, dan carpenter -- 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
[patch] [media] st-hva: fix some error handling in hva_hw_probe()
The devm_ioremap_resource() returns error pointers, never NULL. The
platform_get_resource() returns NULL on error, never error pointers.
The error code needs to be set, as well. The current code returns
PTR_ERR(NULL) which is success.
Fixes: 57b2c0628b60 ("[media] st-hva: multi-format video encoder V4L2 driver")
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/platform/sti/hva/hva-hw.c
b/drivers/media/platform/sti/hva/hva-hw.c
index d341d49..cf2a8d8 100644
--- a/drivers/media/platform/sti/hva/hva-hw.c
+++ b/drivers/media/platform/sti/hva/hva-hw.c
@@ -305,16 +305,16 @@ int hva_hw_probe(struct platform_device *pdev, struct
hva_dev *hva)
/* get memory for registers */
regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
hva->regs = devm_ioremap_resource(dev, regs);
- if (IS_ERR_OR_NULL(hva->regs)) {
+ if (IS_ERR(hva->regs)) {
dev_err(dev, "%s failed to get regs\n", HVA_PREFIX);
return PTR_ERR(hva->regs);
}
/* get memory for esram */
esram = platform_get_resource(pdev, IORESOURCE_MEM, 1);
- if (IS_ERR_OR_NULL(esram)) {
+ if (!esram) {
dev_err(dev, "%s failed to get esram\n", HVA_PREFIX);
- return PTR_ERR(esram);
+ return -ENODEV;
}
hva->esram_addr = esram->start;
hva->esram_size = resource_size(esram);
--
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 02/18] [media] RedRat3: Move two assignments in redrat3_transmit_ir()
I have asked you about six or seven times to only send bug fixes and stop sending clean up patches. You have refused. But now I'm asking you to stop randomly doing things without at least thinking about it for a bit. The original code was correct. regards, dan carpenter -- 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: [bug report] drx: add initial drx-d driver
I'm really sorry. This warning showed up in my new warnings pile and I didn't look at the date. :/ Sorry for the noise. regards, dan carpenter
Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers
On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote: > @@ -1147,10 +1145,8 @@ static int gc2235_probe(struct i2c_client *client) > if (ret) > gc2235_remove(client); This error handling is probably wrong... > > - if (ACPI_HANDLE(&client->dev)) > - ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA); > + return atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA); In the end this should look something like: ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA); if (ret) goto err_free_something; return 0; > > - return ret; > out_free: > v4l2_device_unregister_subdev(&dev->sd); > kfree(dev); regards, dan carpenter
Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers
On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote: > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client) > dev_err(&client->dev, "gpio request/direction_output fail"); > goto fail2; > } > - if (ACPI_HANDLE(&client->dev)) > - err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); > - return 0; > + return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH); > fail2: > media_entity_cleanup(&flash->sd.entity); > v4l2_ctrl_handler_free(&flash->ctrl_handler); Actually every place where we directly return a function call is wrong and needs error handling added. I've been meaning to write a Smatch check for this because it's a common anti-pattern we don't check the last function call for errors. Someone could probably do the same in Coccinelle if they want. regards, dan carpenter
Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers
On Wed, Dec 20, 2017 at 11:30:01AM +0100, Julia Lawall wrote:
>
>
> On Wed, 20 Dec 2017, Dan Carpenter wrote:
>
> > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> > > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
> > > dev_err(&client->dev, "gpio request/direction_output fail");
> > > goto fail2;
> > > }
> > > - if (ACPI_HANDLE(&client->dev))
> > > - err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > > - return 0;
> > > + return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > > fail2:
> > > media_entity_cleanup(&flash->sd.entity);
> > > v4l2_ctrl_handler_free(&flash->ctrl_handler);
> >
> > Actually every place where we directly return a function call is wrong
> > and needs error handling added. I've been meaning to write a Smatch
> > check for this because it's a common anti-pattern we don't check the
> > last function call for errors.
> >
> > Someone could probably do the same in Coccinelle if they want.
>
> I'm not sure what you are suggesting. Is every case of return f(...);
> for any f wrong? Or is it a particular function that is of concern? Or
> would it be that every function call that has error handling somewhere
> should have error handling everywhere? Or is it related to what seems to
> be the problem in the above code that err is initialized but nothing
> happens to it?
>
I was just thinking that it's a common pattern to treat the last
function call differently and one mistake I often see looks like this:
ret = frob();
if (ret) {
cleanup();
return ret;
}
return another_function();
No error handling for the last function call.
regards,
dan carpenter
[PATCH] media: lirc: Fix uninitialized variable in ir_lirc_transmit_ir()
The "txbuf" is uninitialized when we call ir_raw_encode_scancode() so
this failure path would lead to a crash.
Fixes: a74b2bff5945 ("media: lirc: do not pass ERR_PTR to kfree")
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index fae42f120aa4..5efe9cd2309a 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -295,7 +295,7 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const
char __user *buf,
ret = ir_raw_encode_scancode(scan.rc_proto, scan.scancode,
raw, LIRCBUF_SIZE);
if (ret < 0)
- goto out_kfree;
+ goto out_free_raw;
count = ret;
@@ -366,6 +366,7 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const
char __user *buf,
return n;
out_kfree:
kfree(txbuf);
+out_free_raw:
kfree(raw);
out_unlock:
mutex_unlock(&dev->lock);
[PATCH] media: staging/imx: Checking the right variable in vdic_get_ipu_resources()
We recently changed this error handling around but missed this error
pointer check. We're testing "priv->vdi_in_ch_n" instead of "ch" so the
error handling can't be triggered.
Fixes: 0b2e9e7947e7 ("media: staging/imx: remove confusing IS_ERR_OR_NULL
usage")
Signed-off-by: Dan Carpenter
diff --git a/drivers/staging/media/imx/imx-media-vdic.c
b/drivers/staging/media/imx/imx-media-vdic.c
index 433474d58e3e..ed356844cdf6 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -177,7 +177,7 @@ static int vdic_get_ipu_resources(struct vdic_priv *priv)
priv->vdi_in_ch = ch;
ch = ipu_idmac_get(priv->ipu, IPUV3_CHANNEL_MEM_VDI_NEXT);
- if (IS_ERR(priv->vdi_in_ch_n)) {
+ if (IS_ERR(ch)) {
err_chan = IPUV3_CHANNEL_MEM_VDI_NEXT;
ret = PTR_ERR(ch);
goto out_err_chan;
[PATCH] [media] s3c-camif: array underflow in __camif_subdev_try_format()
The while loop is a post op, "while (i-- >= 0)" so the last iteration
will read camif_mbus_formats[-1] and then the loop will exit with "i"
set to -2 and so we do: "mf->code = camif_mbus_formats[-2];".
I've changed it to a pre-op, I've added a check to ensure we found the
right format and I've removed the "mf->code = camif_mbus_formats[i];"
because that is a no-op anyway.
Fixes: babde1c243b2 ("[media] V4L: Add driver for S3C24XX/S3C64XX SoC series
camera interface")
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/platform/s3c-camif/camif-capture.c
b/drivers/media/platform/s3c-camif/camif-capture.c
index 437395a61065..012f4b389c55 100644
--- a/drivers/media/platform/s3c-camif/camif-capture.c
+++ b/drivers/media/platform/s3c-camif/camif-capture.c
@@ -1261,11 +1261,11 @@ static void __camif_subdev_try_format(struct camif_dev
*camif,
/* FIXME: constraints against codec or preview path ? */
pix_lim = &variant->vp_pix_limits[VP_CODEC];
- while (i-- >= 0)
+ while (--i >= 0)
if (camif_mbus_formats[i] == mf->code)
break;
-
- mf->code = camif_mbus_formats[i];
+ if (i < 0)
+ return;
if (pad == CAMIF_SD_PAD_SINK) {
v4l_bound_align_image(&mf->width, 8, CAMIF_MAX_PIX_WIDTH,
[bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
Hello Andrzej Hajda,
The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
identify last buffers in decoder capture queue" from Oct 7, 2015,
leads to the following static checker warning:
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:658 vidioc_dqbuf()
error: buffer overflow 'ctx->dst_bufs' 32 user_rl = '0-u32max'
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
635 /* Dequeue a buffer */
636 static int vidioc_dqbuf(struct file *file, void *priv, struct
v4l2_buffer *buf)
637 {
638 const struct v4l2_event ev = {
639 .type = V4L2_EVENT_EOS
640 };
641 struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
642 int ret;
643
644 if (ctx->state == MFCINST_ERROR) {
645 mfc_err_limited("Call on DQBUF after unrecoverable
error\n");
646 return -EIO;
647 }
648
649 switch (buf->type) {
650 case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
651 return vb2_dqbuf(&ctx->vq_src, buf, file->f_flags &
O_NONBLOCK);
652 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
653 ret = vb2_dqbuf(&ctx->vq_dst, buf, file->f_flags &
O_NONBLOCK);
654 if (ret)
655 return ret;
656
657 if (ctx->state == MFCINST_FINISHED &&
658 (ctx->dst_bufs[buf->index].flags &
MFC_BUF_FLAG_EOS))
^^
Smatch is complaining that "buf->index" is not capped. So far as I can
see this is true. I would have expected it to be checked in
check_array_args() or video_usercopy() but I couldn't find the check.
659 v4l2_event_queue_fh(&ctx->fh, &ev);
660 return 0;
661 default:
662 return -EINVAL;
663 }
664 }
regards,
dan carpenter
[bug report] V4L/DVB(7872): mxl5005s: checkpatch.pl compliance
Hello Steven Toth, The patch d211017b9544: "V4L/DVB(7872): mxl5005s: checkpatch.pl compliance" from May 1, 2008, leads to the following static checker warning: drivers/media/tuners/mxl5005s.c:1817 MXL_BlockInit() warn: both sides of ternary the same: '1' drivers/media/tuners/mxl5005s.c 1812 } 1813 1814 /* Charge Pump Control Dig Ana */ 1815 status += MXL_ControlWrite(fe, RFSYN_CHP_GAIN, state->Mode ? 5 : 8); 1816 status += MXL_ControlWrite(fe, 1817 RFSYN_EN_CHP_HIGAIN, state->Mode ? 1 : 1); ^^^ 1818 status += MXL_ControlWrite(fe, EN_CHP_LIN_B, state->Mode ? 0 : 0); ^^^ What do these mean? What are they supposed to do? 1819 regards, dan carpenter
Re: [PATCH] [media] s3c-camif: array underflow in __camif_subdev_try_format()
On Mon, Jan 22, 2018 at 09:50:04PM +0100, Sylwester Nawrocki wrote:
> On 01/22/2018 11:37 AM, Dan Carpenter wrote:
> > --- a/drivers/media/platform/s3c-camif/camif-capture.c
> > +++ b/drivers/media/platform/s3c-camif/camif-capture.c
> > @@ -1261,11 +1261,11 @@ static void __camif_subdev_try_format(struct
> > camif_dev *camif,
> > /* FIXME: constraints against codec or preview path ? */
> > pix_lim = &variant->vp_pix_limits[VP_CODEC];
> >
> > - while (i-- >= 0)
> > + while (--i >= 0)
> > if (camif_mbus_formats[i] == mf->code)
> > break;
> > -
> > - mf->code = camif_mbus_formats[i];
> > + if (i < 0)
> > + return;
>
> Thanks for the patch Dan. mf->width needs to be aligned by this try_format
> function so we shouldn't return here. Also it needs to be ensured mf->code
> is set to one of the supported values when this function returns. Sorry,
> the current code really doesn't give a clue what was intended.
>
> There is already queued a patch from Arnd [1] addressing the issues you
> have found.
>
> > if (pad == CAMIF_SD_PAD_SINK) {
> > v4l_bound_align_image(&mf->width, 8, CAMIF_MAX_PIX_WIDTH,
> >
>
> [1] https://patchwork.linuxtv.org/patch/46508
>
Hey Arnd,
I happened to be looking at the same bugs but using Smatch. Did you get
these two bugs as well?
drivers/scsi/sym53c8xx_2/sym_hipd.c:549 sym_getsync() error: iterator underflow
'div_10M' (-1)-255
drivers/media/i2c/sr030pc30.c:522 try_fmt() error: iterator underflow
'sr030pc30_formats' (-1)-4
regards,
dan carpenter
Re: [PATCH] staging: media: remove unused VIDEO_ATOMISP_OV8858 kconfig
On Tue, Jan 23, 2018 at 07:31:27PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 23, 2018 at 4:37 PM, Corentin Labbe wrote:
> > Nothing in kernel use VIDEO_ATOMISP_OV8858 since commit 3a81c7660f80
> > ("media: staging: atomisp: Remove IMX sensor support")
> > Lets remove this kconfig option.
>
> First of all, I hardly understand how that change is related.
It's pretty obvious if you look at the commit.
-obj-$(CONFIG_VIDEO_ATOMISP_OV8858) += atomisp-ov8858.o
It sounds like you deleted that line by mistake and re-added it to your
local Makefile?
regards,
dan carpenter
Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
On Thu, Jan 25, 2018 at 10:58:45AM +0100, Andrzej Hajda wrote:
> On 23.01.2018 09:32, Dan Carpenter wrote:
> > Hello Andrzej Hajda,
> >
> > The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
> > identify last buffers in decoder capture queue" from Oct 7, 2015,
> > leads to the following static checker warning:
> >
> > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:658 vidioc_dqbuf()
> > error: buffer overflow 'ctx->dst_bufs' 32 user_rl = '0-u32max'
> >
> > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> >635 /* Dequeue a buffer */
> >636 static int vidioc_dqbuf(struct file *file, void *priv, struct
> > v4l2_buffer *buf)
> >637 {
> >638 const struct v4l2_event ev = {
> >639 .type = V4L2_EVENT_EOS
> >640 };
> >641 struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
> >642 int ret;
> >643
> >644 if (ctx->state == MFCINST_ERROR) {
> >645 mfc_err_limited("Call on DQBUF after unrecoverable
> > error\n");
> >646 return -EIO;
> >647 }
> >648
> >649 switch (buf->type) {
> >650 case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> >651 return vb2_dqbuf(&ctx->vq_src, buf, file->f_flags &
> > O_NONBLOCK);
> >652 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> >653 ret = vb2_dqbuf(&ctx->vq_dst, buf, file->f_flags &
> > O_NONBLOCK);
> >654 if (ret)
> >655 return ret;
> >656
> >657 if (ctx->state == MFCINST_FINISHED &&
> >658 (ctx->dst_bufs[buf->index].flags &
> > MFC_BUF_FLAG_EOS))
> >^^
> > Smatch is complaining that "buf->index" is not capped. So far as I can
> > see this is true. I would have expected it to be checked in
> > check_array_args() or video_usercopy() but I couldn't find the check.
>
> I did not work in V4L2 area for long time, so I could be wrong, but I
> hope the code is correct, below my explanation.
> User provides only type, memory and reserved fields in buf, other fields
> are filled by vb2_dqbuf (line 653) core function, ie index field is
> copied from buffer which was queued by qbuf.
> And vidioc_qbuf calls vb2_qbuf, which calls vb2_queue_or_prepare_buf,
> which checks index bounds [1].
>
> So I suppose this code is correct.
> Btw, I have also looked at other drivers and it looks omap driver
> handles it incorrectly, ie it uses index field provided by user -
> possible memory leak. CC Hans and Mauro, since there is no driver
> maintainer of OMAP.
>
> Btw2, is it possible to check in smatch which fields of passed struct
> given callback can read or fill ? For example here API restrict dqbuf
> callback to read only three fields of buf, and fill the others.
>
> [1]:
> http://elixir.free-electrons.com/linux/latest/source/drivers/media/v4l2-core/videobuf2-v4l2.c#L165
> [2]:
> http://elixir.free-electrons.com/linux/latest/source/drivers/media/platform/omap/omap_vout.c#L1520
>
> Regards
> Andrzej
Smatch does track the feilds... Smatch sees that buf->index is capped
in vidioc_qbuf() but it still complains that buf->index gets set by the
user in the ioctl and not checked before we use it vb2_dqbuf(). The
call tree looks like this:
--> video_usercopy()
Copies _IOC_SIZE(cmd) bytes to parg. The _IOC_SIZE() is
sizeof(struct v4l2_buffer) so all the feilds are reset. Smatch
doesn't track how many bytes the users controls, it just marks
everything in *parg as tainted but it doesn't matter in this case
since all the feilds are set.
video_usercopy() calls err = func(file, cmd, parg);
--> __video_do_ioctl()
calls info->u.func(ops, file, fh, arg);
--> v4l_dqbuf()
calls ops->vidioc_dqbuf(file, fh, p);
--> vidioc_dqbuf()
uses unchecked buf->index
Ah... Hm. Is it the call to vb2_core_dqbuf() which limits buf->index?
I don't see a path from vb2_core_dqbuf() to vb2_qbuf() but I may have
missed it.
regards,
dan carpenter
Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
On Thu, Jan 25, 2018 at 01:31:36PM +0100, Hans Verkuil wrote: > > Ah... Hm. Is it the call to vb2_core_dqbuf() which limits buf->index? > > I don't see a path from vb2_core_dqbuf() to vb2_qbuf() but I may have > > missed it. > > The __fill_v4l2_buffer() function in videobuf2-v4l2.c is called by > vb2_core_dqbuf(). > And that __fill_v4l2_buffer() overwrited the index field: b->index = > vb->index; > > So after the vb2_dqbuf call the buf->index field is correct and bounded. > Ah.. I get it. Thanks. regards, dan carpenter
[PATCH] [media] sr030pc30: prevent array underflow in try_fmt()
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/i2c/sr030pc30.c b/drivers/media/i2c/sr030pc30.c
index 0bf031b7e4fa..2a4882cddc51 100644
--- a/drivers/media/i2c/sr030pc30.c
+++ b/drivers/media/i2c/sr030pc30.c
@@ -511,13 +511,16 @@ static int sr030pc30_get_fmt(struct v4l2_subdev *sd,
static const struct sr030pc30_format *try_fmt(struct v4l2_subdev *sd,
struct v4l2_mbus_framefmt *mf)
{
- int i = ARRAY_SIZE(sr030pc30_formats);
+ int i;
sr030pc30_try_frame_size(mf);
- while (i--)
+ for (i = 0; i < ARRAY_SIZE(sr030pc30_formats); i++) {
if (mf->code == sr030pc30_formats[i].code)
break;
+ }
+ if (i == ARRAY_SIZE(sr030pc30_formats))
+ i = 0;
mf->code = sr030pc30_formats[i].code;
Re: [PATCH 0/4] tree-wide: fix comparison to bitshift when dealing with a mask
On Mon, Feb 05, 2018 at 09:09:57PM +0100, Wolfram Sang wrote:
> In one Renesas driver, I found a typo which turned an intended bit shift
> ('<<')
> into a comparison ('<'). Because this is a subtle issue, I looked tree wide
> for
> similar patterns. This small patch series is the outcome.
>
> Buildbot and checkpatch are happy. Only compile-tested. To be applied
> individually per sub-system, I think. I'd think only the net: amd: patch needs
> to be conisdered for stable, but I leave this to people who actually know this
> driver.
>
> CCing Dan. Maybe he has an idea how to add a test to smatch? In my setup, only
> cppcheck reported a 'coding style' issue with a low prio.
>
Most of these are inside macros so it makes it complicated for Smatch
to warn about them. It might be easier in Coccinelle. Julia the bugs
look like this:
- reissue_mask |= 0x < 4;
+ reissue_mask |= 0x << 4;
regards,
dan carpenter
> Wolfram Sang (4):
> v4l: vsp1: fix mask creation for MULT_ALPHA_RATIO
> drm/exynos: fix comparison to bitshift when dealing with a mask
> v4l: dvb-frontends: stb0899: fix comparison to bitshift when dealing
> with a mask
> net: amd-xgbe: fix comparison to bitshift when dealing with a mask
>
> drivers/gpu/drm/exynos/regs-fimc.h| 2 +-
> drivers/media/dvb-frontends/stb0899_reg.h | 8
> drivers/media/platform/vsp1/vsp1_regs.h | 2 +-
> drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 2 +-
> 4 files changed, 7 insertions(+), 7 deletions(-)
>
> --
> 2.11.0
Re: [PATCH 0/4] tree-wide: fix comparison to bitshift when dealing with a mask
On Tue, Feb 06, 2018 at 02:15:51PM +0100, Julia Lawall wrote:
>
>
> On Tue, 6 Feb 2018, Dan Carpenter wrote:
>
> > On Mon, Feb 05, 2018 at 09:09:57PM +0100, Wolfram Sang wrote:
> > > In one Renesas driver, I found a typo which turned an intended bit shift
> > > ('<<')
> > > into a comparison ('<'). Because this is a subtle issue, I looked tree
> > > wide for
> > > similar patterns. This small patch series is the outcome.
> > >
> > > Buildbot and checkpatch are happy. Only compile-tested. To be applied
> > > individually per sub-system, I think. I'd think only the net: amd: patch
> > > needs
> > > to be conisdered for stable, but I leave this to people who actually know
> > > this
> > > driver.
> > >
> > > CCing Dan. Maybe he has an idea how to add a test to smatch? In my setup,
> > > only
> > > cppcheck reported a 'coding style' issue with a low prio.
> > >
> >
> > Most of these are inside macros so it makes it complicated for Smatch
> > to warn about them. It might be easier in Coccinelle. Julia the bugs
> > look like this:
> >
> > - reissue_mask |= 0x < 4;
> > + reissue_mask |= 0x << 4;
>
> Thanks. I'll take a look. Do you have an example of the macro issue
> handy?
>
It's the same:
#define EXYNOS_CIIMGEFF_PAT_CBCR_MASK ((0xff < 13) | (0xff < 0))
Smatch only sees the outside of the macro (where it is used in the code)
and the pre-processed code.
regards,
dan carpenter
Re: [PATCH 0/4] tree-wide: fix comparison to bitshift when dealing with a mask
That found 4 that I think Wolfram's grep missed. arch/um/drivers/vector_user.h |2 -- drivers/gpu/drm/mxsfb/mxsfb_regs.h|2 -- drivers/video/fbdev/mxsfb.c |2 -- include/drm/drm_scdc_helper.h |3 --- But it didn't find the two bugs that Geert found where the right side wasn't a number literal. drivers/net/can/m_can/m_can.c:#define RXFC_FWM_MASK (0x7f < RXFC_FWM_SHIFT) drivers/usb/gadget/udc/goku_udc.h:#define INT_EPnNAK(n) (0x00100 < (n)) /* 0 < n < 4 */ regards, dan carpenter
[bug report] media: i2c: Add driver for Aptina MT9V111
Hello Jacopo Mondi,
The patch aab7ed1c3927: "media: i2c: Add driver for Aptina MT9V111"
from Jul 25, 2018, leads to the following static checker warning:
drivers/media/i2c/mt9v111.c:1163 mt9v111_probe() warn: passing zero to 'PTR_ERR'
drivers/media/i2c/mt9v111.c:1173 mt9v111_probe() warn: passing zero to 'PTR_ERR'
drivers/media/i2c/mt9v111.c:1184 mt9v111_probe() warn: passing zero to 'PTR_ERR'
drivers/media/i2c/mt9v111.c:1194 mt9v111_probe() warn: passing zero to 'PTR_ERR'
drivers/media/i2c/mt9v111.c
1155 v4l2_ctrl_handler_init(&mt9v111->ctrls, 5);
1156
1157 mt9v111->auto_awb = v4l2_ctrl_new_std(&mt9v111->ctrls,
1158&mt9v111_ctrl_ops,
1159
V4L2_CID_AUTO_WHITE_BALANCE,
11600, 1, 1,
1161V4L2_WHITE_BALANCE_AUTO);
1162 if (IS_ERR_OR_NULL(mt9v111->auto_awb)) {
1163 ret = PTR_ERR(mt9v111->auto_awb);
This just returns success because v4l2_ctrl_new_std() only return NULL
on error, it never returns error pointers. I guess we should set ret to
EINVAL?
if (!mt9v111->auto_awb) {
ret = -EINVAL;
goto error_free_ctrls;
}
1164 goto error_free_ctrls;
1165 }
1166
1167 mt9v111->auto_exp = v4l2_ctrl_new_std_menu(&mt9v111->ctrls,
1168 &mt9v111_ctrl_ops,
1169
V4L2_CID_EXPOSURE_AUTO,
1170 V4L2_EXPOSURE_MANUAL,
1171 0,
V4L2_EXPOSURE_AUTO);
1172 if (IS_ERR_OR_NULL(mt9v111->auto_exp)) {
1173 ret = PTR_ERR(mt9v111->auto_exp);
1174 goto error_free_ctrls;
1175 }
1176
1177 /* Initialize timings */
1178 mt9v111->hblank = v4l2_ctrl_new_std(&mt9v111->ctrls,
&mt9v111_ctrl_ops,
1179 V4L2_CID_HBLANK,
1180 MT9V111_CORE_R05_MIN_HBLANK,
1181
MT9V111_CORE_R05_MAX_HBLANK, 1,
1182
MT9V111_CORE_R05_DEF_HBLANK);
1183 if (IS_ERR_OR_NULL(mt9v111->hblank)) {
1184 ret = PTR_ERR(mt9v111->hblank);
1185 goto error_free_ctrls;
1186 }
1187
1188 mt9v111->vblank = v4l2_ctrl_new_std(&mt9v111->ctrls,
&mt9v111_ctrl_ops,
1189 V4L2_CID_VBLANK,
1190 MT9V111_CORE_R06_MIN_VBLANK,
1191
MT9V111_CORE_R06_MAX_VBLANK, 1,
1192
MT9V111_CORE_R06_DEF_VBLANK);
1193 if (IS_ERR_OR_NULL(mt9v111->vblank)) {
1194 ret = PTR_ERR(mt9v111->vblank);
1195 goto error_free_ctrls;
1196 }
1197
1198 /* PIXEL_RATE is fixed: just expose it to user space. */
1199 v4l2_ctrl_new_std(&mt9v111->ctrls, &mt9v111_ctrl_ops,
regards,
dan carpenter
Re: [PATCH 03/14] staging: media: tegra-vde: Prepare for interlacing support
On Mon, Aug 13, 2018 at 04:50:16PM +0200, Thierry Reding wrote: > static void tegra_vde_setup_iram_tables(struct tegra_vde *vde, > + unsigned int num_ref_pics, > struct video_frame *dpb_frames, > unsigned int ref_frames_nb, > unsigned int with_earlier_poc_nb) > @@ -251,13 +260,17 @@ static void tegra_vde_setup_iram_tables(struct > tegra_vde *vde, > u32 value, aux_addr; > int with_later_poc_nb; > unsigned int i, k; > + size_t size; > + > + size = num_ref_pics * 4 * 8; > + memset(vde->iram, 0, size); I can't get behind the magical size calculation... :( regards, dan carpenter
[PATCH v2] media: sr030pc30: remove NULL in sr030pc30_base_config()
This code doesn't check for NULL consistently and it generates a Smatch warning: drivers/media/i2c/sr030pc30.c:575 sr030pc30_base_config() error: we previously assumed 'info->pdata' could be null (see line 572) Fortunately, "info->pdata" can't be NULL to that check can be removed. The other thing is that if "ret" is an error code here, then we don't want to do the next call to cam_i2c_write(), so actually let's flip that test around and return the error. This is more of a theoretical issue than something which is likely to affect real life. Signed-off-by: Dan Carpenter --- v2: just remove the check Thanks Sakari Ailus for your review. diff --git a/drivers/media/i2c/sr030pc30.c b/drivers/media/i2c/sr030pc30.c index 2a4882cddc51..66d952624731 100644 --- a/drivers/media/i2c/sr030pc30.c +++ b/drivers/media/i2c/sr030pc30.c @@ -569,7 +569,7 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd) if (!ret) ret = sr030pc30_pwr_ctrl(sd, false, false); - if (!ret && !info->pdata) + if (ret) return ret; expmin = EXPOS_MIN_MS * info->pdata->clk_rate / (8 * 1000);
[PATCH] [media] VPU: mediatek: don't pass an unused parameter
The load_requested_vpu() function returns a freed vpu_fw pointer. It's
not used so it doesn't cause any problems, but Smatch complains about
it:
drivers/media/platform/mtk-vpu/mtk_vpu.c:578 vpu_load_firmware()
warn: passing freed memory 'vpu_fw'
We can clean up the code a bit and silence the static checker warning
by not passing the parameter at all.
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/platform/mtk-vpu/mtk_vpu.c
b/drivers/media/platform/mtk-vpu/mtk_vpu.c
index f8d35e3ac1dc..616f78b24a79 100644
--- a/drivers/media/platform/mtk-vpu/mtk_vpu.c
+++ b/drivers/media/platform/mtk-vpu/mtk_vpu.c
@@ -480,12 +480,12 @@ EXPORT_SYMBOL_GPL(vpu_get_plat_device);
/* load vpu program/data memory */
static int load_requested_vpu(struct mtk_vpu *vpu,
- const struct firmware *vpu_fw,
u8 fw_type)
{
size_t tcm_size = fw_type ? VPU_DTCM_SIZE : VPU_PTCM_SIZE;
size_t fw_size = fw_type ? VPU_D_FW_SIZE : VPU_P_FW_SIZE;
char *fw_name = fw_type ? VPU_D_FW : VPU_P_FW;
+ const struct firmware *vpu_fw;
size_t dl_size = 0;
size_t extra_fw_size = 0;
void *dest;
@@ -539,7 +539,6 @@ int vpu_load_firmware(struct platform_device *pdev)
struct mtk_vpu *vpu;
struct device *dev = &pdev->dev;
struct vpu_run *run;
- const struct firmware *vpu_fw = NULL;
int ret;
if (!pdev) {
@@ -568,14 +567,14 @@ int vpu_load_firmware(struct platform_device *pdev)
run->signaled = false;
dev_dbg(vpu->dev, "firmware request\n");
/* Downloading program firmware to device*/
- ret = load_requested_vpu(vpu, vpu_fw, P_FW);
+ ret = load_requested_vpu(vpu, P_FW);
if (ret < 0) {
dev_err(dev, "Failed to request %s, %d\n", VPU_P_FW, ret);
goto OUT_LOAD_FW;
}
/* Downloading data firmware to device */
- ret = load_requested_vpu(vpu, vpu_fw, D_FW);
+ ret = load_requested_vpu(vpu, D_FW);
if (ret < 0) {
dev_err(dev, "Failed to request %s, %d\n", VPU_D_FW, ret);
goto OUT_LOAD_FW;
Re: [PATCH] media: cedrus: don't initialize pointers with zero
On Fri, Dec 07, 2018 at 09:31:06AM -0200, Mauro Carvalho Chehab wrote:
> Em Fri, 7 Dec 2018 12:14:50 +0100
> Hans Verkuil escreveu:
>
> > On 12/07/2018 11:56 AM, Mauro Carvalho Chehab wrote:
> > > A common mistake is to assume that initializing a var with:
> > > struct foo f = { 0 };
> > >
> > > Would initialize a zeroed struct. Actually, what this does is
> > > to initialize the first element of the struct to zero.
> > >
> > > According to C99 Standard 6.7.8.21:
> > >
> > > "If there are fewer initializers in a brace-enclosed
> > > list than there are elements or members of an aggregate,
> > > or fewer characters in a string literal used to initialize
> > > an array of known size than there are elements in the array,
> > > the remainder of the aggregate shall be initialized implicitly
> > > the same as objects that have static storage duration."
> > >
> > > So, in practice, it could zero the entire struct, but, if the
> > > first element is not an integer, it will produce warnings:
> > >
> > >
> > > drivers/staging/media/sunxi/cedrus/cedrus.c:drivers/staging/media/sunxi/cedrus/cedrus.c:78:49:
> > > warning: Using plain integer as NULL pointer
> > >
> > > drivers/staging/media/sunxi/cedrus/cedrus_dec.c:drivers/staging/media/sunxi/cedrus/cedrus_dec.c:29:35:
> > > warning: Using plain integer as NULL pointer
> > >
> > > A proper way to initialize it with gcc is to use:
> > >
> > > struct foo f = { };
> > >
> > > But that seems to be a gcc extension. So, I decided to check upstream
> >
> > No, this is not a gcc extension. It's part of the latest C standard.
>
> Sure? Where the C standard spec states that? I've been seeking for
> such info for a while, as '= {}' is also my personal preference.
>
> I tried to build the Kernel with clang, just to be sure that this
> won't cause issues with the clang support
My test says that clang works with {}.
I support this in Smatch as well.
regards,
dan carpenter
Re: [PATCH v9 00/13] media: staging/imx7: add i.MX7 media driver
On Fri, Dec 07, 2018 at 01:44:00PM +0100, Hans Verkuil wrote: > CHECK: Alignment should match open parenthesis > #936: FILE: drivers/staging/media/imx/imx7-mipi-csis.c:921: > + ret = v4l2_async_register_fwnode_subdev(mipi_sd, > + sizeof(struct v4l2_async_subdev), &sink_port, > 1, > > Apparently the latest coding style is that alignment is more important than > line length, although I personally do not agree. But since you need to > respin in any case due to the wrong SPDX identifier you used you might as > well take this into account. I'm pretty sure it complains about both equally. If you make fix one warning it will complain about the other. So you just have to pick which warning to not care about. regards, dan carpenter
Re: [PATCH 11/11] [media] marvell-ccic: provide a clock for the sensor
Hi Lubomir,
url:
https://github.com/0day-ci/linux/commits/Lubomir-Rintel/media-ov7670-hook-s_power-onto-v4l2-core/20181105-163336
base: git://linuxtv.org/media_tree.git master
smatch warnings:
drivers/media/platform/marvell-ccic/mcam-core.c:1682 mcam_v4l_open() warn:
inconsistent returns 'mutex:&cam->s_mutex'.
Locked on: line 1673
Unlocked on: line 1682
#
https://github.com/0day-ci/linux/commit/a4f7d692c7067355da433bbb534531a4e1a55ac6
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout a4f7d692c7067355da433bbb534531a4e1a55ac6
vim +1682 drivers/media/platform/marvell-ccic/mcam-core.c
abfa3df3 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-06-11 1656
abfa3df3 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-06-11 1657 /*
-- */
abfa3df3 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-06-11 1658 /*
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1659 * Our various file operations.
abfa3df3 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-06-11 1660 */
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1661 static int mcam_v4l_open(struct file *filp)
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1662 {
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1663 struct mcam_camera *cam = video_drvdata(filp);
949bd408 drivers/media/platform/marvell-ccic/mcam-core.c Hans Verkuil
2015-03-05 1664 int ret;
abfa3df3 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-06-11 1665
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1666 mutex_lock(&cam->s_mutex);
949bd408 drivers/media/platform/marvell-ccic/mcam-core.c Hans Verkuil
2015-03-05 1667 ret = v4l2_fh_open(filp);
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1668 if (ret)
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1669 goto out;
949bd408 drivers/media/platform/marvell-ccic/mcam-core.c Hans Verkuil
2015-03-05 1670 if (v4l2_fh_is_singular_file(filp)) {
a4f7d692 drivers/media/platform/marvell-ccic/mcam-core.c Lubomir Rintel
2018-11-05 1671 ret = sensor_call(cam, core, s_power, 1);
05fed816 drivers/media/platform/marvell-ccic/mcam-core.c Libin Yang
2013-07-03 1672 if (ret)
a4f7d692 drivers/media/platform/marvell-ccic/mcam-core.c Lubomir Rintel
2018-11-05 1673 return ret;
^^
This should be a goto out;
a4f7d692 drivers/media/platform/marvell-ccic/mcam-core.c Lubomir Rintel
2018-11-05 1674 mcam_clk_enable(cam);
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1675 __mcam_cam_reset(cam);
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1676 mcam_set_config_needed(cam, 1);
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1677 }
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1678 out:
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 1679 mutex_unlock(&cam->s_mutex);
44fbcb10 drivers/media/platform/marvell-ccic/mcam-core.c Hans Verkuil
2015-03-05 1680 if (ret)
44fbcb10 drivers/media/platform/marvell-ccic/mcam-core.c Hans Verkuil
2015-03-05 1681 v4l2_fh_release(filp);
d43dae75 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-07-08 @1682 return ret;
a9b36e85 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-06-20 1683 }
abfa3df3 drivers/media/video/marvell-ccic/mcam-core.cJonathan Corbet
2011-06-11 1684
:: The code at line 1682 was first introduced by commit
:: d43dae75cc1140bf27a59aa6d8e8bc7a00f009cc [media] marvell-cam: core code
reorganization
:: TO: Jonathan Corbet
:: CC: Mauro Carvalho Chehab
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[PATCH] media: cedrus: Fix a NULL vs IS_ERR() check
The devm_ioremap_resource() function doesn't return NULL pointers, it
returns error pointers.
Fixes: 50e761516f2b ("media: platform: Add Cedrus VPU decoder driver")
Signed-off-by: Dan Carpenter
---
drivers/staging/media/sunxi/cedrus/cedrus_hw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
index 32adbcbe6175..07520a2ce179 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
@@ -255,10 +255,10 @@ int cedrus_hw_probe(struct cedrus_dev *dev)
res = platform_get_resource(dev->pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(dev->dev, res);
- if (!dev->base) {
+ if (IS_ERR(dev->base)) {
v4l2_err(&dev->v4l2_dev, "Failed to map registers\n");
- ret = -ENOMEM;
+ ret = PTR_ERR(dev->base);
goto err_sram;
}
--
2.11.0
Re: [PATCH 3/6] media: ov2659: get rid of extra ifdefs
Hi Lubomir,
url:
https://github.com/0day-ci/linux/commits/Lubomir-Rintel/media-don-t-ifdef-v4l2_subdev_get_try_format-any-more/20181129-205631
base: git://linuxtv.org/media_tree.git master
smatch warnings:
drivers/media/i2c/ov2659.c:1157 ov2659_set_fmt() warn: inconsistent returns
'mutex:&ov2659->lock'.
Locked on: line 1129
Unlocked on: line 1119
#
https://github.com/0day-ci/linux/commit/ceed6707bbb8d34fa04448a9eaf77a574dae59a8
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout ceed6707bbb8d34fa04448a9eaf77a574dae59a8
vim +1157 drivers/media/i2c/ov2659.c
c4c0283a Benoit Parrot 2015-03-20 1098
c4c0283a Benoit Parrot 2015-03-20 1099 static int ov2659_set_fmt(struct
v4l2_subdev *sd,
c4c0283a Benoit Parrot 2015-03-20 1100 struct
v4l2_subdev_pad_config *cfg,
c4c0283a Benoit Parrot 2015-03-20 1101 struct
v4l2_subdev_format *fmt)
c4c0283a Benoit Parrot 2015-03-20 1102 {
c4c0283a Benoit Parrot 2015-03-20 1103struct i2c_client *client =
v4l2_get_subdevdata(sd);
5f5859d1 Dan Carpenter 2015-04-15 1104int index =
ARRAY_SIZE(ov2659_formats);
c4c0283a Benoit Parrot 2015-03-20 1105struct v4l2_mbus_framefmt *mf =
&fmt->format;
c4c0283a Benoit Parrot 2015-03-20 1106const struct ov2659_framesize
*size = NULL;
c4c0283a Benoit Parrot 2015-03-20 1107struct ov2659 *ov2659 =
to_ov2659(sd);
c4c0283a Benoit Parrot 2015-03-20 1108int ret = 0;
c4c0283a Benoit Parrot 2015-03-20 1109
c4c0283a Benoit Parrot 2015-03-20 1110dev_dbg(&client->dev,
"ov2659_set_fmt\n");
c4c0283a Benoit Parrot 2015-03-20
c4c0283a Benoit Parrot 2015-03-20 1112__ov2659_try_frame_size(mf,
&size);
c4c0283a Benoit Parrot 2015-03-20 1113
c4c0283a Benoit Parrot 2015-03-20 1114while (--index >= 0)
c4c0283a Benoit Parrot 2015-03-20 1115if
(ov2659_formats[index].code == mf->code)
c4c0283a Benoit Parrot 2015-03-20 1116break;
c4c0283a Benoit Parrot 2015-03-20 1117
c4c0283a Benoit Parrot 2015-03-20 1118if (index < 0)
c4c0283a Benoit Parrot 2015-03-20 1119return -EINVAL;
c4c0283a Benoit Parrot 2015-03-20 1120
c4c0283a Benoit Parrot 2015-03-20 1121mf->colorspace =
V4L2_COLORSPACE_SRGB;
c4c0283a Benoit Parrot 2015-03-20 1122mf->field = V4L2_FIELD_NONE;
c4c0283a Benoit Parrot 2015-03-20 1123
c4c0283a Benoit Parrot 2015-03-20 1124mutex_lock(&ov2659->lock);
c4c0283a Benoit Parrot 2015-03-20 1125
c4c0283a Benoit Parrot 2015-03-20 1126if (fmt->which ==
V4L2_SUBDEV_FORMAT_TRY) {
c4c0283a Benoit Parrot 2015-03-20 1127mf =
v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
ceed6707 Lubomir Rintel 2018-11-28 1128if (IS_ERR(mf))
ceed6707 Lubomir Rintel 2018-11-28 1129return
PTR_ERR(mf);
^^
goto unlock;
c4c0283a Benoit Parrot 2015-03-20 1130*mf = fmt->format;
c4c0283a Benoit Parrot 2015-03-20 1131} else {
c4c0283a Benoit Parrot 2015-03-20 1132s64 val;
c4c0283a Benoit Parrot 2015-03-20 1133
c4c0283a Benoit Parrot 2015-03-20 1134if (ov2659->streaming) {
c4c0283a Benoit Parrot 2015-03-20 1135
mutex_unlock(&ov2659->lock);
c4c0283a Benoit Parrot 2015-03-20 1136return -EBUSY;
c4c0283a Benoit Parrot 2015-03-20 1137}
c4c0283a Benoit Parrot 2015-03-20 1138
c4c0283a Benoit Parrot 2015-03-20 1139ov2659->frame_size =
size;
c4c0283a Benoit Parrot 2015-03-20 1140ov2659->format =
fmt->format;
c4c0283a Benoit Parrot 2015-03-20 1141
ov2659->format_ctrl_regs =
c4c0283a Benoit Parrot 2015-03-20 1142
ov2659_formats[index].format_ctrl_regs;
c4c0283a Benoit Parrot 2015-03-20 1143
c4c0283a Benoit Parrot 2015-03-20 1144if (ov2659->format.code
!= MEDIA_BUS_FMT_SBGGR8_1X8)
c4c0283a Benoit Parrot 2015-03-20 1145val =
ov2659->pdata->link_frequency / 2;
c4c0283a Benoit Parrot 2015-03-20 1146else
c4c0283a Benoit Parrot 2015-03-20 1147val =
ov2659->pdata->link_frequency;
c4c0283a Benoit Parrot 2015-03-20 1148
c4c0283a Benoit Parrot 2015-03-20 1149ret =
v4l2_ctrl_s_ctrl_int64(ov2659->link_frequency, val);
c4c0283a Benoit Parrot 2015-03-20 1150if (ret < 0)
c4c0283a Benoit Parrot 2015-03-20 1151
dev_warn(&client->dev,
c4c0283a Benoit Parrot 2015-03-20 1152
"
Re: [PATCH 4/6] media: ov2680: get rid of extra ifdefs
Hi Lubomir,
url:
https://github.com/0day-ci/linux/commits/Lubomir-Rintel/media-don-t-ifdef-v4l2_subdev_get_try_format-any-more/20181129-205631
base: git://linuxtv.org/media_tree.git master
smatch warnings:
drivers/media/i2c/ov2680.c:687 ov2680_get_fmt() warn: inconsistent returns
'mutex:&sensor->lock'.
Locked on: line 677
Unlocked on: line 670
#
https://github.com/0day-ci/linux/commit/45699a2f04294ea9ca96a3d178232ecae7f607ed
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 45699a2f04294ea9ca96a3d178232ecae7f607ed
vim +687 drivers/media/i2c/ov2680.c
3ee47cad Rui Miguel Silva 2018-07-03 660
3ee47cad Rui Miguel Silva 2018-07-03 661 static int ov2680_get_fmt(struct
v4l2_subdev *sd,
3ee47cad Rui Miguel Silva 2018-07-03 662 struct
v4l2_subdev_pad_config *cfg,
3ee47cad Rui Miguel Silva 2018-07-03 663 struct
v4l2_subdev_format *format)
3ee47cad Rui Miguel Silva 2018-07-03 664 {
3ee47cad Rui Miguel Silva 2018-07-03 665 struct ov2680_dev *sensor =
to_ov2680_dev(sd);
3ee47cad Rui Miguel Silva 2018-07-03 666 struct v4l2_mbus_framefmt *fmt
= NULL;
3ee47cad Rui Miguel Silva 2018-07-03 667 int ret = 0;
3ee47cad Rui Miguel Silva 2018-07-03 668
3ee47cad Rui Miguel Silva 2018-07-03 669 if (format->pad != 0)
3ee47cad Rui Miguel Silva 2018-07-03 670 return -EINVAL;
3ee47cad Rui Miguel Silva 2018-07-03 671
3ee47cad Rui Miguel Silva 2018-07-03 672 mutex_lock(&sensor->lock);
3ee47cad Rui Miguel Silva 2018-07-03 673
3ee47cad Rui Miguel Silva 2018-07-03 674 if (format->which ==
V4L2_SUBDEV_FORMAT_TRY) {
3ee47cad Rui Miguel Silva 2018-07-03 675 fmt =
v4l2_subdev_get_try_format(&sensor->sd, cfg, format->pad);
45699a2f Lubomir Rintel 2018-11-28 676 if (IS_ERR(fmt))
45699a2f Lubomir Rintel 2018-11-28 677 return
PTR_ERR(fmt);
^^^
goto unlock;
3ee47cad Rui Miguel Silva 2018-07-03 678 } else {
3ee47cad Rui Miguel Silva 2018-07-03 679 fmt = &sensor->fmt;
3ee47cad Rui Miguel Silva 2018-07-03 680 }
3ee47cad Rui Miguel Silva 2018-07-03 681
3ee47cad Rui Miguel Silva 2018-07-03 682 if (fmt)
3ee47cad Rui Miguel Silva 2018-07-03 683 format->format = *fmt;
3ee47cad Rui Miguel Silva 2018-07-03 684
3ee47cad Rui Miguel Silva 2018-07-03 685 mutex_unlock(&sensor->lock);
3ee47cad Rui Miguel Silva 2018-07-03 686
3ee47cad Rui Miguel Silva 2018-07-03 @687 return ret;
3ee47cad Rui Miguel Silva 2018-07-03 688 }
3ee47cad Rui Miguel Silva 2018-07-03 689
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[bug report] media: staging/intel-ipu3: Add imgu top level pci device driver
Hello Yong Zhi,
The patch 7fc7af649ca7: "media: staging/intel-ipu3: Add imgu top
level pci device driver" from Dec 6, 2018, leads to the following
static checker warning:
drivers/staging/media/ipu3/ipu3.c:493 imgu_isr_threaded()
warn: 'b' is an error pointer or valid
drivers/staging/media/ipu3/ipu3.c
472 static irqreturn_t imgu_isr_threaded(int irq, void *imgu_ptr)
473 {
474 struct imgu_device *imgu = imgu_ptr;
475 struct imgu_media_pipe *imgu_pipe;
476 int p;
477
478 /* Dequeue / queue buffers */
479 do {
480 u64 ns = ktime_get_ns();
481 struct ipu3_css_buffer *b;
482 struct imgu_buffer *buf = NULL;
483 unsigned int node, pipe;
484 bool dummy;
485
486 do {
487 mutex_lock(&imgu->lock);
488 b = ipu3_css_buf_dequeue(&imgu->css);
ipu3_css_buf_dequeue() doesn't return NULL.
489 mutex_unlock(&imgu->lock);
490 } while (PTR_ERR(b) == -EAGAIN);
491
492 if (IS_ERR_OR_NULL(b)) {
^
--> 493 if (!b || PTR_ERR(b) == -EBUSY) /* All done */
^^
When a function returns both NULL and error pointers, then NULL is
considered a special case of success. Like perhaps you request a
feature, but that feature isn't enabled in the config. It's fine,
because the user *chose* to turn off the feature, so it's not an error
but we also don't have a valid pointer we can use.
It looks like you were probably trying to do something like that but
you missed part of the commit? Otherwise we should delete the dead
code.
494 break;
495 dev_err(&imgu->pci_dev->dev,
496 "failed to dequeue buffers (%ld)\n",
497 PTR_ERR(b));
498 break;
499 }
500
regards,
dan carpenter
Re: [PATCH AUTOSEL 4.20 056/117] media: cedrus: don't initialize pointers with zero
This is a pure cleanup patch, it doesn't affect runtime.
On Tue, Jan 08, 2019 at 02:25:24PM -0500, Sasha Levin wrote:
> From: Mauro Carvalho Chehab
>
> [ Upstream commit e4d7b113fdccde1acf8638c5879f2a450d492303 ]
>
> A common mistake is to assume that initializing a var with:
> struct foo f = { 0 };
>
> Would initialize a zeroed struct. Actually, what this does is
> to initialize the first element of the struct to zero.
>
> According to C99 Standard 6.7.8.21:
>
> "If there are fewer initializers in a brace-enclosed
> list than there are elements or members of an aggregate,
> or fewer characters in a string literal used to initialize
> an array of known size than there are elements in the array,
> the remainder of the aggregate shall be initialized implicitly
> the same as objects that have static storage duration."
Static storage is initialized to zero so this is fine. It's just
that Sparse complains if you mix NULL and zero.
regards,
dan carpenter
[PATCH] media: s5k4ecgx: delete a bogus error message
This function prints an error message on success. I don't have the
hardware, I just noticed this while reading the code.
Fixes: 8b99312b7214 ("[media] Add v4l2 subdev driver for S5K4ECGX sensor")
Signed-off-by: Dan Carpenter
---
drivers/media/i2c/s5k4ecgx.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c
index 79aa2740edc4..79c1894c2c83 100644
--- a/drivers/media/i2c/s5k4ecgx.c
+++ b/drivers/media/i2c/s5k4ecgx.c
@@ -263,8 +263,6 @@ static int s5k4ecgx_read(struct i2c_client *client, u32
addr, u16 *val)
ret = s5k4ecgx_i2c_write(client, REG_CMDRD_ADDRL, low);
if (!ret)
ret = s5k4ecgx_i2c_read(client, REG_CMDBUF0_ADDR, val);
- if (!ret)
- dev_err(&client->dev, "Failed to execute read command\n");
return ret;
}
--
2.17.1
Re: [PATCH 3/3] Staging: media: ipu3: fixed max charecter style issue
On Sun, Jan 27, 2019 at 10:24:16PM +0530, Prashantha SP wrote:
> fixed coding style issue.
>
> Signed-off-by: Prashantha SP
^^
Please use your full name that you would use to sign legal documents.
> ---
> drivers/staging/media/ipu3/ipu3-css.c | 178 ++
> 1 file changed, 94 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3-css.c
> b/drivers/staging/media/ipu3/ipu3-css.c
> index 44c55639389a..466a1a8cc422 100644
> --- a/drivers/staging/media/ipu3/ipu3-css.c
> +++ b/drivers/staging/media/ipu3/ipu3-css.c
> @@ -186,7 +186,8 @@ static bool ipu3_css_queue_enabled(struct ipu3_css_queue
> *q)
> /*** css hw ***/
>
> /* In the style of writesl() defined in include/asm-generic/io.h */
> -static inline void writes(const void *mem, ssize_t count, void __iomem *addr)
> +static inline void writes(const void *mem, ssize_t count,
> + void __iomem *addr)
> {
> if (count >= 4) {
> const u32 *buf = mem;
> @@ -671,8 +672,9 @@ static void ipu3_css_pipeline_cleanup(struct ipu3_css
> *css, unsigned int pipe)
> ipu3_css_pool_cleanup(imgu, &css->pipes[pipe].pool.obgrid);
>
> for (i = 0; i < IMGU_ABI_NUM_MEMORIES; i++)
> - ipu3_css_pool_cleanup(imgu,
> -
> &css->pipes[pipe].pool.binary_params_p[i]);
> + ipu3_css_pool_cleanup
> + (imgu,
> + &css->pipes[pipe].pool.binary_params_p[i]);
The original is better.
regards,
dan carpenter
[bug report] [media] davinci: vpfe: add v4l2 video driver support
[ This is really old, but it looks like a potentially serious security
bug so we probably want to fix it. -dan ]
Hello Manjunath Hadli,
The patch 622897da67b3: "[media] davinci: vpfe: add v4l2 video driver
support" from Nov 28, 2012, leads to the following static checker
warning:
drivers/staging/media/davinci_vpfe/vpfe_video.c:871 vpfe_s_input()
warn: uncapped user index 'sdinfo->routes[index]'
drivers/staging/media/davinci_vpfe/vpfe_video.c
821 /*
822 * vpfe_s_input() - set input which is pointed by input index
823 * @file: file pointer
824 * @priv: void pointer
825 * @index: pointer to unsigned int
826 *
827 * set input on external subdev
828 *
829 * Return 0 on success, error code otherwise
830 */
831 static int vpfe_s_input(struct file *file, void *priv, unsigned int
index)
^^
index comes from __video_do_ioctl() -> v4l_s_input() -> vpfe_s_input().
It hasn't been checked.
832 {
833 struct vpfe_video_device *video = video_drvdata(file);
834 struct vpfe_device *vpfe_dev = video->vpfe_dev;
835 struct vpfe_ext_subdev_info *sdinfo;
836 struct vpfe_route *route;
837 struct v4l2_input *inps;
838 u32 output;
839 u32 input;
840 int ret;
841 int i;
842
843 v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_s_input\n");
844
845 ret = mutex_lock_interruptible(&video->lock);
846 if (ret)
847 return ret;
848 /*
849 * If streaming is started return device busy
850 * error
851 */
852 if (video->started) {
853 v4l2_err(&vpfe_dev->v4l2_dev, "Streaming is on\n");
854 ret = -EBUSY;
855 goto unlock_out;
856 }
857
858 sdinfo = video->current_ext_subdev;
859 if (!sdinfo->registered) {
860 ret = -EINVAL;
861 goto unlock_out;
862 }
863 if (vpfe_dev->cfg->setup_input &&
864 vpfe_dev->cfg->setup_input(sdinfo->grp_id) < 0) {
865 ret = -EFAULT;
866 v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
867"couldn't setup input for %s\n",
868sdinfo->module_name);
869 goto unlock_out;
870 }
871 route = &sdinfo->routes[index];
We're potentially reading out of bounds here. The problem is that we
don't store the size of the ->routes[] array anywhere (it has a sentinal
at the end) so I'm not sure what to check against.
Please CC me on the fix.
872 if (route && sdinfo->can_route) {
873 input = route->input;
874 output = route->output;
875 ret = v4l2_device_call_until_err(&vpfe_dev->v4l2_dev,
876 sdinfo->grp_id, video,
877 s_routing, input,
output, 0);
878 if (ret) {
879 v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
880 "s_input:error in setting input in
decoder\n");
881 ret = -EINVAL;
882 goto unlock_out;
883 }
884 }
885 /* set standards set by subdev in video device */
886 for (i = 0; i < sdinfo->num_inputs; i++) {
887 inps = &sdinfo->inputs[i];
888 video->video_dev.tvnorms |= inps->std;
889 }
890 video->current_input = index;
891 unlock_out:
892 mutex_unlock(&video->lock);
893 return ret;
894 }
regards,
dan carpenter
Re: [PATCH v5 5/5] [media] allegro: add SPS/PPS nal unit writer
Hi Michael,
url:
https://github.com/0day-ci/linux/commits/Michael-Tretter/Add-ZynqMP-VCU-Allegro-DVT-H-264-encoder-driver/20190504-161958
base: git://linuxtv.org/media_tree.git master
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/media/platform/allegro-dvt/nal-h264.c:205 rbsp_read_bit() warn:
signedness bug returning '(-22)'
drivers/media/platform/allegro-dvt/nal-h264.c:259 rbsp_read_bits() warn:
unsigned 'bit' is never less than zero.
#
https://github.com/0day-ci/linux/commit/eba69588199f08008a1fb4ad24e1f3e66d0080e3
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout eba69588199f08008a1fb4ad24e1f3e66d0080e3
vim +205 drivers/media/platform/allegro-dvt/nal-h264.c
eba69588 Michael Tretter 2019-05-03 188
eba69588 Michael Tretter 2019-05-03 189 static inline bool
rbsp_read_bit(struct rbsp *rbsp)
eba69588 Michael Tretter 2019-05-03 190 {
eba69588 Michael Tretter 2019-05-03 191int shift;
eba69588 Michael Tretter 2019-05-03 192int ofs;
eba69588 Michael Tretter 2019-05-03 193bool bit;
eba69588 Michael Tretter 2019-05-03 194int err;
eba69588 Michael Tretter 2019-05-03 195
eba69588 Michael Tretter 2019-05-03 196if (rbsp->num_consecutive_zeros
== 22) {
eba69588 Michael Tretter 2019-05-03 197err =
discard_emulation_prevention_three_byte(rbsp);
eba69588 Michael Tretter 2019-05-03 198if (err)
eba69588 Michael Tretter 2019-05-03 199return err;
^^
eba69588 Michael Tretter 2019-05-03 200}
eba69588 Michael Tretter 2019-05-03 201
eba69588 Michael Tretter 2019-05-03 202shift = 7 - (rbsp->pos % 8);
eba69588 Michael Tretter 2019-05-03 203ofs = rbsp->pos / 8;
eba69588 Michael Tretter 2019-05-03 204if (ofs >= rbsp->size)
eba69588 Michael Tretter 2019-05-03 @205return -EINVAL;
^^
Probably this function should return int instead of bool.
eba69588 Michael Tretter 2019-05-03 206
eba69588 Michael Tretter 2019-05-03 207bit = (rbsp->data[ofs] >>
shift) & 1;
eba69588 Michael Tretter 2019-05-03 208
eba69588 Michael Tretter 2019-05-03 209rbsp->pos++;
eba69588 Michael Tretter 2019-05-03 210
eba69588 Michael Tretter 2019-05-03 211if (bit == 1 ||
eba69588 Michael Tretter 2019-05-03 212
(rbsp->num_consecutive_zeros < 7 && (rbsp->pos % 8 == 0)))
eba69588 Michael Tretter 2019-05-03 213
rbsp->num_consecutive_zeros = 0;
eba69588 Michael Tretter 2019-05-03 214else
eba69588 Michael Tretter 2019-05-03 215
rbsp->num_consecutive_zeros++;
eba69588 Michael Tretter 2019-05-03 216
eba69588 Michael Tretter 2019-05-03 217return bit;
eba69588 Michael Tretter 2019-05-03 218 }
[ snip ]
eba69588 Michael Tretter 2019-05-03 248 static inline int
rbsp_read_bits(struct rbsp *rbsp, int n, unsigned int *value)
eba69588 Michael Tretter 2019-05-03 249 {
eba69588 Michael Tretter 2019-05-03 250int i;
eba69588 Michael Tretter 2019-05-03 251unsigned int bit;
eba69588 Michael Tretter 2019-05-03 252unsigned int tmp = 0;
eba69588 Michael Tretter 2019-05-03 253
eba69588 Michael Tretter 2019-05-03 254if (n > 8 * sizeof(*value))
eba69588 Michael Tretter 2019-05-03 255return -EINVAL;
eba69588 Michael Tretter 2019-05-03 256
eba69588 Michael Tretter 2019-05-03 257for (i = n; i > 0; i--) {
eba69588 Michael Tretter 2019-05-03 258bit =
rbsp_read_bit(rbsp);
eba69588 Michael Tretter 2019-05-03 @259if (bit < 0)
^^^
eba69588 Michael Tretter 2019-05-03 260return bit;
eba69588 Michael Tretter 2019-05-03 261tmp |= bit << (i - 1);
eba69588 Michael Tretter 2019-05-03 262}
eba69588 Michael Tretter 2019-05-03 263
eba69588 Michael Tretter 2019-05-03 264if (value)
eba69588 Michael Tretter 2019-05-03 265*value = tmp;
eba69588 Michael Tretter 2019-05-03 266
eba69588 Michael Tretter 2019-05-03 267return 0;
eba69588 Michael Tretter 2019-05-03 268 }
eba69588 Michael Tretter 2019-05-03 269
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[bug report] media: platform: meson: Add Amlogic Meson G12A AO CEC Controller driver
Hello Neil Armstrong,
The patch b7778c46683c: "media: platform: meson: Add Amlogic Meson
G12A AO CEC Controller driver" from Apr 12, 2019, leads to the
following static checker warning:
drivers/media/platform/meson/ao-cec-g12a.c:377 meson_ao_cec_g12a_read()
error: scheduling with locks held: 'spin_lock:cec_reg_lock'
drivers/media/platform/meson/ao-cec-g12a.c
363 static int meson_ao_cec_g12a_read(void *context, unsigned int addr,
364unsigned int *data)
365 {
366 struct meson_ao_cec_g12a_device *ao_cec = context;
367 u32 reg = FIELD_PREP(CECB_RW_ADDR, addr);
368 unsigned long flags;
369 int ret = 0;
370
371 spin_lock_irqsave(&ao_cec->cec_reg_lock, flags);
^
Atomic context.
372
373 ret = regmap_write(ao_cec->regmap, CECB_RW_REG, reg);
374 if (ret)
375 goto read_out;
376
377 ret = regmap_read_poll_timeout(ao_cec->regmap, CECB_RW_REG, reg,
378 !(reg & CECB_RW_BUS_BUSY),
379 5, 1000);
^
It sleeps for 5 usecs.
380 if (ret)
381 goto read_out;
382
383 ret = regmap_read(ao_cec->regmap, CECB_RW_REG, ®);
384
385 *data = FIELD_GET(CECB_RW_RD_DATA, reg);
386
387 read_out:
388 spin_unlock_irqrestore(&ao_cec->cec_reg_lock, flags);
389
390 return ret;
391 }
regards,
dan carpenter
[PATCH] media: staging/imx: fix two NULL vs IS_ERR() bugs
The imx_media_pipeline_pad() function return NULL pointers on error, it
never returns error pointers.
Fixes: 3ef46bc97ca2 ("media: staging/imx: Improve pipeline searching")
Signed-off-by: Dan Carpenter
---
drivers/staging/media/imx/imx-media-csi.c | 4 ++--
drivers/staging/media/imx/imx7-media-csi.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index d2f880938af9..0eeb0db6d83f 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -193,8 +193,8 @@ static int csi_get_upstream_endpoint(struct csi_priv *priv,
/* get source pad of entity directly upstream from src */
pad = imx_media_pipeline_pad(src, 0, 0, true);
- if (IS_ERR(pad))
- return PTR_ERR(pad);
+ if (!pad)
+ return -ENODEV;
sd = media_entity_to_v4l2_subdev(pad->entity);
diff --git a/drivers/staging/media/imx/imx7-media-csi.c
b/drivers/staging/media/imx/imx7-media-csi.c
index b1af8694899e..882690561357 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -439,8 +439,8 @@ static int imx7_csi_get_upstream_endpoint(struct imx7_csi
*csi,
skip_video_mux:
/* get source pad of entity directly upstream from src */
pad = imx_media_pipeline_pad(src, 0, 0, true);
- if (IS_ERR(pad))
- return PTR_ERR(pad);
+ if (!pad)
+ return -ENODEV;
sd = media_entity_to_v4l2_subdev(pad->entity);
--
2.20.1
[PATCH] media: rockchip/vpu: remove an unnecessary NULL check
Thus the address of "&ctx->dev->variant->codec_ops[codec_mode]" can't possibly be NULL. Signed-off-by: Dan Carpenter --- drivers/staging/media/rockchip/vpu/rockchip_vpu_v4l2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_v4l2.c b/drivers/staging/media/rockchip/vpu/rockchip_vpu_v4l2.c index 1b80a45df8fe..42f4eb0abc8a 100644 --- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_v4l2.c +++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_v4l2.c @@ -619,7 +619,7 @@ static int rockchip_vpu_start_streaming(struct vb2_queue *q, vpu_debug(4, "Codec mode = %d\n", codec_mode); ctx->codec_ops = &ctx->dev->variant->codec_ops[codec_mode]; - if (ctx->codec_ops && ctx->codec_ops->init) + if (ctx->codec_ops->init) ret = ctx->codec_ops->init(ctx); } -- 2.20.1
Re: [PATCH] media: imx7-media-csi: get csi upstream endpoint
On Tue, Jun 11, 2019 at 04:09:55PM +0100, Rui Miguel Silva wrote:
> When the upstream endpoint is neither a mux nor a CSI2 module, just get
> the source pad directly upstream from the CSI.
>
> Fixes: 05f634040c0d ("media: staging/imx7: add imx7 CSI subdev driver")
> Reported-by: Sebastien Szymanski
> Signed-off-by: Rui Miguel Silva
> ---
> drivers/staging/media/imx/imx7-media-csi.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c
> b/drivers/staging/media/imx/imx7-media-csi.c
> index 9101566f3f67..8979ee0c8202 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -442,6 +442,14 @@ static int imx7_csi_get_upstream_endpoint(struct
> imx7_csi *csi,
>
> src = &csi->src_sd->entity;
>
> + /*
> + * if the source in neither a mux or csi2 get the one directly upstream
^^
is?
> + * from this csi
> + */
> + if (src->function != MEDIA_ENT_F_VID_IF_BRIDGE &&
> + src->function != MEDIA_ENT_F_VID_MUX)
> + src = &csi->sd.entity;
This would be easier to read if the white space were tweaked a little:
if (src->function != MEDIA_ENT_F_VID_IF_BRIDGE &&
src->function != MEDIA_ENT_F_VID_MUX)
src = &csi->sd.entity;
regards,
dan carpenter
[bug report] media: stv090x: Implement probe/remove for stv090x
Hello Tobias Klausmann,
The patch eb5005df886b: "media: stv090x: Implement probe/remove for
stv090x" from May 29, 2019, leads to the following static checker
warning:
drivers/media/dvb-frontends/stv090x.c:5032 stv090x_probe()
warn: 'state' was already freed.
drivers/media/dvb-frontends/stv090x.c
4994 static int stv090x_probe(struct i2c_client *client,
4995 const struct i2c_device_id *id)
4996 {
4997 int ret = 0;
4998 struct stv090x_config *config = client->dev.platform_data;
4999
5000 struct stv090x_state *state = NULL;
5001
5002 state = kzalloc(sizeof(*state), GFP_KERNEL);
5003 if (!state) {
5004 ret = -ENOMEM;
5005 goto error;
5006 }
5007
5008 state->verbose = &verbose;
5009 state->config = config;
5010 state->i2c = client->adapter;
5011 state->frontend.ops = stv090x_ops;
5012 state->frontend.demodulator_priv= state;
5013 state->demod= config->demod;
5014 /* Single or Dual mode
*/
5015 state->demod_mode = config->demod_mode;
5016 state->device = config->device;
5017 /* default */
5018 state->rolloff = STV090x_RO_35;
5019
5020 ret = stv090x_setup_compound(state);
5021 if (ret)
5022 goto error;
stv090x_setup_compound() frees "state" on error.
5023
5024 i2c_set_clientdata(client, state);
5025
5026 /* setup callbacks */
5027 config->get_dvb_frontend = stv090x_get_dvb_frontend;
5028
5029 return 0;
5030
5031 error:
5032 kfree(state);
^^^^
Double free.
5033 return ret;
5034 }
regards,
dan carpenter
[bug report] [media] vmalloc_sg: make sure all pages in vmalloc area are really DMA-ready
Hi linux-media devs, this is similar to one of HCH's patches that
hasn't been applied yet.
The patch 7b4eeed174b7: "[media] vmalloc_sg: make sure all pages in
vmalloc area are really DMA-ready" from Jun 12, 2014, leads to the
following static checker warning:
drivers/media/v4l2-core/videobuf-dma-sg.c:236 videobuf_dma_init_kernel()
error: 'addr' came from dma_alloc_coherent() so we can't do
virt_to_phys()
drivers/media/v4l2-core/videobuf-dma-sg.c
210 static int videobuf_dma_init_kernel(struct videobuf_dmabuf *dma, int
direction,
211 int nr_pages)
212 {
213 int i;
214
215 dprintk(1, "init kernel [%d pages]\n", nr_pages);
216
217 dma->direction = direction;
218 dma->vaddr_pages = kcalloc(nr_pages, sizeof(*dma->vaddr_pages),
219 GFP_KERNEL);
220 if (!dma->vaddr_pages)
221 return -ENOMEM;
222
223 dma->dma_addr = kcalloc(nr_pages, sizeof(*dma->dma_addr),
GFP_KERNEL);
224 if (!dma->dma_addr) {
225 kfree(dma->vaddr_pages);
226 return -ENOMEM;
227 }
228 for (i = 0; i < nr_pages; i++) {
229 void *addr;
230
231 addr = dma_alloc_coherent(dma->dev, PAGE_SIZE,
^
232&(dma->dma_addr[i]),
GFP_KERNEL);
233 if (addr == NULL)
234 goto out_free_pages;
235
236 dma->vaddr_pages[i] = virt_to_page(addr);
^^
Apparently this isn't allowed.
237 }
238 dma->vaddr = vmap(dma->vaddr_pages, nr_pages, VM_MAP |
VM_IOREMAP,
239PAGE_KERNEL);
240 if (NULL == dma->vaddr) {
241 dprintk(1, "vmalloc_32(%d pages) failed\n", nr_pages);
242 goto out_free_pages;
243 }
244
245 dprintk(1, "vmalloc is at addr %p, size=%d\n",
246 dma->vaddr, nr_pages << PAGE_SHIFT);
247
248 memset(dma->vaddr, 0, nr_pages << PAGE_SHIFT);
249 dma->nr_pages = nr_pages;
250
251 return 0;
252 out_free_pages:
253 while (i > 0) {
254 void *addr;
regards,
dan carpenter
Re: [PATCH 01/31] staging: bcm2835-camera: Ensure H264 header bytes get a sensible timestamp
On Thu, Jun 27, 2019 at 08:55:58PM +0200, Stefan Wahren wrote:
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h
> b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h
> index 2b5679e..09273b0 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h
> +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.h
> @@ -90,6 +90,8 @@ struct bm2835_mmal_dev {
> s64 vc_start_timestamp;
> /* Kernel start timestamp for streaming */
> ktime_t kernel_start_ts;
> + /* Timestamp of last frame */
> + u64 last_timestamp;
Not directly related to this patch but the indenting in this .h file is
all higgle-piggledy.
regards,
dan carpenter
Re: [PATCH 06/31] staging: bcm2835-camera: Return early on errors
On Thu, Jun 27, 2019 at 08:56:03PM +0200, Stefan Wahren wrote: > v4l2_dbg(1, bcm2835_v4l2_debug, &dev->v4l2_dev, "connecting %p to %p\n", >src, dst); > ret = vchiq_mmal_port_connect_tunnel(dev->instance, src, dst); > if (!ret) > ret = vchiq_mmal_port_enable(dev->instance, src, NULL); > -error: > + > return ret; In future patches, you probably want to flip this one around as well. Try to do error handling instead of success handling. In other words, keep the success patch indented one tab and the failure path indented two tabs. Don't make the last failure check in the function special. ret = vchiq_mmal_port_connect_tunnel(dev->instance, src, dst); if (ret) return ret; ret = vchiq_mmal_port_enable(dev->instance, src, NULL); if (ret) return ret; return 0; Or you can make the last check a little special if you want... return vchiq_mmal_port_enable(dev->instance, src, NULL); Either format is good. regards, dan carpenter
Re: [PATCH 29/31] staging: bcm2835-camera: Add sanity checks for queue_setup/CREATE_BUFS
On Thu, Jun 27, 2019 at 11:09:27PM +0200, Stefan Wahren wrote: > From: Dave Stevenson > > Fixes a v4l2-compliance failure when passed a buffer that is > too small. > queue_setup wasn't handling the case where !(*nplanes), as ^^^ This is reversed? It wasn't handling where *nplanes is non-zero. > used from CREATE_BUFS and requiring the driver to sanity > check the provided buffer parameters. It was assuming that > it was always being used in the REQBUFS case where it provides > the buffer properties. These patches look really nice. regards, dan carpenter
Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug
On Wed, May 02, 2018 at 05:38:49PM -0500, Wenwen Wang wrote: > At the end of atomisp_subdev_set_selection(), the function > atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since > this function may return a NULL pointer, it is firstly invoked to check > the returned pointer. If the returned pointer is not NULL, then the > function is invoked again to obtain the pointer and the memory content > at the location of the returned pointer is copied to the memory location of > r. In most cases, the pointers returned by the two invocations are same. > However, given that the pointer returned by the function > atomisp_subdev_get_rect() is not a constant, it is possible that the two > invocations return two different pointers. For example, another thread may > race to modify the related pointers during the two invocations. You're assuming a very serious race condition exists. > In that > case, even if the first returned pointer is not null, the second returned > pointer might be null, which will cause issues such as null pointer > dereference. And then complaining that if a really serious bug exists then this very minor bug would exist too... If there were really a race condition like that then we'd want to fix it instead. In other words, this is not a real life bug fix. But it would be fine as a readability or static checker fix so that's fine. regards, dan carpenter
Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1
On Tue, May 15, 2018 at 08:59:53AM -0300, Mauro Carvalho Chehab wrote: > Em Mon, 14 May 2018 22:31:37 -0500 > "Gustavo A. R. Silva" escreveu: > > > Hi Mauro, > > > > On 04/26/2018 06:42 PM, Mauro Carvalho Chehab wrote: > > > > >> > > >> I noticed you changed the status of this series from rejected to new. > > > > > > Yes. > > > > > >> Also, there are other similar issues in media/pci/ > > > > > > Well, the issues will be there everywhere on all media drivers. > > > > > > I marked your patches because I need to study it carefully, after > > > Peter's explanations. My plan is to do it next week. Still not > > > sure if the approach you took is the best one or not. > > > > > > As I said, one possibility is to change the way v4l2-core handles > > > VIDIOC_ENUM_foo ioctls, but that would be make harder to -stable > > > backports. > > > > > > I need a weekend to sleep on it. > > > > > > > I'm curious about how you finally resolved to handle these issues. > > > > I noticed Smatch is no longer reporting them. > > There was no direct fix for it, but maybe this patch has something > to do with the smatch error report cleanup: > > commit 3ad3b7a2ebaefae37a7eafed0779324987ca5e56 > Author: Sami Tolvanen > Date: Tue May 8 13:56:12 2018 -0400 > > media: v4l2-ioctl: replace IOCTL_INFO_STD with stub functions > > This change removes IOCTL_INFO_STD and adds stub functions where > needed using the DEFINE_V4L_STUB_FUNC macro. This fixes indirect call > mismatches with Control-Flow Integrity, caused by calling standard > ioctls using a function pointer that doesn't match the function type. > > Signed-off-by: Sami Tolvanen > Signed-off-by: Hans Verkuil > Signed-off-by: Mauro Carvalho Chehab > Possibly... There was an ancient bug in Smatch's function pointer handling. I just pushed a fix for it now so the warning is there on linux-next. regards, dan carpenter
Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1
On Tue, May 15, 2018 at 12:29:10PM -0500, Gustavo A. R. Silva wrote: > > > On 05/15/2018 09:16 AM, Dan Carpenter wrote: > > > > > > > > I'm curious about how you finally resolved to handle these issues. > > > > > > > > I noticed Smatch is no longer reporting them. > > > > > > There was no direct fix for it, but maybe this patch has something > > > to do with the smatch error report cleanup: > > > > > > commit 3ad3b7a2ebaefae37a7eafed0779324987ca5e56 > > > Author: Sami Tolvanen > > > Date: Tue May 8 13:56:12 2018 -0400 > > > > > > media: v4l2-ioctl: replace IOCTL_INFO_STD with stub functions > > > This change removes IOCTL_INFO_STD and adds stub functions where > > > needed using the DEFINE_V4L_STUB_FUNC macro. This fixes indirect call > > > mismatches with Control-Flow Integrity, caused by calling standard > > > ioctls using a function pointer that doesn't match the function type. > > > Signed-off-by: Sami Tolvanen > > > Signed-off-by: Hans Verkuil > > > Signed-off-by: Mauro Carvalho Chehab > > > > > Thanks, Mauro. > > > > > Possibly... There was an ancient bug in Smatch's function pointer > > handling. I just pushed a fix for it now so the warning is there on > > linux-next. > > > > Dan, > > These are all the Spectre media issues I see smatch is reporting in > linux-next-20180515: > > drivers/media/cec/cec-pin-error-inj.c:170 cec_pin_error_inj_parse_line() > warn: potential spectre issue 'pin->error_inj_args' > drivers/media/dvb-core/dvb_ca_en50221.c:1479 dvb_ca_en50221_io_write() warn: > potential spectre issue 'ca->slot_info' (local cap) > drivers/media/dvb-core/dvb_net.c:252 handle_one_ule_extension() warn: > potential spectre issue 'p->ule_next_hdr' > > I pulled the latest changes from the smatch repository and compiled it. > > I'm running smatch v0.5.0-4459-g2f66d40 now. Is this the latest version? > > I wonder if there is anything I might be missing. > You'd need to rebuild the db (possibly twice but definitely once). regards, dan carpenter
Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1
On Tue, May 15, 2018 at 04:00:33PM -0300, Mauro Carvalho Chehab wrote: > Yeah, that's the same I'm getting from media upstream. > > > drivers/media/cec/cec-pin-error-inj.c:170 cec_pin_error_inj_parse_line() > > warn: potential spectre issue 'pin->error_inj_args' > > This one seems a false positive, as the index var is u8 and the > array has 256 elements, as the userspace input from 'op' is > initialized with: > > u8 v; > u32 op; > > if (!kstrtou8(token, 0, &v)) > op = v; > It's hard to silence this because Smatch stores the current user controlled range list, not what it was initially. I wrote all this code to detect bounds checking errors, so there wasn't any need to save the range list before the bounds check. Since "op" is a u32, I can't even go by the type of the index > > drivers/media/dvb-core/dvb_ca_en50221.c:1479 dvb_ca_en50221_io_write() > > warn: potential spectre issue 'ca->slot_info' (local cap) > > This one seems a real issue to me. Sent a patch for it. > > > drivers/media/dvb-core/dvb_net.c:252 handle_one_ule_extension() warn: > > potential spectre issue 'p->ule_next_hdr' > > I failed to see what's wrong here, or if this is exploited. Oh... Huh. This is a bug in smatch. That line looks like: p->ule_sndu_type = ntohs(*(__be16 *)(p->ule_next_hdr + ((p->ule_dbit ? 2 : 3) * ETH_ALEN))); Smatch see the ntohs() and marks everything inside it as untrusted network data. I'll fix this. regards, dan carpenter
[PATCH] media: vivid: potential integer overflow in vidioc_g_edid()
If we pick a very large "edid->blocks" value then the "edid->start_block
+ edid->blocks" addition could wrap around.
Fixes: ef834f7836ec ("[media] vivid: add the video capture and output parts")
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/platform/vivid/vivid-vid-common.c
b/drivers/media/platform/vivid/vivid-vid-common.c
index e5914be0e12d..be531caa2cdf 100644
--- a/drivers/media/platform/vivid/vivid-vid-common.c
+++ b/drivers/media/platform/vivid/vivid-vid-common.c
@@ -860,7 +860,7 @@ int vidioc_g_edid(struct file *file, void *_fh,
return -ENODATA;
if (edid->start_block >= dev->edid_blocks)
return -EINVAL;
- if (edid->start_block + edid->blocks > dev->edid_blocks)
+ if (edid->blocks > dev->edid_blocks - edid->start_block)
edid->blocks = dev->edid_blocks - edid->start_block;
if (adap)
cec_set_edid_phys_addr(dev->edid, dev->edid_blocks * 128,
adap->phys_addr);
[PATCH] media: v4l2-ioctl: prevent underflow in v4l_enumoutput()
My Smatch allmodconfig build only detects one function implementing
vpbe_device_ops->enum_outputs and that's vpbe_enum_outputs(). The
problem really happens in that function when we do:
int temp_index = output->index;
if (temp_index >= cfg->num_outputs)
return -EINVAL;
Unfortunately, both temp_index and cfg->num_outputs are type int so we
have a potential read before the start of the array if "temp_index" is
negative.
I could have fixed the bug in that function but it's more secure and
future proof to block that bug earlier in a central place. There is no
one who need p->index to be more than INT_MAX.
Fixes: 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index a40dbec271f1..115757ab8bc0 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1099,6 +1099,9 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops
*ops,
if (is_valid_ioctl(vfd, VIDIOC_S_STD))
p->capabilities |= V4L2_OUT_CAP_STD;
+ if (p->index > INT_MAX)
+ return -EINVAL;
+
return ops->vidioc_enum_output(file, fh, p);
}
[PATCH v2] media: davinci vpbe: array underflow in vpbe_enum_outputs()
In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but
the problem is that temp_index can be negative. I've made
cgf->num_outputs unsigned to fix this issue.
Fixes: 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")
Signed-off-by: Dan Carpenter
---
v2: fix it a different way
diff --git a/include/media/davinci/vpbe.h b/include/media/davinci/vpbe.h
index 79a566d7defd..180a05e91497 100644
--- a/include/media/davinci/vpbe.h
+++ b/include/media/davinci/vpbe.h
@@ -92,7 +92,7 @@ struct vpbe_config {
struct encoder_config_info *ext_encoders;
/* amplifier information goes here */
struct amp_config_info *amp;
- int num_outputs;
+ unsigned int num_outputs;
/* Order is venc outputs followed by LCD and then external encoders */
struct vpbe_output *outputs;
};
Re: [PATCH v2] media: davinci vpbe: array underflow in vpbe_enum_outputs()
On Fri, May 25, 2018 at 03:16:21PM +0200, Hans Verkuil wrote: > On 25/05/18 15:12, Dan Carpenter wrote: > > In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but > > the problem is that temp_index can be negative. I've made > > cgf->num_outputs unsigned to fix this issue. > > Shouldn't temp_index also be made unsigned? It certainly would make a lot of > sense to do that. Yeah, sure. It doesn't make any difference in terms of runtime but it's probably cleaner. regards, dan carpenter
Re: [GIT PULL FOR v4.18] R-Car VSP1 TLB optimisation
On Mon, May 28, 2018 at 07:17:54AM -0300, Mauro Carvalho Chehab wrote:
> This (obviously wrong patch) shut up the warning:
>
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -248,6 +248,9 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device
> *vsp1,
> else
> brx = &vsp1->brs->entity;
>
> + if (pipe->brx == brx)
> + pipe->brx = &vsp1->brs->entity;
> +
> /* Switch BRx if needed. */
> if (brx != pipe->brx) {
> struct vsp1_entity *released_brx = NULL;
>
Just to be clear. After this patch, Smatch does *NOT* think that
"pipe->brx" is necessarily non-NULL. What this patch does it that
Smatch says "pipe->brx has been modified on every code path since we
checked for NULL, so forget about the earlier check".
regards,
dan carpenter
Re: [GIT PULL FOR v4.18] R-Car VSP1 TLB optimisation
On Mon, May 28, 2018 at 01:54:18PM +0300, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for your quick reply.
>
> On Monday, 28 May 2018 13:20:49 EEST Dan Carpenter wrote:
> > On Mon, May 28, 2018 at 11:28:41AM +0300, Laurent Pinchart wrote:
> > > and I still get the same warning. I had to write the following (which is
> > > obviously not correct) to silence the warning.
> > >
> > > if (pipe->num_inputs > 2)
> > > brx = &vsp1->bru->entity;
> > > else if (pipe->brx)
> > > brx = pipe->brx;
> > > else if (!vsp1->bru->entity.pipe)
> > > brx = &vsp1->bru->entity;
> > > else {
> > > (void)vsp1->brs->entity;
> > > brx = &vsp1->brs->entity;
> > > }
> > >
> > > Both the (void)vsp1->brs->entity and the removal of the !drm_pipe->
> > > force_brx_release were needed, any of those on its own didn't fix the
> > > problem.
> >
> > The problem in this case is the first "brx = &vsp1->bru->entity;".
> > Smatch assumes &vsp1->bru->entity can be == to pipe->brx and NULL.
>
> Why does smatch assume that &vsp1->bru->entity can be NULL, when the previous
> line dereferences vsp1->bru ?
I'm talking about when pipe->num_inputs > 2. For the second
"brx = &vsp1->bru->entity;" assignment, then Smatch parses it correctly
as you say.
regards,
dan carpenter
Re: [GIT PULL FOR v4.18] R-Car VSP1 TLB optimisation
On Mon, May 28, 2018 at 01:58:41PM +0300, Laurent Pinchart wrote:
> It would indeed be useful to implement, but I share your concern that this
> would be pretty difficult.
>
> However, there's still something that puzzles me. Let's add a bit more
> context.
>
> if (pipe->num_inputs > 2)
> brx = &vsp1->bru->entity;
> else if (pipe->brx && !drm_pipe->force_brx_release)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = &vsp1->bru->entity;
> else
> brx = &vsp1->brs->entity;
>
> 1. if (!brx)
> return -EINVAL;
>
> 2. if (brx != pipe->brx) {
> ...
> 3. pipe->brx = brx;
> ...
> }
>
> 4. format.pad = pipe->brx->source_pad
>
>
> (1) ensures that brx can't be NULL. (2) is thus always true if pipe->brx is
> NULL. (3) then assigns a non-NULL value to pipe->brx. Smatch should thus
> never
> complain about (4), even if it can't backtrack.
Oh wow... That's a very basic and ancient bug. I've written a fix and
will push it out tomorrow if it passes my testing.
regards,
dan carpenter
Re: [GIT PULL FOR v4.18] R-Car VSP1 TLB optimisation
On Mon, May 28, 2018 at 07:03:05AM -0300, Mauro Carvalho Chehab wrote:
> I can't see how brx can be NULL. At the sequence of ifs:
>
> if (pipe->num_inputs > 2)
> brx = &vsp1->bru->entity;
> else if (pipe->brx && !drm_pipe->force_brx_release)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = &vsp1->bru->entity;
> else
> brx = &vsp1->brs->entity;
>
>
> The usage of brx = &(something) will always return a non NULL
> value[1].
That's not right. It can be NULL if it's &foo->first_struct_member
and ->entity is the first struct member. If it weren't the first struct
member then Smatch would say that brx was non-NULL.
> [1] It might be doing a NULL deref - with seems to be your concern
> when you're talking about the case where vsp1->brs is NULL - but
> that's not what Smatch is complaining here.
If vsp1->bru were NULL, it wouldn't be a NULL dereference because it's
not dereferencing it; it's just taking the address. On the path where
we do:
else if (!vsp1->bru->entity.pipe)
brx = &vsp1->bru->entity;
Then Smatch sees that vsp1->bru is dereferenced and marks "brx" as
non-NULL.
regards,
dan carpenter
Re: [GIT PULL FOR v4.18] R-Car VSP1 TLB optimisation
On Mon, May 28, 2018 at 11:31:01AM +0300, Laurent Pinchart wrote: > And that being said, I just tried > > if (pipe->num_inputs > 2) > brx = &vsp1->bru->entity; > else if (pipe->brx && !drm_pipe->force_brx_release) > brx = pipe->brx; > else if (!vsp1->bru->entity.pipe) > brx = &vsp1->bru->entity; > else > brx = &vsp1->brs->entity; > > if (!brx) > return -EINVAL; > > and that didn't help either... Dan, would you have some light to shed on this > problem ? This is a problem in Smatch. We should be able to go backwards and say that "If we know 'brx' is non-NULL then let's mark &vsp1->brs->entity, vsp1->brs, &vsp1->bru->entity and vsp1->bru all as non-NULL as well". But Smatch doesn't go backwards like that. The information is mostly there to do it, but my instinct is that it's really hard to implement. The other potential problem here is that Smatch stores comparisons and values separately. In other words smatch_comparison.c has all the information about brx == &vsp1->bru->entity and smatch_extra.c has the information about if brx is NULL or non-NULL. They don't really share information very well. regards, dan carpenter
Re: [GIT PULL FOR v4.18] R-Car VSP1 TLB optimisation
On Mon, May 28, 2018 at 11:28:41AM +0300, Laurent Pinchart wrote:
> and I still get the same warning. I had to write the following (which is
> obviously not correct) to silence the warning.
>
> if (pipe->num_inputs > 2)
> brx = &vsp1->bru->entity;
> else if (pipe->brx)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = &vsp1->bru->entity;
> else {
> (void)vsp1->brs->entity;
> brx = &vsp1->brs->entity;
> }
>
> Both the (void)vsp1->brs->entity and the removal of the !drm_pipe-
> >force_brx_release were needed, any of those on its own didn't fix the
> problem.
The problem in this case is the first "brx = &vsp1->bru->entity;".
Smatch assumes &vsp1->bru->entity can be == to pipe->brx and NULL.
Adding a "(void)vsp1->bru->entity;" on that path will silence the
warning (hopefully).
regards,
dan carpenter
Re: [GIT PULL FOR v4.18] R-Car VSP1 TLB optimisation
On Sat, May 26, 2018 at 08:28:18AM -0300, Mauro Carvalho Chehab wrote:
> Em Sat, 26 May 2018 03:24:00 +0300
> Laurent Pinchart escreveu:
>
> > Hi Mauro,
> >
> > On Saturday, 26 May 2018 02:39:16 EEST Laurent Pinchart wrote:
> > > On Saturday, 26 May 2018 02:10:27 EEST Mauro Carvalho Chehab wrote:
> > > > Em Sun, 20 May 2018 15:10:50 +0300 Laurent Pinchart escreveu:
> > > >> Hi Mauro,
> > > >>
> > > >> The following changes since commit
> > > >>
> > > >> 8ed8bba70b4355b1ba029b151ade84475dd12991:
> > > >> media: imx274: remove non-indexed pointers from mode_table
> > > >> (2018-05-17
> > > >>
> > > >> 06:22:08 -0400)
> > > >>
> > > >> are available in the Git repository at:
> > > >> git://linuxtv.org/pinchartl/media.git v4l2/vsp1/next
> > > >>
> > > >> for you to fetch changes up to
> > > >> 429f256501652c90a4ed82f2416618f82a77d37c:
> > > >> media: vsp1: Move video configuration to a cached dlb (2018-05-20
> > > >> 09:46:51 +0300)
> > > >>
> > > >> The branch passes the VSP and DU test suites, both on its own and when
> > > >> merged with the drm-next branch.
> > > >
> > > > This series added a new warning:
> > > >
> > > > drivers/media/platform/vsp1/vsp1_dl.c:69: warning: Function parameter or
> > > > member 'refcnt' not described in 'vsp1_dl_body'
> > >
> > > We'll fix that. Kieran, as you authored the code, would you like to give
> > > it
> > > a go ?
> > >
> > > > To the already existing one:
> > > >
> > > > drivers/media/platform/vsp1/vsp1_drm.c:336 vsp1_du_pipeline_setup_brx()
> > > > error: we previously assumed 'pipe->brx' could be null (see line 244)
> > >
> > > That's still on my todo list. I tried to give it a go but received plenty
> > > of
> > > SQL errors. How do you run smatch ?
> >
> > Nevermind, I found out what was wrong (had to specify the data directory
> > manually).
> >
> > I've reproduced the issue and created a minimal test case.
> >
> > 1. struct vsp1_pipeline;
> > 2.
> > 3. struct vsp1_entity {
> > 4. struct vsp1_pipeline *pipe;
> > 5. struct vsp1_entity *sink;
> > 6. unsigned int source_pad;
> > 7. };
> > 8.
> > 9. struct vsp1_pipeline {
> > 10. struct vsp1_entity *brx;
> > 11. };
> > 12.
> > 13. struct vsp1_brx {
> > 14. struct vsp1_entity entity;
> > 15. };
> > 16.
> > 17. struct vsp1_device {
> > 18. struct vsp1_brx *bru;
> > 19. struct vsp1_brx *brs;
> > 20. };
> > 21.
> > 22. unsigned int frob(struct vsp1_device *vsp1, struct vsp1_pipeline *pipe)
> > 23. {
> > 24. struct vsp1_entity *brx;
> > 25.
> > 26. if (pipe->brx)
> > 27. brx = pipe->brx;
> > 28. else if (!vsp1->bru->entity.pipe)
> > 29. brx = &vsp1->bru->entity;
> > 30. else
> > 31. brx = &vsp1->brs->entity;
> > 32.
> > 33. if (brx != pipe->brx)
> > 34. pipe->brx = brx;
> > 35.
> > 36. return pipe->brx->source_pad;
> > 37. }
> >
> > The reason why smatch complains is that it has no guarantee that vsp1->brs
> > is
> > not NULL. It's quite tricky:
> >
> > - On line 26, smatch assumes that pipe->brx can be NULL
> > - On line 27, brx is assigned a non-NULL value (as pipe->brx is not NULL
> > due
> > to line 26)
> > - On line 28, smatch assumes that vsp1->bru is not NULL
> > - On line 29, brx is assigned a non-NULL value (as vsp1->bru is not NULL
> > due
> > to line 28)
> > - On line 31, brx is assigned a possibly NULL value (as there's no
> > information
> > regarding vsp1->brs)
> > - On line 34, pipe->brx is not assigned a non-NULL value if brx is NULL
> > - On line 36 pipe->brx is dereferenced
> >
> > The problem comes from the fact that smatch assumes that vsp1->brs isn't
> > NULL.
> > Adding a "(void)vsp1->brs->entity;" statement on line 25 makes the warning
> > disappear.
> >
I will respond to the other emails in this thread. You guys are
basically spot on. All this analysis is 100% correct. Btw, if you want
to see Smatch's internal state you can do:
#include "/home/whatever/smatch/check_debug.h"
else if (!vsp1->bru->entity.pipe) {
__smatch_implied(&vsp1->bru->entity);
And it tells you what Smatch thinks it is at that point. The
__smatch_about() output can also be useful.
regards,
dan carpenter
[PATCH] media: sr030pc30: inconsistent NULL checking in sr030pc30_base_config()
If info->pdata is NULL then we would oops on the next line. And we can flip the "ret" test around and give up if a failure has already occured. Signed-off-by: Dan Carpenter diff --git a/drivers/media/i2c/sr030pc30.c b/drivers/media/i2c/sr030pc30.c index 2a4882cddc51..4ebd00198d34 100644 --- a/drivers/media/i2c/sr030pc30.c +++ b/drivers/media/i2c/sr030pc30.c @@ -569,8 +569,8 @@ static int sr030pc30_base_config(struct v4l2_subdev *sd) if (!ret) ret = sr030pc30_pwr_ctrl(sd, false, false); - if (!ret && !info->pdata) - return ret; + if (ret || !info->pdata) + return -EIO; expmin = EXPOS_MIN_MS * info->pdata->clk_rate / (8 * 1000); expmax = EXPOS_MAX_MS * info->pdata->clk_rate / (8 * 1000);
[PATCH] media: dvb_ca_en50221: off by one in dvb_ca_en50221_io_do_ioctl()
The > should be >= so we don't read one element beyond the end of the
ca->slot_info[] array. The array is allocated in dvb_ca_en50221_init().
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c
b/drivers/media/dvb-core/dvb_ca_en50221.c
index 1310526b0d49..4d371cea0d5d 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -1391,7 +1391,7 @@ static int dvb_ca_en50221_io_do_ioctl(struct file *file,
struct dvb_ca_slot *sl;
slot = info->num;
- if ((slot > ca->slot_count) || (slot < 0)) {
+ if ((slot >= ca->slot_count) || (slot < 0)) {
err = -EINVAL;
goto out_unlock;
}
[bug report] [media] dib8000: potential off by one
Hello Patrick Boettcher,
The patch 173a64cb3fcf: "[media] dib8000: enhancement" from Apr 22,
2013, leads to the following static checker warning:
drivers/media/dvb-frontends/dib8000.c:2132 dib8000_get_init_prbs()
error: buffer overflow 'lut_prbs_2k' 14 <= 14
drivers/media/dvb-frontends/dib8000.c
2123 static u16 dib8000_get_init_prbs(struct dib8000_state *state, u16
subchannel)
2124 {
2125int sub_channel_prbs_group = 0;
2126
2127sub_channel_prbs_group = (subchannel / 3) + 1;
2128dprintk("sub_channel_prbs_group = %d , subchannel =%d prbs =
0x%04x\n", sub_channel_prbs_group, subchannel,
lut_prbs_8k[sub_channel_prbs_group]);
2129
2130switch (state->fe[0]->dtv_property_cache.transmission_mode) {
2131case TRANSMISSION_MODE_2K:
--> 2132return lut_prbs_2k[sub_channel_prbs_group];
2133case TRANSMISSION_MODE_4K:
2134return lut_prbs_4k[sub_channel_prbs_group];
2135default:
2136case TRANSMISSION_MODE_8K:
2137return lut_prbs_8k[sub_channel_prbs_group];
2138}
2139 }
[ snip ]
3305 break;
3306
3307 case CT_DEMOD_STEP_11: /* 41 : init prbs autosearch */
3308 if (state->subchannel <= 41) {
^^^
The problem is here. If ->subchannel is 41 then we are off by one.
In the original code this was something like state->subchannel % 41 so
I suspect the fix is to change <= to just < but I'm not totally sure.
3309 dib8000_set_subchannel_prbs(state,
dib8000_get_init_prbs(state, state->subchannel));
3310 *tune_state = CT_DEMOD_STEP_9;
3311 } else {
3312 *tune_state = CT_DEMOD_STOP;
3313 state->status = FE_STATUS_TUNE_FAILED;
3314 }
3315 break;
3316
3317 default:
3318 break;
3319 }
regards,
dan carpenter
[PATCH] media: staging/imx7: Fix an error code in mipi_csis_clk_get()
We accidentally return IS_ERR(), which is 1, instead of the PTR_ERR()
which is the negative error code.
Fixes: 7807063b862b ("media: staging/imx7: add MIPI CSI-2 receiver subdev for
i.MX7")
Signed-off-by: Dan Carpenter
---
drivers/staging/media/imx/imx7-mipi-csis.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c
b/drivers/staging/media/imx/imx7-mipi-csis.c
index f4674de09e83..4ac0be9e4487 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -491,7 +491,7 @@ static int mipi_csis_clk_get(struct csi_state *state)
state->wrap_clk = devm_clk_get(dev, "wrap");
if (IS_ERR(state->wrap_clk))
- return IS_ERR(state->wrap_clk);
+ return PTR_ERR(state->wrap_clk);
/* Set clock rate */
ret = clk_set_rate(state->wrap_clk, state->clk_frequency);
--
2.17.1
[PATCH] media: ivtv: update *pos correctly in ivtv_read_pos()
We had intended to update *pos, but the current code is a no-op.
Fixes: 1a0adaf37c30 ("V4L/DVB (5345): ivtv driver for Conexant cx23416/cx23415
MPEG encoder/decoder")
Signed-off-by: Dan Carpenter
---
drivers/media/pci/ivtv/ivtv-fileops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/pci/ivtv/ivtv-fileops.c
b/drivers/media/pci/ivtv/ivtv-fileops.c
index 6196daae4b3e..043ac0ae9ed0 100644
--- a/drivers/media/pci/ivtv/ivtv-fileops.c
+++ b/drivers/media/pci/ivtv/ivtv-fileops.c
@@ -420,7 +420,7 @@ static ssize_t ivtv_read_pos(struct ivtv_stream *s, char
__user *ubuf, size_t co
IVTV_DEBUG_HI_FILE("read %zd from %s, got %zd\n", count, s->name, rc);
if (rc > 0)
- pos += rc;
+ *pos += rc;
return rc;
}
--
2.17.1
[PATCH] media: cx18: update *pos correctly in cx18_read_pos()
We should be updating *pos. The current code is a no-op.
Fixes: 1c1e45d17b66 ("V4L/DVB (7786): cx18: new driver for the Conexant CX23418
MPEG encoder chip")
Signed-off-by: Dan Carpenter
---
drivers/media/pci/cx18/cx18-fileops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/pci/cx18/cx18-fileops.c
b/drivers/media/pci/cx18/cx18-fileops.c
index a3f44e30f821..88c2f3bea2b6 100644
--- a/drivers/media/pci/cx18/cx18-fileops.c
+++ b/drivers/media/pci/cx18/cx18-fileops.c
@@ -484,7 +484,7 @@ static ssize_t cx18_read_pos(struct cx18_stream *s, char
__user *ubuf,
CX18_DEBUG_HI_FILE("read %zd from %s, got %zd\n", count, s->name, rc);
if (rc > 0)
- pos += rc;
+ *pos += rc;
return rc;
}
--
2.17.1
[linux-next:master 8840/10202] drivers/staging/media/imx/imx7-media-csi.c:1082 imx7_csi_set_fmt() error: uninitialized symbol 'cc'.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
master
head: 550f4769c7c4a84e3966f20887c6e249c5f2afc4
commit: 05f634040c0d05f59f2dcd39722157cb3b57c85b [8840/10202] media:
staging/imx7: add imx7 CSI subdev driver
New smatch warnings:
drivers/staging/media/imx/imx7-media-csi.c:1082 imx7_csi_set_fmt() error:
uninitialized symbol 'cc'.
Old smatch warnings:
drivers/staging/media/imx/imx7-media-csi.c:1076 imx7_csi_set_fmt() error:
uninitialized symbol 'outcc'.
#
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=05f634040c0d05f59f2dcd39722157cb3b57c85b
git remote add linux-next
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
git remote update linux-next
git checkout 05f634040c0d05f59f2dcd39722157cb3b57c85b
vim +/cc +1082 drivers/staging/media/imx/imx7-media-csi.c
05f63404 Rui Miguel Silva 2019-02-06 1029
05f63404 Rui Miguel Silva 2019-02-06 1030 static int imx7_csi_set_fmt(struct
v4l2_subdev *sd,
05f63404 Rui Miguel Silva 2019-02-06 1031 struct
v4l2_subdev_pad_config *cfg,
05f63404 Rui Miguel Silva 2019-02-06 1032 struct
v4l2_subdev_format *sdformat)
05f63404 Rui Miguel Silva 2019-02-06 1033 {
05f63404 Rui Miguel Silva 2019-02-06 1034 struct imx7_csi *csi =
v4l2_get_subdevdata(sd);
05f63404 Rui Miguel Silva 2019-02-06 1035 struct imx_media_video_dev
*vdev = csi->vdev;
05f63404 Rui Miguel Silva 2019-02-06 1036 const struct imx_media_pixfmt
*outcc;
05f63404 Rui Miguel Silva 2019-02-06 1037 struct v4l2_mbus_framefmt
*outfmt;
05f63404 Rui Miguel Silva 2019-02-06 1038 struct v4l2_pix_format vdev_fmt;
05f63404 Rui Miguel Silva 2019-02-06 1039 const struct imx_media_pixfmt
*cc;
05f63404 Rui Miguel Silva 2019-02-06 1040 struct v4l2_mbus_framefmt *fmt;
05f63404 Rui Miguel Silva 2019-02-06 1041 struct v4l2_subdev_format
format;
05f63404 Rui Miguel Silva 2019-02-06 1042 int ret = 0;
05f63404 Rui Miguel Silva 2019-02-06 1043
05f63404 Rui Miguel Silva 2019-02-06 1044 if (sdformat->pad >=
IMX7_CSI_PADS_NUM)
05f63404 Rui Miguel Silva 2019-02-06 1045 return -EINVAL;
05f63404 Rui Miguel Silva 2019-02-06 1046
05f63404 Rui Miguel Silva 2019-02-06 1047 mutex_lock(&csi->lock);
05f63404 Rui Miguel Silva 2019-02-06 1048
05f63404 Rui Miguel Silva 2019-02-06 1049 if (csi->is_streaming) {
05f63404 Rui Miguel Silva 2019-02-06 1050 ret = -EBUSY;
05f63404 Rui Miguel Silva 2019-02-06 1051 goto out_unlock;
05f63404 Rui Miguel Silva 2019-02-06 1052 }
05f63404 Rui Miguel Silva 2019-02-06 1053
05f63404 Rui Miguel Silva 2019-02-06 1054 imx7_csi_try_fmt(csi, cfg,
sdformat, &cc);
05f63404 Rui Miguel Silva 2019-02-06 1055
05f63404 Rui Miguel Silva 2019-02-06 1056 fmt = imx7_csi_get_format(csi,
cfg, sdformat->pad, sdformat->which);
05f63404 Rui Miguel Silva 2019-02-06 1057 if (!fmt) {
05f63404 Rui Miguel Silva 2019-02-06 1058 ret = -EINVAL;
05f63404 Rui Miguel Silva 2019-02-06 1059 goto out_unlock;
05f63404 Rui Miguel Silva 2019-02-06 1060 }
05f63404 Rui Miguel Silva 2019-02-06 1061
05f63404 Rui Miguel Silva 2019-02-06 1062 *fmt = sdformat->format;
05f63404 Rui Miguel Silva 2019-02-06 1063
05f63404 Rui Miguel Silva 2019-02-06 1064 if (sdformat->pad ==
IMX7_CSI_PAD_SINK) {
05f63404 Rui Miguel Silva 2019-02-06 1065 /* propagate format to
source pads */
05f63404 Rui Miguel Silva 2019-02-06 1066 format.pad =
IMX7_CSI_PAD_SRC;
05f63404 Rui Miguel Silva 2019-02-06 1067 format.which =
sdformat->which;
05f63404 Rui Miguel Silva 2019-02-06 1068 format.format =
sdformat->format;
05f63404 Rui Miguel Silva 2019-02-06 1069 imx7_csi_try_fmt(csi,
cfg, &format, &outcc);
05f63404 Rui Miguel Silva 2019-02-06 1070
05f63404 Rui Miguel Silva 2019-02-06 1071 outfmt =
imx7_csi_get_format(csi, cfg, IMX7_CSI_PAD_SRC,
05f63404 Rui Miguel Silva 2019-02-06 1072
sdformat->which);
05f63404 Rui Miguel Silva 2019-02-06 1073 *outfmt = format.format;
05f63404 Rui Miguel Silva 2019-02-06 1074
05f63404 Rui Miguel Silva 2019-02-06 1075 if (sdformat->which ==
V4L2_SUBDEV_FORMAT_ACTIVE)
05f63404 Rui Miguel Silva 2019-02-06 1076
csi->cc[IMX7_CSI_PAD_SRC] = outcc;
05f63404 Rui Miguel Silva 2019-02-06 1077 }
05f63404 Rui Miguel Silva 2019-02-06 1078
05f63404 Rui Miguel Silva 2019-02-06 1079 if (sdformat->which ==
V4L2_SUBDEV_FORMAT_TRY)
05f63404 Rui Miguel Silva 2019-02-06 1080 goto out_unlock;
05f63404 Rui Miguel Silva 2019-02-06 1081
05f63404 Rui Miguel Silva 2019-02-06 @1082 csi->cc[sdformat->pad] = cc;
05f63404 Rui Miguel Silva 2019-02-06 1083
05f63404 Rui Miguel Silva 2019-02-06 1084 /* propaga
[PATCH] media: wl128x: Fix an error code in fm_download_firmware()
We forgot to set "ret" on this error path.
Fixes: e8454ff7b9a4 ("[media] drivers:media:radio: wl128x: FM Driver Common
sources")
Signed-off-by: Dan Carpenter
---
drivers/media/radio/wl128x/fmdrv_common.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/media/radio/wl128x/fmdrv_common.c
b/drivers/media/radio/wl128x/fmdrv_common.c
index 3c8987af3772..1614809f7d35 100644
--- a/drivers/media/radio/wl128x/fmdrv_common.c
+++ b/drivers/media/radio/wl128x/fmdrv_common.c
@@ -1268,8 +1268,9 @@ static int fm_download_firmware(struct fmdev *fmdev,
const u8 *fw_name)
switch (action->type) {
case ACTION_SEND_COMMAND: /* Send */
- if (fmc_send_cmd(fmdev, 0, 0, action->data,
- action->size, NULL, NULL))
+ ret = fmc_send_cmd(fmdev, 0, 0, action->data,
+ action->size, NULL, NULL);
+ if (ret)
goto rel_fw;
cmd_cnt++;
--
2.17.1
Re: [PATCH] staging: davinci: drop pointless static qualifier in vpfe_resizer_init()
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. > struct resource *res; > int ret; regards, dan carpenter
Re: [PATCH v2] staging: davinci: drop pointless static qualifier in vpfe_resizer_init()
Thanks! Reviewed-by: Dan Carpenter regards, dan carpenter
Re: [PATCH] staging: davinci: drop pointless static qualifier in vpfe_resizer_init()
On Mon, Mar 11, 2019 at 02:10:12PM +, Colin Ian King wrote: > 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? > We would sometimes want to add extra spaces to make struct declarations line up in a header file. regards, dan carpenter
[PATCH] [media] wl128x: prevent two potential buffer overflows
Smatch marks skb->data as untrusted so it warns that "evt_hdr->dlen"
can copy up to 255 bytes and we only have room for two bytes. Even
if this comes from the firmware and we trust it, the new policy
generally is just to fix it as kernel hardenning.
I can't test this code so I tried to be very conservative. I considered
not allowing "evt_hdr->dlen == 1" because it doesn't initialize the
whole variable but in the end I decided to allow it and manually
initialized "asic_id" and "asic_ver" to zero.
Fixes: e8454ff7b9a4 ("[media] drivers:media:radio: wl128x: FM Driver Common
sources")
Signed-off-by: Dan Carpenter
---
drivers/media/radio/wl128x/fmdrv_common.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/radio/wl128x/fmdrv_common.c
b/drivers/media/radio/wl128x/fmdrv_common.c
index 1614809f7d35..a1fcb80a2191 100644
--- a/drivers/media/radio/wl128x/fmdrv_common.c
+++ b/drivers/media/radio/wl128x/fmdrv_common.c
@@ -489,7 +489,8 @@ int fmc_send_cmd(struct fmdev *fmdev, u8 fm_op, u16 type,
void *payload,
return -EIO;
}
/* Send response data to caller */
- if (response != NULL && response_len != NULL && evt_hdr->dlen) {
+ if (response != NULL && response_len != NULL && evt_hdr->dlen &&
+ evt_hdr->dlen <= payload_len) {
/* Skip header info and copy only response data */
skb_pull(skb, sizeof(struct fm_event_msg_hdr));
memcpy(response, skb->data, evt_hdr->dlen);
@@ -583,6 +584,8 @@ static void fm_irq_handle_flag_getcmd_resp(struct fmdev
*fmdev)
return;
fm_evt_hdr = (void *)skb->data;
+ if (fm_evt_hdr->dlen > sizeof(fmdev->irq_info.flag))
+ return;
/* Skip header info and copy only response data */
skb_pull(skb, sizeof(struct fm_event_msg_hdr));
@@ -1309,7 +1312,7 @@ static int load_default_rx_configuration(struct fmdev
*fmdev)
static int fm_power_up(struct fmdev *fmdev, u8 mode)
{
u16 payload;
- __be16 asic_id, asic_ver;
+ __be16 asic_id = 0, asic_ver = 0;
int resp_len, ret;
u8 fw_name[50];
--
2.17.1
[PATCH] media: wl128x: Fix some error handling in fmc_prepare()
The st_register() returns have changed over time, but these days it
never returns -1. We should just be checking for any negative error
codes.
Signed-off-by: Dan Carpenter
---
drivers/media/radio/wl128x/fmdrv_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/radio/wl128x/fmdrv_common.c
b/drivers/media/radio/wl128x/fmdrv_common.c
index 1614809f7d35..c3b90f2ac93f 100644
--- a/drivers/media/radio/wl128x/fmdrv_common.c
+++ b/drivers/media/radio/wl128x/fmdrv_common.c
@@ -1521,7 +1521,7 @@ int fmc_prepare(struct fmdev *fmdev)
}
ret = 0;
- } else if (ret == -1) {
+ } else if (ret < 0) {
fmerr("st_register failed %d\n", ret);
return -EAGAIN;
}
--
2.17.1
[bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
Hello Andrzej Hajda,
The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
identify last buffers in decoder capture queue" from Oct 7, 2015,
leads to the following static checker warning:
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:664 vidioc_dqbuf()
warn: uncapped user index 'ctx->dst_bufs[buf->index]'
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
642 static int vidioc_dqbuf(struct file *file, void *priv, struct
v4l2_buffer *buf)
643 {
644 const struct v4l2_event ev = {
645 .type = V4L2_EVENT_EOS
646 };
647 struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
648 int ret;
649
650 if (ctx->state == MFCINST_ERROR) {
651 mfc_err_limited("Call on DQBUF after unrecoverable
error\n");
652 return -EIO;
653 }
654
655 switch (buf->type) {
656 case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
657 return vb2_dqbuf(&ctx->vq_src, buf, file->f_flags &
O_NONBLOCK);
658 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
659 ret = vb2_dqbuf(&ctx->vq_dst, buf, file->f_flags &
O_NONBLOCK);
660 if (ret)
661 return ret;
662
663 if (ctx->state == MFCINST_FINISHED &&
--> 664 (ctx->dst_bufs[buf->index].flags &
MFC_BUF_FLAG_EOS))
^^
Smatch is saying that this isn't capped. The truth is that v4l2 code is
a bit complicated for Smatch, but in this case I can't see where
"buf->index" does get capped. I would have expected it to be capped in
check_array_args() where we check "buf->length" but it's not.
I've been going through these warnings really carefully in the past
couple weeks trying to fix false positives so let me know what I'm
missing and I will update the check. Even if I have to manually muck
in the DB.
665 v4l2_event_queue_fh(&ctx->fh, &ev);
666 return 0;
667 default:
668 return -EINVAL;
669 }
670 }
regards,
dan carpenter
Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
On Fri, Apr 05, 2019 at 12:02:49PM +0200, Andrzej Hajda wrote: > Hi Dan, > > On 04.04.2019 17:42, Dan Carpenter wrote: > > Hello Andrzej Hajda, > > > > The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to > > identify last buffers in decoder capture queue" from Oct 7, 2015, > > leads to the following static checker warning: > > > > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:664 vidioc_dqbuf() > > warn: uncapped user index 'ctx->dst_bufs[buf->index]' > > > Almost identical e-mail you have sent about year ago, and me and Hans > have explained you that it is false positive. > > Has something changed? Wow! It's remarkable how similar the emails are. Thank you for your patience on this. I am testing a patch which should silence this false positive in Smatch. regards, dan carpenter
[PATCH] media: pvrusb2: Prevent a buffer overflow
The ctrl_check_input() function is called from pvr2_ctrl_range_check().
It's supposed to validate user supplied input and return true or false
depending on whether the input is valid or not. The problem is that
negative shifts or shifts greater than 31 are undefined in C. In
practice with GCC they result in shift wrapping so this function returns
true for some inputs which are not valid and this could result in a
buffer overflow:
drivers/media/usb/pvrusb2/pvrusb2-ctrl.c:205 pvr2_ctrl_get_valname()
warn: uncapped user index 'names[val]'
The cptr->hdw->input_allowed_mask mask is configured in pvr2_hdw_create()
and the highest valid bit is BIT(4).
Fixes: 7fb20fa38caa ("V4L/DVB (7299): pvrusb2: Improve logic which handles
input choice availability")
Signed-off-by: Dan Carpenter
---
drivers/media/usb/pvrusb2/pvrusb2-hdw.c | 2 ++
drivers/media/usb/pvrusb2/pvrusb2-hdw.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.h
b/drivers/media/usb/pvrusb2/pvrusb2-hdw.h
index 25648add77e5..bd2b7a67b732 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.h
+++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.h
@@ -50,6 +50,7 @@
#define PVR2_CVAL_INPUT_COMPOSITE 2
#define PVR2_CVAL_INPUT_SVIDEO 3
#define PVR2_CVAL_INPUT_RADIO 4
+#define PVR2_CVAL_INPUT_MAX PVR2_CVAL_INPUT_RADIO
enum pvr2_config {
pvr2_config_empty,/* No configuration */
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
index 51112b7988e4..816c85786c2a 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-hdw.c
@@ -666,6 +666,8 @@ static int ctrl_get_input(struct pvr2_ctrl *cptr,int *vp)
static int ctrl_check_input(struct pvr2_ctrl *cptr,int v)
{
+ if (v < 0 || v > PVR2_CVAL_INPUT_MAX)
+ return 0;
return ((1 << v) & cptr->hdw->input_allowed_mask) != 0;
}
--
2.17.1
[PATCH] media: omap_vout: potential buffer overflow in vidioc_dqbuf()
The "b->index" is a u32 the comes from the user in the ioctl. It hasn't
been checked. We aren't supposed to use it but we're instead supposed
to use the value that gets written to it when we call videobuf_dqbuf().
The videobuf_dqbuf() first memsets it to zero and then re-initializes it
inside the videobuf_status() function. It's this final value which we
want.
Fixes: 72915e851da9 ("[media] V4L2: OMAP: VOUT: dma map and unmap v4l2 buffers
in qbuf and dqbuf")
Signed-off-by: Dan Carpenter
---
UNTESTED! I think I understand this code now, but I have struggled to
read it correctly in the past. Please review carefully.
drivers/media/platform/omap/omap_vout.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/omap/omap_vout.c
b/drivers/media/platform/omap/omap_vout.c
index 37f0d7146dfa..15e38990e85a 100644
--- a/drivers/media/platform/omap/omap_vout.c
+++ b/drivers/media/platform/omap/omap_vout.c
@@ -1527,8 +1527,6 @@ static int vidioc_dqbuf(struct file *file, void *fh,
struct v4l2_buffer *b)
unsigned long size;
struct videobuf_buffer *vb;
- vb = q->bufs[b->index];
-
if (!vout->streaming)
return -EINVAL;
@@ -1539,6 +1537,8 @@ static int vidioc_dqbuf(struct file *file, void *fh,
struct v4l2_buffer *b)
/* Call videobuf_dqbuf for blocking mode */
ret = videobuf_dqbuf(q, (struct v4l2_buffer *)b, 0);
+ vb = q->bufs[b->index];
+
addr = (unsigned long) vout->buf_phy_addr[vb->i];
size = (unsigned long) vb->size;
dma_unmap_single(vout->vid_dev->v4l2_dev.dev, addr,
--
2.17.1
[bug report] [media] mxb: fix audio handling
[ Hi Hans,
This might not really be your bug, but I just respect you a lot and
so I always come to you with questions and for advice. -dan ]
Hello Hans Verkuil,
The patch 6680427791c9: "[media] mxb: fix audio handling" from Apr
30, 2012, leads to the following static checker warning:
drivers/media/pci/saa7146/mxb.c:196 tea6420_route()
warn: uncapped user index 'TEA6420_cd[idx]'
drivers/media/pci/saa7146/mxb.c
194 static inline void tea6420_route(struct mxb *mxb, int idx)
195 {
--> 196 v4l2_subdev_call(mxb->tea6420_1, audio, s_routing,
197 TEA6420_cd[idx][0].input, TEA6420_cd[idx][0].output, 0);
^^^
Index overflow. The TEA6420_cd[] array has MXB_AUDIOS + 1 (which is 7
altogether) elements.
198 v4l2_subdev_call(mxb->tea6420_2, audio, s_routing,
199 TEA6420_cd[idx][1].input, TEA6420_cd[idx][1].output, 0);
200 v4l2_subdev_call(mxb->tea6420_1, audio, s_routing,
201 TEA6420_line[idx][0].input,
TEA6420_line[idx][0].output, 0);
202 v4l2_subdev_call(mxb->tea6420_2, audio, s_routing,
203 TEA6420_line[idx][1].input,
TEA6420_line[idx][1].output, 0);
204 }
[ snip ]
650 static int vidioc_s_audio(struct file *file, void *fh, const struct
v4l2_audio *a)
651 {
652 struct saa7146_dev *dev = ((struct saa7146_fh *)fh)->dev;
653 struct mxb *mxb = (struct mxb *)dev->ext_priv;
654
655 DEB_D("VIDIOC_S_AUDIO %d\n", a->index);
656 if (mxb_inputs[mxb->cur_input].audioset & (1 << a->index)) {
This a->index comes from the ioctl and it's a u32 so the shift can wrap.
The .audioset variable is always 0x3f. In other words BIT(6) is the
highest valid bit so we could add a check:
if (a->index > MXB_AUDIOS)
return;
657 if (mxb->cur_audinput != a->index) {
658 mxb->cur_audinput = a->index;
Now here's the complication. We also use a->index as an index into the
mxb_inputs[] array which only has MXB_INPUTS (4) elements, so just
adding the limit would still lead to a different array out of bounds
later...
659 tea6420_route(mxb, a->index);
660 if (mxb->cur_audinput == 0)
661 mxb_update_audmode(mxb);
662 }
663 return 0;
664 }
665 return -EINVAL;
666 }
regards,
dan carpenter
Re: [PATCH] media: omap_vout: potential buffer overflow in vidioc_dqbuf()
On Wed, Apr 10, 2019 at 12:50:31PM +0200, Hans Verkuil wrote: > On 4/9/19 1:29 PM, Dan Carpenter wrote: > > diff --git a/drivers/media/platform/omap/omap_vout.c > > b/drivers/media/platform/omap/omap_vout.c > > index 37f0d7146dfa..15e38990e85a 100644 > > --- a/drivers/media/platform/omap/omap_vout.c > > +++ b/drivers/media/platform/omap/omap_vout.c > > @@ -1527,8 +1527,6 @@ static int vidioc_dqbuf(struct file *file, void *fh, > > struct v4l2_buffer *b) > > unsigned long size; > > struct videobuf_buffer *vb; > > > > - vb = q->bufs[b->index]; > > - > > if (!vout->streaming) > > return -EINVAL; > > > > @@ -1539,6 +1537,8 @@ static int vidioc_dqbuf(struct file *file, void *fh, > > struct v4l2_buffer *b) > > /* Call videobuf_dqbuf for blocking mode */ > > ret = videobuf_dqbuf(q, (struct v4l2_buffer *)b, 0); > > We need a: > > if (ret) > return ret; > > here. Or alternatively, add 'if (!ret)' around the next five lines. > > b->index is only valid if the videobuf_dqbuf call returned 0. > Doh. Thanks. regards, dan carpenter
Re: [bug report] [media] mxb: fix audio handling
On Wed, Apr 10, 2019 at 01:24:44PM +0200, Hans Verkuil wrote:
> On 4/10/19 1:09 PM, Dan Carpenter wrote:
> > [ Hi Hans,
> >
> > This might not really be your bug, but I just respect you a lot and
> > so I always come to you with questions and for advice. -dan ]
>
> Hmm, in other words, I'm too nice!
>
Indeed.
> > 657 if (mxb->cur_audinput != a->index) {
> > 658 mxb->cur_audinput = a->index;
> >
> > Now here's the complication. We also use a->index as an index into the
> > mxb_inputs[] array which only has MXB_INPUTS (4) elements, so just
>
> We do? Where does that happen? I don't see that in the code. That would be
> a bug since mxb_inputs are the video inputs, whereas s_audio deals with
> audio inputs.
>
Oh, you're right. But then there is a smaller problem because we use
it as in index into the mxb_audios[] array and that array only has 6
elements so we're still out of bounds. The bigger array has a mute
element at the end.
regards,
dan carpenter
[PATCH v2] media: omap_vout: potential buffer overflow in vidioc_dqbuf()
The "b->index" is a u32 the comes from the user in the ioctl. It hasn't
been checked. We aren't supposed to use it but we're instead supposed
to use the value that gets written to it when we call videobuf_dqbuf().
The videobuf_dqbuf() first memsets it to zero and then re-initializes it
inside the videobuf_status() function. It's this final value which we
want.
Hans Verkuil pointed out that we need to check the return from
videobuf_dqbuf(). I ended up doing a little cleanup related to that as
well.
Fixes: 72915e851da9 ("[media] V4L2: OMAP: VOUT: dma map and unmap v4l2 buffers
in qbuf and dqbuf")
Signed-off-by: Dan Carpenter
---
v2: check videobuf_dqbuf() return
drivers/media/platform/omap/omap_vout.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/media/platform/omap/omap_vout.c
b/drivers/media/platform/omap/omap_vout.c
index 37f0d7146dfa..cb6a9e3946b6 100644
--- a/drivers/media/platform/omap/omap_vout.c
+++ b/drivers/media/platform/omap/omap_vout.c
@@ -1527,23 +1527,20 @@ static int vidioc_dqbuf(struct file *file, void *fh,
struct v4l2_buffer *b)
unsigned long size;
struct videobuf_buffer *vb;
- vb = q->bufs[b->index];
-
if (!vout->streaming)
return -EINVAL;
- if (file->f_flags & O_NONBLOCK)
- /* Call videobuf_dqbuf for non blocking mode */
- ret = videobuf_dqbuf(q, (struct v4l2_buffer *)b, 1);
- else
- /* Call videobuf_dqbuf for blocking mode */
- ret = videobuf_dqbuf(q, (struct v4l2_buffer *)b, 0);
+ ret = videobuf_dqbuf(q, b, !!(file->f_flags & O_NONBLOCK));
+ if (ret)
+ return ret;
+
+ vb = q->bufs[b->index];
addr = (unsigned long) vout->buf_phy_addr[vb->i];
size = (unsigned long) vb->size;
dma_unmap_single(vout->vid_dev->v4l2_dev.dev, addr,
size, DMA_TO_DEVICE);
- return ret;
+ return 0;
}
static int vidioc_streamon(struct file *file, void *fh, enum v4l2_buf_type i)
--
2.17.1
[PATCH v3 resend] media: davinci/vpbe: array underflow in vpbe_enum_outputs()
In vpbe_enum_outputs() we check if (temp_index >= cfg->num_outputs) but
the problem is that "temp_index" can be negative. This patch changes
the types to unsigned to address this array underflow bug.
Fixes: 66715cdc3224 ("[media] davinci vpbe: VPBE display driver")
Signed-off-by: Dan Carpenter
Acked-by: "Lad, Prabhakar"
---
I sent this patch last May but somehow the spam filters on the lists
must have eaten it. I didn't get a copy from the kernel-janitors list.
The only trace I have of my original patch is that the maintainer Acked
it. Resending.
v2: In the first version, I clamped output->index to 0-INT_MAX for every
driver. In v2, I only changed the vpbe.h driver header file.
v3: In v3 I changed the header and the .c file (All three versions of
patch "worked", they just had philosophical and style issues).
drivers/media/platform/davinci/vpbe.c | 2 +-
include/media/davinci/vpbe.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/davinci/vpbe.c
b/drivers/media/platform/davinci/vpbe.c
index 8339163a5231..4e24f5d781f4 100644
--- a/drivers/media/platform/davinci/vpbe.c
+++ b/drivers/media/platform/davinci/vpbe.c
@@ -104,7 +104,7 @@ static int vpbe_enum_outputs(struct vpbe_device *vpbe_dev,
struct v4l2_output *output)
{
struct vpbe_config *cfg = vpbe_dev->cfg;
- int temp_index = output->index;
+ unsigned int temp_index = output->index;
if (temp_index >= cfg->num_outputs)
return -EINVAL;
diff --git a/include/media/davinci/vpbe.h b/include/media/davinci/vpbe.h
index 5c31a7682492..f76d2f25a824 100644
--- a/include/media/davinci/vpbe.h
+++ b/include/media/davinci/vpbe.h
@@ -92,7 +92,7 @@ struct vpbe_config {
struct encoder_config_info *ext_encoders;
/* amplifier information goes here */
struct amp_config_info *amp;
- int num_outputs;
+ unsigned int num_outputs;
/* Order is venc outputs followed by LCD and then external encoders */
struct vpbe_output *outputs;
};
--
2.18.0
Re: [PATCH v3 1/2] staging: media: davinci_vpfe: add error handling on kmalloc failure
On Tue, Mar 27, 2018 at 02:00:45PM +0900, Ji-Hun Kim wrote: > > Are there any opinions? I'd like to know how this patch is going. > Looks good. Thanks! Greg just hasn't gotten to it yet. regards, dan carpenter
[PATCH] media: dvb-core: Check for allocation failure in dvb_module_probe()
We should check if kzalloc() fails.
Fixes: 8f569c0b4e6b ("media: dvb-core: add helper functions for I2C binding")
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index cf747d753a79..787fe06df217 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -953,6 +953,8 @@ struct i2c_client *dvb_module_probe(const char *module_name,
struct i2c_board_info *board_info;
board_info = kzalloc(sizeof(*board_info), GFP_KERNEL);
+ if (!board_info)
+ return NULL;
if (name)
strlcpy(board_info->type, name, I2C_NAME_SIZE);
Re: [PATCH 12/18] media: staging: atomisp: avoid a warning if 32 bits build
On Mon, Mar 26, 2018 at 05:10:45PM -0400, Mauro Carvalho Chehab wrote: > Checking if a size_t value is bigger than ULONG_INT only makes > sense if building on 64 bits, as warned by: > > drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c:697 > gmin_get_config_var() warn: impossible condition '(*out_len > (~0)) => > (0-u32max > u32max)' > > Signed-off-by: Mauro Carvalho Chehab > --- > .../staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c| 2 > ++ > 1 file changed, 2 insertions(+) > > diff --git > a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c > b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c > index be0c5e11e86b..3283c1b05d6a 100644 > --- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c > +++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c > @@ -693,9 +693,11 @@ static int gmin_get_config_var(struct device *dev, const > char *var, > for (i = 0; i < sizeof(var8) && var8[i]; i++) > var16[i] = var8[i]; > > +#ifdef CONFIG_64BIT > /* To avoid owerflows when calling the efivar API */ > if (*out_len > ULONG_MAX) > return -EINVAL; > +#endif I should just silence this particular warning in Smatch. I feel like this is a pretty common thing and the ifdefs aren't very pretty. :( regards, dan carpenter
Re: [PATCH 02/15] media: staging/imx7: add imx7 CSI subdev driver
On Thu, Apr 19, 2018 at 11:17:59AM +0100, Rui Miguel Silva wrote:
> +static int imx7_csi_link_setup(struct media_entity *entity,
> +const struct media_pad *local,
> +const struct media_pad *remote, u32 flags)
> +{
> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> + struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> + struct v4l2_subdev *remote_sd;
> + int ret = 0;
> +
> + dev_dbg(csi->dev, "link setup %s -> %s\n", remote->entity->name,
> + local->entity->name);
> +
> + mutex_lock(&csi->lock);
> +
> + if (local->flags & MEDIA_PAD_FL_SINK) {
> + if (!is_media_entity_v4l2_subdev(remote->entity)) {
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + remote_sd = media_entity_to_v4l2_subdev(remote->entity);
> +
> + if (flags & MEDIA_LNK_FL_ENABLED) {
> + if (csi->src_sd) {
> + ret = -EBUSY;
> + goto unlock;
> + }
> + csi->src_sd = remote_sd;
> + } else {
> + csi->src_sd = NULL;
> + }
> +
> + goto init;
> + }
> +
> + /* source pad */
> + if (flags & MEDIA_LNK_FL_ENABLED) {
> + if (csi->sink) {
> + ret = -EBUSY;
> + goto unlock;
> + }
> + csi->sink = remote->entity;
> + } else {
> + v4l2_ctrl_handler_free(&csi->ctrl_hdlr);
> + v4l2_ctrl_handler_init(&csi->ctrl_hdlr, 0);
> + csi->sink = NULL;
> + }
> +
> +init:
> + if (csi->sink || csi->src_sd)
> + imx7_csi_init(csi);
> + else
> + imx7_csi_deinit(csi);
> +
> +unlock:
> + mutex_unlock(&csi->lock);
> +
> + return 0;
This should be "return ret;" because the failure paths go through here
as well.
> +}
> +
> +static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
> + struct media_link *link,
> + struct v4l2_subdev_format *source_fmt,
> + struct v4l2_subdev_format *sink_fmt)
> +{
> + struct imx7_csi *csi = v4l2_get_subdevdata(sd);
> + struct v4l2_fwnode_endpoint upstream_ep;
> + int ret;
> +
> + ret = v4l2_subdev_link_validate_default(sd, link, source_fmt, sink_fmt);
> + if (ret)
> + return ret;
> +
> + ret = imx7_csi_get_upstream_endpoint(csi, &upstream_ep, true);
> + if (ret) {
> + v4l2_err(&csi->sd, "failed to find upstream endpoint\n");
> + return ret;
> + }
> +
> + mutex_lock(&csi->lock);
> +
> + csi->upstream_ep = upstream_ep;
> + csi->is_csi2 = (upstream_ep.bus_type == V4L2_MBUS_CSI2);
> +
> + mutex_unlock(&csi->lock);
> +
> + return ret;
return 0;
> +}
> +
[ snip ]
> +
> +static int imx7_csi_remove(struct platform_device *pdev)
> +{
> + return 0;
> +}
There is no need for this empty (struct platform_driver)->remove()
function. See platform_drv_remove() for how it's called.
This looks nice, though.
regards,
dan carpenter
Re: [PATCH 05/15] media: staging/imx7: add MIPI CSI-2 receiver subdev for i.MX7
> +static int mipi_csis_clk_get(struct csi_state *state)
> +{
> + struct device *dev = &state->pdev->dev;
> + int ret = true;
Better to leave ret unitialized.
> +
> + state->mipi_clk = devm_clk_get(dev, "mipi");
> + if (IS_ERR(state->mipi_clk)) {
> + dev_err(dev, "Could not get mipi csi clock\n");
> + return -ENODEV;
> + }
> +
> + state->phy_clk = devm_clk_get(dev, "phy");
> + if (IS_ERR(state->phy_clk)) {
> + dev_err(dev, "Could not get mipi phy clock\n");
> + return -ENODEV;
> + }
> +
> + /* Set clock rate */
> + if (state->clk_frequency)
> + ret = clk_set_rate(state->mipi_clk, state->clk_frequency);
> + else
> + dev_warn(dev, "No clock frequency specified!\n");
> + if (ret < 0) {
> + dev_err(dev, "set rate=%d failed: %d\n", state->clk_frequency,
> + ret);
> + return -EINVAL;
Preserve the error code.
> + }
> +
> + return ret;
This could be "return 0;" (let's not return true). It might be nicer
like:
if (!state->clk_frequency) {
dev_warn(dev, "No clock frequency specified!\n");
return 0;
}
ret = clk_set_rate(state->mipi_clk, state->clk_frequency);
if (ret < 0)
dev_err(dev, "set rate=%d failed: %d\n", state->clk_frequency,
ret);
return ret;
> +}
> +
[ snip ]
> +static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
> +{
> + struct csi_state *state = dev_id;
> + unsigned long flags;
> + u32 status;
> + int i;
> +
> + status = mipi_csis_read(state, MIPI_CSIS_INTSRC);
> +
> + spin_lock_irqsave(&state->slock, flags);
> +
> + /* Update the event/error counters */
> + if ((status & MIPI_CSIS_INTSRC_ERRORS) || 1) {
^^^
Was this supposed to make it into the published code?
> + for (i = 0; i < MIPI_CSIS_NUM_EVENTS; i++) {
> + if (!(status & state->events[i].mask))
> + continue;
> + state->events[i].counter++;
> + }
> + }
> + spin_unlock_irqrestore(&state->slock, flags);
> +
> + mipi_csis_write(state, MIPI_CSIS_INTSRC, status);
> +
> + return IRQ_HANDLED;
> +}
> +
regards,
dan carpenter
Re: [PATCH 01/15] media: staging/imx: add support to media dev for no IPU systems
On Thu, Apr 19, 2018 at 11:17:58AM +0100, Rui Miguel Silva wrote:
> Some i.MX SoC do not have IPU, like the i.MX7, add to the the media device
> infrastructure support to be used in this type of systems that do not have
> internal subdevices besides the CSI.
>
> Signed-off-by: Rui Miguel Silva
> ---
> drivers/staging/media/imx/imx-media-dev.c| 16 +++-
> .../staging/media/imx/imx-media-internal-sd.c| 3 +++
> drivers/staging/media/imx/imx-media.h| 3 +++
> 3 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-dev.c
> b/drivers/staging/media/imx/imx-media-dev.c
> index f67ec8e27093..a8afe0ec4134 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -92,6 +92,9 @@ static int imx_media_get_ipu(struct imx_media_dev *imxmd,
> struct ipu_soc *ipu;
> int ipu_id;
>
> + if (imxmd->no_ipu_present)
It's sort of nicer if variables don't have a negative built in because
otherwise you get confusing double negatives like "if (!no_ipu) {".
It's not hard to invert the varible in this case, because the only thing
we need to change is imx_media_probe() to set:
+ imxmd->ipu_present = true;
regards,
dan carpenter
[PATCH] media: davinci_vpfe: fix some potential overflows
We check "lutdpc->dpc_size" in ipipe_validate_lutdpc_params() but if
it's invalid then we would have corrupted memory already when we do
the memcpy() before calling it.
We don't ever check "gamma->tbl_size" but we should since they come from
the user.
Signed-off-by: Dan Carpenter
diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index 95942768639c..068be224 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -82,6 +82,8 @@ static int ipipe_set_lutdpc_params(struct vpfe_ipipe_device
*ipipe, void *param)
lutdpc->en = dpc_param->en;
lutdpc->repl_white = dpc_param->repl_white;
lutdpc->dpc_size = dpc_param->dpc_size;
+ if (dpc_param->dpc_size > LUT_DPC_MAX_SIZE)
+ return -EINVAL;
memcpy(&lutdpc->table, &dpc_param->table,
(dpc_param->dpc_size * sizeof(struct vpfe_ipipe_lutdpc_entry)));
if (ipipe_validate_lutdpc_params(lutdpc) < 0)
@@ -591,7 +593,7 @@ ipipe_validate_gamma_entry(struct vpfe_ipipe_gamma_entry
*table, int size)
static int
ipipe_validate_gamma_params(struct vpfe_ipipe_gamma *gamma, struct device *dev)
{
- int table_size;
+ unsigned int table_size;
int err;
if (gamma->bypass_r > 1 ||
@@ -603,6 +605,8 @@ ipipe_validate_gamma_params(struct vpfe_ipipe_gamma *gamma,
struct device *dev)
return 0;
table_size = gamma->tbl_size;
+ if (table_size > VPFE_IPIPE_MAX_SIZE_GAMMA)
+ return -EINVAL;
if (!gamma->bypass_r) {
err = ipipe_validate_gamma_entry(gamma->table_r, table_size);
if (err) {
[PATCH] media: vpbe_venc: potential uninitialized variable in ven_sub_dev_init()
Smatch complains that "venc" could be unintialized. There a couple
error paths where it looks like maybe that could happen. I don't know
if it's really a bug, but it's reasonable to set "venc" to NULL and
silence the warning.
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/platform/davinci/vpbe_venc.c
b/drivers/media/platform/davinci/vpbe_venc.c
index 5c255de3b3f8..ba157827192c 100644
--- a/drivers/media/platform/davinci/vpbe_venc.c
+++ b/drivers/media/platform/davinci/vpbe_venc.c
@@ -606,7 +606,7 @@ static int venc_device_get(struct device *dev, void *data)
struct v4l2_subdev *venc_sub_dev_init(struct v4l2_device *v4l2_dev,
const char *venc_name)
{
- struct venc_state *venc;
+ struct venc_state *venc = NULL;
bus_for_each_dev(&platform_bus_type, NULL, &venc,
venc_device_get);
Re: [PATCH 01/11] media: tm6000: fix potential Spectre variant 1
Hi Mauro,
I saw your comment on LWN. You argue on LWN that since the format array
is static the CPU won't speculatively read past the L1 cache?
I don't know if that's true. It should be easy enough to filter out
the reads into static arrays. Peter do you know the answer here?
regards,
dan carpenter
On Mon, Apr 23, 2018 at 03:24:55PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 23 Apr 2018 12:38:03 -0500
> "Gustavo A. R. Silva" escreveu:
>
> > f->index can be controlled by user-space, hence leading to a
> > potential exploitation of the Spectre variant 1 vulnerability.
> >
> > Smatch warning:
> > drivers/media/usb/tm6000/tm6000-video.c:879 vidioc_enum_fmt_vid_cap() warn:
> > potential spectre issue 'format'
> >
> > Fix this by sanitizing f->index before using it to index
> > array _format_
> >
> > Notice that given that speculation windows are large, the policy is
> > to kill the speculation on the first load and not worry if it can be
> > completed with a dependent load/store [1].
> >
> > [1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
> >
> > Cc: sta...@vger.kernel.org
> > Reported-by: Dan Carpenter
> > Signed-off-by: Gustavo A. R. Silva
> > ---
> > drivers/media/usb/tm6000/tm6000-video.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/media/usb/tm6000/tm6000-video.c
> > b/drivers/media/usb/tm6000/tm6000-video.c
> > index b2399d4..d701027 100644
> > --- a/drivers/media/usb/tm6000/tm6000-video.c
> > +++ b/drivers/media/usb/tm6000/tm6000-video.c
> > @@ -26,6 +26,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "tm6000-regs.h"
> > #include "tm6000.h"
> > @@ -875,6 +876,7 @@ static int vidioc_enum_fmt_vid_cap(struct file *file,
> > void *priv,
> > if (f->index >= ARRAY_SIZE(format))
> > return -EINVAL;
> >
> > + f->index = array_index_nospec(f->index, ARRAY_SIZE(format));
>
> Please enlighten me: how do you think this could be exploited?
>
> When an application calls VIDIOC_ENUM_FMT from a /dev/video0 device,
> it will just enumerate a hardware functionality, with is constant
> for a given hardware piece.
>
> The way it works is that userspace do something like:
>
> int ret = 0;
>
> for (i = 0; ret == 0; i++) {
> ret = ioctl(VIDIOC_ENUM_FMT, ...);
> }
>
> in order to read an entire const table.
>
> Usually, it doesn't require any special privilege to call this ioctl,
> but, even if someone changes its permission to 0x400, a simple lsusb
> output is enough to know what hardware model is there. A lsmod
> or cat /proc/modules) also tells that the tm6000 module was loaded,
> with is a very good hint that the tm6000 is there or was there in the
> past.
>
> In the specific case of tm6000, all hardware supports exactly the
> same formats, as this is usually defined per-driver. So, a quick look
> at the driver is enough to know exactly what the ioctl would answer.
> Also, the net is full of other resources that would allow anyone
> to get the supported formats for a piece of hardware.
>
> Even assuming that the OS doesn't have lsusb, that /proc is not
> mounted, that /dev/video0 require special permissions, that the
> potential attacker doesn't have physical access to the equipment (in
> order to see if an USB board is plugged), etc... What possible harm
> he could do by identifying a hardware feature?
>
> Similar notes for the other patches to drivers/media in this
> series: let's not just start adding bloatware where not needed.
>
> Please notice that I'm fine if you want to submit potential
> Spectre variant 1 fixups, but if you're willing to do so,
> please provide an explanation about the potential threat scenarios
> that you're identifying at the code.
>
> Dan,
>
> It probably makes sense to have somewhere at smatch a place where
> we could explicitly mark the false-positives, in order to avoid
> use to receive patches that would just add an extra delay where
> it is not needed.
>
> Regards,
> Mauro
Re: [PATCH][media-next] media: davinci_vpfe: fix memory leaks of params
On Wed, May 02, 2018 at 10:16:58AM +0100, Colin King wrote:
> From: Colin Ian King
>
> There are memory leaks of params; when copy_to_user fails and also
> the exit via the label 'error'. Fix this by kfree'ing params in
> error exit path and jumping to this on the copy_to_user failure path.
>
> Detected by CoverityScan, CID#1467966 ("Resource leak")
>
> Fixes: da43b6ccadcf ("[media] davinci: vpfe: dm365: add IPIPE support for
> media controller driver")
> Signed-off-by: Colin Ian King
> ---
> drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> index 95942768639c..3e67ee6e92f9 100644
> --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> @@ -1252,12 +1252,12 @@ static const struct ipipe_module_if
> ipipe_modules[VPFE_IPIPE_MAX_MODULES] = {
> static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config
> *cfg)
> {
> struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd);
> + struct ipipe_module_params *params;
> unsigned int i;
> int rval = 0;
>
> for (i = 0; i < ARRAY_SIZE(ipipe_modules); i++) {
> const struct ipipe_module_if *module_if;
> - struct ipipe_module_params *params;
> void *from, *to;
> size_t size;
>
> @@ -1275,7 +1275,7 @@ static int ipipe_s_config(struct v4l2_subdev *sd,
> struct vpfe_ipipe_config *cfg)
> if (to && from && size) {
^^
This "to" check is wrong. Say "params" is NULL and
module_if->param_offset is non-zero then "to" is a bogus pointer. We
should just test "params" and give up the first time an allocation
fails.
> if (copy_from_user(to, (void __user *)from, size)) {
> rval = -EFAULT;
> - break;
> + goto error;
> }
> rval = module_if->set(ipipe, to);
> if (rval)
> @@ -1287,7 +1287,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd,
> struct vpfe_ipipe_config *cfg)
> }
> kfree(params);
> }
> + return rval;
Doing a "return 0;" is more readable than "return rval;".
regards,
dan carpenter
[bug report] [media] exynos4-is: Add the FIMC-IS ISP capture DMA driver
[ This code is five years old now. It's so weird to me that the warning
is showing up in my new warnings pile. Perhaps this wasn't included
in my allmodconfig before? - dan ]
Hello Sylwester Nawrocki,
The patch 34947b8aebe3: "[media] exynos4-is: Add the FIMC-IS ISP
capture DMA driver" from Dec 20, 2013, leads to the following static
checker warning:
drivers/media/platform/exynos4-is/fimc-isp-video.c:408
isp_video_try_fmt_mplane()
error: NULL dereference inside function '__isp_video_try_fmt(isp,
&f->fmt.pix_mp, (0))()'.
drivers/media/platform/exynos4-is/fimc-isp-video.c
383 static void __isp_video_try_fmt(struct fimc_isp *isp,
384 struct v4l2_pix_format_mplane *pixm,
385 const struct fimc_fmt **fmt)
386 {
387 *fmt = fimc_isp_find_format(&pixm->pixelformat, NULL, 2);
Unchecked dereference. We're not allowed to pass a NULL "fmt".
388
389 pixm->colorspace = V4L2_COLORSPACE_SRGB;
390 pixm->field = V4L2_FIELD_NONE;
391 pixm->num_planes = (*fmt)->memplanes;
392 pixm->pixelformat = (*fmt)->fourcc;
393 /*
394 * TODO: double check with the docmentation these width/height
395 * constraints are correct.
396 */
397 v4l_bound_align_image(&pixm->width, FIMC_ISP_SOURCE_WIDTH_MIN,
398FIMC_ISP_SOURCE_WIDTH_MAX, 3,
399&pixm->height, FIMC_ISP_SOURCE_HEIGHT_MIN,
400FIMC_ISP_SOURCE_HEIGHT_MAX, 0, 0);
401 }
402
403 static int isp_video_try_fmt_mplane(struct file *file, void *fh,
404 struct v4l2_format *f)
405 {
406 struct fimc_isp *isp = video_drvdata(file);
407
408 __isp_video_try_fmt(isp, &f->fmt.pix_mp, NULL);
And yet here we are.
409 return 0;
410 }
regards,
dan carpenter
[bug report] media: rcar-vin: add group allocator functions
Hello Niklas Söderlund,
The patch 3bb4c3bc85bf: "media: rcar-vin: add group allocator
functions" from Apr 14, 2018, leads to the following static checker
warning:
drivers/media/platform/rcar-vin/rcar-core.c:346 rvin_group_put()
error: potential NULL dereference 'vin->group'.
drivers/media/platform/rcar-vin/rcar-core.c
339 static void rvin_group_put(struct rvin_dev *vin)
340 {
341 mutex_lock(&vin->group->lock);
342
343 vin->group = NULL;
^
Set to NULL.
344 vin->v4l2_dev.mdev = NULL;
345
346 if (WARN_ON(vin->group->vin[vin->id] != vin))
347 goto out;
348
349 vin->group->vin[vin->id] = NULL;
350 out:
351 mutex_unlock(&vin->group->lock);
352
353 kref_put(&vin->group->refcount, rvin_group_release);
^^^^^^^^
There are a bunch of NULL dereferences here...
354 }
regards,
dan carpenter
[bug report] media: videobuf: fix epoll() by calling poll_wait first
Hello Hans Verkuil,
The patch bb436cbeb918: "media: videobuf: fix epoll() by calling
poll_wait first" from Feb 7, 2019, leads to the following static
checker warning:
drivers/media/v4l2-core/videobuf-core.c:1126 videobuf_poll_stream()
warn: passing bogus address: '&buf->done'
drivers/media/v4l2-core/videobuf-core.c
1118 __poll_t videobuf_poll_stream(struct file *file,
1119struct videobuf_queue *q,
1120poll_table *wait)
1121 {
1122 __poll_t req_events = poll_requested_events(wait);
1123 struct videobuf_buffer *buf = NULL;
^^
1124 __poll_t rc = 0;
1125
1126 poll_wait(file, &buf->done, wait);
^^
This will totally crash, because &buf->done is (void *)72 so it's
non-NULL. It's weird that this code was merged in Feb and no one has
complained about it...
1127 videobuf_queue_lock(q);
1128 if (q->streaming) {
1129 if (!list_empty(&q->stream))
1130 buf = list_entry(q->stream.next,
1131 struct videobuf_buffer,
stream);
1132 } else if (req_events & (EPOLLIN | EPOLLRDNORM)) {
1133 if (!q->reading)
1134 __videobuf_read_start(q);
1135 if (!q->reading) {
1136 rc = EPOLLERR;
1137 } else if (NULL == q->read_buf) {
1138 q->read_buf = list_entry(q->stream.next,
regards,
dan carpenter
Re: [Ksummit-discuss] [PATCH] media: add a subsystem profile documentation
On Wed, Sep 18, 2019 at 10:57:28AM -0300, Mauro Carvalho Chehab wrote: > > > +Patches for the media subsystem should be sent to the media mailing list > > > +at linux-media@vger.kernel.org as plain text only e-mail. Emails with > > > +HTML will be automatically rejected by the mail server. There's no need > > > +to copy the maintainer or sub-maintainer(s). > > > > There's too much traffic on mailing lists for me to follow everything, I > > much prefer being CC'ed on patches. > > Well, by using patchwork, the best is to take a look on it at least for > the patches that you're interested. You could script something using > pwclient in order to make it easier. > > Anyway, not sure if the other sub-maintainers see the same way. From my side, > I prefer not to be c/c, as this is just more noise, as I just rely on > patchwork for media patches. What about changing this to: > > Patches for the media subsystem should be sent to the media mailing list > at linux-media@vger.kernel.org as plain text only e-mail. Emails with > HTML will be automatically rejected by the mail server. It could be > wise > to also copy the sub-maintainer(s). The documentation should say "Use get_maintainer.pl" and do what it says. Everything else is too complicated. Occasionally staging maintainers will complain that they aren't CC'd even though the staging/driver/README says to CC them and I am tell them "No, the responsibility is for you to add yourself to MAINTAINERS. It doesn't matter what the documentation says, you messed up so you need to stop getting annoyed with newbies for not reading the documentation when it's your fault you weren't CC'd." When I sent a patch, I use get_maintainer.pl then I add whoever the wrote the commit from the Fixes tag. Then I remove Colin King and Kees Cook from the CC list because they worked all over the tree and I know them. I also normally remove LKML if there is another mailing list but at least one subsystem uses LKML for patchwork so this isn't safe. So the safest instructions are "Use get_matainer.pl and add the person who wrote the commit in the Fixes tag". regards, dan carpenter
Re: [Ksummit-discuss] [PATCH] media: add a subsystem profile documentation
On Wed, Sep 18, 2019 at 03:48:31PM -0300, Mauro Carvalho Chehab wrote: > Em Wed, 18 Sep 2019 20:27:05 +0300 > Laurent Pinchart escreveu: > > > > Anyway, not sure if the other sub-maintainers see the same way. From my > > > side, > > > I prefer not to be c/c, as this is just more noise, as I just rely on > > > patchwork for media patches. What about changing this to: > > > > > > Patches for the media subsystem should be sent to the media mailing list > > > at linux-media@vger.kernel.org as plain text only e-mail. Emails with > > > HTML will be automatically rejected by the mail server. It could be > > > wise > > > to also copy the sub-maintainer(s). > > > > That works for me. As this is really a personal preference, is there a > > way it could be encoded in MAINTAINERS in a per-person fashion ? > > Something that would allow you to opt-out from CC from linux-media (but > > possibly opt-in for other parts of the kernel), and allow me to opt-in > > for the drivers I maintain ? > > I don't think so. Perhaps we could add, instead, something like that at the > sub-maintainers section of the profile. Of course there is a way to add yourself as a maintainer for a specific .c file... Maybe people feel like MAINTAINERS is too crowded? We could update get_maintainer.pl to grep the .c files for a specific tag instead of putting everything in a centralized MAINTAINERS file. But it doesn't make sense to try store that information MY BRAIN! I can't remember anything from one minute to the next so I have no idea who maintains media submodules... regards, dan carpenter
[bug report] [media] rc: Introduce hix5hd2 IR transmitter driver
[ Ancient code, but maybe someone knows the answer. - dan ]
Hello Guoxiong Yan,
The patch a84fcdaa9058: "[media] rc: Introduce hix5hd2 IR transmitter
driver" from Aug 30, 2014, leads to the following static checker
warning:
./drivers/media/rc/ir-hix5hd2.c:112 (null)()
warn: odd binop '0x3e80 & 0x'
drivers/media/rc/ir-hix5hd2.c
95 static int hix5hd2_ir_config(struct hix5hd2_ir_priv *priv)
96 {
97 int timeout = 1;
98 u32 val, rate;
99
100 writel_relaxed(0x01, priv->base + IR_ENABLE);
101 while (readl_relaxed(priv->base + IR_BUSY)) {
102 if (timeout--) {
103 udelay(1);
104 } else {
105 dev_err(priv->dev, "IR_BUSY timeout\n");
106 return -ETIMEDOUT;
107 }
108 }
109
110 /* Now only support raw mode, with symbol start from low to
high */
111 rate = DIV_ROUND_CLOSEST(priv->rate, 100);
112 val = IR_CFG_SYMBOL_MAXWIDTH & IR_CFG_WIDTH_MASK <<
IR_CFG_WIDTH_SHIFT;
This is zero because << has higher precedence than &. But maybe what
was intended was:
val = (IR_CFG_SYMBOL_MAXWIDTH & IR_CFG_WIDTH_MASK) <<
IR_CFG_WIDTH_SHIFT;
113 val |= IR_CFG_SYMBOL_FMT & IR_CFG_FORMAT_MASK <<
IR_CFG_FORMAT_SHIFT;
^
This is zero anyway, so it doesn't matter but maybe there is a precedence
bug here as well.
114 val |= (IR_CFG_INT_THRESHOLD - 1) & IR_CFG_INT_LEVEL_MASK
115 << IR_CFG_INT_LEVEL_SHIFT;
^
Same
116 val |= IR_CFG_MODE_RAW;
117 val |= (rate - 1) & IR_CFG_FREQ_MASK << IR_CFG_FREQ_SHIFT;
^^
Same
118 writel_relaxed(val, priv->base + IR_CONFIG);
119
120 writel_relaxed(0x00, priv->base + IR_INTM);
121 /* write arbitrary value to start */
122 writel_relaxed(0x01, priv->base + IR_START);
123 return 0;
124 }
regards,
dan carpenter
[patch] [media] vpx3220: signedness bug in vpx3220_fp_read()
The intent was to return -1 on error and that's what the callers expect
but the current code returns USHRT_MAX instead.
Signed-off-by: Dan Carpenter
diff --git a/drivers/media/i2c/vpx3220.c b/drivers/media/i2c/vpx3220.c
index 4b564f1..90b693f 100644
--- a/drivers/media/i2c/vpx3220.c
+++ b/drivers/media/i2c/vpx3220.c
@@ -124,7 +124,7 @@ static int vpx3220_fp_write(struct v4l2_subdev *sd, u8
fpaddr, u16 data)
return 0;
}
-static u16 vpx3220_fp_read(struct v4l2_subdev *sd, u16 fpaddr)
+static int vpx3220_fp_read(struct v4l2_subdev *sd, u16 fpaddr)
{
struct i2c_client *client = v4l2_get_subdevdata(sd);
s16 data;
--
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
[bug report] bt8xx/bttv-cards: module_param issue
I was looking at typos in MODULE_PARM_DESC() and I noticed a problem in drivers/media/pci/bt8xx/bttv-cards.c MODULE_PARM_DESC(saa6588, "if 1, then load the saa6588 RDS module, default (0) is to use the card definition."); The saa6588[] array is not set up as a module parameter so you can't actually use this setting. I don't know if it's better to delete it or enable it. regards, dan carpenter -- 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
