* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Sergey Senozhatsky @ 2019-08-29 0:27 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Senozhatsky, Rasmus Villemoes, Jani Nikula, Petr Mladek,
Steven Rostedt, Andrew Morton, Enrico Weigelt, metux IT consult,
Jonathan Corbet, linux-doc, linux-kernel
In-Reply-To: <61cd079f-d41b-75ec-9a1e-ef80f9d1f8fd@kleine-koenig.org>
On (08/28/19 18:22), Uwe Kleine-König wrote:
> That is wrong. When you do
>
> pr_err("There are no round tuits to give out: %dE\n", -ENOENT);
>
> in a kernel that doesn't support %dE you get:
>
> There are no round tuits to give out: -2E
OK. Good point.
-ss
^ permalink raw reply
* Re: [RFC v1 2/2] rcu/tree: Remove dynticks_nmi_nesting counter
From: Joel Fernandes @ 2019-08-29 1:51 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190828231247.GE26530@linux.ibm.com>
On Wed, Aug 28, 2019 at 04:12:47PM -0700, Paul E. McKenney wrote:
> On Wed, Aug 28, 2019 at 06:14:44PM -0400, Joel Fernandes wrote:
> > On Wed, Aug 28, 2019 at 03:01:08PM -0700, Paul E. McKenney wrote:
> > > On Wed, Aug 28, 2019 at 05:42:41PM -0400, Joel Fernandes wrote:
> > > > On Wed, Aug 28, 2019 at 02:19:04PM -0700, Paul E. McKenney wrote:
> > > > > On Wed, Aug 28, 2019 at 05:05:25PM -0400, Joel Fernandes wrote:
> > > > > > On Wed, Aug 28, 2019 at 01:23:30PM -0700, Paul E. McKenney wrote:
> > > > > > > On Mon, Aug 26, 2019 at 09:33:54PM -0400, Joel Fernandes (Google) wrote:
> > > > > > > > The dynticks_nmi_nesting counter serves 4 purposes:
> > > > > > > >
> > > > > > > > (a) rcu_is_cpu_rrupt_from_idle() needs to be able to detect first
> > > > > > > > interrupt nesting level.
> > > > > > > >
> > > > > > > > (b) We need to detect half-interrupts till we are sure they're not an
> > > > > > > > issue. However, change the comparison to DYNTICK_IRQ_NONIDLE with 0.
> > > > > > > >
> > > > > > > > (c) When a quiescent state report is needed from a nohz_full CPU.
> > > > > > > > The nesting counter detects we are a first level interrupt.
> > > > > > > >
> > > > > > > > For (a) we can just use dyntick_nesting == 1 to determine this. Only the
> > > > > > > > outermost interrupt that interrupted an RCU-idle state can set it to 1.
> > > > > > > >
> > > > > > > > For (b), this warning condition has not occurred for several kernel
> > > > > > > > releases. But we still keep the warning but change it to use
> > > > > > > > in_interrupt() instead of the nesting counter. In a later year, we can
> > > > > > > > remove the warning.
> > > > > > > >
> > > > > > > > For (c), the nest check is not really necessary since forced_tick would
> > > > > > > > have been set to true in the outermost interrupt, so the nested/NMI
> > > > > > > > interrupts will check forced_tick anyway, and bail.
> > > > > > >
> > > > > > > Skipping the commit log and documentation for this pass.
> > > > > > [snip]
> > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > > > > index 255cd6835526..1465a3e406f8 100644
> > > > > > > > --- a/kernel/rcu/tree.c
> > > > > > > > +++ b/kernel/rcu/tree.c
> > > > > > > > @@ -81,7 +81,6 @@
> > > > > > > >
> > > > > > > > static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> > > > > > > > .dynticks_nesting = 1,
> > > > > > > > - .dynticks_nmi_nesting = 0,
> > > > > > >
> > > > > > > This should be in the previous patch, give or take naming.
> > > > > >
> > > > > > Done.
> > > > > >
> > > > > > > > .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> > > > > > > > };
> > > > > > > > struct rcu_state rcu_state = {
> > > > > > > > @@ -392,15 +391,9 @@ static int rcu_is_cpu_rrupt_from_idle(void)
> > > > > > > > /* Check for counter underflows */
> > > > > > > > RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nesting) < 0,
> > > > > > > > "RCU dynticks_nesting counter underflow!");
> > > > > > > > - RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> > > > > > > > - "RCU dynticks_nmi_nesting counter underflow/zero!");
> > > > > > > >
> > > > > > > > - /* Are we at first interrupt nesting level? */
> > > > > > > > - if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
> > > > > > > > - return false;
> > > > > > > > -
> > > > > > > > - /* Does CPU appear to be idle from an RCU standpoint? */
> > > > > > > > - return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
> > > > > > > > + /* Are we the outermost interrupt that arrived when RCU was idle? */
> > > > > > > > + return __this_cpu_read(rcu_data.dynticks_nesting) == 1;
> > > > > > > > }
> > > > > > > >
> > > > > > > > #define DEFAULT_RCU_BLIMIT 10 /* Maximum callbacks per rcu_do_batch ... */
> > > > > > > > @@ -564,11 +557,10 @@ static void rcu_eqs_enter(bool user)
> > > > > > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > > > > > >
> > > > > > > > /* Entering usermode/idle from interrupt is not handled. These would
> > > > > > > > - * mean usermode upcalls or idle entry happened from interrupts. But,
> > > > > > > > - * reset the counter if we warn.
> > > > > > > > + * mean usermode upcalls or idle exit happened from interrupts. Remove
> > > > > > > > + * the warning by 2020.
> > > > > > > > */
> > > > > > > > - if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> > > > > > > > - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> > > > > > > > + WARN_ON_ONCE(in_interrupt());
> > > > > > >
> > > > > > > And this is a red flag. Bad things happen should some common code
> > > > > > > that disables BH be invoked from the idle loop. This might not be
> > > > > > > happening now, but we need to avoid this sort of constraint.
> > > > > > > How about instead merging ->dyntick_nesting into the low-order bits
> > > > > > > of ->dyntick_nmi_nesting?
> > > > > > >
> > > > > > > Yes, this assumes that we don't enter process level twice, but it should
> > > > > > > be easy to add a WARN_ON() to test for that. Except that we don't have
> > > > > > > to because there is already this near the end of rcu_eqs_exit():
> > > > > > >
> > > > > > > WARN_ON_ONCE(rdp->dynticks_nmi_nesting);
> > > > > > >
> > > > > > > So the low-order bit of the combined counter could indicate process-level
> > > > > > > non-idle, the next three bits could be unused to make interpretation
> > > > > > > of hex printouts easier, and then the rest of the bits could be used in
> > > > > > > the same way as currently.
> > > > > > >
> > > > > > > This would allow a single read to see the full state, so that 0x1 means
> > > > > > > at process level in the kernel, 0x11 is interrupt (or NMI) from process
> > > > > > > level, 0x10 is interrupt/NMI from idle/user, and so on.
> > > > > > >
> > > > > > > What am I missing here? Why wouldn't this work, and without adding yet
> > > > > > > another RCU-imposed constraint on some other subsystem?
> > > > > >
> > > > > > What about replacing the warning with a WARN_ON_ONCE(in_irq()), would that
> > > > > > address your concern?
> > > > > >
> > > > > > Also, considering this warning condition is most likely never occurring as we
> > > > > > know it, and we are considering deleting it soon enough, is it really worth
> > > > > > reimplementing the whole mechanism with a complex bit-sharing scheme just
> > > > > > because of the BH-disable condition you mentioned, which likely doesn't
> > > > > > happen today? In my implementation, this is just a simple counter. I feel
> > > > > > combining bits in the same counter will just introduce more complexity that
> > > > > > this patch tries to address/avoid.
> > > > > >
> > > > > > OTOH, I also don't mind with just deleting the warning altogether if you are
> > > > > > Ok with that.
> > > > >
> > > > > The big advantage of combining the counters is that all of the state is
> > > > > explicit and visible in one place. Plus it can be accessed atomically.
> > > > > And it avoids setting a time bomb for some poor guys just trying to get
> > > > > their idle-loop jobs done some time in the dim distant future.
> > > >
> > > > I could try the approach you're suggesting but I didn't actually see an issue
> > > > with the patch in its current state other than the WARN_ON_ONCE which I could
> > > > change to WARN_ON_ONCE(in_irq()) to remove the concern. AFAICS, we don't
> > > > detect "half soft-interrupts" in this code in anyway.
> > > >
> > > > I do feel the approach you're suggesting can be a follow up, these 2 patches
> > > > just focus on deleting dynticks_nmi_nesting counter and we can test this
> > > > approach thoroughly for a release or so.
> > > >
> > > > > Besides, this pair of patches already makes a large change from a
> > > > > conceptual viewpoint. If we are going to make a large change, let's
> > > > > get our money's worth out of that change!
> > > >
> > > > IMHO, most of the changes are to code comments, the actual code change is
> > > > very little and is just removal of dynticks_nmi_nesting and simplification;
> > > > its not really an introduction of a new mechanism.
> > >
> > > This change is not fixing a bug, so there is no need for an emergency fix,
> > > and thus no point in additional churn. I understand that it is a bit
> > > annoying to code and test something and have your friendly maintainer say
> > > "sorry, wrong rocks", and the reason that I understand this is that I do
> > > that to myself rather often.
> >
> > The motivation for me for this change is to avoid future bugs such as with
> > the following patch where "== 2" did not take the force write of
> > DYNTICK_IRQ_NONIDLE into account:
> > https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=13c4b07593977d9288e5d0c21c89d9ba27e2ea1f
>
> Yes, the current code does need some simplification.
>
> > I still don't see it as pointless churn, it is also a maintenance cost in its
> > current form and the simplification is worth it IMHO both from a readability,
> > and maintenance stand point.
> >
> > I still don't see what's technically wrong with the patch. I could perhaps
> > add the above "== 2" point in the patch?
>
> I don't know of a crash or splat your patch would cause, if that is
> your question. But that is also true of the current code, so the point
> is simplification, not bug fixing. And from what I can see, there is an
> opportunity to simplify quite a bit further. And with something like
> RCU, further simplification is worth -serious- consideration.
>
> > We could also discuss f2f at LPC to see if we can agree about it?
>
> That might make a lot of sense.
Sure. I am up for a further redesign / simplification. I will think more
about your suggestions and can also further discuss at LPC.
And this patch is on LKML archives and is not going anywhere so there's no
rush I guess ;-)
> In the meantime, could you please create an independent series (or
> more than one series, as the case may be) of the other patches?
The only other patch in this series is to repurpose the dyntick_nesting
counter to do the job of the dynticks_nmi_nesting counter. Did you mean that
one? Or did you mean the dntick tracing patch? I think I should indeed submit
the tracing patch separately.
thanks,
- Joel
^ permalink raw reply
* Re: [RFC v1 2/2] rcu/tree: Remove dynticks_nmi_nesting counter
From: Paul E. McKenney @ 2019-08-29 3:43 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190829015155.GB100789@google.com>
On Wed, Aug 28, 2019 at 09:51:55PM -0400, Joel Fernandes wrote:
> On Wed, Aug 28, 2019 at 04:12:47PM -0700, Paul E. McKenney wrote:
> > On Wed, Aug 28, 2019 at 06:14:44PM -0400, Joel Fernandes wrote:
> > > On Wed, Aug 28, 2019 at 03:01:08PM -0700, Paul E. McKenney wrote:
> > > > On Wed, Aug 28, 2019 at 05:42:41PM -0400, Joel Fernandes wrote:
> > > > > On Wed, Aug 28, 2019 at 02:19:04PM -0700, Paul E. McKenney wrote:
> > > > > > On Wed, Aug 28, 2019 at 05:05:25PM -0400, Joel Fernandes wrote:
> > > > > > > On Wed, Aug 28, 2019 at 01:23:30PM -0700, Paul E. McKenney wrote:
> > > > > > > > On Mon, Aug 26, 2019 at 09:33:54PM -0400, Joel Fernandes (Google) wrote:
> > > > > > > > > The dynticks_nmi_nesting counter serves 4 purposes:
> > > > > > > > >
> > > > > > > > > (a) rcu_is_cpu_rrupt_from_idle() needs to be able to detect first
> > > > > > > > > interrupt nesting level.
> > > > > > > > >
> > > > > > > > > (b) We need to detect half-interrupts till we are sure they're not an
> > > > > > > > > issue. However, change the comparison to DYNTICK_IRQ_NONIDLE with 0.
> > > > > > > > >
> > > > > > > > > (c) When a quiescent state report is needed from a nohz_full CPU.
> > > > > > > > > The nesting counter detects we are a first level interrupt.
> > > > > > > > >
> > > > > > > > > For (a) we can just use dyntick_nesting == 1 to determine this. Only the
> > > > > > > > > outermost interrupt that interrupted an RCU-idle state can set it to 1.
> > > > > > > > >
> > > > > > > > > For (b), this warning condition has not occurred for several kernel
> > > > > > > > > releases. But we still keep the warning but change it to use
> > > > > > > > > in_interrupt() instead of the nesting counter. In a later year, we can
> > > > > > > > > remove the warning.
> > > > > > > > >
> > > > > > > > > For (c), the nest check is not really necessary since forced_tick would
> > > > > > > > > have been set to true in the outermost interrupt, so the nested/NMI
> > > > > > > > > interrupts will check forced_tick anyway, and bail.
> > > > > > > >
> > > > > > > > Skipping the commit log and documentation for this pass.
> > > > > > > [snip]
> > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > > > > > index 255cd6835526..1465a3e406f8 100644
> > > > > > > > > --- a/kernel/rcu/tree.c
> > > > > > > > > +++ b/kernel/rcu/tree.c
> > > > > > > > > @@ -81,7 +81,6 @@
> > > > > > > > >
> > > > > > > > > static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_data) = {
> > > > > > > > > .dynticks_nesting = 1,
> > > > > > > > > - .dynticks_nmi_nesting = 0,
> > > > > > > >
> > > > > > > > This should be in the previous patch, give or take naming.
> > > > > > >
> > > > > > > Done.
> > > > > > >
> > > > > > > > > .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> > > > > > > > > };
> > > > > > > > > struct rcu_state rcu_state = {
> > > > > > > > > @@ -392,15 +391,9 @@ static int rcu_is_cpu_rrupt_from_idle(void)
> > > > > > > > > /* Check for counter underflows */
> > > > > > > > > RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nesting) < 0,
> > > > > > > > > "RCU dynticks_nesting counter underflow!");
> > > > > > > > > - RCU_LOCKDEP_WARN(__this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> > > > > > > > > - "RCU dynticks_nmi_nesting counter underflow/zero!");
> > > > > > > > >
> > > > > > > > > - /* Are we at first interrupt nesting level? */
> > > > > > > > > - if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
> > > > > > > > > - return false;
> > > > > > > > > -
> > > > > > > > > - /* Does CPU appear to be idle from an RCU standpoint? */
> > > > > > > > > - return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
> > > > > > > > > + /* Are we the outermost interrupt that arrived when RCU was idle? */
> > > > > > > > > + return __this_cpu_read(rcu_data.dynticks_nesting) == 1;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > #define DEFAULT_RCU_BLIMIT 10 /* Maximum callbacks per rcu_do_batch ... */
> > > > > > > > > @@ -564,11 +557,10 @@ static void rcu_eqs_enter(bool user)
> > > > > > > > > struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > > > > > > >
> > > > > > > > > /* Entering usermode/idle from interrupt is not handled. These would
> > > > > > > > > - * mean usermode upcalls or idle entry happened from interrupts. But,
> > > > > > > > > - * reset the counter if we warn.
> > > > > > > > > + * mean usermode upcalls or idle exit happened from interrupts. Remove
> > > > > > > > > + * the warning by 2020.
> > > > > > > > > */
> > > > > > > > > - if (WARN_ON_ONCE(rdp->dynticks_nmi_nesting != 0))
> > > > > > > > > - WRITE_ONCE(rdp->dynticks_nmi_nesting, 0);
> > > > > > > > > + WARN_ON_ONCE(in_interrupt());
> > > > > > > >
> > > > > > > > And this is a red flag. Bad things happen should some common code
> > > > > > > > that disables BH be invoked from the idle loop. This might not be
> > > > > > > > happening now, but we need to avoid this sort of constraint.
> > > > > > > > How about instead merging ->dyntick_nesting into the low-order bits
> > > > > > > > of ->dyntick_nmi_nesting?
> > > > > > > >
> > > > > > > > Yes, this assumes that we don't enter process level twice, but it should
> > > > > > > > be easy to add a WARN_ON() to test for that. Except that we don't have
> > > > > > > > to because there is already this near the end of rcu_eqs_exit():
> > > > > > > >
> > > > > > > > WARN_ON_ONCE(rdp->dynticks_nmi_nesting);
> > > > > > > >
> > > > > > > > So the low-order bit of the combined counter could indicate process-level
> > > > > > > > non-idle, the next three bits could be unused to make interpretation
> > > > > > > > of hex printouts easier, and then the rest of the bits could be used in
> > > > > > > > the same way as currently.
> > > > > > > >
> > > > > > > > This would allow a single read to see the full state, so that 0x1 means
> > > > > > > > at process level in the kernel, 0x11 is interrupt (or NMI) from process
> > > > > > > > level, 0x10 is interrupt/NMI from idle/user, and so on.
> > > > > > > >
> > > > > > > > What am I missing here? Why wouldn't this work, and without adding yet
> > > > > > > > another RCU-imposed constraint on some other subsystem?
> > > > > > >
> > > > > > > What about replacing the warning with a WARN_ON_ONCE(in_irq()), would that
> > > > > > > address your concern?
> > > > > > >
> > > > > > > Also, considering this warning condition is most likely never occurring as we
> > > > > > > know it, and we are considering deleting it soon enough, is it really worth
> > > > > > > reimplementing the whole mechanism with a complex bit-sharing scheme just
> > > > > > > because of the BH-disable condition you mentioned, which likely doesn't
> > > > > > > happen today? In my implementation, this is just a simple counter. I feel
> > > > > > > combining bits in the same counter will just introduce more complexity that
> > > > > > > this patch tries to address/avoid.
> > > > > > >
> > > > > > > OTOH, I also don't mind with just deleting the warning altogether if you are
> > > > > > > Ok with that.
> > > > > >
> > > > > > The big advantage of combining the counters is that all of the state is
> > > > > > explicit and visible in one place. Plus it can be accessed atomically.
> > > > > > And it avoids setting a time bomb for some poor guys just trying to get
> > > > > > their idle-loop jobs done some time in the dim distant future.
> > > > >
> > > > > I could try the approach you're suggesting but I didn't actually see an issue
> > > > > with the patch in its current state other than the WARN_ON_ONCE which I could
> > > > > change to WARN_ON_ONCE(in_irq()) to remove the concern. AFAICS, we don't
> > > > > detect "half soft-interrupts" in this code in anyway.
> > > > >
> > > > > I do feel the approach you're suggesting can be a follow up, these 2 patches
> > > > > just focus on deleting dynticks_nmi_nesting counter and we can test this
> > > > > approach thoroughly for a release or so.
> > > > >
> > > > > > Besides, this pair of patches already makes a large change from a
> > > > > > conceptual viewpoint. If we are going to make a large change, let's
> > > > > > get our money's worth out of that change!
> > > > >
> > > > > IMHO, most of the changes are to code comments, the actual code change is
> > > > > very little and is just removal of dynticks_nmi_nesting and simplification;
> > > > > its not really an introduction of a new mechanism.
> > > >
> > > > This change is not fixing a bug, so there is no need for an emergency fix,
> > > > and thus no point in additional churn. I understand that it is a bit
> > > > annoying to code and test something and have your friendly maintainer say
> > > > "sorry, wrong rocks", and the reason that I understand this is that I do
> > > > that to myself rather often.
> > >
> > > The motivation for me for this change is to avoid future bugs such as with
> > > the following patch where "== 2" did not take the force write of
> > > DYNTICK_IRQ_NONIDLE into account:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=13c4b07593977d9288e5d0c21c89d9ba27e2ea1f
> >
> > Yes, the current code does need some simplification.
> >
> > > I still don't see it as pointless churn, it is also a maintenance cost in its
> > > current form and the simplification is worth it IMHO both from a readability,
> > > and maintenance stand point.
> > >
> > > I still don't see what's technically wrong with the patch. I could perhaps
> > > add the above "== 2" point in the patch?
> >
> > I don't know of a crash or splat your patch would cause, if that is
> > your question. But that is also true of the current code, so the point
> > is simplification, not bug fixing. And from what I can see, there is an
> > opportunity to simplify quite a bit further. And with something like
> > RCU, further simplification is worth -serious- consideration.
> >
> > > We could also discuss f2f at LPC to see if we can agree about it?
> >
> > That might make a lot of sense.
>
> Sure. I am up for a further redesign / simplification. I will think more
> about your suggestions and can also further discuss at LPC.
One question that might (or might not) help: Given the compound counter,
where the low-order hex digit indicates whether the corresponding CPU
is running in a non-idle kernel task and the rest of the hex digits
indicate the NMI-style nesting counter shifted up by four bits, what
could rcu_is_cpu_rrupt_from_idle() be reduced to?
> And this patch is on LKML archives and is not going anywhere so there's no
> rush I guess ;-)
True enough! ;-)
> > In the meantime, could you please create an independent series (or
> > more than one series, as the case may be) of the other patches?
>
> The only other patch in this series is to repurpose the dyntick_nesting
> counter to do the job of the dynticks_nmi_nesting counter. Did you mean that
> one? Or did you mean the dntick tracing patch? I think I should indeed submit
> the tracing patch separately.
Heh!
You have sent me a number of patches over the past day or two. Respin
them as needed and resend. My feeling was that one of the groups could
be split, but then again there might have been dependencies among the
patches that I didn't see. You decide.
On the tracing patch... That patch might be a good idea regardless,
but I bet that the reason that you felt the sudden need for it was due
to the loss of information in your eventual ->dynticks_nesting field.
After all, the value 0x1 might be an interrupt from idle, or it might
just as easily be a task running in the kernel at process level.
The reason the patch might nevertheless be a good idea is that redundant
information can be helpful when debugging. Especially when debugging
new architecture-specific code, which is when RCU's dyntick-idle warnings
tend to find bugs.
Thanx, Paul
^ permalink raw reply
* [PATCH] [RFC] i2c: imx: make use of format specifier %dE
From: Uwe Kleine-König @ 2019-08-29 4:29 UTC (permalink / raw)
To: Wolfram Sang, Oleksij Rempel
Cc: kernel, Shawn Guo, Fabio Estevam, NXP Linux Team, linux-i2c,
linux-arm-kernel, Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
Andrew Morton, Jani Nikula, Enrico Weigelt, Jonathan Corbet,
linux-doc, linux-kernel
I created a patch that teaches printk et al to emit a symbolic error
name for an error valued integer[1]. With that applied
dev_err(&pdev->dev, "can't enable I2C clock, ret=%dE\n", ret);
emits
... can't enable I2C clock, ret=EIO
if ret is -EIO. Petr Mladek (i.e. one of the printk maintainers) had
concerns if this would be well received and worth the effort. He asked
to present it to a few subsystems. So for now, this patch converting the
i2c-imx driver shouldn't be applied yet but it would be great to get
some feedback about if you think that being able to easily printk (for
example) "EIO" instead of "-5" is a good idea. Would it help you? Do you
think it helps your users?
Thanks
Uwe
[1] https://lkml.org/lkml/2019/8/27/1456
---
drivers/i2c/busses/i2c-imx.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 15f6cde6452f..359e911cb891 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -289,7 +289,7 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
if (IS_ERR(dma->chan_tx)) {
ret = PTR_ERR(dma->chan_tx);
if (ret != -ENODEV && ret != -EPROBE_DEFER)
- dev_err(dev, "can't request DMA tx channel (%d)\n", ret);
+ dev_err(dev, "can't request DMA tx channel (%dE)\n", ret);
goto fail_al;
}
@@ -300,7 +300,7 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
dma_sconfig.direction = DMA_MEM_TO_DEV;
ret = dmaengine_slave_config(dma->chan_tx, &dma_sconfig);
if (ret < 0) {
- dev_err(dev, "can't configure tx channel (%d)\n", ret);
+ dev_err(dev, "can't configure tx channel (%dE)\n", ret);
goto fail_tx;
}
@@ -308,7 +308,7 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
if (IS_ERR(dma->chan_rx)) {
ret = PTR_ERR(dma->chan_rx);
if (ret != -ENODEV && ret != -EPROBE_DEFER)
- dev_err(dev, "can't request DMA rx channel (%d)\n", ret);
+ dev_err(dev, "can't request DMA rx channel (%dE)\n", ret);
goto fail_tx;
}
@@ -319,7 +319,7 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
dma_sconfig.direction = DMA_DEV_TO_MEM;
ret = dmaengine_slave_config(dma->chan_rx, &dma_sconfig);
if (ret < 0) {
- dev_err(dev, "can't configure rx channel (%d)\n", ret);
+ dev_err(dev, "can't configure rx channel (%dE)\n", ret);
goto fail_rx;
}
@@ -964,7 +964,7 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent);
out:
- dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__,
+ dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %dE\n", __func__,
(result < 0) ? "error" : "success msg",
(result < 0) ? result : num);
return (result < 0) ? result : num;
@@ -1100,7 +1100,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
ret = clk_prepare_enable(i2c_imx->clk);
if (ret) {
- dev_err(&pdev->dev, "can't enable I2C clock, ret=%d\n", ret);
+ dev_err(&pdev->dev, "can't enable I2C clock, ret=%dE\n", ret);
return ret;
}
@@ -1108,7 +1108,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, IRQF_SHARED,
pdev->name, i2c_imx);
if (ret) {
- dev_err(&pdev->dev, "can't claim irq %d\n", irq);
+ dev_err(&pdev->dev, "can't claim irq %dE\n", irq);
goto clk_disable;
}
@@ -1230,7 +1230,7 @@ static int __maybe_unused i2c_imx_runtime_resume(struct device *dev)
ret = clk_enable(i2c_imx->clk);
if (ret)
- dev_err(dev, "can't enable I2C clock, ret=%d\n", ret);
+ dev_err(dev, "can't enable I2C clock, ret=%dE\n", ret);
return ret;
}
--
2.23.0
^ permalink raw reply related
* [PATCH] [RFC] tty/serial: imx: make use of format specifier %dE
From: Uwe Kleine-König @ 2019-08-29 4:37 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: kernel, Shawn Guo, Fabio Estevam, NXP Linux Team, linux-serial,
linux-arm-kernel, Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
Andrew Morton, Jani Nikula, Enrico Weigelt, Jonathan Corbet,
linux-doc, linux-kernel
I created a patch that teaches printk et al to emit a symbolic error
name for an error valued integer[1]. With that applied
dev_err(&pdev->dev, "failed to get ipg clk: %dE\n", ret);
emits
... failed to get ipg clk: EPROBE_DEFER
if ret is -EPROBE_DEFER. Petr Mladek (i.e. one of the printk
maintainers) had concerns if this would be well received and worth the
effort. He asked to present it to a few subsystems. So for now, this
patch converting the imx UART driver shouldn't be applied yet but it
would be great to get some feedback about if you think that being able
to easily printk (for example) "EIO" instead of "-5" is a good idea.
Would it help you? Do you think it helps your users?
Thanks
Uwe
[1] https://lkml.org/lkml/2019/8/27/1456
---
drivers/tty/serial/imx.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 57d6e6ba556e..a3dbb9378e8b 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2143,7 +2143,7 @@ static int imx_uart_probe_dt(struct imx_port *sport,
ret = of_alias_get_id(np, "serial");
if (ret < 0) {
- dev_err(&pdev->dev, "failed to get alias id, errno %d\n", ret);
+ dev_err(&pdev->dev, "failed to get alias id, error %dE\n", ret);
return ret;
}
sport->port.line = ret;
@@ -2236,14 +2236,14 @@ static int imx_uart_probe(struct platform_device *pdev)
sport->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
if (IS_ERR(sport->clk_ipg)) {
ret = PTR_ERR(sport->clk_ipg);
- dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
+ dev_err(&pdev->dev, "failed to get ipg clk: %dE\n", ret);
return ret;
}
sport->clk_per = devm_clk_get(&pdev->dev, "per");
if (IS_ERR(sport->clk_per)) {
ret = PTR_ERR(sport->clk_per);
- dev_err(&pdev->dev, "failed to get per clk: %d\n", ret);
+ dev_err(&pdev->dev, "failed to get per clk: %dE\n", ret);
return ret;
}
@@ -2252,7 +2252,7 @@ static int imx_uart_probe(struct platform_device *pdev)
/* For register access, we only need to enable the ipg clock. */
ret = clk_prepare_enable(sport->clk_ipg);
if (ret) {
- dev_err(&pdev->dev, "failed to enable per clk: %d\n", ret);
+ dev_err(&pdev->dev, "failed to enable per clk: %dE\n", ret);
return ret;
}
@@ -2330,7 +2330,7 @@ static int imx_uart_probe(struct platform_device *pdev)
ret = devm_request_irq(&pdev->dev, rxirq, imx_uart_rxint, 0,
dev_name(&pdev->dev), sport);
if (ret) {
- dev_err(&pdev->dev, "failed to request rx irq: %d\n",
+ dev_err(&pdev->dev, "failed to request rx irq: %dE\n",
ret);
return ret;
}
@@ -2338,7 +2338,7 @@ static int imx_uart_probe(struct platform_device *pdev)
ret = devm_request_irq(&pdev->dev, txirq, imx_uart_txint, 0,
dev_name(&pdev->dev), sport);
if (ret) {
- dev_err(&pdev->dev, "failed to request tx irq: %d\n",
+ dev_err(&pdev->dev, "failed to request tx irq: %dE\n",
ret);
return ret;
}
@@ -2346,7 +2346,7 @@ static int imx_uart_probe(struct platform_device *pdev)
ret = devm_request_irq(&pdev->dev, rtsirq, imx_uart_rtsint, 0,
dev_name(&pdev->dev), sport);
if (ret) {
- dev_err(&pdev->dev, "failed to request rts irq: %d\n",
+ dev_err(&pdev->dev, "failed to request rts irq: %dE\n",
ret);
return ret;
}
@@ -2354,7 +2354,7 @@ static int imx_uart_probe(struct platform_device *pdev)
ret = devm_request_irq(&pdev->dev, rxirq, imx_uart_int, 0,
dev_name(&pdev->dev), sport);
if (ret) {
- dev_err(&pdev->dev, "failed to request irq: %d\n", ret);
+ dev_err(&pdev->dev, "failed to request irq: %dE\n", ret);
return ret;
}
}
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v3 3/3] fbdev: fbmem: avoid exporting fb_center_logo
From: Peter Rosin @ 2019-08-29 7:08 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-kernel@vger.kernel.org, Bartlomiej Zolnierkiewicz,
Jonathan Corbet, dri-devel@lists.freedesktop.org,
linux-fbdev@vger.kernel.org, linux-doc@vger.kernel.org,
Matthew Wilcox
In-Reply-To: <CAMuHMdVkqX7x_D5nf01s-kE=o+y5OLM-5fd3q=2RDKGTcpCfHg@mail.gmail.com>
On 2019-08-27 13:35, Geert Uytterhoeven wrote:
> Hi Peter,
>
> On Tue, Aug 27, 2019 at 1:09 PM Peter Rosin <peda@axentia.se> wrote:
>> The variable is only ever used from fbcon.c which is linked into the
>> same module. Therefore, the export is not needed.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> But note that the same is true for fb_class, so perhaps it can be added
> (or better, removed ;-)?
Right. Someone please let me know if 3/3 needs to be extended. I'm also
happy to just drop it...
> Once drivers/staging/olpc_dcon/olpc_dcon.c stops abusing registered_fb[]
> and num_registered_fb, those can go, too.
>
> Does anyone remembe why au1200fb calls fb_prepare_logo() and fb_show_logo()
> itself?
Maybe there should be a small drivers/video/fbdev/core/fbmem.h file (or
something) with these "internal" declarations, to hide some clutter currently
in include/linux/fb.h?
Feels like that could be done later, after these other cleanups you mention,
so that the new file has a few more things to declare.
Cheers,
Peter
^ permalink raw reply
* [PATCH v10 0/7] Solve postboot supplier cleanup and optimize probe ordering
From: Saravana Kannan @ 2019-08-29 7:45 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand, Jonathan Corbet, Len Brown
Cc: Saravana Kannan, devicetree, linux-kernel, linux-doc, linux-acpi,
clang-built-linux, David Collins, kernel-team
Add device-links to track functional dependencies between devices
after they are created (but before they are probed) by looking at
their common DT bindings like clocks, interconnects, etc.
Having functional dependencies automatically added before the devices
are probed, provides the following benefits:
- Optimizes device probe order and avoids the useless work of
attempting probes of devices that will not probe successfully
(because their suppliers aren't present or haven't probed yet).
For example, in a commonly available mobile SoC, registering just
one consumer device's driver at an initcall level earlier than the
supplier device's driver causes 11 failed probe attempts before the
consumer device probes successfully. This was with a kernel with all
the drivers statically compiled in. This problem gets a lot worse if
all the drivers are loaded as modules without direct symbol
dependencies.
- Supplier devices like clock providers, interconnect providers, etc
need to keep the resources they provide active and at a particular
state(s) during boot up even if their current set of consumers don't
request the resource to be active. This is because the rest of the
consumers might not have probed yet and turning off the resource
before all the consumers have probed could lead to a hang or
undesired user experience.
Some frameworks (Eg: regulator) handle this today by turning off
"unused" resources at late_initcall_sync and hoping all the devices
have probed by then. This is not a valid assumption for systems with
loadable modules. Other frameworks (Eg: clock) just don't handle
this due to the lack of a clear signal for when they can turn off
resources. This leads to downstream hacks to handle cases like this
that can easily be solved in the upstream kernel.
By linking devices before they are probed, we give suppliers a clear
count of the number of dependent consumers. Once all of the
consumers are active, the suppliers can turn off the unused
resources without making assumptions about the number of consumers.
By default we just add device-links to track "driver presence" (probe
succeeded) of the supplier device. If any other functionality provided
by device-links are needed, it is left to the consumer/supplier
devices to change the link when they probe.
v1 -> v2:
- Drop patch to speed up of_find_device_by_node()
- Drop depends-on property and use existing bindings
v2 -> v3:
- Refactor the code to have driver core initiate the linking of devs
- Have driver core link consumers to supplier before it's probed
- Add support for drivers to edit the device links before probing
v3 -> v4:
- Tested edit_links() on system with cyclic dependency. Works.
- Added some checks to make sure device link isn't attempted from
parent device node to child device node.
- Added way to pause/resume sync_state callbacks across
of_platform_populate().
- Recursively parse DT node to create device links from parent to
suppliers of parent and all child nodes.
v4 -> v5:
- Fixed copy-pasta bugs with linked list handling
- Walk up the phandle reference till I find an actual device (needed
for regulators to work)
- Added support for linking devices from regulator DT bindings
- Tested the whole series again to make sure cyclic dependencies are
broken with edit_links() and regulator links are created properly.
v5 -> v6:
- Split, squashed and reordered some of the patches.
- Refactored the device linking code to follow the same code pattern for
any property.
v6 -> v7:
- No functional changes.
- Renamed i to index
- Added comment to clarify not having to check property name for every
index
- Added "matched" variable to clarify code. No functional change.
- Added comments to include/linux/device.h for add_links()
v7 -> v8:
- Rebased on top of linux-next to handle device link changes in [1]
v8 -> v9:
- Fixed kbuild test bot reported errors (docs and const)
v9->v10:
- Changes made based on reviews on LKML [2] and discussions at ELC [3]
- Dropped the edit_links() patch
- Dropped the patch that skips linking for default bus nodes
- 1/7: Changed from bus.add_links() to fwnode.ops.add_links()
- 1/7: Update device link doc
- 1/7: Lots of comments/fn doc updates
- 1/7: Renamed device_link_check_waiting_consumers() to
device_link_add_missing_supplier_links()
- 2/7: Moved DT parsing/linking code from of/platform.c to of/property.c
- 2/7: Lots of comments/fn doc updates
- 2/7: Returned errors for all error cases in of_link_to_phandle()
- 2/7: Some minor code refactor to remove "bool done"
- 2/7: Added debug messages when links not created due permanent errors
- 3/7: Minor comments update
- Added 2 new patches 6/7 and 7/7 to handle cyclic dependencies using
depends-on
[1] - https://lore.kernel.org/lkml/2305283.AStDPdUUnE@kreacher/
[2] - https://lore.kernel.org/lkml/20190724001100.133423-2-saravanak@google.com/
[3] - https://lore.kernel.org/lkml/CAGETcx_pSnC_2D7ufLRyfE3b8uRc814XEf8zu+SpNtT7_Z8NLg@mail.gmail.com/
-Saravana
Saravana Kannan (7):
driver core: Add support for linking devices during device addition
of: property: Add functional dependency link from DT bindings
driver core: Add sync_state driver/bus callback
of/platform: Pause/resume sync state during init and
of_platform_populate()
of: property: Create device links for all child-supplier depencencies
dt-bindings: Add depends-on property to break cyclic inferred
dependencies
of: property: Add "depends-on" parsing support to
of_fwnode_add_links()
.../admin-guide/kernel-parameters.rst | 1 +
.../admin-guide/kernel-parameters.txt | 6 +
.../devicetree/bindings/depends-on.txt | 46 ++++
Documentation/driver-api/device_link.rst | 3 +-
drivers/base/core.c | 154 +++++++++++
drivers/of/platform.c | 12 +
drivers/of/property.c | 258 ++++++++++++++++++
include/linux/device.h | 26 ++
include/linux/fwnode.h | 17 ++
9 files changed, 522 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/depends-on.txt
--
2.23.0.187.g17f5b7556c-goog
^ permalink raw reply
* [PATCH v10 1/7] driver core: Add support for linking devices during device addition
From: Saravana Kannan @ 2019-08-29 7:45 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand, Jonathan Corbet, Len Brown
Cc: Saravana Kannan, devicetree, linux-kernel, linux-doc, linux-acpi,
clang-built-linux, David Collins, kernel-team
In-Reply-To: <20190829074603.70424-1-saravanak@google.com>
The firmware corresponding to a device (dev.fwnode) might be able to
provide functional dependency information between a device and its
supplier and consumer devices. Tracking this functional dependency
allows optimizing device probe order and informing a supplier when all
its consumers have probed (and thereby actively managing their
resources).
The existing device links feature allows tracking and using
supplier-consumer relationships. So, this patch adds the add_links()
fwnode callback to allow firmware to create device links for each
device as the device is added.
However, when consumer devices are added, they might not have a supplier
device to link to despite needing mandatory resources/functionality from
one or more suppliers. A waiting_for_suppliers list is created to track
such consumers and retry linking them when new devices get added.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
Documentation/driver-api/device_link.rst | 3 +-
drivers/base/core.c | 89 ++++++++++++++++++++++++
include/linux/device.h | 2 +
include/linux/fwnode.h | 17 +++++
4 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst
index 1b5020ec6517..bc2d89af88ce 100644
--- a/Documentation/driver-api/device_link.rst
+++ b/Documentation/driver-api/device_link.rst
@@ -281,7 +281,8 @@ State machine
:c:func:`driver_bound()`.)
* Before a consumer device is probed, presence of supplier drivers is
- verified by checking that links to suppliers are in ``DL_STATE_AVAILABLE``
+ verified by checking the consumer device is not in the wait_for_suppliers
+ list and by checking that links to suppliers are in ``DL_STATE_AVAILABLE``
state. The state of the links is updated to ``DL_STATE_CONSUMER_PROBE``.
(Call to :c:func:`device_links_check_suppliers()` from
:c:func:`really_probe()`.)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2db62d98e395..39633bb75f0f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -44,6 +44,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
#endif
/* Device links support. */
+static LIST_HEAD(wait_for_suppliers);
+static DEFINE_MUTEX(wfs_lock);
#ifdef CONFIG_SRCU
static DEFINE_MUTEX(device_links_lock);
@@ -430,6 +432,58 @@ struct device_link *device_link_add(struct device *consumer,
}
EXPORT_SYMBOL_GPL(device_link_add);
+/**
+ * device_link_wait_for_supplier - Add device to wait_for_suppliers list
+ * @consumer: Consumer device
+ *
+ * Marks the @consumer device as waiting for suppliers to become available by
+ * adding it to the wait_for_suppliers list. The consumer device will never be
+ * probed until it's removed from the wait_for_suppliers list.
+ *
+ * The caller is responsible for adding the links to the supplier devices once
+ * they are available and removing the @consumer device from the
+ * wait_for_suppliers list once links to all the suppliers have been created.
+ *
+ * This function is NOT meant to be called from the probe function of the
+ * consumer but rather from code that creates/adds the consumer device.
+ */
+static void device_link_wait_for_supplier(struct device *consumer)
+{
+ mutex_lock(&wfs_lock);
+ list_add_tail(&consumer->links.needs_suppliers, &wait_for_suppliers);
+ mutex_unlock(&wfs_lock);
+}
+
+/**
+ * device_link_add_missing_supplier_links - Add links from consumer devices to
+ * supplier devices, leaving any
+ * consumer with inactive suppliers on
+ * the wait_for_suppliers list
+ *
+ * Loops through all consumers waiting on suppliers and tries to add all their
+ * supplier links. If that succeeds, the consumer device is removed from
+ * wait_for_suppliers list. Otherwise, they are left in the wait_for_suppliers
+ * list. Devices left on the wait_for_suppliers list will not be probed.
+ *
+ * The fwnode add_links callback is expected to return 0 if it has found and
+ * added all the supplier links for the consumer device. It should return an
+ * error if it isn't able to do so.
+ *
+ * The caller of device_link_wait_for_supplier() is expected to call this once
+ * it's aware of potential suppliers becoming available.
+ */
+static void device_link_add_missing_supplier_links(void)
+{
+ struct device *dev, *tmp;
+
+ mutex_lock(&wfs_lock);
+ list_for_each_entry_safe(dev, tmp, &wait_for_suppliers,
+ links.needs_suppliers)
+ if (!fwnode_call_int_op(dev->fwnode, add_links, dev))
+ list_del_init(&dev->links.needs_suppliers);
+ mutex_unlock(&wfs_lock);
+}
+
static void device_link_free(struct device_link *link)
{
while (refcount_dec_not_one(&link->rpm_active))
@@ -564,6 +618,17 @@ int device_links_check_suppliers(struct device *dev)
struct device_link *link;
int ret = 0;
+ /*
+ * Device waiting for supplier to become available is not allowed to
+ * probe.
+ */
+ mutex_lock(&wfs_lock);
+ if (!list_empty(&dev->links.needs_suppliers)) {
+ mutex_unlock(&wfs_lock);
+ return -EPROBE_DEFER;
+ }
+ mutex_unlock(&wfs_lock);
+
device_links_write_lock();
list_for_each_entry(link, &dev->links.suppliers, c_node) {
@@ -848,6 +913,10 @@ static void device_links_purge(struct device *dev)
{
struct device_link *link, *ln;
+ mutex_lock(&wfs_lock);
+ list_del(&dev->links.needs_suppliers);
+ mutex_unlock(&wfs_lock);
+
/*
* Delete all of the remaining links from this device to any other
* devices (either consumers or suppliers).
@@ -1712,6 +1781,7 @@ void device_initialize(struct device *dev)
#endif
INIT_LIST_HEAD(&dev->links.consumers);
INIT_LIST_HEAD(&dev->links.suppliers);
+ INIT_LIST_HEAD(&dev->links.needs_suppliers);
dev->links.status = DL_DEV_NO_DRIVER;
}
EXPORT_SYMBOL_GPL(device_initialize);
@@ -2198,6 +2268,25 @@ int device_add(struct device *dev)
BUS_NOTIFY_ADD_DEVICE, dev);
kobject_uevent(&dev->kobj, KOBJ_ADD);
+
+ /*
+ * Check if any of the other devices (consumers) have been waiting for
+ * this device (supplier) to be added so that they can create a device
+ * link to it.
+ *
+ * This needs to happen after device_pm_add() because device_link_add()
+ * requires the supplier be registered before it's called.
+ *
+ * But this also needs to happe before bus_probe_device() to make sure
+ * waiting consumers can link to it before the driver is bound to the
+ * device and the driver sync_state callback is called for this device.
+ */
+ device_link_add_missing_supplier_links();
+
+ if (fwnode_has_op(dev->fwnode, add_links)
+ && fwnode_call_int_op(dev->fwnode, add_links, dev))
+ device_link_wait_for_supplier(dev);
+
bus_probe_device(dev);
if (parent)
klist_add_tail(&dev->p->knode_parent,
diff --git a/include/linux/device.h b/include/linux/device.h
index ec598ede9455..76458cfbb267 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1131,11 +1131,13 @@ enum dl_dev_state {
* struct dev_links_info - Device data related to device links.
* @suppliers: List of links to supplier devices.
* @consumers: List of links to consumer devices.
+ * @needs_suppliers: Hook to global list of devices waiting for suppliers.
* @status: Driver status information.
*/
struct dev_links_info {
struct list_head suppliers;
struct list_head consumers;
+ struct list_head needs_suppliers;
enum dl_dev_state status;
};
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index a11c8c56c78b..068b0024adef 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -65,6 +65,21 @@ struct fwnode_reference_args {
* endpoint node.
* @graph_get_port_parent: Return the parent node of a port node.
* @graph_parse_endpoint: Parse endpoint for port and endpoint id.
+ * @add_links: Called after the device corresponding to the fwnode is added
+ * using device_add(). The function is expected to create device
+ * links to all the suppliers of the device that are available at
+ * the time this function is called. The function must NOT stop
+ * at the first failed device link if other unlinked supplier
+ * devices are present in the system. If some suppliers are not
+ * yet available, this function will be called again when other
+ * devices are added to allow creating device links to any newly
+ * available suppliers.
+ *
+ * Return 0 if device links have been successfully created to all
+ * the suppliers of this device or if the supplier information is
+ * not known. Return an error if and only if the supplier
+ * information is known but some of the suppliers are not yet
+ * available to create device links to.
*/
struct fwnode_operations {
struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
@@ -102,6 +117,8 @@ struct fwnode_operations {
(*graph_get_port_parent)(struct fwnode_handle *fwnode);
int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode,
struct fwnode_endpoint *endpoint);
+ int (*add_links)(const struct fwnode_handle *fwnode,
+ struct device *dev);
};
#define fwnode_has_op(fwnode, op) \
--
2.23.0.187.g17f5b7556c-goog
^ permalink raw reply related
* [PATCH v10 2/7] of: property: Add functional dependency link from DT bindings
From: Saravana Kannan @ 2019-08-29 7:45 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand, Jonathan Corbet, Len Brown
Cc: Saravana Kannan, devicetree, linux-kernel, linux-doc, linux-acpi,
clang-built-linux, David Collins, kernel-team, kbuild test robot
In-Reply-To: <20190829074603.70424-1-saravanak@google.com>
Add device links after the devices are created (but before they are
probed) by looking at common DT bindings like clocks and
interconnects.
Automatically adding device links for functional dependencies at the
framework level provides the following benefits:
- Optimizes device probe order and avoids the useless work of
attempting probes of devices that will not probe successfully
(because their suppliers aren't present or haven't probed yet).
For example, in a commonly available mobile SoC, registering just
one consumer device's driver at an initcall level earlier than the
supplier device's driver causes 11 failed probe attempts before the
consumer device probes successfully. This was with a kernel with all
the drivers statically compiled in. This problem gets a lot worse if
all the drivers are loaded as modules without direct symbol
dependencies.
- Supplier devices like clock providers, interconnect providers, etc
need to keep the resources they provide active and at a particular
state(s) during boot up even if their current set of consumers don't
request the resource to be active. This is because the rest of the
consumers might not have probed yet and turning off the resource
before all the consumers have probed could lead to a hang or
undesired user experience.
Some frameworks (Eg: regulator) handle this today by turning off
"unused" resources at late_initcall_sync and hoping all the devices
have probed by then. This is not a valid assumption for systems with
loadable modules. Other frameworks (Eg: clock) just don't handle
this due to the lack of a clear signal for when they can turn off
resources. This leads to downstream hacks to handle cases like this
that can easily be solved in the upstream kernel.
By linking devices before they are probed, we give suppliers a clear
count of the number of dependent consumers. Once all of the
consumers are active, the suppliers can turn off the unused
resources without making assumptions about the number of consumers.
By default we just add device-links to track "driver presence" (probe
succeeded) of the supplier device. If any other functionality provided
by device-links are needed, it is left to the consumer/supplier
devices to change the link when they probe.
kbuild test robot reported clang error about missing const
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
.../admin-guide/kernel-parameters.rst | 1 +
.../admin-guide/kernel-parameters.txt | 6 +
drivers/of/property.c | 241 ++++++++++++++++++
3 files changed, 248 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.rst b/Documentation/admin-guide/kernel-parameters.rst
index d05d531b4ec9..6d421694d98e 100644
--- a/Documentation/admin-guide/kernel-parameters.rst
+++ b/Documentation/admin-guide/kernel-parameters.rst
@@ -127,6 +127,7 @@ parameter is applicable::
NET Appropriate network support is enabled.
NUMA NUMA support is enabled.
NFS Appropriate NFS support is enabled.
+ OF Devicetree is enabled.
OSS OSS sound support is enabled.
PV_OPS A paravirtualized kernel is enabled.
PARIDE The ParIDE (parallel port IDE) subsystem is enabled.
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 96383f63cc55..a07f86ba2fd7 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3187,6 +3187,12 @@
This can be set from sysctl after boot.
See Documentation/admin-guide/sysctl/vm.rst for details.
+ of_devlink [OF, KNL] Create device links between consumer and
+ supplier devices by scanning the devictree to infer the
+ consumer/supplier relationships. A consumer device
+ will not be probed until all the supplier devices have
+ probed successfully.
+
ohci1394_dma=early [HW] enable debugging via the ohci1394 driver.
See Documentation/debugging-via-ohci1394.txt for more
info.
diff --git a/drivers/of/property.c b/drivers/of/property.c
index d7fa75e31f22..82052172f508 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -25,6 +25,7 @@
#include <linux/of_device.h>
#include <linux/of_graph.h>
#include <linux/string.h>
+#include <linux/moduleparam.h>
#include "of_private.h"
@@ -985,6 +986,245 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
return of_device_get_match_data(dev);
}
+static bool of_is_ancestor_of(struct device_node *test_ancestor,
+ struct device_node *child)
+{
+ of_node_get(child);
+ while (child) {
+ if (child == test_ancestor) {
+ of_node_put(child);
+ return false;
+ }
+ child = of_get_next_parent(child);
+ }
+ return true;
+}
+
+/**
+ * of_link_to_phandle - Add device link to supplier from supplier phandle
+ * @dev: consumer device
+ * @sup_np: phandle to supplier device tree node
+ *
+ * Given a phandle to a supplier device tree node (@sup_np), this function
+ * finds the device that owns the supplier device tree node and creates a
+ * device link from @dev consumer device to the supplier device. This function
+ * doesn't create device links for invalid scenarios such as trying to create a
+ * link with a parent device as the consumer of its child device. In such
+ * cases, it returns an error.
+ *
+ * Returns:
+ * - 0 if link successfully created to supplier
+ * - -EAGAIN if linking to the supplier should be reattempted
+ * - -EINVAL if the supplier link is invalid and should not be created
+ * - -ENODEV if there is no device that corresponds to the supplier phandle
+ */
+static int of_link_to_phandle(struct device *dev, struct device_node *sup_np)
+{
+ struct platform_device *sup_dev;
+ u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
+ int ret = 0;
+ struct device_node *tmp_np = sup_np;
+
+ of_node_get(sup_np);
+ /*
+ * Find the device node that contains the supplier phandle. It may be
+ * @sup_np or it may be an ancestor of @sup_np.
+ */
+ while (sup_np && !of_find_property(sup_np, "compatible", NULL))
+ sup_np = of_get_next_parent(sup_np);
+ if (!sup_np) {
+ dev_dbg(dev, "Not linking to %pOFP - No device\n", tmp_np);
+ return -ENODEV;
+ }
+
+ /*
+ * Don't allow linking a device node as a consumer of one of its
+ * descendant nodes. By definition, a child node can't be a functional
+ * dependency for the parent node.
+ */
+ if (!of_is_ancestor_of(dev->of_node, sup_np)) {
+ dev_dbg(dev, "Not linking to %pOFP - is descendant\n", sup_np);
+ of_node_put(sup_np);
+ return -EINVAL;
+ }
+ sup_dev = of_find_device_by_node(sup_np);
+ of_node_put(sup_np);
+ if (!sup_dev)
+ return -EAGAIN;
+ if (!device_link_add(dev, &sup_dev->dev, dl_flags))
+ ret = -EAGAIN;
+ put_device(&sup_dev->dev);
+ return ret;
+}
+
+/**
+ * parse_prop_cells - Property parsing function for suppliers
+ *
+ * @np: Pointer to device tree node containing a list
+ * @prop_name: Name of property to be parsed. Expected to hold phandle values
+ * @index: For properties holding a list of phandles, this is the index
+ * into the list.
+ * @list_name: Property name that is known to contain list of phandle(s) to
+ * supplier(s)
+ * @cells_name: property name that specifies phandles' arguments count
+ *
+ * This is a helper function to parse properties that have a known fixed name
+ * and are a list of phandles and phandle arguments.
+ *
+ * Returns:
+ * - phandle node pointer with refcount incremented. Caller must of_node_put()
+ * on it when done.
+ * - NULL if no phandle found at index
+ */
+static struct device_node *parse_prop_cells(struct device_node *np,
+ const char *prop_name, int index,
+ const char *list_name,
+ const char *cells_name)
+{
+ struct of_phandle_args sup_args;
+
+ if (strcmp(prop_name, list_name))
+ return NULL;
+
+ if (of_parse_phandle_with_args(np, list_name, cells_name, index,
+ &sup_args))
+ return NULL;
+
+ return sup_args.np;
+}
+
+static struct device_node *parse_clocks(struct device_node *np,
+ const char *prop_name, int index)
+{
+ return parse_prop_cells(np, prop_name, index, "clocks", "#clock-cells");
+}
+
+static struct device_node *parse_interconnects(struct device_node *np,
+ const char *prop_name, int index)
+{
+ return parse_prop_cells(np, prop_name, index, "interconnects",
+ "#interconnect-cells");
+}
+
+static int strcmp_suffix(const char *str, const char *suffix)
+{
+ unsigned int len, suffix_len;
+
+ len = strlen(str);
+ suffix_len = strlen(suffix);
+ if (len <= suffix_len)
+ return -1;
+ return strcmp(str + len - suffix_len, suffix);
+}
+
+static struct device_node *parse_regulators(struct device_node *np,
+ const char *prop_name, int index)
+{
+ if (index || strcmp_suffix(prop_name, "-supply"))
+ return NULL;
+
+ return of_parse_phandle(np, prop_name, 0);
+}
+
+/**
+ * struct supplier_bindings - Property parsing functions for suppliers
+ *
+ * @parse_prop: function name
+ * parse_prop() finds the node corresponding to a supplier phandle
+ * @parse_prop.np: Pointer to device node holding supplier phandle property
+ * @parse_prop.prop_name: Name of property holding a phandle value
+ * @parse_prop.index: For properties holding a list of phandles, this is the
+ * index into the list
+ *
+ * Returns:
+ * parse_prop() return values are
+ * - phandle node pointer with refcount incremented. Caller must of_node_put()
+ * on it when done.
+ * - NULL if no phandle found at index
+ */
+struct supplier_bindings {
+ struct device_node *(*parse_prop)(struct device_node *np,
+ const char *prop_name, int index);
+};
+
+static const struct supplier_bindings bindings[] = {
+ { .parse_prop = parse_clocks, },
+ { .parse_prop = parse_interconnects, },
+ { .parse_prop = parse_regulators, },
+ {},
+};
+
+/**
+ * of_link_property - Create device links to suppliers listed in a property
+ * @dev: Consumer device
+ * @con_np: The consumer device tree node which contains the property
+ * @prop_name: Name of property to be parsed
+ *
+ * This function checks if the property @prop_name that is present in the
+ * @con_np device tree node is one of the known common device tree bindings
+ * that list phandles to suppliers. If @prop_name isn't one, this function
+ * doesn't do anything.
+ *
+ * If @prop_name is one, this function attempts to create device links from the
+ * consumer device @dev to all the devices of the suppliers listed in
+ * @prop_name.
+ *
+ * Any failed attempt to create a device link will NOT result in an immediate
+ * return. of_link_property() must create links to all the available supplier
+ * devices even when attempts to create a link to one or more suppliers fail.
+ */
+static int of_link_property(struct device *dev, struct device_node *con_np,
+ const char *prop_name)
+{
+ struct device_node *phandle;
+ const struct supplier_bindings *s = bindings;
+ unsigned int i = 0;
+ bool matched = false;
+ int ret = 0;
+
+ /* Do not stop at first failed link, link all available suppliers. */
+ while (!matched && s->parse_prop) {
+ while ((phandle = s->parse_prop(con_np, prop_name, i))) {
+ matched = true;
+ i++;
+ if (of_link_to_phandle(dev, phandle) == -EAGAIN)
+ ret = -EAGAIN;
+ of_node_put(phandle);
+ }
+ s++;
+ }
+ return ret;
+}
+
+static int __of_link_to_suppliers(struct device *dev,
+ struct device_node *con_np)
+{
+ struct device_node *child;
+ struct property *p;
+ int ret = 0;
+
+ for_each_property_of_node(con_np, p)
+ if (of_link_property(dev, con_np, p->name))
+ ret = -EAGAIN;
+
+ return ret;
+}
+
+static bool of_devlink;
+core_param(of_devlink, of_devlink, bool, 0);
+
+static int of_fwnode_add_links(const struct fwnode_handle *fwnode,
+ struct device *dev)
+{
+ if (!of_devlink)
+ return 0;
+
+ if (unlikely(!is_of_node(fwnode)))
+ return 0;
+
+ return __of_link_to_suppliers(dev, to_of_node(fwnode));
+}
+
const struct fwnode_operations of_fwnode_ops = {
.get = of_fwnode_get,
.put = of_fwnode_put,
@@ -1001,5 +1241,6 @@ const struct fwnode_operations of_fwnode_ops = {
.graph_get_remote_endpoint = of_fwnode_graph_get_remote_endpoint,
.graph_get_port_parent = of_fwnode_graph_get_port_parent,
.graph_parse_endpoint = of_fwnode_graph_parse_endpoint,
+ .add_links = of_fwnode_add_links,
};
EXPORT_SYMBOL_GPL(of_fwnode_ops);
--
2.23.0.187.g17f5b7556c-goog
^ permalink raw reply related
* [PATCH v10 4/7] of/platform: Pause/resume sync state during init and of_platform_populate()
From: Saravana Kannan @ 2019-08-29 7:46 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand, Jonathan Corbet, Len Brown
Cc: Saravana Kannan, devicetree, linux-kernel, linux-doc, linux-acpi,
clang-built-linux, David Collins, kernel-team
In-Reply-To: <20190829074603.70424-1-saravanak@google.com>
When all the top level devices are populated from DT during kernel
init, the supplier devices could be added and probed before the
consumer devices are added and linked to the suppliers. To avoid the
sync_state() callback from being called prematurely, pause the
sync_state() callbacks before populating the devices and resume them
at late_initcall_sync().
Similarly, when children devices are populated from a module using
of_platform_populate(), there could be supplier-consumer dependencies
between the children devices that are populated. To avoid the same
problem with sync_state() being called prematurely, pause and resume
sync_state() callbacks across of_platform_populate().
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/of/platform.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b47a2292fe8e..d93891a05f60 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -480,6 +480,7 @@ int of_platform_populate(struct device_node *root,
pr_debug("%s()\n", __func__);
pr_debug(" starting at: %pOF\n", root);
+ device_links_supplier_sync_state_pause();
for_each_child_of_node(root, child) {
rc = of_platform_bus_create(child, matches, lookup, parent, true);
if (rc) {
@@ -487,6 +488,8 @@ int of_platform_populate(struct device_node *root,
break;
}
}
+ device_links_supplier_sync_state_resume();
+
of_node_set_flag(root, OF_POPULATED_BUS);
of_node_put(root);
@@ -518,6 +521,7 @@ static int __init of_platform_default_populate_init(void)
if (!of_have_populated_dt())
return -ENODEV;
+ device_links_supplier_sync_state_pause();
/*
* Handle certain compatibles explicitly, since we don't want to create
* platform_devices for every node in /reserved-memory with a
@@ -538,6 +542,14 @@ static int __init of_platform_default_populate_init(void)
return 0;
}
arch_initcall_sync(of_platform_default_populate_init);
+
+static int __init of_platform_sync_state_init(void)
+{
+ if (of_have_populated_dt())
+ device_links_supplier_sync_state_resume();
+ return 0;
+}
+late_initcall_sync(of_platform_sync_state_init);
#endif
int of_platform_device_destroy(struct device *dev, void *data)
--
2.23.0.187.g17f5b7556c-goog
^ permalink raw reply related
* [PATCH v10 5/7] of: property: Create device links for all child-supplier depencencies
From: Saravana Kannan @ 2019-08-29 7:46 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand, Jonathan Corbet, Len Brown
Cc: Saravana Kannan, devicetree, linux-kernel, linux-doc, linux-acpi,
clang-built-linux, David Collins, kernel-team
In-Reply-To: <20190829074603.70424-1-saravanak@google.com>
A parent device can have child devices that it adds when it probes. But
this probing of the parent device can happen way after kernel init is done
-- for example, when the parent device's driver is loaded as a module.
In such cases, if the child devices depend on a supplier in the system, we
need to make sure the supplier gets the sync_state() callback only after
these child devices are added and probed.
To achieve this, when creating device links for a device by looking at its
DT node, don't just look at DT references at the top node level. Look at DT
references in all the descendant nodes too and create device links from the
ancestor device to all these supplier devices.
This way, when the parent device probes and adds child devices, the child
devices can then create their own device links to the suppliers and further
delay the supplier's sync_state() callback to after the child devices are
probed.
Example:
In this illustration, -> denotes DT references and indentation
represents child status.
Device node A
Device node B -> D
Device node C -> B, D
Device node D
Assume all these devices have their drivers loaded as modules.
Without this patch, this is the sequence of events:
1. D is added.
2. A is added.
3. Device D probes.
4. Device D gets its sync_state() callback.
5. Device B and C might malfunction because their resources got
altered/turned off before they can make active requests for them.
With this patch, this is the sequence of events:
1. D is added.
2. A is added and creates device links to D.
3. Device link from A to B is not added because A is a parent of B.
4. Device D probes.
5. Device D does not get it's sync_state() callback because consumer A
hasn't probed yet.
5. Device A probes.
5. a. Devices B and C are added.
5. b. Device links from B and C to D are added.
5. c. Device A's probe completes.
6. Device D does not get it's sync_state() callback because consumer A
has probed but consumers B and C haven't probed yet.
7. Device B and C probe.
8. Device D gets it's sync_state() callback because all its consumers
have probed.
9. None of the devices malfunction.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/of/property.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 82052172f508..420c2d428184 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1207,6 +1207,10 @@ static int __of_link_to_suppliers(struct device *dev,
if (of_link_property(dev, con_np, p->name))
ret = -EAGAIN;
+ for_each_child_of_node(con_np, child)
+ if (__of_link_to_suppliers(dev, child))
+ ret = -EAGAIN;
+
return ret;
}
--
2.23.0.187.g17f5b7556c-goog
^ permalink raw reply related
* [PATCH v10 6/7] dt-bindings: Add depends-on property to break cyclic inferred dependencies
From: Saravana Kannan @ 2019-08-29 7:46 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand, Jonathan Corbet, Len Brown
Cc: Saravana Kannan, devicetree, linux-kernel, linux-doc, linux-acpi,
clang-built-linux, David Collins, kernel-team
In-Reply-To: <20190829074603.70424-1-saravanak@google.com>
The functional dependencies of a device can be inferred by looking at
the common devicetree bindings like clocks, interconnects and
regulators.
However, this can sometimes result in cyclic dependencies where one of
the inferred dependencies isn't really a functional dependency.
Add a depends-on property that can override inferred dependencies by
explicitly listing the suppliers of a device and thereby allow breaking
any cyclic inferred depenencies.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
.../devicetree/bindings/depends-on.txt | 46 +++++++++++++++++++
1 file changed, 46 insertions(+)
create mode 100644 Documentation/devicetree/bindings/depends-on.txt
diff --git a/Documentation/devicetree/bindings/depends-on.txt b/Documentation/devicetree/bindings/depends-on.txt
new file mode 100644
index 000000000000..e6535917b189
--- /dev/null
+++ b/Documentation/devicetree/bindings/depends-on.txt
@@ -0,0 +1,46 @@
+Explicit listing of dependencies
+================================
+
+Apart from parent-child relationships, devices (consumers) often have
+functional dependencies on other devices (suppliers). Examples of common
+suppliers are clocks, interconnects and regulators.
+
+The consumer-supplier dependencies of most devices can be inferred by
+simply looking at the devicetree bindings of common suppliers like clocks,
+interconnects and regulators. However, this can sometimes result in cyclic
+dependencies where one of the inferred dependencies isn't really a
+functional dependency.
+
+When there is an inferred cyclic dependency between devices, we need a way
+to explicitly list the suppliers of one or more devices in the cycle so
+that we can break the cycle.
+
+The depends-on property fills this need. It can be used to explicitly list
+the suppliers of a device and override any inferred dependencies of that
+device.
+
+This property shall be used ONLY to break cyclic dependencies.
+
+Optional properties:
+- depends-on: A list of phandles to suppliers of the device.
+
+Examples:
+Here, the inferred depencency would state that cc2 is dependent on cc1 and
+cc3; and cc3 is dependent on cc1 and cc2. This creates a cycle between cc2
+and cc3.
+
+With the use of depends-on, cc2 is only dependent on cc1; and cc3 is still
+dependent on cc1 and cc2. This breaks the cycle between cc2 and cc3.
+
+cc2: cc2@40031000 {
+ compatible = "cc2";
+ reg = <0x40031000 0x1000>;
+ clocks = <&cc1 10>, <&cc3 7>;
+ depends-on = <&cc1>;
+};
+
+cc3: cc3@40034000 {
+ compatible = "cc3";
+ reg = <0x40031000 0x1000>;
+ clocks = <&cc1 10>, <&cc2 7>;
+};
--
2.23.0.187.g17f5b7556c-goog
^ permalink raw reply related
* [PATCH v10 7/7] of: property: Add "depends-on" parsing support to of_fwnode_add_links()
From: Saravana Kannan @ 2019-08-29 7:46 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand, Jonathan Corbet, Len Brown
Cc: Saravana Kannan, devicetree, linux-kernel, linux-doc, linux-acpi,
clang-built-linux, David Collins, kernel-team
In-Reply-To: <20190829074603.70424-1-saravanak@google.com>
If dependencies inferred by of_fwnode_add_links() result in a cycle, it
can prevent the probing of all the devices in the cycle. The depends-on
property has been added to explicitly override inferred dependencies
when they create a cycle.
Add depends-on parsing support to of_fwnode_add_links() so that
platforms with cyclic dependencies can use "depends-on" to break the
cycle and continue successfully probing devices.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/of/property.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 420c2d428184..78a262e24686 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1106,6 +1106,12 @@ static struct device_node *parse_interconnects(struct device_node *np,
"#interconnect-cells");
}
+static struct device_node *parse_depends_on(struct device_node *np,
+ const char *prop_name, int index)
+{
+ return parse_prop_cells(np, prop_name, index, "depends-on", NULL);
+}
+
static int strcmp_suffix(const char *str, const char *suffix)
{
unsigned int len, suffix_len;
@@ -1151,6 +1157,7 @@ static const struct supplier_bindings bindings[] = {
{ .parse_prop = parse_clocks, },
{ .parse_prop = parse_interconnects, },
{ .parse_prop = parse_regulators, },
+ { .parse_prop = parse_depends_on, },
{},
};
@@ -1203,6 +1210,12 @@ static int __of_link_to_suppliers(struct device *dev,
struct property *p;
int ret = 0;
+ if (of_find_property(con_np, "depends-on", NULL)) {
+ if (of_link_property(dev, con_np, "depends-on"))
+ ret = -EAGAIN;
+ return ret;
+ }
+
for_each_property_of_node(con_np, p)
if (of_link_property(dev, con_np, p->name))
ret = -EAGAIN;
--
2.23.0.187.g17f5b7556c-goog
^ permalink raw reply related
* [PATCH v10 3/7] driver core: Add sync_state driver/bus callback
From: Saravana Kannan @ 2019-08-29 7:45 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
Frank Rowand, Jonathan Corbet, Len Brown
Cc: Saravana Kannan, devicetree, linux-kernel, linux-doc, linux-acpi,
clang-built-linux, David Collins, kernel-team, kbuild test robot
In-Reply-To: <20190829074603.70424-1-saravanak@google.com>
This sync_state driver/bus callback is called once all the consumers
of a supplier have probed successfully.
This allows the supplier device's driver/bus to sync the supplier
device's state to the software state with the guarantee that all the
consumers are actively managing the resources provided by the supplier
device.
To maintain backwards compatibility and ease transition from existing
frameworks and resource cleanup schemes, late_initcall_sync is the
earliest when the sync_state callback might be called.
There is no upper bound on the time by which the sync_state callback
has to be called. This is because if a consumer device never probes,
the supplier has to maintain its resources in the state left by the
bootloader. For example, if the bootloader leaves the display
backlight at a fixed voltage and the backlight driver is never probed,
you don't want the backlight to ever be turned off after boot up.
Also, when multiple devices are added after kernel init, some
suppliers could be added before their consumer devices get added. In
these instances, the supplier devices could get their sync_state
callback called right after they probe because the consumers devices
haven't had a chance to create device links to the suppliers.
To handle this correctly, this change also provides APIs to
pause/resume sync state callbacks so that when multiple devices are
added, their sync_state callback evaluation can be postponed to happen
after all of them are added.
kbuild test robot reported missing documentation for device.state_synced
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/base/core.c | 65 ++++++++++++++++++++++++++++++++++++++++++
include/linux/device.h | 24 ++++++++++++++++
2 files changed, 89 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 39633bb75f0f..5e79d5820e79 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -46,6 +46,8 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
/* Device links support. */
static LIST_HEAD(wait_for_suppliers);
static DEFINE_MUTEX(wfs_lock);
+static LIST_HEAD(deferred_sync);
+static unsigned int supplier_sync_state_disabled;
#ifdef CONFIG_SRCU
static DEFINE_MUTEX(device_links_lock);
@@ -648,6 +650,62 @@ int device_links_check_suppliers(struct device *dev)
return ret;
}
+static void __device_links_supplier_sync_state(struct device *dev)
+{
+ struct device_link *link;
+
+ if (dev->state_synced)
+ return;
+
+ list_for_each_entry(link, &dev->links.consumers, s_node) {
+ if (!(link->flags & DL_FLAG_MANAGED))
+ continue;
+ if (link->status != DL_STATE_ACTIVE)
+ return;
+ }
+
+ if (dev->bus->sync_state)
+ dev->bus->sync_state(dev);
+ else if (dev->driver && dev->driver->sync_state)
+ dev->driver->sync_state(dev);
+
+ dev->state_synced = true;
+}
+
+void device_links_supplier_sync_state_pause(void)
+{
+ device_links_write_lock();
+ supplier_sync_state_disabled++;
+ device_links_write_unlock();
+}
+
+void device_links_supplier_sync_state_resume(void)
+{
+ struct device *dev, *tmp;
+
+ device_links_write_lock();
+ if (!supplier_sync_state_disabled) {
+ WARN(true, "Unmatched sync_state pause/resume!");
+ goto out;
+ }
+ supplier_sync_state_disabled--;
+ if (supplier_sync_state_disabled)
+ goto out;
+
+ list_for_each_entry_safe(dev, tmp, &deferred_sync, links.defer_sync) {
+ __device_links_supplier_sync_state(dev);
+ list_del_init(&dev->links.defer_sync);
+ }
+out:
+ device_links_write_unlock();
+}
+
+static void __device_links_supplier_defer_sync(struct device *sup)
+{
+ if (list_empty(&sup->links.defer_sync))
+ list_add_tail(&sup->links.defer_sync, &deferred_sync);
+}
+
/**
* device_links_driver_bound - Update device links after probing its driver.
* @dev: Device to update the links for.
@@ -692,6 +750,11 @@ void device_links_driver_bound(struct device *dev)
WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
WRITE_ONCE(link->status, DL_STATE_ACTIVE);
+
+ if (supplier_sync_state_disabled)
+ __device_links_supplier_defer_sync(link->supplier);
+ else
+ __device_links_supplier_sync_state(link->supplier);
}
dev->links.status = DL_DEV_DRIVER_BOUND;
@@ -808,6 +871,7 @@ void device_links_driver_cleanup(struct device *dev)
WRITE_ONCE(link->status, DL_STATE_DORMANT);
}
+ list_del_init(&dev->links.defer_sync);
__device_links_no_driver(dev);
device_links_write_unlock();
@@ -1782,6 +1846,7 @@ void device_initialize(struct device *dev)
INIT_LIST_HEAD(&dev->links.consumers);
INIT_LIST_HEAD(&dev->links.suppliers);
INIT_LIST_HEAD(&dev->links.needs_suppliers);
+ INIT_LIST_HEAD(&dev->links.defer_sync);
dev->links.status = DL_DEV_NO_DRIVER;
}
EXPORT_SYMBOL_GPL(device_initialize);
diff --git a/include/linux/device.h b/include/linux/device.h
index 76458cfbb267..7ab29bb3eb8c 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -80,6 +80,13 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
* that generate uevents to add the environment variables.
* @probe: Called when a new device or driver add to this bus, and callback
* the specific driver's probe to initial the matched device.
+ * @sync_state: Called to sync device state to software state after all the
+ * state tracking consumers linked to this device (present at
+ * the time of late_initcall) have successfully bound to a
+ * driver. If the device has no consumers, this function will
+ * be called at late_initcall_sync level. If the device has
+ * consumers that are never bound to a driver, this function
+ * will never get called until they do.
* @remove: Called when a device removed from this bus.
* @shutdown: Called at shut-down time to quiesce the device.
*
@@ -123,6 +130,7 @@ struct bus_type {
int (*match)(struct device *dev, struct device_driver *drv);
int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
int (*probe)(struct device *dev);
+ void (*sync_state)(struct device *dev);
int (*remove)(struct device *dev);
void (*shutdown)(struct device *dev);
@@ -340,6 +348,13 @@ enum probe_type {
* @probe: Called to query the existence of a specific device,
* whether this driver can work with it, and bind the driver
* to a specific device.
+ * @sync_state: Called to sync device state to software state after all the
+ * state tracking consumers linked to this device (present at
+ * the time of late_initcall) have successfully bound to a
+ * driver. If the device has no consumers, this function will
+ * be called at late_initcall_sync level. If the device has
+ * consumers that are never bound to a driver, this function
+ * will never get called until they do.
* @remove: Called when the device is removed from the system to
* unbind a device from this driver.
* @shutdown: Called at shut-down time to quiesce the device.
@@ -379,6 +394,7 @@ struct device_driver {
const struct acpi_device_id *acpi_match_table;
int (*probe) (struct device *dev);
+ void (*sync_state)(struct device *dev);
int (*remove) (struct device *dev);
void (*shutdown) (struct device *dev);
int (*suspend) (struct device *dev, pm_message_t state);
@@ -1132,12 +1148,14 @@ enum dl_dev_state {
* @suppliers: List of links to supplier devices.
* @consumers: List of links to consumer devices.
* @needs_suppliers: Hook to global list of devices waiting for suppliers.
+ * @defer_sync: Hook to global list of devices that have deferred sync_state.
* @status: Driver status information.
*/
struct dev_links_info {
struct list_head suppliers;
struct list_head consumers;
struct list_head needs_suppliers;
+ struct list_head defer_sync;
enum dl_dev_state status;
};
@@ -1213,6 +1231,9 @@ struct dev_links_info {
* @offline: Set after successful invocation of bus type's .offline().
* @of_node_reused: Set if the device-tree node is shared with an ancestor
* device.
+ * @state_synced: The hardware state of this device has been synced to match
+ * the software state of this device by calling the driver/bus
+ * sync_state() callback.
* @dma_coherent: this particular device is dma coherent, even if the
* architecture supports non-coherent devices.
*
@@ -1309,6 +1330,7 @@ struct device {
bool offline_disabled:1;
bool offline:1;
bool of_node_reused:1;
+ bool state_synced:1;
#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
@@ -1651,6 +1673,8 @@ struct device_link *device_link_add(struct device *consumer,
struct device *supplier, u32 flags);
void device_link_del(struct device_link *link);
void device_link_remove(void *consumer, struct device *supplier);
+void device_links_supplier_sync_state_pause(void);
+void device_links_supplier_sync_state_resume(void);
#ifndef dev_fmt
#define dev_fmt(fmt) fmt
--
2.23.0.187.g17f5b7556c-goog
^ permalink raw reply related
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Petr Mladek @ 2019-08-29 8:12 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Senozhatsky, Steven Rostedt, Andrew Morton, Jani Nikula,
Jonathan Corbet, metux IT consult Enrico Weigelt, linux-doc,
linux-kernel
In-Reply-To: <74303921-aa95-9962-2254-27e556af54f4@kleine-koenig.org>
On Wed 2019-08-28 21:18:37, Uwe Kleine-König wrote:
> Hello Petr,
>
> On 8/28/19 1:32 PM, Petr Mladek wrote:
> > On Tue 2019-08-27 23:12:44, Uwe Kleine-König wrote:
> >> Petr Mladek had some concerns:
> >>> There are ideas to make the code even more tricky to reduce
> >>> the size, keep it fast.
> >>
> >> I think Enrico Weigelt's suggestion to use a case is the best
> >> performance-wise so that's what I picked up. Also I hope that
> >> performance isn't that important because the need to print an error
> >> should not be so common that it really hurts in production.
This is contadicting. The "best" performance-wise solution was
choosen in favor of space. The next sentence says that performance
is not important.
> > I personally do not like switch/case. It is a lot of code.
> > I wonder if it even saved some space.
>
> I guess we have to die either way. Either it is quick or it is space
> efficient.
I am more concerned about the size. Well, array of strings will
be both fast and size efficient.
> With the big case I trust the compiler to pick something
> sensible expecting that it adapts for example to -Osize.
I am not sure what are the expectations here. I can't imagine
another translation than:
if (val == 1)
str = "EPERM";
else if (val == 2)
str = "ENOENT"
else if (val == 3)
str = "ESRCH"
...
It means that all constans will be hardcoded in the code instead
of in data section. Plus there will be instructions for each
if/else part.
> > If you want to safe space, I would use u16 to store the numbers.
> > Or I would use array of strings. There will be only few holes.
> >
> > You might also consider handling only the most commonly
> > used codes from errno.h and errno-base.h (1..133). There will
> > be no holes and the codes are stable.
>
> I'd like to postpone the discussion about "how" until we agreed about
> the "if at all".
It seems that all people like this feature.
BTW: I though more about generating or cut&pasting the arrary.
I can't find any reasonable way how to generate it.
But both, errno.h and errno-base.h, are super stable. Only
comments were modified or new codes added. Most of them
are defined by POSIX so they should remain stable.
Therefore cut&pasted array of strings looks acceptable.
We should only allow to easily check numbers for each code,
e.g. by defining the array as
const err_str * [] {
"0" /* 0 Success */
"EPERM", /* 1 Operation not permitted */
"ENOENT", /* 2 No such file or directory */
"ESRCH", /* 3 No such process */
...
If there is a hole, we could use something like:
"-41", /* 41 Skipped. EWOULDBLOCK is
defined as EAGAIN. Operation would block */
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Juergen Gross @ 2019-08-29 8:27 UTC (permalink / raw)
To: Petr Mladek, Uwe Kleine-König
Cc: Sergey Senozhatsky, Steven Rostedt, Andrew Morton, Jani Nikula,
Jonathan Corbet, metux IT consult Enrico Weigelt, linux-doc,
linux-kernel
In-Reply-To: <20190829081249.3zvvsa4ggb5pfozl@pathway.suse.cz>
On 29.08.19 10:12, Petr Mladek wrote:
> On Wed 2019-08-28 21:18:37, Uwe Kleine-König wrote:
>> Hello Petr,
>>
>> On 8/28/19 1:32 PM, Petr Mladek wrote:
>>> On Tue 2019-08-27 23:12:44, Uwe Kleine-König wrote:
>>>> Petr Mladek had some concerns:
>>>>> There are ideas to make the code even more tricky to reduce
>>>>> the size, keep it fast.
>>>>
>>>> I think Enrico Weigelt's suggestion to use a case is the best
>>>> performance-wise so that's what I picked up. Also I hope that
>>>> performance isn't that important because the need to print an error
>>>> should not be so common that it really hurts in production.
>
> This is contadicting. The "best" performance-wise solution was
> choosen in favor of space. The next sentence says that performance
> is not important.
>
>>> I personally do not like switch/case. It is a lot of code.
>>> I wonder if it even saved some space.
>>
>> I guess we have to die either way. Either it is quick or it is space
>> efficient.
>
> I am more concerned about the size. Well, array of strings will
> be both fast and size efficient.
>
>> With the big case I trust the compiler to pick something
>> sensible expecting that it adapts for example to -Osize.
>
> I am not sure what are the expectations here. I can't imagine
> another translation than:
>
> if (val == 1)
> str = "EPERM";
> else if (val == 2)
> str = "ENOENT"
> else if (val == 3)
> str = "ESRCH"
> ...
>
> It means that all constans will be hardcoded in the code instead
> of in data section. Plus there will be instructions for each
> if/else part.
>
>>> If you want to safe space, I would use u16 to store the numbers.
>>> Or I would use array of strings. There will be only few holes.
>>>
>>> You might also consider handling only the most commonly
>>> used codes from errno.h and errno-base.h (1..133). There will
>>> be no holes and the codes are stable.
>>
>> I'd like to postpone the discussion about "how" until we agreed about
>> the "if at all".
>
> It seems that all people like this feature.
Hmm, what about already existing format strings conatining "%dE"?
Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
nevertheless...
>
> BTW: I though more about generating or cut&pasting the arrary.
> I can't find any reasonable way how to generate it.
Generate the array and errno.h/errno-base.h from the same source?
Juergen
^ permalink raw reply
* Re: [PATCH v3 5/5] ARM: dts: stm32: add ddrperfm on stm32mp157c
From: Alexandre Torgue @ 2019-08-29 8:48 UTC (permalink / raw)
To: Gerald BAEZA, will@kernel.org, mark.rutland@arm.com,
robh+dt@kernel.org, mcoquelin.stm32@gmail.com, corbet@lwn.net,
linux@armlinux.org.uk, olof@lixom.net, arnd@arndb.de,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
In-Reply-To: <1566918464-23927-6-git-send-email-gerald.baeza@st.com>
Hi Gerald
On 8/27/19 5:08 PM, Gerald BAEZA wrote:
> The DDRPERFM is the DDR Performance Monitor embedded
> in STM32MP1 SOC.
>
> Signed-off-by: Gerald Baeza <gerald.baeza@st.com>
> ---
> arch/arm/boot/dts/stm32mp157c.dtsi | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi
> b/arch/arm/boot/dts/stm32mp157c.dtsi
> index 0c4e6eb..6ea6933 100644
> --- a/arch/arm/boot/dts/stm32mp157c.dtsi
> +++ b/arch/arm/boot/dts/stm32mp157c.dtsi
> @@ -1378,6 +1378,14 @@
> };
> };
>
> + ddrperfm: perf@5a007000 {
> + compatible = "st,stm32-ddr-pmu";
> + reg = <0x5a007000 0x400>;
> + clocks = <&rcc DDRPERFM>;
> + resets = <&rcc DDRPERFM_R>;
> + status = "okay";
No need to add "status = "okay"" here.
regards
Alex
> + };
> +
> usart1: serial@5c000000 {
> compatible = "st,stm32h7-uart";
> reg = <0x5c000000 0x400>;
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Rasmus Villemoes @ 2019-08-29 9:00 UTC (permalink / raw)
To: Petr Mladek, Uwe Kleine-König
Cc: Sergey Senozhatsky, Steven Rostedt, Andrew Morton, Jani Nikula,
Jonathan Corbet, metux IT consult Enrico Weigelt, linux-doc,
linux-kernel
In-Reply-To: <20190829081249.3zvvsa4ggb5pfozl@pathway.suse.cz>
On 29/08/2019 10.12, Petr Mladek wrote:
> On Wed 2019-08-28 21:18:37, Uwe Kleine-König wrote:
> BTW: I though more about generating or cut&pasting the arrary.
> I can't find any reasonable way how to generate it.
Something like this seems to work, though it probably needs some massage
to be accepted by kbuild folks:
define filechk_errcode.h
echo '#include <linux/errno.h>' | \
$(CPP) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) -dM - | \
grep 'define E' | sort -k3,3n | \
awk '{print "errcode("$$2")\t/* "$$3" */"}'
endef
include/generated/errcode.h: FORCE
$(call filechk,errcode.h)
Then one can just #define errcode(foo) ... right before #include
<generated/errcode.h>. It cannot be used to generate cases in a switch()
because some expand to the same number, but that's ok, because I can't
imagine the switch actually generating good or small code. I haven't
checked how or whether it works in a cross-compile situation.
Rasmus
^ permalink raw reply
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Rasmus Villemoes @ 2019-08-29 9:09 UTC (permalink / raw)
To: Juergen Gross, Petr Mladek, Uwe Kleine-König
Cc: Sergey Senozhatsky, Steven Rostedt, Andrew Morton, Jani Nikula,
Jonathan Corbet, metux IT consult Enrico Weigelt, linux-doc,
linux-kernel
In-Reply-To: <45cd5b50-9854-fce7-5f08-f7660abb8691@suse.com>
On 29/08/2019 10.27, Juergen Gross wrote:
> On 29.08.19 10:12, Petr Mladek wrote:
>> On Wed 2019-08-28 21:18:37, Uwe Kleine-König wrote:
>>>
>>> I'd like to postpone the discussion about "how" until we agreed about
>>> the "if at all".
>>
>> It seems that all people like this feature.
>
> Hmm, what about already existing format strings conatining "%dE"?
>
> Yes, I could find only one (drivers/staging/speakup/speakup_bns.c), but
> nevertheless...
Indeed, Uwe still needs to respond to how he wants to handle that. I
still prefer making it %pE, both because it's easier to convert integers
to ERR_PTRs than having to worry about the type of PTR_ERR() being long
and not int, and because alphanumerics after %p have been ignored for a
long time (10 years?) whether or not those characters have been
recognized as a %p extension, so nobody relies on %pE putting an E after
the %p output. It also keeps the non-standard extensions in the same
"namespace", so to speak.
Oh, 'E' is taken, well, make it 'e' then.
Rasmus
^ permalink raw reply
* Re: [PATCH v2 1/1] counter: cros_ec: Add synchronization sensor
From: Fabien Lahoudere @ 2019-08-29 11:10 UTC (permalink / raw)
To: Jonathan Cameron
Cc: gwendal, egranata, kernel, William Breathitt Gray,
Jonathan Corbet, Benson Leung, Enric Balletbo i Serra,
Guenter Roeck, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Lee Jones, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre, Nick Vaccaro,
linux-iio, linux-doc, linux-kernel
In-Reply-To: <20190826095612.7455cb05@archlinux>
Le lundi 26 août 2019 à 09:56 +0100, Jonathan Cameron a écrit :
> On Fri, 23 Aug 2019 14:41:27 +0200
> Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
>
> > From: Gwendal Grignou <gwendal@chromium.org>
> >
> > EC returns a counter when there is an event on camera vsync.
> > This patch comes from chromeos kernel 4.4
> >
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> >
> > CROS EC sync sensor was originally designed as an IIO device.
> > Now that the counter subsystem will replace IIO_COUNTER, we
> > have to implement a new way to get sync count.
>
> I'm curious. What is this counter used for?
It is a counter of a pin connected to a camera vsync.
It is used to manage correct orientation of the camera when the
gyroscope detect a change.
>
> This combined counter and iio driver isn't something we would
> normally
> want to support. What is the reasoning behind doing both interfaces?
That patch was originally designed using only IIO with IIO_count and
IIO_TIMESTAMP channel. As IIO_COUNT is deprecated, we need to use the
new counter subsystem. So I just split the driver in two part counter
and iio timestamp.
However after your email I did some investigation and did not find any
use of the timestamp for that sensor. Timestamp is just a common
channel implemented on all CROS EC sensors.
So I think I will drop it now and if a timestamp is needed, I will
probably restart the discussion about timestamp in the counter
subsystemif that make sense to have one.
Thanks
Fabien
>
> > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> > ---
> > Documentation/driver-api/generic-counter.rst | 3 +
> > MAINTAINERS | 7 +
> > drivers/counter/Kconfig | 9 +
> > drivers/counter/Makefile | 1 +
> > drivers/counter/counter.c | 2 +
> > drivers/counter/cros_ec_sensors_sync.c | 208
> > ++++++++++++++++++
> > .../cros_ec_sensors/cros_ec_sensors_core.c | 1 +
> > drivers/mfd/cros_ec_dev.c | 3 +
> > include/linux/counter.h | 1 +
> > 9 files changed, 235 insertions(+)
> > create mode 100644 drivers/counter/cros_ec_sensors_sync.c
> >
> > diff --git a/Documentation/driver-api/generic-counter.rst
> > b/Documentation/driver-api/generic-counter.rst
> > index 8382f01a53e3..beb80714ac8b 100644
> > --- a/Documentation/driver-api/generic-counter.rst
> > +++ b/Documentation/driver-api/generic-counter.rst
> > @@ -44,6 +44,9 @@ Counter interface provides the following
> > available count data types:
> > * COUNT_POSITION:
> > Unsigned integer value representing position.
> >
> > +* COUNT_TALLY:
> > + Unsigned integer value representing tally.
> > +
> > A Count has a count function mode which represents the update
> > behavior
> > for the count data. The Generic Counter interface provides the
> > following
> > available count function modes:
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e60f5c361969..83bd291d103e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3902,6 +3902,13 @@ R: Guenter Roeck <groeck@chromium.org>
> > F: Documentation/devicetree/bindings/sound/google,cros-ec-
> > codec.txt
> > F: sound/soc/codecs/cros_ec_codec.*
> >
> > +CHROMEOS EC COUNTER DRIVER
> > +M: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> > +M: William Breathitt Gray <vilhelm.gray@gmail.com>
> > +L: linux-iio@vger.kernel.org
> > +S: Maintained
> > +F: drivers/counter/cros_ec_sensors_sync.c
> > +
> > CIRRUS LOGIC AUDIO CODEC DRIVERS
> > M: Brian Austin <brian.austin@cirrus.com>
> > M: Paul Handrigan <Paul.Handrigan@cirrus.com>
> > diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> > index 2967d0a9ff91..22287f5715e5 100644
> > --- a/drivers/counter/Kconfig
> > +++ b/drivers/counter/Kconfig
> > @@ -59,4 +59,13 @@ config FTM_QUADDEC
> > To compile this driver as a module, choose M here: the
> > module will be called ftm-quaddec.
> >
> > +config IIO_CROS_EC_SENSORS_SYNC
> > + tristate "ChromeOS EC Counter Sensors"
> > + depends on IIO_CROS_EC_SENSORS_CORE && IIO
> > + help
> > + Module to handle synchronisation sensors presented by the
> > ChromeOS EC
> > + Sensor hub.
> > + Synchronisation sensors are counter sensors triggered when
> > events
> > + occurs from other subsystems.
> > +
> > endif # COUNTER
> > diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> > index 40d35522937d..6fe4c98a446f 100644
> > --- a/drivers/counter/Makefile
> > +++ b/drivers/counter/Makefile
> > @@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o
> > obj-$(CONFIG_STM32_TIMER_CNT) += stm32-timer-cnt.o
> > obj-$(CONFIG_STM32_LPTIMER_CNT) += stm32-lptimer-cnt.o
> > obj-$(CONFIG_FTM_QUADDEC) += ftm-quaddec.o
> > +obj-$(CONFIG_IIO_CROS_EC_SENSORS_SYNC) += cros_ec_sensors_sync.o
> > diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
> > index 106bc7180cd8..53525b109094 100644
> > --- a/drivers/counter/counter.c
> > +++ b/drivers/counter/counter.c
> > @@ -261,6 +261,7 @@ void counter_count_read_value_set(struct
> > counter_count_read_value *const val,
> > {
> > switch (type) {
> > case COUNTER_COUNT_POSITION:
> > + case COUNTER_COUNT_TALLY:
> > val->len = sprintf(val->buf, "%lu\n", *(unsigned long
> > *)data);
> > break;
> > default:
> > @@ -290,6 +291,7 @@ int counter_count_write_value_get(void *const
> > data,
> >
> > switch (type) {
> > case COUNTER_COUNT_POSITION:
> > + case COUNTER_COUNT_TALLY:
> > err = kstrtoul(val->buf, 0, data);
> > if (err)
> > return err;
> > diff --git a/drivers/counter/cros_ec_sensors_sync.c
> > b/drivers/counter/cros_ec_sensors_sync.c
> > new file mode 100644
> > index 000000000000..b6f5e2c6da9f
> > --- /dev/null
> > +++ b/drivers/counter/cros_ec_sensors_sync.c
> > @@ -0,0 +1,208 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver of counter incremented after events on interrupt line in
> > EC.
> > + *
> > + * Copyright 2018 Google, Inc
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/counter.h>
> > +#include <linux/iio/common/cros_ec_sensors_core.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/cros_ec.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define DRV_NAME "cros-ec-sync"
> > +
> > +/*
> > + * One channel for counter, the other for timestamp.
> > + */
> > +#define MAX_CHANNELS (1)
> > +
> > +/* State data for ec_sensors iio driver. */
> > +struct cros_ec_sensors_sync_state {
> > + /* Shared by all sensors */
> > + struct cros_ec_sensors_core_state core;
> > + struct counter_device counter;
> > + struct iio_chan_spec channels[MAX_CHANNELS];
> > +};
> > +
> > +static int cros_ec_sensors_sync_read(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct cros_ec_sensors_sync_state *st = iio_priv(indio_dev);
> > + u16 data;
> > + int ret;
> > +
> > + mutex_lock(&st->core.cmd_lock);
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0),
> > &data);
> > + if (ret < 0)
> > + break;
> > + ret = IIO_VAL_INT;
> > + *val = data;
> > + break;
> > + default:
> > + ret = cros_ec_sensors_core_read(&st->core, chan, val,
> > val2,
> > + mask);
> > + break;
> > + }
> > + mutex_unlock(&st->core.cmd_lock);
> > + return ret;
> > +}
> > +
> > +static struct iio_info cros_ec_sensors_sync_info = {
> > + .read_raw = &cros_ec_sensors_sync_read,
> > + .read_avail = &cros_ec_sensors_core_read_avail,
> > +};
> > +
> > +static struct counter_count cros_ec_sync_counts = {
> > + .id = 0,
> > + .name = "Cros EC sync counter",
> > +};
> > +
> > +static int cros_ec_sync_cnt_read(struct counter_device *counter,
> > + struct counter_count *count,
> > + struct counter_count_read_value *val)
> > +{
> > + s16 cnt;
> > + int ret;
> > + struct iio_dev *indio_dev = counter->priv;
> > + struct cros_ec_sensors_sync_state *const st =
> > iio_priv(indio_dev);
> > + unsigned long data;
> > +
> > + mutex_lock(&st->core.cmd_lock);
> > + ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &cnt);
> > + mutex_unlock(&st->core.cmd_lock);
> > + if (ret != 0) {
> > + dev_warn(&indio_dev->dev, "Unable to read sensor
> > data\n");
> > + return ret;
> > + }
> > +
> > + data = (unsigned long) cnt;
> > + counter_count_read_value_set(val, COUNTER_COUNT_TALLY, &data);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct counter_ops cros_ec_sync_cnt_ops = {
> > + .count_read = cros_ec_sync_cnt_read,
> > +};
> > +
> > +static char *cros_ec_loc[] = {
> > + [MOTIONSENSE_LOC_BASE] = "base",
> > + [MOTIONSENSE_LOC_LID] = "lid",
> > + [MOTIONSENSE_LOC_CAMERA] = "camera",
> > + [MOTIONSENSE_LOC_MAX] = "unknown",
> > +};
> > +
> > +static ssize_t cros_ec_sync_id(struct counter_device *counter,
> > + void *private, char *buf)
> > +{
> > + struct iio_dev *indio_dev = counter->priv;
> > + struct cros_ec_sensors_sync_state *const st =
> > iio_priv(indio_dev);
> > +
> > + return snprintf(buf, PAGE_SIZE, "%d\n", st-
> > >core.param.info.sensor_num);
> > +}
> > +
> > +static ssize_t cros_ec_sync_loc(struct counter_device *counter,
> > + void *private, char *buf)
> > +{
> > + struct iio_dev *indio_dev = counter->priv;
> > + struct cros_ec_sensors_sync_state *const st =
> > iio_priv(indio_dev);
> > +
> > + return snprintf(buf, PAGE_SIZE, "%s\n", cros_ec_loc[st-
> > >core.loc]);
> > +}
> > +
> > +static struct counter_device_ext cros_ec_sync_cnt_ext[] = {
> > + {
> > + .name = "id",
> > + .read = cros_ec_sync_id
> > + },
> > + {
> > + .name = "location",
> > + .read = cros_ec_sync_loc
> > + },
> > +};
> > +
> > +static int cros_ec_sensors_sync_probe(struct platform_device
> > *pdev)
> > +{
> > + struct cros_ec_sensors_sync_state *state;
> > + struct device *dev = &pdev->dev;
> > + struct iio_chan_spec *channel;
> > + struct iio_dev *indio_dev;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->info = &cros_ec_sensors_sync_info;
> > + state = iio_priv(indio_dev);
> > +
> > + if (state->core.type != MOTIONSENSE_TYPE_SYNC)
> > + return -EINVAL;
> > +
> > + /* Initialize IIO device */
> > + channel = state->channels;
> > + channel->type = IIO_TIMESTAMP;
> > + channel->channel = -1;
> > + channel->scan_index = 1;
> > + channel->scan_type.sign = 's';
> > + channel->scan_type.realbits = 64;
> > + channel->scan_type.storagebits = 64;
> > +
> > + indio_dev->channels = state->channels;
> > + indio_dev->num_channels = MAX_CHANNELS;
> > +
> > + state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> > +
> > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > + cros_ec_sensors_capture,
> > NULL);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_iio_device_register(dev, indio_dev);
>
> Hmm. Wasn't expecting to see that here if it's a counter device.
>
>
> > + if (ret)
> > + return ret;
> > +
> > + /* Initialize counter device */
> > + state->counter.name = dev_name(&pdev->dev);
> > + state->counter.parent = &pdev->dev;
> > + state->counter.counts = &cros_ec_sync_counts;
> > + state->counter.num_counts = 1;
> > + state->counter.priv = indio_dev;
> > + state->counter.ops = &cros_ec_sync_cnt_ops;
> > + state->counter.ext = cros_ec_sync_cnt_ext;
> > + state->counter.num_ext = ARRAY_SIZE(cros_ec_sync_cnt_ext);
> > +
> > + return devm_counter_register(&pdev->dev, &state->counter);
> > +}
> > +
> > +static const struct platform_device_id cros_ec_sensors_sync_ids[]
> > = {
> > + { .name = DRV_NAME, },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(platform, cros_ec_sensors_sync_ids);
> > +
> > +static struct platform_driver cros_ec_sensors_sync_platform_driver
> > = {
> > + .driver = {
> > + .name = DRV_NAME,
> > + .pm = &cros_ec_sensors_pm_ops,
> > + },
> > + .probe = cros_ec_sensors_sync_probe,
> > + .id_table = cros_ec_sensors_sync_ids,
> > +};
> > +module_platform_driver(cros_ec_sensors_sync_platform_driver);
> > +
> > +MODULE_DESCRIPTION("ChromeOS EC synchronisation sensor driver");
> > +MODULE_ALIAS("platform:" DRV_NAME);
> > +MODULE_LICENSE("GPL v2");
> > diff --git
> > a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > index 805652250960..2bf183425eaf 100644
> > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > @@ -22,6 +22,7 @@
> > static char *cros_ec_loc[] = {
> > [MOTIONSENSE_LOC_BASE] = "base",
> > [MOTIONSENSE_LOC_LID] = "lid",
> > + [MOTIONSENSE_LOC_CAMERA] = "camera",
> > [MOTIONSENSE_LOC_MAX] = "unknown",
> > };
> >
> > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > index 41dccced5026..1c5c2c38af88 100644
> > --- a/drivers/mfd/cros_ec_dev.c
> > +++ b/drivers/mfd/cros_ec_dev.c
> > @@ -332,6 +332,9 @@ static void cros_ec_sensors_register(struct
> > cros_ec_dev *ec)
> > case MOTIONSENSE_TYPE_ACTIVITY:
> > sensor_cells[id].name = "cros-ec-activity";
> > break;
> > + case MOTIONSENSE_TYPE_SYNC:
> > + sensor_cells[id].name = "cros-ec-sync";
> > + break;
> > default:
> > dev_warn(ec->dev, "unknown type %d\n", resp-
> > >info.type);
> > continue;
> > diff --git a/include/linux/counter.h b/include/linux/counter.h
> > index a061cdcdef7c..1198e675306f 100644
> > --- a/include/linux/counter.h
> > +++ b/include/linux/counter.h
> > @@ -488,6 +488,7 @@ enum counter_signal_value_type {
> >
> > enum counter_count_value_type {
> > COUNTER_COUNT_POSITION = 0,
> > + COUNTER_COUNT_TALLY
> > };
> >
> > void counter_signal_read_value_set(struct
> > counter_signal_read_value *const val,
^ permalink raw reply
* Re: [PATCH v2 1/1] counter: cros_ec: Add synchronization sensor
From: William Breathitt Gray @ 2019-08-29 11:34 UTC (permalink / raw)
To: Fabien Lahoudere
Cc: gwendal, egranata, kernel, Jonathan Corbet, Benson Leung,
Enric Balletbo i Serra, Guenter Roeck, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Lee Jones, Mauro Carvalho Chehab, David S. Miller,
Greg Kroah-Hartman, Nicolas Ferre, Nick Vaccaro, linux-iio,
linux-doc, linux-kernel
In-Reply-To: <d985a8a811996148e8cda78b9fe47bb87b884b56.1566563833.git.fabien.lahoudere@collabora.com>
On Fri, Aug 23, 2019 at 02:41:27PM +0200, Fabien Lahoudere wrote:
> From: Gwendal Grignou <gwendal@chromium.org>
>
> EC returns a counter when there is an event on camera vsync.
> This patch comes from chromeos kernel 4.4
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
>
> CROS EC sync sensor was originally designed as an IIO device.
> Now that the counter subsystem will replace IIO_COUNTER, we
> have to implement a new way to get sync count.
>
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> ---
> Documentation/driver-api/generic-counter.rst | 3 +
> MAINTAINERS | 7 +
> drivers/counter/Kconfig | 9 +
> drivers/counter/Makefile | 1 +
> drivers/counter/counter.c | 2 +
> drivers/counter/cros_ec_sensors_sync.c | 208 ++++++++++++++++++
> .../cros_ec_sensors/cros_ec_sensors_core.c | 1 +
> drivers/mfd/cros_ec_dev.c | 3 +
> include/linux/counter.h | 1 +
> 9 files changed, 235 insertions(+)
> create mode 100644 drivers/counter/cros_ec_sensors_sync.c
>
> diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
> index 8382f01a53e3..beb80714ac8b 100644
> --- a/Documentation/driver-api/generic-counter.rst
> +++ b/Documentation/driver-api/generic-counter.rst
> @@ -44,6 +44,9 @@ Counter interface provides the following available count data types:
> * COUNT_POSITION:
> Unsigned integer value representing position.
>
> +* COUNT_TALLY:
> + Unsigned integer value representing tally.
> +
> A Count has a count function mode which represents the update behavior
> for the count data. The Generic Counter interface provides the following
> available count function modes:
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e60f5c361969..83bd291d103e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3902,6 +3902,13 @@ R: Guenter Roeck <groeck@chromium.org>
> F: Documentation/devicetree/bindings/sound/google,cros-ec-codec.txt
> F: sound/soc/codecs/cros_ec_codec.*
>
> +CHROMEOS EC COUNTER DRIVER
> +M: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> +M: William Breathitt Gray <vilhelm.gray@gmail.com>
No need to include me here since I'm already listed as the maintainer of
the Counter subsystem in its respective entry.
> +L: linux-iio@vger.kernel.org
> +S: Maintained
> +F: drivers/counter/cros_ec_sensors_sync.c
> +
> CIRRUS LOGIC AUDIO CODEC DRIVERS
> M: Brian Austin <brian.austin@cirrus.com>
> M: Paul Handrigan <Paul.Handrigan@cirrus.com>
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 2967d0a9ff91..22287f5715e5 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -59,4 +59,13 @@ config FTM_QUADDEC
> To compile this driver as a module, choose M here: the
> module will be called ftm-quaddec.
>
> +config IIO_CROS_EC_SENSORS_SYNC
> + tristate "ChromeOS EC Counter Sensors"
> + depends on IIO_CROS_EC_SENSORS_CORE && IIO
> + help
> + Module to handle synchronisation sensors presented by the ChromeOS EC
> + Sensor hub.
> + Synchronisation sensors are counter sensors triggered when events
> + occurs from other subsystems.
> +
> endif # COUNTER
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index 40d35522937d..6fe4c98a446f 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o
> obj-$(CONFIG_STM32_TIMER_CNT) += stm32-timer-cnt.o
> obj-$(CONFIG_STM32_LPTIMER_CNT) += stm32-lptimer-cnt.o
> obj-$(CONFIG_FTM_QUADDEC) += ftm-quaddec.o
> +obj-$(CONFIG_IIO_CROS_EC_SENSORS_SYNC) += cros_ec_sensors_sync.o
> diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
> index 106bc7180cd8..53525b109094 100644
> --- a/drivers/counter/counter.c
> +++ b/drivers/counter/counter.c
> @@ -261,6 +261,7 @@ void counter_count_read_value_set(struct counter_count_read_value *const val,
> {
> switch (type) {
> case COUNTER_COUNT_POSITION:
> + case COUNTER_COUNT_TALLY:
> val->len = sprintf(val->buf, "%lu\n", *(unsigned long *)data);
> break;
> default:
> @@ -290,6 +291,7 @@ int counter_count_write_value_get(void *const data,
>
> switch (type) {
> case COUNTER_COUNT_POSITION:
> + case COUNTER_COUNT_TALLY:
> err = kstrtoul(val->buf, 0, data);
> if (err)
> return err;
Dedicate the core counter subsystem related changes to their own patch
so that we can commit them separately. That'll make it easier both to
evaluate the changes and debug later by keeping the history clear.
> diff --git a/drivers/counter/cros_ec_sensors_sync.c b/drivers/counter/cros_ec_sensors_sync.c
> new file mode 100644
> index 000000000000..b6f5e2c6da9f
> --- /dev/null
> +++ b/drivers/counter/cros_ec_sensors_sync.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver of counter incremented after events on interrupt line in EC.
> + *
> + * Copyright 2018 Google, Inc
> + */
> +
> +#include <linux/device.h>
> +#include <linux/counter.h>
> +#include <linux/iio/common/cros_ec_sensors_core.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME "cros-ec-sync"
> +
> +/*
> + * One channel for counter, the other for timestamp.
> + */
> +#define MAX_CHANNELS (1)
> +
> +/* State data for ec_sensors iio driver. */
> +struct cros_ec_sensors_sync_state {
> + /* Shared by all sensors */
> + struct cros_ec_sensors_core_state core;
> + struct counter_device counter;
> + struct iio_chan_spec channels[MAX_CHANNELS];
> +};
> +
> +static int cros_ec_sensors_sync_read(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct cros_ec_sensors_sync_state *st = iio_priv(indio_dev);
> + u16 data;
> + int ret;
> +
> + mutex_lock(&st->core.cmd_lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &data);
> + if (ret < 0)
> + break;
> + ret = IIO_VAL_INT;
> + *val = data;
> + break;
> + default:
> + ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
> + mask);
> + break;
> + }
> + mutex_unlock(&st->core.cmd_lock);
> + return ret;
> +}
> +
> +static struct iio_info cros_ec_sensors_sync_info = {
> + .read_raw = &cros_ec_sensors_sync_read,
> + .read_avail = &cros_ec_sensors_core_read_avail,
> +};
> +
> +static struct counter_count cros_ec_sync_counts = {
> + .id = 0,
> + .name = "Cros EC sync counter",
> +};
> +
> +static int cros_ec_sync_cnt_read(struct counter_device *counter,
> + struct counter_count *count,
> + struct counter_count_read_value *val)
> +{
> + s16 cnt;
> + int ret;
> + struct iio_dev *indio_dev = counter->priv;
> + struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
> + unsigned long data;
> +
> + mutex_lock(&st->core.cmd_lock);
> + ret = cros_ec_sensors_read_cmd(indio_dev, BIT(0), &cnt);
> + mutex_unlock(&st->core.cmd_lock);
> + if (ret != 0) {
> + dev_warn(&indio_dev->dev, "Unable to read sensor data\n");
> + return ret;
> + }
> +
> + data = (unsigned long) cnt;
This cast is not necessary since the data variable is already unsigned
long; conversion can occur implicitly with a simple set operation.
> + counter_count_read_value_set(val, COUNTER_COUNT_TALLY, &data);
> +
> + return 0;
> +}
> +
> +static const struct counter_ops cros_ec_sync_cnt_ops = {
> + .count_read = cros_ec_sync_cnt_read,
> +};
Like in my previous review: does the hardware allow the counter to be
reset back to zero, or is it purely read-only? It's okay if you want to
focus on just reading for this patch, but it'll be good to know the
capabilities of this hardware if we want support for it in the future.
> +
> +static char *cros_ec_loc[] = {
> + [MOTIONSENSE_LOC_BASE] = "base",
> + [MOTIONSENSE_LOC_LID] = "lid",
> + [MOTIONSENSE_LOC_CAMERA] = "camera",
> + [MOTIONSENSE_LOC_MAX] = "unknown",
> +};
> +
> +static ssize_t cros_ec_sync_id(struct counter_device *counter,
> + void *private, char *buf)
> +{
> + struct iio_dev *indio_dev = counter->priv;
> + struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", st->core.param.info.sensor_num);
> +}
> +
> +static ssize_t cros_ec_sync_loc(struct counter_device *counter,
> + void *private, char *buf)
> +{
> + struct iio_dev *indio_dev = counter->priv;
> + struct cros_ec_sensors_sync_state *const st = iio_priv(indio_dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%s\n", cros_ec_loc[st->core.loc]);
> +}
> +
> +static struct counter_device_ext cros_ec_sync_cnt_ext[] = {
> + {
> + .name = "id",
> + .read = cros_ec_sync_id
> + },
> + {
> + .name = "location",
> + .read = cros_ec_sync_loc
> + },
> +};
Create a corresponding documentation file to describe these attributes:
Documentation/ABI/testing/sysfs-bus-counter-cros_ec_sensors_sync
> +
> +static int cros_ec_sensors_sync_probe(struct platform_device *pdev)
> +{
> + struct cros_ec_sensors_sync_state *state;
> + struct device *dev = &pdev->dev;
> + struct iio_chan_spec *channel;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> + if (ret)
> + return ret;
> +
> + indio_dev->info = &cros_ec_sensors_sync_info;
> + state = iio_priv(indio_dev);
> +
> + if (state->core.type != MOTIONSENSE_TYPE_SYNC)
> + return -EINVAL;
> +
> + /* Initialize IIO device */
> + channel = state->channels;
> + channel->type = IIO_TIMESTAMP;
> + channel->channel = -1;
> + channel->scan_index = 1;
> + channel->scan_type.sign = 's';
> + channel->scan_type.realbits = 64;
> + channel->scan_type.storagebits = 64;
> +
> + indio_dev->channels = state->channels;
> + indio_dev->num_channels = MAX_CHANNELS;
> +
> + state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> + cros_ec_sensors_capture, NULL);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + /* Initialize counter device */
> + state->counter.name = dev_name(&pdev->dev);
> + state->counter.parent = &pdev->dev;
> + state->counter.counts = &cros_ec_sync_counts;
> + state->counter.num_counts = 1;
> + state->counter.priv = indio_dev;
> + state->counter.ops = &cros_ec_sync_cnt_ops;
> + state->counter.ext = cros_ec_sync_cnt_ext;
> + state->counter.num_ext = ARRAY_SIZE(cros_ec_sync_cnt_ext);
> +
> + return devm_counter_register(&pdev->dev, &state->counter);
> +}
> +
> +static const struct platform_device_id cros_ec_sensors_sync_ids[] = {
> + { .name = DRV_NAME, },
> + { }
> +};
> +MODULE_DEVICE_TABLE(platform, cros_ec_sensors_sync_ids);
> +
> +static struct platform_driver cros_ec_sensors_sync_platform_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .pm = &cros_ec_sensors_pm_ops,
> + },
> + .probe = cros_ec_sensors_sync_probe,
> + .id_table = cros_ec_sensors_sync_ids,
> +};
> +module_platform_driver(cros_ec_sensors_sync_platform_driver);
> +
> +MODULE_DESCRIPTION("ChromeOS EC synchronisation sensor driver");
> +MODULE_ALIAS("platform:" DRV_NAME);
> +MODULE_LICENSE("GPL v2");
From the email you sent in response to Jonathan Cameron's comments, the
IIO code in this driver will be removed so that should take care of the
error message reported by the kbuild test robot. With only the Counter
subsystem to worry about, this driver should end up a lot simpler in v3.
I'm looking forward to it. :-)
William Breathitt Gray
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index 805652250960..2bf183425eaf 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -22,6 +22,7 @@
> static char *cros_ec_loc[] = {
> [MOTIONSENSE_LOC_BASE] = "base",
> [MOTIONSENSE_LOC_LID] = "lid",
> + [MOTIONSENSE_LOC_CAMERA] = "camera",
> [MOTIONSENSE_LOC_MAX] = "unknown",
> };
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 41dccced5026..1c5c2c38af88 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -332,6 +332,9 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> case MOTIONSENSE_TYPE_ACTIVITY:
> sensor_cells[id].name = "cros-ec-activity";
> break;
> + case MOTIONSENSE_TYPE_SYNC:
> + sensor_cells[id].name = "cros-ec-sync";
> + break;
> default:
> dev_warn(ec->dev, "unknown type %d\n", resp->info.type);
> continue;
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index a061cdcdef7c..1198e675306f 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -488,6 +488,7 @@ enum counter_signal_value_type {
>
> enum counter_count_value_type {
> COUNTER_COUNT_POSITION = 0,
> + COUNTER_COUNT_TALLY
> };
>
> void counter_signal_read_value_set(struct counter_signal_read_value *const val,
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants
From: Andy Shevchenko @ 2019-08-29 13:27 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Andrew Morton, Jonathan Corbet, Linux Documentation List,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM,
Linus Walleij, Bartosz Golaszewski
In-Reply-To: <20190824233724.1775-1-uwe@kleine-koenig.org>
On Sun, Aug 25, 2019 at 2:40 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> pr_info("probing failed (%dE)\n", ret);
>
> expands to
>
> probing failed (EIO)
>
> if ret holds -EIO (or EIO). This introduces an array of error codes. If
> the error code is missing, %dE falls back to %d and so prints the plain
> number.
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello
>
> there are many code sites that benefit from this. Just grep for
> "(%d)" ...
>
> As an example the follow up patch converts a printk to use this new
> format escape.
>
Let's not do this!
We have already a lot of pain with pointer extension, but here is just
a misleading stuff.
Besides above, how you print (float) number out of kernel in sysfs in
very well standard format?
Please, use %p<SMTH> instead.
> Best regards
> Uwe
>
> Documentation/core-api/printk-formats.rst | 3 +
> lib/vsprintf.c | 193 +++++++++++++++++++++-
> 2 files changed, 195 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index c6224d039bcb..81002414f956 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -35,6 +35,9 @@ Integer types
> u64 %llu or %llx
>
>
> +To print the name that corresponds to an integer error constant, use %dE and
> +pass the int.
> +
> If <type> is dependent on a config option for its size (e.g., sector_t,
> blkcnt_t) or is architecture-dependent for its size (e.g., tcflag_t), use a
> format specifier of its largest possible type and explicitly cast to it.
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b0967cf17137..672eab8dab84 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
> return buf;
> }
>
> +#define ERRORCODE(x) { .str = #x, .err = x }
> +
> +static const struct {
> + const char *str;
> + int err;
> +} errorcodes[] = {
> + ERRORCODE(EPERM),
> + ERRORCODE(ENOENT),
> + ERRORCODE(ESRCH),
> + ERRORCODE(EINTR),
> + ERRORCODE(EIO),
> + ERRORCODE(ENXIO),
> + ERRORCODE(E2BIG),
> + ERRORCODE(ENOEXEC),
> + ERRORCODE(EBADF),
> + ERRORCODE(ECHILD),
> + ERRORCODE(EAGAIN),
> + ERRORCODE(ENOMEM),
> + ERRORCODE(EACCES),
> + ERRORCODE(EFAULT),
> + ERRORCODE(ENOTBLK),
> + ERRORCODE(EBUSY),
> + ERRORCODE(EEXIST),
> + ERRORCODE(EXDEV),
> + ERRORCODE(ENODEV),
> + ERRORCODE(ENOTDIR),
> + ERRORCODE(EISDIR),
> + ERRORCODE(EINVAL),
> + ERRORCODE(ENFILE),
> + ERRORCODE(EMFILE),
> + ERRORCODE(ENOTTY),
> + ERRORCODE(ETXTBSY),
> + ERRORCODE(EFBIG),
> + ERRORCODE(ENOSPC),
> + ERRORCODE(ESPIPE),
> + ERRORCODE(EROFS),
> + ERRORCODE(EMLINK),
> + ERRORCODE(EPIPE),
> + ERRORCODE(EDOM),
> + ERRORCODE(ERANGE),
> + ERRORCODE(EDEADLK),
> + ERRORCODE(ENAMETOOLONG),
> + ERRORCODE(ENOLCK),
> + ERRORCODE(ENOSYS),
> + ERRORCODE(ENOTEMPTY),
> + ERRORCODE(ELOOP),
> + ERRORCODE(EWOULDBLOCK),
> + ERRORCODE(ENOMSG),
> + ERRORCODE(EIDRM),
> + ERRORCODE(ECHRNG),
> + ERRORCODE(EL2NSYNC),
> + ERRORCODE(EL3HLT),
> + ERRORCODE(EL3RST),
> + ERRORCODE(ELNRNG),
> + ERRORCODE(EUNATCH),
> + ERRORCODE(ENOCSI),
> + ERRORCODE(EL2HLT),
> + ERRORCODE(EBADE),
> + ERRORCODE(EBADR),
> + ERRORCODE(EXFULL),
> + ERRORCODE(ENOANO),
> + ERRORCODE(EBADRQC),
> + ERRORCODE(EBADSLT),
> + ERRORCODE(EBFONT),
> + ERRORCODE(ENOSTR),
> + ERRORCODE(ENODATA),
> + ERRORCODE(ETIME),
> + ERRORCODE(ENOSR),
> + ERRORCODE(ENONET),
> + ERRORCODE(ENOPKG),
> + ERRORCODE(EREMOTE),
> + ERRORCODE(ENOLINK),
> + ERRORCODE(EADV),
> + ERRORCODE(ESRMNT),
> + ERRORCODE(ECOMM),
> + ERRORCODE(EPROTO),
> + ERRORCODE(EMULTIHOP),
> + ERRORCODE(EDOTDOT),
> + ERRORCODE(EBADMSG),
> + ERRORCODE(EOVERFLOW),
> + ERRORCODE(ENOTUNIQ),
> + ERRORCODE(EBADFD),
> + ERRORCODE(EREMCHG),
> + ERRORCODE(ELIBACC),
> + ERRORCODE(ELIBBAD),
> + ERRORCODE(ELIBSCN),
> + ERRORCODE(ELIBMAX),
> + ERRORCODE(ELIBEXEC),
> + ERRORCODE(EILSEQ),
> + ERRORCODE(ERESTART),
> + ERRORCODE(ESTRPIPE),
> + ERRORCODE(EUSERS),
> + ERRORCODE(ENOTSOCK),
> + ERRORCODE(EDESTADDRREQ),
> + ERRORCODE(EMSGSIZE),
> + ERRORCODE(EPROTOTYPE),
> + ERRORCODE(ENOPROTOOPT),
> + ERRORCODE(EPROTONOSUPPORT),
> + ERRORCODE(ESOCKTNOSUPPORT),
> + ERRORCODE(EOPNOTSUPP),
> + ERRORCODE(EPFNOSUPPORT),
> + ERRORCODE(EAFNOSUPPORT),
> + ERRORCODE(EADDRINUSE),
> + ERRORCODE(EADDRNOTAVAIL),
> + ERRORCODE(ENETDOWN),
> + ERRORCODE(ENETUNREACH),
> + ERRORCODE(ENETRESET),
> + ERRORCODE(ECONNABORTED),
> + ERRORCODE(ECONNRESET),
> + ERRORCODE(ENOBUFS),
> + ERRORCODE(EISCONN),
> + ERRORCODE(ENOTCONN),
> + ERRORCODE(ESHUTDOWN),
> + ERRORCODE(ETOOMANYREFS),
> + ERRORCODE(ETIMEDOUT),
> + ERRORCODE(ECONNREFUSED),
> + ERRORCODE(EHOSTDOWN),
> + ERRORCODE(EHOSTUNREACH),
> + ERRORCODE(EALREADY),
> + ERRORCODE(EINPROGRESS),
> + ERRORCODE(ESTALE),
> + ERRORCODE(EUCLEAN),
> + ERRORCODE(ENOTNAM),
> + ERRORCODE(ENAVAIL),
> + ERRORCODE(EISNAM),
> + ERRORCODE(EREMOTEIO),
> + ERRORCODE(EDQUOT),
> + ERRORCODE(ENOMEDIUM),
> + ERRORCODE(EMEDIUMTYPE),
> + ERRORCODE(ECANCELED),
> + ERRORCODE(ENOKEY),
> + ERRORCODE(EKEYEXPIRED),
> + ERRORCODE(EKEYREVOKED),
> + ERRORCODE(EKEYREJECTED),
> + ERRORCODE(EOWNERDEAD),
> + ERRORCODE(ENOTRECOVERABLE),
> + ERRORCODE(ERFKILL),
> + ERRORCODE(EHWPOISON),
> + ERRORCODE(ERESTARTSYS),
> + ERRORCODE(ERESTARTNOINTR),
> + ERRORCODE(ERESTARTNOHAND),
> + ERRORCODE(ENOIOCTLCMD),
> + ERRORCODE(ERESTART_RESTARTBLOCK),
> + ERRORCODE(EPROBE_DEFER),
> + ERRORCODE(EOPENSTALE),
> + ERRORCODE(ENOPARAM),
> + ERRORCODE(EBADHANDLE),
> + ERRORCODE(ENOTSYNC),
> + ERRORCODE(EBADCOOKIE),
> + ERRORCODE(ENOTSUPP),
> + ERRORCODE(ETOOSMALL),
> + ERRORCODE(ESERVERFAULT),
> + ERRORCODE(EBADTYPE),
> + ERRORCODE(EJUKEBOX),
> + ERRORCODE(EIOCBQUEUED),
> + ERRORCODE(ERECALLCONFLICT),
> +};
> +
> +static noinline_for_stack
> +char *errstr(char *buf, char *end, unsigned long long num,
> + struct printf_spec spec)
> +{
> + char *errname = NULL;
> + size_t errnamelen, copy;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) {
> + if (num == errorcodes[i].err || num == -errorcodes[i].err) {
> + errname = errorcodes[i].str;
> + break;
> + }
> + }
> +
> + if (!errname) {
> + /* fall back to ordinary number */
> + return number(buf, end, num, spec);
> + }
> +
> + copy = errnamelen = strlen(errname);
> + if (copy > end - buf)
> + copy = end - buf;
> + buf = memcpy(buf, errname, copy);
> +
> + return buf + errnamelen;
> +}
> +
> static noinline_for_stack
> char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
> {
> @@ -2566,7 +2752,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
> num = va_arg(args, unsigned int);
> }
>
> - str = number(str, end, num, spec);
> + if (spec.type == FORMAT_TYPE_INT && *fmt == 'E') {
> + fmt++;
> + str = errstr(str, end, num, spec);
> + } else {
> + str = number(str, end, num, spec);
> + }
> }
> }
>
> --
> 2.20.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] vsprintf: introduce %dE for error constants
From: Andy Shevchenko @ 2019-08-29 13:39 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sergey Senozhatsky, Jani Nikula, Petr Mladek, Steven Rostedt,
Andrew Morton, Enrico, Weigelt, metux IT consult, Jonathan Corbet,
Linux Documentation List, Linux Kernel Mailing List
In-Reply-To: <20190827211244.7210-1-uwe@kleine-koenig.org>
On Wed, Aug 28, 2019 at 12:14 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> The new format specifier %dE introduced with this patch pretty-prints
> the typical negative error values. So
>
> pr_info("probing failed (%dE)\n", ret);
>
> yields
>
> probing failed (EIO)
>
> if ret holds -EIO. This is easier to understand than the for now common
>
> probing failed (-5)
>
> (which used %d instead of %dE) for kernel developers who don't know the
> numbers by heart, The error names are also known and understood by
> userspace coders as they match the values the errno variable can have.
> Also to debug the sitation that resulted in the above message very
> probably involves EIO, so the message already gives the string to look
> out for.
>
> glibc (and other libc variants)'s printf have a similar feature: %m
> expands to strerror(errno). I preferred using %dE however because %m in
> userspace doesn't consume an argument (which is however necessary in the
> kernel as there is no global errno variable). Also this is handled
> correctly by the compiler's format string checker as there is a matching
> int variable for the %d the compiler notices.
>
> There are quite some code locations that could be adapted to benefit
> from this:
>
> $ git grep -E '^\s*(printk|(kv?as|sn?|vs(c?n)?)printf|(kvm|dev|pr)_(emerg|alert|crit|err|warn(ing)?|notice|info|cont|devel|debug|dbg))\(.*(\(%d\)|: %d)\\n' v5.3-rc5 | wc -l
> 9141
>
> I expect there are some false negatives where the match is distributed
> over two or more lines and so isn't found.
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello,
>
> in v1 I handled both positive and negative errors which was challenged.
> I decided to drop that and only handle the (common) negative values.
> Readding handling of positive values later is easier than dropping it.
>
> Sergey Senozhatsky and Enrico Weigelt suggested to use strerror as name
> for the function that I called errno2errstr. (In v1 it was not a
> separate function). I didn't pick this up because the semantic of
> strerror in userspace is different. It returns something like
> "Interrupted system call" while errno2errstr only yields "EINTR".
>
> I dropped EWOULDBLOCK from the list of codes as it is on all Linux archs
> identical to EAGAIN and the way the lookup works now breaks otherwise.
> (And having it wasn't useful already in v1.)
>
> Rasmus Villemoes pointed out that the way I wrote the resulting string
> to the buffer was broken. This is fixed according to his suggestion by
> using string_nocheck(). I also added a test to lib/test_printf.c as he
> requested.
>
> Petr Mladek had some concerns:
> > The array is long, created by cpu&paste, the index of each code
> > is not obvious.
>
> Yeah right, the array is long. This cannot really be changed because we
> have that many error codes. I don't understand your concern about the
> index not being obvious. The array was just a list of (number, string)
> pairs where the position in the array didn't have any semantic.
>
> I agree it would be nice to have only one place that has a collection of
> error codes. I thought about reorganizing how the error constants are
> defined, i.e. doing something like:
>
> DEFINE_ERROR(EIO, 5, "I/O error")
>
> but I don't see a possibility to let this expand to
>
> #define EIO 5
>
> without resorting to another macro language. If that were possible the
> list of DEFINE_ERRORs could be reused to help generating the code for
> errno2errstr. So if you have a good idea, please speak up.
>
> > There are ideas to make the code even more tricky to reduce
> > the size, keep it fast.
>
> I think Enrico Weigelt's suggestion to use a case is the best
> performance-wise so that's what I picked up. Also I hope that
> performance isn't that important because the need to print an error
> should not be so common that it really hurts in production.
>
> > Both, %dE modifier and the output format (ECODE) is non-standard.
>
> Yeah, obviously right. The problem is that the new modifier does
> something that wasn't implemented before, so it cannot match any
> standard. %pI is only known on Linux either, so I think being
> non-standard is a weak argument. For user consumption it would be nice
> if we'd get
>
> probing failed (I/O Error)
>
> from pr_info("probing failed (%dE)\n", -EIO); but that would be still
> more expensive because the collection of string constants would become
> bigger that it is already now and "EIO" is the right one to search for
> when debugging the underlaying problem. So I think "EIO" is even more
> useful than "I/O Error".
>
> > Upper letters gain a lot of attention. But the error code is
> > only helper information. Also many error codes are misleading because
> > they are used either wrongly or there was no better available.
>
> This isn't really an argument against the patch I think. Sure, if a
> function returned (say) EIO while ETIMEOUT would be better, my patch
> doesn't improve that detail. Still
>
> mydev: Failed to initialize blablub: EIO
>
> is more expressive than
>
> mydev: Failed to initialize blablub: -5
>
> . With "EIO" in the message it is not worse than with "-5" independant of the
> question if EIO is the right error code used.
>
> > There is no proof that this approach would be widely acceptable for
> > subsystem maintainers. Some might not like mass and "blind" code
> > changes. Some might not like the output at all.
>
> I don't intend to mass convert existing code. I would restrict myself to
> updating the documentation and then maybe send a patch per subsystem as an
> example to let maintainers know and judge for themselves if they like it or
> not. And if it doesn't get picked up, we can just remove the feature again next
> year (or so).
>
> I dropped the example conversion, I think the idea should be clear now
> even without an explicit example.
>
Hold on, can you in less than 20 words put WHY this is needed?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] [RFC] tty/serial: imx: make use of format specifier %dE
From: Andy Shevchenko @ 2019-08-29 13:43 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Jiri Slaby, Sascha Hauer, Shawn Guo,
Fabio Estevam, NXP Linux Team, open list:SERIAL DRIVERS,
linux-arm Mailing List, Petr Mladek, Sergey Senozhatsky,
Steven Rostedt, Andrew Morton, Jani Nikula, Enrico Weigelt,
Jonathan Corbet, Linux Documentation List,
Linux Kernel Mailing List
In-Reply-To: <20190829043716.5223-1-uwe@kleine-koenig.org>
On Thu, Aug 29, 2019 at 7:40 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> I created a patch that teaches printk et al to emit a symbolic error
> name for an error valued integer[1]. With that applied
>
> dev_err(&pdev->dev, "failed to get ipg clk: %dE\n", ret);
>
> emits
>
> ... failed to get ipg clk: EPROBE_DEFER
>
> if ret is -EPROBE_DEFER. Petr Mladek (i.e. one of the printk
> maintainers) had concerns if this would be well received and worth the
> effort. He asked to present it to a few subsystems. So for now, this
> patch converting the imx UART driver shouldn't be applied yet but it
> would be great to get some feedback about if you think that being able
> to easily printk (for example) "EIO" instead of "-5" is a good idea.
> Would it help you? Do you think it helps your users?
No, it makes sense only for debug where the user is supposed to be
developer and thus needs anyway to know code base better than average.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC v1 2/2] rcu/tree: Remove dynticks_nmi_nesting counter
From: Joel Fernandes @ 2019-08-29 13:59 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, Frederic Weisbecker, Jonathan Corbet, Josh Triplett,
kernel-team, Lai Jiangshan, linux-doc, Mathieu Desnoyers,
Mauro Carvalho Chehab, rcu, Steven Rostedt
In-Reply-To: <20190829034336.GD4125@linux.ibm.com>
On Wed, Aug 28, 2019 at 08:43:36PM -0700, Paul E. McKenney wrote:
[snip]
> On the tracing patch... That patch might be a good idea regardless,
> but I bet that the reason that you felt the sudden need for it was due
> to the loss of information in your eventual ->dynticks_nesting field.
> After all, the value 0x1 might be an interrupt from idle, or it might
> just as easily be a task running in the kernel at process level.
True, however what really triggered me to do it was the existing code which
does not distinguish between entry/exit from USER and IDLE.
> The reason the patch might nevertheless be a good idea is that redundant
> information can be helpful when debugging. Especially when debugging
> new architecture-specific code, which is when RCU's dyntick-idle warnings
> tend to find bugs.
Sure, and also that it is more readable to ordinary human beings than "++="
and "--=" :-D.
thanks,
- Joel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox