RE: [RFC] power/hibernate: Make passing hibernate offsets more friendly
> -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
> -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
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
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
> -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
> -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
On Wed, Feb 28, 2018 at 7:43 PM, Mario Limonciellowrote: > 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
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