* [PATCH] rcar-vin: Clean up correct notifier in error path
@ 2019-07-02 1:24 Niklas Söderlund
2019-07-02 8:32 ` Sergei Shtylyov
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Niklas Söderlund @ 2019-07-02 1:24 UTC (permalink / raw)
To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Niklas Söderlund
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(-)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] rcar-vin: Clean up correct notifier in error path
2019-07-02 1:24 [PATCH] rcar-vin: Clean up correct notifier in error path Niklas Söderlund
@ 2019-07-02 8:32 ` Sergei Shtylyov
2019-07-02 8:53 ` Jacopo Mondi
2019-07-02 11:42 ` Geert Uytterhoeven
2 siblings, 0 replies; 5+ messages in thread
From: Sergei Shtylyov @ 2019-07-02 8:32 UTC (permalink / raw)
To: Niklas Söderlund, Laurent Pinchart, linux-media; +Cc: linux-renesas-soc
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rcar-vin: Clean up correct notifier in error path
2019-07-02 1:24 [PATCH] rcar-vin: Clean up correct notifier in error path Niklas Söderlund
2019-07-02 8:32 ` Sergei Shtylyov
@ 2019-07-02 8:53 ` Jacopo Mondi
2019-07-02 11:42 ` Geert Uytterhoeven
2 siblings, 0 replies; 5+ messages in thread
From: Jacopo Mondi @ 2019-07-02 8:53 UTC (permalink / raw)
To: Niklas Söderlund; +Cc: Laurent Pinchart, linux-media, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 1718 bytes --]
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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rcar-vin: Clean up correct notifier in error path
2019-07-02 1:24 [PATCH] rcar-vin: Clean up correct notifier in error path Niklas Söderlund
2019-07-02 8:32 ` Sergei Shtylyov
2019-07-02 8:53 ` Jacopo Mondi
@ 2019-07-02 11:42 ` Geert Uytterhoeven
2019-07-02 12:00 ` Niklas Söderlund
2 siblings, 1 reply; 5+ messages in thread
From: Geert Uytterhoeven @ 2019-07-02 11:42 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Laurent Pinchart, Linux Media Mailing List, Linux-Renesas
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
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rcar-vin: Clean up correct notifier in error path
2019-07-02 11:42 ` Geert Uytterhoeven
@ 2019-07-02 12:00 ` Niklas Söderlund
0 siblings, 0 replies; 5+ messages in thread
From: Niklas Söderlund @ 2019-07-02 12:00 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Laurent Pinchart, Linux Media Mailing List, Linux-Renesas
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
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-02 12:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-02 1:24 [PATCH] rcar-vin: Clean up correct notifier in error path Niklas Söderlund
2019-07-02 8:32 ` Sergei Shtylyov
2019-07-02 8:53 ` Jacopo Mondi
2019-07-02 11:42 ` Geert Uytterhoeven
2019-07-02 12:00 ` Niklas Söderlund
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).