On Fri, 2005-12-30 at 00:12 -0800, Greg KH wrote:
> This has grown
> into a huge file, can't you split it up into smaller pieces?
Absolutely.
> Why even save off the return value if you don't do anything with it?
I think that's just a throwback to an earlier rev of the driver.
> And please don't put assignments in the middle of if statements, that's
> just messy and harder to read (the fact that gcc made you put an extra
> () should be a hint that you were doing something wrong...)
OK.
> And does your driver work with udev? I didn't see where you were
> exporting the major:minor number of your devices to sysfs, but I might
> have missed it.
It was written in a pre-udev world, so it still uses a fixed major and
minor number. How important is this to you? Is it "nice to have", or
"blocker"? :-)
> Are you sure that's a good idea? Please do the proper thing and tear
> down your infrastructure if something fails, that's the correct thing to
> do. That way you can actually recover if something that you call in
> this function fails (like driver_create_file(), or
> pci_register_driver().) Functions don't return error values just so you
> can ignore them :)
This will take a bit of cleaning up, but it's a reasonable request.
> > +/*
> > + * note: if for some reason the unload fails after this routine, and leaves
> > + * the driver enterable by user code, we'll almost certainly crash and burn...
> > + */
>
> See, you admit that what you are doing isn't the wisest thing, which
> should tell you something...
Indeed.
> This is the call that should have cleaned up all of the memory and other
> stuff that you do above. If not, then your driver will not work in any
> hotplug pci systems, which would not be a good thing. Please do like
> Roland says and put your resources and stuff in the device specific
> structures, like the rest of the kernel drivers do.
I'm working on the appropriate hearts and minds as we speak :-)
> Why not just export ipath_ht_get_boardname instead?
Because that's too specific to HT for my personal liking.
> > +module_init(infinipath_init);
> > +module_exit(infinipath_cleanup);
> > +
> > +EXPORT_SYMBOL(infinipath_debug);
> > +EXPORT_SYMBOL(ipath_get_boardname);
>
> EXPORT_SYMBOL_GPL() ?
I don't see a problem with that.
> And put them next to the functions themselves, it's easier to notice
> that way.
OK.
Thanks again for the review,
<b
-
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]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
