RE: [RFC] power/hibernate: Make passing hibernate offsets more friendly

[Mario.Limonciello]

> -Original Message-
> From: rjwyso...@gmail.com [mailto:rjwyso...@gmail.com] On Behalf Of Rafael J.
> Wysocki
> Sent: Friday, March 2, 2018 3:43 AM
> To: Limonciello, Mario <mario_limoncie...@dell.com>
> Cc: Andy Shevchenko <andy.shevche...@gmail.com>; Rafael J. Wysocki
> <r...@rjwysocki.net>; ACPI Devel Maling List <linux-a...@vger.kernel.org>; 
> Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>
> Subject: Re: [RFC] power/hibernate: Make passing hibernate offsets more 
> friendly
> 
> On Wed, Feb 28, 2018 at 9:05 PM,  <mario.limoncie...@dell.com> wrote:
> >
> >
> >> -Original Message-
> >> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> >> Sent: Wednesday, February 28, 2018 12:11 PM
> >> To: Limonciello, Mario <mario_limoncie...@dell.com>
> >> Cc: Rafael J . Wysocki <r...@rjwysocki.net>; ACPI Devel Maling List  >> a...@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>
> >> Subject: Re: [RFC] power/hibernate: Make passing hibernate offsets more 
> >> friendly
> >>
> >> On Wed, Feb 28, 2018 at 7:43 PM, Mario Limonciello
> >> <mario.limoncie...@dell.com> wrote:
> >> > Currently the only way to specify a hibernate offset for a swap
> >> > file is on the kernel command line.
> >> >
> >> > This makes some changes to improve:
> >> > 1) Add a new /sys/power/disk_offset that lets userspace specify
> >> > the offset and disk to use when initiating a hibernate cycle.
> >> >
> >> > 2) Adjust /sys/power/resume interpretation to also read in an
> >> > offset.
> >>
> >> Read is okay per se (not consistent though), showing is not.
> >> It might break an ABI.
> >
> > Right this is part of why I was proposing making a new attribute.
> >
> > The current RFC implementation I sent keeps the read output the
> > same for /sys/power/resume.
> 
> You also need to retain the write behavior of it.
> 
> A new attribute is fine if it helps, but the behavior of the existing
> one cannot change (both sides).

Thanks for your feedback, I'll adjust it as such.

RE: [RFC] power/hibernate: Make passing hibernate offsets more friendly

[Mario.Limonciello]

> -Original Message-
> From: rjwyso...@gmail.com [mailto:rjwyso...@gmail.com] On Behalf Of Rafael J.
> Wysocki
> Sent: Friday, March 2, 2018 3:43 AM
> To: Limonciello, Mario 
> Cc: Andy Shevchenko ; Rafael J. Wysocki
> ; ACPI Devel Maling List ; 
> Linux
> Kernel Mailing List 
> Subject: Re: [RFC] power/hibernate: Make passing hibernate offsets more 
> friendly
> 
> On Wed, Feb 28, 2018 at 9:05 PM,   wrote:
> >
> >
> >> -Original Message-
> >> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> >> Sent: Wednesday, February 28, 2018 12:11 PM
> >> To: Limonciello, Mario 
> >> Cc: Rafael J . Wysocki ; ACPI Devel Maling List  >> a...@vger.kernel.org>; LKML 
> >> Subject: Re: [RFC] power/hibernate: Make passing hibernate offsets more 
> >> friendly
> >>
> >> On Wed, Feb 28, 2018 at 7:43 PM, Mario Limonciello
> >>  wrote:
> >> > Currently the only way to specify a hibernate offset for a swap
> >> > file is on the kernel command line.
> >> >
> >> > This makes some changes to improve:
> >> > 1) Add a new /sys/power/disk_offset that lets userspace specify
> >> > the offset and disk to use when initiating a hibernate cycle.
> >> >
> >> > 2) Adjust /sys/power/resume interpretation to also read in an
> >> > offset.
> >>
> >> Read is okay per se (not consistent though), showing is not.
> >> It might break an ABI.
> >
> > Right this is part of why I was proposing making a new attribute.
> >
> > The current RFC implementation I sent keeps the read output the
> > same for /sys/power/resume.
> 
> You also need to retain the write behavior of it.
> 
> A new attribute is fine if it helps, but the behavior of the existing
> one cannot change (both sides).

Thanks for your feedback, I'll adjust it as such.

Re: [RFC] power/hibernate: Make passing hibernate offsets more friendly

[Rafael J. Wysocki]

On Wed, Feb 28, 2018 at 9:05 PM,  <mario.limoncie...@dell.com> wrote:
>
>
>> -Original Message-
>> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
>> Sent: Wednesday, February 28, 2018 12:11 PM
>> To: Limonciello, Mario <mario_limoncie...@dell.com>
>> Cc: Rafael J . Wysocki <r...@rjwysocki.net>; ACPI Devel Maling List > a...@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>
>> Subject: Re: [RFC] power/hibernate: Make passing hibernate offsets more 
>> friendly
>>
>> On Wed, Feb 28, 2018 at 7:43 PM, Mario Limonciello
>> <mario.limoncie...@dell.com> wrote:
>> > Currently the only way to specify a hibernate offset for a swap
>> > file is on the kernel command line.
>> >
>> > This makes some changes to improve:
>> > 1) Add a new /sys/power/disk_offset that lets userspace specify
>> > the offset and disk to use when initiating a hibernate cycle.
>> >
>> > 2) Adjust /sys/power/resume interpretation to also read in an
>> > offset.
>>
>> Read is okay per se (not consistent though), showing is not.
>> It might break an ABI.
>
> Right this is part of why I was proposing making a new attribute.
>
> The current RFC implementation I sent keeps the read output the
> same for /sys/power/resume.

You also need to retain the write behavior of it.

A new attribute is fine if it helps, but the behavior of the existing
one cannot change (both sides).

Re: [RFC] power/hibernate: Make passing hibernate offsets more friendly

[Rafael J. Wysocki]

On Wed, Feb 28, 2018 at 9:05 PM,   wrote:
>
>
>> -Original Message-
>> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
>> Sent: Wednesday, February 28, 2018 12:11 PM
>> To: Limonciello, Mario 
>> Cc: Rafael J . Wysocki ; ACPI Devel Maling List > a...@vger.kernel.org>; LKML 
>> Subject: Re: [RFC] power/hibernate: Make passing hibernate offsets more 
>> friendly
>>
>> On Wed, Feb 28, 2018 at 7:43 PM, Mario Limonciello
>>  wrote:
>> > Currently the only way to specify a hibernate offset for a swap
>> > file is on the kernel command line.
>> >
>> > This makes some changes to improve:
>> > 1) Add a new /sys/power/disk_offset that lets userspace specify
>> > the offset and disk to use when initiating a hibernate cycle.
>> >
>> > 2) Adjust /sys/power/resume interpretation to also read in an
>> > offset.
>>
>> Read is okay per se (not consistent though), showing is not.
>> It might break an ABI.
>
> Right this is part of why I was proposing making a new attribute.
>
> The current RFC implementation I sent keeps the read output the
> same for /sys/power/resume.

You also need to retain the write behavior of it.

A new attribute is fine if it helps, but the behavior of the existing
one cannot change (both sides).

RE: [RFC] power/hibernate: Make passing hibernate offsets more friendly

[Mario.Limonciello]

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Wednesday, February 28, 2018 12:11 PM
> To: Limonciello, Mario <mario_limoncie...@dell.com>
> Cc: Rafael J . Wysocki <r...@rjwysocki.net>; ACPI Devel Maling List  a...@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>
> Subject: Re: [RFC] power/hibernate: Make passing hibernate offsets more 
> friendly
> 
> On Wed, Feb 28, 2018 at 7:43 PM, Mario Limonciello
> <mario.limoncie...@dell.com> wrote:
> > Currently the only way to specify a hibernate offset for a swap
> > file is on the kernel command line.
> >
> > This makes some changes to improve:
> > 1) Add a new /sys/power/disk_offset that lets userspace specify
> > the offset and disk to use when initiating a hibernate cycle.
> >
> > 2) Adjust /sys/power/resume interpretation to also read in an
> > offset.
> 
> Read is okay per se (not consistent though), showing is not.
> It might break an ABI.

Right this is part of why I was proposing making a new attribute.

The current RFC implementation I sent keeps the read output the
same for /sys/power/resume.

> 
> > Actually klibc's /bin/resume has supported passing a hibernate
> > offset in since 20695264e21dcbde309cd81f73cfe2cea42e779d.
> >
> > The kernel was just lobbing anything after the device specified
> > off the string.  Instead parse that and populate hibernate offset
> > with it.
> 
> > An alternative to introducing a new sysfs parameter may be to document
> > setting these values via /sys/power/resume.  If the wrong signature is found
> > on the swapfile/swap partition by the kernel it does show an error
> > but it updates the values and they'll work when actually invoked later.
> 
> Don't you need to document new node?

Yes, I wanted to get feedback before I reworked documentation and that's
why I implemented both approaches right now.

Maybe both even make sense.
When I resubmit as a patch I'll make sure documentation is updated.

> 
> > +static int parse_device_input(const char *buf, size_t n)
> >  {
> > +   unsigned long long offset;
> > dev_t res;
> > int len = n;
> > char *name;
> > +   char *last;
> >
> > if (len && buf[len-1] == '\n')
> > len--;
> 
> I'm not sure first part even needed, but okay, it's in original code.
> 
> > name = kstrndup(buf, len, GFP_KERNEL);
> > if (!name)
> > return -ENOMEM;
> 
> Side notes.
> This whole dance b/c of high probability of '\n' at the end which
> breaks _some_ kernel parsers.
> It might make sense to do a wrapper and call the guts of this function
> with or without memory allocation depending on presence of '\n'.
> 

OK.

> > -
> 
> This is not needed to be removed.
> 
> > +   last = strrchr(name, ':');
> 
> > +   printk("%lu %s %s %d", last-name, name, last, len);
> 
> Ouch. I guess it's only for RFC.

Yes I was having problems originally and it was debug, it won't
be there when submitted for application.

> 
> > +   if (last != NULL &&
> 
> > +   (last-name) != len-1 &&
> 
> > +   sscanf(last+1, "%llu", ) == 1)
> 
> This is effectively
> 
> if (last && *(last+1)) {
>   int ret = kstrtoull(..._resume_block...);
>   if (ret)
>...warn?..
> }
> 
> ?

I'll have to look more closely, but if this simplification
works I'll switch over.

> 
> > +swsusp_resume_block = offset;
> 
> > +   swsusp_resume_device = res;
> > +
> 
> > +   return 1;
> 
> ???
> Why not traditional 0?
> 
> > +}
> 
> > @@ -1125,7 +1161,6 @@ static int __init pm_disk_init(void)
> >
> >  core_initcall(pm_disk_init);
> >
> > -
> 
> This doesn't belong to the change.
> 
> --
> With Best Regards,
> Andy Shevchenko

RE: [RFC] power/hibernate: Make passing hibernate offsets more friendly

[Mario.Limonciello]

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Wednesday, February 28, 2018 12:11 PM
> To: Limonciello, Mario 
> Cc: Rafael J . Wysocki ; ACPI Devel Maling List  a...@vger.kernel.org>; LKML 
> Subject: Re: [RFC] power/hibernate: Make passing hibernate offsets more 
> friendly
> 
> On Wed, Feb 28, 2018 at 7:43 PM, Mario Limonciello
>  wrote:
> > Currently the only way to specify a hibernate offset for a swap
> > file is on the kernel command line.
> >
> > This makes some changes to improve:
> > 1) Add a new /sys/power/disk_offset that lets userspace specify
> > the offset and disk to use when initiating a hibernate cycle.
> >
> > 2) Adjust /sys/power/resume interpretation to also read in an
> > offset.
> 
> Read is okay per se (not consistent though), showing is not.
> It might break an ABI.

Right this is part of why I was proposing making a new attribute.

The current RFC implementation I sent keeps the read output the
same for /sys/power/resume.

> 
> > Actually klibc's /bin/resume has supported passing a hibernate
> > offset in since 20695264e21dcbde309cd81f73cfe2cea42e779d.
> >
> > The kernel was just lobbing anything after the device specified
> > off the string.  Instead parse that and populate hibernate offset
> > with it.
> 
> > An alternative to introducing a new sysfs parameter may be to document
> > setting these values via /sys/power/resume.  If the wrong signature is found
> > on the swapfile/swap partition by the kernel it does show an error
> > but it updates the values and they'll work when actually invoked later.
> 
> Don't you need to document new node?

Yes, I wanted to get feedback before I reworked documentation and that's
why I implemented both approaches right now.

Maybe both even make sense.
When I resubmit as a patch I'll make sure documentation is updated.

> 
> > +static int parse_device_input(const char *buf, size_t n)
> >  {
> > +   unsigned long long offset;
> > dev_t res;
> > int len = n;
> > char *name;
> > +   char *last;
> >
> > if (len && buf[len-1] == '\n')
> > len--;
> 
> I'm not sure first part even needed, but okay, it's in original code.
> 
> > name = kstrndup(buf, len, GFP_KERNEL);
> > if (!name)
> > return -ENOMEM;
> 
> Side notes.
> This whole dance b/c of high probability of '\n' at the end which
> breaks _some_ kernel parsers.
> It might make sense to do a wrapper and call the guts of this function
> with or without memory allocation depending on presence of '\n'.
> 

OK.

> > -
> 
> This is not needed to be removed.
> 
> > +   last = strrchr(name, ':');
> 
> > +   printk("%lu %s %s %d", last-name, name, last, len);
> 
> Ouch. I guess it's only for RFC.

Yes I was having problems originally and it was debug, it won't
be there when submitted for application.

> 
> > +   if (last != NULL &&
> 
> > +   (last-name) != len-1 &&
> 
> > +   sscanf(last+1, "%llu", ) == 1)
> 
> This is effectively
> 
> if (last && *(last+1)) {
>   int ret = kstrtoull(..._resume_block...);
>   if (ret)
>...warn?..
> }
> 
> ?

I'll have to look more closely, but if this simplification
works I'll switch over.

> 
> > +swsusp_resume_block = offset;
> 
> > +   swsusp_resume_device = res;
> > +
> 
> > +   return 1;
> 
> ???
> Why not traditional 0?
> 
> > +}
> 
> > @@ -1125,7 +1161,6 @@ static int __init pm_disk_init(void)
> >
> >  core_initcall(pm_disk_init);
> >
> > -
> 
> This doesn't belong to the change.
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [RFC] power/hibernate: Make passing hibernate offsets more friendly

[Andy Shevchenko]

On Wed, Feb 28, 2018 at 7:43 PM, Mario Limonciello
 wrote:
> Currently the only way to specify a hibernate offset for a swap
> file is on the kernel command line.
>
> This makes some changes to improve:
> 1) Add a new /sys/power/disk_offset that lets userspace specify
> the offset and disk to use when initiating a hibernate cycle.
>
> 2) Adjust /sys/power/resume interpretation to also read in an
> offset.

Read is okay per se (not consistent though), showing is not.
It might break an ABI.

> Actually klibc's /bin/resume has supported passing a hibernate
> offset in since 20695264e21dcbde309cd81f73cfe2cea42e779d.
>
> The kernel was just lobbing anything after the device specified
> off the string.  Instead parse that and populate hibernate offset
> with it.

> An alternative to introducing a new sysfs parameter may be to document
> setting these values via /sys/power/resume.  If the wrong signature is found
> on the swapfile/swap partition by the kernel it does show an error
> but it updates the values and they'll work when actually invoked later.

Don't you need to document new node?

> +static int parse_device_input(const char *buf, size_t n)
>  {
> +   unsigned long long offset;
> dev_t res;
> int len = n;
> char *name;
> +   char *last;
>
> if (len && buf[len-1] == '\n')
> len--;

I'm not sure first part even needed, but okay, it's in original code.

> name = kstrndup(buf, len, GFP_KERNEL);
> if (!name)
> return -ENOMEM;

Side notes.
This whole dance b/c of high probability of '\n' at the end which
breaks _some_ kernel parsers.
It might make sense to do a wrapper and call the guts of this function
with or without memory allocation depending on presence of '\n'.

> -

This is not needed to be removed.

> +   last = strrchr(name, ':');

> +   printk("%lu %s %s %d", last-name, name, last, len);

Ouch. I guess it's only for RFC.

> +   if (last != NULL &&

> +   (last-name) != len-1 &&

> +   sscanf(last+1, "%llu", ) == 1)

This is effectively

if (last && *(last+1)) {
  int ret = kstrtoull(..._resume_block...);
  if (ret)
   ...warn?..
}

?

> +swsusp_resume_block = offset;

> +   swsusp_resume_device = res;
> +

> +   return 1;

???
Why not traditional 0?

> +}

> @@ -1125,7 +1161,6 @@ static int __init pm_disk_init(void)
>
>  core_initcall(pm_disk_init);
>
> -

This doesn't belong to the change.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] power/hibernate: Make passing hibernate offsets more friendly

[Andy Shevchenko]

On Wed, Feb 28, 2018 at 7:43 PM, Mario Limonciello
 wrote:
> Currently the only way to specify a hibernate offset for a swap
> file is on the kernel command line.
>
> This makes some changes to improve:
> 1) Add a new /sys/power/disk_offset that lets userspace specify
> the offset and disk to use when initiating a hibernate cycle.
>
> 2) Adjust /sys/power/resume interpretation to also read in an
> offset.

Read is okay per se (not consistent though), showing is not.
It might break an ABI.

> Actually klibc's /bin/resume has supported passing a hibernate
> offset in since 20695264e21dcbde309cd81f73cfe2cea42e779d.
>
> The kernel was just lobbing anything after the device specified
> off the string.  Instead parse that and populate hibernate offset
> with it.

> An alternative to introducing a new sysfs parameter may be to document
> setting these values via /sys/power/resume.  If the wrong signature is found
> on the swapfile/swap partition by the kernel it does show an error
> but it updates the values and they'll work when actually invoked later.

Don't you need to document new node?

> +static int parse_device_input(const char *buf, size_t n)
>  {
> +   unsigned long long offset;
> dev_t res;
> int len = n;
> char *name;
> +   char *last;
>
> if (len && buf[len-1] == '\n')
> len--;

I'm not sure first part even needed, but okay, it's in original code.

> name = kstrndup(buf, len, GFP_KERNEL);
> if (!name)
> return -ENOMEM;

Side notes.
This whole dance b/c of high probability of '\n' at the end which
breaks _some_ kernel parsers.
It might make sense to do a wrapper and call the guts of this function
with or without memory allocation depending on presence of '\n'.

> -

This is not needed to be removed.

> +   last = strrchr(name, ':');

> +   printk("%lu %s %s %d", last-name, name, last, len);

Ouch. I guess it's only for RFC.

> +   if (last != NULL &&

> +   (last-name) != len-1 &&

> +   sscanf(last+1, "%llu", ) == 1)

This is effectively

if (last && *(last+1)) {
  int ret = kstrtoull(..._resume_block...);
  if (ret)
   ...warn?..
}

?

> +swsusp_resume_block = offset;

> +   swsusp_resume_device = res;
> +

> +   return 1;

???
Why not traditional 0?

> +}

> @@ -1125,7 +1161,6 @@ static int __init pm_disk_init(void)
>
>  core_initcall(pm_disk_init);
>
> -

This doesn't belong to the change.

-- 
With Best Regards,
Andy Shevchenko