On Fri, 2005-12-30 at 00:39 -0800, Greg KH wrote:
> > +void ipath_chip_done(void)
> > +{
> > +}
> > +
> > +void ipath_chip_cleanup(struct ipath_devdata * dd)
> > +{
> > +}
>
> What are these two empty functions for?
They're just as dead as they look.
> > +static ssize_t show_status_str(struct device *dev,
> how big can this "status string" be?
Just a few dozen bytes.
> If it's even getting close to
> PAGE_SIZE, this doesn't need to be a sysfs attribute, but you should
> break it up into its individual pieces.
Do you think that's still warranted, given this?
> > +static ssize_t show_unit(struct device *dev,
> Don't you mean -ENODEV?
Yes, thanks.
> > + snprintf(buf, PAGE_SIZE, "%u\n", dd->ipath_unit);
> > + return strlen(buf);
>
> return the snprintf() call instead of calling strlen() all the time
> please.
OK.
> > +const struct pci_device_id infinipath_pci_tbl[] = {
> > + {
> > + PCI_VENDOR_ID_PATHSCALE, PCI_DEVICE_ID_PATHSCALE_INFINIPATH_HT,
> > + PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
>
> PCI_DEVICE() instead?
OK.
> > + {0,}
>
> {},
> is all that is needed here.
OK.
> > + .driver.owner = THIS_MODULE,
>
> This line is not needed, you can remove it.
OK.
> {} not needed here.
OK.
> > +#if defined (pgprot_writecombine) && defined(_PAGE_MA_WC)
> > + printk("Remapping pages WC\n");
>
> No KERN_ level?
That should just become a debug statement.
> > + /*
> > + * set these up before registering the interrupt handler, just
> > + * in case
> > + */
> > + devdata[dev].pcidev = pdev;
> > + pci_set_drvdata(pdev, &(devdata[dev]));
>
> It's not a "just in case" type thing, you have to do this before you
> register that interrupt handler, as you can be instantly called here.
OK, I'll remove the misleading comment.
> Are you sure everything else is set up properly here before calling that
> function?
I believe so. I'll double check.
> > + device_create_file(&(pdev->dev), &dev_attr_status);
> > + device_create_file(&(pdev->dev), &dev_attr_status_str);
> > + device_create_file(&(pdev->dev), &dev_attr_lid);
> > + device_create_file(&(pdev->dev), &dev_attr_mlid);
> > + device_create_file(&(pdev->dev), &dev_attr_guid);
> > + device_create_file(&(pdev->dev), &dev_attr_nguid);
> > + device_create_file(&(pdev->dev), &dev_attr_serial);
> > + device_create_file(&(pdev->dev), &dev_attr_unit);
>
> Why not use an attribute array? Makes for proper error handling if one
> of those calls does not work...
OK, thanks.
> > + /*
> > + * We used to cleanup here, with pci_release_regions, etc. but that
> > + * can cause other problems if we want to run diags, etc., so instead
> > + * defer that until driver unload.
> > + */
>
> So memory leaks are acceptable?
That clearly needs a bit of attention.
> > +fail: /* after we've done at least some of the pci setup */
> > + if (ret == -EPERM) /* disabled device, don't want module load error;
> > + * just want to carry status through to this point */
> > + ret = 0;
>
> Module load error does not happen no matter what kind of return value
> you send back from this function. So the comment is wrong, and the fact
> that you failed initializing the device is also wrong, please don't do
> this.
OK.
Thanks for the extensive comments,
<b
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
