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

[Bryan O'Sullivan]

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