Re: [PATCH 1/2] dpt_i2o: 64 bit support (take 4)

[Rolf Eike Beer <eike-kernel@sf-tec.de>]

[-- Attachment #1: Type: text/plain, Size: 6039 bytes --]

Miquel van Smoorenburg wrote:
> I've taken out the ifdef __linux__ code that was added, and
> I removed the unused reboot_notifier code.

This is yet another patch as it has nothing to do with the 64 bit changes.

> As before, 1/2 is the 64 bit code, 2/2 is the sysfs code.
>
> # -----
>
> This patch is an update for drivers/scsi/dpt_i2o.c.
> It applies to both 2.6.24.4 and 2.6.25
>
> It contains the following changes:
>
> * 64 bit code based on unofficial Adaptec 64 bit driver
> * removes scsi_module.c dependency, adds module_init / module_exit
>   this is needed because we need to pass the proper device to
>   scsi_add_host(), and the scsi_module.c passes NULL. With NULL,
>   code like arch/x64/kernel/pci-gart_64.c::need_iommu() crashes
>   because the dev pointer it is passed is NULL.
> * adds sysfs entry for /sys/class/dpt_i2o/dptiX so that udev
>   can create /dev/dptiX dynamically

Please trim the changelog to only describe what you are doing in this exact 
patch. For a general description you might want to send a "[patch 0/3]" mail 
with some text about the complete series of patches. Sending all patches of 
this series as a reply to the "0/n" mail also helps keeping the things 
together in mail clients.


> +#ifdef CONFIG_COMPAT
> +static long compat_adpt_ioctl(struct file *, unsigned int, unsigned long);
> +#endif
> +
>  static const struct file_operations adpt_fops = {
>  	.ioctl		= adpt_ioctl,
>  	.open		= adpt_open,
> -	.release	= adpt_close
> -};
> -
> -#ifdef REBOOT_NOTIFIER
> -static struct notifier_block adpt_reboot_notifier =
> -{
> -	 adpt_reboot_event,
> -	 NULL,
> -	 0
> -};
> +	.release	= adpt_close,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl	= compat_adpt_ioctl,
>  #endif
> +};
>
>  /* Structures and definitions for synchronous message posting.
>   * See adpt_i2o_post_wait() for description

I doubt this has anything to do with 64 bit changes, has it?

> @@ -178,8 +192,6 @@
>  	struct pci_dev *pDev = NULL;
>  	adpt_hba* pHba;
>
> -	adpt_init();
> -
>  	PINFO("Detecting Adaptec I2O RAID controllers...\n");
>
>          /* search for all Adatpec I2O RAID cards */
> @@ -248,7 +260,7 @@
>  	}
>
>  	for (pHba = hba_chain; pHba; pHba = pHba->next) {
> -		if( adpt_scsi_register(pHba,sht) < 0){
> +		if (adpt_scsi_host_alloc(pHba, sht) < 0){
>  			adpt_i2o_delete_hba(pHba);
>  			continue;
>  		}

This too.

> @@ -338,8 +354,16 @@
>
>  	/* Now fill in the SGList and command */
>  	*lenptr = len;
> -	*mptr++ = 0xD0000000|direction|len;
> -	*mptr++ = virt_to_bus(buf);
> +	if (dpt_dma64(pHba)) {
> +		*mptr++ = (0x7C<<24)+(2<<16)+0x02; /* Enable 64 bit */
> +		*mptr++ = 1 << PAGE_SHIFT;
> +		*mptr++ = 0xD0000000|direction|len;
> +		*mptr++ = dma_low(addr);
> +		*mptr++ = dma_high(addr);
> +	} else {
> +		*mptr++ = 0xD0000000|direction|len;
> +		*mptr++ = addr;
> +	}
>
>  	// Send it on it's way
>  	rcode = adpt_i2o_post_wait(pHba, msg, reqlen<<2, 120);

There are some spaces missing to get the equations readable.

> @@ -347,7 +371,7 @@
>  		sprintf(pHba->detail, "Adaptec I2O RAID");
>  		printk(KERN_INFO "%s: Inquiry Error (%d)\n",pHba->name,rcode);
>  		if (rcode != -ETIME && rcode != -EINTR)
> -			kfree(buf);
> +			pci_free_consistent(pHba->pDev, 80, buf, addr);
>  	} else {
>  		memset(pHba->detail, 0, sizeof(pHba->detail));
>  		memcpy(&(pHba->detail), "Vendor: Adaptec ", 16);
> @@ -356,7 +380,7 @@
>  		memcpy(&(pHba->detail[40]), " FW: ", 4);
>  		memcpy(&(pHba->detail[44]), (u8*) &buf[32], 4);
>  		pHba->detail[48] = '\0';	/* precautionary */
> -		kfree(buf);
> +		pci_free_consistent(pHba->pDev, 80, buf, addr);
>  	}
>  	adpt_i2o_status_get(pHba);
>  	return ;

Too many magic numbers. Please put the 80 (and 14 and 17 from above) into some 
defines. Something like FOO_ADAPTER_MESSAGE_SIZE, describing roughly what 
that number is actually meant for.

> +/*
> + *	Turn a pointer to ioctl reply data into an u32 'context'
> + */
> +static u32 adpt_ioctl_to_context(adpt_hba * pHba, void *reply)
> +{
> +#if BITS_PER_LONG == 32
> +	return (u32)(unsigned long)reply;
> +#else
> +	ulong flags = 0;
> +	u32 nr, i;
> +
> +	spin_lock_irqsave(pHba->host->host_lock, flags);
> +	nr =  sizeof(pHba->ioctl_reply_context) /
> +		sizeof(pHba->ioctl_reply_context[0]);

ARRAY_SIZE(pHba->ioctl_reply_context)

> @@ -906,8 +1004,20 @@
>  	}
>
>  	pci_set_master(pDev);
> -	if (pci_set_dma_mask(pDev, DMA_32BIT_MASK))
> -		return -EINVAL;
> +	/*
> +	 *	Only try to enable DMA 64 bit mode mode if the dma_addr_t
> +	 *	type can hold 64 bit addresses.
> +	 */
> +	if (sizeof(dma_addr_t) == 8 &&
> +	    pci_set_dma_mask(pDev, DMA_64BIT_MASK) == 0) {

Identation with tabs?

> +		dma64 = 1;
> +	} else {
> +		if (pci_set_dma_mask(pDev, DMA_32BIT_MASK) != 0)
> +			return -EINVAL;
> +	}
> +
> +	/* Make sure pci_alloc_consistent returns 32 bit addresses */
> +	pci_set_consistent_dma_mask(pDev, DMA_32BIT_MASK);
>
>  	base_addr0_phys = pci_resource_start(pDev,0);
>  	hba_map0_area_size = pci_resource_len(pDev,0);

I don't think that this comment is neccessary, it's the only purpose of this 
function. If you want to comment something better write down something 
like "adapter does only support message blocks below 4GB".

> @@ -929,6 +1039,19 @@
>  		raptorFlag = TRUE;
>  	}
>
> +#if BITS_PER_LONG == 64
> +	/* x86_64 machines need more optimal mappings */
> +	if (raptorFlag == TRUE) {
> +		if (hba_map0_area_size > 128)
> +			hba_map0_area_size = 128;
> +		if (hba_map1_area_size > 524288)
> +			hba_map1_area_size = 524288;
> +	} else {
> +		if (hba_map0_area_size > 524288)
> +			hba_map0_area_size = 524288;
> +	}
> +#endif
> +
>  	base_addr_virt = ioremap(base_addr0_phys,hba_map0_area_size);
>  	if (!base_addr_virt) {
>  		pci_release_regions(pDev);

This is very very personal taste, but I would write "512 * 1024" so it's 
absolutely clear what this number is.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]