Prev: oprofile hotplug fixes for x86
Next: [PATCH] AT91: PM: dual ram controller support
From: Paul Mundt on 10 May 2010 07:20
On Mon, May 10, 2010 at 01:37:34PM +0300, Eduardo Valentin wrote:
> + */
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/proc_fs.h>
> +#include <linux/seq_file.h>
> +
> +extern const struct seq_operations socinfo_op;

This doesn't look promising..

> +static int socinfo_open(struct inode *inode, struct file *file)
> +{
> + return seq_open(file, &socinfo_op);
> +}
> +
If you use single_open() ..

> +static const struct file_operations proc_socinfo_operations = {
> + .open = socinfo_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = seq_release,
> +};
> +
... and single_release(), then none of the seq_operations are necessary.

> +static int __init proc_socinfo_init(void)
> +{
> + proc_create("socinfo", 0, NULL, &proc_socinfo_operations);
> + return 0;
> +}
> +module_init(proc_socinfo_init);

proc_create() can fail, so some error handling here would be nice.

On Mon, May 10, 2010 at 01:37:35PM +0300, Eduardo Valentin wrote:
> +static int c_show(struct seq_file *m, void *v)
> +{
> + seq_printf(m, "SoC\t: %s\n", socinfo);
> +
> + return 0;
> +}
> +
> +static void *c_start(struct seq_file *m, loff_t *pos)
> +{
> + return *pos < 1 ? (void *)1 : NULL;
> +}
> +
> +static void *c_next(struct seq_file *m, void *v, loff_t *pos)
> +{
> + ++*pos;
> + return NULL;
> +}
> +
> +static void c_stop(struct seq_file *m, void *v)
> +{
> +}
> +
> +const struct seq_operations socinfo_op = {
> + .start = c_start,
> + .next = c_next,
> + .stop = c_stop,
> + .show = c_show
> +};

You'll still need the show function, but all of the rest of this is just
duplicating what single_open() already does. If the socinfo string is
static you may also want to rework this a bit so you can just stash the
string in the proc_dir_entry private data. Combine this with something
like kstrdup() and you'll save yourself a bit of stack while you're at
it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Eduardo Valentin on 10 May 2010 08:40
On Mon, May 10, 2010 at 01:13:00PM +0200, ext Paul Mundt wrote:
> On Mon, May 10, 2010 at 01:37:34PM +0300, Eduardo Valentin wrote:
> > + */
> > +#include <linux/fs.h>
> > +#include <linux/init.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/seq_file.h>
> > +
> > +extern const struct seq_operations socinfo_op;
>
> This doesn't look promising..

Right.. as stated in patch description (maybe not that clear), this was
basically same thing which you see under fs/proc/cpuinfo.c

>
> > +static int socinfo_open(struct inode *inode, struct file *file)
> > +{
> > + return seq_open(file, &socinfo_op);
> > +}
> > +
> If you use single_open() ..
>
> > +static const struct file_operations proc_socinfo_operations = {
> > + .open = socinfo_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = seq_release,
> > +};
> > +
> .. and single_release(), then none of the seq_operations are necessary.

Good! will replace than with single_[open,release].

>
> > +static int __init proc_socinfo_init(void)
> > +{
> > + proc_create("socinfo", 0, NULL, &proc_socinfo_operations);
> > + return 0;
> > +}
> > +module_init(proc_socinfo_init);
>
> proc_create() can fail, so some error handling here would be nice.

right. will add it.

>
> On Mon, May 10, 2010 at 01:37:35PM +0300, Eduardo Valentin wrote:
> > +static int c_show(struct seq_file *m, void *v)
> > +{
> > + seq_printf(m, "SoC\t: %s\n", socinfo);
> > +
> > + return 0;
> > +}
> > +
> > +static void *c_start(struct seq_file *m, loff_t *pos)
> > +{
> > + return *pos < 1 ? (void *)1 : NULL;
> > +}
> > +
> > +static void *c_next(struct seq_file *m, void *v, loff_t *pos)
> > +{
> > + ++*pos;
> > + return NULL;
> > +}
> > +
> > +static void c_stop(struct seq_file *m, void *v)
> > +{
> > +}
> > +
> > +const struct seq_operations socinfo_op = {
> > + .start = c_start,
> > + .next = c_next,
> > + .stop = c_stop,
> > + .show = c_show
> > +};
>
> You'll still need the show function, but all of the rest of this is just
> duplicating what single_open() already does. If the socinfo string is
> static you may also want to rework this a bit so you can just stash the
> string in the proc_dir_entry private data. Combine this with something
> like kstrdup() and you'll save yourself a bit of stack while you're at
> it.

yeah. will add those comments as well.


Thanks for reviewing,

--
Eduardo Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Paul Mundt on 10 May 2010 08:40
On Mon, May 10, 2010 at 03:35:14PM +0300, Eduardo Valentin wrote:
> On Mon, May 10, 2010 at 01:13:00PM +0200, ext Paul Mundt wrote:
> > On Mon, May 10, 2010 at 01:37:34PM +0300, Eduardo Valentin wrote:
> > > + */
> > > +#include <linux/fs.h>
> > > +#include <linux/init.h>
> > > +#include <linux/proc_fs.h>
> > > +#include <linux/seq_file.h>
> > > +
> > > +extern const struct seq_operations socinfo_op;
> >
> > This doesn't look promising..
>
> Right.. as stated in patch description (maybe not that clear), this was
> basically same thing which you see under fs/proc/cpuinfo.c
>
The cpuinfo case is a bit more complex since you have actual real work to
do in the ->start op and you will iterate over the ->show op for each
CPU. In your socinfo case you're just following the single_xxx()
semantics so using those helpers there simplifies things quite a bit.

Architectures that do not support SMP technically employ single_open()
semantics, but the fs/proc/cpuinfo.c abstraction requires the
architecture to provide seq ops regardless.

Note that in the cpuinfo case there is often special handling for the
single (or boot CPU) case, such as printing out a descriptor for the
machine type and so on. You might be better off just extending cpuinfo
rather than introducing another /proc abstraction, presumably your
socinfo string will be fixed regardless of whether it's SMP or not.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Felipe Balbi on 10 May 2010 09:00
On Mon, May 10, 2010 at 01:13:00PM +0200, ext Paul Mundt wrote:
>You'll still need the show function, but all of the rest of this is just
>duplicating what single_open() already does. If the socinfo string is
>static you may also want to rework this a bit so you can just stash the
>string in the proc_dir_entry private data. Combine this with something
>like kstrdup() and you'll save yourself a bit of stack while you're at
>it.

doesn't ksrtdup() cause memleak ?? Or is it only when used with
module parameters ??

--
balbi

DefectiveByDesign.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Eduardo Valentin on 10 May 2010 09:10
On Mon, May 10, 2010 at 02:54:40PM +0200, Balbi Felipe (Nokia-D/Helsinki) wrote:
> On Mon, May 10, 2010 at 01:13:00PM +0200, ext Paul Mundt wrote:
> >You'll still need the show function, but all of the rest of this is just
> >duplicating what single_open() already does. If the socinfo string is
> >static you may also want to rework this a bit so you can just stash the
> >string in the proc_dir_entry private data. Combine this with something
> >like kstrdup() and you'll save yourself a bit of stack while you're at
> >it.
>
> doesn't ksrtdup() cause memleak ?? Or is it only when used with
> module parameters ??

I'm not aware of the module parameter stuff.. but the leak might be other thing
than kstrdup?
>
> --
> balbi
>
> DefectiveByDesign.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: oprofile hotplug fixes for x86
Next: [PATCH] AT91: PM: dual ram controller support