diff mbox

sh7724 regression: commit 8222dbe21e79338de92d5e1956cd1e3994cc9f93

Message ID 20160229092441.2e999f8d@thinkpad-w530
State New
Headers show

Commit Message

David Hildenbrand Feb. 29, 2016, 8:24 a.m. UTC
> On 02/27/2016 03:28 PM, John Paul Adrian Glaubitz wrote:
> > Hi Hans!
> > 
> > On 02/27/2016 03:20 PM, Geert Uytterhoeven wrote:  
> >>> I noticed that 4.1 was ok and v4.2 wasn't, so I did a git bisect and ended up with
> >>> commit 8222dbe21e79338de92d5e1956cd1e3994cc9f93 (sched/preempt, mm/fault: Decouple
> >>> preemption from the page fault logic) as the culprit.  
> > 
> > If you like, please report this the kernel bug tracker and set
> > "Hardware" to "SuperH". This way we are able to keep track of
> > the issue.
> > 
> > I think Yoshinori Sato and Rich Felker will have a look at this.
> > 
> > Adrian
> >   
> 
> Done: https://bugzilla.kernel.org/show_bug.cgi?id=113321
> 
> Regards,
> 
> 	Hans
> 

Looks like the code then requires to not be allowed to sleep/preempt in these
sections (just like kmap_atomic, or kmap_coherent on mips).

So the fix should be simple as (untested):

From d29f0d0cb670175c688cdd181c7a693d28b27a33 Mon Sep 17 00:00:00 2001             
From: David Hildenbrand <dahi@linux.vnet.ibm.com>                                  
Date: Mon, 29 Feb 2016 09:19:24 +0100                                              
Subject: [PATCH] sched/preempt, sh: kmap_coherent relies on disabled               
 preemption                                                                        
                                                                                   
kmap_coherent needs disabled preemption to not schedule in the critical            
section, just like kmap_coherent on mips and kmap_atomic in general.               
                                                                                   
Fixes: 8222dbe21e79 "sched/preempt, mm/fault: Decouple preemption from the page fault logic"
Reported-by: Hans Verkuil <hverkuil@xs4all.nl>                                     
Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>                         
---                                                                                
 arch/sh/mm/kmap.c | 2 ++                                                          
 1 file changed, 2 insertions(+)                                                   
                                                                                   
--                                                                                 
2.3.9                                                                              
         

David

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Hans Verkuil Feb. 29, 2016, 9:01 a.m. UTC | #1
On 02/29/2016 09:24 AM, David Hildenbrand wrote:
>> On 02/27/2016 03:28 PM, John Paul Adrian Glaubitz wrote:
>>> Hi Hans!
>>>
>>> On 02/27/2016 03:20 PM, Geert Uytterhoeven wrote:  
>>>>> I noticed that 4.1 was ok and v4.2 wasn't, so I did a git bisect and ended up with
>>>>> commit 8222dbe21e79338de92d5e1956cd1e3994cc9f93 (sched/preempt, mm/fault: Decouple
>>>>> preemption from the page fault logic) as the culprit.  
>>>
>>> If you like, please report this the kernel bug tracker and set
>>> "Hardware" to "SuperH". This way we are able to keep track of
>>> the issue.
>>>
>>> I think Yoshinori Sato and Rich Felker will have a look at this.
>>>
>>> Adrian
>>>   
>>
>> Done: https://bugzilla.kernel.org/show_bug.cgi?id=113321
>>
>> Regards,
>>
>> 	Hans
>>
> 
> Looks like the code then requires to not be allowed to sleep/preempt in these
> sections (just like kmap_atomic, or kmap_coherent on mips).
> 
> So the fix should be simple as (untested):

That fixes it for me:

Tested-by: Hans Verkuil <hans.verkuil@cisco.com>

BTW, your patch is garbled and I had to manually make the change (not a big
deal for a simple change like this).

Thanks!

	Hans

> 
> From d29f0d0cb670175c688cdd181c7a693d28b27a33 Mon Sep 17 00:00:00 2001             
> From: David Hildenbrand <dahi@linux.vnet.ibm.com>                                  
> Date: Mon, 29 Feb 2016 09:19:24 +0100                                              
> Subject: [PATCH] sched/preempt, sh: kmap_coherent relies on disabled               
>  preemption                                                                        
>                                                                                    
> kmap_coherent needs disabled preemption to not schedule in the critical            
> section, just like kmap_coherent on mips and kmap_atomic in general.               
>                                                                                    
> Fixes: 8222dbe21e79 "sched/preempt, mm/fault: Decouple preemption from the page fault logic"
> Reported-by: Hans Verkuil <hverkuil@xs4all.nl>                                     
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>                         
> ---                                                                                
>  arch/sh/mm/kmap.c | 2 ++                                                          
>  1 file changed, 2 insertions(+)                                                   
>                                                                                    
> diff --git a/arch/sh/mm/kmap.c b/arch/sh/mm/kmap.c                                 
> index ec29e14..bf25d7c 100644                                                      
> --- a/arch/sh/mm/kmap.c                                                            
> +++ b/arch/sh/mm/kmap.c                                                            
> @@ -36,6 +36,7 @@ void *kmap_coherent(struct page *page, unsigned long addr)    
>                                                                                    
>         BUG_ON(!test_bit(PG_dcache_clean, &page->flags));                          
>                                                                                    
> +       preempt_disable();                                                         
>         pagefault_disable();                                                       
>                                                                                    
>         idx = FIX_CMAP_END -                                                       
> @@ -64,4 +65,5 @@ void kunmap_coherent(void *kvaddr)                               
>         }                                                                          
>                                                                                    
>         pagefault_enable();                                                        
> +       preempt_enable();                                                          
>  }                                                                                 
> --                                                                                 
> 2.3.9                                                                              
>          
> 
> David
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Hildenbrand Feb. 29, 2016, 11:03 a.m. UTC | #2
> On 02/29/2016 09:24 AM, David Hildenbrand wrote:
> >> On 02/27/2016 03:28 PM, John Paul Adrian Glaubitz wrote:  
> >>> Hi Hans!
> >>>
> >>> On 02/27/2016 03:20 PM, Geert Uytterhoeven wrote:    
> >>>>> I noticed that 4.1 was ok and v4.2 wasn't, so I did a git bisect and ended up with
> >>>>> commit 8222dbe21e79338de92d5e1956cd1e3994cc9f93 (sched/preempt, mm/fault: Decouple
> >>>>> preemption from the page fault logic) as the culprit.    
> >>>
> >>> If you like, please report this the kernel bug tracker and set
> >>> "Hardware" to "SuperH". This way we are able to keep track of
> >>> the issue.
> >>>
> >>> I think Yoshinori Sato and Rich Felker will have a look at this.
> >>>
> >>> Adrian
> >>>     
> >>
> >> Done: https://bugzilla.kernel.org/show_bug.cgi?id=113321
> >>
> >> Regards,
> >>
> >> 	Hans
> >>  
> > 
> > Looks like the code then requires to not be allowed to sleep/preempt in these
> > sections (just like kmap_atomic, or kmap_coherent on mips).
> > 
> > So the fix should be simple as (untested):  
> 
> That fixes it for me:
> 
> Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> BTW, your patch is garbled and I had to manually make the change (not a big
> deal for a simple change like this).
> 

Thanks for testing. I just copy-pasted the patch, so this was somehow
expected :)

@maintainers, how to proceed with this? Shall I send that patch again directly
to the linux-sh list?

David

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman March 2, 2016, 12:20 a.m. UTC | #3
[CC Super H maintainers: Sato-san and Rich Felker]

On Mon, Feb 29, 2016 at 12:03:38PM +0100, David Hildenbrand wrote:
> > On 02/29/2016 09:24 AM, David Hildenbrand wrote:
> > >> On 02/27/2016 03:28 PM, John Paul Adrian Glaubitz wrote:  
> > >>> Hi Hans!
> > >>>
> > >>> On 02/27/2016 03:20 PM, Geert Uytterhoeven wrote:    
> > >>>>> I noticed that 4.1 was ok and v4.2 wasn't, so I did a git bisect and ended up with
> > >>>>> commit 8222dbe21e79338de92d5e1956cd1e3994cc9f93 (sched/preempt, mm/fault: Decouple
> > >>>>> preemption from the page fault logic) as the culprit.    
> > >>>
> > >>> If you like, please report this the kernel bug tracker and set
> > >>> "Hardware" to "SuperH". This way we are able to keep track of
> > >>> the issue.
> > >>>
> > >>> I think Yoshinori Sato and Rich Felker will have a look at this.
> > >>>
> > >>> Adrian
> > >>>     
> > >>
> > >> Done: https://bugzilla.kernel.org/show_bug.cgi?id=113321
> > >>
> > >> Regards,
> > >>
> > >> 	Hans
> > >>  
> > > 
> > > Looks like the code then requires to not be allowed to sleep/preempt in these
> > > sections (just like kmap_atomic, or kmap_coherent on mips).
> > > 
> > > So the fix should be simple as (untested):  
> > 
> > That fixes it for me:
> > 
> > Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > BTW, your patch is garbled and I had to manually make the change (not a big
> > deal for a simple change like this).
> > 
> 
> Thanks for testing. I just copy-pasted the patch, so this was somehow
> expected :)
> 
> @maintainers, how to proceed with this? Shall I send that patch again directly
> to the linux-sh list?
> 
> David
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Hildenbrand March 11, 2016, 11:06 a.m. UTC | #4
> > > That fixes it for me:
> > > 
> > > Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > 
> > > BTW, your patch is garbled and I had to manually make the change (not a big
> > > deal for a simple change like this).
> > >   
> > 
> > Thanks for testing. I just copy-pasted the patch, so this was somehow
> > expected :)
> > 
> > @maintainers, how to proceed with this? Shall I send that patch again directly
> > to the linux-sh list?
> > 
> > David  
> 

How to proceed with this patch? Anyone?

David

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rich Felker March 13, 2016, 7:08 p.m. UTC | #5
On Fri, Mar 11, 2016 at 12:06:22PM +0100, David Hildenbrand wrote:
> 
> > > > That fixes it for me:
> > > > 
> > > > Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > 
> > > > BTW, your patch is garbled and I had to manually make the change (not a big
> > > > deal for a simple change like this).
> > > >   
> > > 
> > > Thanks for testing. I just copy-pasted the patch, so this was somehow
> > > expected :)
> > > 
> > > @maintainers, how to proceed with this? Shall I send that patch again directly
> > > to the linux-sh list?
> > > 
> > > David  
> > 
> 
> How to proceed with this patch? Anyone?

Sorry for not getting you a response sooner. I don't yet have the
hardware to test this fix on (would like to find some), but it looks
reasonable to me and like it doesn't risk introducing other
regressions. Sato-san, do you have any thoughts or objections to the
patch?

Rich
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rich Felker March 17, 2016, 12:35 a.m. UTC | #6
On Sun, Mar 13, 2016 at 03:08:42PM -0400, Rich Felker wrote:
> On Fri, Mar 11, 2016 at 12:06:22PM +0100, David Hildenbrand wrote:
> > 
> > > > > That fixes it for me:
> > > > > 
> > > > > Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > > 
> > > > > BTW, your patch is garbled and I had to manually make the change (not a big
> > > > > deal for a simple change like this).
> > > > >   
> > > > 
> > > > Thanks for testing. I just copy-pasted the patch, so this was somehow
> > > > expected :)
> > > > 
> > > > @maintainers, how to proceed with this? Shall I send that patch again directly
> > > > to the linux-sh list?
> > > > 
> > > > David  
> > > 
> > 
> > How to proceed with this patch? Anyone?
> 
> Sorry for not getting you a response sooner. I don't yet have the
> hardware to test this fix on (would like to find some), but it looks
> reasonable to me and like it doesn't risk introducing other
> regressions. Sato-san, do you have any thoughts or objections to the
> patch?

Can you follow up with a well-formed patch that I can apply. No
promises since I'm new to kernel maintainership workflow but I'll try
to get it in this merge window if I can.

Rich
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rich Felker March 17, 2016, 5:38 p.m. UTC | #7
On Wed, Mar 16, 2016 at 08:35:20PM -0400, Rich Felker wrote:
> On Sun, Mar 13, 2016 at 03:08:42PM -0400, Rich Felker wrote:
> > On Fri, Mar 11, 2016 at 12:06:22PM +0100, David Hildenbrand wrote:
> > > 
> > > > > > That fixes it for me:
> > > > > > 
> > > > > > Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > > > 
> > > > > > BTW, your patch is garbled and I had to manually make the change (not a big
> > > > > > deal for a simple change like this).
> > > > > >   
> > > > > 
> > > > > Thanks for testing. I just copy-pasted the patch, so this was somehow
> > > > > expected :)
> > > > > 
> > > > > @maintainers, how to proceed with this? Shall I send that patch again directly
> > > > > to the linux-sh list?
> > > > > 
> > > > > David  
> > > > 
> > > 
> > > How to proceed with this patch? Anyone?
> > 
> > Sorry for not getting you a response sooner. I don't yet have the
> > hardware to test this fix on (would like to find some), but it looks
> > reasonable to me and like it doesn't risk introducing other
> > regressions. Sato-san, do you have any thoughts or objections to the
> > patch?
> 
> Can you follow up with a well-formed patch that I can apply. No
> promises since I'm new to kernel maintainership workflow but I'll try
> to get it in this merge window if I can.

I was able to clean it up (looks like terminal copy&paste just botched
all the spaces) so I'm applying it to my tree, since it seems likely
that most SH models with MMU are broken without it. Thanks!

Rich
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Hildenbrand March 29, 2016, 6:33 a.m. UTC | #8
> On Wed, Mar 16, 2016 at 08:35:20PM -0400, Rich Felker wrote:
> > On Sun, Mar 13, 2016 at 03:08:42PM -0400, Rich Felker wrote:  
> > > On Fri, Mar 11, 2016 at 12:06:22PM +0100, David Hildenbrand wrote:  
> > > >   
> > > > > > > That fixes it for me:
> > > > > > > 
> > > > > > > Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > > > > > 
> > > > > > > BTW, your patch is garbled and I had to manually make the change (not a big
> > > > > > > deal for a simple change like this).
> > > > > > >     
> > > > > > 
> > > > > > Thanks for testing. I just copy-pasted the patch, so this was somehow
> > > > > > expected :)
> > > > > > 
> > > > > > @maintainers, how to proceed with this? Shall I send that patch again directly
> > > > > > to the linux-sh list?
> > > > > > 
> > > > > > David    
> > > > >   
> > > > 
> > > > How to proceed with this patch? Anyone?  
> > > 
> > > Sorry for not getting you a response sooner. I don't yet have the
> > > hardware to test this fix on (would like to find some), but it looks
> > > reasonable to me and like it doesn't risk introducing other
> > > regressions. Sato-san, do you have any thoughts or objections to the
> > > patch?  
> > 
> > Can you follow up with a well-formed patch that I can apply. No
> > promises since I'm new to kernel maintainership workflow but I'll try
> > to get it in this merge window if I can.  
> 
> I was able to clean it up (looks like terminal copy&paste just botched
> all the spaces) so I'm applying it to my tree, since it seems likely
> that most SH models with MMU are broken without it. Thanks!
> 
> Rich
> 

Hi Rich,

sorry, was on vacation and could respond quickly ;)

Thanks for applying this!

David

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/sh/mm/kmap.c b/arch/sh/mm/kmap.c                                 
index ec29e14..bf25d7c 100644                                                      
--- a/arch/sh/mm/kmap.c                                                            
+++ b/arch/sh/mm/kmap.c                                                            
@@ -36,6 +36,7 @@  void *kmap_coherent(struct page *page, unsigned long addr)    
                                                                                   
        BUG_ON(!test_bit(PG_dcache_clean, &page->flags));                          
                                                                                   
+       preempt_disable();                                                         
        pagefault_disable();                                                       
                                                                                   
        idx = FIX_CMAP_END -                                                       
@@ -64,4 +65,5 @@  void kunmap_coherent(void *kvaddr)                               
        }                                                                          
                                                                                   
        pagefault_enable();                                                        
+       preempt_enable();                                                          
 }