* Re: [PATCH][RFC] drivers: sh: late disabling of clocks
2011-06-09 9:13 [PATCH][RFC] drivers: sh: late disabling of clocks Magnus Damm
@ 2011-06-13 8:59 ` Simon Horman
2011-06-14 6:41 ` Simon Horman
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Simon Horman @ 2011-06-13 8:59 UTC (permalink / raw)
To: linux-sh
On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
>
> This RFC patch kills two birds with one stone:
>
> 1) Fix existing issue where in-use clocks without software
> reference are disabled during boot. One example of this
> is the handling of the Mackerel serial console output.
>
> 2) Make sure so far unused clocks actually get turned off
Is access to allow_disable SMP-safe?
In particular, does the the access in clk_late_init()
need to be guarded by clock_lock as calls to __clk_disable() are?
>
> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
>
> drivers/sh/clk/core.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
>
> --- 0001/drivers/sh/clk/core.c
> +++ work/drivers/sh/clk/core.c 2011-06-09 14:13:02.000000000 +0900
> @@ -34,6 +34,9 @@ static LIST_HEAD(clock_list);
> static DEFINE_SPINLOCK(clock_lock);
> static DEFINE_MUTEX(clock_list_sem);
>
> +/* clock disable operations are not passed on to hardware during boot */
> +static int allow_disable;
> +
> void clk_rate_table_build(struct clk *clk,
> struct cpufreq_frequency_table *freq_table,
> int nr_freqs,
> @@ -228,7 +231,7 @@ static void __clk_disable(struct clk *cl
> return;
>
> if (!(--clk->usecount)) {
> - if (likely(clk->ops && clk->ops->disable))
> + if (likely(allow_disable && clk->ops && clk->ops->disable))
> clk->ops->disable(clk);
> if (likely(clk->parent))
> __clk_disable(clk->parent);
> @@ -747,3 +750,24 @@ err_out:
> return err;
> }
> late_initcall(clk_debugfs_init);
> +
> +static int __init clk_late_init(void)
> +{
> + unsigned long flags;
> + struct clk *clk;
> +
> + /* from now on allow clock disable operations */
> + allow_disable = 1;
> +
> + /* disable all clocks with zero use count */
> + mutex_lock(&clock_list_sem);
> + list_for_each_entry(clk, &clock_list, node) {
> + spin_lock_irqsave(&clock_lock, flags);
> + if (!clk->usecount && clk->ops && clk->ops->disable)
> + clk->ops->disable(clk);
> + spin_unlock_irqrestore(&clock_lock, flags);
> + }
> + mutex_unlock(&clock_list_sem);
> + return 0;
> +}
> +late_initcall(clk_late_init);
> --
> 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH][RFC] drivers: sh: late disabling of clocks
2011-06-09 9:13 [PATCH][RFC] drivers: sh: late disabling of clocks Magnus Damm
2011-06-13 8:59 ` Simon Horman
@ 2011-06-14 6:41 ` Simon Horman
2011-06-15 14:21 ` Magnus Damm
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Simon Horman @ 2011-06-14 6:41 UTC (permalink / raw)
To: linux-sh
On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
> > From: Magnus Damm <damm@opensource.se>
> >
> > This RFC patch kills two birds with one stone:
> >
> > 1) Fix existing issue where in-use clocks without software
> > reference are disabled during boot. One example of this
> > is the handling of the Mackerel serial console output.
> >
> > 2) Make sure so far unused clocks actually get turned off
>
> Is access to allow_disable SMP-safe?
> In particular, does the the access in clk_late_init()
> need to be guarded by clock_lock as calls to __clk_disable() are?
On reflection I think that the problem is a bit deeper.
I think allow_disable is set to 1 prematurely, as it indicates
to __clk_disable() that calling clk->ops->disable(clk) is allowed,
even if the iteration of the loop in clk_late_init() is not complete.
My proposed re-working of clk_late_init() is below.
Completely untested.
>
> > Signed-off-by: Magnus Damm <damm@opensource.se>
> > ---
> >
> > drivers/sh/clk/core.c | 26 +++++++++++++++++++++++++-
> > 1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > --- 0001/drivers/sh/clk/core.c
> > +++ work/drivers/sh/clk/core.c 2011-06-09 14:13:02.000000000 +0900
> > @@ -34,6 +34,9 @@ static LIST_HEAD(clock_list);
> > static DEFINE_SPINLOCK(clock_lock);
> > static DEFINE_MUTEX(clock_list_sem);
> >
> > +/* clock disable operations are not passed on to hardware during boot */
> > +static int allow_disable;
> > +
> > void clk_rate_table_build(struct clk *clk,
> > struct cpufreq_frequency_table *freq_table,
> > int nr_freqs,
> > @@ -228,7 +231,7 @@ static void __clk_disable(struct clk *cl
> > return;
> >
> > if (!(--clk->usecount)) {
> > - if (likely(clk->ops && clk->ops->disable))
> > + if (likely(allow_disable && clk->ops && clk->ops->disable))
> > clk->ops->disable(clk);
> > if (likely(clk->parent))
> > __clk_disable(clk->parent);
> > @@ -747,3 +750,24 @@ err_out:
> > return err;
> > }
> > late_initcall(clk_debugfs_init);
> > +
> > +static int __init clk_late_init(void)
> > +{
> > + unsigned long flags;
> > + struct clk *clk;
> > +
> > + /* from now on allow clock disable operations */
> > + allow_disable = 1;
> > +
> > + /* disable all clocks with zero use count */
> > + mutex_lock(&clock_list_sem);
> > + list_for_each_entry(clk, &clock_list, node) {
> > + spin_lock_irqsave(&clock_lock, flags);
> > + if (!clk->usecount && clk->ops && clk->ops->disable)
> > + clk->ops->disable(clk);
> > + spin_unlock_irqrestore(&clock_lock, flags);
> > + }
> > + mutex_unlock(&clock_list_sem);
> > + return 0;
> > +}
static int __init clk_late_init(void)
{
unsigned long flags;
struct clk *clk;
mutex_lock(&clock_list_sem);
spin_lock_irqsave(&clock_lock, flags);
/* disable all clocks with zero use count */
list_for_each_entry(clk, &clock_list, node)
if (!clk->usecount && clk->ops && clk->ops->disable)
clk->ops->disable(clk);
/* from now on allow clock disable operations */
allow_disable = 1;
spin_unlock_irqrestore(&clock_lock, flags);
mutex_unlock(&clock_list_sem);
return 0;
}
> > +late_initcall(clk_late_init);
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH][RFC] drivers: sh: late disabling of clocks
2011-06-09 9:13 [PATCH][RFC] drivers: sh: late disabling of clocks Magnus Damm
2011-06-13 8:59 ` Simon Horman
2011-06-14 6:41 ` Simon Horman
@ 2011-06-15 14:21 ` Magnus Damm
2011-06-15 22:54 ` Simon Horman
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Magnus Damm @ 2011-06-15 14:21 UTC (permalink / raw)
To: linux-sh
On Tue, Jun 14, 2011 at 3:41 PM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
>> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
>> > From: Magnus Damm <damm@opensource.se>
>> >
>> > This RFC patch kills two birds with one stone:
>> >
>> > 1) Fix existing issue where in-use clocks without software
>> > reference are disabled during boot. One example of this
>> > is the handling of the Mackerel serial console output.
>> >
>> > 2) Make sure so far unused clocks actually get turned off
>>
>> Is access to allow_disable SMP-safe?
>> In particular, does the the access in clk_late_init()
>> need to be guarded by clock_lock as calls to __clk_disable() are?
>
> On reflection I think that the problem is a bit deeper.
>
> I think allow_disable is set to 1 prematurely, as it indicates
> to __clk_disable() that calling clk->ops->disable(clk) is allowed,
> even if the iteration of the loop in clk_late_init() is not complete.
>
> My proposed re-working of clk_late_init() is below.
> Completely untested.
Right, I was thinking something along those lines too. The spinlock
should make sure we're serialized.
Thanks,
/ magnus
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH][RFC] drivers: sh: late disabling of clocks
2011-06-09 9:13 [PATCH][RFC] drivers: sh: late disabling of clocks Magnus Damm
` (2 preceding siblings ...)
2011-06-15 14:21 ` Magnus Damm
@ 2011-06-15 22:54 ` Simon Horman
2011-06-21 2:14 ` Simon Horman
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Simon Horman @ 2011-06-15 22:54 UTC (permalink / raw)
To: linux-sh
On Wed, Jun 15, 2011 at 11:21:40PM +0900, Magnus Damm wrote:
> On Tue, Jun 14, 2011 at 3:41 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
> >> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
> >> > From: Magnus Damm <damm@opensource.se>
> >> >
> >> > This RFC patch kills two birds with one stone:
> >> >
> >> > 1) Fix existing issue where in-use clocks without software
> >> > reference are disabled during boot. One example of this
> >> > is the handling of the Mackerel serial console output.
> >> >
> >> > 2) Make sure so far unused clocks actually get turned off
> >>
> >> Is access to allow_disable SMP-safe?
> >> In particular, does the the access in clk_late_init()
> >> need to be guarded by clock_lock as calls to __clk_disable() are?
> >
> > On reflection I think that the problem is a bit deeper.
> >
> > I think allow_disable is set to 1 prematurely, as it indicates
> > to __clk_disable() that calling clk->ops->disable(clk) is allowed,
> > even if the iteration of the loop in clk_late_init() is not complete.
> >
> > My proposed re-working of clk_late_init() is below.
> > Completely untested.
>
> Right, I was thinking something along those lines too. The spinlock
> should make sure we're serialized.
Great. Feel free to take my ideas verbatim if it helps.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH][RFC] drivers: sh: late disabling of clocks
2011-06-09 9:13 [PATCH][RFC] drivers: sh: late disabling of clocks Magnus Damm
` (3 preceding siblings ...)
2011-06-15 22:54 ` Simon Horman
@ 2011-06-21 2:14 ` Simon Horman
2011-06-21 2:29 ` Magnus Damm
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Simon Horman @ 2011-06-21 2:14 UTC (permalink / raw)
To: linux-sh
On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
> On Wed, Jun 15, 2011 at 11:21:40PM +0900, Magnus Damm wrote:
> > On Tue, Jun 14, 2011 at 3:41 PM, Simon Horman <horms@verge.net.au> wrote:
> > > On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
> > >> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
> > >> > From: Magnus Damm <damm@opensource.se>
> > >> >
> > >> > This RFC patch kills two birds with one stone:
> > >> >
> > >> > 1) Fix existing issue where in-use clocks without software
> > >> > reference are disabled during boot. One example of this
> > >> > is the handling of the Mackerel serial console output.
> > >> >
> > >> > 2) Make sure so far unused clocks actually get turned off
> > >>
> > >> Is access to allow_disable SMP-safe?
> > >> In particular, does the the access in clk_late_init()
> > >> need to be guarded by clock_lock as calls to __clk_disable() are?
> > >
> > > On reflection I think that the problem is a bit deeper.
> > >
> > > I think allow_disable is set to 1 prematurely, as it indicates
> > > to __clk_disable() that calling clk->ops->disable(clk) is allowed,
> > > even if the iteration of the loop in clk_late_init() is not complete.
> > >
> > > My proposed re-working of clk_late_init() is below.
> > > Completely untested.
> >
> > Right, I was thinking something along those lines too. The spinlock
> > should make sure we're serialized.
>
> Great. Feel free to take my ideas verbatim if it helps.
Hi Magnus,
have you had time to look into this?
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH][RFC] drivers: sh: late disabling of clocks
2011-06-09 9:13 [PATCH][RFC] drivers: sh: late disabling of clocks Magnus Damm
` (4 preceding siblings ...)
2011-06-21 2:14 ` Simon Horman
@ 2011-06-21 2:29 ` Magnus Damm
2011-06-21 3:02 ` Simon Horman
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Magnus Damm @ 2011-06-21 2:29 UTC (permalink / raw)
To: linux-sh
On Tue, Jun 21, 2011 at 11:14 AM, Simon Horman <horms@verge.net.au> wrote:
> On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
>> On Wed, Jun 15, 2011 at 11:21:40PM +0900, Magnus Damm wrote:
>> > On Tue, Jun 14, 2011 at 3:41 PM, Simon Horman <horms@verge.net.au> wrote:
>> > > On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
>> > >> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
>> > >> > From: Magnus Damm <damm@opensource.se>
>> > >> >
>> > >> > This RFC patch kills two birds with one stone:
>> > >> >
>> > >> > 1) Fix existing issue where in-use clocks without software
>> > >> > reference are disabled during boot. One example of this
>> > >> > is the handling of the Mackerel serial console output.
>> > >> >
>> > >> > 2) Make sure so far unused clocks actually get turned off
>> > >>
>> > >> Is access to allow_disable SMP-safe?
>> > >> In particular, does the the access in clk_late_init()
>> > >> need to be guarded by clock_lock as calls to __clk_disable() are?
>> > >
>> > > On reflection I think that the problem is a bit deeper.
>> > >
>> > > I think allow_disable is set to 1 prematurely, as it indicates
>> > > to __clk_disable() that calling clk->ops->disable(clk) is allowed,
>> > > even if the iteration of the loop in clk_late_init() is not complete.
>> > >
>> > > My proposed re-working of clk_late_init() is below.
>> > > Completely untested.
>> >
>> > Right, I was thinking something along those lines too. The spinlock
>> > should make sure we're serialized.
>>
>> Great. Feel free to take my ideas verbatim if it helps.
>
> Hi Magnus,
>
> have you had time to look into this?
I have not updated this patch yet, mainly due to lack of feedback. Not
so much due to lack of feedback from you - I appreciate your ideas and
I agree that your rework of the ordering is correct. I'm however still
not sure if other people agree with my approach of delaying the
disable operations until a certain point in time.
Thanks,
/ magnus
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH][RFC] drivers: sh: late disabling of clocks
2011-06-09 9:13 [PATCH][RFC] drivers: sh: late disabling of clocks Magnus Damm
` (5 preceding siblings ...)
2011-06-21 2:29 ` Magnus Damm
@ 2011-06-21 3:02 ` Simon Horman
2011-06-21 5:27 ` Paul Mundt
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Simon Horman @ 2011-06-21 3:02 UTC (permalink / raw)
To: linux-sh
On Tue, Jun 21, 2011 at 11:29:31AM +0900, Magnus Damm wrote:
> On Tue, Jun 21, 2011 at 11:14 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
> >> On Wed, Jun 15, 2011 at 11:21:40PM +0900, Magnus Damm wrote:
> >> > On Tue, Jun 14, 2011 at 3:41 PM, Simon Horman <horms@verge.net.au> wrote:
> >> > > On Mon, Jun 13, 2011 at 05:59:08PM +0900, Simon Horman wrote:
> >> > >> On Thu, Jun 09, 2011 at 06:13:39PM +0900, Magnus Damm wrote:
> >> > >> > From: Magnus Damm <damm@opensource.se>
> >> > >> >
> >> > >> > This RFC patch kills two birds with one stone:
> >> > >> >
> >> > >> > 1) Fix existing issue where in-use clocks without software
> >> > >> > reference are disabled during boot. One example of this
> >> > >> > is the handling of the Mackerel serial console output.
> >> > >> >
> >> > >> > 2) Make sure so far unused clocks actually get turned off
> >> > >>
> >> > >> Is access to allow_disable SMP-safe?
> >> > >> In particular, does the the access in clk_late_init()
> >> > >> need to be guarded by clock_lock as calls to __clk_disable() are?
> >> > >
> >> > > On reflection I think that the problem is a bit deeper.
> >> > >
> >> > > I think allow_disable is set to 1 prematurely, as it indicates
> >> > > to __clk_disable() that calling clk->ops->disable(clk) is allowed,
> >> > > even if the iteration of the loop in clk_late_init() is not complete.
> >> > >
> >> > > My proposed re-working of clk_late_init() is below.
> >> > > Completely untested.
> >> >
> >> > Right, I was thinking something along those lines too. The spinlock
> >> > should make sure we're serialized.
> >>
> >> Great. Feel free to take my ideas verbatim if it helps.
> >
> > Hi Magnus,
> >
> > have you had time to look into this?
>
> I have not updated this patch yet, mainly due to lack of feedback. Not
> so much due to lack of feedback from you - I appreciate your ideas and
> I agree that your rework of the ordering is correct. I'm however still
> not sure if other people agree with my approach of delaying the
> disable operations until a certain point in time.
Understood.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH][RFC] drivers: sh: late disabling of clocks
2011-06-09 9:13 [PATCH][RFC] drivers: sh: late disabling of clocks Magnus Damm
` (6 preceding siblings ...)
2011-06-21 3:02 ` Simon Horman
@ 2011-06-21 5:27 ` Paul Mundt
2011-06-21 5:48 ` Magnus Damm
2011-06-21 6:39 ` Paul Mundt
9 siblings, 0 replies; 11+ messages in thread
From: Paul Mundt @ 2011-06-21 5:27 UTC (permalink / raw)
To: linux-sh
On Tue, Jun 21, 2011 at 11:29:31AM +0900, Magnus Damm wrote:
> On Tue, Jun 21, 2011 at 11:14 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
> >> > Right, I was thinking something along those lines too. The spinlock
> >> > should make sure we're serialized.
> >>
> >> Great. Feel free to take my ideas verbatim if it helps.
> >
> > Hi Magnus,
> >
> > have you had time to look into this?
>
> I have not updated this patch yet, mainly due to lack of feedback. Not
> so much due to lack of feedback from you - I appreciate your ideas and
> I agree that your rework of the ordering is correct. I'm however still
> not sure if other people agree with my approach of delaying the
> disable operations until a certain point in time.
>
Except that's not really how it works. I don't have the time or
inclination to go through and provide feedback for every single patch
that crosses my inbox. If something has been posted for awhile and has
feedback, then it's expected that the feedback is addressed and an
updated version is posted. If no other action happens within an arbitrary
but not too long period of time, I'll then roll it in to one of my trees
for testing (assuming there are no glaring issues with the concept or
implementation prior to integration/testing). You've posted multiple
versions of the same patches in the past upon receiving feedback, so it's
a bit perplexing as to why this time you've simply opted to throw the
patch over the fence and do nothing.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH][RFC] drivers: sh: late disabling of clocks
2011-06-09 9:13 [PATCH][RFC] drivers: sh: late disabling of clocks Magnus Damm
` (7 preceding siblings ...)
2011-06-21 5:27 ` Paul Mundt
@ 2011-06-21 5:48 ` Magnus Damm
2011-06-21 6:39 ` Paul Mundt
9 siblings, 0 replies; 11+ messages in thread
From: Magnus Damm @ 2011-06-21 5:48 UTC (permalink / raw)
To: linux-sh
On Tue, Jun 21, 2011 at 2:27 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Tue, Jun 21, 2011 at 11:29:31AM +0900, Magnus Damm wrote:
>> On Tue, Jun 21, 2011 at 11:14 AM, Simon Horman <horms@verge.net.au> wrote:
>> > On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
>> >> > Right, I was thinking something along those lines too. The spinlock
>> >> > should make sure we're serialized.
>> >>
>> >> Great. Feel free to take my ideas verbatim if it helps.
>> >
>> > Hi Magnus,
>> >
>> > have you had time to look into this?
>>
>> I have not updated this patch yet, mainly due to lack of feedback. Not
>> so much due to lack of feedback from you - I appreciate your ideas and
>> I agree that your rework of the ordering is correct. I'm however still
>> not sure if other people agree with my approach of delaying the
>> disable operations until a certain point in time.
>>
> Except that's not really how it works. I don't have the time or
> inclination to go through and provide feedback for every single patch
> that crosses my inbox. If something has been posted for awhile and has
> feedback, then it's expected that the feedback is addressed and an
> updated version is posted. If no other action happens within an arbitrary
> but not too long period of time, I'll then roll it in to one of my trees
> for testing (assuming there are no glaring issues with the concept or
> implementation prior to integration/testing). You've posted multiple
> versions of the same patches in the past upon receiving feedback, so it's
> a bit perplexing as to why this time you've simply opted to throw the
> patch over the fence and do nothing.
I believe I received some feedback through face-to-face conversations
earlier, and that feedback was not all that positive. I decided to
post the code anyway, and Simon kindly provided detailed feedback via
email about internal ordering. The main idea behind the patch is not
really changed, so in that area there still is silence.
I'll resend a new version in a little while. Thanks.
/ magnus
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH][RFC] drivers: sh: late disabling of clocks
2011-06-09 9:13 [PATCH][RFC] drivers: sh: late disabling of clocks Magnus Damm
` (8 preceding siblings ...)
2011-06-21 5:48 ` Magnus Damm
@ 2011-06-21 6:39 ` Paul Mundt
9 siblings, 0 replies; 11+ messages in thread
From: Paul Mundt @ 2011-06-21 6:39 UTC (permalink / raw)
To: linux-sh
On Tue, Jun 21, 2011 at 02:48:44PM +0900, Magnus Damm wrote:
> On Tue, Jun 21, 2011 at 2:27 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> > On Tue, Jun 21, 2011 at 11:29:31AM +0900, Magnus Damm wrote:
> >> On Tue, Jun 21, 2011 at 11:14 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > On Thu, Jun 16, 2011 at 07:54:38AM +0900, Simon Horman wrote:
> >> >> > Right, I was thinking something along those lines too. The spinlock
> >> >> > should make sure we're serialized.
> >> >>
> >> >> Great. Feel free to take my ideas verbatim if it helps.
> >> >
> >> > Hi Magnus,
> >> >
> >> > have you had time to look into this?
> >>
> >> I have not updated this patch yet, mainly due to lack of feedback. Not
> >> so much due to lack of feedback from you - I appreciate your ideas and
> >> I agree that your rework of the ordering is correct. I'm however still
> >> not sure if other people agree with my approach of delaying the
> >> disable operations until a certain point in time.
> >>
> > Except that's not really how it works. I don't have the time or
> > inclination to go through and provide feedback for every single patch
> > that crosses my inbox. If something has been posted for awhile and has
> > feedback, then it's expected that the feedback is addressed and an
> > updated version is posted. If no other action happens within an arbitrary
> > but not too long period of time, I'll then roll it in to one of my trees
> > for testing (assuming there are no glaring issues with the concept or
> > implementation prior to integration/testing). You've posted multiple
> > versions of the same patches in the past upon receiving feedback, so it's
> > a bit perplexing as to why this time you've simply opted to throw the
> > patch over the fence and do nothing.
>
> I believe I received some feedback through face-to-face conversations
> earlier, and that feedback was not all that positive. I decided to
> post the code anyway, and Simon kindly provided detailed feedback via
> email about internal ordering. The main idea behind the patch is not
> really changed, so in that area there still is silence.
>
> I'll resend a new version in a little while. Thanks.
>
I don't have any particular problems with the approach at least, and at
this point it's really just the locking stuff pointed out by Simon that
is keeping it from being applied. It's obviously somewhat of a hack and
I'm certainly not thrilled by it, but the fact we're forced in to this
position and are entering with undefined state is certainly not your
fault, nor something we can really do much about. We also have half a
dozen syscall ABIs for no good reason because of hardware designer
muppetry, such is life.
Having said that, it might need a bit of fiddling in terms of deciding on
how to enable it or present it to the user, but those are fairly
superficial details that can be worked out once an updated version is
posted.
^ permalink raw reply [flat|nested] 11+ messages in thread