[PATCH] aacraid: cleanup and version stamp driver

[PATCH] aacraid: cleanup and version stamp driver

[Salyzyn, Mark]

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

There is some residual cleanup of the last series of patches and the
need to bump the revision number to draw the line in the sand.

The cmd->SCp.phase is set in the aac_valid_context routine, then set
again to the same value following it's return. The cmd->scsi_done is set
twice in the aac_queuecommand routine. Free up the scsidev FILO in
aac_probe_container as it is not needed further down the function in any
case. Improve the efficiency of the abort handler kernel print
parameters. Bump revision number of driver to approximate the equivalent
in the Adaptec supplied version.

This attached patch is against current scsi-misc-2.6

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

---

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_cleanup_and_version.patch --]
[-- Type: application/octet-stream, Size: 2795 bytes --]

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c	2007-03-21 13:23:19.345260702 -0400
+++ b/drivers/scsi/aacraid/aachba.c	2007-03-21 13:37:32.995408010 -0400
@@ -365,7 +365,6 @@
 	struct scsi_cmnd * scsicmd;
 
 	scsicmd = (struct scsi_cmnd *) context;
-	scsicmd->SCp.phase = AAC_OWNER_MIDLEVEL;
 
 	if (!aac_valid_context(scsicmd, fibptr))
 		return;
@@ -629,9 +628,9 @@
 	if (_aac_probe_container(scsicmd, aac_probe_container_callback1) == 0)
 		while (scsicmd->device == scsidev)
 			schedule();
+	kfree(scsidev);
 	status = scsicmd->SCp.Status;
 	kfree(scsicmd);
-	kfree(scsidev);
 	return status;
 }
 
@@ -1321,7 +1320,6 @@
 	u32 cid;
 
 	scsicmd = (struct scsi_cmnd *) context;
-	scsicmd->SCp.phase = AAC_OWNER_MIDLEVEL;
 
 	if (!aac_valid_context(scsicmd, fibptr))
 		return;
@@ -1571,7 +1569,6 @@
 	struct scsi_cmnd *cmd;
 
 	cmd = context;
-	cmd->SCp.phase = AAC_OWNER_MIDLEVEL;
 
 	if (!aac_valid_context(cmd, fibptr))
 		return;
@@ -2127,7 +2124,6 @@
 	struct scsi_cmnd *scsicmd;
 
 	scsicmd = (struct scsi_cmnd *) context;
-	scsicmd->SCp.phase = AAC_OWNER_MIDLEVEL;
 
 	if (!aac_valid_context(scsicmd, fibptr))
 		return;
diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h	2007-03-21 13:23:19.346260576 -0400
+++ b/drivers/scsi/aacraid/aacraid.h	2007-03-21 13:38:04.094421971 -0400
@@ -12,8 +12,8 @@
  *----------------------------------------------------------------------------*/
 
 #ifndef AAC_DRIVER_BUILD
-# define AAC_DRIVER_BUILD 2423
-# define AAC_DRIVER_BRANCH "-mh3"
+# define AAC_DRIVER_BUILD 2437
+# define AAC_DRIVER_BRANCH "-mh4"
 #endif
 #define MAXIMUM_NUM_CONTAINERS	32
 
diff -ru a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
--- a/drivers/scsi/aacraid/linit.c	2007-03-21 13:23:19.347260450 -0400
+++ b/drivers/scsi/aacraid/linit.c	2007-03-21 13:38:04.095421842 -0400
@@ -260,7 +260,6 @@
 		    (cmd->SCp.phase == AAC_OWNER_FIRMWARE))
 			return 0; /* Already owned by Adapter */
 	}
-	cmd->scsi_done = done;
 	cmd->SCp.phase = AAC_OWNER_LOWLEVEL;
 	return (aac_scsi_cmd(cmd) ? FAILED : 0);
 } 
@@ -461,15 +460,15 @@
 
 static int aac_eh_abort(struct scsi_cmnd* cmd)
 {
-	struct Scsi_Host * host = cmd->device->host;
+	struct scsi_device * dev = cmd->device;
+	struct Scsi_Host * host = dev->host;
 	struct aac_dev * aac = (struct aac_dev *)host->hostdata;
 	int count;
 	int ret = FAILED;
 
 	printk(KERN_ERR "%s: Host adapter abort request (%d,%d,%d,%d)\n",
 		AAC_DRIVERNAME,
-		cmd->device->host->host_no, sdev_channel(cmd->device),
-		sdev_id(cmd->device), cmd->device->lun);
+		host->host_no, sdev_channel(dev), sdev_id(dev), dev->lun);
 	switch (cmd->cmnd[0]) {
 	case SERVICE_ACTION_IN:
 		if (!(aac->raw_io_interface) ||

Re: [PATCH] aacraid: cleanup and version stamp driver

[Boaz Harrosh]

Salyzyn, Mark wrote:
> There is some residual cleanup of the last series of patches and the
> need to bump the revision number to draw the line in the sand.
> 
> The cmd->SCp.phase is set in the aac_valid_context routine, then set
> again to the same value following it's return. The cmd->scsi_done is set
> twice in the aac_queuecommand routine. Free up the scsidev FILO in
> aac_probe_container as it is not needed further down the function in any
> case. Improve the efficiency of the abort handler kernel print
> parameters. Bump revision number of driver to approximate the equivalent
> in the Adaptec supplied version.
> 
> This attached patch is against current scsi-misc-2.6
> 
> Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>
> 
> ---
> 
> Sincerely -- Mark Salyzyn

looking in mail source

> This attached patch is against current scsi-misc-2.6
> 
> Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>
> 
> ---
> 
> Sincerely -- Mark Salyzyn
> 
> ------_=_NextPart_001_01C76BE1.51493AEC
> Content-Type: application/octet-stream;
> 	name="aacraid_cleanup_and_version.patch"
> Content-Transfer-Encoding: base64
> Content-Description: aacraid_cleanup_and_version.patch
> Content-Disposition: attachment;
> 	filename="aacraid_cleanup_and_version.patch"
> 
> ZGlmZiAtcnUgYS9kcml2ZXJzL3Njc2kvYWFjcmFpZC9hYWNoYmEuYyBiL2RyaXZlcnMvc2NzaS9h
> YWNyYWlkL2FhY2hiYS5jCi0tLSBhL2RyaXZlcnMvc2NzaS9hYWNyYWlkL2FhY2hiYS5jCTIwMDct
> MDMtMjEgMTM6MjM6MTkuMzQ1MjYwNzAyIC0wNDAwCisrKyBiL2RyaXZlcnMvc2NzaS9hYWNyYWlk
> L2FhY2hiYS5jCTIwMDctMDMtMjEgMTM6Mzc6MzIuOTk1NDA4MDEwIC0wNDAwCkBAIC0zNjUsNyAr
> MzY1LDYgQEAKIAlzdHJ1Y3Qgc2NzaV9jbW5kICogc2NzaWNtZDsKIAogCXNjc2ljbWQgPSAoc3Ry

why is the text attachment marked "Content-Type: application/octet-stream"
and encoded? attachments are OK but why not plain text?

Boaz

RE: [PATCH] aacraid: cleanup and version stamp driver

[Salyzyn, Mark]

Because Adaptec Policy requires me to send all email through Outlook ...

Outlook has demonstrated that it will change text, either as inlined or
as an attachment, into something that is not a functional patch. Years
of sending patches to Mark Haverkamp have tuned me to send them this way
to ensure accuracy of delivery. I understand that my procedures may need
to be changed, waivers requested perhaps, as I issue the patches
directly to the list now that OSDL has had to deal with reductions in
finances and force and I will take your comment to heart.

Sorry for any inconvenience when trying to view the content of the
patch.

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Boaz Harrosh [mailto:bharrosh@panasas.com] 
> Sent: Wednesday, March 21, 2007 2:50 PM
> To: Salyzyn, Mark
> Cc: linux-scsi
> Subject: Re: [PATCH] aacraid: cleanup and version stamp driver
> 
> 
> Salyzyn, Mark wrote:
> > There is some residual cleanup of the last series of patches and the
> > need to bump the revision number to draw the line in the sand.
> > 
> > The cmd->SCp.phase is set in the aac_valid_context routine, then set
> > again to the same value following it's return. The 
> cmd->scsi_done is set
> > twice in the aac_queuecommand routine. Free up the scsidev FILO in
> > aac_probe_container as it is not needed further down the 
> function in any
> > case. Improve the efficiency of the abort handler kernel print
> > parameters. Bump revision number of driver to approximate 
> the equivalent
> > in the Adaptec supplied version.
> > 
> > This attached patch is against current scsi-misc-2.6
> > 
> > Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>
> > 
> > ---
> > 
> > Sincerely -- Mark Salyzyn
> 
> looking in mail source
> 
> > This attached patch is against current scsi-misc-2.6
> > 
> > Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>
> > 
> > ---
> > 
> > Sincerely -- Mark Salyzyn
> > 
> > ------_=_NextPart_001_01C76BE1.51493AEC
> > Content-Type: application/octet-stream;
> > 	name="aacraid_cleanup_and_version.patch"
> > Content-Transfer-Encoding: base64
> > Content-Description: aacraid_cleanup_and_version.patch
> > Content-Disposition: attachment;
> > 	filename="aacraid_cleanup_and_version.patch"
> > 
> > 
> ZGlmZiAtcnUgYS9kcml2ZXJzL3Njc2kvYWFjcmFpZC9hYWNoYmEuYyBiL2RyaX
> ZlcnMvc2NzaS9h
> > 
> YWNyYWlkL2FhY2hiYS5jCi0tLSBhL2RyaXZlcnMvc2NzaS9hYWNyYWlkL2FhY2
> hiYS5jCTIwMDct
> > 
> MDMtMjEgMTM6MjM6MTkuMzQ1MjYwNzAyIC0wNDAwCisrKyBiL2RyaXZlcnMvc2
> NzaS9hYWNyYWlk
> > 
> L2FhY2hiYS5jCTIwMDctMDMtMjEgMTM6Mzc6MzIuOTk1NDA4MDEwIC0wNDAwCk
> BAIC0zNjUsNyAr
> > 
> MzY1LDYgQEAKIAlzdHJ1Y3Qgc2NzaV9jbW5kICogc2NzaWNtZDsKIAogCXNjc2
> ljbWQgPSAoc3Ry
> 
> why is the text attachment marked "Content-Type: 
> application/octet-stream"
> and encoded? attachments are OK but why not plain text?
> 
> Boaz
> 
> 
> 

[PATCH] aacraid: Add likely() and unlikely()

[Salyzyn, Mark]

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

Add some likely() and unlikely() compiler hints in some of the aacraid
hardware interface layers. There should be no operational side effects
resulting from this patch and the changes should be mostly benign on x86
platforms.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
handling of patches attachments.

This attached patch is against current scsi-misc-2.6

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

---

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_likely_unlikely.patch --]
[-- Type: application/octet-stream, Size: 8177 bytes --]

diff -ru a//drivers/scsi/aacraid/commsup.c b//drivers/scsi/aacraid/commsup.c
--- a//drivers/scsi/aacraid/commsup.c	2007-03-21 15:15:59.470057770 -0400
+++ b//drivers/scsi/aacraid/commsup.c	2007-03-21 15:32:21.779160958 -0400
@@ -519,7 +519,7 @@
 		spin_unlock_irqrestore(&fibptr->event_lock, flags);
 		BUG_ON(fibptr->done == 0);
 			
-		if((fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
+		if(unlikely(fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
 			return -ETIMEDOUT;
 		} else {
 			return 0;
diff -ru a//drivers/scsi/aacraid/rx.c b//drivers/scsi/aacraid/rx.c
--- a//drivers/scsi/aacraid/rx.c	2007-03-21 15:15:59.472057518 -0400
+++ b//drivers/scsi/aacraid/rx.c	2007-03-21 15:32:21.780160831 -0400
@@ -5,7 +5,7 @@
  * based on the old aacraid driver that is..
  * Adaptec aacraid device driver for Linux.
  *
- * Copyright (c) 2000 Adaptec, Inc. (aacraid@adaptec.com)
+ * Copyright (c) 2000-2007 Adaptec, Inc. (aacraid@adaptec.com)
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -57,25 +57,25 @@
 	 *	been enabled.
 	 *	Check to see if this is our interrupt.  If it isn't just return
 	 */
-	if (intstat & ~(dev->OIMR)) {
+	if (likely(intstat & ~(dev->OIMR))) {
 		bellbits = rx_readl(dev, OutboundDoorbellReg);
-		if (bellbits & DoorBellPrintfReady) {
+		if (unlikely(bellbits & DoorBellPrintfReady)) {
 			aac_printf(dev, readl (&dev->IndexRegs->Mailbox[5]));
 			rx_writel(dev, MUnit.ODR,DoorBellPrintfReady);
 			rx_writel(dev, InboundDoorbellReg,DoorBellPrintfDone);
 		}
-		else if (bellbits & DoorBellAdapterNormCmdReady) {
+		else if (unlikely(bellbits & DoorBellAdapterNormCmdReady)) {
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormCmdReady);
 			aac_command_normal(&dev->queues->queue[HostNormCmdQueue]);
 		}
-		else if (bellbits & DoorBellAdapterNormRespReady) {
+		else if (likely(bellbits & DoorBellAdapterNormRespReady)) {
 			rx_writel(dev, MUnit.ODR,DoorBellAdapterNormRespReady);
 			aac_response_normal(&dev->queues->queue[HostNormRespQueue]);
 		}
-		else if (bellbits & DoorBellAdapterNormCmdNotFull) {
+		else if (unlikely(bellbits & DoorBellAdapterNormCmdNotFull)) {
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormCmdNotFull);
 		}
-		else if (bellbits & DoorBellAdapterNormRespNotFull) {
+		else if (unlikely(bellbits & DoorBellAdapterNormRespNotFull)) {
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormCmdNotFull);
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormRespNotFull);
 		}
@@ -88,11 +88,11 @@
 {
 	struct aac_dev *dev = dev_id;
 	u32 Index = rx_readl(dev, MUnit.OutboundQueue);
-	if (Index == 0xFFFFFFFFL)
+	if (unlikely(Index == 0xFFFFFFFFL))
 		Index = rx_readl(dev, MUnit.OutboundQueue);
-	if (Index != 0xFFFFFFFFL) {
+	if (likely(Index != 0xFFFFFFFFL)) {
 		do {
-			if (aac_intr_normal(dev, Index)) {
+			if (unlikely(aac_intr_normal(dev, Index))) {
 				rx_writel(dev, MUnit.OutboundQueue, Index);
 				rx_writel(dev, MUnit.ODR, DoorBellAdapterNormRespReady);
 			}
@@ -204,7 +204,7 @@
 		 */
 		msleep(1);
 	}
-	if (ok != 1) {
+	if (unlikely(ok != 1)) {
 		/*
 		 *	Restore interrupt mask even though we timed out
 		 */
@@ -214,15 +214,15 @@
 	/*
 	 *	Pull the synch status from Mailbox 0.
 	 */
-	if (status)
+	if (likely(status != NULL))
 		*status = readl(&dev->IndexRegs->Mailbox[0]);
-	if (r1)
+	if (likely(r1 != NULL))
 		*r1 = readl(&dev->IndexRegs->Mailbox[1]);
-	if (r2)
+	if (likely(r2 != NULL))
 		*r2 = readl(&dev->IndexRegs->Mailbox[2]);
-	if (r3)
+	if (likely(r3 != NULL))
 		*r3 = readl(&dev->IndexRegs->Mailbox[3]);
-	if (r4)
+	if (likely(r4 != NULL))
 		*r4 = readl(&dev->IndexRegs->Mailbox[4]);
 	/*
 	 *	Clear the synch command doorbell.
@@ -319,12 +319,12 @@
 	/*
 	 *	Check to see if the board failed any self tests.
 	 */
-	if (status & SELF_TEST_FAILED)
+	if (unlikely(status & SELF_TEST_FAILED))
 		return -1;
 	/*
 	 *	Check to see if the board panic'd.
 	 */
-	if (status & KERNEL_PANIC) {
+	if (unlikely(status & KERNEL_PANIC)) {
 		char * buffer;
 		struct POSTSTATUS {
 			__le32 Post_Command;
@@ -333,15 +333,15 @@
 		dma_addr_t paddr, baddr;
 		int ret;
 
-		if ((status & 0xFF000000L) == 0xBC000000L)
+		if (likely((status & 0xFF000000L) == 0xBC000000L))
 			return (status >> 16) & 0xFF;
 		buffer = pci_alloc_consistent(dev->pdev, 512, &baddr);
 		ret = -2;
-		if (buffer == NULL)
+		if (unlikely(buffer == NULL))
 			return ret;
 		post = pci_alloc_consistent(dev->pdev,
 		  sizeof(struct POSTSTATUS), &paddr);
-		if (post == NULL) {
+		if (unlikely(post == NULL)) {
 			pci_free_consistent(dev->pdev, 512, buffer, baddr);
 			return ret;
 		}
@@ -353,7 +353,7 @@
 		  NULL, NULL, NULL, NULL, NULL);
 		pci_free_consistent(dev->pdev, sizeof(struct POSTSTATUS),
 		  post, paddr);
-		if ((buffer[0] == '0') && ((buffer[1] == 'x') || (buffer[1] == 'X'))) {
+		if (likely((buffer[0] == '0') && ((buffer[1] == 'x') || (buffer[1] == 'X')))) {
 			ret = (buffer[2] <= '9') ? (buffer[2] - '0') : (buffer[2] - 'A' + 10);
 			ret <<= 4;
 			ret += (buffer[3] <= '9') ? (buffer[3] - '0') : (buffer[3] - 'A' + 10);
@@ -364,7 +364,7 @@
 	/*
 	 *	Wait for the adapter to be up and running.
 	 */
-	if (!(status & KERNEL_UP_AND_RUNNING))
+	if (unlikely(!(status & KERNEL_UP_AND_RUNNING)))
 		return -3;
 	/*
 	 *	Everything is OK
@@ -419,9 +419,9 @@
 	spin_unlock_irqrestore(q->lock, qflags);
 	for(;;) {
 		Index = rx_readl(dev, MUnit.InboundQueue);
-		if (Index == 0xFFFFFFFFL)
+		if (unlikely(Index == 0xFFFFFFFFL))
 			Index = rx_readl(dev, MUnit.InboundQueue);
-		if (Index != 0xFFFFFFFFL)
+		if (likely(Index != 0xFFFFFFFFL))
 			break;
 		if (--count == 0) {
 			spin_lock_irqsave(q->lock, qflags);
@@ -454,7 +454,7 @@
 		return 0;
 	}
 	dev->base = dev->regs.rx = ioremap(dev->scsi_host_ptr->base, size);
-	if (dev->base == NULL)
+	if (unlikely(dev->base == NULL))
 		return -1;
 	dev->IndexRegs = &dev->regs.rx->IndexRegs;
 	return 0;
@@ -526,13 +526,10 @@
 {
 	unsigned long start;
 	unsigned long status;
-	int instance;
-	const char * name;
+	int instance = dev->id;
+	const char * name = dev->name;
 
-	instance = dev->id;
-	name     = dev->name;
-
-	if (aac_adapter_ioremap(dev, dev->base_size)) {
+	if (unlikely(aac_adapter_ioremap(dev, dev->base_size))) {
 		printk(KERN_WARNING "%s: unable to map adapter.\n", name);
 		goto error_iounmap;
 	}
@@ -541,7 +538,7 @@
 	 *	Check to see if the board panic'd while booting.
 	 */
 	status = rx_readl(dev, MUnit.OMRx[0]);
-	if (status & KERNEL_PANIC) {
+	if (unlikely(status & KERNEL_PANIC)) {
 		if ((status = aac_rx_check_health(dev)) <= 0)
 			goto error_iounmap;
 		if (aac_rx_restart_adapter(dev, status))
@@ -551,14 +548,14 @@
 	 *	Check to see if the board failed any self tests.
 	 */
 	status = rx_readl(dev, MUnit.OMRx[0]);
-	if (status & SELF_TEST_FAILED) {
+	if (unlikely(status & SELF_TEST_FAILED)) {
 		printk(KERN_ERR "%s%d: adapter self-test failed.\n", dev->name, instance);
 		goto error_iounmap;
 	}
 	/*
 	 *	Check to see if the monitor panic'd while booting.
 	 */
-	if (status & MONITOR_PANIC) {
+	if (unlikely(status & MONITOR_PANIC)) {
 		printk(KERN_ERR "%s%d: adapter monitor panic.\n", dev->name, instance);
 		goto error_iounmap;
 	}
@@ -568,8 +565,7 @@
 	 */
 	while (!((status = rx_readl(dev, MUnit.OMRx[0])) & KERNEL_UP_AND_RUNNING))
 	{
-		if(time_after(jiffies, start+startup_timeout*HZ))
-		{
+		if(unlikely(time_after(jiffies, start+startup_timeout*HZ))) {
 			printk(KERN_ERR "%s%d: adapter kernel failed to start, init status = %lx.\n", 
 					dev->name, instance, status);
 			goto error_iounmap;
@@ -595,11 +591,11 @@
 	rx_writel(dev, MUnit.ODR, 0xffffffff);
 	aac_adapter_enable_int(dev);
 
-	if (aac_init_adapter(dev) == NULL)
+	if (unlikely(aac_init_adapter(dev) == NULL))
 		goto error_iounmap;
 	aac_adapter_comm(dev, dev->comm_interface);
-	if (request_irq(dev->scsi_host_ptr->irq, dev->a_ops.adapter_intr,
-			IRQF_SHARED|IRQF_DISABLED, "aacraid", dev) < 0) {
+	if (unlikely(request_irq(dev->scsi_host_ptr->irq, dev->a_ops.adapter_intr,
+			IRQF_SHARED|IRQF_DISABLED, "aacraid", dev) < 0)) {
 		printk(KERN_ERR "%s%d: Interrupt unavailable.\n",
 			name, instance);
 		goto error_iounmap;

Re: [PATCH] aacraid: Add likely() and unlikely()

[Arjan van de Ven]

On Wed, 2007-03-21 at 15:43 -0400, Salyzyn, Mark wrote:
> Add some likely() and unlikely() compiler hints in some of the aacraid
> hardware interface layers. There should be no operational side effects
> resulting from this patch and the changes should be mostly benign on x86
> platforms.
> 
> ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
> handling of patches attachments.
> 
> This attached patch is against current scsi-misc-2.6

-               if((fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
+               if(unlikely(fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
                        return -ETIMEDOUT;
                } else {
                        return 0;

while you're at it, please remove the extra {}'s as well

(I assume you're aware that likely/unlikely should only be used for 99:1
or higher ratios, this one looks correct for sure)

Also I suggest you don't add about half of these; they appear init time
only code, at which point adding likely/unlikely is just code noise that
clutters the driver...
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

[PATCH] aacraid: Add likely() and unlikely() Take 2

[Salyzyn, Mark]

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

Thanks Arjan for the comments, much appreciated. I have removed the
superfluous braces and limited the usage of the likely and unlikely to
runtime code. Sorry for the delay, took a four day vacation ...

Add some likely() and unlikely() compiler hints in some of the aacraid
hardware interface layers. There should be no operational side effects
resulting from this patch and the changes should be mostly benign on x86
platforms.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
handling of patches attachments.

This attached patch is against current scsi-misc-2.6

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

---

Sincerely -- Mark Salyzyn

> -----Original Message-----
> From: Arjan van de Ven [mailto:arjan@infradead.org] 
> Sent: Thursday, March 22, 2007 5:24 AM
> To: Salyzyn, Mark
> Cc: linux-scsi
> Subject: Re: [PATCH] aacraid: Add likely() and unlikely()
> 
> On Wed, 2007-03-21 at 15:43 -0400, Salyzyn, Mark wrote:
> 
> -               if((fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
> +               if(unlikely(fibptr->flags &
FIB_CONTEXT_FLAG_TIMED_OUT)){
>                         return -ETIMEDOUT;
>                 } else {
>                         return 0;
> 
> while you're at it, please remove the extra {}'s as well
> 
> (I assume you're aware that likely/unlikely should only be used for
99:1 or higher ratios, this one looks correct for sure)
> 
> Also I suggest you don't add about half of these; they appear init
time only code, at which point adding likely/unlikely is just code noise
that clutters the driver...

[-- Attachment #2: aacraid_likely_unlikely_2.patch --]
[-- Type: application/octet-stream, Size: 5934 bytes --]

diff -ru a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
--- a/drivers/scsi/aacraid/commsup.c	2007-03-26 09:06:32.073857134 -0400
+++ b/drivers/scsi/aacraid/commsup.c	2007-03-26 09:09:43.453622817 -0400
@@ -519,11 +519,9 @@
 		spin_unlock_irqrestore(&fibptr->event_lock, flags);
 		BUG_ON(fibptr->done == 0);
 			
-		if((fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
+		if(unlikely(fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT))
 			return -ETIMEDOUT;
-		} else {
-			return 0;
-		}
+		return 0;
 	}
 	/*
 	 *	If the user does not want a response than return success otherwise
diff -ru a/drivers/scsi/aacraid/rx.c b/drivers/scsi/aacraid/rx.c
--- a/drivers/scsi/aacraid/rx.c	2007-03-26 09:06:33.807636560 -0400
+++ b/drivers/scsi/aacraid/rx.c	2007-03-26 09:13:34.528425867 -0400
@@ -5,7 +5,7 @@
  * based on the old aacraid driver that is..
  * Adaptec aacraid device driver for Linux.
  *
- * Copyright (c) 2000 Adaptec, Inc. (aacraid@adaptec.com)
+ * Copyright (c) 2000-2007 Adaptec, Inc. (aacraid@adaptec.com)
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -57,25 +57,25 @@
 	 *	been enabled.
 	 *	Check to see if this is our interrupt.  If it isn't just return
 	 */
-	if (intstat & ~(dev->OIMR)) {
+	if (likely(intstat & ~(dev->OIMR))) {
 		bellbits = rx_readl(dev, OutboundDoorbellReg);
-		if (bellbits & DoorBellPrintfReady) {
+		if (unlikely(bellbits & DoorBellPrintfReady)) {
 			aac_printf(dev, readl (&dev->IndexRegs->Mailbox[5]));
 			rx_writel(dev, MUnit.ODR,DoorBellPrintfReady);
 			rx_writel(dev, InboundDoorbellReg,DoorBellPrintfDone);
 		}
-		else if (bellbits & DoorBellAdapterNormCmdReady) {
+		else if (unlikely(bellbits & DoorBellAdapterNormCmdReady)) {
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormCmdReady);
 			aac_command_normal(&dev->queues->queue[HostNormCmdQueue]);
 		}
-		else if (bellbits & DoorBellAdapterNormRespReady) {
+		else if (likely(bellbits & DoorBellAdapterNormRespReady)) {
 			rx_writel(dev, MUnit.ODR,DoorBellAdapterNormRespReady);
 			aac_response_normal(&dev->queues->queue[HostNormRespQueue]);
 		}
-		else if (bellbits & DoorBellAdapterNormCmdNotFull) {
+		else if (unlikely(bellbits & DoorBellAdapterNormCmdNotFull)) {
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormCmdNotFull);
 		}
-		else if (bellbits & DoorBellAdapterNormRespNotFull) {
+		else if (unlikely(bellbits & DoorBellAdapterNormRespNotFull)) {
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormCmdNotFull);
 			rx_writel(dev, MUnit.ODR, DoorBellAdapterNormRespNotFull);
 		}
@@ -88,11 +88,11 @@
 {
 	struct aac_dev *dev = dev_id;
 	u32 Index = rx_readl(dev, MUnit.OutboundQueue);
-	if (Index == 0xFFFFFFFFL)
+	if (unlikely(Index == 0xFFFFFFFFL))
 		Index = rx_readl(dev, MUnit.OutboundQueue);
-	if (Index != 0xFFFFFFFFL) {
+	if (likely(Index != 0xFFFFFFFFL)) {
 		do {
-			if (aac_intr_normal(dev, Index)) {
+			if (unlikely(aac_intr_normal(dev, Index))) {
 				rx_writel(dev, MUnit.OutboundQueue, Index);
 				rx_writel(dev, MUnit.ODR, DoorBellAdapterNormRespReady);
 			}
@@ -204,7 +204,7 @@
 		 */
 		msleep(1);
 	}
-	if (ok != 1) {
+	if (unlikely(ok != 1)) {
 		/*
 		 *	Restore interrupt mask even though we timed out
 		 */
@@ -319,12 +319,12 @@
 	/*
 	 *	Check to see if the board failed any self tests.
 	 */
-	if (status & SELF_TEST_FAILED)
+	if (unlikely(status & SELF_TEST_FAILED))
 		return -1;
 	/*
 	 *	Check to see if the board panic'd.
 	 */
-	if (status & KERNEL_PANIC) {
+	if (unlikely(status & KERNEL_PANIC)) {
 		char * buffer;
 		struct POSTSTATUS {
 			__le32 Post_Command;
@@ -333,15 +333,15 @@
 		dma_addr_t paddr, baddr;
 		int ret;
 
-		if ((status & 0xFF000000L) == 0xBC000000L)
+		if (likely((status & 0xFF000000L) == 0xBC000000L))
 			return (status >> 16) & 0xFF;
 		buffer = pci_alloc_consistent(dev->pdev, 512, &baddr);
 		ret = -2;
-		if (buffer == NULL)
+		if (unlikely(buffer == NULL))
 			return ret;
 		post = pci_alloc_consistent(dev->pdev,
 		  sizeof(struct POSTSTATUS), &paddr);
-		if (post == NULL) {
+		if (unlikely(post == NULL)) {
 			pci_free_consistent(dev->pdev, 512, buffer, baddr);
 			return ret;
 		}
@@ -353,7 +353,7 @@
 		  NULL, NULL, NULL, NULL, NULL);
 		pci_free_consistent(dev->pdev, sizeof(struct POSTSTATUS),
 		  post, paddr);
-		if ((buffer[0] == '0') && ((buffer[1] == 'x') || (buffer[1] == 'X'))) {
+		if (likely((buffer[0] == '0') && ((buffer[1] == 'x') || (buffer[1] == 'X')))) {
 			ret = (buffer[2] <= '9') ? (buffer[2] - '0') : (buffer[2] - 'A' + 10);
 			ret <<= 4;
 			ret += (buffer[3] <= '9') ? (buffer[3] - '0') : (buffer[3] - 'A' + 10);
@@ -364,7 +364,7 @@
 	/*
 	 *	Wait for the adapter to be up and running.
 	 */
-	if (!(status & KERNEL_UP_AND_RUNNING))
+	if (unlikely(!(status & KERNEL_UP_AND_RUNNING)))
 		return -3;
 	/*
 	 *	Everything is OK
@@ -419,9 +419,9 @@
 	spin_unlock_irqrestore(q->lock, qflags);
 	for(;;) {
 		Index = rx_readl(dev, MUnit.InboundQueue);
-		if (Index == 0xFFFFFFFFL)
+		if (unlikely(Index == 0xFFFFFFFFL))
 			Index = rx_readl(dev, MUnit.InboundQueue);
-		if (Index != 0xFFFFFFFFL)
+		if (likely(Index != 0xFFFFFFFFL))
 			break;
 		if (--count == 0) {
 			spin_lock_irqsave(q->lock, qflags);
@@ -526,11 +526,8 @@
 {
 	unsigned long start;
 	unsigned long status;
-	int instance;
-	const char * name;
-
-	instance = dev->id;
-	name     = dev->name;
+	int instance = dev->id;
+	const char * name = dev->name;
 
 	if (aac_adapter_ioremap(dev, dev->base_size)) {
 		printk(KERN_WARNING "%s: unable to map adapter.\n", name);
@@ -568,8 +565,7 @@
 	 */
 	while (!((status = rx_readl(dev, MUnit.OMRx[0])) & KERNEL_UP_AND_RUNNING))
 	{
-		if(time_after(jiffies, start+startup_timeout*HZ))
-		{
+		if(time_after(jiffies, start+startup_timeout*HZ)) {
 			printk(KERN_ERR "%s%d: adapter kernel failed to start, init status = %lx.\n", 
 					dev->name, instance, status);
 			goto error_iounmap;

[PATCH] aacraid: Add SMC and SUN products to README

[Salyzyn, Mark]

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

Add SMC and SUN products to aacraid documentation

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
handling of patches attachments.

This attached patch is against current scsi-misc-2.6

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

---

Sincerely -- Mark Salyzyn

[-- Attachment #2: aacraid_SUN_SMC_documentation.patch --]
[-- Type: application/octet-stream, Size: 1106 bytes --]

diff -ru a/Documentation/scsi/aacraid.txt b/Documentation/scsi/aacraid.txt
--- a/Documentation/scsi/aacraid.txt	2007-03-27 11:35:23.587692508 -0400
+++ b/Documentation/scsi/aacraid.txt	2007-03-27 11:46:46.793782677 -0400
@@ -37,7 +37,11 @@
 	9005:0286:9005:029d	Adaptec	2420SA (Intruder HP release)
 	9005:0286:9005:02ac	Adaptec	1800 (Typhoon44)
 	9005:0285:9005:02b5	Adaptec	5445 (Voodoo44)
+	9005:0285:15d9:02b5	SMC	AOC-USAS-S4i
+	9005:0285:15d9:02c9	SMC	AOC-USAS-S4iB
 	9005:0285:9005:02b6	Adaptec	5805 (Voodoo80)
+	9005:0285:15d9:02b6	SMC	AOC-USAS-S8i
+	9005:0285:15d9:02ca	SMC	AOC-USAS-S8iB
 	9005:0285:9005:02b7	Adaptec	5085 (Voodoo08)
 	9005:0285:9005:02bb	Adaptec	3405 (Marauder40LP)
 	9005:0285:9005:02bc	Adaptec	3805 (Marauder80LP)
@@ -93,6 +97,9 @@
 	9005:0286:9005:02ae		(Aurora Lite ARK)
 	9005:0285:9005:02b0		(Sunrise Lake ARK)
 	9005:0285:9005:02b1	Adaptec	(Voodoo 8 internal 8 external)
+	9005:0285:108e:7aac	SUN	STK RAID REM (Voodoo44 Coyote)
+	9005:0285:108e:0286	SUN	SG-XPCIESAS-R-IN (Cougar)
+	9005:0285:108e:0287	SUN	SG-XPCIESAS-R-EX (Prometheus)
 
 People
 -------------------------

[PATCH] aacraid: resolve compiler warnings using ptrdiff_t

[Salyzyn, Mark]

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

Unsigned long is not always the same size as a pointer, namely on 32 bit
systems with 64 bit address space. Ptrdiff_t is the same size as a
pointer in all configurations. By using ptrdiff_t we can mitigate the
warning messages on these configurations. There should be no side
effects of this patch.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
handling of patches attachments.

This attached patch is against current scsi-misc-2.6

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

[-- Attachment #2: aacraid_ptrdiff.patch --]
[-- Type: application/octet-stream, Size: 2073 bytes --]

diff -ru a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
--- a/drivers/scsi/aacraid/commctrl.c	2007-03-27 14:51:11.670124719 -0400
+++ b/drivers/scsi/aacraid/commctrl.c	2007-03-27 15:00:53.286882719 -0400
@@ -391,10 +391,8 @@
 		/*
 		 *	Extract the fibctx from the input parameters
 		 */
-		if (fibctx->unique == (u32)(unsigned long)arg) {   
-			/* We found a winner */
+		if (fibctx->unique == (u32)(ptrdiff_t)arg) /* We found a winner */
 			break;
-		}
 		entry = entry->next;
 		fibctx = NULL;
 	}
Only in b: commctrl.c.orig
diff -ru a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
--- a/drivers/scsi/aacraid/comminit.c	2007-03-27 14:51:11.670124719 -0400
+++ b/drivers/scsi/aacraid/comminit.c	2007-03-27 14:59:35.019740844 -0400
@@ -5,7 +5,7 @@
  * based on the old aacraid driver that is..
  * Adaptec aacraid device driver for Linux.
  *
- * Copyright (c) 2000 Adaptec, Inc. (aacraid@adaptec.com)
+ * Copyright (c) 2000-2007 Adaptec, Inc. (aacraid@adaptec.com)
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -110,7 +110,7 @@
 	/*
 	 *	Align the beginning of Headers to commalign
 	 */
-	align = (commalign - ((unsigned long)(base) & (commalign - 1)));
+	align = (commalign - ((ptrdiff_t)(base) & (commalign - 1)));
 	base = base + align;
 	phys = phys + align;
 	/*
diff -ru a/drivers/scsi/aacraid/dpcsup.c b/drivers/scsi/aacraid/dpcsup.c
--- a/drivers/scsi/aacraid/dpcsup.c	2007-03-27 14:51:11.671124593 -0400
+++ b/drivers/scsi/aacraid/dpcsup.c	2007-03-27 14:59:35.019740844 -0400
@@ -256,7 +256,8 @@
 			return 1;
 		}
 		memset(hw_fib, 0, sizeof(struct hw_fib));
-		memcpy(hw_fib, (struct hw_fib *)(((unsigned long)(dev->regs.sa)) + (index & ~0x00000002L)), sizeof(struct hw_fib));
+		memcpy(hw_fib, (struct hw_fib *)(((ptrdiff_t)(dev->regs.sa)) +
+		  (index & ~0x00000002L)), sizeof(struct hw_fib));
 		memset(fib, 0, sizeof(struct fib));
 		INIT_LIST_HEAD(&fib->fiblink);
 		fib->type = FSAFS_NTC_FIB_CONTEXT;

[PATCH] aacraid: remove unused or deprecated firmware constants

[Salyzyn, Mark]

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

Just sweeping the floor clean in one spot. Some of these constants have
never been used in the driver or in the firmware (and thus are
meaningless). Triggered this patch because I discovered one of the
unused constants was actually incorrect and figured it was better to
clean them out than correct and update. There are no side effects at all
regarding this patch, it is purely cosmetic.

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
handling of patches.

This attached patch is against current scsi-misc-2.6

Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

[-- Attachment #2: aacraid_unused_firmware_constants.patch --]
[-- Type: application/octet-stream, Size: 2408 bytes --]

diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h	2007-03-28 08:29:40.028813161 -0400
+++ b/drivers/scsi/aacraid/aacraid.h	2007-03-28 09:09:31.131800825 -0400
@@ -48,49 +48,13 @@
 
 
 /*
- *	DON'T CHANGE THE ORDER, this is set by the firmware
+ *	Firmware constants
  */
  
 #define		CT_NONE			0
-#define		CT_VOLUME		1
-#define		CT_MIRROR		2
-#define		CT_STRIPE		3
-#define		CT_RAID5		4
-#define		CT_SSRW			5
-#define		CT_SSRO			6
-#define		CT_MORPH		7
-#define		CT_PASSTHRU		8
-#define		CT_RAID4		9
-#define		CT_RAID10		10	/* stripe of mirror */
-#define		CT_RAID00		11	/* stripe of stripe */
-#define		CT_VOLUME_OF_MIRRORS	12	/* volume of mirror */
-#define		CT_PSEUDO_RAID		13	/* really raid4 */
-#define		CT_LAST_VOLUME_TYPE	14
 #define 	CT_OK        		218
-
-/*
- *	Types of objects addressable in some fashion by the client.
- *	This is a superset of those objects handled just by the filesystem
- *	and includes "raw" objects that an administrator would use to
- *	configure containers and filesystems.
- */
-
-#define		FT_REG		1	/* regular file */
-#define		FT_DIR		2	/* directory */
-#define		FT_BLK		3	/* "block" device - reserved */
-#define		FT_CHR		4	/* "character special" device - reserved */
-#define		FT_LNK		5	/* symbolic link */
-#define		FT_SOCK		6	/* socket */
-#define		FT_FIFO		7	/* fifo */
 #define		FT_FILESYS	8	/* ADAPTEC's "FSA"(tm) filesystem */
 #define		FT_DRIVE	9	/* physical disk - addressable in scsi by bus/id/lun */
-#define		FT_SLICE	10	/* virtual disk - raw volume - slice */
-#define		FT_PARTITION	11	/* FSA partition - carved out of a slice - building block for containers */
-#define		FT_VOLUME	12	/* Container - Volume Set */
-#define		FT_STRIPE	13	/* Container - Stripe Set */
-#define		FT_MIRROR	14	/* Container - Mirror Set */
-#define		FT_RAID5	15	/* Container - Raid 5 Set */
-#define		FT_DATABASE	16	/* Storage object with "foreign" content manager */
 
 /*
  *	Host side memory scatter gather list
@@ -1519,8 +1483,7 @@
 	struct creation_info	create_info;	/* if applicable */
 	__le32			capacity;
 	__le32			vol;    	/* substrate structure */
-	__le32			obj;	        /* FT_FILESYS, 
-						   FT_DATABASE, etc. */
+	__le32			obj;	        /* FT_FILESYS, etc. */ 
 	__le32			state;		/* unready for mounting, 
 						   readonly, etc. */
 	union aac_contentinfo	fileinfo;	/* Info specific to content 

[PATCH] aacraid: fix print of Firmware Build Date and add TSID

[Salyzyn, Mark]

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

The Adapter build date that is to be printed on instantiation was not
displayed as a result of the supplemental adapter information structure
not being in sync with the Firmware; the driver took an early test cycle
version that had a miss-sized padded region at the head and the
structure was not re-checked at the end of qualification. The Build Date
was not a priority and is merely a cosmetic enhancement, and the wrong
location for the start of the structure member would not induce any
side-effect problems. We updated the structure to match the actual
format, and added the TSID (Tech Support Identification) value print,
should it be present, to the adapter instantiation announcements during
driver load.

This later enhancement should improve the relationship between Service
folk & Tech Support if the printed value of the TSID found it's way into
the circular file labeled G...

Neither of these values show in sysfs (yet).

ObligatoryDisclaimer: Please accept my condolences regarding Outlook's
handling of patches.

This attached patch is against current scsi-misc-2.6
 
Signed-off-by: Mark Salyzyn <aacraid@adaptec.com>

[-- Attachment #2: aacraid_build_date_tsid.patch --]
[-- Type: application/octet-stream, Size: 1511 bytes --]

diff -ru a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
--- a/drivers/scsi/aacraid/aachba.c	2007-03-28 09:23:37.990745437 -0400
+++ b/drivers/scsi/aacraid/aachba.c	2007-03-28 13:26:10.036377528 -0400
@@ -1194,6 +1194,12 @@
 			printk(KERN_INFO "%s%d: serial %x\n",
 				dev->name, dev->id,
 				le32_to_cpu(dev->adapter_info.serial[0]));
+		if (dev->supplement_adapter_info.VpdInfo.Tsid[0]) {
+			printk(KERN_INFO "%s%d: TSID %.*s\n",
+			  dev->name, dev->id,
+			  (int)sizeof(dev->supplement_adapter_info.VpdInfo.Tsid),
+			  dev->supplement_adapter_info.VpdInfo.Tsid);
+		}
 	}
 
 	dev->nondasd_support = 0;
diff -ru a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
--- a/drivers/scsi/aacraid/aacraid.h	2007-03-28 09:23:37.991745310 -0400
+++ b/drivers/scsi/aacraid/aacraid.h	2007-03-28 13:26:10.061374374 -0400
@@ -843,10 +843,25 @@
 	__le32	Version;
 	__le32	FeatureBits;
 	u8	SlotNumber;
-	u8	ReservedPad0[0];
+	u8	ReservedPad0[3];
 	u8	BuildDate[12];
 	__le32	CurrentNumberPorts;
-	__le32	ReservedGrowth[24];
+	struct {
+		u8	AssemblyPn[8];
+		u8	FruPn[8];
+		u8	BatteryFruPn[8];
+		u8	EcVersionString[8];
+		u8	Tsid[12];
+	}	VpdInfo;
+	__le32	FlashFirmwareRevision;
+	__le32	FlashFirmwareBuild;
+	__le32	RaidTypeMorphOptions;
+	__le32	FlashFirmwareBootRevision;
+	__le32	FlashFirmwareBootBuild;
+	u8	MfgPcbaSerialNo[12];
+	u8	MfgWWNName[8];
+	__le32	MoreFeatureBits;
+	__le32	ReservedGrowth[1];
 };
 #define AAC_FEATURE_FALCON	0x00000010
 #define AAC_SIS_VERSION_V3	3

RE: [PATCH] aacraid: Add likely() and unlikely()

[Allexio Ju]

On Thu, 2007-03-22 at 2007 2:24 AM, Arjan van de Ven wrote:
> (I assume you're aware that likely/unlikely should only be
> used for 99:1 or higher ratios, this one looks correct for sure)
Could you share details of reasons why those macros should be used in the way?
I thought those macros simply tell compiler to layout code in such a
way that minimizes unnecessary jumps.

Thank you,

Allexio

> On Wed, 2007-03-21 at 15:43 -0400, Salyzyn, Mark wrote:
> > Add some likely() and unlikely() compiler hints in some of
> the aacraid
> > hardware interface layers. There should be no operational
> side effects
> > resulting from this patch and the changes should be mostly
> benign on
> > x86 platforms.
> >
> > ObligatoryDisclaimer: Please accept my condolences
> regarding Outlook's
> > handling of patches attachments.
> >
> > This attached patch is against current scsi-misc-2.6
>
> -               if((fibptr->flags & FIB_CONTEXT_FLAG_TIMED_OUT)){
> +               if(unlikely(fibptr->flags &
> + FIB_CONTEXT_FLAG_TIMED_OUT)){
>                         return -ETIMEDOUT;
>                 } else {
>                         return 0;
>
> while you're at it, please remove the extra {}'s as well
>
> (I assume you're aware that likely/unlikely should only be
> used for 99:1 or higher ratios, this one looks correct for sure)

RE: [PATCH] aacraid: Add likely() and unlikely()

[Arjan van de Ven]

On Tue, 2007-03-27 at 09:17 -0700, Allexio Ju wrote:
> On Thu, 2007-03-22 at 2007 2:24 AM, Arjan van de Ven wrote:
> > (I assume you're aware that likely/unlikely should only be
> > used for 99:1 or higher ratios, this one looks correct for sure)
> Could you share details of reasons why those macros should be used in the way?
> I thought those macros simply tell compiler to layout code in such a
> way that minimizes unnecessary jumps.


it's more than that. it generally also tells the processor what the
branch will be, at which point most processors disable their own branch
prediction logic. Trying to hand-layout code is almost always a
mistake... don't do that. GCC also is quite good at recognizing certain
patterns to keep the code flow working. Trying to override that only
hurts...
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: [PATCH] aacraid: Add likely() and unlikely()

[Allexio Ju]

On 3/28/07, Arjan van de Ven <arjan@infradead.org> wrote:
> On Tue, 2007-03-27 at 09:17 -0700, Allexio Ju wrote:
> > I thought those macros simply tell compiler to layout code in such a
> > way that minimizes unnecessary jumps.
> it's more than that. it generally also tells the processor what the
> branch will be, at which point most processors disable their own branch
> prediction logic. Trying to hand-layout code is almost always a
> mistake... don't do that. GCC also is quite good at recognizing certain
> patterns to keep the code flow working. Trying to override that only
> hurts...
I see... thanks for clarification.

allexio