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

[Benjamin Herrenschmidt <benh@au1.ibm.com>]

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",