lkml.org 
[lkml]   [2006]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -mm] swsusp: userland interface (rev 2)
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> Hi,
>
> This patch introduces a user space interface for swsusp.

How will we know if/when this feature is ready for mainline? What criteria
can we use to judge that?

Will you be developing and long-term maintaining the userspace tools? Is
it your expectation/hope that distros will migrate onto using them? etc.

> +
> +static int snapshot_open(struct inode *inode, struct file *filp)
> +{
> + struct snapshot_data *data;
> +
> + if (!atomic_dec_and_test(&device_available)) {
> + atomic_inc(&device_available);

You may find that atomic_add_unless(..., -1, ...) is neater here, and
closes the tiny race.

> + return -EBUSY;
> + }
> +
> + if ((filp->f_flags & O_ACCMODE) == O_RDWR)
> + return -ENOSYS;
> +
> + nonseekable_open(inode, filp);
> + data = &snapshot_state;
> + filp->private_data = data;
> + memset(&data->handle, 0, sizeof(struct snapshot_handle));

<goes off hunting elsewhere for the defn of data->handle. grr>

> +static ssize_t snapshot_read(struct file *filp, char __user *buf,
> + size_t count, loff_t *offp)
> +{
> + struct snapshot_data *data;
> + ssize_t res;
> +
> + data = filp->private_data;
> + res = snapshot_read_next(&data->handle, count);
> + if (res > 0) {
> + if (copy_to_user(buf, data_of(data->handle), res))
> + res = -EFAULT;
> + else
> + *offp = data->handle.offset;
> + }
> + return res;
> +}

It's more conventional for a read() to return less-than-was-asked-for when
it hits a fault. Doesn't matter though - lots of drivers do it this way.

> +static ssize_t snapshot_write(struct file *filp, const char __user *buf,
> + size_t count, loff_t *offp)
> +{
> + struct snapshot_data *data;
> + ssize_t res;
> +
> + data = filp->private_data;
> + res = snapshot_write_next(&data->handle, count);
> + if (res > 0) {
> + if (copy_from_user(data_of(data->handle), buf, res))
> + res = -EFAULT;
> + else
> + *offp = data->handle.offset;
> + }
> + return res;
> +}

Ditto.

> +static int snapshot_ioctl(struct inode *inode, struct file *filp,
> + unsigned int cmd, unsigned long arg)
> +{
>
> ...
>
> + case SNAPSHOT_ATOMIC_RESTORE:
> + if (data->mode != O_WRONLY || !data->frozen ||
> + !snapshot_image_loaded(&data->handle)) {
> + error = -EPERM;
> + break;
> + }
> + down(&pm_sem);
> + pm_prepare_console();
> + error = device_suspend(PMSG_FREEZE);
> + if (!error) {
> + mb();
> + error = swsusp_resume();
> + device_resume();
> + }

whee, what does the mystery barrier do? It'd be nice to comment this
(please always comment open-coded barriers).

> + case SNAPSHOT_GET_SWAP_PAGE:
> + if (!access_ok(VERIFY_WRITE, (unsigned long __user *)arg, _IOC_SIZE(cmd))) {
> + error = -EINVAL;
> + break;
> + }

Why do we need an access_ok() here?

Should it return -EFAULT?


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-01-24 22:14    [W:0.074 / U:0.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site