[v3,3/3] powerpc/pseries: Report in kernel device tree update to drmgr

Traditionally it has been drmgr's responsibilty to update the device tree
through the /proc/ppc64/ofdt interface after a suspend/resume operation.
This patchset however has modified suspend/resume ops to preform that update
entirely in the kernel during the resume. Therefore, a mechanism is required
for drmgr to determine who is responsible for the update. This patch adds a
show function to the "hibernate" attribute that returns 1 if the kernel
updates the device tree after the resume and 0 if drmgr is responsible.

Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/suspend.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

On Fri, 2014-01-31 at 15:58 -0800, Tyrel Datwyler wrote:
> Traditionally it has been drmgr's responsibilty to update the device tree
> through the /proc/ppc64/ofdt interface after a suspend/resume operation.
> This patchset however has modified suspend/resume ops to preform that update
> entirely in the kernel during the resume. Therefore, a mechanism is required
> for drmgr to determine who is responsible for the update. This patch adds a
> show function to the "hibernate" attribute that returns 1 if the kernel
> updates the device tree after the resume and 0 if drmgr is responsible.

This is not right.

We cannot change the kernel behaviour in a way that is incompatible with
existing userspace, and unless you can explain me why that is not the
case, it feels like this patch set does just that ....

What happens if you have an older drmgr which still does the update
itself ?

You can't just change user visible behaviours and interface without at
least explaining why it's ok to do so at the very least (and ensuring
that of course) without breaking existing userspace.

Now it's possible that the double update caused by this series is
harmless, in which case please explain it in the changeset, I certainly
can't assume it and if it's not, you'll need some other way for drmgr to
signal the kernel to indicate it supports the new behaviour before you
enable it.

Ben.

> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/suspend.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/suspend.c b/arch/powerpc/platforms/pseries/suspend.c
> index 1d9c580..b87b978 100644
> --- a/arch/powerpc/platforms/pseries/suspend.c
> +++ b/arch/powerpc/platforms/pseries/suspend.c
> @@ -192,7 +192,30 @@ out:
>  	return rc;
>  }
>  
> -static DEVICE_ATTR(hibernate, S_IWUSR, NULL, store_hibernate);
> +#define USER_DT_UPDATE	0
> +#define KERN_DT_UPDATE	1
> +
> +/**
> + * show_hibernate - Report device tree update responsibilty
> + * @dev:		subsys root device
> + * @attr:		device attribute struct
> + * @buf:		buffer
> + *
> + * Report whether a device tree update is performed by the kernel after a
> + * resume, or if drmgr must coordinate the update from user space.
> + *
> + * Return value:
> + *	0 if drmgr is to initiate update, and 1 otherwise
> + **/
> +static ssize_t show_hibernate(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	return sprintf(buf, "%d\n", KERN_DT_UPDATE);
> +}
> +
> +static DEVICE_ATTR(hibernate, S_IWUSR | S_IRUGO,
> +		   show_hibernate, store_hibernate);
>  
>  static struct bus_type suspend_subsys = {
>  	.name = "power",

On 02/16/2014 04:22 PM, Benjamin Herrenschmidt wrote:
> On Fri, 2014-01-31 at 15:58 -0800, Tyrel Datwyler wrote:
>> Traditionally it has been drmgr's responsibilty to update the device tree
>> through the /proc/ppc64/ofdt interface after a suspend/resume operation.
>> This patchset however has modified suspend/resume ops to preform that update
>> entirely in the kernel during the resume. Therefore, a mechanism is required
>> for drmgr to determine who is responsible for the update. This patch adds a
>> show function to the "hibernate" attribute that returns 1 if the kernel
>> updates the device tree after the resume and 0 if drmgr is responsible.
> 
> This is not right.
> 
> We cannot change the kernel behaviour in a way that is incompatible with
> existing userspace, and unless you can explain me why that is not the
> case, it feels like this patch set does just that ....
> 
> What happens if you have an older drmgr which still does the update
> itself ?
> 

In this case we get a double update which I clearly neglected to mention
in the patch. The first patch in this series actually removes an
unnecessary double update from the existing kernel implementation. The
same information is returned by the update-nodes/properties call the
second time around and aside from being a waste of a few cycles is harmless.

Tyrel

> You can't just change user visible behaviours and interface without at
> least explaining why it's ok to do so at the very least (and ensuring
> that of course) without breaking existing userspace.
> 
> Now it's possible that the double update caused by this series is
> harmless, in which case please explain it in the changeset, I certainly
> can't assume it and if it's not, you'll need some other way for drmgr to
> signal the kernel to indicate it supports the new behaviour before you
> enable it.
> 
> Ben.
> 
>> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/suspend.c | 25 ++++++++++++++++++++++++-
>>  1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/suspend.c b/arch/powerpc/platforms/pseries/suspend.c
>> index 1d9c580..b87b978 100644
>> --- a/arch/powerpc/platforms/pseries/suspend.c
>> +++ b/arch/powerpc/platforms/pseries/suspend.c
>> @@ -192,7 +192,30 @@ out:
>>  	return rc;
>>  }
>>  
>> -static DEVICE_ATTR(hibernate, S_IWUSR, NULL, store_hibernate);
>> +#define USER_DT_UPDATE	0
>> +#define KERN_DT_UPDATE	1
>> +
>> +/**
>> + * show_hibernate - Report device tree update responsibilty
>> + * @dev:		subsys root device
>> + * @attr:		device attribute struct
>> + * @buf:		buffer
>> + *
>> + * Report whether a device tree update is performed by the kernel after a
>> + * resume, or if drmgr must coordinate the update from user space.
>> + *
>> + * Return value:
>> + *	0 if drmgr is to initiate update, and 1 otherwise
>> + **/
>> +static ssize_t show_hibernate(struct device *dev,
>> +			      struct device_attribute *attr,
>> +			      char *buf)
>> +{
>> +	return sprintf(buf, "%d\n", KERN_DT_UPDATE);
>> +}
>> +
>> +static DEVICE_ATTR(hibernate, S_IWUSR | S_IRUGO,
>> +		   show_hibernate, store_hibernate);
>>  
>>  static struct bus_type suspend_subsys = {
>>  	.name = "power",
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>

On Tue, 2014-02-18 at 15:28 -0800, Tyrel Datwyler wrote:
> In this case we get a double update which I clearly neglected to
> mention
> in the patch. The first patch in this series actually removes an
> unnecessary double update from the existing kernel implementation. The
> same information is returned by the update-nodes/properties call the
> second time around and aside from being a waste of a few cycles is
> harmless.

Thanks. Can you resend the patches with updated descriptions ? I will
try to send them to Linus by end of week.

Cheers,
Ben.