Re: [PATCH 1/1] dma: actions: Fix lockdep splat for owl-dma
Hi Andreas, On Wed, Apr 29, 2020 at 10:36:01AM +0200, Andreas Färber wrote: > Am 28.04.20 um 20:18 schrieb Manivannan Sadhasivam: > > On Tue, Apr 28, 2020 at 09:11:15PM +0300, Cristian Ciocaltea wrote: > > > On Tue, Apr 28, 2020 at 10:19:21PM +0530, Manivannan Sadhasivam wrote: > > > > On Tue, Apr 28, 2020 at 01:56:12PM +0300, Cristian Ciocaltea wrote: > > > > > When the kernel is build with lockdep support and the owl-dma driver > > > > > is > > > > > used, the following message is shown: > [...] > > > > > The required fix is to use spin_lock_init() on the pchan lock before > > > > > attempting to call any spin_lock_irqsave() in owl_dma_get_pchan(). > > > > > > > > Right, this is a bug. But while looking at the code now, I feel that we > > > > don't > > > > need 'pchan->lock'. The idea was to protect 'pchan->vchan', but I think > > > > 'od->lock' is the better candidate for that since it already protects > > > > it in > > > > 'owl_dma_terminate_pchan'. > > > > > > > > So I'd be happy if you remove the lock from 'pchan' and just directly > > > > use the > > > > one in 'od'. > > > > > > > > Out of curiosity, on which platform you're testing this? > > > > > > Totally agree, I will send a new patch revision as soon as I do some > > > more testing. > > > > Coo[l], thanks! > > > > > I'm currently experimenting on an Actions S500 based board (Roseapple Pi) > > > trying to extend, if possible, the existing mainline support for those > > > SoCs. > > > > Awesome! It's great to see that Actions platform is seeing some attention > > these days :) > > > > > I don't have much progress so far, since I started quite recently > > > and I also lack experience in the kernel development area, but I do my > > > best to come back with more patches once I get a consistent functionality. > > > > No worries. Feel free to reach out to me if you have any questions. There is > > a lot of work to do and for sure it will be a good learning curve. > > > > We do have an IRC channel (##linux-actions) for quick discussions. Fee[l] > > free > > to join! > > Please also CC the linux-actions mailing list on any patches: > > https://lists.infradead.org/mailman/listinfo/linux-actions > > Mani, do you have a 5.7-rc1 tree set up or should I queue patches this > round? I haven't set up the branch. You can do the maintainership duties for this cycle. > It still seems missing in MAINTAINERS, and then there's Matheus' > patches in review. > Yeah, the MAINTAINERS patch has fallen through cracks: [PATCH v2 6/6] MAINTAINERS: Add linux-actions mailing list for Actions Semi I did this as a part of S500 clk series. Feel free to pick it up. Thanks, Mani > Thanks, > Andreas > > -- > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer > HRB 36809 (AG Nürnberg)
Re: [PATCH 1/1] dma: actions: Fix lockdep splat for owl-dma
Am 28.04.20 um 20:18 schrieb Manivannan Sadhasivam: On Tue, Apr 28, 2020 at 09:11:15PM +0300, Cristian Ciocaltea wrote: On Tue, Apr 28, 2020 at 10:19:21PM +0530, Manivannan Sadhasivam wrote: On Tue, Apr 28, 2020 at 01:56:12PM +0300, Cristian Ciocaltea wrote: When the kernel is build with lockdep support and the owl-dma driver is used, the following message is shown: [...] The required fix is to use spin_lock_init() on the pchan lock before attempting to call any spin_lock_irqsave() in owl_dma_get_pchan(). Right, this is a bug. But while looking at the code now, I feel that we don't need 'pchan->lock'. The idea was to protect 'pchan->vchan', but I think 'od->lock' is the better candidate for that since it already protects it in 'owl_dma_terminate_pchan'. So I'd be happy if you remove the lock from 'pchan' and just directly use the one in 'od'. Out of curiosity, on which platform you're testing this? Totally agree, I will send a new patch revision as soon as I do some more testing. Coo[l], thanks! I'm currently experimenting on an Actions S500 based board (Roseapple Pi) trying to extend, if possible, the existing mainline support for those SoCs. Awesome! It's great to see that Actions platform is seeing some attention these days :) I don't have much progress so far, since I started quite recently and I also lack experience in the kernel development area, but I do my best to come back with more patches once I get a consistent functionality. No worries. Feel free to reach out to me if you have any questions. There is a lot of work to do and for sure it will be a good learning curve. We do have an IRC channel (##linux-actions) for quick discussions. Fee[l] free to join! Please also CC the linux-actions mailing list on any patches: https://lists.infradead.org/mailman/listinfo/linux-actions Mani, do you have a 5.7-rc1 tree set up or should I queue patches this round? It still seems missing in MAINTAINERS, and then there's Matheus' patches in review. Thanks, Andreas -- SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer HRB 36809 (AG Nürnberg)
Re: [PATCH 1/1] dma: actions: Fix lockdep splat for owl-dma
On Tue, Apr 28, 2020 at 09:11:15PM +0300, Cristian Ciocaltea wrote: > On Tue, Apr 28, 2020 at 10:19:21PM +0530, Manivannan Sadhasivam wrote: > > Hi, > > > > On Tue, Apr 28, 2020 at 01:56:12PM +0300, Cristian Ciocaltea wrote: > > > When the kernel is build with lockdep support and the owl-dma driver is > > > used, the following message is shown: > > > > > > [2.496939] INFO: trying to register non-static key. > > > [2.501889] the code is fine but needs lockdep annotation. > > > [2.507357] turning off the locking correctness validator. > > > [2.512834] CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.6.3+ #15 > > > [2.519084] Hardware name: Generic DT based system > > > [2.523878] Workqueue: events_freezable mmc_rescan > > > [2.528681] [<801127f0>] (unwind_backtrace) from [<8010da58>] > > > (show_stack+0x10/0x14) > > > [2.536420] [<8010da58>] (show_stack) from [<8080fbe8>] > > > (dump_stack+0xb4/0xe0) > > > [2.543645] [<8080fbe8>] (dump_stack) from [<8017efa4>] > > > (register_lock_class+0x6f0/0x718) > > > [2.551816] [<8017efa4>] (register_lock_class) from [<8017b7d0>] > > > (__lock_acquire+0x78/0x25f0) > > > [2.560330] [<8017b7d0>] (__lock_acquire) from [<8017e5e4>] > > > (lock_acquire+0xd8/0x1f4) > > > [2.568159] [<8017e5e4>] (lock_acquire) from [<80831fb0>] > > > (_raw_spin_lock_irqsave+0x3c/0x50) > > > [2.576589] [<80831fb0>] (_raw_spin_lock_irqsave) from [<8051b5fc>] > > > (owl_dma_issue_pending+0xbc/0x120) > > > [2.585884] [<8051b5fc>] (owl_dma_issue_pending) from [<80668cbc>] > > > (owl_mmc_request+0x1b0/0x390) > > > [2.594655] [<80668cbc>] (owl_mmc_request) from [<80650ce0>] > > > (mmc_start_request+0x94/0xbc) > > > [2.602906] [<80650ce0>] (mmc_start_request) from [<80650ec0>] > > > (mmc_wait_for_req+0x64/0xd0) > > > [2.611245] [<80650ec0>] (mmc_wait_for_req) from [<8065aa10>] > > > (mmc_app_send_scr+0x10c/0x144) > > > [2.619669] [<8065aa10>] (mmc_app_send_scr) from [<80659b3c>] > > > (mmc_sd_setup_card+0x4c/0x318) > > > [2.628092] [<80659b3c>] (mmc_sd_setup_card) from [<80659f0c>] > > > (mmc_sd_init_card+0x104/0x430) > > > [2.636601] [<80659f0c>] (mmc_sd_init_card) from [<8065a3e0>] > > > (mmc_attach_sd+0xcc/0x16c) > > > [2.644678] [<8065a3e0>] (mmc_attach_sd) from [<8065301c>] > > > (mmc_rescan+0x3ac/0x40c) > > > [2.652332] [<8065301c>] (mmc_rescan) from [<80143244>] > > > (process_one_work+0x2d8/0x780) > > > [2.660239] [<80143244>] (process_one_work) from [<80143730>] > > > (worker_thread+0x44/0x598) > > > [2.668323] [<80143730>] (worker_thread) from [<8014b5f8>] > > > (kthread+0x148/0x150) > > > [2.675708] [<8014b5f8>] (kthread) from [<801010b4>] > > > (ret_from_fork+0x14/0x20) > > > [2.682912] Exception stack(0xee8fdfb0 to 0xee8fdff8) > > > [2.687954] dfa0: > > > > > > [2.696118] dfc0: > > > > > > [2.704277] dfe0: 0013 > > > > > > The required fix is to use spin_lock_init() on the pchan lock before > > > attempting to call any spin_lock_irqsave() in owl_dma_get_pchan(). > > > > > > > Right, this is a bug. But while looking at the code now, I feel that we > > don't > > need 'pchan->lock'. The idea was to protect 'pchan->vchan', but I think > > 'od->lock' is the better candidate for that since it already protects it in > > 'owl_dma_terminate_pchan'. > > > > So I'd be happy if you remove the lock from 'pchan' and just directly use > > the > > one in 'od'. > > > > Out of curiosity, on which platform you're testing this? > > > > Thanks, > > Mani > > > > Hi Mani, > > Totally agree, I will send a new patch revision as soon as I do some > more testing. > Coo, thanks! > I'm currently experimenting on an Actions S500 based board (Roseapple Pi) > trying to extend, if possible, the existing mainline support for those > SoCs. Awesome! It's great to see that Actions platform is seeing some attention these days :) > I don't have much progress so far, since I started quite recently > and I also lack experience in the kernel development area, but I do my > best to come back with more patches once I get a consistent functionality. > No worries. Feel free to reach out to me if you have any questions. There is a lot of work to do and for sure it will be a good learning curve. We do have an IRC channel (##linux-actions) for quick discussions. Fee free to join! Thanks, Mani > Thanks a lot for your support, > Cristi > > > > Signed-off-by: Cristian Ciocaltea > > > --- > > > drivers/dma/owl-dma.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c > > > index c683051257fd..d9d0f0488e70 100644 > > > --- a/drivers/dma/owl-dma.c > > > +++ b/drivers/dma/owl-dma.c > > > @@ -1131,6 +1131,7 @@ static int
Re: [PATCH 1/1] dma: actions: Fix lockdep splat for owl-dma
On Tue, Apr 28, 2020 at 10:19:21PM +0530, Manivannan Sadhasivam wrote: > Hi, > > On Tue, Apr 28, 2020 at 01:56:12PM +0300, Cristian Ciocaltea wrote: > > When the kernel is build with lockdep support and the owl-dma driver is > > used, the following message is shown: > > > > [2.496939] INFO: trying to register non-static key. > > [2.501889] the code is fine but needs lockdep annotation. > > [2.507357] turning off the locking correctness validator. > > [2.512834] CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.6.3+ #15 > > [2.519084] Hardware name: Generic DT based system > > [2.523878] Workqueue: events_freezable mmc_rescan > > [2.528681] [<801127f0>] (unwind_backtrace) from [<8010da58>] > > (show_stack+0x10/0x14) > > [2.536420] [<8010da58>] (show_stack) from [<8080fbe8>] > > (dump_stack+0xb4/0xe0) > > [2.543645] [<8080fbe8>] (dump_stack) from [<8017efa4>] > > (register_lock_class+0x6f0/0x718) > > [2.551816] [<8017efa4>] (register_lock_class) from [<8017b7d0>] > > (__lock_acquire+0x78/0x25f0) > > [2.560330] [<8017b7d0>] (__lock_acquire) from [<8017e5e4>] > > (lock_acquire+0xd8/0x1f4) > > [2.568159] [<8017e5e4>] (lock_acquire) from [<80831fb0>] > > (_raw_spin_lock_irqsave+0x3c/0x50) > > [2.576589] [<80831fb0>] (_raw_spin_lock_irqsave) from [<8051b5fc>] > > (owl_dma_issue_pending+0xbc/0x120) > > [2.585884] [<8051b5fc>] (owl_dma_issue_pending) from [<80668cbc>] > > (owl_mmc_request+0x1b0/0x390) > > [2.594655] [<80668cbc>] (owl_mmc_request) from [<80650ce0>] > > (mmc_start_request+0x94/0xbc) > > [2.602906] [<80650ce0>] (mmc_start_request) from [<80650ec0>] > > (mmc_wait_for_req+0x64/0xd0) > > [2.611245] [<80650ec0>] (mmc_wait_for_req) from [<8065aa10>] > > (mmc_app_send_scr+0x10c/0x144) > > [2.619669] [<8065aa10>] (mmc_app_send_scr) from [<80659b3c>] > > (mmc_sd_setup_card+0x4c/0x318) > > [2.628092] [<80659b3c>] (mmc_sd_setup_card) from [<80659f0c>] > > (mmc_sd_init_card+0x104/0x430) > > [2.636601] [<80659f0c>] (mmc_sd_init_card) from [<8065a3e0>] > > (mmc_attach_sd+0xcc/0x16c) > > [2.644678] [<8065a3e0>] (mmc_attach_sd) from [<8065301c>] > > (mmc_rescan+0x3ac/0x40c) > > [2.652332] [<8065301c>] (mmc_rescan) from [<80143244>] > > (process_one_work+0x2d8/0x780) > > [2.660239] [<80143244>] (process_one_work) from [<80143730>] > > (worker_thread+0x44/0x598) > > [2.668323] [<80143730>] (worker_thread) from [<8014b5f8>] > > (kthread+0x148/0x150) > > [2.675708] [<8014b5f8>] (kthread) from [<801010b4>] > > (ret_from_fork+0x14/0x20) > > [2.682912] Exception stack(0xee8fdfb0 to 0xee8fdff8) > > [2.687954] dfa0: > > > > [2.696118] dfc0: > > > > [2.704277] dfe0: 0013 > > > > The required fix is to use spin_lock_init() on the pchan lock before > > attempting to call any spin_lock_irqsave() in owl_dma_get_pchan(). > > > > Right, this is a bug. But while looking at the code now, I feel that we don't > need 'pchan->lock'. The idea was to protect 'pchan->vchan', but I think > 'od->lock' is the better candidate for that since it already protects it in > 'owl_dma_terminate_pchan'. > > So I'd be happy if you remove the lock from 'pchan' and just directly use the > one in 'od'. > > Out of curiosity, on which platform you're testing this? > > Thanks, > Mani > Hi Mani, Totally agree, I will send a new patch revision as soon as I do some more testing. I'm currently experimenting on an Actions S500 based board (Roseapple Pi) trying to extend, if possible, the existing mainline support for those SoCs. I don't have much progress so far, since I started quite recently and I also lack experience in the kernel development area, but I do my best to come back with more patches once I get a consistent functionality. Thanks a lot for your support, Cristi > > Signed-off-by: Cristian Ciocaltea > > --- > > drivers/dma/owl-dma.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c > > index c683051257fd..d9d0f0488e70 100644 > > --- a/drivers/dma/owl-dma.c > > +++ b/drivers/dma/owl-dma.c > > @@ -1131,6 +1131,7 @@ static int owl_dma_probe(struct platform_device *pdev) > > > > pchan->id = i; > > pchan->base = od->base + OWL_DMA_CHAN_BASE(i); > > + spin_lock_init(>lock); > > } > > > > /* Init virtual channel */ > > -- > > 2.26.2 > >
Re: [PATCH 1/1] dma: actions: Fix lockdep splat for owl-dma
Hi, On Tue, Apr 28, 2020 at 01:56:12PM +0300, Cristian Ciocaltea wrote: > When the kernel is build with lockdep support and the owl-dma driver is > used, the following message is shown: > > [2.496939] INFO: trying to register non-static key. > [2.501889] the code is fine but needs lockdep annotation. > [2.507357] turning off the locking correctness validator. > [2.512834] CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.6.3+ #15 > [2.519084] Hardware name: Generic DT based system > [2.523878] Workqueue: events_freezable mmc_rescan > [2.528681] [<801127f0>] (unwind_backtrace) from [<8010da58>] > (show_stack+0x10/0x14) > [2.536420] [<8010da58>] (show_stack) from [<8080fbe8>] > (dump_stack+0xb4/0xe0) > [2.543645] [<8080fbe8>] (dump_stack) from [<8017efa4>] > (register_lock_class+0x6f0/0x718) > [2.551816] [<8017efa4>] (register_lock_class) from [<8017b7d0>] > (__lock_acquire+0x78/0x25f0) > [2.560330] [<8017b7d0>] (__lock_acquire) from [<8017e5e4>] > (lock_acquire+0xd8/0x1f4) > [2.568159] [<8017e5e4>] (lock_acquire) from [<80831fb0>] > (_raw_spin_lock_irqsave+0x3c/0x50) > [2.576589] [<80831fb0>] (_raw_spin_lock_irqsave) from [<8051b5fc>] > (owl_dma_issue_pending+0xbc/0x120) > [2.585884] [<8051b5fc>] (owl_dma_issue_pending) from [<80668cbc>] > (owl_mmc_request+0x1b0/0x390) > [2.594655] [<80668cbc>] (owl_mmc_request) from [<80650ce0>] > (mmc_start_request+0x94/0xbc) > [2.602906] [<80650ce0>] (mmc_start_request) from [<80650ec0>] > (mmc_wait_for_req+0x64/0xd0) > [2.611245] [<80650ec0>] (mmc_wait_for_req) from [<8065aa10>] > (mmc_app_send_scr+0x10c/0x144) > [2.619669] [<8065aa10>] (mmc_app_send_scr) from [<80659b3c>] > (mmc_sd_setup_card+0x4c/0x318) > [2.628092] [<80659b3c>] (mmc_sd_setup_card) from [<80659f0c>] > (mmc_sd_init_card+0x104/0x430) > [2.636601] [<80659f0c>] (mmc_sd_init_card) from [<8065a3e0>] > (mmc_attach_sd+0xcc/0x16c) > [2.644678] [<8065a3e0>] (mmc_attach_sd) from [<8065301c>] > (mmc_rescan+0x3ac/0x40c) > [2.652332] [<8065301c>] (mmc_rescan) from [<80143244>] > (process_one_work+0x2d8/0x780) > [2.660239] [<80143244>] (process_one_work) from [<80143730>] > (worker_thread+0x44/0x598) > [2.668323] [<80143730>] (worker_thread) from [<8014b5f8>] > (kthread+0x148/0x150) > [2.675708] [<8014b5f8>] (kthread) from [<801010b4>] > (ret_from_fork+0x14/0x20) > [2.682912] Exception stack(0xee8fdfb0 to 0xee8fdff8) > [2.687954] dfa0: > > [2.696118] dfc0: > > [2.704277] dfe0: 0013 > > The required fix is to use spin_lock_init() on the pchan lock before > attempting to call any spin_lock_irqsave() in owl_dma_get_pchan(). > Right, this is a bug. But while looking at the code now, I feel that we don't need 'pchan->lock'. The idea was to protect 'pchan->vchan', but I think 'od->lock' is the better candidate for that since it already protects it in 'owl_dma_terminate_pchan'. So I'd be happy if you remove the lock from 'pchan' and just directly use the one in 'od'. Out of curiosity, on which platform you're testing this? Thanks, Mani > Signed-off-by: Cristian Ciocaltea > --- > drivers/dma/owl-dma.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c > index c683051257fd..d9d0f0488e70 100644 > --- a/drivers/dma/owl-dma.c > +++ b/drivers/dma/owl-dma.c > @@ -1131,6 +1131,7 @@ static int owl_dma_probe(struct platform_device *pdev) > > pchan->id = i; > pchan->base = od->base + OWL_DMA_CHAN_BASE(i); > + spin_lock_init(>lock); > } > > /* Init virtual channel */ > -- > 2.26.2 >
