[PATCH] rcar-vin: Use of_nodes as specified by the subdev

[PATCH] rcar-vin: Use of_nodes as specified by the subdev

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The rvin_digital_notify_bound() call dereferences the subdev->dev
pointer to obtain the of_node. On some error paths, this dev node can be
set as NULL. The of_node is mapped into the subdevice structure on
initialisation, so this is a safer source to compare the nodes.

Dereference the of_node from the subdev structure instead of the dev
structure.

Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
	bound")
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 5861ab281150..a530dc388b95 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
 
 	v4l2_set_subdev_hostdata(subdev, vin);
 
-	if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
+	if (vin->digital.asd.match.of.node == subdev->of_node) {
 		/* Find surce and sink pad of remote subdevice */
 
 		ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
-- 
2.7.4

Re: [PATCH] rcar-vin: Use of_nodes as specified by the subdev

[Simon Horman]

Hi Kieran,

On Tue, Apr 25, 2017 at 03:55:00PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The rvin_digital_notify_bound() call dereferences the subdev->dev
> pointer to obtain the of_node. On some error paths, this dev node can be
> set as NULL. The of_node is mapped into the subdevice structure on
> initialisation, so this is a safer source to compare the nodes.
> 
> Dereference the of_node from the subdev structure instead of the dev
> structure.
> 
> Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
> 	bound")
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 5861ab281150..a530dc388b95 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
>  
>  	v4l2_set_subdev_hostdata(subdev, vin);
>  
> -	if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> +	if (vin->digital.asd.match.of.node == subdev->of_node) {
>  		/* Find surce and sink pad of remote subdevice */
>  
>  		ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);

I see two different accesses to subdev->dev->of_node in the version of
rcar-core.c in linux-next. So I'm unsure if the following comment makes
sense in the context of the version you are working on. It is that
I wonder if all accesses to subdev->dev->of_node should be updated.

Re: [PATCH] rcar-vin: Use of_nodes as specified by the subdev

[Kieran Bingham]

Hi Simon,

On 26/04/17 08:23, Simon Horman wrote:
> Hi Kieran,
> 
> On Tue, Apr 25, 2017 at 03:55:00PM +0100, Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> The rvin_digital_notify_bound() call dereferences the subdev->dev
>> pointer to obtain the of_node. On some error paths, this dev node can be
>> set as NULL. The of_node is mapped into the subdevice structure on
>> initialisation, so this is a safer source to compare the nodes.
>>
>> Dereference the of_node from the subdev structure instead of the dev
>> structure.
>>
>> Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
>> 	bound")
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
>> index 5861ab281150..a530dc388b95 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
>> @@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
>>  
>>  	v4l2_set_subdev_hostdata(subdev, vin);
>>  
>> -	if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
>> +	if (vin->digital.asd.match.of.node == subdev->of_node) {
>>  		/* Find surce and sink pad of remote subdevice */
>>  
>>  		ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> 
> I see two different accesses to subdev->dev->of_node in the version of
> rcar-core.c in linux-next. So I'm unsure if the following comment makes
> sense in the context of the version you are working on. It is that
> I wonder if all accesses to subdev->dev->of_node should be updated.

Yes, all uses in rcar-core should be updated, This patch is targeted directly at
mainline, in which only one reference occurs.

I presume(?) the references in linux-next, relate to the previous version of
this patch which I posted as a reply to Niklas' patch series:
 "[PATCH v3 00/27] rcar-vin: Add Gen3 with media controller support"

The first version of this patch (which was titled differently) covered three
uses, but two of them were not yet in mainline.

The 'fixes' for those references are going to be squashed in to Niklas' next
version of his patchset.

--
Regards

Kieran

Re: [PATCH] rcar-vin: Use of_nodes as specified by the subdev

[Simon Horman]

On Wed, Apr 26, 2017 at 08:48:25AM +0100, Kieran Bingham wrote:
> Hi Simon,
> 
> On 26/04/17 08:23, Simon Horman wrote:
> > Hi Kieran,
> > 
> > On Tue, Apr 25, 2017 at 03:55:00PM +0100, Kieran Bingham wrote:
> >> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>
> >> The rvin_digital_notify_bound() call dereferences the subdev->dev
> >> pointer to obtain the of_node. On some error paths, this dev node can be
> >> set as NULL. The of_node is mapped into the subdevice structure on
> >> initialisation, so this is a safer source to compare the nodes.
> >>
> >> Dereference the of_node from the subdev structure instead of the dev
> >> structure.
> >>
> >> Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
> >> 	bound")
> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >> ---
> >>  drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> >> index 5861ab281150..a530dc388b95 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> >> @@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> >>  
> >>  	v4l2_set_subdev_hostdata(subdev, vin);
> >>  
> >> -	if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> >> +	if (vin->digital.asd.match.of.node == subdev->of_node) {
> >>  		/* Find surce and sink pad of remote subdevice */
> >>  
> >>  		ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> > 
> > I see two different accesses to subdev->dev->of_node in the version of
> > rcar-core.c in linux-next. So I'm unsure if the following comment makes
> > sense in the context of the version you are working on. It is that
> > I wonder if all accesses to subdev->dev->of_node should be updated.
> 
> Yes, all uses in rcar-core should be updated, This patch is targeted directly at
> mainline, in which only one reference occurs.
> 
> I presume(?) the references in linux-next, relate to the previous version of
> this patch which I posted as a reply to Niklas' patch series:
>  "[PATCH v3 00/27] rcar-vin: Add Gen3 with media controller support"
> 
> The first version of this patch (which was titled differently) covered three
> uses, but two of them were not yet in mainline.
> 
> The 'fixes' for those references are going to be squashed in to Niklas' next
> version of his patchset.

Understood, sounds good to me.

Re: [PATCH] rcar-vin: Use of_nodes as specified by the subdev

[Niklas Söderlund]

Hi Simon,

Thanks for your feedback.

On 2017-04-26 09:23:20 +0200, Simon Horman wrote:
> Hi Kieran,
> 
> On Tue, Apr 25, 2017 at 03:55:00PM +0100, Kieran Bingham wrote:
> > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > The rvin_digital_notify_bound() call dereferences the subdev->dev
> > pointer to obtain the of_node. On some error paths, this dev node can be
> > set as NULL. The of_node is mapped into the subdevice structure on
> > initialisation, so this is a safer source to compare the nodes.
> > 
> > Dereference the of_node from the subdev structure instead of the dev
> > structure.
> > 
> > Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
> > 	bound")
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 5861ab281150..a530dc388b95 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> >  
> >  	v4l2_set_subdev_hostdata(subdev, vin);
> >  
> > -	if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> > +	if (vin->digital.asd.match.of.node == subdev->of_node) {
> >  		/* Find surce and sink pad of remote subdevice */
> >  
> >  		ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> 
> I see two different accesses to subdev->dev->of_node in the version of
> rcar-core.c in linux-next. So I'm unsure if the following comment makes
> sense in the context of the version you are working on. It is that
> I wonder if all accesses to subdev->dev->of_node should be updated.

Are you sure you checked linux-next and not renesas-drivers? I checked 
next-20170424.

$ git grep "dev->of_node" -- drivers/media/platform/rcar-vin/
drivers/media/platform/rcar-vin/rcar-core.c:107:        if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
drivers/media/platform/rcar-vin/rcar-core.c:161:        ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);

Here vin->dev->of_node is correct and subdev->dev->of_node should be 
fixed by Kieran patch. I'm only asking to be sure I did not miss 
anything. In renesas-drivers the Gen3 patches are included and more 
references to subdev->dev->of_node exists, but as Kieran sates these 
fixes will be squashed into those patches since they are not yet picked 
up.

-- 
Regards,
Niklas Söderlund

Re: [PATCH] rcar-vin: Use of_nodes as specified by the subdev

[Simon Horman]

On Wed, Apr 26, 2017 at 11:00:30AM +0200, Niklas Söderlund wrote:
> Hi Simon,
> 
> Thanks for your feedback.
> 
> On 2017-04-26 09:23:20 +0200, Simon Horman wrote:
> > Hi Kieran,
> > 
> > On Tue, Apr 25, 2017 at 03:55:00PM +0100, Kieran Bingham wrote:
> > > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > 
> > > The rvin_digital_notify_bound() call dereferences the subdev->dev
> > > pointer to obtain the of_node. On some error paths, this dev node can be
> > > set as NULL. The of_node is mapped into the subdevice structure on
> > > initialisation, so this is a safer source to compare the nodes.
> > > 
> > > Dereference the of_node from the subdev structure instead of the dev
> > > structure.
> > > 
> > > Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
> > > 	bound")
> > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index 5861ab281150..a530dc388b95 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> > >  
> > >  	v4l2_set_subdev_hostdata(subdev, vin);
> > >  
> > > -	if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> > > +	if (vin->digital.asd.match.of.node == subdev->of_node) {
> > >  		/* Find surce and sink pad of remote subdevice */
> > >  
> > >  		ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> > 
> > I see two different accesses to subdev->dev->of_node in the version of
> > rcar-core.c in linux-next. So I'm unsure if the following comment makes
> > sense in the context of the version you are working on. It is that
> > I wonder if all accesses to subdev->dev->of_node should be updated.
> 
> Are you sure you checked linux-next and not renesas-drivers? I checked 
> next-20170424.
> 
> $ git grep "dev->of_node" -- drivers/media/platform/rcar-vin/
> drivers/media/platform/rcar-vin/rcar-core.c:107:        if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> drivers/media/platform/rcar-vin/rcar-core.c:161:        ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
> 
> Here vin->dev->of_node is correct and subdev->dev->of_node should be 
> fixed by Kieran patch. I'm only asking to be sure I did not miss 
> anything. In renesas-drivers the Gen3 patches are included and more 
> references to subdev->dev->of_node exists, but as Kieran sates these 
> fixes will be squashed into those patches since they are not yet picked 
> up.

I think we are seeing the same thing, sorry for the noise.

git show next-20170424:drivers/media/platform/rcar-vin/rcar-core.c | grep -A 3 "dev->of_node"
        if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
                vin_dbg(vin, "bound digital subdev %s\n", subdev->name);
                vin->digital.subdev = subdev;
                return 0;
--
        ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
        if (!ep)
                return 0;

Re: [PATCH] rcar-vin: Use of_nodes as specified by the subdev

[Niklas Söderlund]

Hi Kieran,

Thanks for your patch. I took it for a spin on my Koelsch and it worked 
nicely.

Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

On 2017-04-25 15:55:00 +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The rvin_digital_notify_bound() call dereferences the subdev->dev
> pointer to obtain the of_node. On some error paths, this dev node can be
> set as NULL. The of_node is mapped into the subdevice structure on
> initialisation, so this is a safer source to compare the nodes.
> 
> Dereference the of_node from the subdev structure instead of the dev
> structure.
> 
> Fixes: 83fba2c06f19 ("rcar-vin: rework how subdevice is found and
> 	bound")
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 5861ab281150..a530dc388b95 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -469,7 +469,7 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
>  
>  	v4l2_set_subdev_hostdata(subdev, vin);
>  
> -	if (vin->digital.asd.match.of.node == subdev->dev->of_node) {
> +	if (vin->digital.asd.match.of.node == subdev->of_node) {
>  		/* Find surce and sink pad of remote subdevice */
>  
>  		ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund