lkml.org 
[lkml]   [2004]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2.4] add SMBIOS information to /proc/smbios/
From
Date
On Thu, 2004-04-29 at 00:50, viro@parcelfarce.linux.theplanet.co.uk
wrote:
> On Thu, Apr 29, 2004 at 12:39:33AM -0500, Michael Brown wrote:
> > +smbios_read_table_entry_point(char *page, char **start,
> > + off_t off, int count,
> > + int *eof, void *data)
> > +{
> > + unsigned int max_off = sizeof(the_smbios_device.table_eps);
> > + MOD_INC_USE_COUNT;
> > +
> > + memcpy(page, &the_smbios_device.table_eps, max_off);
> > +
> > + MOD_DEC_USE_COUNT;
> > + return max_off;
> > +}
>
> *LART*
>
> Use ->owner instead of (racy) messing with MOD_{INC,DEC}_USE_COUNT.

First, thanks for the feedback Al. I really appreciate it.

I had set ->owner in my module init function.

in smbios_init():
+ smbios_dir->owner = THIS_MODULE;
and a bit further down...
+ table_eps_file->owner = THIS_MODULE;

Documentation/DocBook/procfs_example.c is what I used as my example
along with the "Linux Device Drivers" book. The in-tree docs have
MOD_{INC,DEC}_USE_COUNT, so that is what I did. If these are not
necessary, I will remove them.

Also note that one of my standard tests that I do before submission is a
"while true; insmod smbios; rmmod smbios; done &"
"while true; hexdump -C /proc/smbios/*; done"

and let it run for a few hours. This code had all passed that test.

>
> > +/* Use the more flexible procfs style. We tell the core that we can handle
> > + * >4k transactions by setting *start. When doing this we also have to handle
> > + * our own *eof and make sure not to overrun the page or count given.
> > + */
> > +static ssize_t
> > +smbios_read_table(char *page, char **start,
> > + off_t offset, int count,
> > + int *eof, void *data)
> > +{
> > + u8 *buf;
> > + int i = 0;
> > + int max_off = the_smbios_device.smbios_table_real_length;
> > + MOD_INC_USE_COUNT;
> > +
> > + if (offset > max_off)
> > + return 0;
> > +
> > + if (count > (max_off - offset))
> > + count = max_off - offset;
> > +
> > + buf = ioremap(the_smbios_device.table_eps.table_address, max_off);
> > + if (buf == NULL)
> > + return -ENXIO;
> > +
> > + /* memcpy( buffer, buf+pos, count ); */
> > + for (i = 0; i < count; ++i) {
> > + page[i] = readb( buf+offset+i );
> > + }
> > +
> > + iounmap(buf);
> > +
> > + *start = page; /* tells procfs that we handle >1 page */
> > + MOD_DEC_USE_COUNT;
> > + return count;
> > +}
>
> ... and the reason why you need to go through procfs default ->read()
> instead of providing an obvious one of your own would be...? Note that
> it would be smaller and simpler than your callback above.

You will have to excuse my ignorace here. I used the in tree
Documentation/, google, and fs/proc/proc_misc.c as reference. I assume
you mean something like this:


table_file = create_proc_read_entry( "table",
0444, smbios_dir,
smbios_read_table,
NULL);
if(table_file)
table_file->fops.read = &smbios_read_table;

Correct?

And then in my smbios_read_table, do a normal read-like method where I
put_user().

Comparing the code above to proc_misc.c, it looks like almost the same
amount of code, but it is probably easier to understand than the above
magic (which, admittedly took a while of googling to figure out), but I
thought that was just normal proc/ style and I was just unfamiliar with
it.

I will make these two changes and resubmit later today.

Thanks,
Michael






-
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-03-22 14:02    [W:0.045 / U:0.508 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site