Re: [PATCH 7/7] gdth: switch to modern scsi host registration

[Jeff Garzik <jeff@garzik.org>]

in general, is sane.  three problems...


Christoph Hellwig wrote:
> +	/* As default we do not probe for EISA or ISA controllers */
> +	if (probe_eisa_isa) {
> +		/* scanning for controllers, at first: ISA controller */
> +#ifdef CONFIG_ISA
> +		for (isa_bios = 0xc8000UL; isa_bios <= 0xd8000UL;
> +			     isa_bios += 0x8000UL) {
> +			if (gdth_ctr_count >= MAXHA)
> +                		break;
> +			gdth_isa_probe_one(isa_bios);
> +		}
> +#endif
> +#ifdef CONFIG_EISA
> +		for (eisa_slot = 0x1000; eisa_slot <= 0x8000; eisa_slot += 0x1000) {
> +			if (gdth_ctr_count >= MAXHA)
> +				break;
> +			gdth_eisa_probe_one(eisa_slot);
> +		}
> +	}
> +#endif

1) endif should be above the brace


> +static void __exit gdth_exit(void)
> +{
> +	gdth_ha_str *ha;
> +
> +	list_for_each_entry(ha, &gdth_instances, list)
> +		gdth_remove_one(ha);

2) I think it's nicer to have two loops:  scsi_remove_host() for all 
ha-attached devices, and then the cleanup.  More pragmatic:  gives 
hardware more time to "settle" before turning off DMA engines.  A safer 
approach, and one that scsi_module.c follows.

3) This introduces a modicum of confusion between the gdth_ctr_tab[] 
stuff and the gdth_instances list.  While not strictly broken, this 
separation creates a foundation where the two can easily get out of sync.

	Jeff