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

From: Mario.Limonciello
Date: Wed Feb 28 2018 - 15:05:12 EST




> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@xxxxxxxxx]
> Sent: Wednesday, February 28, 2018 12:11 PM
> To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Cc: Rafael J . Wysocki <rjw@xxxxxxxxxxxxx>; ACPI Devel Maling List <linux-
> acpi@xxxxxxxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>
> Subject: Re: [RFC] power/hibernate: Make passing hibernate offsets more friendly
>
> On Wed, Feb 28, 2018 at 7:43 PM, Mario Limonciello
> <mario.limonciello@xxxxxxxx> 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", &offset) == 1)
>
> This is effectively
>
> if (last && *(last+1)) {
> int ret = kstrtoull(...&swsusp_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