[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
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
Re: Unicode characters in commit messages?
On Wed, Oct 25, 2017 at 09:39:39AM +0200, SF Markus Elfring wrote: > >> Would you like to support Unicode characters there? > > > > Multiple people have answered this question already and I have answered > > it multiple times. > > I found the corresponding feedback not sufficient so far to reach > a final consensus. Markus, you really have to listen better or you're going to get banned from more subsystems. These long email threads are a waste of time when we already answered your questions completely and over and over. The feedback was clear. regards, dan carpenter
Re: drm/rcar-du: Adjust 14 checks for null pointers
On Wed, Oct 25, 2017 at 08:44:38AM +0200, SF Markus Elfring wrote: > >> The script “checkpatch.pl” pointed information out like the following. > >> > >> Comparison to NULL could be written !… > >> > >> Thus fix the affected source code places. > >> > > > > This one is fine > > This kind of feedback is nice. > > > > except for the commit message. > > Would you like to support Unicode characters there? > Multiple people have answered this question already and I have answered it multiple times. regards, dan carpenter
Re: [PATCH 2/2] drm/rcar-du: Adjust 14 checks for null pointers
On Tue, Oct 24, 2017 at 06:02:30PM +0200, SF Markus Elfring wrote: > From: Markus Elfring <elfr...@users.sourceforge.net> > Date: Tue, 24 Oct 2017 17:47:37 +0200 > 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. > This one is fine except for the commit message. regards, dan carpenter
Re: [PATCH 1/2] drm/rcar-du: Use common error handling code in rcar_du_encoders_init()
This is a subtle thing but my preference on this type of thing is the
way the original code is written. I'm still slightly annoyed that
someone once made me rewrite a patch using the new style... But anyways
I guess other people sometimes disagree with me.
Unwinding is for when you allocate five things in a row. You have
to undo four if the last allocation fails. But say you have to take a
lock part way through and drop it before the end of the function. The
lock/unlock is not part of the list of five resources that you want the
function to take so it doesn't belong in the unwind code.
If you add the lock/unlock to the unwind code, then it makes things a
bit tricky because then you have to do funny things like:
free_four:
free(four);
goto free_three: <-- little bunny hop
unlock: <-- less useful label
unlock();
free_three:
free_three();
free_two:
free(two);
free_one:
free(one);
return ret;
It's better to just do the unlocking before the goto. That way the
lock and unlock are close together.
if (!four) {
unlock();
ret = -EFAIL;
goto free_three;
}
Of course, having a big unlock label makes sense if you take a lock at
the start of the function and need to drop it at the end. But in this
case we are taking a lock then dropping it, and taking the next, then
dropping it and so on. It's a different situation.
regards,
dan carpenter
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Mon, Jul 17, 2017 at 04:26:23PM +0200, Arnd Bergmann wrote: > On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverk...@xs4all.nl> wrote: > > On 14/07/17 11:36, Arnd Bergmann wrote: > >> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file > >> *file, void *fh, > >>* digitizer/slicer. Note, cx18_av_vbi() wipes the passed in > >>* fmt->fmt.sliced under valid calling conditions > >>*/ > >> - if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, >fmt.sliced)) > >> - return -EINVAL; > >> + ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, > >> >fmt.sliced); > >> + if (ret) > >> + return ret; > > > > Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' > > wouldn't > > break something. > > I think Dan was recommending the opposite here, if I understood you > both correctly: > he said we should propagate the error code unless we know it's wrong, while > you > want to keep the current behavior to avoid introducing changes ;-) > I don't know the subsystem rules at all, so don't listen to me. regards, dan carpenter
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Fri, Jul 14, 2017 at 03:55:26PM +0300, Dan Carpenter wrote: > I don't agree with it as a static analysis dev... What I mean is if it's a macro that returns -ENODEV or a function that returns -ENODEV, they should both be treated the same. The other warnings this check prints are quite clever. regards, dan carpenter
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
On Fri, Jul 14, 2017 at 11:36:56AM +0200, Arnd Bergmann wrote:
> @@ -1158,7 +1158,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
>*/
> fmt_src.pad = pad->index;
> fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> - if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, _src)) {
> + ret = v4l2_subdev_call(sensor, pad, get_fmt, NULL, _src);
> + if (!ret) {
> fmt_info = omap3isp_video_format_info(fmt_src.format.code);
> depth_in = fmt_info->width;
> }
Is the original code buggy?
media/platform/omap3isp/ispccdc.c
1156 /* Compute the lane shifter shift value and enable the bridge
when the
1157 * input format is a non-BT.656 YUV variant.
1158 */
1159 fmt_src.pad = pad->index;
1160 fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
1161 if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, _src)) {
1162 fmt_info =
omap3isp_video_format_info(fmt_src.format.code);
1163 depth_in = fmt_info->width;
1164 }
If v4l2_subdev_call() then depth_in is zero.
1165
1166 fmt_info = omap3isp_video_format_info(format->code);
1167 depth_out = fmt_info->width;
1168 shift = depth_in - depth_out;
How do we know that this subtraction doesn't set "shift" to a very high
positive?
1169
1170 if (ccdc->bt656)
1171 bridge = ISPCTRL_PAR_BRIDGE_DISABLE;
1172 else if (fmt_info->code == MEDIA_BUS_FMT_YUYV8_2X8)
1173 bridge = ISPCTRL_PAR_BRIDGE_LENDIAN;
1174 else if (fmt_info->code == MEDIA_BUS_FMT_UYVY8_2X8)
1175 bridge = ISPCTRL_PAR_BRIDGE_BENDIAN;
1176 else
1177 bridge = ISPCTRL_PAR_BRIDGE_DISABLE;
1178
1179 omap3isp_configure_bridge(isp, ccdc->input, parcfg, shift,
bridge);
regards,
dan carpenter
Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
Changing:
- if (!frob()) {
+ if (frob() == 0) {
is a totally pointless change. They're both bad, because they're doing
success testing instead of failure testing, but probably the second one
is slightly worse.
This warning seems dumb. I can't imagine it has even a 10% success rate
at finding real bugs. Just disable it.
Changing the code to propagate error codes, is the right thing of course
so long as it doesn't introduce bugs.
regards,
dan carpenter
[PATCH] drm: shmobile: checking for NULL instead if IS_ERR()
We changed from ioremap_nocache() to devm_ioremap_resource() so the
check needs to be changed from checking for NULL to checking for error
pointers.
Fixes: 16ad3b2ce8dd ("drm/shmobile: Use devm_* managed functions")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
index 800d1d2c435d..6bd777a3dc17 100644
--- a/drivers/gpu/drm/shmobile/shmob_drm_drv.c
+++ b/drivers/gpu/drm/shmobile/shmob_drm_drv.c
@@ -235,8 +235,8 @@ static int shmob_drm_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
sdev->mmio = devm_ioremap_resource(>dev, res);
- if (sdev->mmio == NULL)
- return -ENOMEM;
+ if (IS_ERR(sdev->mmio))
+ return PTR_ERR(sdev->mmio);
ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
if (ret < 0)
Re: [bug report] mmc: tmio-mmc: add support for 32bit data port
On Thu, Jun 29, 2017 at 04:02:11PM +, Chris Brandt wrote: > Personally, I think a buffer of raw data should be a 'u8 *', not a > 'unsigned short *'. That works, or you could make it a void pointer. (void *)foo + x and (u8 *)foo + x are equivalent in kernel code. > And, the parameter name "count" should really be something like "size" > or "length" (but that is still confusing because the passed array 'buf' > was designated as an array of "unsigned short *", so what would 'size' > really mean??) > > Anyway, that is the API that's in place today, so that's what I needed > to work with. You can change it. This is the kernel so you can change anything if it makes sense. > > > > > >417 else > >418 sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, > > (u32 *)buf, > >419 count >> 2); > >420 > >421 /* if count was multiple of 4 */ > >422 if (!(count & 0x3)) > >423 return; > >424 > >425 buf8 = (u8 *)(buf + (count >> 2)); > > > > > > And this looks like the bug... buf is a u16 pointer but we're adding > > "count / sizeof(u32)" when we should be adding "count / (sizeof(*buf))". > > I agree this math does not work out to what I wanted because of the > differences in data types. > > Since I simply wanted to get a pointer to where I left off in the > buffer, maybe the most clear thing to do is just cast buf and do simple math: > >buf8 = (u8 *)buf + (count & ~3); > > > Do you agree? Sure. Unless you want to round up? regards, dan carpenter
[PATCH] drm: rcar-du: remove an unneeded NULL check
"params" can't be NULL here. The next lines assume that we either
hit the break statement of "params->mpixelclock == ~0UL". The
inconsistent NULL checking makes static checkers complain. I've just
removed the test.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
index 7539626b8ebd..dc85b53d58ef 100644
--- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
+++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
@@ -45,7 +45,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
{
const struct rcar_hdmi_phy_params *params = rcar_hdmi_phy_params;
- for (; params && params->mpixelclock != ~0UL; ++params) {
+ for (; params->mpixelclock != ~0UL; ++params) {
if (mpixelclock <= params->mpixelclock)
break;
}
Re: [PATCH 0/4] ASoC: tidyup return method from probe()
Thanks! regards, dan carpenter
[PATCH net] ravb: Double free on error in ravb_start_xmit()
If skb_put_padto() fails then it frees the skb. I shifted that code
up a bit to make my error handling a little simpler.
Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/net/ethernet/renesas/ravb_main.c
b/drivers/net/ethernet/renesas/ravb_main.c
index 8cfc4a54f2dc..3cd7989c007d 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1516,11 +1516,12 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb,
struct net_device *ndev)
spin_unlock_irqrestore(>lock, flags);
return NETDEV_TX_BUSY;
}
- entry = priv->cur_tx[q] % (priv->num_tx_ring[q] * NUM_TX_DESC);
- priv->tx_skb[q][entry / NUM_TX_DESC] = skb;
if (skb_put_padto(skb, ETH_ZLEN))
- goto drop;
+ goto exit;
+
+ entry = priv->cur_tx[q] % (priv->num_tx_ring[q] * NUM_TX_DESC);
+ priv->tx_skb[q][entry / NUM_TX_DESC] = skb;
buffer = PTR_ALIGN(priv->tx_align[q], DPTR_ALIGN) +
entry / NUM_TX_DESC * DPTR_ALIGN;
[patch] net: renesas: ravb: unintialized return value
We want to set the other "err" variable here so that we can return it
later. My version of GCC misses this issue but I caught it with a
static checker.
Fixes: 9f70eb339f52 ("net: ethernet: renesas: ravb: fix fixed-link phydev
leaks")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
Applies to the net tree for 4.10.
diff --git a/drivers/net/ethernet/renesas/ravb_main.c
b/drivers/net/ethernet/renesas/ravb_main.c
index 2c0357c..92d7692 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1016,8 +1016,6 @@ static int ravb_phy_init(struct net_device *ndev)
* at this time.
*/
if (priv->chip_id == RCAR_GEN3) {
- int err;
-
err = phy_set_max_speed(phydev, SPEED_100);
if (err) {
netdev_err(ndev, "failed to limit PHY to 100Mbit/s\n");
