Re: [PATCH 3/21] advansys: Convert to pci_register_driver interface

[Jeff Garzik <jeff@garzik.org>]

Matthew Wilcox wrote:
> +static int __devinit
> +advansys_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> +	int ioport;
> +	struct Scsi_Host *shost;
> +
> +	if (pci_enable_device(pdev))
> +		goto fail;
> +
> +	ioport = pci_resource_start(pdev, 0);
> +	shost = advansys_board_found(ioport, &pdev->dev, ASC_IS_PCI);
> +
> +	if (shost) {
> +		pci_set_drvdata(pdev, shost);
> +		return 0;
> +	}
> +
> +	pci_disable_device(pdev);
> + fail:
> +	return -ENODEV;
> +}

1) you should propagate pci_enable_device return value to caller on error

1.1) in general, following my comment #1 will make this function look 
more like other normal Linux code, i.e.

	rc = foo
	if (rc)
		goto err_out

	return 0;

      err_out:
	return rc;

2) you dropped the check for pci_resource_start() returning zero (look 
for 'iop == 0' in original code)

3) what happened to PCI BAR #1 ?  shouldn't you move that call here too? 
  it gets ioremapped (look at 'pci_memory_address' in old code, right 
next to pci_resource_start call)

4) it is often wise to add sanity checks that ensure that PCI BAR #0 == 
IORESOURCE_IO and PCI BAR #1 == IORESOURCE_MEM



> +static void __devexit advansys_pci_remove(struct pci_dev *pdev)
> +{
> +	int i;
> +	struct Scsi_Host *shost = pci_get_drvdata(pdev);
> +	scsi_remove_host(shost);
> +	advansys_release(shost);
> +
> +	for (i = 0; i < asc_board_count; i++) {
> +		if (asc_host[i] == shost) {
> +			asc_host[i] = NULL;
> +			break;
> +		}
> +	}
> +
> +	pci_disable_device(pdev);

5) call pci_set_drvdata(pdev, NULL) as with other PCI drivers


> +static struct pci_driver advansys_pci_driver = {
> +	.name = "advansys",
> +	.id_table = advansys_pci_tbl,
> +	.probe = advansys_pci_probe,
> +	.remove = __devexit_p(advansys_pci_remove),
> +};

6) suggestions:  tab alignment for struct member values; makes it far 
easier to read.


> +static int __init advansys_init(void)
> +{
> +	int count, error;
> +	count = advansys_detect();
> +	error = pci_register_driver(&advansys_pci_driver);
> +
> +	/*
> +	 * If we found some ISA, EISA or VLB devices, we must not fail.
> +	 * We may not drive any PCI devices, but it's better to drive
> +	 * the cards that we successfully discovered than none at all.
> +	 */
> +	if (count > 0)
> +		error = 0;
> +	return error;
> +}
> +
> +static void __exit advansys_exit(void)
> +{
> +	int i;
> +
> +	pci_unregister_driver(&advansys_pci_driver);

7) bug: don't unregister, if pci_register_driver() failed during init


> +	for (i = 0; i < asc_board_count; i++) {
> +		struct Scsi_Host *host = asc_host[i];
> +		if (!host)
> +			continue;
> +		scsi_remove_host(host);
> +		advansys_release(host);
> +		asc_host[i] = NULL;

this last line of code is rather pointless, isn't it?