[PATCH] ACPI/Processor: Add CPU_STARTING_FROZEN check in the acpi_cpu_soft_notify()

[PATCH] ACPI/Processor: Add CPU_STARTING_FROZEN check in the acpi_cpu_soft_notify()

[Lan Tianyu]

The callback of CPU_STARTING event can't sleep and so acpi_cpu_soft_notify()
return directly when CPU_STARTING event is triggered. But cpu hotplug also
happens during S2RAM. The action will become CPU_STARTING_FROZEN. This
patch is to fix missing check the frozen event.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/acpi/processor_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 4fcbd67..66e2249 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -125,7 +125,7 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
 	 * CPU_STARTING and CPU_DYING must not sleep. Return here since
 	 * acpi_bus_get_device() may sleep.
 	 */
-	if (action == CPU_STARTING || action == CPU_DYING)
+	if ((action & ~CPU_TASKS_FROZEN) == CPU_STARTING || action == CPU_DYING)
 		return NOTIFY_DONE;
 
 	if (!pr || acpi_bus_get_device(pr->handle, &device))
-- 
1.8.4.rc0.1.g8f6a3e5.dirty

Re: [PATCH] ACPI/Processor: Add CPU_STARTING_FROZEN check in the acpi_cpu_soft_notify()

[Rafael J. Wysocki]

On Thursday, July 31, 2014 05:20:26 PM Lan Tianyu wrote:
> The callback of CPU_STARTING event can't sleep and so acpi_cpu_soft_notify()
> return directly when CPU_STARTING event is triggered. But cpu hotplug also
> happens during S2RAM. The action will become CPU_STARTING_FROZEN. This
> patch is to fix missing check the frozen event.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

There is work to restructure the handling of CPU_TASKS_FROZEN under way
and Chen Gong is driving it.  That's likely to conflict with the last
two patches from you.  Can you pleaes coordinate with Gong?

Rafael


> ---
>  drivers/acpi/processor_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 4fcbd67..66e2249 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -125,7 +125,7 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
>  	 * CPU_STARTING and CPU_DYING must not sleep. Return here since
>  	 * acpi_bus_get_device() may sleep.
>  	 */
> -	if (action == CPU_STARTING || action == CPU_DYING)
> +	if ((action & ~CPU_TASKS_FROZEN) == CPU_STARTING || action == CPU_DYING)
>  		return NOTIFY_DONE;
>  
>  	if (!pr || acpi_bus_get_device(pr->handle, &device))
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI/Processor: Add CPU_STARTING_FROZEN check in the acpi_cpu_soft_notify()

[Lan Tianyu]

On 2014年08月01日 05:20, Rafael J. Wysocki wrote:
> On Thursday, July 31, 2014 05:20:26 PM Lan Tianyu wrote:
>> The callback of CPU_STARTING event can't sleep and so acpi_cpu_soft_notify()
>> return directly when CPU_STARTING event is triggered. But cpu hotplug also
>> happens during S2RAM. The action will become CPU_STARTING_FROZEN. This
>> patch is to fix missing check the frozen event.
>>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> 
> There is work to restructure the handling of CPU_TASKS_FROZEN under way
> and Chen Gong is driving it.  That's likely to conflict with the last
> two patches from you.  Can you please coordinate with Gong?

Hi Rafael:

Thanks for reminder. I just checked Chen Gong's patchset "Gloabl CPU
Hot-plug flag _FROZEN Clean up". There is no conflict between our
patches. Gong's patch is to remove the following macro.

CPU_ONLINE_FROZEN
CPU_UP_PREPARE_FROZEN
CPU_UP_CANCELED_FROZEN
CPU_DOWN_PREPARE_FROZEN
CPU_DOWN_FAILED_FROZEN
CPU_DEAD_FROZEN
CPU_DYING_FROZEN
CPU_STARTING_FROZEN

CPU_TASKS_FROZEN is still available and the CPU events during S2RAM
are still (CPU_xxx | CPU_TASKS_FROZEN).

BTW, this is a bug fix from my opinion and it should be backported to
stable tree.

> 
> Rafael
> 
> 
>> ---
>>  drivers/acpi/processor_driver.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 4fcbd67..66e2249 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -125,7 +125,7 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
>>  	 * CPU_STARTING and CPU_DYING must not sleep. Return here since
>>  	 * acpi_bus_get_device() may sleep.
>>  	 */
>> -	if (action == CPU_STARTING || action == CPU_DYING)
>> +	if ((action & ~CPU_TASKS_FROZEN) == CPU_STARTING || action == CPU_DYING)
>>  		return NOTIFY_DONE;
>>  
>>  	if (!pr || acpi_bus_get_device(pr->handle, &device))
>>
> 


-- 
Best regards
Tianyu Lan

Re: [PATCH] ACPI/Processor: Add CPU_STARTING_FROZEN check in the acpi_cpu_soft_notify()

[Rafael J. Wysocki]

On Friday, August 01, 2014 10:51:46 AM Lan Tianyu wrote:
> On 2014年08月01日 05:20, Rafael J. Wysocki wrote:
> > On Thursday, July 31, 2014 05:20:26 PM Lan Tianyu wrote:
> >> The callback of CPU_STARTING event can't sleep and so acpi_cpu_soft_notify()
> >> return directly when CPU_STARTING event is triggered. But cpu hotplug also
> >> happens during S2RAM. The action will become CPU_STARTING_FROZEN. This
> >> patch is to fix missing check the frozen event.

OK

What's the impact of the bug you're fixing?  Do we sleep when we shouldn't?

In particular, are there any bugs reported that are related to it?

> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> > 
> > There is work to restructure the handling of CPU_TASKS_FROZEN under way
> > and Chen Gong is driving it.  That's likely to conflict with the last
> > two patches from you.  Can you please coordinate with Gong?
> 
> Hi Rafael:
> 
> Thanks for reminder. I just checked Chen Gong's patchset "Gloabl CPU
> Hot-plug flag _FROZEN Clean up". There is no conflict between our
> patches. Gong's patch is to remove the following macro.
> 
> CPU_ONLINE_FROZEN
> CPU_UP_PREPARE_FROZEN
> CPU_UP_CANCELED_FROZEN
> CPU_DOWN_PREPARE_FROZEN
> CPU_DOWN_FAILED_FROZEN
> CPU_DEAD_FROZEN
> CPU_DYING_FROZEN
> CPU_STARTING_FROZEN
> 
> CPU_TASKS_FROZEN is still available and the CPU events during S2RAM
> are still (CPU_xxx | CPU_TASKS_FROZEN).

OK

> BTW, this is a bug fix from my opinion and it should be backported to
> stable tree.

We'll see.


> >> ---
> >>  drivers/acpi/processor_driver.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> >> index 4fcbd67..66e2249 100644
> >> --- a/drivers/acpi/processor_driver.c
> >> +++ b/drivers/acpi/processor_driver.c
> >> @@ -125,7 +125,7 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
> >>  	 * CPU_STARTING and CPU_DYING must not sleep. Return here since
> >>  	 * acpi_bus_get_device() may sleep.
> >>  	 */
> >> -	if (action == CPU_STARTING || action == CPU_DYING)
> >> +	if ((action & ~CPU_TASKS_FROZEN) == CPU_STARTING || action == CPU_DYING)
> >>  		return NOTIFY_DONE;
> >>  
> >>  	if (!pr || acpi_bus_get_device(pr->handle, &device))
> >>
> > 
> 
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI/Processor: Add CPU_STARTING_FROZEN check in the acpi_cpu_soft_notify()

[Lan Tianyu]

On 2014年08月06日 09:30, Rafael J. Wysocki wrote:
> On Friday, August 01, 2014 10:51:46 AM Lan Tianyu wrote:
>> On 2014年08月01日 05:20, Rafael J. Wysocki wrote:
>>> On Thursday, July 31, 2014 05:20:26 PM Lan Tianyu wrote:
>>>> The callback of CPU_STARTING event can't sleep and so acpi_cpu_soft_notify()
>>>> return directly when CPU_STARTING event is triggered. But cpu hotplug also
>>>> happens during S2RAM. The action will become CPU_STARTING_FROZEN. This
>>>> patch is to fix missing check the frozen event.
> 
> OK
> 
> What's the impact of the bug you're fixing?  Do we sleep when we shouldn't?
> 
> In particular, are there any bugs reported that are related to it?

So far, there is such bug report. I found the issue when test my patch
of parallel non-boot cpu up with resuming device for S2RAM. Because
original code will wait all cpu up and then do other resume task, the
semaphore of ACPICA can always be get in the cpu notify callback and it
will not fall in the sleep. It's different situation for paralleling
non-boot cpu. The lock will be held by other threads and fall into
sleep. Further thinking, this may not cause bug in the old kernel and it
isn't needed by stable tree but the code should consider cpu frozen event.

> 
>>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>>>
>>> There is work to restructure the handling of CPU_TASKS_FROZEN under way
>>> and Chen Gong is driving it.  That's likely to conflict with the last
>>> two patches from you.  Can you please coordinate with Gong?
>>
>> Hi Rafael:
>>
>> Thanks for reminder. I just checked Chen Gong's patchset "Gloabl CPU
>> Hot-plug flag _FROZEN Clean up". There is no conflict between our
>> patches. Gong's patch is to remove the following macro.
>>
>> CPU_ONLINE_FROZEN
>> CPU_UP_PREPARE_FROZEN
>> CPU_UP_CANCELED_FROZEN
>> CPU_DOWN_PREPARE_FROZEN
>> CPU_DOWN_FAILED_FROZEN
>> CPU_DEAD_FROZEN
>> CPU_DYING_FROZEN
>> CPU_STARTING_FROZEN
>>
>> CPU_TASKS_FROZEN is still available and the CPU events during S2RAM
>> are still (CPU_xxx | CPU_TASKS_FROZEN).
> 
> OK
> 
>> BTW, this is a bug fix from my opinion and it should be backported to
>> stable tree.
> 
> We'll see.
> 
> 
>>>> ---
>>>>  drivers/acpi/processor_driver.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>>>> index 4fcbd67..66e2249 100644
>>>> --- a/drivers/acpi/processor_driver.c
>>>> +++ b/drivers/acpi/processor_driver.c
>>>> @@ -125,7 +125,7 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
>>>>  	 * CPU_STARTING and CPU_DYING must not sleep. Return here since
>>>>  	 * acpi_bus_get_device() may sleep.
>>>>  	 */
>>>> -	if (action == CPU_STARTING || action == CPU_DYING)
>>>> +	if ((action & ~CPU_TASKS_FROZEN) == CPU_STARTING || action == CPU_DYING)
>>>>  		return NOTIFY_DONE;
>>>>  
>>>>  	if (!pr || acpi_bus_get_device(pr->handle, &device))
>>>>
>>>
>>
>>
>>
> 


-- 
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI/Processor: Add CPU_STARTING_FROZEN check in the acpi_cpu_soft_notify()

[Rafael J. Wysocki]

On Wednesday, August 06, 2014 11:02:05 AM Lan Tianyu wrote:
> On 2014年08月06日 09:30, Rafael J. Wysocki wrote:
> > On Friday, August 01, 2014 10:51:46 AM Lan Tianyu wrote:
> >> On 2014年08月01日 05:20, Rafael J. Wysocki wrote:
> >>> On Thursday, July 31, 2014 05:20:26 PM Lan Tianyu wrote:
> >>>> The callback of CPU_STARTING event can't sleep and so acpi_cpu_soft_notify()
> >>>> return directly when CPU_STARTING event is triggered. But cpu hotplug also
> >>>> happens during S2RAM. The action will become CPU_STARTING_FROZEN. This
> >>>> patch is to fix missing check the frozen event.
> > 
> > OK
> > 
> > What's the impact of the bug you're fixing?  Do we sleep when we shouldn't?
> > 
> > In particular, are there any bugs reported that are related to it?
> 
> So far, there is such bug report. I found the issue when test my patch
> of parallel non-boot cpu up with resuming device for S2RAM. Because
> original code will wait all cpu up and then do other resume task, the
> semaphore of ACPICA can always be get in the cpu notify callback and it
> will not fall in the sleep. It's different situation for paralleling
> non-boot cpu. The lock will be held by other threads and fall into
> sleep. Further thinking, this may not cause bug in the old kernel and it
> isn't needed by stable tree but the code should consider cpu frozen event.

OK, so we don't need to propagate this to -stable I'd say.

I'll queue it up for the second 3.17 pull request.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html