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), _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] 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
[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 = >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;
[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);
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
[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(>ctrls, 5);
1156
1157 mt9v111->auto_awb = v4l2_ctrl_new_std(>ctrls,
1158_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(>ctrls,
1168 _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(>ctrls,
_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(>ctrls,
_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(>ctrls, _ctrl_ops,
regards,
dan carpenter
[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;
}
[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);
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 <laurent.pinch...@ideasonboard.com> 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 = >bru->entity;
> > 30. else
> > 31. brx = >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(>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
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 = >bru->entity;
> else if (pipe->brx)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = >bru->entity;
> else {
> (void)vsp1->brs->entity;
> brx = >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 = >bru->entity;".
Smatch assumes >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 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 = >bru->entity; > else if (pipe->brx && !drm_pipe->force_brx_release) > brx = pipe->brx; > else if (!vsp1->bru->entity.pipe) > brx = >bru->entity; > else > brx = >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 >brs->entity, vsp1->brs, >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 == >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 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 = >bru->entity;
> else if (pipe->brx && !drm_pipe->force_brx_release)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = >bru->entity;
> else
> brx = >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 >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 = >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 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 = >bru->entity;
> else if (pipe->brx && !drm_pipe->force_brx_release)
> brx = pipe->brx;
> else if (!vsp1->bru->entity.pipe)
> brx = >bru->entity;
> else
> brx = >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 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 = >bru->entity;
> > > else if (pipe->brx)
> > > brx = pipe->brx;
> > > else if (!vsp1->bru->entity.pipe)
> > > brx = >bru->entity;
> > > else {
> > > (void)vsp1->brs->entity;
> > > brx = >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 = >bru->entity;".
> > Smatch assumes >bru->entity can be == to pipe->brx and NULL.
>
> Why does smatch assume that >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 = >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 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 = >brs->entity;
>
> + if (pipe->brx == brx)
> + pipe->brx = >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: [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
[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 <dan.carpen...@oracle.com>
---
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;
};
[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 <dan.carpen...@oracle.com>
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] 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 <dan.carpen...@oracle.com>
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);
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, )) > 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
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 <samitolva...@google.com> > > > 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 <samitolva...@google.com> > > > Signed-off-by: Hans Verkuil <hansv...@cisco.com> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org> > > > > > 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 08:59:53AM -0300, Mauro Carvalho Chehab wrote: > Em Mon, 14 May 2018 22:31:37 -0500 > "Gustavo A. R. Silva" <gust...@embeddedor.com> 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 <samitolva...@google.com> > 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 <samitolva...@google.com> > Signed-off-by: Hans Verkuil <hansv...@cisco.com> > Signed-off-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org> > 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] 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
[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(>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(>group->lock);
352
353 kref_put(>group->refcount, rvin_group_release);
There are a bunch of NULL dereferences here...
354 }
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,
>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(>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(>width, FIMC_ISP_SOURCE_WIDTH_MIN,
398FIMC_ISP_SOURCE_WIDTH_MAX, 3,
399>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, >fmt.pix_mp, NULL);
And yet here we are.
409 return 0;
410 }
regards,
dan carpenter
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 <colin.k...@canonical.com>
>
> 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 <colin.k...@canonical.com>
> ---
> 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
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" <gust...@embeddedor.com> 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=152449131114778=2
> >
> > Cc: sta...@vger.kernel.org
> > Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
> > Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
> > ---
> > 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
[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 <dan.carpen...@oracle.com>
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(_bus_type, NULL, ,
venc_device_get);
[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 <dan.carpen...@oracle.com>
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(>table, _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) {
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 <rui.si...@linaro.org>
> ---
> 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
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 = >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(>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(>slock, flags);
> +
> + mipi_csis_write(state, MIPI_CSIS_INTSRC, status);
> +
> + return IRQ_HANDLED;
> +}
> +
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(>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(>ctrl_hdlr);
> + v4l2_ctrl_handler_init(>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(>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, _ep, true);
> + if (ret) {
> + v4l2_err(>sd, "failed to find upstream endpoint\n");
> + return ret;
> + }
> +
> + mutex_lock(>lock);
> +
> + csi->upstream_ep = upstream_ep;
> + csi->is_csi2 = (upstream_ep.bus_type == V4L2_MBUS_CSI2);
> +
> + mutex_unlock(>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 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 <mche...@s-opensource.com> > --- > .../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
[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 <dan.carpen...@oracle.com>
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 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
Re: [PATCH 02/30] media: imx-media-utils: fix a warning
On Fri, Mar 23, 2018 at 07:56:48AM -0400, Mauro Carvalho Chehab wrote: > The logic at find_format() is a little bit confusing even for > humans, and it tricks static code analyzers: > > drivers/staging/media/imx/imx-media-utils.c:259 find_format() error: > buffer overflow 'array' 14 <= 20 It's always good to simplify the code, but I have a fix for this that I will publish very soon. regards, dan carpenter
Re: Double-free in /drivers/media/usb/zr364xx driver
Hi Linux Media Devs, There is a double free on error in zr364xx_probe(). The bug report explains it pretty well. v4l2_device_unregister() calls zr364xx_release() which frees "cam" but we also to another kfree(cam); before the "return err;". Please give reported by credit to: Reported-by: "Yavuz, Tuba" <t...@ece.ufl.edu> regards, dan carpenter On Tue, Mar 20, 2018 at 02:30:45PM +, Yavuz, Tuba wrote: > Hello, > > > It looks like there is a double-free on an error path in the zr364xx_probe > function of the zr364xx driver. > > fail: > v4l2_ctrl_handler_free(hdl); > v4l2_device_unregister(>v4l2_dev); > => > v4l2_device_disconnect >=> > put_device > => > kobject_put > => > kref_put > => > v4l2_device_release > => > zr364xx_release (CALLBACK) > => > kfree(cam) > kfree(cam); > > The vulnerability exists since the initial > commit<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/media/usb/zr364xx?id=0aa77f6c2954896b132f8b6f2e9f063c52800913> > 0aa77f6c2954896b132f8b6f2e9f063c52800913 of the driver. > > > Best, > > Tuba Yavuz, Ph.D. > Assistant Professor > Electrical and Computer Engineering Department > University of Florida > Gainesville, FL 32611 > Webpage: http://www.tuba.ece.ufl.edu/ > Email: t...@ece.ufl.edu > Phone: (352) 846 0202
Re: [PATCH v3 2/2] staging: media: davinci_vpfe: add kfree() on goto err statement
Looks good. Thanks! regards, dan carpenter
Re: Double-free in /drivers/media/usb/hackrf driver
Hi Linux Media Devs, This is similar to the one before. Please give reported-by credit to Reported-by: "Yavuz, Tuba" <t...@ece.ufl.edu> Or maybe flip the names around? The hackrf_probe() function has a double free on error after we set up: dev->v4l2_dev.release = hackrf_video_release; then the calls to: video_unregister_device(>rx_vdev); and kfree(dev); are a double free. regards, dan carpenter On Tue, Mar 20, 2018 at 01:48:17PM +, Yavuz, Tuba wrote: > Hello, > > > It looks like there is a double-free vulnerability on an error path in the > hackrf_probe function of the hackrf driver. > > err_video_unregister_device_rx: >video_unregister_device(>rx_vdev); >=> > v4l2_device_disconnect > => > put_device > => > kobject_put > => > kref_put > => > v4l2_device_release > => > hackrf_video_release (CALLBACK) > => > kfree(dev) > ... > err_kfree: > kfree(dev); > > The vulnerability has been introduced with commit > 8bc4a9ed85046c214458c9e82aea75d2f46cfffd, which added support for > transmitter<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/media/usb/hackrf?id=8bc4a9ed85046c214458c9e82aea75d2f46cfffd>. > > > Best, > > Tuba Yavuz, Ph.D. > Assistant Professor > Electrical and Computer Engineering Department > University of Florida > Gainesville, FL 32611 > Webpage: http://www.tuba.ece.ufl.edu/ > Email: t...@ece.ufl.edu > Phone: (352) 846 0202
Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
On Mon, Mar 19, 2018 at 01:24:57PM +0900, Ji-Hun Kim wrote:
> > 1294 } else if (to && !from && size) {
> > 1295 rval = module_if->set(ipipe, NULL);
> > 1296 if (rval)
> > 1297 goto error;
> >
> > And here again goto free_params.
> >
> > 1298 }
> > 1299 kfree(params);
> > 1300 }
> > 1301 }
> > 1302 error:
> > 1303 return rval;
> >
> >
> > Change this to:
> >
> > return 0;
> Instead of returning rval, returning 0 would be fine? It looks that should
> return rval in normal case.
>
In the proposed code, the errors all do a return or a goto so "rval"
would be zero here. Then the error path would look like:
err_free_params:
kfree(params);
return rval;
}
regards,
dan carpenter
Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure
On Fri, Mar 16, 2018 at 01:58:23PM +0900, Ji-Hun Kim wrote:
> There is no failure checking on the param value which will be allocated
> memory by kmalloc. Add a null pointer checking statement. Then goto error:
> and return -ENOMEM error code when kmalloc is failed.
>
> Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
> ---
> drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> index 6a3434c..55a922c 100644
> --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> @@ -1280,6 +1280,10 @@ static int ipipe_s_config(struct v4l2_subdev *sd,
> struct vpfe_ipipe_config *cfg)
>
> params = kmalloc(sizeof(struct ipipe_module_params),
>GFP_KERNEL);
> + if (!params) {
> + rval = -ENOMEM;
> + goto error;
^^
What does "goto error" do, do you think? It's not clear from the name.
When you have an unclear goto like this it often means the error
handling is going to be buggy.
In this case, it does nothing so a direct "return -ENOMEM;" would be
more clear. But the rest of the error handling is buggy.
1263 static int ipipe_s_config(struct v4l2_subdev *sd, struct
vpfe_ipipe_config *cfg)
1264 {
1265 struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd);
1266 unsigned int i;
1267 int rval = 0;
1268
1269 for (i = 0; i < ARRAY_SIZE(ipipe_modules); i++) {
1270 unsigned int bit = 1 << i;
1271
1272 if (cfg->flag & bit) {
1273 const struct ipipe_module_if *module_if =
1274 _modules[i];
1275 struct ipipe_module_params *params;
1276 void __user *from = *(void * __user *)
1277 ((void *)cfg +
module_if->config_offset);
1278 size_t size;
1279 void *to;
1280
1281 params = kmalloc(sizeof(struct
ipipe_module_params),
1282 GFP_KERNEL);
Do a direct return:
if (!params)
return -ENOMEM;
1283 to = (void *)params + module_if->param_offset;
1284 size = module_if->param_size;
1285
1286 if (to && from && size) {
1287 if (copy_from_user(to, from, size)) {
1288 rval = -EFAULT;
1289 break;
The most recent thing we allocated is "params" so lets do a
"goto free_params;". We'll have to declare "params" at the start of the
function instead inside this block.
1290 }
1291 rval = module_if->set(ipipe, to);
1292 if (rval)
1293 goto error;
goto free_params again since params is still the most recent thing we
allocated.
1294 } else if (to && !from && size) {
1295 rval = module_if->set(ipipe, NULL);
1296 if (rval)
1297 goto error;
And here again goto free_params.
1298 }
1299 kfree(params);
1300 }
1301 }
1302 error:
1303 return rval;
Change this to:
return 0;
free_params:
kfree(params);
return rval;
1304 }
regards,
dan carpenter
Re: [PATCH v4 2/2] media: staging/imx: fill vb2_v4l2_buffer sequence entry
MUCH BETTER!111!Exclamationmark! Thanks! :) regards, dan carpenter
Re: [PATCH] media: staging/imx: fill vb2_v4l2_buffer sequence entry
We need a changelog. How does this affect user space? What bug does
this fix?
On Tue, Mar 13, 2018 at 09:00:54PM +0100, Peter Seiderer wrote:
> Signed-off-by: Peter Seiderer <ps.rep...@gmx.net>
> ---
> drivers/staging/media/imx/imx-media-csi.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 5a195f80a24d..3a6a645b9dce 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -111,6 +111,7 @@ struct csi_priv {
> struct v4l2_ctrl_handler ctrl_hdlr;
>
> int stream_count; /* streaming counter */
> + __u32 frame_sequence; /* frame sequence counter */
Use u32 because this is not a user space header.
> bool last_eof; /* waiting for last EOF at stream off */
> bool nfb4eof;/* NFB4EOF encountered during streaming */
> struct completion last_eof_comp;
regards,
dan carpenter
[PATCH] media: em28xx-cards: fix em28xx_duplicate_dev()
There is a double sizeof() typo here so we don't duplicate the struct
properly.
Fixes: be7fd3c3a8c5 ("media: em28xx: Hauppauge DualHD second tuner
functionality")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/media/usb/em28xx/em28xx-cards.c
b/drivers/media/usb/em28xx/em28xx-cards.c
index 6e8247849c4f..6e0e67d23876 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3515,7 +3515,7 @@ static int em28xx_duplicate_dev(struct em28xx *dev)
dev->dev_next = NULL;
return -ENOMEM;
}
- memcpy(sec_dev, dev, sizeof(sizeof(*sec_dev)));
+ memcpy(sec_dev, dev, sizeof(*sec_dev));
/* Check to see next free device and mark as used */
do {
nr = find_first_zero_bit(em28xx_devused, EM28XX_MAXBOARDS);
Re: [bug report] media: i2c: Copy tw9910 soc_camera sensor driver
On Mon, Mar 05, 2018 at 09:51:48AM +0100, jacopo mondi wrote: > In your suggested fix: > > > (((vdelay >> 8) & 0x3) << 6) | > > (((vact >> 8) & 0x3) << 4) | > > (((hedelay >> 8) & 0x3) << 2) | > > ((hact >> 8) & 0x03); > > > > Won't your analyzer in that case point out that > "15 >> 8 is zero" again? I may have been underestimating it though > It will complain, yes, but it's a pretty common false positive and I have it in the back of my head to teach the static checker to look for that situation. Eventually I will get around to it. regards, dan carpenter
Re: [bug report] media: i2c: Copy tw9910 soc_camera sensor driver
On Fri, Mar 02, 2018 at 03:20:16PM +0100, jacopo mondi wrote:
> Hi Dan,
>
> On Thu, Mar 01, 2018 at 12:59:54PM +0300, Dan Carpenter wrote:
> > [ I know you're just copying files, but you might have a fix for these
> > since you're looking at the code. - dan ]
>
> According to the static analyzer I should simply substitute all those
> expressions with 0s.
I really try not to print warnings for stuff which is just white space
complaints like that. For example, Smatch ignores inconsistent NULL
checking if every caller passes non-NULL parameters or Smatch ignores
comparing unsigned with zero if it's just clamping to between zero and
max.
> I would instead keep them for sake of readability
> and accordance with register description in the video decoder manual.
>
> Thanks
>j
>
[ snip ]
> >511 static int tw9910_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
> >512 {
> >513 struct i2c_client *client = v4l2_get_subdevdata(sd);
> >514 struct tw9910_priv *priv = to_tw9910(client);
> >515 const unsigned int hact = 720;
> >516 const unsigned int hdelay = 15;
> >^^^
> >517 unsigned int vact;
> >518 unsigned int vdelay;
> >519 int ret;
> >520
> >521 if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
> >522 return -EINVAL;
> >523
> >524 priv->norm = norm;
> >525 if (norm & V4L2_STD_525_60) {
> >526 vact = 240;
> >527 vdelay = 18;
> >528 ret = tw9910_mask_set(client, VVBI, 0x10, 0x10);
> >529 } else {
> >530 vact = 288;
> >531 vdelay = 24;
> >532 ret = tw9910_mask_set(client, VVBI, 0x10, 0x00);
> >533 }
> >534 if (!ret)
> >535 ret = i2c_smbus_write_byte_data(client, CROP_HI,
> >536 ((vdelay >> 2) &
> > 0xc0) |
> >537 ((vact >> 4) & 0x30) |
> >538 ((hdelay >> 6) & 0x0c) |
> > ^^^
> > 15 >> 6 is zero.
> >
> >539 ((hact >> 8) & 0x03));
I looked at the spec and it seems to me that we should doing something
like:
(((vdelay >> 8) & 0x3) << 6) |
(((vact >> 8) & 0x3) << 4) |
(((hedelay >> 8) & 0x3) << 2) |
((hact >> 8) & 0x03);
But this is the first time I've looked and it and I can't even be sure
I'm looking in the right place.
regards,
dan carpenter
[PATCH] media: ov5695: Off by one in ov5695_enum_frame_sizes()
The ">" should be ">=" so that we don't read one element beyond the end
of the array.
Fixes: 8a77009be4be ("media: ov5695: add support for OV5695 sensor")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
index a4985a4715f5..9be38a0a2046 100644
--- a/drivers/media/i2c/ov5695.c
+++ b/drivers/media/i2c/ov5695.c
@@ -884,7 +884,7 @@ static int ov5695_enum_frame_sizes(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_frame_size_enum *fse)
{
- if (fse->index > ARRAY_SIZE(supported_modes))
+ if (fse->index >= ARRAY_SIZE(supported_modes))
return -EINVAL;
if (fse->code != MEDIA_BUS_FMT_SBGGR10_1X10)
[bug report] media: i2c: Copy tw9910 soc_camera sensor driver
[ I know you're just copying files, but you might have a fix for these
since you're looking at the code. - dan ]
Hello Jacopo Mondi,
The patch e0d76c3842ee: "media: i2c: Copy tw9910 soc_camera sensor
driver" from Feb 21, 2018, leads to the following static checker
warning:
drivers/media/i2c/tw9910.c:395 tw9910_set_hsync()
warn: odd binop '0x260 & 0x7'
drivers/media/i2c/tw9910.c:396 tw9910_set_hsync()
warn: odd binop '0x300 & 0x7'
drivers/media/i2c/tw9910.c:538 tw9910_s_std()
warn: odd binop '0x0 & 0xc'
drivers/media/i2c/tw9910.c
374 static int tw9910_set_hsync(struct i2c_client *client)
375 {
376 struct tw9910_priv *priv = to_tw9910(client);
377 int ret;
378
379 /* bit 10 - 3 */
380 ret = i2c_smbus_write_byte_data(client, HSBEGIN,
381 (HSYNC_START & 0x07F8) >> 3);
382 if (ret < 0)
383 return ret;
384
385 /* bit 10 - 3 */
386 ret = i2c_smbus_write_byte_data(client, HSEND,
387 (HSYNC_END & 0x07F8) >> 3);
388 if (ret < 0)
389 return ret;
390
391 /* So far only revisions 0 and 1 have been seen */
392 /* bit 2 - 0 */
393 if (priv->revision == 1)
394 ret = tw9910_mask_set(client, HSLOWCTL, 0x77,
395(HSYNC_START & 0x0007) << 4 |
396(HSYNC_END & 0x0007));
These always mask to zero.
397
398 return ret;
399 }
[ snip ]
511 static int tw9910_s_std(struct v4l2_subdev *sd, v4l2_std_id norm)
512 {
513 struct i2c_client *client = v4l2_get_subdevdata(sd);
514 struct tw9910_priv *priv = to_tw9910(client);
515 const unsigned int hact = 720;
516 const unsigned int hdelay = 15;
^^^
517 unsigned int vact;
518 unsigned int vdelay;
519 int ret;
520
521 if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
522 return -EINVAL;
523
524 priv->norm = norm;
525 if (norm & V4L2_STD_525_60) {
526 vact = 240;
527 vdelay = 18;
528 ret = tw9910_mask_set(client, VVBI, 0x10, 0x10);
529 } else {
530 vact = 288;
531 vdelay = 24;
532 ret = tw9910_mask_set(client, VVBI, 0x10, 0x00);
533 }
534 if (!ret)
535 ret = i2c_smbus_write_byte_data(client, CROP_HI,
536 ((vdelay >> 2) & 0xc0) |
537 ((vact >> 4) & 0x30) |
538 ((hdelay >> 6) & 0x0c) |
^^^
15 >> 6 is zero.
539 ((hact >> 8) & 0x03));
540 if (!ret)
541 ret = i2c_smbus_write_byte_data(client, VDELAY_LO,
542 vdelay & 0xff);
543 if (!ret)
544 ret = i2c_smbus_write_byte_data(client, VACTIVE_LO,
545 vact & 0xff);
546
547 return ret;
548 }
regards,
dan carpenter
[bug report] V4L/DVB (3420): Added iocls to configure VBI on tvp5150
[ This is obviously ancient code. It's probably fine. I've just been
going through all array overflow warnings recently. - dan ]
Hello Mauro Carvalho Chehab,
The patch 12db56071b47: "V4L/DVB (3420): Added iocls to configure VBI
on tvp5150" from Jan 23, 2006, leads to the following static checker
warning:
drivers/media/i2c/tvp5150.c:730 tvp5150_get_vbi()
error: buffer overflow 'regs' 5 <= 14
drivers/media/i2c/tvp5150.c
699 static int tvp5150_get_vbi(struct v4l2_subdev *sd,
700 const struct i2c_vbi_ram_value *regs, int line)
701 {
702 struct tvp5150 *decoder = to_tvp5150(sd);
703 v4l2_std_id std = decoder->norm;
704 u8 reg;
705 int pos, type = 0;
706 int i, ret = 0;
707
708 if (std == V4L2_STD_ALL) {
709 dev_err(sd->dev, "VBI can't be configured without
knowing number of lines\n");
710 return 0;
711 } else if (std & V4L2_STD_625_50) {
712 /* Don't follow NTSC Line number convension */
713 line += 3;
714 }
715
716 if (line < 6 || line > 27)
717 return 0;
718
719 reg = ((line - 6) << 1) + TVP5150_LINE_MODE_INI;
720
721 for (i = 0; i <= 1; i++) {
722 ret = tvp5150_read(sd, reg + i);
723 if (ret < 0) {
724 dev_err(sd->dev, "%s: failed with error = %d\n",
725 __func__, ret);
726 return 0;
727 }
728 pos = ret & 0x0f;
729 if (pos < 0x0f)
^^
Smatch thinks this implies pos can be 0-14.
730 type |= regs[pos].type.vbi_type;
^
This array only has 5 elements.
731 }
732
733 return type;
734 }
regards,
dan carpenter
Re: [PATCH] Staging: bcm2048: Fix function argument alignment in radio-bcm2048.c.
On Sun, Feb 18, 2018 at 04:44:46PM -0800, Quytelda Kahja wrote:
> Fix a coding style problem.
>
> Signed-off-by: Quytelda Kahja <quyte...@tamalin.org>
> ---
> drivers/staging/media/bcm2048/radio-bcm2048.c | 24
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/media/bcm2048/radio-bcm2048.c
> b/drivers/staging/media/bcm2048/radio-bcm2048.c
> index 06d1920150da..94141a11e51b 100644
> --- a/drivers/staging/media/bcm2048/radio-bcm2048.c
> +++ b/drivers/staging/media/bcm2048/radio-bcm2048.c
> @@ -1846,6 +1846,7 @@ static int bcm2048_deinit(struct bcm2048_device *bdev)
> static int bcm2048_probe(struct bcm2048_device *bdev)
> {
> int err;
> + u8 default_threshold = BCM2048_DEFAULT_RSSI_THRESHOLD;
>
> err = bcm2048_set_power_state(bdev, BCM2048_POWER_ON);
> if (err < 0)
> @@ -1863,8 +1864,7 @@ static int bcm2048_probe(struct bcm2048_device *bdev)
> if (err < 0)
> goto unlock;
>
> - err = bcm2048_set_fm_search_rssi_threshold(bdev,
> - BCM2048_DEFAULT_RSSI_THRESHOLD);
> + err = bcm2048_set_fm_search_rssi_threshold(bdev, default_threshold);
Nah. Don't do this.
There were some of your earlier patches where I thought:
gdm->tty_dev->send_func(...
Could be shortened to:
tty->send_func(...
So sometimes introducing shorter aliases is the right thing. But here
it just makes it look like a variable when it's a constant. It's makes
the code slightly less readable.
Just ignore the warning.
regards,
dan carpenter
Re: [PATCH v2 5/5] drm: adv7511: Add support for i2c_new_secondary_device
On Mon, Feb 12, 2018 at 06:11:57PM +, Kieran Bingham wrote:
> + adv7511->i2c_packet = i2c_new_secondary_device(i2c, "packet",
> + ADV7511_PACKET_I2C_ADDR_DEFAULT);
> + if (!adv7511->i2c_packet) {
> + ret = -EINVAL;
> + goto err_unregister_cec;
> + }
> +
> + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR,
> + adv7511->i2c_packet->addr << 1);
> +
> INIT_WORK(>hpd_work, adv7511_hpd_work);
>
> if (i2c->irq) {
> @@ -1181,7 +1190,7 @@ static int adv7511_probe(struct i2c_client *i2c, const
> struct i2c_device_id *id)
> IRQF_ONESHOT, dev_name(dev),
> adv7511);
> if (ret)
> - goto err_unregister_cec;
> + goto err_unregister_packet;
> }
>
> adv7511_power_off(adv7511);
There is another goto which needs to be updated if adv7511_cec_init()
fails.
> @@ -1203,6 +1212,8 @@ static int adv7511_probe(struct i2c_client *i2c, const
> struct i2c_device_id *id)
> adv7511_audio_init(dev, adv7511);
> return 0;
>
> +err_unregister_packet:
> + i2c_unregister_device(adv7511->i2c_packet);
> err_unregister_cec:
> i2c_unregister_device(adv7511->i2c_cec);
> if (adv7511->cec_clk)
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
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
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
[PATCH] [media] sr030pc30: prevent array underflow in try_fmt()
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
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: [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
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(>vq_src, buf, file->f_flags &
> > O_NONBLOCK);
> >652 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> >653 ret = vb2_dqbuf(>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: [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 <cla...@baylibre.com> 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: [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 = >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(>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
[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
[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(>vq_src, buf, file->f_flags &
O_NONBLOCK);
652 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
653 ret = vb2_dqbuf(>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(>fh, );
660 return 0;
661 default:
662 return -EINVAL;
663 }
664 }
regards,
dan carpenter
[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 <dan.carpen...@oracle.com>
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 = >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(>width, 8, CAMIF_MAX_PIX_WIDTH,
[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 <dan.carpen...@oracle.com>
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: 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 <dan.carpen...@oracle.com>
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(>lock);
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(>dev, "gpio request/direction_output fail");
> > > goto fail2;
> > > }
> > > - if (ACPI_HANDLE(>dev))
> > > - err = atomisp_register_i2c_module(>sd, NULL, LED_FLASH);
> > > - return 0;
> > > + return atomisp_register_i2c_module(>sd, NULL, LED_FLASH);
> > > fail2:
> > > media_entity_cleanup(>sd.entity);
> > > v4l2_ctrl_handler_free(>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
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(>dev, "gpio request/direction_output fail"); > goto fail2; > } > - if (ACPI_HANDLE(>dev)) > - err = atomisp_register_i2c_module(>sd, NULL, LED_FLASH); > - return 0; > + return atomisp_register_i2c_module(>sd, NULL, LED_FLASH); > fail2: > media_entity_cleanup(>sd.entity); > v4l2_ctrl_handler_free(>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 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(>dev)) > - ret = atomisp_register_i2c_module(>sd, gcpdev, RAW_CAMERA); > + return atomisp_register_i2c_module(>sd, gcpdev, RAW_CAMERA); In the end this should look something like: ret = atomisp_register_i2c_module(>sd, gcpdev, RAW_CAMERA); if (ret) goto err_free_something; return 0; > > - return ret; > out_free: > v4l2_device_unregister_subdev(>sd); > kfree(dev); regards, dan carpenter
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
[bug report] drx: add initial drx-d driver
Hello Ralph Metzler,
The patch 126f1e618870: "drx: add initial drx-d driver" from Mar 12,
2011, leads to the following static checker warning:
drivers/media/dvb-frontends/drxd_hard.c:1305 SC_WaitForReady()
info: return a literal instead of 'status'
drivers/media/dvb-frontends/drxd_hard.c
1298 static int SC_WaitForReady(struct drxd_state *state)
1299 {
1300 int i;
1301
1302 for (i = 0; i < DRXD_MAX_RETRIES; i += 1) {
1303 int status = Read16(state, SC_RA_RAM_CMD__A, NULL, 0);
1304 if (status == 0)
1305 return status;
^
The register is set to zero when ready? The answer should obviously be
yes, but it wouldn't totally surprise me if this function just always
looped 1000 times... Few of the callers check the return. Anyway, it's
more clear to just "return 0;"
1306 }
1307 return -1;
^^
-1 is not a proper error code.
1308 }
regards,
dan carpenter
[bug report] media: lirc: improve locking
Hello Sean Young,
The patch 131fd7fc3c01: "media: lirc: improve locking" from Nov 4,
2017, leads to the following static checker warning:
drivers/media/rc/lirc_dev.c:373 ir_lirc_transmit_ir()
error: 'txbuf' dereferencing possible ERR_PTR()
drivers/media/rc/lirc_dev.c
330 txbuf = memdup_user(buf, n);
331 if (IS_ERR(txbuf)) {
332 ret = PTR_ERR(txbuf);
333 goto out;
This used to be a direct return...
334 }
335 }
336
337 for (i = 0; i < count; i++) {
338 if (txbuf[i] > IR_MAX_DURATION / 1000 - duration ||
!txbuf[i]) {
339 ret = -EINVAL;
340 goto out;
341 }
342
343 duration += txbuf[i];
344 }
345
346 ret = dev->tx_ir(dev, txbuf, count);
347 if (ret < 0)
348 goto out;
349
350 if (fh->send_mode == LIRC_MODE_SCANCODE) {
351 ret = n;
352 } else {
353 for (duration = i = 0; i < ret; i++)
354 duration += txbuf[i];
355
356 ret *= sizeof(unsigned int);
357
358 /*
359 * The lircd gap calculation expects the write function
to
360 * wait for the actual IR signal to be transmitted
before
361 * returning.
362 */
363 towait = ktime_us_delta(ktime_add_us(start, duration),
364 ktime_get());
365 if (towait > 0) {
366 set_current_state(TASK_INTERRUPTIBLE);
367 schedule_timeout(usecs_to_jiffies(towait));
^
This looks like a long wait? Are you sure you want to hold the lock
this whole time?
368 }
369 }
370
371 out:
372 mutex_unlock(>lock);
373 kfree(txbuf);
^
Can't pass an error pointer to kfree().
374 kfree(raw);
regards,
dan carpenter
[PATCH] media: imx274: Silence uninitialized variable warning
Smatch complains that "err" can be uninitialized if we have a zero size
write. The flow analysis is a little complicated so I'm not sure if
that's possible or not, but it's harmless to set this to zero and it
makes the code easier to read.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 800b9bf9cdd3..e7c12933cfd2 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -634,7 +634,7 @@ static int imx274_regmap_util_write_table_8(struct regmap
*regmap,
const struct reg_8 table[],
u16 wait_ms_addr, u16 end_addr)
{
- int err;
+ int err = 0;
const struct reg_8 *next;
u8 val;
Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.
On Sat, Dec 02, 2017 at 08:41:48PM +, Jeremy Sowden wrote:
> On 2017-12-02, at 10:35:06 +, Jeremy Sowden wrote:
> > On 2017-12-02, at 13:20:09 +0300, Dan Carpenter wrote:
> > > On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> > > > -#define DEFAULT_PIPE_INFO \
> > > > -{ \
> > > > - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> > > > - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> > > > - IA_CSS_BINARY_DEFAULT_FRAME_INFO, /* raw_output_info */ \
> > > > - { 0, 0},/* output system in res
> > > > */ \
> > > > - DEFAULT_SHADING_INFO, /* shading_info */ \
> > > > - DEFAULT_GRID_INFO, /* grid_info */ \
> > > > - 0 /* num_invalid_frames
> > > > */ \
> > > > -}
> > > > +#define DEFAULT_PIPE_INFO ( \
> > >
> > > Why does this have a ( now? That can't compile can it??
> >
> > It does.
>
> That was a bit terse: the macros expand to compound-literals, so
> putting parens around them is no different from:
>
> #define THREE (3)
Yeah. Thanks. I figured it out despite the terseness... I try review
as fast as I can, so it means you get the stream of conciousness output
that often has mistakes. Sorry about that.
regards,
dan carpenter
Re: [PATCH v2 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.
On Fri, Dec 01, 2017 at 05:19:37PM +, Jeremy Sowden wrote:
> -#define DEFAULT_PIPE_INFO \
> -{ \
> - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* output_info */ \
> - {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, /* vf_output_info */ \
> - IA_CSS_BINARY_DEFAULT_FRAME_INFO, /* raw_output_info */ \
> - { 0, 0},/* output system in res */ \
> - DEFAULT_SHADING_INFO, /* shading_info */ \
> - DEFAULT_GRID_INFO, /* grid_info */ \
> - 0 /* num_invalid_frames */ \
> -}
> +#define DEFAULT_PIPE_INFO ( \
Why does this have a ( now? That can't compile can it??
> + (struct ia_css_pipe_info) { \
> + .output_info=
> {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> + .vf_output_info =
> {IA_CSS_BINARY_DEFAULT_FRAME_INFO}, \
> + .raw_output_info=
> IA_CSS_BINARY_DEFAULT_FRAME_INFO, \
> + .output_system_in_res_info = { 0, 0 }, \
> + .shading_info = DEFAULT_SHADING_INFO, \
> + .grid_info = DEFAULT_GRID_INFO, \
> + .num_invalid_frames = 0 \
> + } \
> +)
We need to get better compile test coverage on this... :/ There are
some others as well.
regards,
dan carpenter
Re: [PATCH 1/3] media: atomisp: convert default struct values to use compound-literals with designated initializers.
I can't apply this (to today's linux-next) but does this really work:
> +(struct ia_css_3a_grid_info) { \
> + .ae_enable = 0, \
> + .ae_grd_info= (struct ae_public_config_grid_config) { \
> + width = 0, \
> + height = 0, \
> + b_width = 0, \
> + b_height = 0, \
> + x_start = 0, \
> + y_start = 0, \
> + x_end = 0, \
> + y_end = 0 \
I'm pretty sure those lines should start with a period.
- width = 0, \
+ .width = 0, \
regards,
dan
[bug report] media: dvb_frontend: cleanup ioctl handling logic
Hello Mauro Carvalho Chehab,
The patch d73dcf0cdb95: "media: dvb_frontend: cleanup ioctl handling
logic" from Sep 18, 2017, leads to the following static checker
warning:
drivers/media/dvb-core/dvb_frontend.c:2469 dvb_frontend_handle_ioctl()
error: uninitialized symbol 'err'.
drivers/media/dvb-core/dvb_frontend.c
2427 case FE_READ_UNCORRECTED_BLOCKS:
2428 if (fe->ops.read_ucblocks) {
2429 if (fepriv->thread)
2430 err = fe->ops.read_ucblocks(fe, parg);
2431 else
2432 err = -EAGAIN;
2433 }
"err" isn't initialized if ->ops.read_ucblocks is NULL.
2434 break;
2435
2436 /* DEPRECATED DVBv3 ioctls */
2437
2438 case FE_SET_FRONTEND:
2439 err = dvbv3_set_delivery_system(fe);
2440 if (err)
2441 break;
2442
2443 err = dtv_property_cache_sync(fe, c, parg);
2444 if (err)
2445 break;
2446 err = dtv_set_frontend(fe);
2447 break;
2448 case FE_GET_EVENT:
2449 err = dvb_frontend_get_event (fe, parg, file->f_flags);
2450 break;
2451
2452 case FE_GET_FRONTEND: {
2453 struct dtv_frontend_properties getp =
fe->dtv_property_cache;
2454
2455 /*
2456 * Let's use our own copy of property cache, in order to
2457 * avoid mangling with DTV zigzag logic, as drivers
might
2458 * return crap, if they don't check if the data is
available
2459 * before updating the properties cache.
2460 */
2461 err = dtv_get_frontend(fe, , parg);
2462 break;
2463 }
2464
2465 default:
2466 return -ENOTSUPP;
2467 } /* switch */
2468
2469 return err;
^^
2470 }
regards,
dan carpenter
Re: [PATCH v2 1/3] media: staging: atomisp: fix for sparse "using plain integer as NULL pointer" warnings.
On Tue, Nov 28, 2017 at 11:33:37PM +, Jeremy Sowden wrote:
> On 2017-11-28, at 17:15:24 +0300, Dan Carpenter wrote:
> > On Mon, Nov 27, 2017 at 12:44:48PM +, Jeremy Sowden wrote:
> > > The "address" member of struct ia_css_host_data is a
> > > pointer-to-char, so define default as NULL.
> > >
> > > ---
> > > a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h
> > > +++
> > > b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h
> > > @@ -95,7 +95,7 @@ union ia_css_all_memory_offsets {
> > > };
> > >
> > > #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \
> > > - { { { { 0, 0 } } } }
> > > + { { { { NULL, 0 } } } }
> >
> > This define is way ugly and instead of making superficial changes, you
> > should try to eliminate it.
> >
> > People look at warnings as a bad thing but they are actually a
> > valuable resource which call attention to bad code. By making this
> > change you're kind of wasting the warning. The bad code is still
> > there, it's just swept under the rug but like a dead mouse carcass
> > it's still stinking up the living room. We should leave the warning
> > there until it irritates someone enough to fix it properly.
>
> Tracking down the offending initializer was definitely a pain.
>
> Compound literals with designated initializers would make this macro
> (and a number of others) easier to understand and more type-safe:
>
>#define IA_CSS_DEFAULT_ISP_MEM_PARAMS \
> - { { { { 0, 0 } } } }
> + (struct ia_css_isp_param_host_segments) { \
> + .params = { { \
> + (struct ia_css_host_data) { \
> + .address = NULL, \
> + .size = 0 \
> + } \
> + } } \
> + }
Using designated initializers is good, yes. Can't we just use an
empty initializer since this is all zeroed memory anyway?
(struct ia_css_isp_param_host_segments) { }
I haven't tried it.
>
> Unfortunately this default value is one end of a chain of default values
Yeah. A really long chain...
> used to initialize members of default values of enclosing structs where
> the outermost values are used to initialize some static variables:
>
> static enum ia_css_err
> init_pipe_defaults(enum ia_css_pipe_mode mode,
>struct ia_css_pipe *pipe,
>bool copy_pipe)
> {
> static struct ia_css_pipe default_pipe = IA_CSS_DEFAULT_PIPE;
> static struct ia_css_preview_settings prev =
> IA_CSS_DEFAULT_PREVIEW_SETTINGS;
> static struct ia_css_capture_settings capt =
> IA_CSS_DEFAULT_CAPTURE_SETTINGS;
> static struct ia_css_video_settings video =
> IA_CSS_DEFAULT_VIDEO_SETTINGS;
> static struct ia_css_yuvpp_settings yuvpp =
> IA_CSS_DEFAULT_YUVPP_SETTINGS;
>
> if (pipe == NULL) {
> IA_CSS_ERROR("NULL pipe parameter");
> return IA_CSS_ERR_INVALID_ARGUMENTS;
> }
>
> /* Initialize pipe to pre-defined defaults */
> *pipe = default_pipe;
>
> /* TODO: JB should not be needed, but temporary backward reference */
> switch (mode) {
> case IA_CSS_PIPE_MODE_PREVIEW:
> pipe->mode = IA_CSS_PIPE_ID_PREVIEW;
> pipe->pipe_settings.preview = prev;
> break;
> case IA_CSS_PIPE_MODE_CAPTURE:
> if (copy_pipe) {
> pipe->mode = IA_CSS_PIPE_ID_COPY;
> } else {
> pipe->mode = IA_CSS_PIPE_ID_CAPTURE;
> }
> pipe->pipe_settings.capture = capt;
> break;
> case IA_CSS_PIPE_MODE_VIDEO:
> pipe->mode = IA_CSS_PIPE_ID_VIDEO;
> pipe->pipe_settings.video = video;
> break;
> case IA_CSS_PIPE_MODE_ACC:
> pipe->mode = IA_CSS_PIPE_ID_ACC;
> break;
> case IA_CSS_PIPE_MODE_COPY:
> pipe->mode = IA_CSS_PIPE_ID_CAPTURE;
> break;
> case IA_CSS_PIPE_MODE_YUVPP:
> pipe->mode = IA_CSS_PIPE_ID_YUVPP;
> pipe->pipe_settings.yuvpp = yuvpp;
> break;
> default:
> return IA_CSS_ERR_INVALID_ARGUMENTS;
> }
>
> return IA_CSS_SUCCESS;
> }
>
> and GCC's limited support for using compound literals to initialize
> static variables doesn't stretch this far.
>
> I'm not convinced, however, that those variables actually achieve very
> much. If I change the code to assign the defaults directly, the problem
> goes away:
>
> diff --git a/drivers/staging/media/atomisp/pci/atomi
Re: [PATCH v2 1/3] media: staging: atomisp: fix for sparse "using plain integer as NULL pointer" warnings.
On Mon, Nov 27, 2017 at 12:44:48PM +, Jeremy Sowden wrote:
> The "address" member of struct ia_css_host_data is a pointer-to-char, so
> define default as NULL.
>
> Signed-off-by: Jeremy Sowden <jer...@azazel.net>
> ---
> .../css2400/runtime/isp_param/interface/ia_css_isp_param_types.h| 2
> +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h
>
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h
> index 8e651b80345a..6fee3f7fd184 100644
> ---
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h
> +++
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h
> @@ -95,7 +95,7 @@ union ia_css_all_memory_offsets {
> };
>
> #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \
> - { { { { 0, 0 } } } }
> + { { { { NULL, 0 } } } }
This define is way ugly and instead of making superficial changes, you
should try to eliminate it.
People look at warnings as a bad thing but they are actually a valuable
resource which call attention to bad code. By making this change you're
kind of wasting the warning. The bad code is still there, it's just
swept under the rug but like a dead mouse carcass it's still stinking up
the living room. We should leave the warning there until it irritates
someone enough to fix it properly.
regards,
dan carpenter
Re: [PATCH v4 3/5] staging: Introduce NVIDIA Tegra video decoder driver
On Sat, Nov 11, 2017 at 04:06:52PM +0200, Vladimir Zapolskiy wrote:
> > + if (!wait_dma)
> > + return 0;
> > +
> > + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value,
> > +!(value & BSE_DMA_BUSY), 1, 100);
> > + if (err) {
> > + dev_err(dev, "BSEV DMA timeout\n");
> > + return err;
> > + }
> > +
> > + return 0;
>
> if (err)
> dev_err(dev, "BSEV DMA timeout\n");
>
> return err;
>
> is two lines shorter.
>
This is fine, but just watch out because getting clever with a last if
statement is a common anti-pattern. For example, you often see it where
people do success handling instead of failure handling. And it leads
to static checker bugs, and makes the code slightly more subtle.
> > + err = tegra_vde_attach_dmabuf(dev, source->aux_fd,
> > + source->aux_offset, csize,
> > + >aux_dmabuf_attachment,
> > + >aux_addr,
> > + >aux_sgt,
> > + NULL, dma_dir);
> > + if (err)
> > + goto err_release_cr;
> > + }
> > +
> > + return 0;
>
> if (!err)
> return 0;
>
> and then remove a check above.
>
Argh Success handling. Always do failure handling, never success
handling.
The rest of your comments I agree with, though.
regards,
dan carpenter
[PATCH] [media] cpia2: Fix a couple off by one bugs
The cam->buffers[] array has cam->num_frames elements so the > needs to
be changed to >= to avoid going beyond the end of the array. The
->buffers[] array is allocated in cpia2_allocate_buffers() if you want
to confirm.
Fixes: ab33d5071de7 ("V4L/DVB (3376): Add cpia2 camera support")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c
b/drivers/media/usb/cpia2/cpia2_v4l.c
index 3dedd83f0b19..a1c59f19cf2d 100644
--- a/drivers/media/usb/cpia2/cpia2_v4l.c
+++ b/drivers/media/usb/cpia2/cpia2_v4l.c
@@ -808,7 +808,7 @@ static int cpia2_querybuf(struct file *file, void *fh,
struct v4l2_buffer *buf)
struct camera_data *cam = video_drvdata(file);
if(buf->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
- buf->index > cam->num_frames)
+ buf->index >= cam->num_frames)
return -EINVAL;
buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
@@ -859,7 +859,7 @@ static int cpia2_qbuf(struct file *file, void *fh, struct
v4l2_buffer *buf)
if(buf->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
buf->memory != V4L2_MEMORY_MMAP ||
- buf->index > cam->num_frames)
+ buf->index >= cam->num_frames)
return -EINVAL;
DBG("QBUF #%d\n", buf->index);
[ragnatech:media-tree 2807/2822] drivers/media/i2c/imx274.c:659 imx274_regmap_util_write_table_8() error: uninitialized symbol 'err'.
tree: git://git.ragnatech.se/linux media-tree
head: bbae615636155fa43a9b0fe0ea31c678984be864
commit: 0985dd306f727df6c0e71cd8a8eda93e8fa5206e [2807/2822] media: imx274:
V4l2 driver for Sony imx274 CMOS sensor
drivers/media/i2c/imx274.c:659 imx274_regmap_util_write_table_8() error:
uninitialized symbol 'err'.
git remote add ragnatech git://git.ragnatech.se/linux
git remote update ragnatech
git checkout 0985dd306f727df6c0e71cd8a8eda93e8fa5206e
vim +/err +659 drivers/media/i2c/imx274.c
0985dd30 Leon Luo 2017-10-05 621
0985dd30 Leon Luo 2017-10-05 622 /*
0985dd30 Leon Luo 2017-10-05 623 * imx274_regmap_util_write_table_8 -
Function for writing register table
0985dd30 Leon Luo 2017-10-05 624 * @regmap: Pointer to device reg map
structure
0985dd30 Leon Luo 2017-10-05 625 * @table: Table containing register values
0985dd30 Leon Luo 2017-10-05 626 * @wait_ms_addr: Flag for performing delay
0985dd30 Leon Luo 2017-10-05 627 * @end_addr: Flag for incating end of table
0985dd30 Leon Luo 2017-10-05 628 *
0985dd30 Leon Luo 2017-10-05 629 * This is used to write register table into
sensor's reg map.
0985dd30 Leon Luo 2017-10-05 630 *
0985dd30 Leon Luo 2017-10-05 631 * Return: 0 on success, errors otherwise
0985dd30 Leon Luo 2017-10-05 632 */
0985dd30 Leon Luo 2017-10-05 633 static int
imx274_regmap_util_write_table_8(struct regmap *regmap,
0985dd30 Leon Luo 2017-10-05 634
const struct reg_8 table[],
0985dd30 Leon Luo 2017-10-05 635 u16
wait_ms_addr, u16 end_addr)
0985dd30 Leon Luo 2017-10-05 636 {
0985dd30 Leon Luo 2017-10-05 637 int err;
0985dd30 Leon Luo 2017-10-05 638 const struct reg_8 *next;
0985dd30 Leon Luo 2017-10-05 639 u8 val;
0985dd30 Leon Luo 2017-10-05 640
0985dd30 Leon Luo 2017-10-05 641 int range_start = -1;
0985dd30 Leon Luo 2017-10-05 642 int range_count = 0;
0985dd30 Leon Luo 2017-10-05 643 u8 range_vals[16];
0985dd30 Leon Luo 2017-10-05 644 int max_range_vals =
ARRAY_SIZE(range_vals);
0985dd30 Leon Luo 2017-10-05 645
0985dd30 Leon Luo 2017-10-05 646 for (next = table;; next++) {
0985dd30 Leon Luo 2017-10-05 647 if ((next->addr != range_start
+ range_count) ||
0985dd30 Leon Luo 2017-10-05 648 (next->addr == end_addr) ||
0985dd30 Leon Luo 2017-10-05 649 (next->addr ==
wait_ms_addr) ||
0985dd30 Leon Luo 2017-10-05 650 (range_count ==
max_range_vals)) {
0985dd30 Leon Luo 2017-10-05 651 if (range_count == 1)
0985dd30 Leon Luo 2017-10-05 652 err =
regmap_write(regmap,
0985dd30 Leon Luo 2017-10-05 653
range_start, range_vals[0]);
0985dd30 Leon Luo 2017-10-05 654 else if (range_count >
1)
0985dd30 Leon Luo 2017-10-05 655 err =
regmap_bulk_write(regmap, range_start,
0985dd30 Leon Luo 2017-10-05 656
_vals[0],
0985dd30 Leon Luo 2017-10-05 657
range_count);
0985dd30 Leon Luo 2017-10-05 658
0985dd30 Leon Luo 2017-10-05 @659 if (err)
0985dd30 Leon Luo 2017-10-05 660 return err;
0985dd30 Leon Luo 2017-10-05 661
0985dd30 Leon Luo 2017-10-05 662 range_start = -1;
0985dd30 Leon Luo 2017-10-05 663 range_count = 0;
0985dd30 Leon Luo 2017-10-05 664
0985dd30 Leon Luo 2017-10-05 665 /* Handle special
address values */
0985dd30 Leon Luo 2017-10-05 666 if (next->addr ==
end_addr)
0985dd30 Leon Luo 2017-10-05 667 break;
0985dd30 Leon Luo 2017-10-05 668
0985dd30 Leon Luo 2017-10-05 669 if (next->addr ==
wait_ms_addr) {
0985dd30 Leon Luo 2017-10-05 670
msleep_range(next->val);
0985dd30 Leon Luo 2017-10-05 671 continue;
0985dd30 Leon Luo 2017-10-05 672 }
0985dd30 Leon Luo 2017-10-05 673 }
0985dd30 Leon Luo 2017-10-05 674
0985dd30 Leon Luo 2017-10-05 675 val = next->val;
0985dd30 Leon Luo 2017-10-05 676
0985dd30 Leon Luo 2017-10-05 677 if (range_start == -1)
0985dd30 Leon Luo 2017-10-05 678 range_start =
next->addr;
0985dd30 Leon Luo 2017-10-05 679
0985dd30 Leon Luo 2017-10-05 680 range_vals[range_count++] = val;
0985dd30 Leon Luo 2017-10-05 681 }
0985dd30 Leon Luo 2017-10-05 682 return 0;
0985dd30 Leon Luo 2017-10-05 683 }
0985dd30 Leon Luo 2017-10-05 684
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all
Re: [PATCH] [STABLE-4.13] [media] imx-media-of: avoid uninitialized variable warning
On Wed, Oct 04, 2017 at 03:34:55PM +0200, Arnd Bergmann wrote:
> Replaces upstream commit 0b2e9e7947e7 ("media: staging/imx: remove
> confusing IS_ERR_OR_NULL usage")
>
> We get a harmless warning about a potential uninitialized variable
> use in the driver:
>
> drivers/staging/media/imx/imx-media-of.c: In function 'of_parse_subdev':
> drivers/staging/media/imx/imx-media-of.c:216:4: warning: 'remote_np' may be
> used uninitialized in this function [-Wmaybe-uninitialized]
>
> I reworked that code to be easier to understand by gcc in mainline,
> but that commit is too large to backport. This is a much simpler
> workaround, avoiding the warning by adding a fake initialization
> to the variable. The driver was only introduced in linux-4.13,
> so the workaround is not needed for earlier stable kernels.
>
> Fixes: e130291212df ("[media] media: Add i.MX media core driver")
I normally leave off the Fixes tag when it's not a bugfix. The warning
is, as you mentioned, harmless.
regards,
dan carpenter
[bug report] V4L/DVB (13717): [MB86A16] Statistics Updates
Hello Manu Abraham,
The patch 77557abef0de: "V4L/DVB (13717): [MB86A16] Statistics
Updates" from Dec 3, 2009, leads to the following static checker
warning:
drivers/media/dvb-frontends/mb86a16.c:1690 mb86a16_read_ber()
error: uninitialized symbol 'timer'.
drivers/media/dvb-frontends/mb86a16.c:1706 mb86a16_read_ber()
error: uninitialized symbol 'timer'.
drivers/media/dvb-frontends/mb86a16.c
1649 static int mb86a16_read_ber(struct dvb_frontend *fe, u32 *ber)
1650 {
1651 u8 ber_mon, ber_tab, ber_lsb, ber_mid, ber_msb, ber_tim,
ber_rst;
1652 u32 timer;
^
1653
1654 struct mb86a16_state *state = fe->demodulator_priv;
1655
1656 *ber = 0;
1657 if (mb86a16_read(state, MB86A16_BERMON, _mon) != 2)
1658 goto err;
1659 if (mb86a16_read(state, MB86A16_BERTAB, _tab) != 2)
1660 goto err;
1661 if (mb86a16_read(state, MB86A16_BERLSB, _lsb) != 2)
1662 goto err;
1663 if (mb86a16_read(state, MB86A16_BERMID, _mid) != 2)
1664 goto err;
1665 if (mb86a16_read(state, MB86A16_BERMSB, _msb) != 2)
1666 goto err;
1667 /* BER monitor invalid when BER_EN = 0 */
1668 if (ber_mon & 0x04) {
1669 /* coarse, fast calculation */
1670 *ber = ber_tab & 0x1f;
1671 dprintk(verbose, MB86A16_DEBUG, 1, "BER
coarse=[0x%02x]", *ber);
1672 if (ber_mon & 0x01) {
1673 /*
1674 * BER_SEL = 1, The monitored BER is the
estimated
1675 * value with a Reed-Solomon decoder error
amount at
1676 * the deinterleaver output.
1677 * monitored BER is expressed as a 20 bit
output in total
1678 */
1679 ber_rst = ber_mon >> 3;
^^^
How do we know ber_rst is in the 0-3 range?
1680 *ber = (((ber_msb << 8) | ber_mid) << 8) |
ber_lsb;
1681 if (ber_rst == 0)
1682 timer = 1250;
1683 if (ber_rst == 1)
1684 timer = 2500;
1685 if (ber_rst == 2)
1686 timer = 5000;
1687 if (ber_rst == 3)
1688 timer = 1;
1689
1690 *ber /= timer;
1691 dprintk(verbose, MB86A16_DEBUG, 1, "BER
fine=[0x%02x]", *ber);
1692 } else {
regards,
dan carpenter
[PATCH] media: tc358743: remove an unneeded condition
We can remove the check for if "state->cec_adap" is NULL. The
cec_allocate_adapter() function never returns NULL and also we verified
that "state->cec_adap" is an error pointer.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index e1d8eef7055e..6bbe112be267 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -2122,7 +2122,7 @@ static int tc358743_probe(struct i2c_client *client,
state, dev_name(>dev),
CEC_CAP_DEFAULTS | CEC_CAP_MONITOR_ALL, CEC_MAX_LOG_ADDRS);
if (IS_ERR(state->cec_adap)) {
- err = state->cec_adap ? PTR_ERR(state->cec_adap) : -ENOMEM;
+ err = PTR_ERR(state->cec_adap);
goto err_hdl;
}
irq_mask |= MASK_CEC_RMSK | MASK_CEC_TMSK;
Re: [PATCH] [media] bt8xx: Use common error handling code in two functions
On Mon, Sep 25, 2017 at 10:18:29PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Mon, 25 Sep 2017 22:10:17 +0200
>
> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of these functions.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
> ---
> drivers/media/pci/bt8xx/dst.c| 19 +++
> drivers/media/pci/bt8xx/dst_ca.c | 30 +++---
> 2 files changed, 26 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/media/pci/bt8xx/dst.c b/drivers/media/pci/bt8xx/dst.c
> index 7166d2279465..1290419aca0b 100644
> --- a/drivers/media/pci/bt8xx/dst.c
> +++ b/drivers/media/pci/bt8xx/dst.c
> @@ -134,17 +134,20 @@ EXPORT_SYMBOL(rdc_reset_state);
> static int rdc_8820_reset(struct dst_state *state)
> {
> dprintk(3, "Resetting DST\n");
> - if (dst_gpio_outb(state, RDC_8820_RESET, RDC_8820_RESET, 0, NO_DELAY) <
> 0) {
> - pr_err("dst_gpio_outb ERROR !\n");
> - return -1;
> - }
> + if (dst_gpio_outb(state, RDC_8820_RESET, RDC_8820_RESET, 0, NO_DELAY)
> + < 0)
> + goto report_failure;
> +
> udelay(1000);
> - if (dst_gpio_outb(state, RDC_8820_RESET, RDC_8820_RESET,
> RDC_8820_RESET, DELAY) < 0) {
> - pr_err("dst_gpio_outb ERROR !\n");
> - return -1;
> - }
> + if (dst_gpio_outb(state, RDC_8820_RESET, RDC_8820_RESET,
> + RDC_8820_RESET, DELAY) < 0)
> + goto report_failure;
>
> return 0;
> +
> +report_failure:
> + pr_err("dst_gpio_outb ERROR !\n");
> + return -1;
This code is ugly and this patch doesn't improve it; it just shuffles
it around.
regards,
dan carpenter
[PATCH] [media] stk-webcam: Fix use after free on disconnect
We free the stk_camera device too early. It's allocate first in probe
and it should be freed last in stk_camera_disconnect().
Reported-by: Andrey Konovalov <andreyk...@google.com>
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
Not tested but these bug reports seem surprisingly straight forward.
Thanks Andrey!
diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c
b/drivers/media/usb/stkwebcam/stk-webcam.c
index c0bba773db25..e748c976d967 100644
--- a/drivers/media/usb/stkwebcam/stk-webcam.c
+++ b/drivers/media/usb/stkwebcam/stk-webcam.c
@@ -1241,7 +1241,6 @@ static void stk_v4l_dev_release(struct video_device *vd)
if (dev->sio_bufs != NULL || dev->isobufs != NULL)
pr_err("We are leaking memory\n");
usb_put_intf(dev->interface);
- kfree(dev);
}
static const struct video_device stk_v4l_data = {
@@ -1391,6 +1390,7 @@ static void stk_camera_disconnect(struct usb_interface
*interface)
video_unregister_device(>vdev);
v4l2_ctrl_handler_free(>hdl);
v4l2_device_unregister(>v4l2_dev);
+ kfree(dev);
}
#ifdef CONFIG_PM
Re: [PATCH 2/4] [media] usbvision-core: Use common error handling code in usbvision_set_compress_params()
On Thu, Sep 21, 2017 at 05:07:06PM +0200, SF Markus Elfring wrote: > @@ -1913,11 +1908,12 @@ static int usbvision_set_compress_params(struct > usb_usbvision *usbvision) >USB_DIR_OUT | USB_TYPE_VENDOR | >USB_RECIP_ENDPOINT, 0, >(__u16) USBVISION_PCM_THR1, value, 6, HZ); > + if (rc < 0) > +report_failure: > + dev_err(>dev->dev, > + "%s: ERROR=%d. USBVISION stopped - reconnect or reload > driver.\n", > + __func__, rc); You've been asked several times not to write code like this. You do it later in the patch series as well. regards, dan carpenter
Re: [PATCH 3/4] [media] usbvision-core: Delete unnecessary braces in 11 functions
No. Multi-line indents get curly braces for readability. regards, dan carpenter
Re: [media] s2255drv: Adjust 13 checks for null pointers
On Thu, Sep 21, 2017 at 10:12:56AM +0200, SF Markus Elfring wrote: > >> MIME-Version: 1.0 > >> Content-Type: text/plain; charset=UTF-8 > >> Content-Transfer-Encoding: 8bit > >> > > > > You've been told several times that this stuff doesn't work. > > This functionality might not exactly work in the way that you expect so far. > > > > Try applying this patch with `git am` and you'll see why. > > I find that these extra message fields work in the way that was designed > by the Git software developers. > > elfring@Sonne:~/Projekte/Linux/next-patched> LANG=C git checkout -b > next_deletion_of_oom_messages_in_s2255drv_test_20170921 > next_deletion_of_oom_messages-20170905 && LANG=C git am '../[PATCH 2_5] > [media] s2255drv: Adjust 13 checks for null pointers.eml' > Switched to a new branch > 'next_deletion_of_oom_messages_in_s2255drv_test_20170921' > Applying: s2255drv: Adjust 13 checks for null pointers > > > Would you like to clarify corresponding concerns any more? > Look at the `git log` and it just copies those lines: commit 2a47170a824697783d8c2d28355a806f075c76e4 (HEAD) Author: Markus Elfring <elfr...@users.sourceforge.net> Date: Wed Sep 20 16:46:19 2017 +0200 s2255drv: Adjust 13 checks for null pointers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The script “checkpatch.pl” pointed information out like the following. Comparison to NULL could be written !… Thus fix the affected source code places. regards, dan carpenter
Re: [PATCH] media: dvb_ca_en50221: sanity check slot number from userspace
Looks good. Reviewed-by: Dan Carpenter <dan.carpen...@oracle.com> regards, dan carpenter
Re: [PATCH 2/5] [media] s2255drv: Adjust 13 checks for null pointers
On Wed, Sep 20, 2017 at 06:58:56PM +0200, SF Markus Elfring wrote: > From: Markus Elfring <elfr...@users.sourceforge.net> > Date: Wed, 20 Sep 2017 16:46:19 +0200 > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > You've been told several times that this stuff doesn't work. Try applying this patch with `git am` and you'll see why. regards, dan carpenter
Re: [PATCH] [media] Siano: Use common error handling code in smsusb_init_device()
On Wed, Sep 20, 2017 at 02:40:28PM +0200, SF Markus Elfring wrote: > From: Markus Elfring <elfr...@users.sourceforge.net> > Date: Wed, 20 Sep 2017 14:30:55 +0200 > > Add a jump target so that a bit of exception handling can be better > reused at the end of this function. > > This refactoring might fix also an error situation where the > function "kfree" was not called after a software failure > was noticed in two cases. > No. It doesn't fix a leak, it introduces a double free. If smscore_register_device() succeeds then mdev is freed when we call smsusb_term_device(intf); The call tree is: smsusb_term_device() -> smscore_unregister_device() -> smscore_notify_clients() -> smsdvb_onremove() -> smsdvb_unregister_client() -> smsdvb_media_device_unregister() -> kfree(coredev->media_dev); regards, dan carpenter
Re: [PATCH 4/6] [media] go7007: Use common error handling code in s2250_probe()
On Wed, Sep 20, 2017 at 09:09:16AM +0200, SF Markus Elfring wrote:
> >> @@ -555,17 +553,13 @@ static int s2250_probe(struct i2c_client *client,
> >>/* initialize the audio */
> >>if (write_regs(audio, aud_regs) < 0) {
> >>dev_err(>dev, "error initializing audio\n");
> >> - goto fail;
> >> + goto e_io;
> >
> > Preserve the error code.
>
> Do you suggest then to adjust the implementation of the function "write_regs"
> so that a more meaningful value would be used instead of the failure
> indication "-1"?
>
If you want to, yeah, that would be good.
regards,
dan carpenter
Re: [PATCH 5/6] [media] go7007: Use common error handling code in go7007_snd_init()
On Mon, Sep 18, 2017 at 03:58:37PM +0200, SF Markus Elfring wrote:
> diff --git a/drivers/media/usb/go7007/snd-go7007.c
> b/drivers/media/usb/go7007/snd-go7007.c
> index 68e421bf38e1..7ae4d03ed3f7 100644
> --- a/drivers/media/usb/go7007/snd-go7007.c
> +++ b/drivers/media/usb/go7007/snd-go7007.c
> @@ -243,22 +243,18 @@ int go7007_snd_init(struct go7007 *go)
> gosnd->capturing = 0;
> ret = snd_card_new(go->dev, index[dev], id[dev], THIS_MODULE, 0,
> >card);
> - if (ret < 0) {
> - kfree(gosnd);
> - return ret;
> - }
> + if (ret < 0)
> + goto free_snd;
> +
> ret = snd_device_new(gosnd->card, SNDRV_DEV_LOWLEVEL, go,
> _snd_device_ops);
> - if (ret < 0) {
> - kfree(gosnd);
> - return ret;
> - }
> + if (ret < 0)
> + goto free_snd;
> +
I think the original code is buggy. It should probably call
snd_card_free() if snd_device_new() fails.
regards,
dan carpenter
Re: [PATCH 4/6] [media] go7007: Use common error handling code in s2250_probe()
On Mon, Sep 18, 2017 at 03:57:21PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Mon, 18 Sep 2017 13:50:45 +0200
>
> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of this function.
>
> This refactoring might fix also an error situation where the
> function "i2c_unregister_device" was not called after a software failure
> was noticed by the data member "hdl.error".
>
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
> ---
> drivers/media/usb/go7007/s2250-board.c | 37
> +-
> 1 file changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/usb/go7007/s2250-board.c
> b/drivers/media/usb/go7007/s2250-board.c
> index 1fd4c09dd516..1bd9a7f2e7a3 100644
> --- a/drivers/media/usb/go7007/s2250-board.c
> +++ b/drivers/media/usb/go7007/s2250-board.c
> @@ -510,6 +510,7 @@ static int s2250_probe(struct i2c_client *client,
> u8 *data;
> struct go7007 *go = i2c_get_adapdata(adapter);
> struct go7007_usb *usb = go->hpi_context;
> + int code;
It should be called "int rc" to match the rest of the driver. "ret" or
"err" would also have been acceptable.
>
> audio = i2c_new_dummy(adapter, TLV320_ADDRESS >> 1);
> if (!audio)
> @@ -517,8 +518,8 @@ static int s2250_probe(struct i2c_client *client,
>
> state = kzalloc(sizeof(*state), GFP_KERNEL);
> if (!state) {
> - i2c_unregister_device(audio);
> - return -ENOMEM;
> + code = -ENOMEM;
> + goto unregister_device;
> }
>
> sd = >sd;
> @@ -538,11 +539,8 @@ static int s2250_probe(struct i2c_client *client,
> V4L2_CID_HUE, -512, 511, 1, 0);
> sd->ctrl_handler = >hdl;
> if (state->hdl.error) {
> - int err = state->hdl.error;
> -
> - v4l2_ctrl_handler_free(>hdl);
> - kfree(state);
> - return err;
> + code = state->hdl.error;
> + goto free_handler;
> }
>
> state->std = V4L2_STD_NTSC;
> @@ -555,17 +553,13 @@ static int s2250_probe(struct i2c_client *client,
> /* initialize the audio */
> if (write_regs(audio, aud_regs) < 0) {
> dev_err(>dev, "error initializing audio\n");
> - goto fail;
> + goto e_io;
Preserve the error code.
regards,
dan carpenter
Re: [PATCH v3] drivers/staging:[media]atomisp:use ARRAY_SIZE() instead of open coding.
On Wed, Sep 13, 2017 at 02:27:53PM +0530, Allen Pais wrote: > Signed-off-by: Allen Pais <allen.l...@gmail.com> Sorry, the patch is right, but the commit is still totally messed up. bad: [PATCH v3] drivers/staging:[media]atomisp:use ARRAY_SIZE() instead of open coding. good: [PATCH v4] [media] atomisp: use ARRAY_SIZE() instead of open coding. Please, copy the "[media] atomisp: " prefix exactly as I wrote it. Then the commit message can say something like: The array_length() macro just duplicates ARRAY_SIZE() so we can delete it. > Signed-off-by: Allen Pais <allen.l...@gmail.com> > --- ^^^ Then under the --- line put: v4: Update the commit message. regards, dan carpenter
Re: [PATCH v2] drivers:staging/media:Use ARRAY_SIZE() for the size calculation of the array
On Wed, Sep 13, 2017 at 02:09:25PM +0530, Allen Pais wrote: > Signed-off-by: Allen Pais <allen.l...@gmail.com> This is going through linux-media and maybe they won't insist on some kind of commit message, but I know Greg does. regards, dan carpenter
Re: [PATCH] drivers:staging/media:Use ARRAY_SIZE() for the size calculation of the array
Also change the subject prefix to: [media] atomisp: so it's: Subject: [PATCH v2] [media] atomisp: Use ARRAY_SIZE() instead of open coding it regards, dan carpenter
Re: [PATCH] drivers:staging/media:Use ARRAY_SIZE() for the size calculation of the array
You need a changelog.
On Wed, Sep 13, 2017 at 01:34:39PM +0530, Allen Pais wrote:
> Signed-off-by: Allen Pais <allen.l...@gmail.com>
> ---
> drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> index e882b55..d822918 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> @@ -451,7 +451,7 @@ static enum ia_css_frame_format yuv422_copy_formats[] = {
> IA_CSS_FRAME_FORMAT_YUYV
> };
>
> -#define array_length(array) (sizeof(array)/sizeof(array[0]))
> +#define array_length(array) (ARRAY_SIZE(array))
Just get rid of this array_length macro and use ARRAY_SIZE() directly.
regards,
dan carpenter
Re: [PATCH 1/2] staging: atomisp: add menu entries to choose between ATOMISP_2400 and ATOMISP_2401.
No changelog. No Signed-off-by line. Without reading too carefully, or trying to do a build, it looks like we're activating the menu items and then fixing the build. It should be the other way around so that we don't break git bisect. People are always doing randconfig and the autobuilders detect breakage really quick. On Mon, Sep 11, 2017 at 08:50:26PM +0200, Vincent Hervieux wrote: > @@ -347,8 +347,16 @@ DEFINES := -DHRT_HW -DHRT_ISP_CSS_CUSTOM_HOST > -DHRT_USE_VIR_ADDRS -D__HOST__ > #DEFINES += -DPUNIT_CAMERA_BUSY > #DEFINES += -DUSE_KMEM_CACHE > > +ifeq ($(CONFIG_VIDEO_ATOMISP_ISP2400),y) > +# Merrifield, Baytrail > DEFINES += -DATOMISP_POSTFIX=\"css2400b0_v21\" -DISP2400B0 > DEFINES += -DSYSTEM_hive_isp_css_2400_system -DISP2400 > +endif > +ifeq ($(CONFIG_VIDEO_ATOMISP_ISP2401),y) > +# Anniedale (Merrifield+ / Moorefield), Cherrytrail > +DEFINES += -DATOMISP_POSTFIX=\"css2401a0_v21\" -DISP2401A0 > +DEFINES += -DSYSTEM_hive_isp_css_2400_system -DISP2401 > -DISP2401_NEW_INPUT_SYSTEM These defines are a bit ugly. Eventually we will want to get rid of these. regards, dan carpenter
Re: [PATCH 2/2] staging: atomisp: fix compilation errors in case of ISP2401.
We always need a changelog. And actually this seems a bit involved so
there is stuff to explain.
On Mon, Sep 11, 2017 at 08:51:15PM +0200, Vincent Hervieux wrote:
> Signed-off-by: Vincent Hervieux <vincent.hervi...@gmail.com>
> ---
> .../media/atomisp/pci/atomisp2/atomisp_cmd.c | 5 ++---
> .../media/atomisp/pci/atomisp2/atomisp_v4l2.c | 6 +-
> .../pci/atomisp2/css2400/ia_css_acc_types.h| 1 +
> .../css2400/runtime/debug/src/ia_css_debug.c | 3 ---
> .../media/atomisp/pci/atomisp2/css2400/sh_css.c| 24
> ++
> .../atomisp/pci/atomisp2/css2400/sh_css_params.c | 8 +---
> 6 files changed, 20 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index f48bf451c1f5..d79a3cfb834d 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -1045,9 +1045,8 @@ void atomisp_buf_done(struct atomisp_sub_device *asd,
> int error,
> asd->re_trigger_capture = false;
> dev_dbg(isp->dev, "Trigger capture again for
> new buffer. err=%d\n",
> err);
> - } else {
> - asd->re_trigger_capture = true;
> - }
> + } else {
> + asd->re_trigger_capture = true;
> #endif
> }
> break;
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> index 663aa916e3ca..1e61f66437d2 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
> @@ -1602,4 +1602,8 @@ module_exit(atomisp_exit);
> MODULE_AUTHOR("Wen Wang <wen.w.w...@intel.com>");
> MODULE_AUTHOR("Xiaolin Zhang <xiaolin.zh...@intel.com>");
> MODULE_LICENSE("GPL");
> -MODULE_DESCRIPTION("Intel ATOM Platform ISP Driver");
> +#if defined(ISP2400) || defined(ISP2400B0)
> +MODULE_DESCRIPTION("Intel ATOM Platform ISP Driver 2400");
> +#elif defined(ISP2401)
> +MODULE_DESCRIPTION("Intel ATOM Platform ISP Driver 2401");
> +#endif
> diff --git
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_acc_types.h
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_acc_types.h
> index a2a1873aca83..3bcbd0fa0637 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_acc_types.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/ia_css_acc_types.h
> @@ -222,6 +222,7 @@ struct ia_css_binary_info {
> uint8_t luma_only;
> uint8_t input_yuv;
> uint8_t input_raw;
> + uint8_t lace_stats;
> #endif
> uint8_t reduced_pipe;
> uint8_t vf_veceven;
> diff --git
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
>
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
> index 0fa7cb2423d8..6f6e30cb7550 100644
> ---
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
> +++
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
> @@ -49,9 +49,6 @@
> #include "assert_support.h"
> #include "print_support.h"
> #include "string_support.h"
> -#ifdef ISP2401
> -#include "ia_css_system_ctrl.h"
> -#endif
>
> #include "fifo_monitor.h"
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> index e882b5596813..1d2e56e74e01 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c
> @@ -1496,7 +1496,7 @@ sh_css_invalidate_shading_tables(struct ia_css_stream
> *stream)
> "sh_css_invalidate_shading_tables() leave: return_void\n");
> }
>
> -#ifndef ISP2401
> +#if 1 /* was ndef ISP2401 but this function is used by ISP2401 on line 1758
> */
Just delete the #if. (I haven't looked at the code). These comments
should probably be in the changelog. You probably want to break this
patch up into several patches and add a little changelog for each
explaining what's going on.
Extra curly brace. Bad indenting. Add a missing struct member.
Delete references to header file that doesn't exist. Delete defines.
regards,
dan carpenter
