lkml.org 
[lkml]   [2014]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3 v2] drivers/bus: Added Freescale Management Complex APIs

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



\
 
 \ /
  Last update: 2014-09-25 19:21    [W:0.057 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site