| Date | Wed, 6 Jul 2005 09:18:37 +0300 | From | Pekka Enberg <> | Subject | Re: [PATCH] [43/48] Suspend2 2.1.9.8 for 2.6.12: 619-userspace-nofreeze.patch |
| |
On 7/6/05, Nigel Cunningham <nigel@suspend2.net> wrote: > diff -ruNp 620-userui-header.patch-old/kernel/power/suspend2_core/ui.c 620-userui-header.patch-new/kernel/power/suspend2_core/ui.c > --- 620-userui-header.patch-old/kernel/power/suspend2_core/ui.c 1970-01-01 10:00:00.000000000 +1000
Is a directory this deep really necessary? Please consider putting this under kernel/power/.
> +++ 620-userui-header.patch-new/kernel/power/suspend2_core/ui.c 2005-07-05 23:48:59.000000000 +1000 > @@ -0,0 +1,1186 @@ > +/* > + * kernel/power/ui.c > + * > + * Copyright (C) 1998-2001 Gabor Kuti <seasons@fornax.hu> > + * Copyright (C) 1998,2001,2002 Pavel Machek <pavel@suse.cz> > + * Copyright (C) 2002-2003 Florent Chabaud <fchabaud@free.fr> > + * Copyright (C) 2002-2005 Nigel Cunningham <nigel@suspend2.net> > + * > + * This file is released under the GPLv2. > + * > + * Routines for Software Suspend's user interface. > + * > + * The user interface code talks to a userspace program via a > + * netlink socket. > + * > + * The kernel side: > + * - starts the userui program; > + * - sends text messages and progress bar status; > + * > + * The user space side: > + * - passes messages regarding user requests (abort, toggle reboot etc) > + * > + */ > +#define SUSPEND_CONSOLE_C > + > +#include "../power.h"
Please either move this file under kernel/power/ or move the header to include/linux/.
> +void s2_userui_message(unsigned long section, unsigned long level, > + int normally_logged, > + const char *fmt, va_list args); > +unsigned long userui_update_progress(unsigned long value, unsigned long maximum, > + const char *fmt, va_list args); > +void userui_prepare_console(void); > +void userui_cleanup_console(void);
Shouldn't these be extern and in a header file?
> +static int userui_nl_set_state(int n) > +{ > + /* Only let them change certain settings */ > + static const int suspend_action_mask = > + (1 << SUSPEND_REBOOT) | (1 << SUSPEND_PAUSE) | (1 << SUSPEND_SLOW) | > + (1 << SUSPEND_LOGALL) | (1 << SUSPEND_SINGLESTEP) | > + (1 << SUSPEND_PAUSE_NEAR_PAGESET_END); > + > + suspend_action = (suspend_action & (~suspend_action_mask)) | > + (n & suspend_action_mask); > + > + return 0;
Always returns zero so drop the return value.
> +} > + > +static int userui_nl_set_progress_granularity(int n) > +{ > + if (n < 1) n = 1; > + progress_granularity = n; > + return 0;
Same here.
> +} > + > +static int userui_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, int *errp) > +{ > + int type; > + int *data; > + > + *errp = 0; > + > + if (!(nlh->nlmsg_flags & NLM_F_REQUEST)) > + return 0; > + > + type = nlh->nlmsg_type; > + > + /* A control message: ignore them */ > + if (type < USERUI_MSG_BASE) > + return 0; > + > + /* Unknown message: reply with EINVAL */ > + if (type >= USERUI_MSG_MAX) { > + *errp = -EINVAL; > + return -1;
Just return the error value and errp can go away.
> +static unsigned long userui_memory_needed(void) > +{ > + /* ball park figure of 128 pages */ > + return (128 * PAGE_SIZE);
Where does this magic 128 come from?
> +} > + > +unsigned long userui_update_progress(unsigned long value, unsigned long maximum, > + const char *fmt, va_list args) > +{ > + static int last_step = -1; > + struct userui_msg_params msg; > + int bitshift = generic_fls(maximum) - 16;
What's this magic 16?
> +char suspend_wait_for_keypress(int timeout) > +{ > + int fd; > + char key = '\0'; > + struct termios t, t_backup; > + > + if (userui_pid != -1) { > + wait_for_key_via_userui(); > + key = 32;
What's this magic 32?
> +/* abort_suspend > + * > + * Description: Begin to abort a cycle. If this wasn't at the user's request > + * (and we're displaying output), tell the user why and wait for > + * them to acknowledge the message. > + * Arguments: A parameterised string (imagine this is printk) to display, > + * telling the user why we're aborting. > + */
Please use proper kerneldoc format instead of inventing your own.
> +void suspend2_schedule_message(int message_number) > +{ > + struct waiting_message * new_message = > + kmalloc(sizeof(struct waiting_message), GFP_ATOMIC); > + > + if (!new_message) { > + printk("Argh. Unable to allocate memory for " > + "scheduling the display of a message.\n");
KERN_* constants please.
> +extern asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, > + unsigned long arg);
Looks as if you're doing quite a bit of sys_* calls in the kernel. Could this stuff be pushed out to userspace by any chance? - 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/
|