[PATCH ver3 1/4] qla4xxx: Remove funcs with no callers in ql4_init.c

[PATCH ver3 1/4] qla4xxx: Remove funcs with no callers in ql4_init.c

[Doug Maxey]

From: Doug Maxey <dwm@enoyolf.org>

These can be added later when a caller exists.
    qla4xxx_login_device()
    qla4xxx_logout_device()
    qla4xxx_delete_device()

Signed-off-by: Doug Maxey <dwm@enoyolf.org>
---

 drivers/scsi/qla4xxx/ql4_init.c |  163 +--------------------------------------
 1 files changed, 5 insertions(+), 158 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_init.c b/drivers/scsi/qla4xxx/ql4_init.c
index 1e55e42..8608106 100644
--- a/drivers/scsi/qla4xxx/ql4_init.c
+++ b/drivers/scsi/qla4xxx/ql4_init.c
@@ -41,7 +41,7 @@ static void ql4xxx_set_mac_number(struct
 }
 
 /**
- * qla4xxx_free_ddb - deallocate ddb	
+ * qla4xxx_free_ddb - deallocate ddb
  * @ha: pointer to host adapter structure.
  * @ddb_entry: pointer to device database entry
  *
@@ -376,7 +376,7 @@ static struct ddb_entry* qla4xxx_get_ddb
  *
  * This routine updates the driver's internal device database entry
  * with information retrieved from the firmware's device database
- * entry for the specified device. The ddb_entry->fw_ddb_index field 
+ * entry for the specified device. The ddb_entry->fw_ddb_index field
  * must be initialized prior to	calling this routine
  *
  **/
@@ -494,7 +494,7 @@ struct ddb_entry * qla4xxx_alloc_ddb(str
  * @ha: Pointer to host adapter structure.
  *
  * This routine searches for all valid firmware ddb entries and builds
- * an internal ddb list. Ddbs that are considered valid are those with 
+ * an internal ddb list. Ddbs that are considered valid are those with
  * a device state of SESSION_ACTIVE.
  **/
 static int qla4xxx_build_ddb_list(struct scsi_qla_host *ha)
@@ -540,7 +540,7 @@ static int qla4xxx_build_ddb_list(struct
 				qla4xxx_set_ddb_entry(ha, fw_ddb_index, 0);
 		}
 
-		if (ddb_state != DDB_DS_SESSION_ACTIVE) 
+		if (ddb_state != DDB_DS_SESSION_ACTIVE)
 			goto next_one;
 		/*
 		 * if fw_ddb with session active state found,
@@ -1134,7 +1134,7 @@ static int qla4xxx_start_firmware(struct
  * @renew_ddb_list: Indicates what to do with the adapter's ddb list
  *	after adapter recovery has completed.
  *	0=preserve ddb list, 1=destroy and rebuild ddb list
- * 
+ *
  * This routine parforms all of the steps necessary to initialize the adapter.
  *
  **/
@@ -1340,156 +1340,3 @@ int qla4xxx_process_ddb_changed(struct s
 	return QLA_SUCCESS;
 }
 
-/**
- * qla4xxx_login_device - login to target device
- * @ha: Pointer to host adapter structure.
- * @fw_ddb_index: Index of the device to login
- * @connection_id: Connection ID of the device to login
- *
- * This routine is called by the login IOCTL to log in the specified device.
- **/
-int qla4xxx_login_device(struct scsi_qla_host *ha, uint16_t fw_ddb_index,
-			 uint16_t connection_id)
-{
-	struct ddb_entry * ddb_entry;
-	int status = QLA_ERROR;
-
-	ddb_entry = qla4xxx_lookup_ddb_by_fw_index(ha, fw_ddb_index);
-	if (ddb_entry == NULL)
-		goto exit_login_device;
-
-	if (qla4xxx_get_fwddb_entry(ha, fw_ddb_index, NULL, 0, NULL, NULL,
-				    &ddb_entry->fw_ddb_device_state, NULL,
-				    NULL, NULL) == QLA_ERROR)
-		goto exit_login_device;
-
-	if (ddb_entry->fw_ddb_device_state == DDB_DS_SESSION_ACTIVE) {
-		status = QLA_SUCCESS;
-		goto exit_login_device;
-	}
-
-	if (qla4xxx_conn_close_sess_logout(ha, fw_ddb_index, connection_id,
-					   LOGOUT_OPTION_RELOGIN)
-	    != QLA_SUCCESS)
-		goto exit_login_device;
-
-	status = QLA_SUCCESS;
-
-exit_login_device:
-	return status;
-}
-
-/**
- * qla4xxx_logout_device - logout target device
- * @ha: Pointer to host adapter structure.
- * @fw_ddb_index: Index of the device to logout
- * @connection_id: Connection ID of the device to logout
- *
- * This support routine is called by the logout IOCTL to log out
- * the specified device.
- **/
-int qla4xxx_logout_device(struct scsi_qla_host *ha, uint16_t fw_ddb_index,
-			  uint16_t connection_id)
-{
-	int status = QLA_ERROR;
-	struct ddb_entry * ddb_entry;
-	uint32_t old_fw_ddb_device_state;
-
-	ddb_entry = qla4xxx_lookup_ddb_by_fw_index(ha, fw_ddb_index);
-	if (ddb_entry == NULL)
-		goto exit_logout_device;
-
-	if (qla4xxx_get_fwddb_entry(ha, fw_ddb_index, NULL, 0, NULL, NULL,
-				    &old_fw_ddb_device_state, NULL, NULL,
-				    NULL) != QLA_SUCCESS)
-		goto exit_logout_device;
-
-	set_bit(DF_NO_RELOGIN, &ddb_entry->flags);
-	if (qla4xxx_conn_close_sess_logout(ha, fw_ddb_index, connection_id,
-					   LOGOUT_OPTION_CLOSE_SESSION)
-	    != QLA_SUCCESS)
-		goto exit_logout_device;
-
-	status = QLA_SUCCESS;
-
-exit_logout_device:
-	return status;
-}
-
-/**
- * qla4xxx_delete_device - delete specified ddb entry
- * @ha: Pointer to host adapter structure.
- * @fw_ddb_index: Index of the device to delete
- * @connection_id: Connection ID of the device to delete
- *
- * This routine is called by the logout IOCTL to delete the specified
- * device. Send the LOGOUT and DELETE_DDB commands for the specified
- * target, even if it's not in our internal database.
- **/
-int qla4xxx_delete_device(struct scsi_qla_host *ha, uint16_t fw_ddb_index,
-			  uint16_t connection_id)
-{
-	int status = QLA_ERROR;
-	uint32_t fw_ddb_device_state = 0xFFFF;
-	u_long wait_count;
-	struct ddb_entry * ddb_entry;
-
-	/* If the device is in our internal tables, set the NO_RELOGIN bit. */
-	ddb_entry = qla4xxx_lookup_ddb_by_fw_index(ha, fw_ddb_index);
-	if (ddb_entry != NULL)
-		set_bit(DF_NO_RELOGIN, &ddb_entry->flags);
-
-	/*
-	 * If the device state is already one that we can delete, bypass the
-	 * logout command.
-	 */
-	qla4xxx_get_fwddb_entry(ha, fw_ddb_index, NULL, 0, NULL, NULL,
-				&fw_ddb_device_state, NULL, NULL, NULL);
-	if (fw_ddb_device_state == DDB_DS_UNASSIGNED ||
-	    fw_ddb_device_state == DDB_DS_NO_CONNECTION_ACTIVE ||
-	    fw_ddb_device_state == DDB_DS_SESSION_FAILED)
-		goto delete_ddb;
-
-	/* First logout index */
-	if (qla4xxx_conn_close_sess_logout(ha, fw_ddb_index, connection_id,
-					   LOGOUT_OPTION_CLOSE_SESSION) !=
-	    QLA_SUCCESS) {
-		DEBUG2(printk("scsi%ld: %s: LOGOUT_OPTION_CLOSE_SESSION "
-			      "failed index [%d]\n", ha->host_no, __func__,
-			      fw_ddb_index));
-		goto exit_delete_ddb;
-	}
-
-	/* Wait enough time to complete logout */
-	wait_count = jiffies + LOGOUT_TOV * HZ;
-	while (qla4xxx_get_fwddb_entry(ha, fw_ddb_index, NULL, 0, NULL, NULL,
-				       &fw_ddb_device_state, NULL, NULL, NULL)
-	       == QLA_SUCCESS) {
-		if (time_after_eq(jiffies, wait_count))
-			goto exit_delete_ddb;
-		if (fw_ddb_device_state == DDB_DS_UNASSIGNED ||
-		    fw_ddb_device_state == DDB_DS_NO_CONNECTION_ACTIVE ||
-		    fw_ddb_device_state == DDB_DS_SESSION_FAILED)
-			break;
-		udelay(50);
-	}
-
-delete_ddb:
-	/* Now delete index */
-	if (qla4xxx_clear_database_entry(ha, fw_ddb_index) == QLA_SUCCESS) {
-		status = QLA_SUCCESS;
-
-		if (!ddb_entry)
-			goto exit_delete_ddb;
-
-		atomic_set(&ddb_entry->state, DDB_STATE_DEAD);
-		DEBUG(printk("scsi%ld: %s: removing index %d.\n", ha->host_no,
-			     __func__, fw_ddb_index));
-		ha->fw_ddb_index_map[fw_ddb_index] =
-			(struct ddb_entry *)INVALID_ENTRY;
-	}
-
-exit_delete_ddb:
-	return status;
-
-}

[PATCH ver3 2/4] qla4xxx: Add a timeout period and return status from ql4xxx_lock_drvr_wait()

[Doug Maxey]

From: Doug Maxey <dwm@enoyolf.org>

Change the func to only wait for QL4_LOCK_DRVR_WAIT ms.
Fix callers to use the return status.

Signed-off-by: Doug Maxey <dwm@enoyolf.org>
---

 drivers/scsi/qla4xxx/ql4_init.c |   26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_init.c b/drivers/scsi/qla4xxx/ql4_init.c
index 8608106..8fb39b5 100644
--- a/drivers/scsi/qla4xxx/ql4_init.c
+++ b/drivers/scsi/qla4xxx/ql4_init.c
@@ -988,25 +988,29 @@ static int qla4xxx_start_firmware_from_f
 	return status;
 }
 
-static void ql4xxx_lock_drvr_wait(struct scsi_qla_host *a)
+static int ql4xxx_lock_drvr_wait(struct scsi_qla_host *a)
 {
-	int i = 0;
-	while (1) {
+#define QL4_LOCK_DRVR_WAIT (20000)
+#define QL4_LOCK_DRVR_SLEEP (10)
+	int i = QL4_LOCK_DRVR_WAIT;
+
+	while (i > 0) {
 		if (ql4xxx_lock_drvr(a) == 0) {
-			msleep(10);
-			if (!i) {
+			msleep(QL4_LOCK_DRVR_SLEEP);
+			if (i == QL4_LOCK_DRVR_WAIT) {
 				DEBUG2(printk("scsi%ld: %s: Waiting for "
 					      "Global Init Semaphore...n",
 					      a->host_no,
 					      __func__));
-				i++;
 			}
+			i -= QL4_LOCK_DRVR_SLEEP;
 		} else {
 			DEBUG2(printk("scsi%ld: %s: Global Init Semaphore "
-				      "acquired.n", a->host_no, __func__));
-			break;
+				      "acquired\n", a->host_no, __func__));
+			return QLA_SUCCESS;
 		}
 	}
+	return QLA_ERROR;
 }
 
 /**
@@ -1030,7 +1034,8 @@ static int qla4xxx_start_firmware(struct
 	if (is_qla4022(ha))
 		ql4xxx_set_mac_number(ha);
 
-	ql4xxx_lock_drvr_wait(ha);
+	if (ql4xxx_lock_drvr_wait(ha))
+		return QLA_ERROR;
 
 	spin_lock_irqsave(&ha->hardware_lock, flags);
 
@@ -1107,7 +1112,8 @@ static int qla4xxx_start_firmware(struct
 		config_chip = 1;
 
 		/* Reset clears the semaphore, so aquire again */
-		ql4xxx_lock_drvr_wait(ha);
+		if (ql4xxx_lock_drvr_wait(ha))
+			return QLA_ERROR;
 	}
 
 	if (config_chip) {

[PATCH ver3 3/4] qla4xxx: use dev_xxx on some pci/dma resource alloc warning and error printks

[Doug Maxey]

From: Doug Maxey <dwm@enoyolf.org>

Rework some of the printk's in the pci and dma resource allocations to
- use the appropriate dev_xxx macro.
- be non conditional

Signed-off-by: Doug Maxey <dwm@enoyolf.org>
---

 drivers/scsi/qla4xxx/ql4_os.c |   46 +++++++++++++++--------------------------
 1 files changed, 17 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index 1da7cf8..d88c84f 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -688,7 +688,7 @@ static void qla4xxx_free_adapter(struct 
  * @ha: pointer to adapter structure
  *
  * This routines maps HBA's registers from the pci address space
- * into the kernel virtual address space for memory mapped i/o. 
+ * into the kernel virtual address space for memory mapped i/o.
  **/
 static int qla4xxx_iospace_config(struct scsi_qla_host *ha)
 {
@@ -700,15 +700,12 @@ static int qla4xxx_iospace_config(struct
 	pio_flags = pci_resource_flags(ha->pdev, 0);
 	if (pio_flags & IORESOURCE_IO) {
 		if (pio_len < MIN_IOBASE_LEN) {
-			ql4_printk(KERN_WARNING, ha,
-				   "Invalid PCI I/O region size (%s)...\n",
-				   pci_name(ha->pdev));
+			dev_warn(&ha->pdev->dev,
+				   "Invalid PCI I/O region size\n");
 			pio = 0;
 		}
 	} else {
-		ql4_printk(KERN_WARNING, ha,
-			   "region #0 not a PIO resource (%s)...\n",
-			   pci_name(ha->pdev));
+		dev_warn(&ha->pdev->dev, "region #0 not a PIO resource\n");
 		pio = 0;
 	}
 
@@ -718,23 +715,19 @@ static int qla4xxx_iospace_config(struct
 	mmio_flags = pci_resource_flags(ha->pdev, 1);
 
 	if (!(mmio_flags & IORESOURCE_MEM)) {
-		ql4_printk(KERN_ERR, ha,
-			   "region #0 not an MMIO resource (%s), aborting\n",
-			   pci_name(ha->pdev));
+		dev_err(&ha->pdev->dev,
+			   "region #0 not an MMIO resource, aborting\n");
 		goto iospace_error_exit;
 	}
 	if (mmio_len < MIN_IOBASE_LEN) {
-		ql4_printk(KERN_ERR, ha,
-			   "Invalid PCI mem region size (%s), aborting\n",
-			   pci_name(ha->pdev));
+		dev_err(&ha->pdev->dev,
+			   "Invalid PCI mem region size, aborting\n");
 		goto iospace_error_exit;
 	}
 
 	if (pci_request_regions(ha->pdev, DRIVER_NAME)) {
-		ql4_printk(KERN_WARNING, ha,
-			   "Failed to reserve PIO/MMIO regions (%s)\n",
-			   pci_name(ha->pdev));
-
+		dev_warn(&ha->pdev->dev,
+			 "Failed to reserve PIO/MMIO regions\n");
 		goto iospace_error_exit;
 	}
 
@@ -742,10 +735,7 @@ static int qla4xxx_iospace_config(struct
 	ha->pio_length = pio_len;
 	ha->reg = ioremap(mmio, MIN_IOBASE_LEN);
 	if (!ha->reg) {
-		ql4_printk(KERN_ERR, ha,
-			   "cannot remap MMIO (%s), aborting\n",
-			   pci_name(ha->pdev));
-
+		dev_err(&ha->pdev->dev, "cannot remap MMIO, aborting\n");
 		goto iospace_error_exit;
 	}
 
@@ -782,7 +772,7 @@ void qla4xxx_config_dma_addressing(struc
 /**
  * qla4xxx_mem_alloc - allocates memory for use by adapter.
  * @ha: Pointer to host adapter structure
- * 
+ *
  * Allocates DMA memory for request and response queues. Also allocates memory
  * for srbs.
  **/
@@ -799,9 +789,8 @@ static int qla4xxx_mem_alloc(struct scsi
 	ha->queues = dma_alloc_coherent(&ha->pdev->dev, ha->queues_len,
 					&ha->queues_dma, GFP_KERNEL);
 	if (ha->queues == NULL) {
-		ql4_printk(KERN_WARNING, ha,
-			   "Memory Allocation failed - queues.\n");
-
+		dev_warn(&ha->pdev->dev,
+			 "Memory Allocation failed - queues.\n");
 		goto mem_alloc_error_exit;
 	}
 	memset(ha->queues, 0, ha->queues_len);
@@ -836,9 +825,8 @@ static int qla4xxx_mem_alloc(struct scsi
 	ha->srb_mempool = mempool_create(SRB_MIN_REQ, mempool_alloc_slab,
 					 mempool_free_slab, srb_cachep);
 	if (ha->srb_mempool == NULL) {
-		ql4_printk(KERN_WARNING, ha,
-			   "Memory Allocation failed - SRB Pool.\n");
-
+		dev_warn(&ha->pdev->dev,
+			 "Memory Allocation failed - SRB Pool.\n");
 		goto mem_alloc_error_exit;
 	}
 
@@ -1709,7 +1697,7 @@ static int qla4xxx_eh_bus_reset(struct s
  * @ddb_entry: Pointer to device database entry
  *
  * This routine issues either a warm or cold target reset to the
- * specified device. The caller must ensure that the ddb_entry pointer 
+ * specified device. The caller must ensure that the ddb_entry pointer
  * is valid before calling this routine.
  *
  **/

[PATCH ver3 4/4] qla4xxx: improve symmetry in buffer codepaths

[Doug Maxey]

From: Doug Maxey <dwm@enoyolf.org>

In qla4xxx_send_command_to_isp(), pci_map_*() is used.  At the other
end, in qla4xxx_srb_free_dma(), dma_unmap_*() is used.  Replace the call
with the symmetrical call to pci_unmap_*().

Signed-off-by: Doug Maxey <dwm@enoyolf.org>
---

 drivers/scsi/qla4xxx/ql4_os.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index d88c84f..5e10f30 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -382,10 +382,10 @@ static void qla4xxx_srb_free_dma(struct 
 
 	if (srb->flags & SRB_DMA_VALID) {
 		if (cmd->use_sg) {
-			dma_unmap_sg(&ha->pdev->dev, cmd->request_buffer,
+			pci_unmap_sg(ha->pdev, cmd->request_buffer,
 				     cmd->use_sg, cmd->sc_data_direction);
 		} else if (cmd->request_bufflen) {
-			dma_unmap_single(&ha->pdev->dev, srb->dma_handle,
+			pci_unmap_single(ha->pdev, srb->dma_handle,
 					 cmd->request_bufflen,
 					 cmd->sc_data_direction);
 		}

Re: [PATCH ver3 3/4] qla4xxx: use dev_xxx on some pci/dma resource alloc warning and error printks

[Mike Christie]

Doug Maxey wrote:
> From: Doug Maxey <dwm@enoyolf.org>
> 
> Rework some of the printk's in the pci and dma resource allocations to
> - use the appropriate dev_xxx macro.
> - be non conditional
> 
> Signed-off-by: Doug Maxey <dwm@enoyolf.org>
> ---
> 
>  drivers/scsi/qla4xxx/ql4_os.c |   46 +++++++++++++++--------------------------
>  1 files changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
> index 1da7cf8..d88c84f 100644
> --- a/drivers/scsi/qla4xxx/ql4_os.c
> +++ b/drivers/scsi/qla4xxx/ql4_os.c
> @@ -688,7 +688,7 @@ static void qla4xxx_free_adapter(struct 
>   * @ha: pointer to adapter structure
>   *
>   * This routines maps HBA's registers from the pci address space
> - * into the kernel virtual address space for memory mapped i/o. 
> + * into the kernel virtual address space for memory mapped i/o.
>   **/
>  static int qla4xxx_iospace_config(struct scsi_qla_host *ha)
>  {
> @@ -700,15 +700,12 @@ static int qla4xxx_iospace_config(struct
>  	pio_flags = pci_resource_flags(ha->pdev, 0);
>  	if (pio_flags & IORESOURCE_IO) {
>  		if (pio_len < MIN_IOBASE_LEN) {
> -			ql4_printk(KERN_WARNING, ha,
> -				   "Invalid PCI I/O region size (%s)...\n",
> -				   pci_name(ha->pdev));
> +			dev_warn(&ha->pdev->dev,
> +				   "Invalid PCI I/O region size\n");
>  			pio = 0;
>  		}
>  	} else {
> -		ql4_printk(KERN_WARNING, ha,
> -			   "region #0 not a PIO resource (%s)...\n",
> -			   pci_name(ha->pdev));
> +		dev_warn(&ha->pdev->dev, "region #0 not a PIO resource\n");
>  		pio = 0;
>  	}
>  
> @@ -718,23 +715,19 @@ static int qla4xxx_iospace_config(struct
>  	mmio_flags = pci_resource_flags(ha->pdev, 1);
>  
>  	if (!(mmio_flags & IORESOURCE_MEM)) {
> -		ql4_printk(KERN_ERR, ha,
> -			   "region #0 not an MMIO resource (%s), aborting\n",
> -			   pci_name(ha->pdev));
> +		dev_err(&ha->pdev->dev,
> +			   "region #0 not an MMIO resource, aborting\n");
>  		goto iospace_error_exit;
>  	}
>  	if (mmio_len < MIN_IOBASE_LEN) {
> -		ql4_printk(KERN_ERR, ha,
> -			   "Invalid PCI mem region size (%s), aborting\n",
> -			   pci_name(ha->pdev));
> +		dev_err(&ha->pdev->dev,
> +			   "Invalid PCI mem region size, aborting\n");
>  		goto iospace_error_exit;
>  	}
>  
>  	if (pci_request_regions(ha->pdev, DRIVER_NAME)) {
> -		ql4_printk(KERN_WARNING, ha,
> -			   "Failed to reserve PIO/MMIO regions (%s)\n",
> -			   pci_name(ha->pdev));
> -
> +		dev_warn(&ha->pdev->dev,
> +			 "Failed to reserve PIO/MMIO regions\n");
>  		goto iospace_error_exit;
>  	}
>  
> @@ -742,10 +735,7 @@ static int qla4xxx_iospace_config(struct
>  	ha->pio_length = pio_len;
>  	ha->reg = ioremap(mmio, MIN_IOBASE_LEN);
>  	if (!ha->reg) {
> -		ql4_printk(KERN_ERR, ha,
> -			   "cannot remap MMIO (%s), aborting\n",
> -			   pci_name(ha->pdev));
> -
> +		dev_err(&ha->pdev->dev, "cannot remap MMIO, aborting\n");
>  		goto iospace_error_exit;
>  	}
>  
> @@ -782,7 +772,7 @@ void qla4xxx_config_dma_addressing(struc
>  /**
>   * qla4xxx_mem_alloc - allocates memory for use by adapter.
>   * @ha: Pointer to host adapter structure
> - * 
> + *
>   * Allocates DMA memory for request and response queues. Also allocates memory
>   * for srbs.
>   **/
> @@ -799,9 +789,8 @@ static int qla4xxx_mem_alloc(struct scsi
>  	ha->queues = dma_alloc_coherent(&ha->pdev->dev, ha->queues_len,
>  					&ha->queues_dma, GFP_KERNEL);
>  	if (ha->queues == NULL) {
> -		ql4_printk(KERN_WARNING, ha,
> -			   "Memory Allocation failed - queues.\n");
> -
> +		dev_warn(&ha->pdev->dev,
> +			 "Memory Allocation failed - queues.\n");
>  		goto mem_alloc_error_exit;
>  	}
>  	memset(ha->queues, 0, ha->queues_len);
> @@ -836,9 +825,8 @@ static int qla4xxx_mem_alloc(struct scsi
>  	ha->srb_mempool = mempool_create(SRB_MIN_REQ, mempool_alloc_slab,
>  					 mempool_free_slab, srb_cachep);
>  	if (ha->srb_mempool == NULL) {
> -		ql4_printk(KERN_WARNING, ha,
> -			   "Memory Allocation failed - SRB Pool.\n");
> -
> +		dev_warn(&ha->pdev->dev,
> +			 "Memory Allocation failed - SRB Pool.\n");
>  		goto mem_alloc_error_exit;

Sorry for the late response on this one. As you know I was out for a
while and I was waiting to get internet access yesterday.

For these host messages, do we want something like the sdev_printk and
starget_printk or does it really make more sense to use the pci bus id
for the message prefix? What about other scsi host messages, will they
always go with the pci bus id or some scsi-ml id? And even if we want to
print out the pci bus id as the prefix instead of some scsi info, should
we still have some scsi wrapper?

Re: [PATCH ver3 3/4] qla4xxx: use dev_xxx on some pci/dma resource alloc warning and error printks

[Doug Maxey]

On Wed, 09 Aug 2006 13:52:07 EDT, Mike Christie wrote:
...
> >  	if (ha->srb_mempool == NULL) {
> > -		ql4_printk(KERN_WARNING, ha,
> > -			   "Memory Allocation failed - SRB Pool.\n");
> > -
> > +		dev_warn(&ha->pdev->dev,
> > +			 "Memory Allocation failed - SRB Pool.\n");
> >  		goto mem_alloc_error_exit;
> 
> Sorry for the late response on this one. As you know I was out for a
> while and I was waiting to get internet access yesterday.
> 
> For these host messages, do we want something like the sdev_printk and
> starget_printk or does it really make more sense to use the pci bus id
> for the message prefix? What about other scsi host messages, will they
> always go with the pci bus id or some scsi-ml id? And even if we want to
> print out the pci bus id as the prefix instead of some scsi info, should
> we still have some scsi wrapper?
> 

Ha.  Just pre-replied to the this issue in the patchset just sent.

Will spin up an RFC patch and see where this goes.

Re: [PATCH ver3 3/4] qla4xxx: use dev_xxx on some pci/dma resource alloc warning and error printks

[Doug Maxey]

On Wed, 09 Aug 2006 13:52:07 EDT, Mike Christie wrote:
...
> > @@ -836,9 +825,8 @@ static int qla4xxx_mem_alloc(struct scsi
> >  	ha->srb_mempool = mempool_create(SRB_MIN_REQ, mempool_alloc_slab,
> >  					 mempool_free_slab, srb_cachep);
> >  	if (ha->srb_mempool == NULL) {
> > -		ql4_printk(KERN_WARNING, ha,
> > -			   "Memory Allocation failed - SRB Pool.\n");
> > -
> > +		dev_warn(&ha->pdev->dev,
> > +			 "Memory Allocation failed - SRB Pool.\n");
> >  		goto mem_alloc_error_exit;
> 
> Sorry for the late response on this one. As you know I was out for a
> while and I was waiting to get internet access yesterday.
> 
> For these host messages, do we want something like the sdev_printk and
> starget_printk or does it really make more sense to use the pci bus id
> for the message prefix? What about other scsi host messages, will they
> always go with the pci bus id or some scsi-ml id? And even if we want to
> print out the pci bus id as the prefix instead of some scsi info, should
> we still have some scsi wrapper?
> 
 
I do agree that iscsi_transport sessions could use a new macro.

My intention in this instance was to go with the widely used idiom, and 
to not have a driver specific one.  Was trying to replace
 #define ql4_printk(level, ha, format, arg...)			\
     dev_printk(level, &((ha)->pdev->dev), format, ## arg)

But to follow on from irc, one more pass at this to help me clarify and
understand what is need here.

dev_xxx is a wrapper around dev_print(xxx ...).  

In the specific instance above, this should print
scsiN arg...

Since qla4xxx_mem_alloc() is for the host, would an sdev_printk be
the right thing?  I don't believe we have any context of a target.
Of course that may be a misunderstanding on my part.

For this driver, I don't see any instances of a scsi_target.  

++doug

Re: [PATCH ver3 3/4] qla4xxx: use dev_xxx on some pci/dma resource alloc warning and error printks

[Ravi Anand]

>On Wed, 09 Aug 2006, Doug Maxey wrote:
> On Wed, 09 Aug 2006 13:52:07 EDT, Mike Christie wrote:
> ...
> > > @@ -836,9 +825,8 @@ static int qla4xxx_mem_alloc(struct scsi
> > >  	ha->srb_mempool = mempool_create(SRB_MIN_REQ, mempool_alloc_slab,
> > >  					 mempool_free_slab, srb_cachep);
> > >  	if (ha->srb_mempool == NULL) {
> > > -		ql4_printk(KERN_WARNING, ha,
> > > -			   "Memory Allocation failed - SRB Pool.\n");
> > > -
> > > +		dev_warn(&ha->pdev->dev,
> > > +			 "Memory Allocation failed - SRB Pool.\n");
> > >  		goto mem_alloc_error_exit;
> > 
> > Sorry for the late response on this one. As you know I was out for a
> > while and I was waiting to get internet access yesterday.
> > 
> > For these host messages, do we want something like the sdev_printk and
> > starget_printk or does it really make more sense to use the pci bus id
> > for the message prefix? What about other scsi host messages, will they
> > always go with the pci bus id or some scsi-ml id? And even if we want to
> > print out the pci bus id as the prefix instead of some scsi info, should
> > we still have some scsi wrapper?
> > 
>  
> I do agree that iscsi_transport sessions could use a new macro.
> 
> My intention in this instance was to go with the widely used idiom, and 
> to not have a driver specific one.  Was trying to replace
>  #define ql4_printk(level, ha, format, arg...)			\
>      dev_printk(level, &((ha)->pdev->dev), format, ## arg)
> 
> But to follow on from irc, one more pass at this to help me clarify and
> understand what is need here.
> 
> dev_xxx is a wrapper around dev_print(xxx ...).  
> 
> In the specific instance above, this should print
> scsiN arg...
> 
> Since qla4xxx_mem_alloc() is for the host, would an sdev_printk be
> the right thing?  I don't believe we have any context of a target.
> Of course that may be a misunderstanding on my part.
> 
> For this driver, I don't see any instances of a scsi_target.  

You are correct. Its during init time. So you dont have any instance of scsi_target.

Ravi

Re: [PATCH ver3 3/4] qla4xxx: use dev_xxx on some pci/dma resource alloc warning and error printks

[Mike Christie]

Doug Maxey wrote:
> On Wed, 09 Aug 2006 13:52:07 EDT, Mike Christie wrote:
> ...
>>> @@ -836,9 +825,8 @@ static int qla4xxx_mem_alloc(struct scsi
>>>  	ha->srb_mempool = mempool_create(SRB_MIN_REQ, mempool_alloc_slab,
>>>  					 mempool_free_slab, srb_cachep);
>>>  	if (ha->srb_mempool == NULL) {
>>> -		ql4_printk(KERN_WARNING, ha,
>>> -			   "Memory Allocation failed - SRB Pool.\n");
>>> -
>>> +		dev_warn(&ha->pdev->dev,
>>> +			 "Memory Allocation failed - SRB Pool.\n");
>>>  		goto mem_alloc_error_exit;
>> Sorry for the late response on this one. As you know I was out for a
>> while and I was waiting to get internet access yesterday.
>>
>> For these host messages, do we want something like the sdev_printk and
>> starget_printk or does it really make more sense to use the pci bus id
>> for the message prefix? What about other scsi host messages, will they
>> always go with the pci bus id or some scsi-ml id? And even if we want to
>> print out the pci bus id as the prefix instead of some scsi info, should
>> we still have some scsi wrapper?
>>
>  
> I do agree that iscsi_transport sessions could use a new macro.
> 
> My intention in this instance was to go with the widely used idiom, and 
> to not have a driver specific one.  Was trying to replace
>  #define ql4_printk(level, ha, format, arg...)			\
>      dev_printk(level, &((ha)->pdev->dev), format, ## arg)
> 
> But to follow on from irc, one more pass at this to help me clarify and
> understand what is need here.
> 
> dev_xxx is a wrapper around dev_print(xxx ...).  
> 
> In the specific instance above, this should print
> scsiN arg...

I think the ones in qla4xxx_mem_alloc print

%pci_id: arg...

> 
> Since qla4xxx_mem_alloc() is for the host, would an sdev_printk be
> the right thing?  I don't believe we have any context of a target.

I do not think so. To answer the sdev_printk question for future
reference and maybe to clear things up, I think sdev_printk is for
struct scsi_devices so it should be used for messages about a scsi
device. I think where we may be missing each other on this is that sdev
is a common abbreviation for the scsi_device (as in scsi SPEC logic
unit). It is not a abbreviation for scsi device as in any old scsi
object that has a struct device in it or is a device that scsi messes
with. So maybe in functions like the device reset eh callback we could
use sdev_printk. For functions just interacting with the scsi host that
want to print something about the host you would not.

But to really answer what we should have been taking about, I think just
using dev_printk is probably ok for now. When you said in some other
mail, that you thought we needed to use dev_printk, I thought you meant
that we needed to covert all the messages to dev_printk or one of the
scsi wrappers around it. After reading the mail above that you are just
removing the needless ql4_printk wrapper, I think just replacing
ql4_printk usage with dev_printk is ok for now.

> Of course that may be a misunderstanding on my part.
> 
> For this driver, I don't see any instances of a scsi_target.  
> 

The driver does not really deal with struct scsi_targets. The driver
interacts with structs like iscsi_sessions or iscsi_connections. The
iscsi_session hides most of the struct scsi_target details from the
driver. My mention of the scsi target was just because there is a
starget_printk and I was wondering if we need a shost_printk that the
LLD could use and that would complete the API.

For the iscsi conversion comment on irc, in scsi_transport_iscsi.c, we
do dev_printk(&session->dev and dev_printk(&connection->dev, and we
could maybe use some wrapper around them (like a iscsi_conn_printk() and
iscsi_sess_printk() that take a session and connection) like is done for
scsi_target and scsi_device structs and we could then use that in
libiscsi and iscsi_tcp and iscsi_iser. Right now in some of the iscsi
code we have some silly error messages that just spit out a error but do
not tell you what session, connection or host or device it is for.

Re: [PATCH ver3 3/4] qla4xxx: use dev_xxx on some pci/dma resource alloc warning and error printks

[Mike Christie]

Mike Christie wrote:
> Doug Maxey wrote:
>> On Wed, 09 Aug 2006 13:52:07 EDT, Mike Christie wrote:
>> ...
>>>> @@ -836,9 +825,8 @@ static int qla4xxx_mem_alloc(struct scsi
>>>>  	ha->srb_mempool = mempool_create(SRB_MIN_REQ, mempool_alloc_slab,
>>>>  					 mempool_free_slab, srb_cachep);
>>>>  	if (ha->srb_mempool == NULL) {
>>>> -		ql4_printk(KERN_WARNING, ha,
>>>> -			   "Memory Allocation failed - SRB Pool.\n");
>>>> -
>>>> +		dev_warn(&ha->pdev->dev,
>>>> +			 "Memory Allocation failed - SRB Pool.\n");
>>>>  		goto mem_alloc_error_exit;
>>> Sorry for the late response on this one. As you know I was out for a
>>> while and I was waiting to get internet access yesterday.
>>>
>>> For these host messages, do we want something like the sdev_printk and
>>> starget_printk or does it really make more sense to use the pci bus id
>>> for the message prefix? What about other scsi host messages, will they
>>> always go with the pci bus id or some scsi-ml id? And even if we want to
>>> print out the pci bus id as the prefix instead of some scsi info, should
>>> we still have some scsi wrapper?
>>>
>>  
>> I do agree that iscsi_transport sessions could use a new macro.
>>
>> My intention in this instance was to go with the widely used idiom, and 
>> to not have a driver specific one.  Was trying to replace
>>  #define ql4_printk(level, ha, format, arg...)			\
>>      dev_printk(level, &((ha)->pdev->dev), format, ## arg)
>>
>> But to follow on from irc, one more pass at this to help me clarify and
>> understand what is need here.
>>
>> dev_xxx is a wrapper around dev_print(xxx ...).  
>>
>> In the specific instance above, this should print
>> scsiN arg...
> 
> I think the ones in qla4xxx_mem_alloc print
> 
> %pci_id: arg...
> 

Maybe I was wrong...

Given that some messages are printing the pci bus id (the ones like
above in qla4xxx_mem_alloc using ql4_printk), and other messages are
printing the host number (the ones using printk with "scsi%ld" coded in
them) maybe this means....

> removing the needless ql4_printk wrapper, I think just replacing
> ql4_printk usage with dev_printk is ok for now.
> 

that this comment is wrong and maybe we need to to more.

> starget_printk and I was wondering if we need a shost_printk that the
> LLD could use and that would complete the API.
> 

And maybe we need a shost_printk to go along with the sdev and starget
printks, so drivers do not end up printing different prefixes for the
same objects like is happening with qla4xxx.

Re: [PATCH ver3 3/4] qla4xxx: use dev_xxx on some pci/dma resource alloc warning and error printks

[James Bottomley]

On Thu, 2006-08-10 at 06:18 -0400, Mike Christie wrote:
> And maybe we need a shost_printk to go along with the sdev and starget
> printks, so drivers do not end up printing different prefixes for the
> same objects like is happening with qla4xxx.

Sure ... add it if you need it.

James