Re: [PATCH] powerpc/pseries: Avoid context switch in EEH reset if required

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Wed, Jan 21, 2015 at 09:56:07AM +1100, Gavin Shan wrote:
>On Tue, Jan 20, 2015 at 10:28:16AM +0100, Benjamin Herrenschmidt wrote:
>>On Mon, 2015-01-19 at 09:47 +1100, Gavin Shan wrote:
>>> On pseries platform, the EEH reset backend pseries_eeh_reset() can
>>> be called in atomic context as follows. For this case, we should
>>> call udelay() instead of msleep() to avoid context switching.
>>> 
>>>      drivers/scsi/ipr.c::ipr_reset_slot_reset_done()
>>>      drivers/pci/pci.c::pci_set_pcie_reset_state()
>>>      arch/powerpc/kernel/eeh.c::pcibios_set_pcie_reset_state()
>>>      arch/powerpc/platforms/pseries/eeh_pseries.c::pseries_eeh_reset()
>>
>>It's not acceptable to introduce multi-millisecond delays at interrupt
>>time. In fact, we should generally not use udelay in such context.
>>
>>I understand that this is an exceptional error handling case but it's
>>still not right.
>>
>
>Yes, I agree it's unsafe to udelay for multi-milliseconds as the queued
>works in atomic context is expected to be completed as soon as possible.
>
>>Are there many other users of pci_set_pcie_reset_state() at interrupt
>>time ? Can we have a discussion with the PCI folks as to whether that
>>should be legal or not ?
>>
>>I'm tempted to require that it's made illegal.
>
>Currently, there are 2 drivers calling this function: IPR and misc/genwqe.
>Also, VFIO would call this function for IBM and Mellanox adapters in PowerKVM
>repository. For now, IPR driver is the only one call this function in atomic
>context. 
>
>Sure, I'll send one email to confirm with PCI folks. I guess it's illegal
>to call pci_set_pcie_reset_state() in atomic context. If it's the case,
>I'm afraid Wendy has to change IPR driver to replace the reset timer with
>something else (e.g. workqueue).
>

Another way is to drop the hold/settle delays for pcibios_set_pcie_reset_state()
and IPR relies on the timer interval to cover them. Wendy, could you please
let me know if it would work for you or not?

    Start reset timer;
    Timer expires, assert the reset. Restart the timer with assert delay;
    Timer expires, deassert the reset. Restart the timer with settle delay;
    Timer expires, ready for subsequent works;

Thanks,
Gavin

>>
>>Ben.
>>
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> Tested-by: Wen Xiong<wenxiong@linux.vnet.ibm.com>
>>> ---
>>>  arch/powerpc/platforms/pseries/eeh_pseries.c | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
>>> index a6c7e19..67623a3 100644
>>> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
>>> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
>>> @@ -503,8 +503,7 @@ static int pseries_eeh_get_state(struct eeh_pe *pe, int *state)
>>>   */
>>>  static int pseries_eeh_reset(struct eeh_pe *pe, int option)
>>>  {
>>> -	int config_addr;
>>> -	int ret;
>>> +	int config_addr, delay, ret;
>>>  
>>>  	/* Figure out PE address */
>>>  	config_addr = pe->config_addr;
>>> @@ -528,9 +527,14 @@ static int pseries_eeh_reset(struct eeh_pe *pe, int option)
>>>  	/* We need reset hold or settlement delay */
>>>  	if (option == EEH_RESET_FUNDAMENTAL ||
>>>  	    option == EEH_RESET_HOT)
>>> -		msleep(EEH_PE_RST_HOLD_TIME);
>>> +		delay = EEH_PE_RST_HOLD_TIME;
>>> +	else
>>> +		delay = EEH_PE_RST_SETTLE_TIME;
>>> +
>>> +	if (in_atomic())
>>> +		udelay(delay * 1000);
>>>  	else
>>> -		msleep(EEH_PE_RST_SETTLE_TIME);
>>> +		msleep(delay);
>>>  
>>>  	return ret;
>>>  }
>>
>>