| Message ID | 1493057666-27961-1-git-send-email-kbingham@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Kieran, Thanks for your patch. On 2017-04-24 19:14:26 +0100, Kieran Bingham wrote: > From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > The unbind function dereferences the subdev->dev node to obtain the > of_node. In error paths, the subdev->dev can be set to NULL, whilst the > correct reference to the of_node is available as subdev->of_node. > > Correct the dereferencing, and move the variable outside of the loop as > it is constant against the subdev, and not initialised per CSI, for both > the bind and unbind functions > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index 48557628e76d..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 */ This code is already present in upstream. Could you break this out in a separate patch and resubmit it? > > ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE); > @@ -738,12 +738,11 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier, > struct v4l2_async_subdev *asd) > { > struct rvin_dev *vin = notifier_to_vin(notifier); > + struct device_node *del = subdev->of_node; > unsigned int i; > > mutex_lock(&vin->group->lock); > for (i = 0; i < RVIN_CSI_MAX; i++) { > - struct device_node *del = subdev->dev->of_node; > - > if (vin->group->bridge[i].asd.match.of.node == del) { > vin_dbg(vin, "Unbind bridge %s\n", subdev->name); > vin->group->bridge[i].subdev = NULL; > @@ -768,13 +767,13 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier, > struct v4l2_async_subdev *asd) > { > struct rvin_dev *vin = notifier_to_vin(notifier); > + struct device_node *new = subdev->of_node; > unsigned int i; > > v4l2_set_subdev_hostdata(subdev, vin); > > mutex_lock(&vin->group->lock); > for (i = 0; i < RVIN_CSI_MAX; i++) { > - struct device_node *new = subdev->dev->of_node; > > if (vin->group->bridge[i].asd.match.of.node == new) { > vin_dbg(vin, "Bound bridge %s\n", subdev->name); And I will squash these fixes in to the next version of my 'Gen3 with media controller support' series since that is not yet picked up. Is that OK with you? > -- > 2.7.4 >
Hi Niklas, On 25/04/17 15:30, Niklas Söderlund wrote: > Hi Kieran, > > Thanks for your patch. > > On 2017-04-24 19:14:26 +0100, Kieran Bingham wrote: >> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> >> The unbind function dereferences the subdev->dev node to obtain the >> of_node. In error paths, the subdev->dev can be set to NULL, whilst the >> correct reference to the of_node is available as subdev->of_node. >> >> Correct the dereferencing, and move the variable outside of the loop as >> it is constant against the subdev, and not initialised per CSI, for both >> the bind and unbind functions >> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> --- >> drivers/media/platform/rcar-vin/rcar-core.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c >> index 48557628e76d..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 */ > > This code is already present in upstream. Could you break this out in a > separate patch and resubmit it? Sure, no problem. >> >> ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE); >> @@ -738,12 +738,11 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier, >> struct v4l2_async_subdev *asd) >> { >> struct rvin_dev *vin = notifier_to_vin(notifier); >> + struct device_node *del = subdev->of_node; >> unsigned int i; >> >> mutex_lock(&vin->group->lock); >> for (i = 0; i < RVIN_CSI_MAX; i++) { >> - struct device_node *del = subdev->dev->of_node; >> - >> if (vin->group->bridge[i].asd.match.of.node == del) { >> vin_dbg(vin, "Unbind bridge %s\n", subdev->name); >> vin->group->bridge[i].subdev = NULL; >> @@ -768,13 +767,13 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier, >> struct v4l2_async_subdev *asd) >> { >> struct rvin_dev *vin = notifier_to_vin(notifier); >> + struct device_node *new = subdev->of_node; >> unsigned int i; >> >> v4l2_set_subdev_hostdata(subdev, vin); >> >> mutex_lock(&vin->group->lock); >> for (i = 0; i < RVIN_CSI_MAX; i++) { >> - struct device_node *new = subdev->dev->of_node; >> >> if (vin->group->bridge[i].asd.match.of.node == new) { >> vin_dbg(vin, "Bound bridge %s\n", subdev->name); > > And I will squash these fixes in to the next version of my 'Gen3 with > media controller support' series since that is not yet picked up. Is > that OK with you? Yes, squashing this in seems appropriate. Regards Kieran >> -- >> 2.7.4 >> >
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index 48557628e76d..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); @@ -738,12 +738,11 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd) { struct rvin_dev *vin = notifier_to_vin(notifier); + struct device_node *del = subdev->of_node; unsigned int i; mutex_lock(&vin->group->lock); for (i = 0; i < RVIN_CSI_MAX; i++) { - struct device_node *del = subdev->dev->of_node; - if (vin->group->bridge[i].asd.match.of.node == del) { vin_dbg(vin, "Unbind bridge %s\n", subdev->name); vin->group->bridge[i].subdev = NULL; @@ -768,13 +767,13 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd) { struct rvin_dev *vin = notifier_to_vin(notifier); + struct device_node *new = subdev->of_node; unsigned int i; v4l2_set_subdev_hostdata(subdev, vin); mutex_lock(&vin->group->lock); for (i = 0; i < RVIN_CSI_MAX; i++) { - struct device_node *new = subdev->dev->of_node; if (vin->group->bridge[i].asd.match.of.node == new) { vin_dbg(vin, "Bound bridge %s\n", subdev->name);