diff mbox series

rcar-vin: Clean up correct notifier in error path

Message ID 20190702012458.31828-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive)
State New, archived
Headers show
Series rcar-vin: Clean up correct notifier in error path | expand

Commit Message

Niklas Söderlund July 2, 2019, 1:24 a.m. UTC 
When adding the v4l2_async_notifier_cleanup() callas the wrong notifier
was cleaned up if the parallel notifier registration failed. Fix this by
cleaning up the correct one.

Fixes: 9863bc8695bc36e3 ("media: rcar-vin: Cleanup notifier in error path")
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sergei Shtylyov July 2, 2019, 8:32 a.m. UTC | #1 
Hello!

On 02.07.2019 4:24, Niklas Söderlund wrote:

> When adding the v4l2_async_notifier_cleanup() callas the wrong notifier

    Callas? :-)

> was cleaned up if the parallel notifier registration failed. Fix this by
> cleaning up the correct one.
> 
> Fixes: 9863bc8695bc36e3 ("media: rcar-vin: Cleanup notifier in error path")
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei
Jacopo Mondi July 2, 2019, 8:53 a.m. UTC | #2 
Hi Niklas,

On Tue, Jul 02, 2019 at 03:24:58AM +0200, Niklas Söderlund wrote:
> When adding the v4l2_async_notifier_cleanup() callas the wrong notifier

I would re-word this by removing "When adding" (and fix the 'callas'
Sergei noticed here) with something along the lines of:

"The parallel input initialization error path cleans up the wrong
async notifier, leaking the resources associated with the one whose
registration actually failed.

Fix this by cleaning up the correct notifier in the parallel input
registration error handling."

What do you think?

> was cleaned up if the parallel notifier registration failed. Fix this by
> cleaning up the correct one.
>
> Fixes: 9863bc8695bc36e3 ("media: rcar-vin: Cleanup notifier in error path")
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The patch itself is good! Nice catch!

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

> ---
>  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 64f9cf790445d14e..a6efe1a8099a6ae6 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -633,7 +633,7 @@ static int rvin_parallel_init(struct rvin_dev *vin)
>  	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
>  	if (ret < 0) {
>  		vin_err(vin, "Notifier registration failed\n");
> -		v4l2_async_notifier_cleanup(&vin->group->notifier);
> +		v4l2_async_notifier_cleanup(&vin->notifier);
>  		return ret;
>  	}
>
> --
> 2.21.0
>
Geert Uytterhoeven July 2, 2019, 11:42 a.m. UTC | #3 
Hi Niklas,

On Tue, Jul 2, 2019 at 3:25 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> When adding the v4l2_async_notifier_cleanup() callas the wrong notifier
> was cleaned up if the parallel notifier registration failed. Fix this by
> cleaning up the correct one.
>
> Fixes: 9863bc8695bc36e3 ("media: rcar-vin: Cleanup notifier in error path")
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Thank you, this fixes the following boot regression on koelsch:

Unable to handle kernel NULL pointer dereference at virtual address 000001d8
pgd = (ptrval)
[000001d8] *pgd=00000000
Internal error: Oops: 5 [#1] SMP ARM
CPU: 1 PID: 1 Comm: swapper/0 Not tainted
5.2.0-rc7-shmobile-07178-g1a639e48dea73e76 #280
Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
PC is at __v4l2_async_notifier_cleanup+0xc/0x70
LR is at v4l2_async_notifier_cleanup+0x1c/0x2c
pc : [<c0556580>]    lr : [<c0556c78>]    psr: 20000013
sp : eb08ddc8  ip : 00000000  fp : 00000018
r10: 00000000  r9 : eb1a5410  r8 : c0c04c08
r7 : eb1a5400  r6 : ffffffea  r5 : 000001c8  r4 : c0c61854
r3 : eb08b740  r2 : 00000000  r1 : 00000000  r0 : 000001c8
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 4000406a  DAC: 00000051
Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
Stack: (0xeb08ddc8 to 0xeb08e000)
ddc0:                   c0c61854 000001c8 ffffffea eb1a5400 c0c04c08 c0556c78
dde0: ea940040 ffffffea ffffffea c057661c c0575c9c c0ff7148 00000001 0286d9e6
de00: 00000000 eb1a5410 00000000 c0c637bc c0cbe13c 00000000 c0c637bc 00000000
de20: 00000018 c04408b4 eb1a5410 00000000 c0cbe24c c043ef0c eb1a5410 c0c637bc
de40: c0c637bc c043f4f8 00000000 c0b54844 c0b6f4b0 c043f354 c0c637bc eb1a5410
de60: 00000000 00000000 eb1a5410 c0c637bc c043f4f8 00000000 c0b54844 c043f4e0
de80: 00000000 eb1a5410 c0c637bc c043f5a4 eb1a5410 c0c04c08 c0c637bc c043d6c4
dea0: c0b6f4b0 eb150f58 eb1aefb4 0286d9e6 00000000 c0c637bc ea956f80 00000000
dec0: c0c51c50 c043ddf4 c09c6620 c09c6621 00000072 c0c637bc c0c7c840 c0c04c08
dee0: c0b34c88 c043ffc0 ffffe000 c0c7c840 c0c04c08 c0b01050 00000000 c09fa420
df00: 00000000 c013ad00 00000000 c0b0078c c09f93a4 000000d4 00000006 00000006
df20: 00000000 c09fa434 000000d3 c09fa434 c0b54820 ebfffbe9 ebfffbf1 0286d9e6
df40: 00000000 0286d9e6 c0c7c840 00000007 c0c7c840 c0c83f00 c0b5483c c0c83f00
df60: c0b54844 c0b013ec 00000006 00000006 00000000 c0b0078c c074a3d0 000000d4
df80: 00000000 00000000 c074a3d0 00000000 00000000 00000000 00000000 00000000
dfa0: 00000000 c074a3d8 00000000 c01010e8 00000000 00000000 00000000 00000000
dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[<c0556580>] (__v4l2_async_notifier_cleanup) from [<c0556c78>]
(v4l2_async_notifier_cleanup+0x1c/0x2c)
[<c0556c78>] (v4l2_async_notifier_cleanup) from [<c057661c>]
(rcar_vin_probe+0x4f8/0x5a4)
[<c057661c>] (rcar_vin_probe) from [<c04408b4>] (platform_drv_probe+0x48/0x94)
[<c04408b4>] (platform_drv_probe) from [<c043ef0c>] (really_probe+0x1f0/0x2b8)
[<c043ef0c>] (really_probe) from [<c043f354>] (driver_probe_device+0x140/0x15c)
[<c043f354>] (driver_probe_device) from [<c043f4e0>]
(device_driver_attach+0x44/0x5c)
[<c043f4e0>] (device_driver_attach) from [<c043f5a4>]
(__driver_attach+0xac/0xb4)
[<c043f5a4>] (__driver_attach) from [<c043d6c4>] (bus_for_each_dev+0x64/0xa0)
[<c043d6c4>] (bus_for_each_dev) from [<c043ddf4>] (bus_add_driver+0x148/0x1a8)
[<c043ddf4>] (bus_add_driver) from [<c043ffc0>] (driver_register+0xac/0xf0)
[<c043ffc0>] (driver_register) from [<c0b01050>] (do_one_initcall+0xa8/0x1d4)
[<c0b01050>] (do_one_initcall) from [<c0b013ec>]
(kernel_init_freeable+0x270/0x2cc)
[<c0b013ec>] (kernel_init_freeable) from [<c074a3d8>] (kernel_init+0x8/0x10c)
[<c074a3d8>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
Niklas Söderlund July 2, 2019, noon UTC | #4 
Hi Geert,

On 2019-07-02 13:42:44 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Tue, Jul 2, 2019 at 3:25 AM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > When adding the v4l2_async_notifier_cleanup() callas the wrong notifier
> > was cleaned up if the parallel notifier registration failed. Fix this by
> > cleaning up the correct one.
> >
> > Fixes: 9863bc8695bc36e3 ("media: rcar-vin: Cleanup notifier in error path")
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Thank you, this fixes the following boot regression on koelsch:

Indeed it does, but this patch only address the symptom and fixes the 
problem in the error path, VIN is still broken on Gen2 with this patch.  
The patch which fixes the regression is,

    https://www.mail-archive.com/linux-media@vger.kernel.org/msg148213.html

> 
> Unable to handle kernel NULL pointer dereference at virtual address 000001d8
> pgd = (ptrval)
> [000001d8] *pgd=00000000
> Internal error: Oops: 5 [#1] SMP ARM
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted
> 5.2.0-rc7-shmobile-07178-g1a639e48dea73e76 #280
> Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> PC is at __v4l2_async_notifier_cleanup+0xc/0x70
> LR is at v4l2_async_notifier_cleanup+0x1c/0x2c
> pc : [<c0556580>]    lr : [<c0556c78>]    psr: 20000013
> sp : eb08ddc8  ip : 00000000  fp : 00000018
> r10: 00000000  r9 : eb1a5410  r8 : c0c04c08
> r7 : eb1a5400  r6 : ffffffea  r5 : 000001c8  r4 : c0c61854
> r3 : eb08b740  r2 : 00000000  r1 : 00000000  r0 : 000001c8
> Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 4000406a  DAC: 00000051
> Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
> Stack: (0xeb08ddc8 to 0xeb08e000)
> ddc0:                   c0c61854 000001c8 ffffffea eb1a5400 c0c04c08 c0556c78
> dde0: ea940040 ffffffea ffffffea c057661c c0575c9c c0ff7148 00000001 0286d9e6
> de00: 00000000 eb1a5410 00000000 c0c637bc c0cbe13c 00000000 c0c637bc 00000000
> de20: 00000018 c04408b4 eb1a5410 00000000 c0cbe24c c043ef0c eb1a5410 c0c637bc
> de40: c0c637bc c043f4f8 00000000 c0b54844 c0b6f4b0 c043f354 c0c637bc eb1a5410
> de60: 00000000 00000000 eb1a5410 c0c637bc c043f4f8 00000000 c0b54844 c043f4e0
> de80: 00000000 eb1a5410 c0c637bc c043f5a4 eb1a5410 c0c04c08 c0c637bc c043d6c4
> dea0: c0b6f4b0 eb150f58 eb1aefb4 0286d9e6 00000000 c0c637bc ea956f80 00000000
> dec0: c0c51c50 c043ddf4 c09c6620 c09c6621 00000072 c0c637bc c0c7c840 c0c04c08
> dee0: c0b34c88 c043ffc0 ffffe000 c0c7c840 c0c04c08 c0b01050 00000000 c09fa420
> df00: 00000000 c013ad00 00000000 c0b0078c c09f93a4 000000d4 00000006 00000006
> df20: 00000000 c09fa434 000000d3 c09fa434 c0b54820 ebfffbe9 ebfffbf1 0286d9e6
> df40: 00000000 0286d9e6 c0c7c840 00000007 c0c7c840 c0c83f00 c0b5483c c0c83f00
> df60: c0b54844 c0b013ec 00000006 00000006 00000000 c0b0078c c074a3d0 000000d4
> df80: 00000000 00000000 c074a3d0 00000000 00000000 00000000 00000000 00000000
> dfa0: 00000000 c074a3d8 00000000 c01010e8 00000000 00000000 00000000 00000000
> dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [<c0556580>] (__v4l2_async_notifier_cleanup) from [<c0556c78>]
> (v4l2_async_notifier_cleanup+0x1c/0x2c)
> [<c0556c78>] (v4l2_async_notifier_cleanup) from [<c057661c>]
> (rcar_vin_probe+0x4f8/0x5a4)
> [<c057661c>] (rcar_vin_probe) from [<c04408b4>] (platform_drv_probe+0x48/0x94)
> [<c04408b4>] (platform_drv_probe) from [<c043ef0c>] (really_probe+0x1f0/0x2b8)
> [<c043ef0c>] (really_probe) from [<c043f354>] (driver_probe_device+0x140/0x15c)
> [<c043f354>] (driver_probe_device) from [<c043f4e0>]
> (device_driver_attach+0x44/0x5c)
> [<c043f4e0>] (device_driver_attach) from [<c043f5a4>]
> (__driver_attach+0xac/0xb4)
> [<c043f5a4>] (__driver_attach) from [<c043d6c4>] (bus_for_each_dev+0x64/0xa0)
> [<c043d6c4>] (bus_for_each_dev) from [<c043ddf4>] (bus_add_driver+0x148/0x1a8)
> [<c043ddf4>] (bus_add_driver) from [<c043ffc0>] (driver_register+0xac/0xf0)
> [<c043ffc0>] (driver_register) from [<c0b01050>] (do_one_initcall+0xa8/0x1d4)
> [<c0b01050>] (do_one_initcall) from [<c0b013ec>]
> (kernel_init_freeable+0x270/0x2cc)
> [<c0b013ec>] (kernel_init_freeable) from [<c074a3d8>] (kernel_init+0x8/0x10c)
> [<c074a3d8>] (kernel_init) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
> 
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
diff mbox series

Patch

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 64f9cf790445d14e..a6efe1a8099a6ae6 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -633,7 +633,7 @@  static int rvin_parallel_init(struct rvin_dev *vin)
 	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
 	if (ret < 0) {
 		vin_err(vin, "Notifier registration failed\n");
-		v4l2_async_notifier_cleanup(&vin->group->notifier);
+		v4l2_async_notifier_cleanup(&vin->notifier);
 		return ret;
 	}