Re: [PATCH/RFC] libata: fix can't free irq 14 on legacy mode pata driver

[Albert Lee <albertcc@tw.ibm.com>]

Unicorn Chang wrote:
> -fix can't free irq 14 on legacy mode pata driver
> -fix the ata_host_set_remove() memory leak problem
> 
> Signed-off-by: Unicorn Chang <uchang@tw.ibm.com>
> ---
> 
> keep all ata_host_set structure for free irq use on legacy mode pata
> driver. 
> (Patch against upstream branch)
> 
> using "rmmod pata_amd" and then "modprobe pata_amd" to produce bug and
> same bug report by Krzysztof Halasa:
> http://marc.theaimsgroup.com/?l=linux-kernel&m=114122847813360&w=2
> 
> --- a/drivers/scsi/libata-core.c	2006-04-10 16:30:46.718750000 +0800
> +++ b/drivers/scsi/libata-core.c	2006-04-11 17:02:52.109375000 +0800
> @@ -4794,6 +4794,8 @@ int ata_device_add(const struct ata_prob
>  		ata_scsi_scan_host(ap);
>  	}
>  
> +	/* don't lose last one, free irq will need it */
> +	host_set->host_set_chain = dev_get_drvdata(dev);
>  	dev_set_drvdata(dev, host_set);
>  
>  	VPRINTK("EXIT, returning %u\n", ent->n_ports);
> @@ -4826,6 +4828,9 @@ void ata_host_set_remove(struct ata_host
>  	struct ata_port *ap;
>  	unsigned int i;
>  
> +	if(host_set->host_set_chain)
> +		ata_host_set_remove(host_set->host_set_chain);
> +	
>  	for (i = 0; i < host_set->n_ports; i++) {
>  		ap = host_set->ports[i];
>  		scsi_remove_host(ap->host);
> --- a/include/linux/libata.h	2006-04-10 16:32:06.656250000 +0800
> +++ b/include/linux/libata.h	2006-04-10 17:51:19.812500000 +0800
> @@ -304,6 +304,7 @@ struct ata_host_set {
>  	int			simplex_claimed;	/* Keep seperate in case we
>  							   ever need to do this locked */
>  	struct ata_port *	ports[0];
> +	struct ata_host_set	*host_set_chain;	/* Keep last host_set */
>  };
>  
>  struct ata_queued_cmd { 
> 
> 

Jeff,

Althouth this patch fixes the irq/host_set leak, the SIMPLEX flag
is still assigned to both host_sets of legacy primary/secondary port.
Under legacy two-port case, both ports might claim the SIMPLEX flag (and DMA engine).
This is another problem not fixed by this patch.

Is there any consideration to assign each legacy port its own host_set?
Maybe we could let both primary/secondary legacy port share the same host_set,
as done in the native case? (Maybe adding an addtional irq to host_set is good enough.)
For each PCI devfn, assigning a corresponding host_set (instead of two) looks more reasonable.

--
albert