> -----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 ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
