lkml.org 
[lkml]   [2005]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2005-07-06 09:55    [W:0.030 / U:0.708 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site