diff for duplicates of <20150730233530.23791.17746@quantum> diff --git a/a/1.txt b/N1/1.txt index f1ad345..1cc5246 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,63 +1,44 @@ Quoting Lee Jones (2015-07-30 04:17:47) > On Wed, 29 Jul 2015, Michael Turquette wrote: -> = - +> > > Hi Lee, -> > = - +> > > > + linux-clk ml -> > = - +> > > > Quoting Lee Jones (2015-07-22 06:04:13) -> > > These new API calls will firstly provide a mechanisms to tag a clock = -as -> > > critical and secondly allow any knowledgeable driver to (un)gate cloc= -ks, +> > > These new API calls will firstly provide a mechanisms to tag a clock as +> > > critical and secondly allow any knowledgeable driver to (un)gate clocks, > > > even if they are marked as critical. -> > > = - +> > > > > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > --- -> > > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++++++++= -++++++++++ +> > > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/clk-provider.h | 2 ++ > > > include/linux/clk.h | 30 +++++++++++++++++++++++++++++ > > > 3 files changed, 77 insertions(+) -> > > = - +> > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > index 61c3fc5..486b1da 100644 > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c -> > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char= - *name); -> > > = - +> > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name); +> > > > > > /*** private data structures ***/ -> > > = - +> > > > > > +/** -> > > + * struct critical - Provides 'play' over critical clocks. A cloc= -k can be -> > > + * marked as critical, meaning that it should no= -t be -> > > + * disabled. However, if a driver which is awar= -e of the -> > > + * critical behaviour wants to control it, it ca= -n do so -> > > + * using clk_enable_critical() and clk_disable_c= -ritical(). +> > > + * struct critical - Provides 'play' over critical clocks. A clock can be +> > > + * marked as critical, meaning that it should not be +> > > + * disabled. However, if a driver which is aware of the +> > > + * critical behaviour wants to control it, it can do so +> > > + * using clk_enable_critical() and clk_disable_critical(). > > > + * > > > + * @enabled Is clock critical? Once set, doesn't change -> > > + * @leave_on Self explanatory. Can be disabled by knowledgeable d= -rivers -> > = - +> > > + * @leave_on Self explanatory. Can be disabled by knowledgeable drivers +> > > > Not self explanatory. I need this explained to me. What does leave_on > > do? Better yet, what would happen if leave_on did not exist? -> > = - +> > > > > + */ > > > +struct critical { > > > + bool enabled; @@ -73,39 +54,31 @@ rivers > > > struct kref ref; > > > + struct critical critical; > > > }; -> > > = - +> > > > > > struct clk { -> > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *c= -lk) -> > > if (WARN_ON(clk->enable_count =3D=3D 0)) +> > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk) +> > > if (WARN_ON(clk->enable_count == 0)) > > > return; -> > > = - +> > > > > > + /* Refuse to turn off a critical clock */ -> > > + if (clk->enable_count =3D=3D 1 && clk->critical.leave_on) +> > > + if (clk->enable_count == 1 && clk->critical.leave_on) > > > + return; -> > = - +> > > > How do we get to this point? clk_enable_critical actually calls > > clk_enable, thus incrementing the enable_count. The only time that we > > could hit the above case is if, -> > = - +> > > > a) there is an imbalance in clk_enable and clk_disable calls. If this is > > the case then the drivers need to be fixed. Or better yet some > > infrastructure to catch that, now that we have per-user struct clk > > cookies. -> > = - +> > > > b) a driver knowingly calls clk_enable_critical(foo) and then regular, > > old clk_disable(foo). But why would a driver do that? -> > = - +> > > > It might be that I am missing the point here, so please feel free to > > clue me in. -> = - +> > This check behaves in a very similar to the WARN() above. It's more > of a fail-safe. If all drivers are behaving properly, then it > shouldn't ever be true. If they're not, it prevents an incorrectly @@ -115,8 +88,7 @@ Then this check should be replaced with a generic approach that refuses to honor imbalances anyways. Below are two patches that probably resolve the issue of badly behaving drivers that cause enable imbalances. -> = - +> > As I said in the other mail. We can do without these 3 new wrappers. > We _could_ just write a driver which only calls clk_enable() _after_ > it calls clk_disable(), a kind of intentional unbalance and it would @@ -136,7 +108,7 @@ Mike -From=203599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001 +>From 3599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001 From: Michael Turquette <mturquette@baylibre.com> Date: Wed, 29 Jul 2015 18:22:45 -0700 Subject: [PATCH 1/2] clk: per-user clk prepare & enable ref counts @@ -162,15 +134,13 @@ index 898052e..72feee9 100644 + unsigned int enable_count; + unsigned int prepare_count; }; - = - + /*** locking ***/ @@ -600,6 +602,9 @@ void clk_unprepare(struct clk *clk) return; - = - + clk_prepare_lock(); -+ if (WARN_ON(clk->prepare_count =3D=3D 0)) ++ if (WARN_ON(clk->prepare_count == 0)) + return; + clk->prepare_count--; clk_core_unprepare(clk->core); @@ -178,20 +148,17 @@ index 898052e..72feee9 100644 } @@ -657,6 +662,7 @@ int clk_prepare(struct clk *clk) return 0; - = - + clk_prepare_lock(); + clk->prepare_count++; - ret =3D clk_core_prepare(clk->core); + ret = clk_core_prepare(clk->core); clk_prepare_unlock(); - = - + @@ -707,6 +713,9 @@ void clk_disable(struct clk *clk) return; - = - - flags =3D clk_enable_lock(); -+ if (WARN_ON(clk->enable_count =3D=3D 0)) + + flags = clk_enable_lock(); ++ if (WARN_ON(clk->enable_count == 0)) + return; + clk->enable_count--; clk_core_disable(clk->core); @@ -199,24 +166,21 @@ index 898052e..72feee9 100644 } @@ -769,6 +778,7 @@ int clk_enable(struct clk *clk) return 0; - = - - flags =3D clk_enable_lock(); + + flags = clk_enable_lock(); + clk->enable_count++; - ret =3D clk_core_enable(clk->core); + ret = clk_core_enable(clk->core); clk_enable_unlock(flags); - = - --- = - + +-- 1.9.1 -From=20ace76f6ed634a69c499f8440a98d4b5a54d78368 Mon Sep 17 00:00:00 2001 +>From ace76f6ed634a69c499f8440a98d4b5a54d78368 Mon Sep 17 00:00:00 2001 From: Michael Turquette <mturquette@baylibre.com> Date: Thu, 30 Jul 2015 12:52:26 -0700 Subject: [PATCH 2/2] clk: clk_put WARNs if user has not disabled clk -From=20the clk_put kerneldoc in include/linux/clk.h: +>From the clk_put kerneldoc in include/linux/clk.h: """ Note: drivers must ensure that all clk_enable calls made on this clock @@ -244,8 +208,7 @@ index 72feee9..6ec0f77 100644 @@ -2764,6 +2764,14 @@ void __clk_put(struct clk *clk) clk->max_rate < clk->core->req_rate) clk_core_set_rate_nolock(clk->core, clk->core->req_rate); - = - + + /* + * before calling clk_put, all calls to clk_prepare and clk_enable from + * a given user must be balanced with calls to clk_disable and @@ -254,10 +217,8 @@ index 72feee9..6ec0f77 100644 + WARN_ON(clk->prepare_count); + WARN_ON(clk->enable_count); + - owner =3D clk->core->owner; + owner = clk->core->owner; kref_put(&clk->core->ref, __clk_release); - = - --- = - + +-- 1.9.1 diff --git a/a/content_digest b/N1/content_digest index f287ca6..f2b0fe5 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -2,82 +2,53 @@ "ref\01437570255-21049-4-git-send-email-lee.jones@linaro.org\0" "ref\020150730010213.642.10831@quantum\0" "ref\020150730111747.GF14642@x1\0" - "From\0Michael Turquette <mturquette@linaro.org>\0" - "Subject\0Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework\0" + "From\0mturquette@linaro.org (Michael Turquette)\0" + "Subject\0[PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework\0" "Date\0Thu, 30 Jul 2015 16:35:30 -0700\0" - "To\0Lee Jones <lee.jones@linaro.org>" - "\0" - "Cc\0linux-arm-kernel@lists.infradead.org" - linux-kernel@vger.kernel.org - kernel@stlinux.com - sboyd@codeaurora.org - devicetree@vger.kernel.org - geert@linux-m68k.org - maxime.ripard@free-electrons.com - s.hauer@pengutronix.de - " linux-clk@vger.kernel.org\0" + "To\0linux-arm-kernel@lists.infradead.org\0" "\00:1\0" "b\0" "Quoting Lee Jones (2015-07-30 04:17:47)\n" "> On Wed, 29 Jul 2015, Michael Turquette wrote:\n" - "> =\n" - "\n" + "> \n" "> > Hi Lee,\n" - "> > =\n" - "\n" + "> > \n" "> > + linux-clk ml\n" - "> > =\n" - "\n" + "> > \n" "> > Quoting Lee Jones (2015-07-22 06:04:13)\n" - "> > > These new API calls will firstly provide a mechanisms to tag a clock =\n" - "as\n" - "> > > critical and secondly allow any knowledgeable driver to (un)gate cloc=\n" - "ks,\n" + "> > > These new API calls will firstly provide a mechanisms to tag a clock as\n" + "> > > critical and secondly allow any knowledgeable driver to (un)gate clocks,\n" "> > > even if they are marked as critical.\n" - "> > > =\n" - "\n" + "> > > \n" "> > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>\n" "> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>\n" "> > > ---\n" - "> > > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++++++++=\n" - "++++++++++\n" + "> > > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++++++++++++++++++\n" "> > > include/linux/clk-provider.h | 2 ++\n" "> > > include/linux/clk.h | 30 +++++++++++++++++++++++++++++\n" "> > > 3 files changed, 77 insertions(+)\n" - "> > > =\n" - "\n" + "> > > \n" "> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c\n" "> > > index 61c3fc5..486b1da 100644\n" "> > > --- a/drivers/clk/clk.c\n" "> > > +++ b/drivers/clk/clk.c\n" - "> > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char=\n" - " *name);\n" - "> > > =\n" - "\n" + "> > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);\n" + "> > > \n" "> > > /*** private data structures ***/\n" - "> > > =\n" - "\n" + "> > > \n" "> > > +/**\n" - "> > > + * struct critical - Provides 'play' over critical clocks. A cloc=\n" - "k can be\n" - "> > > + * marked as critical, meaning that it should no=\n" - "t be\n" - "> > > + * disabled. However, if a driver which is awar=\n" - "e of the\n" - "> > > + * critical behaviour wants to control it, it ca=\n" - "n do so\n" - "> > > + * using clk_enable_critical() and clk_disable_c=\n" - "ritical().\n" + "> > > + * struct critical - Provides 'play' over critical clocks. A clock can be\n" + "> > > + * marked as critical, meaning that it should not be\n" + "> > > + * disabled. However, if a driver which is aware of the\n" + "> > > + * critical behaviour wants to control it, it can do so\n" + "> > > + * using clk_enable_critical() and clk_disable_critical().\n" "> > > + *\n" "> > > + * @enabled Is clock critical? Once set, doesn't change\n" - "> > > + * @leave_on Self explanatory. Can be disabled by knowledgeable d=\n" - "rivers\n" - "> > =\n" - "\n" + "> > > + * @leave_on Self explanatory. Can be disabled by knowledgeable drivers\n" + "> > \n" "> > Not self explanatory. I need this explained to me. What does leave_on\n" "> > do? Better yet, what would happen if leave_on did not exist?\n" - "> > =\n" - "\n" + "> > \n" "> > > + */\n" "> > > +struct critical {\n" "> > > + bool enabled;\n" @@ -93,39 +64,31 @@ "> > > struct kref ref;\n" "> > > + struct critical critical;\n" "> > > };\n" - "> > > =\n" - "\n" + "> > > \n" "> > > struct clk {\n" - "> > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *c=\n" - "lk)\n" - "> > > if (WARN_ON(clk->enable_count =3D=3D 0))\n" + "> > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)\n" + "> > > if (WARN_ON(clk->enable_count == 0))\n" "> > > return;\n" - "> > > =\n" - "\n" + "> > > \n" "> > > + /* Refuse to turn off a critical clock */\n" - "> > > + if (clk->enable_count =3D=3D 1 && clk->critical.leave_on)\n" + "> > > + if (clk->enable_count == 1 && clk->critical.leave_on)\n" "> > > + return;\n" - "> > =\n" - "\n" + "> > \n" "> > How do we get to this point? clk_enable_critical actually calls\n" "> > clk_enable, thus incrementing the enable_count. The only time that we\n" "> > could hit the above case is if,\n" - "> > =\n" - "\n" + "> > \n" "> > a) there is an imbalance in clk_enable and clk_disable calls. If this is\n" "> > the case then the drivers need to be fixed. Or better yet some\n" "> > infrastructure to catch that, now that we have per-user struct clk\n" "> > cookies.\n" - "> > =\n" - "\n" + "> > \n" "> > b) a driver knowingly calls clk_enable_critical(foo) and then regular,\n" "> > old clk_disable(foo). But why would a driver do that?\n" - "> > =\n" - "\n" + "> > \n" "> > It might be that I am missing the point here, so please feel free to\n" "> > clue me in.\n" - "> =\n" - "\n" + "> \n" "> This check behaves in a very similar to the WARN() above. It's more\n" "> of a fail-safe. If all drivers are behaving properly, then it\n" "> shouldn't ever be true. If they're not, it prevents an incorrectly\n" @@ -135,8 +98,7 @@ "to honor imbalances anyways. Below are two patches that probably resolve\n" "the issue of badly behaving drivers that cause enable imbalances.\n" "\n" - "> =\n" - "\n" + "> \n" "> As I said in the other mail. We can do without these 3 new wrappers.\n" "> We _could_ just write a driver which only calls clk_enable() _after_\n" "> it calls clk_disable(), a kind of intentional unbalance and it would\n" @@ -156,7 +118,7 @@ "\n" "\n" "\n" - "From=203599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001\n" + ">From 3599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001\n" "From: Michael Turquette <mturquette@baylibre.com>\n" "Date: Wed, 29 Jul 2015 18:22:45 -0700\n" "Subject: [PATCH 1/2] clk: per-user clk prepare & enable ref counts\n" @@ -182,15 +144,13 @@ "+\tunsigned int enable_count;\n" "+\tunsigned int prepare_count;\n" " };\n" - " =\n" - "\n" + " \n" " /*** locking ***/\n" "@@ -600,6 +602,9 @@ void clk_unprepare(struct clk *clk)\n" " \t\treturn;\n" - " =\n" - "\n" + " \n" " \tclk_prepare_lock();\n" - "+\tif (WARN_ON(clk->prepare_count =3D=3D 0))\n" + "+\tif (WARN_ON(clk->prepare_count == 0))\n" "+\t\treturn;\n" "+\tclk->prepare_count--;\n" " \tclk_core_unprepare(clk->core);\n" @@ -198,20 +158,17 @@ " }\n" "@@ -657,6 +662,7 @@ int clk_prepare(struct clk *clk)\n" " \t\treturn 0;\n" - " =\n" - "\n" + " \n" " \tclk_prepare_lock();\n" "+\tclk->prepare_count++;\n" - " \tret =3D clk_core_prepare(clk->core);\n" + " \tret = clk_core_prepare(clk->core);\n" " \tclk_prepare_unlock();\n" - " =\n" - "\n" + " \n" "@@ -707,6 +713,9 @@ void clk_disable(struct clk *clk)\n" " \t\treturn;\n" - " =\n" - "\n" - " \tflags =3D clk_enable_lock();\n" - "+\tif (WARN_ON(clk->enable_count =3D=3D 0))\n" + " \n" + " \tflags = clk_enable_lock();\n" + "+\tif (WARN_ON(clk->enable_count == 0))\n" "+\t\treturn;\n" "+\tclk->enable_count--;\n" " \tclk_core_disable(clk->core);\n" @@ -219,24 +176,21 @@ " }\n" "@@ -769,6 +778,7 @@ int clk_enable(struct clk *clk)\n" " \t\treturn 0;\n" - " =\n" - "\n" - " \tflags =3D clk_enable_lock();\n" + " \n" + " \tflags = clk_enable_lock();\n" "+\tclk->enable_count++;\n" - " \tret =3D clk_core_enable(clk->core);\n" + " \tret = clk_core_enable(clk->core);\n" " \tclk_enable_unlock(flags);\n" - " =\n" - "\n" - "-- =\n" - "\n" + " \n" + "-- \n" "1.9.1\n" "\n" - "From=20ace76f6ed634a69c499f8440a98d4b5a54d78368 Mon Sep 17 00:00:00 2001\n" + ">From ace76f6ed634a69c499f8440a98d4b5a54d78368 Mon Sep 17 00:00:00 2001\n" "From: Michael Turquette <mturquette@baylibre.com>\n" "Date: Thu, 30 Jul 2015 12:52:26 -0700\n" "Subject: [PATCH 2/2] clk: clk_put WARNs if user has not disabled clk\n" "\n" - "From=20the clk_put kerneldoc in include/linux/clk.h:\n" + ">From the clk_put kerneldoc in include/linux/clk.h:\n" "\n" "\"\"\"\n" "Note: drivers must ensure that all clk_enable calls made on this clock\n" @@ -264,8 +218,7 @@ "@@ -2764,6 +2764,14 @@ void __clk_put(struct clk *clk)\n" " \t clk->max_rate < clk->core->req_rate)\n" " \t\tclk_core_set_rate_nolock(clk->core, clk->core->req_rate);\n" - " =\n" - "\n" + " \n" "+\t/*\n" "+\t * before calling clk_put, all calls to clk_prepare and clk_enable from\n" "+\t * a given user must be balanced with calls to clk_disable and\n" @@ -274,12 +227,10 @@ "+\tWARN_ON(clk->prepare_count);\n" "+\tWARN_ON(clk->enable_count);\n" "+\n" - " \towner =3D clk->core->owner;\n" + " \towner = clk->core->owner;\n" " \tkref_put(&clk->core->ref, __clk_release);\n" - " =\n" - "\n" - "-- =\n" - "\n" + " \n" + "-- \n" 1.9.1 -81e804f26cd6ea7034bc654cfd1c6029cada808ea499f5b67e9d2ac2b158f418 +dbf1854be23354b1b6b2083896e4b889f3df38938d22e759545e6ef82f43b037
diff --git a/a/1.txt b/N2/1.txt index f1ad345..1cc5246 100644 --- a/a/1.txt +++ b/N2/1.txt @@ -1,63 +1,44 @@ Quoting Lee Jones (2015-07-30 04:17:47) > On Wed, 29 Jul 2015, Michael Turquette wrote: -> = - +> > > Hi Lee, -> > = - +> > > > + linux-clk ml -> > = - +> > > > Quoting Lee Jones (2015-07-22 06:04:13) -> > > These new API calls will firstly provide a mechanisms to tag a clock = -as -> > > critical and secondly allow any knowledgeable driver to (un)gate cloc= -ks, +> > > These new API calls will firstly provide a mechanisms to tag a clock as +> > > critical and secondly allow any knowledgeable driver to (un)gate clocks, > > > even if they are marked as critical. -> > > = - +> > > > > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > --- -> > > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++++++++= -++++++++++ +> > > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/clk-provider.h | 2 ++ > > > include/linux/clk.h | 30 +++++++++++++++++++++++++++++ > > > 3 files changed, 77 insertions(+) -> > > = - +> > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > index 61c3fc5..486b1da 100644 > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c -> > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char= - *name); -> > > = - +> > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name); +> > > > > > /*** private data structures ***/ -> > > = - +> > > > > > +/** -> > > + * struct critical - Provides 'play' over critical clocks. A cloc= -k can be -> > > + * marked as critical, meaning that it should no= -t be -> > > + * disabled. However, if a driver which is awar= -e of the -> > > + * critical behaviour wants to control it, it ca= -n do so -> > > + * using clk_enable_critical() and clk_disable_c= -ritical(). +> > > + * struct critical - Provides 'play' over critical clocks. A clock can be +> > > + * marked as critical, meaning that it should not be +> > > + * disabled. However, if a driver which is aware of the +> > > + * critical behaviour wants to control it, it can do so +> > > + * using clk_enable_critical() and clk_disable_critical(). > > > + * > > > + * @enabled Is clock critical? Once set, doesn't change -> > > + * @leave_on Self explanatory. Can be disabled by knowledgeable d= -rivers -> > = - +> > > + * @leave_on Self explanatory. Can be disabled by knowledgeable drivers +> > > > Not self explanatory. I need this explained to me. What does leave_on > > do? Better yet, what would happen if leave_on did not exist? -> > = - +> > > > > + */ > > > +struct critical { > > > + bool enabled; @@ -73,39 +54,31 @@ rivers > > > struct kref ref; > > > + struct critical critical; > > > }; -> > > = - +> > > > > > struct clk { -> > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *c= -lk) -> > > if (WARN_ON(clk->enable_count =3D=3D 0)) +> > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk) +> > > if (WARN_ON(clk->enable_count == 0)) > > > return; -> > > = - +> > > > > > + /* Refuse to turn off a critical clock */ -> > > + if (clk->enable_count =3D=3D 1 && clk->critical.leave_on) +> > > + if (clk->enable_count == 1 && clk->critical.leave_on) > > > + return; -> > = - +> > > > How do we get to this point? clk_enable_critical actually calls > > clk_enable, thus incrementing the enable_count. The only time that we > > could hit the above case is if, -> > = - +> > > > a) there is an imbalance in clk_enable and clk_disable calls. If this is > > the case then the drivers need to be fixed. Or better yet some > > infrastructure to catch that, now that we have per-user struct clk > > cookies. -> > = - +> > > > b) a driver knowingly calls clk_enable_critical(foo) and then regular, > > old clk_disable(foo). But why would a driver do that? -> > = - +> > > > It might be that I am missing the point here, so please feel free to > > clue me in. -> = - +> > This check behaves in a very similar to the WARN() above. It's more > of a fail-safe. If all drivers are behaving properly, then it > shouldn't ever be true. If they're not, it prevents an incorrectly @@ -115,8 +88,7 @@ Then this check should be replaced with a generic approach that refuses to honor imbalances anyways. Below are two patches that probably resolve the issue of badly behaving drivers that cause enable imbalances. -> = - +> > As I said in the other mail. We can do without these 3 new wrappers. > We _could_ just write a driver which only calls clk_enable() _after_ > it calls clk_disable(), a kind of intentional unbalance and it would @@ -136,7 +108,7 @@ Mike -From=203599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001 +>From 3599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001 From: Michael Turquette <mturquette@baylibre.com> Date: Wed, 29 Jul 2015 18:22:45 -0700 Subject: [PATCH 1/2] clk: per-user clk prepare & enable ref counts @@ -162,15 +134,13 @@ index 898052e..72feee9 100644 + unsigned int enable_count; + unsigned int prepare_count; }; - = - + /*** locking ***/ @@ -600,6 +602,9 @@ void clk_unprepare(struct clk *clk) return; - = - + clk_prepare_lock(); -+ if (WARN_ON(clk->prepare_count =3D=3D 0)) ++ if (WARN_ON(clk->prepare_count == 0)) + return; + clk->prepare_count--; clk_core_unprepare(clk->core); @@ -178,20 +148,17 @@ index 898052e..72feee9 100644 } @@ -657,6 +662,7 @@ int clk_prepare(struct clk *clk) return 0; - = - + clk_prepare_lock(); + clk->prepare_count++; - ret =3D clk_core_prepare(clk->core); + ret = clk_core_prepare(clk->core); clk_prepare_unlock(); - = - + @@ -707,6 +713,9 @@ void clk_disable(struct clk *clk) return; - = - - flags =3D clk_enable_lock(); -+ if (WARN_ON(clk->enable_count =3D=3D 0)) + + flags = clk_enable_lock(); ++ if (WARN_ON(clk->enable_count == 0)) + return; + clk->enable_count--; clk_core_disable(clk->core); @@ -199,24 +166,21 @@ index 898052e..72feee9 100644 } @@ -769,6 +778,7 @@ int clk_enable(struct clk *clk) return 0; - = - - flags =3D clk_enable_lock(); + + flags = clk_enable_lock(); + clk->enable_count++; - ret =3D clk_core_enable(clk->core); + ret = clk_core_enable(clk->core); clk_enable_unlock(flags); - = - --- = - + +-- 1.9.1 -From=20ace76f6ed634a69c499f8440a98d4b5a54d78368 Mon Sep 17 00:00:00 2001 +>From ace76f6ed634a69c499f8440a98d4b5a54d78368 Mon Sep 17 00:00:00 2001 From: Michael Turquette <mturquette@baylibre.com> Date: Thu, 30 Jul 2015 12:52:26 -0700 Subject: [PATCH 2/2] clk: clk_put WARNs if user has not disabled clk -From=20the clk_put kerneldoc in include/linux/clk.h: +>From the clk_put kerneldoc in include/linux/clk.h: """ Note: drivers must ensure that all clk_enable calls made on this clock @@ -244,8 +208,7 @@ index 72feee9..6ec0f77 100644 @@ -2764,6 +2764,14 @@ void __clk_put(struct clk *clk) clk->max_rate < clk->core->req_rate) clk_core_set_rate_nolock(clk->core, clk->core->req_rate); - = - + + /* + * before calling clk_put, all calls to clk_prepare and clk_enable from + * a given user must be balanced with calls to clk_disable and @@ -254,10 +217,8 @@ index 72feee9..6ec0f77 100644 + WARN_ON(clk->prepare_count); + WARN_ON(clk->enable_count); + - owner =3D clk->core->owner; + owner = clk->core->owner; kref_put(&clk->core->ref, __clk_release); - = - --- = - + +-- 1.9.1 diff --git a/a/content_digest b/N2/content_digest index f287ca6..480d94b 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -5,79 +5,59 @@ "From\0Michael Turquette <mturquette@linaro.org>\0" "Subject\0Re: [PATCH v7 3/5] clk: Supply the critical clock {init, enable, disable} framework\0" "Date\0Thu, 30 Jul 2015 16:35:30 -0700\0" - "To\0Lee Jones <lee.jones@linaro.org>" - "\0" - "Cc\0linux-arm-kernel@lists.infradead.org" - linux-kernel@vger.kernel.org + "To\0Lee Jones <lee.jones@linaro.org>\0" + "Cc\0devicetree@vger.kernel.org" kernel@stlinux.com + s.hauer@pengutronix.de sboyd@codeaurora.org - devicetree@vger.kernel.org + linux-kernel@vger.kernel.org geert@linux-m68k.org maxime.ripard@free-electrons.com - s.hauer@pengutronix.de - " linux-clk@vger.kernel.org\0" + linux-clk@vger.kernel.org + " linux-arm-kernel@lists.infradead.org\0" "\00:1\0" "b\0" "Quoting Lee Jones (2015-07-30 04:17:47)\n" "> On Wed, 29 Jul 2015, Michael Turquette wrote:\n" - "> =\n" - "\n" + "> \n" "> > Hi Lee,\n" - "> > =\n" - "\n" + "> > \n" "> > + linux-clk ml\n" - "> > =\n" - "\n" + "> > \n" "> > Quoting Lee Jones (2015-07-22 06:04:13)\n" - "> > > These new API calls will firstly provide a mechanisms to tag a clock =\n" - "as\n" - "> > > critical and secondly allow any knowledgeable driver to (un)gate cloc=\n" - "ks,\n" + "> > > These new API calls will firstly provide a mechanisms to tag a clock as\n" + "> > > critical and secondly allow any knowledgeable driver to (un)gate clocks,\n" "> > > even if they are marked as critical.\n" - "> > > =\n" - "\n" + "> > > \n" "> > > Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>\n" "> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>\n" "> > > ---\n" - "> > > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++++++++=\n" - "++++++++++\n" + "> > > drivers/clk/clk.c | 45 ++++++++++++++++++++++++++++++++++++++++++++\n" "> > > include/linux/clk-provider.h | 2 ++\n" "> > > include/linux/clk.h | 30 +++++++++++++++++++++++++++++\n" "> > > 3 files changed, 77 insertions(+)\n" - "> > > =\n" - "\n" + "> > > \n" "> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c\n" "> > > index 61c3fc5..486b1da 100644\n" "> > > --- a/drivers/clk/clk.c\n" "> > > +++ b/drivers/clk/clk.c\n" - "> > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char=\n" - " *name);\n" - "> > > =\n" - "\n" + "> > > @@ -46,6 +46,21 @@ static struct clk_core *clk_core_lookup(const char *name);\n" + "> > > \n" "> > > /*** private data structures ***/\n" - "> > > =\n" - "\n" + "> > > \n" "> > > +/**\n" - "> > > + * struct critical - Provides 'play' over critical clocks. A cloc=\n" - "k can be\n" - "> > > + * marked as critical, meaning that it should no=\n" - "t be\n" - "> > > + * disabled. However, if a driver which is awar=\n" - "e of the\n" - "> > > + * critical behaviour wants to control it, it ca=\n" - "n do so\n" - "> > > + * using clk_enable_critical() and clk_disable_c=\n" - "ritical().\n" + "> > > + * struct critical - Provides 'play' over critical clocks. A clock can be\n" + "> > > + * marked as critical, meaning that it should not be\n" + "> > > + * disabled. However, if a driver which is aware of the\n" + "> > > + * critical behaviour wants to control it, it can do so\n" + "> > > + * using clk_enable_critical() and clk_disable_critical().\n" "> > > + *\n" "> > > + * @enabled Is clock critical? Once set, doesn't change\n" - "> > > + * @leave_on Self explanatory. Can be disabled by knowledgeable d=\n" - "rivers\n" - "> > =\n" - "\n" + "> > > + * @leave_on Self explanatory. Can be disabled by knowledgeable drivers\n" + "> > \n" "> > Not self explanatory. I need this explained to me. What does leave_on\n" "> > do? Better yet, what would happen if leave_on did not exist?\n" - "> > =\n" - "\n" + "> > \n" "> > > + */\n" "> > > +struct critical {\n" "> > > + bool enabled;\n" @@ -93,39 +73,31 @@ "> > > struct kref ref;\n" "> > > + struct critical critical;\n" "> > > };\n" - "> > > =\n" - "\n" + "> > > \n" "> > > struct clk {\n" - "> > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *c=\n" - "lk)\n" - "> > > if (WARN_ON(clk->enable_count =3D=3D 0))\n" + "> > > @@ -995,6 +1011,10 @@ static void clk_core_disable(struct clk_core *clk)\n" + "> > > if (WARN_ON(clk->enable_count == 0))\n" "> > > return;\n" - "> > > =\n" - "\n" + "> > > \n" "> > > + /* Refuse to turn off a critical clock */\n" - "> > > + if (clk->enable_count =3D=3D 1 && clk->critical.leave_on)\n" + "> > > + if (clk->enable_count == 1 && clk->critical.leave_on)\n" "> > > + return;\n" - "> > =\n" - "\n" + "> > \n" "> > How do we get to this point? clk_enable_critical actually calls\n" "> > clk_enable, thus incrementing the enable_count. The only time that we\n" "> > could hit the above case is if,\n" - "> > =\n" - "\n" + "> > \n" "> > a) there is an imbalance in clk_enable and clk_disable calls. If this is\n" "> > the case then the drivers need to be fixed. Or better yet some\n" "> > infrastructure to catch that, now that we have per-user struct clk\n" "> > cookies.\n" - "> > =\n" - "\n" + "> > \n" "> > b) a driver knowingly calls clk_enable_critical(foo) and then regular,\n" "> > old clk_disable(foo). But why would a driver do that?\n" - "> > =\n" - "\n" + "> > \n" "> > It might be that I am missing the point here, so please feel free to\n" "> > clue me in.\n" - "> =\n" - "\n" + "> \n" "> This check behaves in a very similar to the WARN() above. It's more\n" "> of a fail-safe. If all drivers are behaving properly, then it\n" "> shouldn't ever be true. If they're not, it prevents an incorrectly\n" @@ -135,8 +107,7 @@ "to honor imbalances anyways. Below are two patches that probably resolve\n" "the issue of badly behaving drivers that cause enable imbalances.\n" "\n" - "> =\n" - "\n" + "> \n" "> As I said in the other mail. We can do without these 3 new wrappers.\n" "> We _could_ just write a driver which only calls clk_enable() _after_\n" "> it calls clk_disable(), a kind of intentional unbalance and it would\n" @@ -156,7 +127,7 @@ "\n" "\n" "\n" - "From=203599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001\n" + ">From 3599ed206da9ce770bfafcfd95cbb9a03ac44473 Mon Sep 17 00:00:00 2001\n" "From: Michael Turquette <mturquette@baylibre.com>\n" "Date: Wed, 29 Jul 2015 18:22:45 -0700\n" "Subject: [PATCH 1/2] clk: per-user clk prepare & enable ref counts\n" @@ -182,15 +153,13 @@ "+\tunsigned int enable_count;\n" "+\tunsigned int prepare_count;\n" " };\n" - " =\n" - "\n" + " \n" " /*** locking ***/\n" "@@ -600,6 +602,9 @@ void clk_unprepare(struct clk *clk)\n" " \t\treturn;\n" - " =\n" - "\n" + " \n" " \tclk_prepare_lock();\n" - "+\tif (WARN_ON(clk->prepare_count =3D=3D 0))\n" + "+\tif (WARN_ON(clk->prepare_count == 0))\n" "+\t\treturn;\n" "+\tclk->prepare_count--;\n" " \tclk_core_unprepare(clk->core);\n" @@ -198,20 +167,17 @@ " }\n" "@@ -657,6 +662,7 @@ int clk_prepare(struct clk *clk)\n" " \t\treturn 0;\n" - " =\n" - "\n" + " \n" " \tclk_prepare_lock();\n" "+\tclk->prepare_count++;\n" - " \tret =3D clk_core_prepare(clk->core);\n" + " \tret = clk_core_prepare(clk->core);\n" " \tclk_prepare_unlock();\n" - " =\n" - "\n" + " \n" "@@ -707,6 +713,9 @@ void clk_disable(struct clk *clk)\n" " \t\treturn;\n" - " =\n" - "\n" - " \tflags =3D clk_enable_lock();\n" - "+\tif (WARN_ON(clk->enable_count =3D=3D 0))\n" + " \n" + " \tflags = clk_enable_lock();\n" + "+\tif (WARN_ON(clk->enable_count == 0))\n" "+\t\treturn;\n" "+\tclk->enable_count--;\n" " \tclk_core_disable(clk->core);\n" @@ -219,24 +185,21 @@ " }\n" "@@ -769,6 +778,7 @@ int clk_enable(struct clk *clk)\n" " \t\treturn 0;\n" - " =\n" - "\n" - " \tflags =3D clk_enable_lock();\n" + " \n" + " \tflags = clk_enable_lock();\n" "+\tclk->enable_count++;\n" - " \tret =3D clk_core_enable(clk->core);\n" + " \tret = clk_core_enable(clk->core);\n" " \tclk_enable_unlock(flags);\n" - " =\n" - "\n" - "-- =\n" - "\n" + " \n" + "-- \n" "1.9.1\n" "\n" - "From=20ace76f6ed634a69c499f8440a98d4b5a54d78368 Mon Sep 17 00:00:00 2001\n" + ">From ace76f6ed634a69c499f8440a98d4b5a54d78368 Mon Sep 17 00:00:00 2001\n" "From: Michael Turquette <mturquette@baylibre.com>\n" "Date: Thu, 30 Jul 2015 12:52:26 -0700\n" "Subject: [PATCH 2/2] clk: clk_put WARNs if user has not disabled clk\n" "\n" - "From=20the clk_put kerneldoc in include/linux/clk.h:\n" + ">From the clk_put kerneldoc in include/linux/clk.h:\n" "\n" "\"\"\"\n" "Note: drivers must ensure that all clk_enable calls made on this clock\n" @@ -264,8 +227,7 @@ "@@ -2764,6 +2764,14 @@ void __clk_put(struct clk *clk)\n" " \t clk->max_rate < clk->core->req_rate)\n" " \t\tclk_core_set_rate_nolock(clk->core, clk->core->req_rate);\n" - " =\n" - "\n" + " \n" "+\t/*\n" "+\t * before calling clk_put, all calls to clk_prepare and clk_enable from\n" "+\t * a given user must be balanced with calls to clk_disable and\n" @@ -274,12 +236,10 @@ "+\tWARN_ON(clk->prepare_count);\n" "+\tWARN_ON(clk->enable_count);\n" "+\n" - " \towner =3D clk->core->owner;\n" + " \towner = clk->core->owner;\n" " \tkref_put(&clk->core->ref, __clk_release);\n" - " =\n" - "\n" - "-- =\n" - "\n" + " \n" + "-- \n" 1.9.1 -81e804f26cd6ea7034bc654cfd1c6029cada808ea499f5b67e9d2ac2b158f418 +8e01093a9212d32e2113d377123938a28a7ad0329fedfc160864e7ab101e8cd0
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.