Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler

[Javier Martinez Canillas]

Hello Krzysztof,

Thanks for your feedback.

On 10/22/2015 02:36 AM, Krzysztof Kozlowski wrote:
> On 22.10.2015 00:15, Javier Martinez Canillas wrote:
>> The pwrseq_emmc driver does a eMMC card reset before a system reboot to
>> allow broken or limited ROM boot-loaders (that don't have an eMMC reset
>> logic) to be able to read the second stage from the eMMC.
>>
>> But this has to be called before a system reboot handler and while most
>> of them use the priority 128, there are other restart handlers (such as
>> the syscon-reboot one) that use a higher priority. So, use the highest
>> priority to make sure that the eMMC hw is reset before a system reboot.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>> Tested-by: Markus Reichl <m.reichl@xxxxxxxxxxxxx>
>> Tested-by: Anand Moon <linux.amoon@xxxxxxxxx>
>> Reviewed-by: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>
>>
>> ---
>> Hello,
>>
>> This patch was needed since a recent series from Alim [0] added
>> syscon reboot and poweroff support to Exynos SoCs and removed
>> the reset handler in the Exynos Power Management Unit (PMU) code.
>>
>> But the PMU and syscon-reboot restart handler have a different
>> priority so [0] breaks restart when eMMC is used on these boards.
>>
>> [0]: http://www.spinics.net/lists/arm-kernel/msg454396.html
>>
>> So this patch must be merged before [0] to avoid regressions.
>>
>> Best regards,
>> Javier
>>
>>  drivers/mmc/core/pwrseq_emmc.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c
>> index 137c97fb7aa8..ad4f94ec7e8d 100644
>> --- a/drivers/mmc/core/pwrseq_emmc.c
>> +++ b/drivers/mmc/core/pwrseq_emmc.c
>> @@ -84,11 +84,11 @@ struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host,
>>  
>>  	/*
>>  	 * register reset handler to ensure emmc reset also from
>> -	 * emergency_reboot(), priority 129 schedules it just before
>> -	 * system reboot
>> +	 * emergency_reboot(), priority 255 is the highest priority
>> +	 * so it will be executed before any system reboot handler.
>>  	 */
>>  	pwrseq->reset_nb.notifier_call = mmc_pwrseq_emmc_reset_nb;
>> -	pwrseq->reset_nb.priority = 129;
>> +	pwrseq->reset_nb.priority = 255;
> 
> I see the problem which you are trying to solve but this may be tricker
> then just kicking the number. Some of restart handlers are registered
> with priority 192. I found few of such, like: at91_restart_nb,
> zynq_slcr_restart_nb, rmobile_reset_nb (maybe more, I did not grep too
> much).
> 

Yes, the syscon-reboot restart handler also uses a priority 192 and that
is why reboot with eMMC broke with Alim's patches since the PMU restart
handler priority is 128.

> I guess they chose the "192" priority on purpose.
>

I tried to understand what's the policy w.r.t priority numbering for
restart handlers but only found this in the register_restart_handler
kernel-doc [0]:

/**
 *	register_restart_handler - Register function to be called to reset
 *				   the system
 *	@nb: Info about handler function to be called
 *	@nb->priority:	Handler priority. Handlers should follow the
 *			following guidelines for setting priorities.
 *			0:	Restart handler of last resort,
 *				with limited restart capabilities
 *			128:	Default restart handler; use if no other
 *				restart handler is expected to be available,
 *				and/or if restart functionality is
 *				sufficient to restart the entire system
 *			255:	Highest priority restart handler, will
 *				preempt all other restart handlers

So, reading that is not clear to me if only the values 0, 128 and 255
should be used or any value from 0-255.

What's clear to me is that restart handlers to reset a specific hw block
should be called before the restart handler that resets the whole system.

The 192 seems to be used because there are other default restart handlers
that are using a prio of 128. See for example the commit that changed the
syscon-reboot prio from 128 to 192:

b81180b3fd48 power: reset: adjust priority of simple syscon reboot driver

So probably the 192 value was chosen because is in the middle of 128 and
255 but it seems to me a rather arbitrary value and I would prefer it to
be documented in some place.

> Effectively, now the emmc handler will be executed before their
> handlers... is it an issue? Maybe some testing on these platforms is
> necessary?
>

I don't think is an issue, the reason why I chose 255 is that it is
a documented value in the kernel-doc and since is the highest prio,
it makes sure the eMMC will be reset before any system restart handler.

Also, the pwrseq_emmc driver is only used in platforms whose SoC ROM
can either leave the eMMC in an unknown state so the kernel needs to
hw reset the eMMC or does not have a reset logic so it can only read
from an eMMC if is in a known state (i.e: after a reset from kernel).

Since the current mmc_pwrseq_emmc_reset_nb notifier priority is 129,
eMMC reset will not work if one of the platforms you mentioned needs
this since the system restart handler with prio 192 will be executed
before the eMMC one, leaving the eMMC in an unknown state on reboot.

And $SUBJECT should not cause any regressions for the platforms that
are currently using the pwrseq_emmc, since the restart handler was
already being called before the system restart handler so bumping
the priority should not cause any effect.

> Best regards,
> Krzysztof
> 

[0]: http://lxr.free-electrons.com/source/kernel/reboot.c#L113

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/