Messages in this thread | | | Subject | Re: [PATCH 2.4] add SMBIOS information to /proc/smbios/ | From | Michael Brown <> | Date | Thu, 29 Apr 2004 07:58:16 -0500 |
| |
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/
| |