[openib-general] Re: [PATCH 11 of 20] ipath - core driver, part 4 of 4

[Bryan O'Sullivan]

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