Boot hang regression 3.10.0-rc4 -> 3.10.0

[Grygorii Strashko]

Hi,

On 07/09/2013 09:42 AM, Felipe Balbi wrote:
> Hi,
>
> On Tue, Jul 09, 2013 at 11:03:54AM +0530, Rajendra Nayak wrote:
>> On Monday 08 July 2013 07:05 PM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Mon, Jul 08, 2013 at 06:50:01PM +0530, Rajendra Nayak wrote:
>>>>>>>>>> I wonder if this is because the timeouts get now initialized to 0 instead
>>>>>>>>>> of -1 for the serial driver?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> You meant initialized to -1, right? There's an additional check for timeout being 0. Unless i
>>>>>>>>> am missing something DT-boot will start off with timeout set to 0 and then get forced to -1.
>>>>>>>
>>>>>>> OK
>>>>>>
>>>>>> Issue 2: Causing boot to stop when serial driver is initialized.
>>>>>> (After Issue 1 is fixed)
>>>>>>
>>>>>> I could narrow this down to the change done to return -EINVAL
>>>>>> instead of 0 in serial_omap_get_context_loss_count() as part of
>>>>>> commit 'a630fbfbb1beeffc5bbe542a7986bf2068874633' "serial: omap:
>>>>>> Fix device tree based PM runtime"
>>>>>>
>>>>>> What this change in turn seems to do is cause a
>>>>>> serial_omap_restore_context() to get called as part of
>>>>>> serial_omap_runtime_resume() which was not the case when
>>>>>> serial_omap_get_context_loss_count() returned 0
>>>>>>
>>>>>> from serial_omap_runtime_resume():
>>>>>> -----
>>>>>>          int loss_cnt = serial_omap_get_context_loss_count(up);
>>>>>>
>>>>>>          if (loss_cnt < 0) {
>>>>>>                  dev_dbg(dev, "serial_omap_get_context_loss_count failed : %d\n",
>>>>>>                          loss_cnt);
>>>>>>                  serial_omap_restore_context(up);
>>>>>>          } else if (up->context_loss_cnt != loss_cnt) {
>>>>>>                  serial_omap_restore_context(up);
>>>>>>          }
>>>>>> -----
>>>>>>
>>>>>> I am still working on why a serial_omap_restore_context() could
>>>>>> have caused console to die. I will work with Sourav on this and
>>>>>> post the fixes for both issue 1 and issue2 once its clear on whats
>>>>>> really causing issue 2.
>>>>>
>>>>> That's because we don't have the omap specific pdata callbacks for
>>>>> context loss any longer. We may be able to detect when the context
>>>>> was really lost in the serial driver, and only then call the
>>>>> serial_omap_restore_context().
>>>>
>>>> Right, but calling serial_omap_restore_context() even when the context
>>>> is not lost, should not ideally cause an issue.
>>>
>>> it does in one condition. If context hasn't been saved before. And that
>>> can happen in the case of wrong pm runtime status for that device.
>>>
>>> Imagine the device is marked as suspended even though it's fully enabled
>>> (it hasn't been suspended by hwmod due to NO_IDLE flag). In that case
>>> your context structure is all zeroes (context has never been saved
>>> before) then when you call pm_runtime_get_sync() on probe() your
>>> ->runtime_resume() will get called, which will restore context,
>>> essentially undoing anything which was configured by u-boot.
>>
>> This could be a problem for drivers which do a save context in ->runtime_suspend()
>> but from what I see with omap serial, there is no save context done as part of
>> ->runtime_suspend.
>
> right, because context is "saved" in set_termios. probe() will get
> called much before set_termios() has a chance to run, right ?
>
> Same problem will trigger in that case.
>
> I still think patch below is necessary
>
>>> (completely untested, didn't even try to compile, just to illustrate)
>>>
>>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
>>> index 7341eff..d8dca68 100644
>>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>>> @@ -2559,6 +2559,12 @@ static void __init _setup_postsetup(struct omap_hwmod *oh)
>>>   	    (postsetup_state == _HWMOD_STATE_IDLE)) {
>>>   		oh->_int_flags |= _HWMOD_SKIP_ENABLE;
>>>   		postsetup_state = _HWMOD_STATE_ENABLED;
>>> +
>>> +		/* tell pm_runtime this device is already active */
>>> +		pm_runtime_set_active(&oh->od->pdev->dev);
>>> +	} else {
>>> +		/* tell pm_runtime this device is trully suspended */
>>> +		pm_runtime_set_suspended(&oh->od->pdev->dev);
>>>   	}
>>>
>>>   	if (postsetup_state == _HWMOD_STATE_IDLE)
>

This will not work - _setup_postsetup() is called from core_initcall 
level and OMAP devices have not been created at this moment yet 
(of_platform_populate() is called from
customize_machine->init_machine->omap_generic_init() at arch_initcall time)

More over, I don't recommend to depend on hwmod->od field - it's been 
created to support OPPs and it's obsolete now in case of DT use.

Seems, This issue need to be handled in driver for DT boot use case, 
possibly cmdline need to be parsed in the same way as it's done in
omap_serial_early_init().

Regards,
-grygorii