Re: [PATCH 1/3 v2] drivers/bus: Added Freescale Management Complex APIs

[Scott Wood <scottwood@freescale.com>]

On Thu, 2014-09-25 at 11:44 -0500, German Rivera wrote:
> On 09/25/2014 11:16 AM, Scott Wood wrote:
> > Then again, the management complex is not supposed to be on the
> > performance critical path, so why not simplify by just always do the
> > locking here?
> >
> But about the few MC commands that need to run on interrupt context
> (such as to inspect or clear MC interrupts)?

I think that should be disallowed.  Unless you can guarantee that the MC
firmware will respond in a very short time, have those interrupts move
their work into a workqueue, with the MC interrupt masked until
completion.  We do similar things for interrupts from i2c devices, etc.

> >> The intent of doing the udelay() in the middle of the polling loop was
> >> to throttle down the frequency of I/Os done while polling for the
> >> completion of the command. Can you elaborate on why ".5ms udelay upsets
> >> basic kernel functionality"?
> >
> > It introduces latency, especially since it's possible for it to happen
> > with interrupts disabled.  And you're actually potentially blocking for
> > more than that, since 500us is just one iteration of the loop.
> >
> > The jiffies test for exiting the loop is 500ms, which is *way* too long
> > to spend in a critical section.
> >
> But that would be a worst case, since that is a timeout check.
> What timeout value do you think would be more appropriate in this case?

Worst case latency matters.  Some workloads would be bothered by an
occasional 500us.  Even normal interactive use would notice 500ms.

> >> Would it be better to just use "plain delay loop", instead of the udelay
> >> call, such as the following?
> >>
> >> for (i = 0; i < 1000; i++)
> >>      ;
> >
> > No, never do that.  You have no idea how long it will actually take.
> > GCC might even optimize it out entirely.
> >
> Ok, so udelay is not good here, a plain vanilla delay loop is not good 
> either. Are there other alternatives or we just don't worry about
> throttling down the frequency of I/Os done in the polling loop?

udelay is OK for this purpose, but not for so long, and find some way to
make the kernel preemptible between loop iterations.

> (Given the fact that MC commands are not expected to be executed that 
> frequently, to frequently cause a lot of I/O traffic with this polling loop)
> 
> >> I can see that in the cases where we use "completion interrupts", the
> >> ISR can signal the completion, and the polling loop can be replaced by
> >> waiting on the completion. However, I don't see how using a completion
> >> can help make a polling loop more efficient, if you don't have a
> >> "completion interrupt" to signal the completion.
> >
> > It's not about making it more efficient.  It's about not causing
> > problems for other things going on in the system.
> >
> But still I don't see how a completion can help here, unless
> you can signal the completion from an ISR.

Right, but you can still sleep on a timer instead of busy waiting if you
need such a long timeout.

-Scott