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

[German Rivera <German.Rivera@freescale.com>]

On 09/25/2014 11:16 AM, Scott Wood wrote:
> On Thu, 2014-09-25 at 10:44 -0500, German Rivera wrote:
>>
>> On 09/24/2014 10:40 PM, Kim Phillips wrote:
>>> On Wed, 24 Sep 2014 21:23:59 -0500
>>> German Rivera <German.Rivera@freescale.com> wrote:
>>>
>>>> On 09/23/2014 07:49 PM, Kim Phillips wrote:
>>>>> On Fri, 19 Sep 2014 17:49:39 -0500
>>>>> "J. German Rivera" <German.Rivera@freescale.com> wrote:
>>>>>> +	mc_io->portal_virt_addr = NULL;
>>>>>> +	devm_kfree(mc_io->dev, mc_io);
>>>>>
>>>>> like I said before, there's really no point in clearing something
>>>>> out right before it's freed.
>>>>>
>>>> I disagree. This can help detect cases of double-freeing.
>>>
>>> ?  freeing NULL does nothing - it just returns - which doesn't help
>>> detect anything.  What's more, the kernel has a memory debugging
>>> infrastructure that detects double freeing of the same object.
>> I know that, but silently doing nothing when freeing NULL is a bad
>> practice in general, because it hides a bug.
>
> It doesn't hide a bug "in general".  I'm not sure what the relevance is
> here, though -- this seems to be about reusing portal_virt_addr as a
> debug flag rather than anything to do with actually calling free() on
> portal_virt_addr.
>
>> Is the memory debugging infrastructure enabled by default? If so, then
>> I would agree with you. If not, we would need to be able to reproduce
>> the bug while having memory debugging enabled. This assumes that the
>> bug is easy to reproduce. What if it is not easy to reproduce? Having
>> "first-failure data capture" checks is useful for helping diagnose bugs
>> that are not easy to reproduce.
>>
>> In this case, if due to some bug, fsl_destroy_mc_io() is
>> called twice for the same mc_io object, the combination of doing
>> "mc_io->portal_virt_addr = NULL" and the original "if (WARN_ON",
>> that you want removed, would have helped catch the bug on
>> "first failure".
>> Even removing the "if (WARN_ON)" but keeping the
>> "mc_io->portal_virt_addr = NULL" would still help catch the bug
>> on "first failure", assuming that the system crashes when calling
>> "devm_iounmap(mc_io->dev, NULL);" or at least prints a stack trace.
>
> iounmap of NULL will not crash or print a stack trace.  It is a no-op.
>
Ok, so if iounmap silently does nothing for NULL, and free silently does
nothing for NULL, that means we may not be be able to easily catch 
"destroy double-call" bugs on first-failure, in this case.
If that is not an issue, then I'll remove the "mc_io->portal_virt_addr = 
NULL" as well.

>>>>>> +int mc_send_command(struct fsl_mc_io *mc_io, struct mc_command *cmd)
>>>>>> +{
>>>>>> +	enum mc_cmd_status status;
>>>>>> +	int error;
>>>>>> +	unsigned long irqsave_flags = 0;
>>>>>> +	unsigned long jiffies_until_timeout =
>>>>>> +	    jiffies + MC_CMD_COMPLETION_TIMEOUT_JIFFIES;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * Acquire lock depending on mc_io flags:
>>>>>> +	 */
>>>>>> +	if (mc_io->flags & FSL_MC_PORTAL_SHARED_BY_INT_HANDLERS) {
>>>>>> +		if (mc_io->flags & FSL_MC_PORTAL_SHARED_BY_THREADS)
>>>>>> +			spin_lock_irqsave(&mc_io->spinlock, irqsave_flags);
>>>>>> +		else
>>>>>> +			spin_lock(&mc_io->spinlock);
>>>>>> +	} else if (mc_io->flags & FSL_MC_PORTAL_SHARED_BY_THREADS) {
>>>>>> +		mutex_lock(&mc_io->mutex);
>>>>>> +	}
>>>>>
>>>>> again, I think we need to drop the coming from h/w IRQ context here
>>>>> (SHARED_BY_INT_HANDLERS); there's no IRQ handlers in this
>>>>> patchseries, and calling this function from an IRQ handler would be
>>>>> prohibitively wasteful, guessing by the udelay and timeout values
>>>>> below.
>>>>>
>>>>> Can we just mutex_lock for now, and unconditionally (no
>>>>> SHARED_BY_THREADS check), to protect from nesting?
>>>>>
>>>> I would still prefer to keep the SHARED_BY_THREADS flag, to give option
>>>> of not doing any locking, in cases where the portal used in
>>>> mc_send_command() is not shared among concurrent callers
>>>
>>> how can you guarantee there won't be concurrent callers?  The linux
>>> kernel is multithreaded.
>>>
>> The owner of the portal should know if his/her code can be invoked using
>> the same portal, from multiple threads or not.
>
> Or more generally, whether the caller is responsible for
> synchronization.
>
> Would it make sense to simplify by saying the caller is always
> responsible for synchronization?
>
Certainly this will simplify mc_send_command() but it will put the 
additional burden on all the callers of MC commands. If there is no 
objection, we can remove locking entirely from mc_send_command() and
the the callers of MC commands deal with that.

> 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)?

>>>>>> +	/*
>>>>>> +	 * Wait for response from the MC hardware:
>>>>>> +	 */
>>>>>> +	for (;;) {
>>>>>> +		status = mc_read_response(mc_io->portal_virt_addr, cmd);
>>>>>> +		if (status != MC_CMD_STATUS_READY)
>>>>>> +			break;
>>>>>> +
>>>>>> +		/*
>>>>>> +		 * TODO: When MC command completion interrupts are supported
>>>>>> +		 * call wait function here instead of udelay()
>>>>>> +		 */
>>>>>> +		udelay(MC_CMD_COMPLETION_POLLING_INTERVAL_USECS);
>>>>>
>>>>> this pauses any caller for 0.5ms on every successful command
>>>>> write.  Can the next submission of the patchseries wait until
>>>>> completion IRQs are indeed supported, since both that and the above
>>>>> locking needs to be resolved?
>>>>>
>>>> No. Interrupt handlers will come in a later patch series as they are
>>>> not needed for using the basic MC functionality.
>>>
>>> meanwhile unnecessarily udelaying kernel threads for .5ms upsets
>>> basic kernel functionality :)  Would using the kernel's
>>> wait_for_completion API be a good compromise here?  See
>>> include/linux/completion.h.
>>>
>> 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?

>> 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?
(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.

-German