Hi Sakari, On 2017-10-02 14:58:10 +0300, Sakari Ailus wrote: > Hi Niklas, > > On 09/30/17 16:17, Niklas Söderlund wrote: > > Hi Sakari, > > > > Thanks for your patch, I like it. Unfortunately it causes issues :-( > > > > I picked the first 7 patches of this series on top of media-next and it > > produce problems when tested on Koelsch with CONFIG_OF_DYNAMIC=y. > > > > 1. It print's 'OF: ERROR: Bad of_node_put() on /video@e6ef0000/port' > > messages during boot. > > Do you have your own patch to fix fwnode_graph_get_port_parent() > applied? I noticed it doesn't seem to be in Rob's tree; let's continue > in the other thread. > > <URL:https://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg117450.html> To produce this issue the fix is not applied. But as I try to describe at the end of my email applying it fixes both issues. So I think this patch is correct (and that is why I Acked it) but my concern is that if it's picked up before the fwnode_graph_get_port_parent() issue is sorted out there will be problems for rcar-vin, and if possible I would like to avoid that. > > > > > OF: ERROR: Bad of_node_put() on /video@e6ef0000/port > > CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.13.0-rc4-00632-gfae12f9c98a8c567 #7 > > Hardware name: Generic R8A7791 (Flattened Device Tree) [snip] > > Could fixing the other issue fix this one as well? > > I'll see how the rest works at my end with CONFIG_OF_DYNAMIC enabled. Yes, as I try to describe bellow, applying the fix for fwnode_graph_get_port_parent() solves both issues :-) What is troublesome for me is that I don't understand why cdev_alloc() would change/corrupt the subdevice pointer if CONFIG_OF_DYNAMIC=y or the fix is _not_ applied. > > > [<c052507c>] (v4l2_async_match_notify) from [<c0525288>] (v4l2_async_notifier_register+0x11c/0x150) > > r7:eb3cda50 r6:ea9e2868 r5:c0a5ff18 r4:eaa56bd8 > > [<c052516c>] (v4l2_async_notifier_register) from [<c05433f0>] (rcar_vin_probe+0x104/0x178) > > r9:eaa56bd8 r8:00000000 r7:eb251a10 r6:eb251a00 r5:00000000 r4:eaa56810 > > [<c05432ec>] (rcar_vin_probe) from [<c0420a3c>] > > (platform_drv_probe+0x58/0xa4) > > r9:00000000 r8:c0a6c504 r7:00000000 r6:c0a6c504 r5:eb251a10 r4:c05432ec > > [<c04209e4>] (platform_drv_probe) from [<c041f320>] (driver_probe_device+0x210/0x2d8) > > r7:00000000 r6:c0ac72d4 r5:c0ac72c8 r4:eb251a10 > > [<c041f110>] (driver_probe_device) from [<c041f46c>] (__driver_attach+0x84/0xb0) > > r10:00000000 r9:c096d224 r8:00000000 r7:c0a50cf0 r6:c0a6c504 r5:eb251a44 > > r4:eb251a10 r3:00000000 > > [<c041f3e8>] (__driver_attach) from [<c041da08>] (bus_for_each_dev+0x88/0x98) > > r7:c0a50cf0 r6:c041f3e8 r5:c0a6c504 r4:00000000 > > [<c041d980>] (bus_for_each_dev) from [<c041f5bc>] (driver_attach+0x20/0x28) > > r6:00000000 r5:eaa55f80 r4:c0a6c504 > > [<c041f59c>] (driver_attach) from [<c041e190>] (bus_add_driver+0x170/0x1e0) > > [<c041e020>] (bus_add_driver) from [<c0420068>] (driver_register+0xa8/0xe8) > > r7:c095883c r6:000000cb r5:ffffe000 r4:c0a6c504 > > [<c041ffc0>] (driver_register) from [<c04214b0>] (__platform_driver_register+0x38/0x4c) > > r5:ffffe000 r4:c0935328 > > [<c0421478>] (__platform_driver_register) from [<c0935340>] (rcar_vin_driver_init+0x18/0x20) > > [<c0935328>] (rcar_vin_driver_init) from [<c0900ecc>] (do_one_initcall+0x12c/0x154) > > [<c0900da0>] (do_one_initcall) from [<c0901080>] (kernel_init_freeable+0x18c/0x1d0) > > r8:c0a8a700 r7:c095883c r6:000000cb r5:c0a8a700 r4:00000007 > > [<c0900ef4>] (kernel_init_freeable) from [<c06eac64>] (kernel_init+0x10/0x110) > > r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c06eac54 r4:00000000 > > [<c06eac54>] (kernel_init) from [<c0106db8>] (ret_from_fork+0x14/0x3c) > > r5:c06eac54 r4:00000000 > > Code: 03e05012 e5803368 0a00000a e5963068 (e593300c) > > ---[ end trace 82aa2a1c6173a5f6 ]--- > > > > > > Oddly enough setting CONFIG_OF_DYNAMIC=n or applying the patch > > '[PATCH v2] device property: preserve usecount for node passed to > > of_fwnode_graph_get_port_parent()' fixes _both_ issues. It obviously > > would fix the 'Bad of_node_put() on ...' messages that it also fixes the > > OOPS is strange, so I did some digging. > > > > The problem is introduced when rcar-vin in its complete callback calls > > v4l2_device_register_subdev_nodes(). Before the call > > vin->digital->subdev pointer is correct but after the call the > > vin->digital->subdev pointer is changed to a for me random value. And > > this is what is causing the OOPS in rvin_v4l2_probe() once it tries to > > operate on the subdevice using v4l2_subdev_call() using this bad > > pointer. > > > > I tried to track down the issue but I can't understand what is causing > > it, but I managed to narrow it down. The callchain is: > > > > - rvin_digital_notify_complete > > - pr_dbg("sd: %p\n", vin->digital->subdev); # prints good pointer > > - v4l2_device_register_subdev_nodes() > > - __video_register_device() > > - cdev_alloc() # Here the pointer gets corrupted > > - pr_dbg("sd: %p\n", vin->digital->subdev); # prints bad pointer > > > > I can't figure out why cdev_alloc() would corrupt it. I can even corrupt > > the pointer by calling cdev_alloc() directly from the rcar-vin driver > > itself. I added to following to the top of the complete callback before > > v4l2_device_register_subdev_nodes() is called. > > > > pr_err("digital: %p sd: %p\n", vin->digital, vin->digital->subdev); > > cdev_alloc(); > > pr_err("digital: %p sd: %p\n", vin->digital, vin->digital->subdev); > > > > And the result is: > > > > [ 2.865306] digital: eb1284c0 sd: ea953068 > > [ 2.869414] digital: eb1284c0 sd: c0a1e6dc > > > > If I set CONFIG_OF_DYNAMIC=n or apply the patch above the result is OK, > > > > [ 1.961142] digital: ea8f8cc0 sd: ea8bac50 > > [ 1.965240] digital: ea8f8cc0 sd: ea8bac50 > > > > I can capture without issues so this patch in it self is good I think. > > So please add > > > > Acked-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx> > > > > However I would like the issue that is revealed by this patch to be > > sorted out before this patch is picked up as it causes problems with > > CONFIG_OF_DYNAMIC=y which is enabled by using the shmobile_defconfig. > > > > On 2017-09-26 01:25:18 +0300, Sakari Ailus wrote: > >> Instead of using a custom driver implementation, use > >> v4l2_async_notifier_parse_fwnode_endpoints() to parse the fwnode endpoints > >> of the device. > >> > >> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > >> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > >> --- > >> drivers/media/platform/rcar-vin/rcar-core.c | 107 +++++++++------------------- > >> drivers/media/platform/rcar-vin/rcar-dma.c | 10 +-- > >> drivers/media/platform/rcar-vin/rcar-v4l2.c | 14 ++-- > >> drivers/media/platform/rcar-vin/rcar-vin.h | 4 +- > >> 4 files changed, 46 insertions(+), 89 deletions(-) > >> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > >> index 142de447aaaa..380288658601 100644 > >> --- a/drivers/media/platform/rcar-vin/rcar-core.c > >> +++ b/drivers/media/platform/rcar-vin/rcar-core.c > >> @@ -21,6 +21,7 @@ > >> #include <linux/platform_device.h> > >> #include <linux/pm_runtime.h> > >> > >> +#include <media/v4l2-async.h> > >> #include <media/v4l2-fwnode.h> > >> > >> #include "rcar-vin.h" > >> @@ -77,14 +78,14 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier) > >> int ret; > >> > >> /* Verify subdevices mbus format */ > >> - if (!rvin_mbus_supported(&vin->digital)) { > >> + if (!rvin_mbus_supported(vin->digital)) { > >> vin_err(vin, "Unsupported media bus format for %s\n", > >> - vin->digital.subdev->name); > >> + vin->digital->subdev->name); > >> return -EINVAL; > >> } > >> > >> vin_dbg(vin, "Found media bus format for %s: %d\n", > >> - vin->digital.subdev->name, vin->digital.code); > >> + vin->digital->subdev->name, vin->digital->code); > >> > >> ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev); > >> if (ret < 0) { > >> @@ -103,7 +104,7 @@ static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier, > >> > >> vin_dbg(vin, "unbind digital subdev %s\n", subdev->name); > >> rvin_v4l2_remove(vin); > >> - vin->digital.subdev = NULL; > >> + vin->digital->subdev = NULL; > >> } > >> > >> static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier, > >> @@ -120,117 +121,71 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier, > >> ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE); > >> if (ret < 0) > >> return ret; > >> - vin->digital.source_pad = ret; > >> + vin->digital->source_pad = ret; > >> > >> ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK); > >> - vin->digital.sink_pad = ret < 0 ? 0 : ret; > >> + vin->digital->sink_pad = ret < 0 ? 0 : ret; > >> > >> - vin->digital.subdev = subdev; > >> + vin->digital->subdev = subdev; > >> > >> vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n", > >> - subdev->name, vin->digital.source_pad, > >> - vin->digital.sink_pad); > >> + subdev->name, vin->digital->source_pad, > >> + vin->digital->sink_pad); > >> > >> return 0; > >> } > >> > >> -static int rvin_digitial_parse_v4l2(struct rvin_dev *vin, > >> - struct device_node *ep, > >> - struct v4l2_mbus_config *mbus_cfg) > >> +static int rvin_digital_parse_v4l2(struct device *dev, > >> + struct v4l2_fwnode_endpoint *vep, > >> + struct v4l2_async_subdev *asd) > >> { > >> - struct v4l2_fwnode_endpoint v4l2_ep; > >> - int ret; > >> + struct rvin_dev *vin = dev_get_drvdata(dev); > >> + struct rvin_graph_entity *rvge = > >> + container_of(asd, struct rvin_graph_entity, asd); > >> > >> - ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep); > >> - if (ret) { > >> - vin_err(vin, "Could not parse v4l2 endpoint\n"); > >> - return -EINVAL; > >> - } > >> + if (vep->base.port || vep->base.id) > >> + return -ENOTCONN; > >> > >> - mbus_cfg->type = v4l2_ep.bus_type; > >> + rvge->mbus_cfg.type = vep->bus_type; > >> > >> - switch (mbus_cfg->type) { > >> + switch (rvge->mbus_cfg.type) { > >> case V4L2_MBUS_PARALLEL: > >> vin_dbg(vin, "Found PARALLEL media bus\n"); > >> - mbus_cfg->flags = v4l2_ep.bus.parallel.flags; > >> + rvge->mbus_cfg.flags = vep->bus.parallel.flags; > >> break; > >> case V4L2_MBUS_BT656: > >> vin_dbg(vin, "Found BT656 media bus\n"); > >> - mbus_cfg->flags = 0; > >> + rvge->mbus_cfg.flags = 0; > >> break; > >> default: > >> vin_err(vin, "Unknown media bus type\n"); > >> return -EINVAL; > >> } > >> > >> - return 0; > >> -} > >> - > >> -static int rvin_digital_graph_parse(struct rvin_dev *vin) > >> -{ > >> - struct device_node *ep, *np; > >> - int ret; > >> - > >> - vin->digital.asd.match.fwnode.fwnode = NULL; > >> - vin->digital.subdev = NULL; > >> - > >> - /* > >> - * Port 0 id 0 is local digital input, try to get it. > >> - * Not all instances can or will have this, that is OK > >> - */ > >> - ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0); > >> - if (!ep) > >> - return 0; > >> - > >> - np = of_graph_get_remote_port_parent(ep); > >> - if (!np) { > >> - vin_err(vin, "No remote parent for digital input\n"); > >> - of_node_put(ep); > >> - return -EINVAL; > >> - } > >> - of_node_put(np); > >> - > >> - ret = rvin_digitial_parse_v4l2(vin, ep, &vin->digital.mbus_cfg); > >> - of_node_put(ep); > >> - if (ret) > >> - return ret; > >> - > >> - vin->digital.asd.match.fwnode.fwnode = of_fwnode_handle(np); > >> - vin->digital.asd.match_type = V4L2_ASYNC_MATCH_FWNODE; > >> + vin->digital = rvge; > >> > >> return 0; > >> } > >> > >> static int rvin_digital_graph_init(struct rvin_dev *vin) > >> { > >> - struct v4l2_async_subdev **subdevs = NULL; > >> int ret; > >> > >> - ret = rvin_digital_graph_parse(vin); > >> + ret = v4l2_async_notifier_parse_fwnode_endpoints( > >> + vin->dev, &vin->notifier, > >> + sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2); > >> if (ret) > >> return ret; > >> > >> - if (!vin->digital.asd.match.fwnode.fwnode) { > >> - vin_dbg(vin, "No digital subdevice found\n"); > >> + if (!vin->digital) > >> return -ENODEV; > >> - } > >> - > >> - /* Register the subdevices notifier. */ > >> - subdevs = devm_kzalloc(vin->dev, sizeof(*subdevs), GFP_KERNEL); > >> - if (subdevs == NULL) > >> - return -ENOMEM; > >> - > >> - subdevs[0] = &vin->digital.asd; > >> > >> vin_dbg(vin, "Found digital subdevice %pOF\n", > >> - to_of_node(subdevs[0]->match.fwnode.fwnode)); > >> + to_of_node(vin->digital->asd.match.fwnode.fwnode)); > >> > >> - vin->notifier.num_subdevs = 1; > >> - vin->notifier.subdevs = subdevs; > >> vin->notifier.bound = rvin_digital_notify_bound; > >> vin->notifier.unbind = rvin_digital_notify_unbind; > >> vin->notifier.complete = rvin_digital_notify_complete; > >> - > >> ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier); > >> if (ret < 0) { > >> vin_err(vin, "Notifier registration failed\n"); > >> @@ -290,6 +245,8 @@ static int rcar_vin_probe(struct platform_device *pdev) > >> if (ret) > >> return ret; > >> > >> + platform_set_drvdata(pdev, vin); > >> + > >> ret = rvin_digital_graph_init(vin); > >> if (ret < 0) > >> goto error; > >> @@ -297,11 +254,10 @@ static int rcar_vin_probe(struct platform_device *pdev) > >> pm_suspend_ignore_children(&pdev->dev, true); > >> pm_runtime_enable(&pdev->dev); > >> > >> - platform_set_drvdata(pdev, vin); > >> - > >> return 0; > >> error: > >> rvin_dma_remove(vin); > >> + v4l2_async_notifier_cleanup(&vin->notifier); > >> > >> return ret; > >> } > >> @@ -313,6 +269,7 @@ static int rcar_vin_remove(struct platform_device *pdev) > >> pm_runtime_disable(&pdev->dev); > >> > >> v4l2_async_notifier_unregister(&vin->notifier); > >> + v4l2_async_notifier_cleanup(&vin->notifier); > >> > >> rvin_dma_remove(vin); > >> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > >> index b136844499f6..23fdff7a7370 100644 > >> --- a/drivers/media/platform/rcar-vin/rcar-dma.c > >> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > >> @@ -183,7 +183,7 @@ static int rvin_setup(struct rvin_dev *vin) > >> /* > >> * Input interface > >> */ > >> - switch (vin->digital.code) { > >> + switch (vin->digital->code) { > >> case MEDIA_BUS_FMT_YUYV8_1X16: > >> /* BT.601/BT.1358 16bit YCbCr422 */ > >> vnmc |= VNMC_INF_YUV16; > >> @@ -191,7 +191,7 @@ static int rvin_setup(struct rvin_dev *vin) > >> break; > >> case MEDIA_BUS_FMT_UYVY8_2X8: > >> /* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */ > >> - vnmc |= vin->digital.mbus_cfg.type == V4L2_MBUS_BT656 ? > >> + vnmc |= vin->digital->mbus_cfg.type == V4L2_MBUS_BT656 ? > >> VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601; > >> input_is_yuv = true; > >> break; > >> @@ -200,7 +200,7 @@ static int rvin_setup(struct rvin_dev *vin) > >> break; > >> case MEDIA_BUS_FMT_UYVY10_2X10: > >> /* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */ > >> - vnmc |= vin->digital.mbus_cfg.type == V4L2_MBUS_BT656 ? > >> + vnmc |= vin->digital->mbus_cfg.type == V4L2_MBUS_BT656 ? > >> VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601; > >> input_is_yuv = true; > >> break; > >> @@ -212,11 +212,11 @@ static int rvin_setup(struct rvin_dev *vin) > >> dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1); > >> > >> /* Hsync Signal Polarity Select */ > >> - if (!(vin->digital.mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)) > >> + if (!(vin->digital->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)) > >> dmr2 |= VNDMR2_HPS; > >> > >> /* Vsync Signal Polarity Select */ > >> - if (!(vin->digital.mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)) > >> + if (!(vin->digital->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)) > >> dmr2 |= VNDMR2_VPS; > >> > >> /* > >> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c > >> index dd37ea811680..b479b882da12 100644 > >> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > >> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > >> @@ -111,7 +111,7 @@ static int rvin_reset_format(struct rvin_dev *vin) > >> struct v4l2_mbus_framefmt *mf = &fmt.format; > >> int ret; > >> > >> - fmt.pad = vin->digital.source_pad; > >> + fmt.pad = vin->digital->source_pad; > >> > >> ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt); > >> if (ret) > >> @@ -172,13 +172,13 @@ static int __rvin_try_format_source(struct rvin_dev *vin, > >> > >> sd = vin_to_source(vin); > >> > >> - v4l2_fill_mbus_format(&format.format, pix, vin->digital.code); > >> + v4l2_fill_mbus_format(&format.format, pix, vin->digital->code); > >> > >> pad_cfg = v4l2_subdev_alloc_pad_config(sd); > >> if (pad_cfg == NULL) > >> return -ENOMEM; > >> > >> - format.pad = vin->digital.source_pad; > >> + format.pad = vin->digital->source_pad; > >> > >> field = pix->field; > >> > >> @@ -555,7 +555,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh, > >> if (timings->pad) > >> return -EINVAL; > >> > >> - timings->pad = vin->digital.sink_pad; > >> + timings->pad = vin->digital->sink_pad; > >> > >> ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings); > >> > >> @@ -607,7 +607,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh, > >> if (cap->pad) > >> return -EINVAL; > >> > >> - cap->pad = vin->digital.sink_pad; > >> + cap->pad = vin->digital->sink_pad; > >> > >> ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap); > >> > >> @@ -625,7 +625,7 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid) > >> if (edid->pad) > >> return -EINVAL; > >> > >> - edid->pad = vin->digital.sink_pad; > >> + edid->pad = vin->digital->sink_pad; > >> > >> ret = v4l2_subdev_call(sd, pad, get_edid, edid); > >> > >> @@ -643,7 +643,7 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid) > >> if (edid->pad) > >> return -EINVAL; > >> > >> - edid->pad = vin->digital.sink_pad; > >> + edid->pad = vin->digital->sink_pad; > >> > >> ret = v4l2_subdev_call(sd, pad, set_edid, edid); > >> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h > >> index 9bfb5a7c4dc4..5382078143fb 100644 > >> --- a/drivers/media/platform/rcar-vin/rcar-vin.h > >> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h > >> @@ -126,7 +126,7 @@ struct rvin_dev { > >> struct v4l2_device v4l2_dev; > >> struct v4l2_ctrl_handler ctrl_handler; > >> struct v4l2_async_notifier notifier; > >> - struct rvin_graph_entity digital; > >> + struct rvin_graph_entity *digital; > >> > >> struct mutex lock; > >> struct vb2_queue queue; > >> @@ -145,7 +145,7 @@ struct rvin_dev { > >> struct v4l2_rect compose; > >> }; > >> > >> -#define vin_to_source(vin) vin->digital.subdev > >> +#define vin_to_source(vin) ((vin)->digital->subdev) > >> > >> /* Debug */ > >> #define vin_dbg(d, fmt, arg...) dev_dbg(d->dev, fmt, ##arg) > >> -- > >> 2.11.0 > >> > > > > > -- > Regards, > > Sakari Ailus > sakari.ailus@xxxxxxxxxxxxxxx -- Regards, Niklas Söderlund -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
