| Message ID | 1363165608-13739-1-git-send-email-santosh.shilimkar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
(Forgot to CC Thomas) On Wednesday 13 March 2013 02:36 PM, Santosh Shilimkar wrote: > With recent arm broadcast time clean-up from Mark Rutland, the dummy > broadcast device is always registered with timer subsystem. And since > the rating of the dummy clock event is very high, it is preferred > over a real broad-cast clock event. > > This is a change in behavior from past and not an intended > one. So reduce the rating of the dummy clockevent so that > real broadcast device is selected when available. > > Without this all the C states with C3STOP won't work since > the broad cast notifier will take an abort. > > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Russell King <linux@arm.linux.org.uk> > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > --- > Its a regression so hopefully can get into the 3.9-rcx. Noticed > this one on A15 platform. A9 platform the issue may not be seen > since the local timer check avoids dummy timer registration. > Some one pointed me to a fix made by Mark which was discussed under '[BUG] ARM Architected timers appear broken in 3.9-rc1' subject. That patch seems to be more of work around since the root of the problem is incorrect dummy timer rating. Either way, both patches fix the issue. > arch/arm/kernel/smp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c > index 31644f1..79078ed 100644 > --- a/arch/arm/kernel/smp.c > +++ b/arch/arm/kernel/smp.c > @@ -480,7 +480,7 @@ static void __cpuinit broadcast_timer_setup(struct clock_event_device *evt) > evt->features = CLOCK_EVT_FEAT_ONESHOT | > CLOCK_EVT_FEAT_PERIODIC | > CLOCK_EVT_FEAT_DUMMY; > - evt->rating = 400; > + evt->rating = 100; > evt->mult = 1; > evt->set_mode = broadcast_timer_set_mode; > >
Hi Santosh, On Wed, Mar 13, 2013 at 09:28:22AM +0000, Santosh Shilimkar wrote: > (Forgot to CC Thomas) > > On Wednesday 13 March 2013 02:36 PM, Santosh Shilimkar wrote: > > With recent arm broadcast time clean-up from Mark Rutland, the dummy > > broadcast device is always registered with timer subsystem. And since > > the rating of the dummy clock event is very high, it is preferred > > over a real broad-cast clock event. > > > > This is a change in behavior from past and not an intended > > one. So reduce the rating of the dummy clockevent so that > > real broadcast device is selected when available. > > > > Without this all the C states with C3STOP won't work since > > the broad cast notifier will take an abort. > > > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: Russell King <linux@arm.linux.org.uk> > > > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > > --- > > Its a regression so hopefully can get into the 3.9-rcx. Noticed > > this one on A15 platform. A9 platform the issue may not be seen > > since the local timer check avoids dummy timer registration. > > > Some one pointed me to a fix made by Mark which was discussed > under '[BUG] ARM Architected timers appear broken in 3.9-rc1' subject. > That patch seems to be more of work around since the root of the > problem is incorrect dummy timer rating. Either way, both patches > fix the issue. Is the problem that the dummy timer is being registered as the broadcast source, or that it is selected as a local timer in preference of real timers? If it is the former, Then I believe my patch solve the issue more generally - if you happen to register a dummy timer before other timers, it will become the broadcast source. Regardless of how temporary this is, it should never happen, and lowering the rating of the dummy won't fix this. If it is the latter, then this patch would ensure that a real timer with a rating over 100 is selected in preference to the dummy, which is certainly what we want. The proposed generic dummy timer in Stephen Boyd's local timer API removal series [1] similarly uses a low rating to ensure that real timers are selected in preference to an always-registered dummy. I note that the architected timer has a higher rating (450) than the dummy (400), so this shouldn't currently be a problem. Have I missed something? [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-March/153208.html Thanks, Mark.
On Wednesday 13 March 2013 03:46 PM, Mark Rutland wrote: > Hi Santosh, > > On Wed, Mar 13, 2013 at 09:28:22AM +0000, Santosh Shilimkar wrote: >> (Forgot to CC Thomas) >> >> On Wednesday 13 March 2013 02:36 PM, Santosh Shilimkar wrote: >>> With recent arm broadcast time clean-up from Mark Rutland, the dummy >>> broadcast device is always registered with timer subsystem. And since >>> the rating of the dummy clock event is very high, it is preferred >>> over a real broad-cast clock event. >>> >>> This is a change in behavior from past and not an intended >>> one. So reduce the rating of the dummy clockevent so that >>> real broadcast device is selected when available. >>> >>> Without this all the C states with C3STOP won't work since >>> the broad cast notifier will take an abort. >>> >>> Cc: Mark Rutland <mark.rutland@arm.com> >>> Cc: Russell King <linux@arm.linux.org.uk> >>> >>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>> --- >>> Its a regression so hopefully can get into the 3.9-rcx. Noticed >>> this one on A15 platform. A9 platform the issue may not be seen >>> since the local timer check avoids dummy timer registration. >>> >> Some one pointed me to a fix made by Mark which was discussed >> under '[BUG] ARM Architected timers appear broken in 3.9-rc1' subject. >> That patch seems to be more of work around since the root of the >> problem is incorrect dummy timer rating. Either way, both patches >> fix the issue. > > Is the problem that the dummy timer is being registered as the broadcast > source, or that it is selected as a local timer in preference of real timers? > Dummy timer is preferred over real broadcast timer. > If it is the former, Then I believe my patch solve the issue more generally - > if you happen to register a dummy timer before other timers, it will become the > broadcast source. Regardless of how temporary this is, it should never happen, > and lowering the rating of the dummy won't fix this. > Well by the time we need active broadcast functionality, clock-events are already chosen if the ratings are appropriate. > If it is the latter, then this patch would ensure that a real timer with a > rating over 100 is selected in preference to the dummy, which is certainly what > we want. The proposed generic dummy timer in Stephen Boyd's local timer API > removal series [1] similarly uses a low rating to ensure that real timers are > selected in preference to an always-registered dummy. I note that the > architected timer has a higher rating (450) than the dummy (400), so this > shouldn't currently be a problem. > Because we always register dummy broadcast on ARM now and with higher rating, it is picked as broadcast source. We definitely don't want such a behavior when we have real broadcast device is available. With your patch, we are trying to avoid the registration which goes against the the whole idea of registering it always and picking the right clock-event based on rating by clock-event core. The clock-event core except the proper ratings to be provided based on the capability of the timer source and its resolution and in that case dummy should have the lowest rating which is what I tried to patch. > Have I missed something? > Not much. I was just looking at x86 code as well after your email to see how the LAPIC issue is handled. They seems to also have correct rating to take care of the selection. Probably we can merge both the fixes but from clock-event core code perspective, the ratings fix is more than enough. Thomas might be able to comment better. Regards, Santosh
On Wed, Mar 13, 2013 at 11:24:01AM +0000, Santosh Shilimkar wrote: > On Wednesday 13 March 2013 03:46 PM, Mark Rutland wrote: > > Hi Santosh, > > > > On Wed, Mar 13, 2013 at 09:28:22AM +0000, Santosh Shilimkar wrote: > >> (Forgot to CC Thomas) > >> > >> On Wednesday 13 March 2013 02:36 PM, Santosh Shilimkar wrote: > >>> With recent arm broadcast time clean-up from Mark Rutland, the dummy > >>> broadcast device is always registered with timer subsystem. And since > >>> the rating of the dummy clock event is very high, it is preferred > >>> over a real broad-cast clock event. > >>> > >>> This is a change in behavior from past and not an intended > >>> one. So reduce the rating of the dummy clockevent so that > >>> real broadcast device is selected when available. > >>> > >>> Without this all the C states with C3STOP won't work since > >>> the broad cast notifier will take an abort. > >>> > >>> Cc: Mark Rutland <mark.rutland@arm.com> > >>> Cc: Russell King <linux@arm.linux.org.uk> > >>> > >>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > >>> --- > >>> Its a regression so hopefully can get into the 3.9-rcx. Noticed > >>> this one on A15 platform. A9 platform the issue may not be seen > >>> since the local timer check avoids dummy timer registration. > >>> > >> Some one pointed me to a fix made by Mark which was discussed > >> under '[BUG] ARM Architected timers appear broken in 3.9-rc1' subject. > >> That patch seems to be more of work around since the root of the > >> problem is incorrect dummy timer rating. Either way, both patches > >> fix the issue. > > > > Is the problem that the dummy timer is being registered as the broadcast > > source, or that it is selected as a local timer in preference of real timers? > > > Dummy timer is preferred over real broadcast timer. Ok. > > > If it is the former, Then I believe my patch solve the issue more generally - > > if you happen to register a dummy timer before other timers, it will become the > > broadcast source. Regardless of how temporary this is, it should never happen, > > and lowering the rating of the dummy won't fix this. > > > Well by the time we need active broadcast functionality, clock-events are > already chosen if the ratings are appropriate. That's a fair point. I still think it's worth having the check in the core broadcast code - rejecting dummy timers is always a sensible thing to do, and it prevents future crashes if new timers are added without sensible rating values. > > > If it is the latter, then this patch would ensure that a real timer with a > > rating over 100 is selected in preference to the dummy, which is certainly what > > we want. The proposed generic dummy timer in Stephen Boyd's local timer API > > removal series [1] similarly uses a low rating to ensure that real timers are > > selected in preference to an always-registered dummy. I note that the > > architected timer has a higher rating (450) than the dummy (400), so this > > shouldn't currently be a problem. > > > Because we always register dummy broadcast on ARM now > and with higher rating, it is picked as broadcast source. We definitely don't > want such a behavior when we have real broadcast device is available. Agreed. We *never* want to pick a dummy timer as a broadcast source, as this never makes sense. > With your patch, we are trying to avoid the registration which goes > against the the whole idea of registering it always and picking > the right clock-event based on rating by clock-event core. > The clock-event core except the proper ratings to be provided based on > the capability of the timer source and its resolution and in that case > dummy should have the lowest rating which is what I tried to patch. > > > Have I missed something? > > > Not much. I was just looking at x86 code as well after your email > to see how the LAPIC issue is handled. They seems to also have > correct rating to take care of the selection. > > Probably we can merge both the fixes but from clock-event core > code perspective, the ratings fix is more than enough. I do agree it'd be worth lowering the dummy timer's rating to ensure it doesn't override a real timer elsewhere. Thomas has already pulled my fix into tip timers/urgent [1]. [1] https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=timers/urgent&id=a7dc19b8652c862d5b7c4d2339bd3c428bd29c4a Thanks, Mark.
On Wed, 13 Mar 2013, Santosh Shilimkar wrote: > On Wednesday 13 March 2013 02:36 PM, Santosh Shilimkar wrote: > > With recent arm broadcast time clean-up from Mark Rutland, the dummy > > broadcast device is always registered with timer subsystem. And since > > the rating of the dummy clock event is very high, it is preferred > > over a real broad-cast clock event. > > > > This is a change in behavior from past and not an intended > > one. So reduce the rating of the dummy clockevent so that > > real broadcast device is selected when available. > > > > Without this all the C states with C3STOP won't work since > > the broad cast notifier will take an abort. > > > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: Russell King <linux@arm.linux.org.uk> > > > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > > --- > > Its a regression so hopefully can get into the 3.9-rcx. Noticed > > this one on A15 platform. A9 platform the issue may not be seen > > since the local timer check avoids dummy timer registration. > > > Some one pointed me to a fix made by Mark which was discussed > under '[BUG] ARM Architected timers appear broken in 3.9-rc1' subject. > That patch seems to be more of work around since the root of the > problem is incorrect dummy timer rating. Either way, both patches > fix the issue. Well, using a dummy timer as the broadcast event device is a bug, no matter what the rating is. The fix is in Linus tree already. Though making the rating of the dummy lower is definitely a good thing, so a real hardware device which is detected later can replace the dummy device. So yes, the rating should be low for the dummy timer. Thanks, tglx
On Wednesday 13 March 2013 07:49 PM, Thomas Gleixner wrote: > On Wed, 13 Mar 2013, Santosh Shilimkar wrote: >> On Wednesday 13 March 2013 02:36 PM, Santosh Shilimkar wrote: >>> With recent arm broadcast time clean-up from Mark Rutland, the dummy >>> broadcast device is always registered with timer subsystem. And since >>> the rating of the dummy clock event is very high, it is preferred >>> over a real broad-cast clock event. >>> >>> This is a change in behavior from past and not an intended >>> one. So reduce the rating of the dummy clockevent so that >>> real broadcast device is selected when available. >>> >>> Without this all the C states with C3STOP won't work since >>> the broad cast notifier will take an abort. >>> >>> Cc: Mark Rutland <mark.rutland@arm.com> >>> Cc: Russell King <linux@arm.linux.org.uk> >>> >>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>> --- >>> Its a regression so hopefully can get into the 3.9-rcx. Noticed >>> this one on A15 platform. A9 platform the issue may not be seen >>> since the local timer check avoids dummy timer registration. >>> >> Some one pointed me to a fix made by Mark which was discussed >> under '[BUG] ARM Architected timers appear broken in 3.9-rc1' subject. >> That patch seems to be more of work around since the root of the >> problem is incorrect dummy timer rating. Either way, both patches >> fix the issue. > > Well, using a dummy timer as the broadcast event device is a bug, no > matter what the rating is. The fix is in Linus tree already. > Agree. > Though making the rating of the dummy lower is definitely a good > thing, so a real hardware device which is detected later can replace > the dummy device. So yes, the rating should be low for the dummy > timer. > Exactly. As Mark pointed out, you have already applied Mark's patch. I was just wondering whether you can take the $subject patch as well with ack from Russell, Mark. Regards, Santosh
On Wednesday 13 March 2013 05:55 PM, Mark Rutland wrote: > On Wed, Mar 13, 2013 at 11:24:01AM +0000, Santosh Shilimkar wrote: >> On Wednesday 13 March 2013 03:46 PM, Mark Rutland wrote: >>> Hi Santosh, [..] >>> >>> Is the problem that the dummy timer is being registered as the broadcast >>> source, or that it is selected as a local timer in preference of real timers? >>> >> Dummy timer is preferred over real broadcast timer. > > Ok. > >> >>> If it is the former, Then I believe my patch solve the issue more generally - >>> if you happen to register a dummy timer before other timers, it will become the >>> broadcast source. Regardless of how temporary this is, it should never happen, >>> and lowering the rating of the dummy won't fix this. >>> >> Well by the time we need active broadcast functionality, clock-events are >> already chosen if the ratings are appropriate. > > That's a fair point. I still think it's worth having the check in the core > broadcast code - rejecting dummy timers is always a sensible thing to do, and > it prevents future crashes if new timers are added without sensible rating > values. > >> >>> If it is the latter, then this patch would ensure that a real timer with a >>> rating over 100 is selected in preference to the dummy, which is certainly what >>> we want. The proposed generic dummy timer in Stephen Boyd's local timer API >>> removal series [1] similarly uses a low rating to ensure that real timers are >>> selected in preference to an always-registered dummy. I note that the >>> architected timer has a higher rating (450) than the dummy (400), so this >>> shouldn't currently be a problem. >>> >> Because we always register dummy broadcast on ARM now >> and with higher rating, it is picked as broadcast source. We definitely don't >> want such a behavior when we have real broadcast device is available. > > Agreed. We *never* want to pick a dummy timer as a broadcast source, as this > never makes sense. > >> With your patch, we are trying to avoid the registration which goes >> against the the whole idea of registering it always and picking >> the right clock-event based on rating by clock-event core. >> The clock-event core except the proper ratings to be provided based on >> the capability of the timer source and its resolution and in that case >> dummy should have the lowest rating which is what I tried to patch. >> >>> Have I missed something? >>> >> Not much. I was just looking at x86 code as well after your email >> to see how the LAPIC issue is handled. They seems to also have >> correct rating to take care of the selection. >> >> Probably we can merge both the fixes but from clock-event core >> code perspective, the ratings fix is more than enough. > > I do agree it'd be worth lowering the dummy timer's rating to ensure it doesn't > override a real timer elsewhere. > Yep. Can I add you acked-by tag then for $subject patch ? Would be good to get this one merged as well. Regards Santosh
On Wed, Mar 13, 2013 at 03:44:03PM +0000, Santosh Shilimkar wrote: > On Wednesday 13 March 2013 05:55 PM, Mark Rutland wrote: > > On Wed, Mar 13, 2013 at 11:24:01AM +0000, Santosh Shilimkar wrote: > >> On Wednesday 13 March 2013 03:46 PM, Mark Rutland wrote: > >>> Hi Santosh, > > [..] > > >>> > >>> Is the problem that the dummy timer is being registered as the broadcast > >>> source, or that it is selected as a local timer in preference of real timers? > >>> > >> Dummy timer is preferred over real broadcast timer. > > > > Ok. [...] > > I do agree it'd be worth lowering the dummy timer's rating to ensure it doesn't > > override a real timer elsewhere. > > > Yep. Can I add you acked-by tag then for $subject patch ? > Would be good to get this one merged as well. Sure. Though could you reword the commit message? The patch solves the more general issue of a dummy being preferred over real hardware even outside of choosing the broadcast device. Acked-by: Mark Rutland <mark.rutland@arm.com>
On Wed, 13 Mar 2013, Santosh Shilimkar wrote: > On Wednesday 13 March 2013 07:49 PM, Thomas Gleixner wrote: > > Though making the rating of the dummy lower is definitely a good > > thing, so a real hardware device which is detected later can replace > > the dummy device. So yes, the rating should be low for the dummy > > timer. > > > Exactly. As Mark pointed out, you have already applied Mark's patch. > I was just wondering whether you can take the $subject patch as well > with ack from Russell, Mark. It can go through the ARM tree as well, no dependencies on my stuff. Thanks, tglx
On Thursday 14 March 2013 12:01 AM, Thomas Gleixner wrote: > On Wed, 13 Mar 2013, Santosh Shilimkar wrote: >> On Wednesday 13 March 2013 07:49 PM, Thomas Gleixner wrote: >>> Though making the rating of the dummy lower is definitely a good >>> thing, so a real hardware device which is detected later can replace >>> the dummy device. So yes, the rating should be low for the dummy >>> timer. >>> >> Exactly. As Mark pointed out, you have already applied Mark's patch. >> I was just wondering whether you can take the $subject patch as well >> with ack from Russell, Mark. > > It can go through the ARM tree as well, no dependencies on my stuff. > Sure. I will push that through ARM tree. Regards, Santosh
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index 31644f1..79078ed 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -480,7 +480,7 @@ static void __cpuinit broadcast_timer_setup(struct clock_event_device *evt) evt->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_DUMMY; - evt->rating = 400; + evt->rating = 100; evt->mult = 1; evt->set_mode = broadcast_timer_set_mode;
With recent arm broadcast time clean-up from Mark Rutland, the dummy broadcast device is always registered with timer subsystem. And since the rating of the dummy clock event is very high, it is preferred over a real broad-cast clock event. This is a change in behavior from past and not an intended one. So reduce the rating of the dummy clockevent so that real broadcast device is selected when available. Without this all the C states with C3STOP won't work since the broad cast notifier will take an abort. Cc: Mark Rutland <mark.rutland@arm.com> Cc: Russell King <linux@arm.linux.org.uk> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> --- Its a regression so hopefully can get into the 3.9-rcx. Noticed this one on A15 platform. A9 platform the issue may not be seen since the local timer check avoids dummy timer registration. arch/arm/kernel/smp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)