On 7/6/05, Nigel Cunningham <[email protected]> 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 <[email protected]>
> + * Copyright (C) 1998,2001,2002 Pavel Machek <[email protected]>
> + * Copyright (C) 2002-2003 Florent Chabaud <[email protected]>
> + * Copyright (C) 2002-2005 Nigel Cunningham <[email protected]>
> + *
> + * 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |