[PATCH 00/10] aacraid: Patchset for aacraid driver version 41052

[PATCH 00/10] aacraid: Patchset for aacraid driver version 41052

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

This patchset includes the following changes (bug fixes and
new feature support) specific to aacraid driver.

Raghava Aditya Renukunta (10):
  [SCSI] aacraid: SCSI blk tag support
  [SCSI] aacraid: Fix RRQ overload
  [SCSI] aacraid: Added EEH support
  [SCSI] aacraid: Fix memory leak in aac_fib_map_free
  [SCSI] aacraid: Set correct msix count for EEH recovery
  [SCSI] aacraid: Fundamental reset support for Series 7
  [SCSI] aacraid: Fix AIF triggered IOP_RESET
  [SCSI] aacraid: Disable device ID wildcard
  [SCSI] aacraid: Fix character device re-initialization
  [SCSI] aacraid: Update driver version

 drivers/scsi/aacraid/aachba.c  |  40 +++++----
 drivers/scsi/aacraid/aacraid.h |   7 +-
 drivers/scsi/aacraid/commsup.c |  70 +++++++++++++--
 drivers/scsi/aacraid/dpcsup.c  |   4 +-
 drivers/scsi/aacraid/linit.c   | 196 +++++++++++++++++++++++++++++++++++++++--
 drivers/scsi/aacraid/src.c     |  30 ++-----
 6 files changed, 293 insertions(+), 54 deletions(-)

-- 
1.9.1

[PATCH 01/10] aacraid: SCSI blk tag support

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

The method to allocate and free FIB's in the present code utilizes
spinlocks.Multiple IO's have to wait on the spinlock to acquire or
free fibs creating a performance bottleneck.

An alternative solution would be to use block layer tags to keep track
of the fibs allocated and freed. To this end 2 functions
aac_fib_alloc_tag and aac_fib_free_tag were created which utilize the
blk layer tags to plug into the Fib pool.These functions are used
exclusively in the IO path. 8 fibs are reserved for the use of AIF
management software and utilize the previous spinlock based
implementations.

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/aachba.c  | 35 +++++++++++++++++-------------
 drivers/scsi/aacraid/aacraid.h |  2 ++
 drivers/scsi/aacraid/commsup.c | 49 +++++++++++++++++++++++++++++++++++++++---
 drivers/scsi/aacraid/dpcsup.c  |  4 ++--
 drivers/scsi/aacraid/linit.c   |  2 ++
 5 files changed, 72 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index e4c2437..06cbab8 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -323,7 +323,7 @@ static inline int aac_valid_context(struct scsi_cmnd *scsicmd,
 	if (unlikely(!scsicmd || !scsicmd->scsi_done)) {
 		dprintk((KERN_WARNING "aac_valid_context: scsi command corrupt\n"));
 		aac_fib_complete(fibptr);
-		aac_fib_free(fibptr);
+		aac_fib_free_tag(fibptr);
 		return 0;
 	}
 	scsicmd->SCp.phase = AAC_OWNER_MIDLEVEL;
@@ -331,7 +331,7 @@ static inline int aac_valid_context(struct scsi_cmnd *scsicmd,
 	if (unlikely(!device || !scsi_device_online(device))) {
 		dprintk((KERN_WARNING "aac_valid_context: scsi device corrupt\n"));
 		aac_fib_complete(fibptr);
-		aac_fib_free(fibptr);
+		aac_fib_free_tag(fibptr);
 		return 0;
 	}
 	return 1;
@@ -541,7 +541,7 @@ static void get_container_name_callback(void *context, struct fib * fibptr)
 	scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_GOOD;
 
 	aac_fib_complete(fibptr);
-	aac_fib_free(fibptr);
+	aac_fib_free_tag(fibptr);
 	scsicmd->scsi_done(scsicmd);
 }
 
@@ -557,7 +557,8 @@ static int aac_get_container_name(struct scsi_cmnd * scsicmd)
 
 	dev = (struct aac_dev *)scsicmd->device->host->hostdata;
 
-	if (!(cmd_fibcontext = aac_fib_alloc(dev)))
+	cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
+	if (!cmd_fibcontext)
 		return -ENOMEM;
 
 	aac_fib_init(cmd_fibcontext);
@@ -586,7 +587,7 @@ static int aac_get_container_name(struct scsi_cmnd * scsicmd)
 
 	printk(KERN_WARNING "aac_get_container_name: aac_fib_send failed with status: %d.\n", status);
 	aac_fib_complete(cmd_fibcontext);
-	aac_fib_free(cmd_fibcontext);
+	aac_fib_free_tag(cmd_fibcontext);
 	return -1;
 }
 
@@ -1024,7 +1025,7 @@ static void get_container_serial_callback(void *context, struct fib * fibptr)
 	scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_GOOD;
 
 	aac_fib_complete(fibptr);
-	aac_fib_free(fibptr);
+	aac_fib_free_tag(fibptr);
 	scsicmd->scsi_done(scsicmd);
 }
 
@@ -1040,7 +1041,8 @@ static int aac_get_container_serial(struct scsi_cmnd * scsicmd)
 
 	dev = (struct aac_dev *)scsicmd->device->host->hostdata;
 
-	if (!(cmd_fibcontext = aac_fib_alloc(dev)))
+	cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
+	if (!cmd_fibcontext)
 		return -ENOMEM;
 
 	aac_fib_init(cmd_fibcontext);
@@ -1068,7 +1070,7 @@ static int aac_get_container_serial(struct scsi_cmnd * scsicmd)
 
 	printk(KERN_WARNING "aac_get_container_serial: aac_fib_send failed with status: %d.\n", status);
 	aac_fib_complete(cmd_fibcontext);
-	aac_fib_free(cmd_fibcontext);
+	aac_fib_free_tag(cmd_fibcontext);
 	return -1;
 }
 
@@ -1869,7 +1871,7 @@ static void io_callback(void *context, struct fib * fibptr)
 		break;
 	}
 	aac_fib_complete(fibptr);
-	aac_fib_free(fibptr);
+	aac_fib_free_tag(fibptr);
 
 	scsicmd->scsi_done(scsicmd);
 }
@@ -1954,7 +1956,8 @@ static int aac_read(struct scsi_cmnd * scsicmd)
 	/*
 	 *	Alocate and initialize a Fib
 	 */
-	if (!(cmd_fibcontext = aac_fib_alloc(dev))) {
+	cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
+	if (!cmd_fibcontext) {
 		printk(KERN_WARNING "aac_read: fib allocation failed\n");
 		return -1;
 	}
@@ -2051,7 +2054,8 @@ static int aac_write(struct scsi_cmnd * scsicmd)
 	/*
 	 *	Allocate and initialize a Fib then setup a BlockWrite command
 	 */
-	if (!(cmd_fibcontext = aac_fib_alloc(dev))) {
+	cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
+	if (!cmd_fibcontext) {
 		/* FIB temporarily unavailable,not catastrophic failure */
 
 		/* scsicmd->result = DID_ERROR << 16;
@@ -2285,7 +2289,7 @@ static int aac_start_stop(struct scsi_cmnd *scsicmd)
 	/*
 	 *	Allocate and initialize a Fib
 	 */
-	cmd_fibcontext = aac_fib_alloc(aac);
+	cmd_fibcontext = aac_fib_alloc_tag(aac, scsicmd);
 	if (!cmd_fibcontext)
 		return SCSI_MLQUEUE_HOST_BUSY;
 
@@ -3157,7 +3161,7 @@ static void aac_srb_callback(void *context, struct fib * fibptr)
 	scsicmd->result |= le32_to_cpu(srbreply->scsi_status);
 
 	aac_fib_complete(fibptr);
-	aac_fib_free(fibptr);
+	aac_fib_free_tag(fibptr);
 	scsicmd->scsi_done(scsicmd);
 }
 
@@ -3187,9 +3191,10 @@ static int aac_send_srb_fib(struct scsi_cmnd* scsicmd)
 	/*
 	 *	Allocate and initialize a Fib then setup a BlockWrite command
 	 */
-	if (!(cmd_fibcontext = aac_fib_alloc(dev))) {
+	cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
+	if (!cmd_fibcontext)
 		return -1;
-	}
+
 	status = aac_adapter_scsi(cmd_fibcontext, scsicmd);
 
 	/*
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 074878b..da227e8 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -2114,9 +2114,11 @@ int aac_acquire_irq(struct aac_dev *dev);
 void aac_free_irq(struct aac_dev *dev);
 const char *aac_driverinfo(struct Scsi_Host *);
 struct fib *aac_fib_alloc(struct aac_dev *dev);
+struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd *scmd);
 int aac_fib_setup(struct aac_dev *dev);
 void aac_fib_map_free(struct aac_dev *dev);
 void aac_fib_free(struct fib * context);
+void aac_fib_free_tag(struct fib *context);
 void aac_fib_init(struct fib * context);
 void aac_printf(struct aac_dev *dev, u32 val);
 int aac_fib_send(u16 command, struct fib * context, unsigned long size, int priority, int wait, int reply, fib_callback callback, void *ctxt);
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index a1f90fe..b5b653c 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -156,9 +156,9 @@ int aac_fib_setup(struct aac_dev * dev)
 	 */
 	dev->fibs[dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB - 1].next = NULL;
 	/*
-	 *	Enable this to debug out of queue space
-	 */
-	dev->free_fib = &dev->fibs[0];
+	*	Set 8 fibs aside for management tools
+	*/
+	dev->free_fib = &dev->fibs[dev->scsi_host_ptr->can_queue];
 	return 0;
 }
 
@@ -166,6 +166,49 @@ int aac_fib_setup(struct aac_dev * dev)
  *	aac_fib_alloc	-	allocate a fib
  *	@dev: Adapter to allocate the fib for
  *
+ *	Allocate a fib from the adapter fib pool using tags
+ *	from the blk layer.
+ */
+
+struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd *scmd)
+{
+	struct fib *fibptr;
+
+	fibptr = &dev->fibs[scmd->request->tag];
+	/*
+	 *	Set the proper node type code and node byte size
+	 */
+	fibptr->type = FSAFS_NTC_FIB_CONTEXT;
+	fibptr->size = sizeof(struct fib);
+	/*
+	 *	Null out fields that depend on being zero at the start of
+	 *	each I/O
+	 */
+	fibptr->hw_fib_va->header.XferState = 0;
+	fibptr->flags = 0;
+	fibptr->callback = NULL;
+	fibptr->callback_data = NULL;
+
+	return fibptr;
+}
+
+/**
+ *	aac_fib_free_tag	free a fib
+ *	@fibptr: fib to free up
+ *
+ *	Placeholder to free tag allocated fibs
+ *	Does not do anything
+ */
+
+void aac_fib_free_tag(struct fib *fibptr)
+{
+	(void)fibptr;
+}
+
+/**
+ *	aac_fib_alloc	-	allocate a fib
+ *	@dev: Adapter to allocate the fib for
+ *
  *	Allocate a fib from the adapter fib pool. If the pool is empty we
  *	return NULL.
  */
diff --git a/drivers/scsi/aacraid/dpcsup.c b/drivers/scsi/aacraid/dpcsup.c
index da9d993..c0127f0 100644
--- a/drivers/scsi/aacraid/dpcsup.c
+++ b/drivers/scsi/aacraid/dpcsup.c
@@ -394,7 +394,7 @@ unsigned int aac_intr_normal(struct aac_dev *dev, u32 index,
 				fib->callback(fib->callback_data, fib);
 			} else {
 				aac_fib_complete(fib);
-				aac_fib_free(fib);
+				aac_fib_free_tag(fib);
 			}
 		} else {
 			unsigned long flagv;
@@ -416,7 +416,7 @@ unsigned int aac_intr_normal(struct aac_dev *dev, u32 index,
 				fib->done = 0;
 				spin_unlock_irqrestore(&fib->event_lock, flagv);
 				aac_fib_complete(fib);
-				aac_fib_free(fib);
+				aac_fib_free_tag(fib);
 			}
 
 		}
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 3b6e5c6..fa0fc44 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -454,6 +454,8 @@ static int aac_slave_configure(struct scsi_device *sdev)
 	} else
 		scsi_change_queue_depth(sdev, 1);
 
+		sdev->tagged_supported = 1;
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH 01/10] aacraid: SCSI blk tag support

[Johannes Thumshirn]

On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> The method to allocate and free FIB's in the present code utilizes
> spinlocks.Multiple IO's have to wait on the spinlock to acquire or
> free fibs creating a performance bottleneck.
> 
> An alternative solution would be to use block layer tags to keep track
> of the fibs allocated and freed. To this end 2 functions
> aac_fib_alloc_tag and aac_fib_free_tag were created which utilize the
> blk layer tags to plug into the Fib pool.These functions are used
> exclusively in the IO path. 8 fibs are reserved for the use of AIF
> management software and utilize the previous spinlock based
> implementations.
> 
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/aachba.c  | 35 +++++++++++++++++-------------
>  drivers/scsi/aacraid/aacraid.h |  2 ++
>  drivers/scsi/aacraid/commsup.c | 49 +++++++++++++++++++++++++++++++++++++++-
> --
>  drivers/scsi/aacraid/dpcsup.c  |  4 ++--
>  drivers/scsi/aacraid/linit.c   |  2 ++
>  5 files changed, 72 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index e4c2437..06cbab8 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -323,7 +323,7 @@ static inline int aac_valid_context(struct scsi_cmnd 

[...]

> 
>  	return 0;
>  }
>  
> @@ -166,6 +166,49 @@ int aac_fib_setup(struct aac_dev * dev)
>   *	aac_fib_alloc	-	allocate a fib

Nitpick/Typo aac_fib_alloc_tag

>   *	@dev: Adapter to allocate the fib for
>   *
> + *	Allocate a fib from the adapter fib pool using tags
> + *	from the blk layer.
> + */
> +
> +struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd *scmd)
> +{
> +	struct fib *fibptr;
> +
> +	fibptr = &dev->fibs[scmd->request->tag];
> +	/*
> +	 *	Set the proper node type code and node byte size
> +	 */
> +	fibptr->type = FSAFS_NTC_FIB_CONTEXT;
> +	fibptr->size = sizeof(struct fib);
> +	/*
> +	 *	Null out fields that depend on being zero at the start of
> +	 *	each I/O
> +	 */
> +	fibptr->hw_fib_va->header.XferState = 0;
> +	fibptr->flags = 0;
> +	fibptr->callback = NULL;
> +	fibptr->callback_data = NULL;
> +
> +	return fibptr;
> +}
> +
> +/**
> + *	aac_fib_free_tag	free a fib
> + *	@fibptr: fib to free up
> + *
> + *	Placeholder to free tag allocated fibs
> + *	Does not do anything
> + */
> +
> +void aac_fib_free_tag(struct fib *fibptr)
> +{
> +	(void)fibptr;
> +}
> +

I'm not sure if I like this placeholder. Sure it's a NOP and doesn't cost
anything but it's dead code. But if I'm the only one objecting here I'm fine
with it.

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/10] aacraid: SCSI blk tag support

[Tomas Henzl]

On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
>
> The method to allocate and free FIB's in the present code utilizes
> spinlocks.Multiple IO's have to wait on the spinlock to acquire or
> free fibs creating a performance bottleneck.
>
> An alternative solution would be to use block layer tags to keep track
> of the fibs allocated and freed. To this end 2 functions
> aac_fib_alloc_tag and aac_fib_free_tag were created which utilize the
> blk layer tags to plug into the Fib pool.These functions are used
> exclusively in the IO path. 8 fibs are reserved for the use of AIF
> management software and utilize the previous spinlock based
> implementations.
>
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/aachba.c  | 35 +++++++++++++++++-------------
>  drivers/scsi/aacraid/aacraid.h |  2 ++
>  drivers/scsi/aacraid/commsup.c | 49 +++++++++++++++++++++++++++++++++++++++---
>  drivers/scsi/aacraid/dpcsup.c  |  4 ++--
>  drivers/scsi/aacraid/linit.c   |  2 ++
>  5 files changed, 72 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index e4c2437..06cbab8 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -323,7 +323,7 @@ static inline int aac_valid_context(struct scsi_cmnd *scsicmd,
>  	if (unlikely(!scsicmd || !scsicmd->scsi_done)) {
>  		dprintk((KERN_WARNING "aac_valid_context: scsi command corrupt\n"));
>  		aac_fib_complete(fibptr);
> -		aac_fib_free(fibptr);
> +		aac_fib_free_tag(fibptr);
>  		return 0;
>  	}
>  	scsicmd->SCp.phase = AAC_OWNER_MIDLEVEL;
> @@ -331,7 +331,7 @@ static inline int aac_valid_context(struct scsi_cmnd *scsicmd,
>  	if (unlikely(!device || !scsi_device_online(device))) {
>  		dprintk((KERN_WARNING "aac_valid_context: scsi device corrupt\n"));
>  		aac_fib_complete(fibptr);
> -		aac_fib_free(fibptr);
> +		aac_fib_free_tag(fibptr);
>  		return 0;
>  	}
>  	return 1;
> @@ -541,7 +541,7 @@ static void get_container_name_callback(void *context, struct fib * fibptr)
>  	scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_GOOD;
>  
>  	aac_fib_complete(fibptr);
> -	aac_fib_free(fibptr);
> +	aac_fib_free_tag(fibptr);
>  	scsicmd->scsi_done(scsicmd);
>  }
>  
> @@ -557,7 +557,8 @@ static int aac_get_container_name(struct scsi_cmnd * scsicmd)
>  
>  	dev = (struct aac_dev *)scsicmd->device->host->hostdata;
>  
> -	if (!(cmd_fibcontext = aac_fib_alloc(dev)))
> +	cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
> +	if (!cmd_fibcontext)
>  		return -ENOMEM;
>  
>  	aac_fib_init(cmd_fibcontext);
> @@ -586,7 +587,7 @@ static int aac_get_container_name(struct scsi_cmnd * scsicmd)
>  
>  	printk(KERN_WARNING "aac_get_container_name: aac_fib_send failed with status: %d.\n", status);
>  	aac_fib_complete(cmd_fibcontext);
> -	aac_fib_free(cmd_fibcontext);
> +	aac_fib_free_tag(cmd_fibcontext);
>  	return -1;
>  }
>  
> @@ -1024,7 +1025,7 @@ static void get_container_serial_callback(void *context, struct fib * fibptr)
>  	scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_GOOD;
>  
>  	aac_fib_complete(fibptr);
> -	aac_fib_free(fibptr);
> +	aac_fib_free_tag(fibptr);
>  	scsicmd->scsi_done(scsicmd);
>  }
>  
> @@ -1040,7 +1041,8 @@ static int aac_get_container_serial(struct scsi_cmnd * scsicmd)
>  
>  	dev = (struct aac_dev *)scsicmd->device->host->hostdata;
>  
> -	if (!(cmd_fibcontext = aac_fib_alloc(dev)))
> +	cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
> +	if (!cmd_fibcontext)
>  		return -ENOMEM;
>  
>  	aac_fib_init(cmd_fibcontext);
> @@ -1068,7 +1070,7 @@ static int aac_get_container_serial(struct scsi_cmnd * scsicmd)
>  
>  	printk(KERN_WARNING "aac_get_container_serial: aac_fib_send failed with status: %d.\n", status);
>  	aac_fib_complete(cmd_fibcontext);
> -	aac_fib_free(cmd_fibcontext);
> +	aac_fib_free_tag(cmd_fibcontext);
>  	return -1;
>  }
>  
> @@ -1869,7 +1871,7 @@ static void io_callback(void *context, struct fib * fibptr)
>  		break;
>  	}
>  	aac_fib_complete(fibptr);
> -	aac_fib_free(fibptr);
> +	aac_fib_free_tag(fibptr);
>  
>  	scsicmd->scsi_done(scsicmd);
>  }
> @@ -1954,7 +1956,8 @@ static int aac_read(struct scsi_cmnd * scsicmd)
>  	/*
>  	 *	Alocate and initialize a Fib
>  	 */
> -	if (!(cmd_fibcontext = aac_fib_alloc(dev))) {
> +	cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
> +	if (!cmd_fibcontext) {
>  		printk(KERN_WARNING "aac_read: fib allocation failed\n");
>  		return -1;
>  	}
> @@ -2051,7 +2054,8 @@ static int aac_write(struct scsi_cmnd * scsicmd)
>  	/*
>  	 *	Allocate and initialize a Fib then setup a BlockWrite command
>  	 */
> -	if (!(cmd_fibcontext = aac_fib_alloc(dev))) {
> +	cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
> +	if (!cmd_fibcontext) {
>  		/* FIB temporarily unavailable,not catastrophic failure */
>  
>  		/* scsicmd->result = DID_ERROR << 16;
> @@ -2285,7 +2289,7 @@ static int aac_start_stop(struct scsi_cmnd *scsicmd)
>  	/*
>  	 *	Allocate and initialize a Fib
>  	 */
> -	cmd_fibcontext = aac_fib_alloc(aac);
> +	cmd_fibcontext = aac_fib_alloc_tag(aac, scsicmd);
>  	if (!cmd_fibcontext)
>  		return SCSI_MLQUEUE_HOST_BUSY;
>  
> @@ -3157,7 +3161,7 @@ static void aac_srb_callback(void *context, struct fib * fibptr)
>  	scsicmd->result |= le32_to_cpu(srbreply->scsi_status);
>  
>  	aac_fib_complete(fibptr);
> -	aac_fib_free(fibptr);
> +	aac_fib_free_tag(fibptr);
>  	scsicmd->scsi_done(scsicmd);
>  }
>  
> @@ -3187,9 +3191,10 @@ static int aac_send_srb_fib(struct scsi_cmnd* scsicmd)
>  	/*
>  	 *	Allocate and initialize a Fib then setup a BlockWrite command
>  	 */
> -	if (!(cmd_fibcontext = aac_fib_alloc(dev))) {
> +	cmd_fibcontext = aac_fib_alloc_tag(dev, scsicmd);
> +	if (!cmd_fibcontext)
>  		return -1;
> -	}
> +
>  	status = aac_adapter_scsi(cmd_fibcontext, scsicmd);
>  
>  	/*
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> index 074878b..da227e8 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -2114,9 +2114,11 @@ int aac_acquire_irq(struct aac_dev *dev);
>  void aac_free_irq(struct aac_dev *dev);
>  const char *aac_driverinfo(struct Scsi_Host *);
>  struct fib *aac_fib_alloc(struct aac_dev *dev);
> +struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd *scmd);
>  int aac_fib_setup(struct aac_dev *dev);
>  void aac_fib_map_free(struct aac_dev *dev);
>  void aac_fib_free(struct fib * context);
> +void aac_fib_free_tag(struct fib *context);
>  void aac_fib_init(struct fib * context);
>  void aac_printf(struct aac_dev *dev, u32 val);
>  int aac_fib_send(u16 command, struct fib * context, unsigned long size, int priority, int wait, int reply, fib_callback callback, void *ctxt);
> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
> index a1f90fe..b5b653c 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -156,9 +156,9 @@ int aac_fib_setup(struct aac_dev * dev)
>  	 */
>  	dev->fibs[dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB - 1].next = NULL;
>  	/*
> -	 *	Enable this to debug out of queue space
> -	 */
> -	dev->free_fib = &dev->fibs[0];
> +	*	Set 8 fibs aside for management tools
> +	*/
> +	dev->free_fib = &dev->fibs[dev->scsi_host_ptr->can_queue];
>  	return 0;
>  }
>  
> @@ -166,6 +166,49 @@ int aac_fib_setup(struct aac_dev * dev)
>   *	aac_fib_alloc	-	allocate a fib
>   *	@dev: Adapter to allocate the fib for
>   *
> + *	Allocate a fib from the adapter fib pool using tags
> + *	from the blk layer.
> + */
> +
> +struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd *scmd)
> +{
> +	struct fib *fibptr;
> +
> +	fibptr = &dev->fibs[scmd->request->tag];
> +	/*
> +	 *	Set the proper node type code and node byte size
> +	 */

Can't you just make most of the initialisation below just once when the driver starts?

> +	fibptr->type = FSAFS_NTC_FIB_CONTEXT;
> +	fibptr->size = sizeof(struct fib);
> +	/*
> +	 *	Null out fields that depend on being zero at the start of
> +	 *	each I/O
> +	 */
> +	fibptr->hw_fib_va->header.XferState = 0;
> +	fibptr->flags = 0;

For example the flags field is initialised  here to '0', then
in aac_fib_send again to zero and later is 'fibptr->flags = FIB_CONTEXT_FLAG;'
Removing part of it would help I think.
The code here is not new so, if I am right, please plan it for a next series. 

> +	fibptr->callback = NULL;
> +	fibptr->callback_data = NULL;
> +
> +	return fibptr;
> +}
> +
> +/**
> + *	aac_fib_free_tag	free a fib
> + *	@fibptr: fib to free up
> + *
> + *	Placeholder to free tag allocated fibs
> + *	Does not do anything
> + */
> +
> +void aac_fib_free_tag(struct fib *fibptr)
> +{
> +	(void)fibptr;
> +}

I agree with Johannes, I also don't like the aac_fib_free_tag as it is.
With the aac_fib_free_tag you may add -
Reviewed-by: Tomas Henzl <thenzl@redhat.com>
 
Tomas

RE: [PATCH 01/10] aacraid: SCSI blk tag support

[Raghava Aditya Renukunta]

Hello Tomas,

> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Thursday, December 3, 2015 7:52 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 01/10] aacraid: SCSI blk tag support
> 
> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> >
> > The method to allocate and free FIB's in the present code utilizes
> > spinlocks.Multiple IO's have to wait on the spinlock to acquire or
> > free fibs creating a performance bottleneck.
> >
> > An alternative solution would be to use block layer tags to keep track
> > of the fibs allocated and freed. To this end 2 functions
> > aac_fib_alloc_tag and aac_fib_free_tag were created which utilize the
> > blk layer tags to plug into the Fib pool.These functions are used
> > exclusively in the IO path. 8 fibs are reserved for the use of AIF
> > management software and utilize the previous spinlock based
> > implementations.
> >


[....]

> >
> > @@ -166,6 +166,49 @@ int aac_fib_setup(struct aac_dev * dev)
> >   *	aac_fib_alloc	-	allocate a fib
> >   *	@dev: Adapter to allocate the fib for
> >   *
> > + *	Allocate a fib from the adapter fib pool using tags
> > + *	from the blk layer.
> > + */
> > +
> > +struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd
> *scmd)
> > +{
> > +	struct fib *fibptr;
> > +
> > +	fibptr = &dev->fibs[scmd->request->tag];
> > +	/*
> > +	 *	Set the proper node type code and node byte size
> > +	 */
> 
> Can't you just make most of the initialisation below just once when the driver
> starts?
> 
> > +	fibptr->type = FSAFS_NTC_FIB_CONTEXT;
> > +	fibptr->size = sizeof(struct fib);
> > +	/*
> > +	 *	Null out fields that depend on being zero at the start of
> > +	 *	each I/O
> > +	 */
> > +	fibptr->hw_fib_va->header.XferState = 0;
> > +	fibptr->flags = 0;
> 
> For example the flags field is initialised  here to '0', then
> in aac_fib_send again to zero and later is 'fibptr->flags =
> FIB_CONTEXT_FLAG;'
> Removing part of it would help I think.
> The code here is not new so, if I am right, please plan it for a next series.

I will look into it and see where the code is being duplicated and will remove it,
If it does not do anything new.

> 
> > +	fibptr->callback = NULL;
> > +	fibptr->callback_data = NULL;
> > +
> > +	return fibptr;
> > +}
> > +
> > +/**
> > + *	aac_fib_free_tag	free a fib
> > + *	@fibptr: fib to free up
> > + *
> > + *	Placeholder to free tag allocated fibs
> > + *	Does not do anything
> > + */
> > +
> > +void aac_fib_free_tag(struct fib *fibptr)
> > +{
> > +	(void)fibptr;
> > +}
> 
> I agree with Johannes, I also don't like the aac_fib_free_tag as it is.
> With the aac_fib_free_tag you may add -

The reason it is still there is that , future versions can use it to add 
something meaningful(set flags etc).I could remove it now since 
it serves no purpose

> Reviewed-by: Tomas Henzl <thenzl@redhat.com>

Regards,
Raghava Aditya

[PATCH 02/10] aacraid: Fix RRQ overload

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

The driver utilizes an array of atomic variables to keep track of
IO submissions to each vector. To submit an IO multiple threads
iterate through the array to find a vector which has empty slots
to send an IO. The reading and updating of the variable is not atomic,
causing race conditions when a thread uses a full vector to
submit an IO.

Fixed by mapping each FIB to a vector, the submission path then uses
said vector to submit IO thereby removing the possibly of a race
condition.The vector assignment is started from 1 since vector 0 is
reserved for the use of AIF management FIBS.If the number of MSIx
vectors is 1 (MSI or INTx mode) then all the fibs are allocated to
vector 0.

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/aacraid.h |  1 +
 drivers/scsi/aacraid/commsup.c | 12 ++++++++++++
 drivers/scsi/aacraid/src.c     | 30 +++++++-----------------------
 3 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index da227e8..d133c4a 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -944,6 +944,7 @@ struct fib {
 	 */
 	struct list_head	fiblink;
 	void			*data;
+	u32			vector_no;
 	struct hw_fib		*hw_fib_va;		/* Actual shared object */
 	dma_addr_t		hw_fib_pa;		/* physical address of hw_fib*/
 };
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index b5b653c..b257d3b 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -104,6 +104,7 @@ int aac_fib_setup(struct aac_dev * dev)
 	struct hw_fib *hw_fib;
 	dma_addr_t hw_fib_pa;
 	int i;
+	u32 vector = 1;
 
 	while (((i = fib_map_alloc(dev)) == -ENOMEM)
 	 && (dev->scsi_host_ptr->can_queue > (64 - AAC_NUM_MGT_FIB))) {
@@ -150,6 +151,17 @@ int aac_fib_setup(struct aac_dev * dev)
 			dev->max_fib_size + sizeof(struct aac_fib_xporthdr));
 		hw_fib_pa = hw_fib_pa +
 			dev->max_fib_size + sizeof(struct aac_fib_xporthdr);
+
+		if ((dev->max_msix == 1) ||
+		  (i > ((dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB - 1)
+			- dev->vector_cap))) {
+			fibptr->vector_no = 0;
+		} else {
+			fibptr->vector_no = vector;
+			vector++;
+			if (vector == dev->max_msix)
+				vector = 1;
+		}
 	}
 	/*
 	 *	Add the fib chain to the free list
diff --git a/drivers/scsi/aacraid/src.c b/drivers/scsi/aacraid/src.c
index 2aa34ea..bc0203f 100644
--- a/drivers/scsi/aacraid/src.c
+++ b/drivers/scsi/aacraid/src.c
@@ -156,8 +156,8 @@ static irqreturn_t aac_src_intr_message(int irq, void *dev_id)
 				break;
 			if (dev->msi_enabled && dev->max_msix > 1)
 				atomic_dec(&dev->rrq_outstanding[vector_no]);
-			aac_intr_normal(dev, handle-1, 0, isFastResponse, NULL);
 			dev->host_rrq[index++] = 0;
+			aac_intr_normal(dev, handle-1, 0, isFastResponse, NULL);
 			if (index == (vector_no + 1) * dev->vector_cap)
 				index = vector_no * dev->vector_cap;
 			dev->host_rrq_idx[vector_no] = index;
@@ -452,36 +452,20 @@ static int aac_src_deliver_message(struct fib *fib)
 #endif
 
 	u16 hdr_size = le16_to_cpu(fib->hw_fib_va->header.Size);
+	u16 vector_no;
 
 	atomic_inc(&q->numpending);
 
 	if (dev->msi_enabled && fib->hw_fib_va->header.Command != AifRequest &&
 	    dev->max_msix > 1) {
-		u_int16_t vector_no, first_choice = 0xffff;
-
-		vector_no = dev->fibs_pushed_no % dev->max_msix;
-		do {
-			vector_no += 1;
-			if (vector_no == dev->max_msix)
-				vector_no = 1;
-			if (atomic_read(&dev->rrq_outstanding[vector_no]) <
-			    dev->vector_cap)
-				break;
-			if (0xffff == first_choice)
-				first_choice = vector_no;
-			else if (vector_no == first_choice)
-				break;
-		} while (1);
-		if (vector_no == first_choice)
-			vector_no = 0;
-		atomic_inc(&dev->rrq_outstanding[vector_no]);
-		if (dev->fibs_pushed_no == 0xffffffff)
-			dev->fibs_pushed_no = 0;
-		else
-			dev->fibs_pushed_no++;
+		vector_no = fib->vector_no;
 		fib->hw_fib_va->header.Handle += (vector_no << 16);
+	} else {
+		vector_no = 0;
 	}
 
+	atomic_inc(&dev->rrq_outstanding[vector_no]);
+
 	if (dev->comm_interface == AAC_COMM_MESSAGE_TYPE2) {
 		/* Calculate the amount to the fibsize bits */
 		fibsize = (hdr_size + 127) / 128 - 1;
-- 
1.9.1

Re: [PATCH 02/10] aacraid: Fix RRQ overload

[Johannes Thumshirn]

On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> The driver utilizes an array of atomic variables to keep track of
> IO submissions to each vector. To submit an IO multiple threads
> iterate through the array to find a vector which has empty slots
> to send an IO. The reading and updating of the variable is not atomic,
> causing race conditions when a thread uses a full vector to
> submit an IO.
> 
> Fixed by mapping each FIB to a vector, the submission path then uses
> said vector to submit IO thereby removing the possibly of a race
> condition.The vector assignment is started from 1 since vector 0 is
> reserved for the use of AIF management FIBS.If the number of MSIx
> vectors is 1 (MSI or INTx mode) then all the fibs are allocated to
> vector 0.
> 
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/aacraid.h |  1 +
>  drivers/scsi/aacraid/commsup.c | 12 ++++++++++++
>  drivers/scsi/aacraid/src.c     | 30 +++++++-----------------------
>  3 files changed, 20 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> index da227e8..d133c4a 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -944,6 +944,7 @@ struct fib {
>  	 */
>  	struct list_head	fiblink;
>  	void			*data;
> +	u32			vector_no;
>  	struct hw_fib		*hw_fib_va;		/* Actual
> shared object */
>  	dma_addr_t		hw_fib_pa;		/* physical
> address of hw_fib*/
>  };
> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
> index b5b653c..b257d3b 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -104,6 +104,7 @@ int aac_fib_setup(struct aac_dev * dev)
>  	struct hw_fib *hw_fib;
>  	dma_addr_t hw_fib_pa;
>  	int i;
> +	u32 vector = 1;
>  
>  	while (((i = fib_map_alloc(dev)) == -ENOMEM)
>  	 && (dev->scsi_host_ptr->can_queue > (64 - AAC_NUM_MGT_FIB))) {
> @@ -150,6 +151,17 @@ int aac_fib_setup(struct aac_dev * dev)
>  			dev->max_fib_size + sizeof(struct
> aac_fib_xporthdr));
>  		hw_fib_pa = hw_fib_pa +
>  			dev->max_fib_size + sizeof(struct aac_fib_xporthdr);
> +
> +		if ((dev->max_msix == 1) ||
> +		  (i > ((dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB -
> 1)
> +			- dev->vector_cap))) {
> +			fibptr->vector_no = 0;
> +		} else {
> +			fibptr->vector_no = vector;
> +			vector++;
> +			if (vector == dev->max_msix)
> +				vector = 1;
> +		}
>  	}
>  	/*
>  	 *	Add the fib chain to the free list
> diff --git a/drivers/scsi/aacraid/src.c b/drivers/scsi/aacraid/src.c
> index 2aa34ea..bc0203f 100644
> --- a/drivers/scsi/aacraid/src.c
> +++ b/drivers/scsi/aacraid/src.c
> @@ -156,8 +156,8 @@ static irqreturn_t aac_src_intr_message(int irq, void
> *dev_id)
>  				break;
>  			if (dev->msi_enabled && dev->max_msix > 1)
>  				atomic_dec(&dev-
> >rrq_outstanding[vector_no]);
> -			aac_intr_normal(dev, handle-1, 0, isFastResponse,
> NULL);
>  			dev->host_rrq[index++] = 0;
> +			aac_intr_normal(dev, handle-1, 0, isFastResponse,
> NULL);
>  			if (index == (vector_no + 1) * dev->vector_cap)
>  				index = vector_no * dev->vector_cap;
>  			dev->host_rrq_idx[vector_no] = index;
> @@ -452,36 +452,20 @@ static int aac_src_deliver_message(struct fib *fib)
>  #endif
>  
>  	u16 hdr_size = le16_to_cpu(fib->hw_fib_va->header.Size);
> +	u16 vector_no;
>  
>  	atomic_inc(&q->numpending);
>  
>  	if (dev->msi_enabled && fib->hw_fib_va->header.Command != AifRequest
> &&
>  	    dev->max_msix > 1) {
> -		u_int16_t vector_no, first_choice = 0xffff;
> -
> -		vector_no = dev->fibs_pushed_no % dev->max_msix;
> -		do {
> -			vector_no += 1;
> -			if (vector_no == dev->max_msix)
> -				vector_no = 1;
> -			if (atomic_read(&dev->rrq_outstanding[vector_no]) <
> -			    dev->vector_cap)
> -				break;
> -			if (0xffff == first_choice)
> -				first_choice = vector_no;
> -			else if (vector_no == first_choice)
> -				break;
> -		} while (1);
> -		if (vector_no == first_choice)
> -			vector_no = 0;
> -		atomic_inc(&dev->rrq_outstanding[vector_no]);
> -		if (dev->fibs_pushed_no == 0xffffffff)
> -			dev->fibs_pushed_no = 0;
> -		else
> -			dev->fibs_pushed_no++;
> +		vector_no = fib->vector_no;
>  		fib->hw_fib_va->header.Handle += (vector_no << 16);
> +	} else {
> +		vector_no = 0;
>  	}
>  
> +	atomic_inc(&dev->rrq_outstanding[vector_no]);
> +
>  	if (dev->comm_interface == AAC_COMM_MESSAGE_TYPE2) {
>  		/* Calculate the amount to the fibsize bits */
>  		fibsize = (hdr_size + 127) / 128 - 1;

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Fixes: 495c0217 "aacraid: MSI-x support"
Cc: stable@vger.kernel.org # v4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/10] aacraid: Fix RRQ overload

[Tomas Henzl]

On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
>
> The driver utilizes an array of atomic variables to keep track of
> IO submissions to each vector. To submit an IO multiple threads
> iterate through the array to find a vector which has empty slots
> to send an IO. The reading and updating of the variable is not atomic,
> causing race conditions when a thread uses a full vector to
> submit an IO.
>
> Fixed by mapping each FIB to a vector, the submission path then uses
> said vector to submit IO thereby removing the possibly of a race
> condition.The vector assignment is started from 1 since vector 0 is
> reserved for the use of AIF management FIBS.If the number of MSIx
> vectors is 1 (MSI or INTx mode) then all the fibs are allocated to
> vector 0.
>
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

Reviewed-by: Tomas Henzl <thenzl@redhat.com>

Tomas

[PATCH 03/10] aacraid: Added EEH support

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

Added support for PCI EEH(extended error handling).

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/aacraid.h |   1 +
 drivers/scsi/aacraid/linit.c   | 138 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 139 insertions(+)

diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index d133c4a..594de5f 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -1235,6 +1235,7 @@ struct aac_dev
 	struct msix_entry	msixentry[AAC_MAX_MSIX];
 	struct aac_msix_ctx	aac_msix[AAC_MAX_MSIX]; /* context */
 	u8			adapter_shutdown;
+	u32			handle_pci_error;
 };
 
 #define aac_adapter_interrupt(dev) \
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index fa0fc44..0147210 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -38,6 +38,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/pci.h>
+#include <linux/aer.h>
 #include <linux/pci-aspm.h>
 #include <linux/slab.h>
 #include <linux/mutex.h>
@@ -1298,6 +1299,9 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto out_deinit;
 	scsi_scan_host(shost);
 
+	pci_enable_pcie_error_reporting(pdev);
+	pci_save_state(pdev);
+
 	return 0;
 
  out_deinit:
@@ -1501,6 +1505,139 @@ static void aac_remove_one(struct pci_dev *pdev)
 	}
 }
 
+void aac_flush_ios(struct aac_dev *aac)
+{
+	int i;
+	struct scsi_cmnd *cmd;
+
+	for (i = 0; i < aac->scsi_host_ptr->can_queue; i++) {
+		cmd = (struct scsi_cmnd *)aac->fibs[i].callback_data;
+		if (cmd && (cmd->SCp.phase == AAC_OWNER_FIRMWARE)) {
+			scsi_dma_unmap(cmd);
+			aac_fib_free_tag(&aac->fibs[i]);
+
+			if (aac->handle_pci_error)
+				cmd->result = DID_NO_CONNECT << 16;
+			else
+				cmd->result = DID_RESET << 16;
+
+			cmd->scsi_done(cmd);
+		}
+	}
+}
+
+pci_ers_result_t aac_pci_error_detected(struct pci_dev *pdev,
+					enum pci_channel_state error)
+{
+	struct Scsi_Host *shost = pci_get_drvdata(pdev);
+	struct aac_dev *aac = shost_priv(shost);
+
+	dev_err(&pdev->dev, "aacraid: PCI error detected %x\n", error);
+
+	switch (error) {
+	case pci_channel_io_normal:
+		return PCI_ERS_RESULT_CAN_RECOVER;
+	case pci_channel_io_frozen:
+
+		aac->handle_pci_error = 1;
+
+		scsi_block_requests(aac->scsi_host_ptr);
+		aac_flush_ios(aac);
+		aac_release_resources(aac);
+
+		pci_disable_pcie_error_reporting(pdev);
+		aac_adapter_ioremap(aac, 0);
+
+		return PCI_ERS_RESULT_NEED_RESET;
+	case pci_channel_io_perm_failure:
+		aac->handle_pci_error = 1;
+		aac_flush_ios(aac);
+		return PCI_ERS_RESULT_DISCONNECT;
+	}
+
+	return PCI_ERS_RESULT_NEED_RESET;
+}
+
+pci_ers_result_t aac_pci_mmio_enabled(struct pci_dev *pdev)
+{
+	dev_err(&pdev->dev, "aacraid: PCI error - mmio enabled\n");
+	return PCI_ERS_RESULT_NEED_RESET;
+}
+
+pci_ers_result_t aac_pci_slot_reset(struct pci_dev *pdev)
+{
+	dev_err(&pdev->dev, "aacraid: PCI error - slot reset\n");
+	pci_restore_state(pdev);
+	if (pci_enable_device(pdev)) {
+		dev_warn(&pdev->dev,
+			"aacraid: failed to enable slave\n");
+		goto fail_device;
+	}
+
+	pci_set_master(pdev);
+
+	if (pci_enable_device_mem(pdev)) {
+		dev_err(&pdev->dev, "pci_enable_device_mem failed\n");
+		goto fail_device;
+	}
+
+	return PCI_ERS_RESULT_RECOVERED;
+
+fail_device:
+	dev_err(&pdev->dev, "aacraid: PCI error - slot reset failed\n");
+	return PCI_ERS_RESULT_DISCONNECT;
+}
+
+
+void aac_pci_resume(struct pci_dev *pdev)
+{
+	struct Scsi_Host *shost = pci_get_drvdata(pdev);
+	struct scsi_device *sdev = NULL;
+	struct aac_dev *aac = (struct aac_dev *)shost_priv(shost);
+
+	pci_cleanup_aer_uncorrect_error_status(pdev);
+
+	if (aac_adapter_ioremap(aac, aac->base_size)) {
+
+		dev_err(&pdev->dev, "aacraid: ioremap failed\n");
+		/* remap failed, go back ... */
+		aac->comm_interface = AAC_COMM_PRODUCER;
+		if (aac_adapter_ioremap(aac, AAC_MIN_FOOTPRINT_SIZE)) {
+			dev_warn(&pdev->dev,
+				"aacraid: unable to map adapter.\n");
+
+			return;
+		}
+	}
+
+	msleep(10000);
+
+	aac_acquire_resources(aac);
+
+	/*
+	 * reset this flag to unblock ioctl() as it was set
+	 * at aac_send_shutdown() to block ioctls from upperlayer
+	 */
+	aac->adapter_shutdown = 0;
+	aac->handle_pci_error = 0;
+
+	shost_for_each_device(sdev, shost)
+		if (sdev->sdev_state == SDEV_OFFLINE)
+			sdev->sdev_state = SDEV_RUNNING;
+	scsi_unblock_requests(aac->scsi_host_ptr);
+	scsi_scan_host(aac->scsi_host_ptr);
+	pci_save_state(pdev);
+
+	dev_err(&pdev->dev, "aacraid: PCI error - resume\n");
+}
+
+static struct pci_error_handlers aac_pci_err_handler = {
+	.error_detected		= aac_pci_error_detected,
+	.mmio_enabled		= aac_pci_mmio_enabled,
+	.slot_reset		= aac_pci_slot_reset,
+	.resume			= aac_pci_resume,
+};
+
 static struct pci_driver aac_pci_driver = {
 	.name		= AAC_DRIVERNAME,
 	.id_table	= aac_pci_tbl,
@@ -1511,6 +1648,7 @@ static struct pci_driver aac_pci_driver = {
 	.resume		= aac_resume,
 #endif
 	.shutdown	= aac_shutdown,
+	.err_handler    = &aac_pci_err_handler,
 };
 
 static int __init aac_init(void)
-- 
1.9.1

Re: [PATCH 03/10] aacraid: Added EEH support

[Johannes Thumshirn]

On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> Added support for PCI EEH(extended error handling).
> 
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/aacraid.h |   1 +
>  drivers/scsi/aacraid/linit.c   | 138
> +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 139 insertions(+)
> 
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> index d133c4a..594de5f 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -1235,6 +1235,7 @@ struct aac_dev
>  	struct msix_entry	msixentry[AAC_MAX_MSIX];
>  	struct aac_msix_ctx	aac_msix[AAC_MAX_MSIX]; /* context */
>  	u8			adapter_shutdown;
> +	u32			handle_pci_error;
>  };
>  
>  #define aac_adapter_interrupt(dev) \
> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> index fa0fc44..0147210 100644
> --- a/drivers/scsi/aacraid/linit.c
> +++ b/drivers/scsi/aacraid/linit.c
> @@ -38,6 +38,7 @@
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
>  #include <linux/pci.h>
> +#include <linux/aer.h>
>  #include <linux/pci-aspm.h>
>  #include <linux/slab.h>
>  #include <linux/mutex.h>
> @@ -1298,6 +1299,9 @@ static int aac_probe_one(struct pci_dev *pdev, const
> struct pci_device_id *id)
>  		goto out_deinit;
>  	scsi_scan_host(shost);
>  
> +	pci_enable_pcie_error_reporting(pdev);
> +	pci_save_state(pdev);
> +
>  	return 0;
>  
>   out_deinit:
> @@ -1501,6 +1505,139 @@ static void aac_remove_one(struct pci_dev *pdev)
>  	}
>  }
>  
> +void aac_flush_ios(struct aac_dev *aac)
> +{
> +	int i;
> +	struct scsi_cmnd *cmd;
> +
> +	for (i = 0; i < aac->scsi_host_ptr->can_queue; i++) {
> +		cmd = (struct scsi_cmnd *)aac->fibs[i].callback_data;
> +		if (cmd && (cmd->SCp.phase == AAC_OWNER_FIRMWARE)) {
> +			scsi_dma_unmap(cmd);
> +			aac_fib_free_tag(&aac->fibs[i]);
> +
> +			if (aac->handle_pci_error)
> +				cmd->result = DID_NO_CONNECT << 16;
> +			else
> +				cmd->result = DID_RESET << 16;
> +
> +			cmd->scsi_done(cmd);
> +		}
> +	}
> +}
> +
> +pci_ers_result_t aac_pci_error_detected(struct pci_dev *pdev,
> +					enum pci_channel_state error)
> +{
> +	struct Scsi_Host *shost = pci_get_drvdata(pdev);
> +	struct aac_dev *aac = shost_priv(shost);
> +
> +	dev_err(&pdev->dev, "aacraid: PCI error detected %x\n", error);
> +
> +	switch (error) {
> +	case pci_channel_io_normal:
> +		return PCI_ERS_RESULT_CAN_RECOVER;
> +	case pci_channel_io_frozen:
> +
> +		aac->handle_pci_error = 1;
> +
> +		scsi_block_requests(aac->scsi_host_ptr);
> +		aac_flush_ios(aac);
> +		aac_release_resources(aac);
> +
> +		pci_disable_pcie_error_reporting(pdev);
> +		aac_adapter_ioremap(aac, 0);
> +
> +		return PCI_ERS_RESULT_NEED_RESET;
> +	case pci_channel_io_perm_failure:
> +		aac->handle_pci_error = 1;
> +		aac_flush_ios(aac);
> +		return PCI_ERS_RESULT_DISCONNECT;
> +	}
> +
> +	return PCI_ERS_RESULT_NEED_RESET;
> +}
> +
> +pci_ers_result_t aac_pci_mmio_enabled(struct pci_dev *pdev)
> +{
> +	dev_err(&pdev->dev, "aacraid: PCI error - mmio enabled\n");
> +	return PCI_ERS_RESULT_NEED_RESET;
> +}
> +
> +pci_ers_result_t aac_pci_slot_reset(struct pci_dev *pdev)
> +{
> +	dev_err(&pdev->dev, "aacraid: PCI error - slot reset\n");
> +	pci_restore_state(pdev);
> +	if (pci_enable_device(pdev)) {
> +		dev_warn(&pdev->dev,
> +			"aacraid: failed to enable slave\n");
> +		goto fail_device;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	if (pci_enable_device_mem(pdev)) {
> +		dev_err(&pdev->dev, "pci_enable_device_mem failed\n");
> +		goto fail_device;
> +	}
> +
> +	return PCI_ERS_RESULT_RECOVERED;
> +
> +fail_device:
> +	dev_err(&pdev->dev, "aacraid: PCI error - slot reset failed\n");
> +	return PCI_ERS_RESULT_DISCONNECT;
> +}
> +
> +
> +void aac_pci_resume(struct pci_dev *pdev)
> +{
> +	struct Scsi_Host *shost = pci_get_drvdata(pdev);
> +	struct scsi_device *sdev = NULL;
> +	struct aac_dev *aac = (struct aac_dev *)shost_priv(shost);
> +
> +	pci_cleanup_aer_uncorrect_error_status(pdev);
> +
> +	if (aac_adapter_ioremap(aac, aac->base_size)) {
> +
> +		dev_err(&pdev->dev, "aacraid: ioremap failed\n");
> +		/* remap failed, go back ... */
> +		aac->comm_interface = AAC_COMM_PRODUCER;
> +		if (aac_adapter_ioremap(aac, AAC_MIN_FOOTPRINT_SIZE)) {
> +			dev_warn(&pdev->dev,
> +				"aacraid: unable to map adapter.\n");
> +
> +			return;
> +		}
> +	}
> +
> +	msleep(10000);
> +
> +	aac_acquire_resources(aac);
> +
> +	/*
> +	 * reset this flag to unblock ioctl() as it was set
> +	 * at aac_send_shutdown() to block ioctls from upperlayer
> +	 */
> +	aac->adapter_shutdown = 0;
> +	aac->handle_pci_error = 0;
> +
> +	shost_for_each_device(sdev, shost)
> +		if (sdev->sdev_state == SDEV_OFFLINE)
> +			sdev->sdev_state = SDEV_RUNNING;
> +	scsi_unblock_requests(aac->scsi_host_ptr);
> +	scsi_scan_host(aac->scsi_host_ptr);
> +	pci_save_state(pdev);
> +
> +	dev_err(&pdev->dev, "aacraid: PCI error - resume\n");
> +}
> +
> +static struct pci_error_handlers aac_pci_err_handler = {
> +	.error_detected		= aac_pci_error_detected,
> +	.mmio_enabled		= aac_pci_mmio_enabled,
> +	.slot_reset		= aac_pci_slot_reset,
> +	.resume			= aac_pci_resume,
> +};
> +
>  static struct pci_driver aac_pci_driver = {
>  	.name		= AAC_DRIVERNAME,
>  	.id_table	= aac_pci_tbl,
> @@ -1511,6 +1648,7 @@ static struct pci_driver aac_pci_driver = {
>  	.resume		= aac_resume,
>  #endif
>  	.shutdown	= aac_shutdown,
> +	.err_handler    = &aac_pci_err_handler,
>  };
>  
>  static int __init aac_init(void)

I really think the new aac_flush_io() and pci error hanler methods should be
static as they're nowhere used outside of this file's scope. 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 03/10] aacraid: Added EEH support

[Raghava Aditya Renukunta]

Hello Johannes,

> -----Original Message-----
> From: Johannes Thumshirn [mailto:jthumshirn@suse.de]
> Sent: Wednesday, December 2, 2015 1:42 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 03/10] aacraid: Added EEH support
> 
> On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> >
> > Added support for PCI EEH(extended error handling).
> >
> > Signed-off-by: Raghava Aditya Renukunta
> > <raghavaaditya.renukunta@pmcs.com>
> > ---
> >  drivers/scsi/aacraid/aacraid.h |   1 +
> >  drivers/scsi/aacraid/linit.c   | 138
> > +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 139 insertions(+)
> >
> > diff --git a/drivers/scsi/aacraid/aacraid.h
> > b/drivers/scsi/aacraid/aacraid.h index d133c4a..594de5f 100644
> > --- a/drivers/scsi/aacraid/aacraid.h
> > +++ b/drivers/scsi/aacraid/aacraid.h
> > @@ -1235,6 +1235,7 @@ struct aac_dev
> >  	struct msix_entry	msixentry[AAC_MAX_MSIX];
> >  	struct aac_msix_ctx	aac_msix[AAC_MAX_MSIX]; /* context */
> >  	u8			adapter_shutdown;
> > +	u32			handle_pci_error;
> >  };
> >
> >  #define aac_adapter_interrupt(dev) \
> > diff --git a/drivers/scsi/aacraid/linit.c
> > b/drivers/scsi/aacraid/linit.c index fa0fc44..0147210 100644
> > --- a/drivers/scsi/aacraid/linit.c
> > +++ b/drivers/scsi/aacraid/linit.c
> > @@ -38,6 +38,7 @@
> >  #include <linux/module.h>
> >  #include <linux/moduleparam.h>
> >  #include <linux/pci.h>
> > +#include <linux/aer.h>
> >  #include <linux/pci-aspm.h>
> >  #include <linux/slab.h>
> >  #include <linux/mutex.h>
> > @@ -1298,6 +1299,9 @@ static int aac_probe_one(struct pci_dev *pdev,
> > const struct pci_device_id *id)
> >  		goto out_deinit;
> >  	scsi_scan_host(shost);
> >
> > +	pci_enable_pcie_error_reporting(pdev);
> > +	pci_save_state(pdev);
> > +
> >  	return 0;
> >
> >   out_deinit:
> > @@ -1501,6 +1505,139 @@ static void aac_remove_one(struct pci_dev
> *pdev)
> >  	}
> >  }
> >
> > +void aac_flush_ios(struct aac_dev *aac) {
> > +	int i;
> > +	struct scsi_cmnd *cmd;
> > +
> > +	for (i = 0; i < aac->scsi_host_ptr->can_queue; i++) {
> > +		cmd = (struct scsi_cmnd *)aac->fibs[i].callback_data;
> > +		if (cmd && (cmd->SCp.phase == AAC_OWNER_FIRMWARE))
> {
> > +			scsi_dma_unmap(cmd);
> > +			aac_fib_free_tag(&aac->fibs[i]);
> > +
> > +			if (aac->handle_pci_error)
> > +				cmd->result = DID_NO_CONNECT << 16;
> > +			else
> > +				cmd->result = DID_RESET << 16;
> > +
> > +			cmd->scsi_done(cmd);
> > +		}
> > +	}
> > +}
> > +
> > +pci_ers_result_t aac_pci_error_detected(struct pci_dev *pdev,
> > +					enum pci_channel_state error)
> > +{
> > +	struct Scsi_Host *shost = pci_get_drvdata(pdev);
> > +	struct aac_dev *aac = shost_priv(shost);
> > +
> > +	dev_err(&pdev->dev, "aacraid: PCI error detected %x\n", error);
> > +
> > +	switch (error) {
> > +	case pci_channel_io_normal:
> > +		return PCI_ERS_RESULT_CAN_RECOVER;
> > +	case pci_channel_io_frozen:
> > +
> > +		aac->handle_pci_error = 1;
> > +
> > +		scsi_block_requests(aac->scsi_host_ptr);
> > +		aac_flush_ios(aac);
> > +		aac_release_resources(aac);
> > +
> > +		pci_disable_pcie_error_reporting(pdev);
> > +		aac_adapter_ioremap(aac, 0);
> > +
> > +		return PCI_ERS_RESULT_NEED_RESET;
> > +	case pci_channel_io_perm_failure:
> > +		aac->handle_pci_error = 1;
> > +		aac_flush_ios(aac);
> > +		return PCI_ERS_RESULT_DISCONNECT;
> > +	}
> > +
> > +	return PCI_ERS_RESULT_NEED_RESET;
> > +}
> > +
> > +pci_ers_result_t aac_pci_mmio_enabled(struct pci_dev *pdev) {
> > +	dev_err(&pdev->dev, "aacraid: PCI error - mmio enabled\n");
> > +	return PCI_ERS_RESULT_NEED_RESET;
> > +}
> > +
> > +pci_ers_result_t aac_pci_slot_reset(struct pci_dev *pdev) {
> > +	dev_err(&pdev->dev, "aacraid: PCI error - slot reset\n");
> > +	pci_restore_state(pdev);
> > +	if (pci_enable_device(pdev)) {
> > +		dev_warn(&pdev->dev,
> > +			"aacraid: failed to enable slave\n");
> > +		goto fail_device;
> > +	}
> > +
> > +	pci_set_master(pdev);
> > +
> > +	if (pci_enable_device_mem(pdev)) {
> > +		dev_err(&pdev->dev, "pci_enable_device_mem failed\n");
> > +		goto fail_device;
> > +	}
> > +
> > +	return PCI_ERS_RESULT_RECOVERED;
> > +
> > +fail_device:
> > +	dev_err(&pdev->dev, "aacraid: PCI error - slot reset failed\n");
> > +	return PCI_ERS_RESULT_DISCONNECT;
> > +}
> > +
> > +
> > +void aac_pci_resume(struct pci_dev *pdev) {
> > +	struct Scsi_Host *shost = pci_get_drvdata(pdev);
> > +	struct scsi_device *sdev = NULL;
> > +	struct aac_dev *aac = (struct aac_dev *)shost_priv(shost);
> > +
> > +	pci_cleanup_aer_uncorrect_error_status(pdev);
> > +
> > +	if (aac_adapter_ioremap(aac, aac->base_size)) {
> > +
> > +		dev_err(&pdev->dev, "aacraid: ioremap failed\n");
> > +		/* remap failed, go back ... */
> > +		aac->comm_interface = AAC_COMM_PRODUCER;
> > +		if (aac_adapter_ioremap(aac, AAC_MIN_FOOTPRINT_SIZE)) {
> > +			dev_warn(&pdev->dev,
> > +				"aacraid: unable to map adapter.\n");
> > +
> > +			return;
> > +		}
> > +	}
> > +
> > +	msleep(10000);
> > +
> > +	aac_acquire_resources(aac);
> > +
> > +	/*
> > +	 * reset this flag to unblock ioctl() as it was set
> > +	 * at aac_send_shutdown() to block ioctls from upperlayer
> > +	 */
> > +	aac->adapter_shutdown = 0;
> > +	aac->handle_pci_error = 0;
> > +
> > +	shost_for_each_device(sdev, shost)
> > +		if (sdev->sdev_state == SDEV_OFFLINE)
> > +			sdev->sdev_state = SDEV_RUNNING;
> > +	scsi_unblock_requests(aac->scsi_host_ptr);
> > +	scsi_scan_host(aac->scsi_host_ptr);
> > +	pci_save_state(pdev);
> > +
> > +	dev_err(&pdev->dev, "aacraid: PCI error - resume\n"); }
> > +
> > +static struct pci_error_handlers aac_pci_err_handler = {
> > +	.error_detected		= aac_pci_error_detected,
> > +	.mmio_enabled		= aac_pci_mmio_enabled,
> > +	.slot_reset		= aac_pci_slot_reset,
> > +	.resume			= aac_pci_resume,
> > +};
> > +
> >  static struct pci_driver aac_pci_driver = {
> >  	.name		= AAC_DRIVERNAME,
> >  	.id_table	= aac_pci_tbl,
> > @@ -1511,6 +1648,7 @@ static struct pci_driver aac_pci_driver = {
> >  	.resume		= aac_resume,
> >  #endif
> >  	.shutdown	= aac_shutdown,
> > +	.err_handler    = &aac_pci_err_handler,
> >  };
> >
> >  static int __init aac_init(void)
> 
> I really think the new aac_flush_io() and pci error hanler methods should be
> static as they're nowhere used outside of this file's scope.

Yes, I will make them static.

Regards,
Raghava Aditya

Re: [PATCH 03/10] aacraid: Added EEH support

[Tomas Henzl]

On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
>
> Added support for PCI EEH(extended error handling).
>
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

Reviewed-by: Tomas Henzl <thenzl@redhat.com>

Tomas

[PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

aac_fib_map_free() calls pci_free_consistent() without checking that
dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
are indeed NULL/0, this will result in a hang as pci_free_consistent()
will attempt to invalidate cache for the entire 64-bit address space
(which would take a very long time).

Fixed by adding a check to make sure that dev->hw_fib_va and
dev->max_fib_size are not NULL and 0 respectively.

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/commsup.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index b257d3b..9533f47 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
 
 void aac_fib_map_free(struct aac_dev *dev)
 {
-	pci_free_consistent(dev->pdev,
-	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB),
-	  dev->hw_fib_va, dev->hw_fib_pa);
+	if (dev->hw_fib_va && dev->max_fib_size) {
+		pci_free_consistent(dev->pdev,
+		(dev->max_fib_size *
+		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
+		dev->hw_fib_va, dev->hw_fib_pa);
+	}
 	dev->hw_fib_va = NULL;
 	dev->hw_fib_pa = 0;
 }
-- 
1.9.1

Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free

[Johannes Thumshirn]

On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> aac_fib_map_free() calls pci_free_consistent() without checking that
> dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
> are indeed NULL/0, this will result in a hang as pci_free_consistent()
> will attempt to invalidate cache for the entire 64-bit address space
> (which would take a very long time).
> 
> Fixed by adding a check to make sure that dev->hw_fib_va and
> dev->max_fib_size are not NULL and 0 respectively.
> 
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/commsup.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
> index b257d3b..9533f47 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
>  
>  void aac_fib_map_free(struct aac_dev *dev)
>  {
> -	pci_free_consistent(dev->pdev,
> -	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue +
> AAC_NUM_MGT_FIB),
> -	  dev->hw_fib_va, dev->hw_fib_pa);
> +	if (dev->hw_fib_va && dev->max_fib_size) {
> +		pci_free_consistent(dev->pdev,
> +		(dev->max_fib_size *
> +		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
> +		dev->hw_fib_va, dev->hw_fib_pa);
> +	}
>  	dev->hw_fib_va = NULL;
>  	dev->hw_fib_pa = 0;
>  }

Fixes: 9ad5204d6 - "[SCSI] aacraid: incorrect dma mapping mask during blinkled
recover or user initiated reset"
Cc: stable@vger.kernel.org
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free

[Tomas Henzl]

On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
>
> aac_fib_map_free() calls pci_free_consistent() without checking that
> dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
> are indeed NULL/0, this will result in a hang as pci_free_consistent()
> will attempt to invalidate cache for the entire 64-bit address space
> (which would take a very long time).
>
> Fixed by adding a check to make sure that dev->hw_fib_va and
> dev->max_fib_size are not NULL and 0 respectively.
>
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

Reviewed-by: Tomas Henzl <thenzl@redhat.com>

Is the can_queue constant during the driver's life, or is it possible
to manipulate it (aac_change_queue_depth)?

Tomas

> ---
>  drivers/scsi/aacraid/commsup.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
> index b257d3b..9533f47 100644
> --- a/drivers/scsi/aacraid/commsup.c
> +++ b/drivers/scsi/aacraid/commsup.c
> @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
>  
>  void aac_fib_map_free(struct aac_dev *dev)
>  {
> -	pci_free_consistent(dev->pdev,
> -	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB),
> -	  dev->hw_fib_va, dev->hw_fib_pa);
> +	if (dev->hw_fib_va && dev->max_fib_size) {
> +		pci_free_consistent(dev->pdev,
> +		(dev->max_fib_size *
> +		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
> +		dev->hw_fib_va, dev->hw_fib_pa);
> +	}
>  	dev->hw_fib_va = NULL;
>  	dev->hw_fib_pa = 0;
>  }

RE: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free

[Raghava Aditya Renukunta]

Hello Tomas,


> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Friday, December 4, 2015 6:35 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free
> 
> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> >
> > aac_fib_map_free() calls pci_free_consistent() without checking that
> > dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
> > are indeed NULL/0, this will result in a hang as pci_free_consistent()
> > will attempt to invalidate cache for the entire 64-bit address space
> > (which would take a very long time).
> >
> > Fixed by adding a check to make sure that dev->hw_fib_va and
> > dev->max_fib_size are not NULL and 0 respectively.
> >
> > Signed-off-by: Raghava Aditya Renukunta
> <raghavaaditya.renukunta@pmcs.com>
> 
> Reviewed-by: Tomas Henzl <thenzl@redhat.com>
> 
> Is the can_queue constant during the driver's life, or is it possible
> to manipulate it (aac_change_queue_depth)?
> 
> Tomas

can_queue is only changed in aac_init_adapter. Do you want to save 
 (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB) in a variable
So that the whole can_queue dereference need not be used?

Regards,
Raghava Aditya

> > ---
> >  drivers/scsi/aacraid/commsup.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/scsi/aacraid/commsup.c
> b/drivers/scsi/aacraid/commsup.c
> > index b257d3b..9533f47 100644
> > --- a/drivers/scsi/aacraid/commsup.c
> > +++ b/drivers/scsi/aacraid/commsup.c
> > @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
> >
> >  void aac_fib_map_free(struct aac_dev *dev)
> >  {
> > -	pci_free_consistent(dev->pdev,
> > -	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue +
> AAC_NUM_MGT_FIB),
> > -	  dev->hw_fib_va, dev->hw_fib_pa);
> > +	if (dev->hw_fib_va && dev->max_fib_size) {
> > +		pci_free_consistent(dev->pdev,
> > +		(dev->max_fib_size *
> > +		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
> > +		dev->hw_fib_va, dev->hw_fib_pa);
> > +	}
> >  	dev->hw_fib_va = NULL;
> >  	dev->hw_fib_pa = 0;
> >  }

Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free

[Tomas Henzl]

On 5.12.2015 01:40, Raghava Aditya Renukunta wrote:
> Hello Tomas,
>
>
>> -----Original Message-----
>> From: Tomas Henzl [mailto:thenzl@redhat.com]
>> Sent: Friday, December 4, 2015 6:35 AM
>> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
>> scsi@vger.kernel.org
>> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
>> aacraid@pmc-sierra.com; Rich Bono
>> Subject: Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free
>>
>> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
>>> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
>>>
>>> aac_fib_map_free() calls pci_free_consistent() without checking that
>>> dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
>>> are indeed NULL/0, this will result in a hang as pci_free_consistent()
>>> will attempt to invalidate cache for the entire 64-bit address space
>>> (which would take a very long time).
>>>
>>> Fixed by adding a check to make sure that dev->hw_fib_va and
>>> dev->max_fib_size are not NULL and 0 respectively.
>>>
>>> Signed-off-by: Raghava Aditya Renukunta
>> <raghavaaditya.renukunta@pmcs.com>
>>
>> Reviewed-by: Tomas Henzl <thenzl@redhat.com>
>>
>> Is the can_queue constant during the driver's life, or is it possible
>> to manipulate it (aac_change_queue_depth)?
>>
>> Tomas
> can_queue is only changed in aac_init_adapter. Do you want to save 
>  (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB) in a variable
> So that the whole can_queue dereference need not be used?

It's fine as it is, (I thought it may change elsewhere in your code
but now I think that I was wrong).

--tm

>
> Regards,
> Raghava Aditya
>
>>> ---
>>>  drivers/scsi/aacraid/commsup.c | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/aacraid/commsup.c
>> b/drivers/scsi/aacraid/commsup.c
>>> index b257d3b..9533f47 100644
>>> --- a/drivers/scsi/aacraid/commsup.c
>>> +++ b/drivers/scsi/aacraid/commsup.c
>>> @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
>>>
>>>  void aac_fib_map_free(struct aac_dev *dev)
>>>  {
>>> -	pci_free_consistent(dev->pdev,
>>> -	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue +
>> AAC_NUM_MGT_FIB),
>>> -	  dev->hw_fib_va, dev->hw_fib_pa);
>>> +	if (dev->hw_fib_va && dev->max_fib_size) {
>>> +		pci_free_consistent(dev->pdev,
>>> +		(dev->max_fib_size *
>>> +		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
>>> +		dev->hw_fib_va, dev->hw_fib_pa);
>>> +	}
>>>  	dev->hw_fib_va = NULL;
>>>  	dev->hw_fib_pa = 0;
>>>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free

[Raghava Aditya Renukunta]

Hello Tomas,

> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Monday, December 7, 2015 6:06 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free
> 
> On 5.12.2015 01:40, Raghava Aditya Renukunta wrote:
> > Hello Tomas,
> >
> >
> >> -----Original Message-----
> >> From: Tomas Henzl [mailto:thenzl@redhat.com]
> >> Sent: Friday, December 4, 2015 6:35 AM
> >> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> >> scsi@vger.kernel.org
> >> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> >> aacraid@pmc-sierra.com; Rich Bono
> >> Subject: Re: [PATCH 04/10] aacraid: Fix memory leak in aac_fib_map_free
> >>
> >> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> >>> From: Raghava Aditya Renukunta
> <raghavaaditya.renukunta@pmcs.com>
> >>>
> >>> aac_fib_map_free() calls pci_free_consistent() without checking that
> >>> dev->hw_fib_va is not NULL and dev->max_fib_size is not zero.If they
> >>> are indeed NULL/0, this will result in a hang as pci_free_consistent()
> >>> will attempt to invalidate cache for the entire 64-bit address space
> >>> (which would take a very long time).
> >>>
> >>> Fixed by adding a check to make sure that dev->hw_fib_va and
> >>> dev->max_fib_size are not NULL and 0 respectively.
> >>>
> >>> Signed-off-by: Raghava Aditya Renukunta
> >> <raghavaaditya.renukunta@pmcs.com>
> >>
> >> Reviewed-by: Tomas Henzl <thenzl@redhat.com>
> >>
> >> Is the can_queue constant during the driver's life, or is it possible
> >> to manipulate it (aac_change_queue_depth)?
> >>
> >> Tomas
> > can_queue is only changed in aac_init_adapter. Do you want to save
> >  (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB) in a variable
> > So that the whole can_queue dereference need not be used?
> 
> It's fine as it is, (I thought it may change elsewhere in your code
> but now I think that I was wrong).
> 
> --tm

I will leave it as it is then.


> 
> >
> > Regards,
> > Raghava Aditya
> >
> >>> ---
> >>>  drivers/scsi/aacraid/commsup.c | 9 ++++++---
> >>>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/scsi/aacraid/commsup.c
> >> b/drivers/scsi/aacraid/commsup.c
> >>> index b257d3b..9533f47 100644
> >>> --- a/drivers/scsi/aacraid/commsup.c
> >>> +++ b/drivers/scsi/aacraid/commsup.c
> >>> @@ -83,9 +83,12 @@ static int fib_map_alloc(struct aac_dev *dev)
> >>>
> >>>  void aac_fib_map_free(struct aac_dev *dev)
> >>>  {
> >>> -	pci_free_consistent(dev->pdev,
> >>> -	  dev->max_fib_size * (dev->scsi_host_ptr->can_queue +
> >> AAC_NUM_MGT_FIB),
> >>> -	  dev->hw_fib_va, dev->hw_fib_pa);
> >>> +	if (dev->hw_fib_va && dev->max_fib_size) {
> >>> +		pci_free_consistent(dev->pdev,
> >>> +		(dev->max_fib_size *
> >>> +		(dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB)),
> >>> +		dev->hw_fib_va, dev->hw_fib_pa);
> >>> +	}
> >>>  	dev->hw_fib_va = NULL;
> >>>  	dev->hw_fib_pa = 0;
> >>>  }
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/10] aacraid: Set correct msix count for EEH recovery

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

During EEH recovery number of online CPU's might change thereby changing
the number of MSIx vectors. Since each fib is allocated to a vector,
changes in the number of vectors causes fib to be sent thru invalid
vectors.In addition the correct number of MSIx vectors is not
updated in the INIT struct sent to the controller, when it is
reinitialized.

Fixed by reassigning vectors to fibs based on the updated number of MSIx
vectors and updating the INIT structure before sending to controller.

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/linit.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 0147210..f88f1132 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1355,6 +1355,7 @@ static int aac_acquire_resources(struct aac_dev *dev)
 	int i, j;
 	int instance = dev->id;
 	const char *name = dev->name;
+	int vector = 0;
 	unsigned long status;
 	/*
 	 *	First clear out all interrupts.  Then enable the one's that we
@@ -1409,9 +1410,31 @@ static int aac_acquire_resources(struct aac_dev *dev)
 	}
 
 	aac_adapter_enable_int(dev);
+	/*max msix may change  after EEH
+	 * Re-assign vectors to fibs
+	 */
+	 for (i = 0;
+		i < (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB);
+		i++) {
+		if ((dev->max_msix == 1) ||
+		   (i > ((dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB - 1)
+			- dev->vector_cap))) {
+			dev->fibs[i].vector_no = 0;
+		} else {
+			dev->fibs[i].vector_no = vector;
+			vector++;
+			if (vector == dev->max_msix)
+				vector = 1;
+		}
+	}
 
-	if (!dev->sync_mode)
+	if (!dev->sync_mode) {
+		/* After EEH recovery or suspend resume, max_msix count
+		 * may change, therfore updating in init as well.
+		 */
 		aac_adapter_start(dev);
+		dev->init->Sa_MSIXVectors = cpu_to_le32(dev->max_msix);
+	}
 	return 0;
 
 error_iounmap:
-- 
1.9.1

Re: [PATCH 05/10] aacraid: Set correct msix count for EEH recovery

[Johannes Thumshirn]

Hi Raghava,

On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> During EEH recovery number of online CPU's might change thereby changing
> the number of MSIx vectors. Since each fib is allocated to a vector,
> changes in the number of vectors causes fib to be sent thru invalid
> vectors.In addition the correct number of MSIx vectors is not
> updated in the INIT struct sent to the controller, when it is
> reinitialized.
> 
> Fixed by reassigning vectors to fibs based on the updated number of MSIx
> vectors and updating the INIT structure before sending to controller.
> 

[...]
 
> -	if (!dev->sync_mode)
> +	if (!dev->sync_mode) {
> +		/* After EEH recovery or suspend resume, max_msix count
> +		 * may change, therfore updating in init as well.
> +		 */
>  		aac_adapter_start(dev);
> +		dev->init->Sa_MSIXVectors = cpu_to_le32(dev->max_msix);
> +	}
>  	return 0;
>  
>  error_iounmap:


This is for EEH and suspend/resume isn't it? If it fixes MSI-X vector
calculation for suspend/resume as well you probably should tag it for inclusion
into stable as well.

Thanks,
	Johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 05/10] aacraid: Set correct msix count for EEH recovery

[Raghava Aditya Renukunta]

Hello Johannes,

> -----Original Message-----
> From: Johannes Thumshirn [mailto:jthumshirn@suse.de]
> Sent: Wednesday, December 2, 2015 2:27 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 05/10] aacraid: Set correct msix count for EEH recovery
> 
> Hi Raghava,
> 
> On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> >
> > During EEH recovery number of online CPU's might change thereby
> > changing the number of MSIx vectors. Since each fib is allocated to a
> > vector, changes in the number of vectors causes fib to be sent thru
> > invalid vectors.In addition the correct number of MSIx vectors is not
> > updated in the INIT struct sent to the controller, when it is
> > reinitialized.
> >
> > Fixed by reassigning vectors to fibs based on the updated number of
> > MSIx vectors and updating the INIT structure before sending to controller.
> >
> 
> [...]
> 
> > -	if (!dev->sync_mode)
> > +	if (!dev->sync_mode) {
> > +		/* After EEH recovery or suspend resume, max_msix count
> > +		 * may change, therfore updating in init as well.
> > +		 */
> >  		aac_adapter_start(dev);
> > +		dev->init->Sa_MSIXVectors = cpu_to_le32(dev->max_msix);
> > +	}
> >  	return 0;
> >
> >  error_iounmap:
> 
> 
> This is for EEH and suspend/resume isn't it? If it fixes MSI-X vector calculation
> for suspend/resume as well you probably should tag it for inclusion into
> stable as well.

Yes this is for EEH and suspend and resume. It does fix MSI-X vector calculation for
Suspend/resume. I will tag it into for inclusion to stable.
 
Thank you,
Raghava Aditya

Re: [PATCH 05/10] aacraid: Set correct msix count for EEH recovery

[Tomas Henzl]

On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
>
> During EEH recovery number of online CPU's might change thereby changing
> the number of MSIx vectors. Since each fib is allocated to a vector,
> changes in the number of vectors causes fib to be sent thru invalid
> vectors.In addition the correct number of MSIx vectors is not
> updated in the INIT struct sent to the controller, when it is
> reinitialized.
>
> Fixed by reassigning vectors to fibs based on the updated number of MSIx
> vectors and updating the INIT structure before sending to controller.
>
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/linit.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> index 0147210..f88f1132 100644
> --- a/drivers/scsi/aacraid/linit.c
> +++ b/drivers/scsi/aacraid/linit.c
> @@ -1355,6 +1355,7 @@ static int aac_acquire_resources(struct aac_dev *dev)
>  	int i, j;
>  	int instance = dev->id;
>  	const char *name = dev->name;
> +	int vector = 0;
>  	unsigned long status;
>  	/*
>  	 *	First clear out all interrupts.  Then enable the one's that we
> @@ -1409,9 +1410,31 @@ static int aac_acquire_resources(struct aac_dev *dev)
>  	}
>  
>  	aac_adapter_enable_int(dev);
> +	/*max msix may change  after EEH
> +	 * Re-assign vectors to fibs
> +	 */
> +	 for (i = 0;
> +		i < (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB);
> +		i++) {
> +		if ((dev->max_msix == 1) ||
> +		   (i > ((dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB - 1)
> +			- dev->vector_cap))) {
> +			dev->fibs[i].vector_no = 0;
> +		} else {
> +			dev->fibs[i].vector_no = vector;
> +			vector++;
> +			if (vector == dev->max_msix)
> +				vector = 1;
> +		}
> +	}

The above hunk added code looks identical to the part you have just added
in 02/10 "aacraid: Fix RRQ overload" could you make this to function ?
Thanks, tomash

>  
> -	if (!dev->sync_mode)
> +	if (!dev->sync_mode) {
> +		/* After EEH recovery or suspend resume, max_msix count
> +		 * may change, therfore updating in init as well.
> +		 */
>  		aac_adapter_start(dev);
> +		dev->init->Sa_MSIXVectors = cpu_to_le32(dev->max_msix);
> +	}
>  	return 0;
>  
>  error_iounmap:

RE: [PATCH 05/10] aacraid: Set correct msix count for EEH recovery

[Raghava Aditya Renukunta]

Hello Tomas,

> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Friday, December 4, 2015 6:10 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 05/10] aacraid: Set correct msix count for EEH recovery
> 
> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> >
> > During EEH recovery number of online CPU's might change thereby
> changing
> > the number of MSIx vectors. Since each fib is allocated to a vector,
> > changes in the number of vectors causes fib to be sent thru invalid
> > vectors.In addition the correct number of MSIx vectors is not
> > updated in the INIT struct sent to the controller, when it is
> > reinitialized.
> >
> > Fixed by reassigning vectors to fibs based on the updated number of MSIx
> > vectors and updating the INIT structure before sending to controller.
> >
> > Signed-off-by: Raghava Aditya Renukunta
> <raghavaaditya.renukunta@pmcs.com>
> > ---
> >  drivers/scsi/aacraid/linit.c | 25 ++++++++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> > index 0147210..f88f1132 100644
> > --- a/drivers/scsi/aacraid/linit.c
> > +++ b/drivers/scsi/aacraid/linit.c
> > @@ -1355,6 +1355,7 @@ static int aac_acquire_resources(struct aac_dev
> *dev)
> >  	int i, j;
> >  	int instance = dev->id;
> >  	const char *name = dev->name;
> > +	int vector = 0;
> >  	unsigned long status;
> >  	/*
> >  	 *	First clear out all interrupts.  Then enable the one's that we
> > @@ -1409,9 +1410,31 @@ static int aac_acquire_resources(struct aac_dev
> *dev)
> >  	}
> >
> >  	aac_adapter_enable_int(dev);
> > +	/*max msix may change  after EEH
> > +	 * Re-assign vectors to fibs
> > +	 */
> > +	 for (i = 0;
> > +		i < (dev->scsi_host_ptr->can_queue +
> AAC_NUM_MGT_FIB);
> > +		i++) {
> > +		if ((dev->max_msix == 1) ||
> > +		   (i > ((dev->scsi_host_ptr->can_queue +
> AAC_NUM_MGT_FIB - 1)
> > +			- dev->vector_cap))) {
> > +			dev->fibs[i].vector_no = 0;
> > +		} else {
> > +			dev->fibs[i].vector_no = vector;
> > +			vector++;
> > +			if (vector == dev->max_msix)
> > +				vector = 1;
> > +		}
> > +	}
> 
> The above hunk added code looks identical to the part you have just added
> in 02/10 "aacraid: Fix RRQ overload" could you make this to function ?
> Thanks, tomash

Yes, I will make the necessary changes.
> >
> > -	if (!dev->sync_mode)
> > +	if (!dev->sync_mode) {
> > +		/* After EEH recovery or suspend resume, max_msix count
> > +		 * may change, therfore updating in init as well.
> > +		 */
> >  		aac_adapter_start(dev);
> > +		dev->init->Sa_MSIXVectors = cpu_to_le32(dev->max_msix);
> > +	}
> >  	return 0;
> >
> >  error_iounmap:

Regards,
Raghava Aditya

[PATCH 06/10] aacraid: Fundamental reset support for Series 7

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

Series 7 does not support PCI hot reset used by EEH.

Enabled fundamental reset only for Series 7

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/linit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index f88f1132..6912efd 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1135,6 +1135,12 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	u64 dmamask;
 	extern int aac_sync_mode;
 
+	/*
+	 * Only series 7 needs freset.
+	 */
+	 if (pdev->device == PMC_DEVICE_S7)
+		pdev->needs_freset = 1;
+
 	list_for_each_entry(aac, &aac_devices, entry) {
 		if (aac->id > unique_id)
 			break;
-- 
1.9.1

Re: [PATCH 06/10] aacraid: Fundamental reset support for Series 7

[Johannes Thumshirn]

On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> Series 7 does not support PCI hot reset used by EEH.
> 
> Enabled fundamental reset only for Series 7
> 
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/linit.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> index f88f1132..6912efd 100644
> --- a/drivers/scsi/aacraid/linit.c
> +++ b/drivers/scsi/aacraid/linit.c
> @@ -1135,6 +1135,12 @@ static int aac_probe_one(struct pci_dev *pdev, const
> struct pci_device_id *id)
>  	u64 dmamask;
>  	extern int aac_sync_mode;
>  
> +	/*
> +	 * Only series 7 needs freset.
> +	 */
> +	 if (pdev->device == PMC_DEVICE_S7)
> +		pdev->needs_freset = 1;
> +
>  	list_for_each_entry(aac, &aac_devices, entry) {
>  		if (aac->id > unique_id)
>  			break;

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/10] aacraid: Fix AIF triggered IOP_RESET

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

while driver removal is in progress or PCI shutdown is invoked, driver
kills AIF aacraid thread, but IOCTL requests from the management tools
re-start AIF thread leading to IOP_RESET.

Fixed by setting adapter_shutdown flag when PCI shutdown is invoked.

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/linit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 6912efd..3a4dbe7 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1454,6 +1454,7 @@ static int aac_suspend(struct pci_dev *pdev, pm_message_t state)
 	struct aac_dev *aac = (struct aac_dev *)shost->hostdata;
 
 	scsi_block_requests(shost);
+	aac->adapter_shutdown = 1;
 	aac_send_shutdown(aac);
 
 	aac_release_resources(aac);
-- 
1.9.1

Re: [PATCH 07/10] aacraid: Fix AIF triggered IOP_RESET

[Johannes Thumshirn]

Hi Raghava,

On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> while driver removal is in progress or PCI shutdown is invoked, driver
> kills AIF aacraid thread, but IOCTL requests from the management tools
> re-start AIF thread leading to IOP_RESET.
> 
> Fixed by setting adapter_shutdown flag when PCI shutdown is invoked.
> 
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/linit.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> index 6912efd..3a4dbe7 100644
> --- a/drivers/scsi/aacraid/linit.c
> +++ b/drivers/scsi/aacraid/linit.c
> @@ -1454,6 +1454,7 @@ static int aac_suspend(struct pci_dev *pdev,
> pm_message_t state)
>  	struct aac_dev *aac = (struct aac_dev *)shost->hostdata;
>  
>  	scsi_block_requests(shost);
> +	aac->adapter_shutdown = 1;
>  	aac_send_shutdown(aac);
>  
>  	aac_release_resources(aac);

I don't quite undestand that, the following is from aac_send_shutdown():


229         /* FIB should be freed only after getting the response from the F/W
*/
230         if (status != -ERESTARTSYS)
231                 aac_fib_free(fibctx);
232         dev->adapter_shutdown = 1;
233         if ((dev->pdev->device == PMC_DEVICE_S7 ||
234              dev->pdev->device == PMC_DEVICE_S8 ||



in line 232 you're already setting the adapter shutdown flag, why do you need
to pre-set it before calling aac_send_shutdown()? 

Thanks,
	Johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 07/10] aacraid: Fix AIF triggered IOP_RESET

[Raghava Aditya Renukunta]

Hello Johannes,

> -----Original Message-----
> From: Johannes Thumshirn [mailto:jthumshirn@suse.de]
> Sent: Wednesday, December 2, 2015 2:01 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 07/10] aacraid: Fix AIF triggered IOP_RESET
> 
> Hi Raghava,
> 
> On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> >
> > while driver removal is in progress or PCI shutdown is invoked, driver
> > kills AIF aacraid thread, but IOCTL requests from the management tools
> > re-start AIF thread leading to IOP_RESET.
> >
> > Fixed by setting adapter_shutdown flag when PCI shutdown is invoked.
> >
> > Signed-off-by: Raghava Aditya Renukunta
> > <raghavaaditya.renukunta@pmcs.com>
> > ---
> >  drivers/scsi/aacraid/linit.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/scsi/aacraid/linit.c
> > b/drivers/scsi/aacraid/linit.c index 6912efd..3a4dbe7 100644
> > --- a/drivers/scsi/aacraid/linit.c
> > +++ b/drivers/scsi/aacraid/linit.c
> > @@ -1454,6 +1454,7 @@ static int aac_suspend(struct pci_dev *pdev,
> > pm_message_t state)
> >  	struct aac_dev *aac = (struct aac_dev *)shost->hostdata;
> >
> >  	scsi_block_requests(shost);
> > +	aac->adapter_shutdown = 1;
> >  	aac_send_shutdown(aac);
> >
> >  	aac_release_resources(aac);
> 
> I don't quite undestand that, the following is from aac_send_shutdown():
> 
> 
> 229         /* FIB should be freed only after getting the response from the F/W
> */
> 230         if (status != -ERESTARTSYS)
> 231                 aac_fib_free(fibctx);
> 232         dev->adapter_shutdown = 1;
> 233         if ((dev->pdev->device == PMC_DEVICE_S7 ||
> 234              dev->pdev->device == PMC_DEVICE_S8 ||
> 
> 
> 
> in line 232 you're already setting the adapter shutdown flag, why do you
> need to pre-set it before calling aac_send_shutdown()?

Originally the problem was that dev->adapter_shutdown was set after the
Shutdown command was sent to the controller, leading to a race
condition  If after the controller was shut down and before
 dev->adapter_shutdown  was set , the driver received  a AIF
 command from management tool , it  would still be sent to the 
controller in aac_cfg_ioctl() leading to a  IOP_RESET.

A fix would  be to set dev->adapter_shutdown at the very 
beginning of aac_send_shutdown().

> 
> Thanks,
> 	Johannes

Thank you,
	Raghava Aditya

Re: [PATCH 07/10] aacraid: Fix AIF triggered IOP_RESET

[Johannes Thumshirn]

Hi Raghava,

On Wed, 2015-12-02 at 22:29 +0000, Raghava Aditya Renukunta wrote:
> Hello Johannes,
> 
> > -----Original Message-----

[...]

> Originally the problem was that dev->adapter_shutdown was set after the
> Shutdown command was sent to the controller, leading to a race
> condition  If after the controller was shut down and before
>  dev->adapter_shutdown  was set , the driver received  a AIF
>  command from management tool , it  would still be sent to the 
> controller in aac_cfg_ioctl() leading to a  IOP_RESET.
> 
> A fix would  be to set dev->adapter_shutdown at the very 
> beginning of aac_send_shutdown().


That's what I though as well

	Johannes


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/10] aacraid: Disable device ID wildcard

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

Added module parameter that disables device ID wild card binding.

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/aachba.c  | 5 +++++
 drivers/scsi/aacraid/aacraid.h | 1 +
 drivers/scsi/aacraid/linit.c   | 5 +++++
 3 files changed, 11 insertions(+)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 06cbab8..87f4f21 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -315,6 +315,11 @@ MODULE_PARM_DESC(wwn, "Select a WWN type for the arrays:\n"
 	"\t1 - Array Meta Data Signature (default)\n"
 	"\t2 - Adapter Serial Number");
 
+int aac_disable_device_id_wildcards;
+module_param_named(disable_device_id_wildcards,
+	aac_disable_device_id_wildcards, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(disable_device_id_wildcards,
+	"Disable device ID wildcards");
 
 static inline int aac_valid_context(struct scsi_cmnd *scsicmd,
 		struct fib *fibptr) {
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 594de5f..7708a2c 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -2177,3 +2177,4 @@ extern int aac_commit;
 extern int update_interval;
 extern int check_interval;
 extern int aac_check_reset;
+extern int aac_disable_device_id_wildcards;
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 3a4dbe7..2094842 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1135,6 +1135,11 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	u64 dmamask;
 	extern int aac_sync_mode;
 
+	if (aac_disable_device_id_wildcards &&
+		id->subvendor == PCI_ANY_ID &&
+		id->subdevice == PCI_ANY_ID)
+		return -ENODEV;
+
 	/*
 	 * Only series 7 needs freset.
 	 */
-- 
1.9.1

Re: [PATCH 08/10] aacraid: Disable device ID wildcard

[Johannes Thumshirn]

On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> Added module parameter that disables device ID wild card binding.
> 
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/aachba.c  | 5 +++++
>  drivers/scsi/aacraid/aacraid.h | 1 +
>  drivers/scsi/aacraid/linit.c   | 5 +++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index 06cbab8..87f4f21 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -315,6 +315,11 @@ MODULE_PARM_DESC(wwn, "Select a WWN type for the
> arrays:\n"
>  	"\t1 - Array Meta Data Signature (default)\n"
>  	"\t2 - Adapter Serial Number");
>  
> +int aac_disable_device_id_wildcards;
> +module_param_named(disable_device_id_wildcards,
> +	aac_disable_device_id_wildcards, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(disable_device_id_wildcards,
> +	"Disable device ID wildcards");
>  
>  static inline int aac_valid_context(struct scsi_cmnd *scsicmd,
>  		struct fib *fibptr) {
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> index 594de5f..7708a2c 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -2177,3 +2177,4 @@ extern int aac_commit;
>  extern int update_interval;
>  extern int check_interval;
>  extern int aac_check_reset;
> +extern int aac_disable_device_id_wildcards;
> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> index 3a4dbe7..2094842 100644
> --- a/drivers/scsi/aacraid/linit.c
> +++ b/drivers/scsi/aacraid/linit.c
> @@ -1135,6 +1135,11 @@ static int aac_probe_one(struct pci_dev *pdev, const
> struct pci_device_id *id)
>  	u64 dmamask;
>  	extern int aac_sync_mode;
>  
> +	if (aac_disable_device_id_wildcards &&
> +		id->subvendor == PCI_ANY_ID &&
> +		id->subdevice == PCI_ANY_ID)
> +		return -ENODEV;
> +
>  	/*
>  	 * Only series 7 needs freset.
>  	 */

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/10] aacraid: Disable device ID wildcard

[Tomas Henzl]

On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
>
> Added module parameter that disables device ID wild card binding.

What is the use case for this?

It looks like something which could be solved in the modprobe config,
without a module option.

Cheers,
Tomas

>
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/aachba.c  | 5 +++++
>  drivers/scsi/aacraid/aacraid.h | 1 +
>  drivers/scsi/aacraid/linit.c   | 5 +++++
>  3 files changed, 11 insertions(+)
>
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index 06cbab8..87f4f21 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -315,6 +315,11 @@ MODULE_PARM_DESC(wwn, "Select a WWN type for the arrays:\n"
>  	"\t1 - Array Meta Data Signature (default)\n"
>  	"\t2 - Adapter Serial Number");
>  
> +int aac_disable_device_id_wildcards;
> +module_param_named(disable_device_id_wildcards,
> +	aac_disable_device_id_wildcards, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(disable_device_id_wildcards,
> +	"Disable device ID wildcards");
>  
>  static inline int aac_valid_context(struct scsi_cmnd *scsicmd,
>  		struct fib *fibptr) {
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> index 594de5f..7708a2c 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -2177,3 +2177,4 @@ extern int aac_commit;
>  extern int update_interval;
>  extern int check_interval;
>  extern int aac_check_reset;
> +extern int aac_disable_device_id_wildcards;
> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> index 3a4dbe7..2094842 100644
> --- a/drivers/scsi/aacraid/linit.c
> +++ b/drivers/scsi/aacraid/linit.c
> @@ -1135,6 +1135,11 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
>  	u64 dmamask;
>  	extern int aac_sync_mode;
>  
> +	if (aac_disable_device_id_wildcards &&
> +		id->subvendor == PCI_ANY_ID &&
> +		id->subdevice == PCI_ANY_ID)
> +		return -ENODEV;
> +
>  	/*
>  	 * Only series 7 needs freset.
>  	 */

RE: [PATCH 08/10] aacraid: Disable device ID wildcard

[Raghava Aditya Renukunta]

Hello Tomas,


> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Thursday, December 3, 2015 7:55 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 08/10] aacraid: Disable device ID wildcard
> 
> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> >
> > Added module parameter that disables device ID wild card binding.
> 
> What is the use case for this?
> 
> It looks like something which could be solved in the modprobe config,
> without a module option.

This will enable us to prevent aacraid from loading for PCI devices that match
device ID wildcards. Enabling us to use say a new driver for future devices.

> Cheers,
> Tomas
> 
> >
> > Signed-off-by: Raghava Aditya Renukunta
> <raghavaaditya.renukunta@pmcs.com>
> > ---
> >  drivers/scsi/aacraid/aachba.c  | 5 +++++
> >  drivers/scsi/aacraid/aacraid.h | 1 +
> >  drivers/scsi/aacraid/linit.c   | 5 +++++
> >  3 files changed, 11 insertions(+)
> >
> > diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> > index 06cbab8..87f4f21 100644
> > --- a/drivers/scsi/aacraid/aachba.c
> > +++ b/drivers/scsi/aacraid/aachba.c
> > @@ -315,6 +315,11 @@ MODULE_PARM_DESC(wwn, "Select a WWN type
> for the arrays:\n"
> >  	"\t1 - Array Meta Data Signature (default)\n"
> >  	"\t2 - Adapter Serial Number");
> >
> > +int aac_disable_device_id_wildcards;
> > +module_param_named(disable_device_id_wildcards,
> > +	aac_disable_device_id_wildcards, int, S_IRUGO | S_IWUSR);
> > +MODULE_PARM_DESC(disable_device_id_wildcards,
> > +	"Disable device ID wildcards");
> >
> >  static inline int aac_valid_context(struct scsi_cmnd *scsicmd,
> >  		struct fib *fibptr) {
> > diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> > index 594de5f..7708a2c 100644
> > --- a/drivers/scsi/aacraid/aacraid.h
> > +++ b/drivers/scsi/aacraid/aacraid.h
> > @@ -2177,3 +2177,4 @@ extern int aac_commit;
> >  extern int update_interval;
> >  extern int check_interval;
> >  extern int aac_check_reset;
> > +extern int aac_disable_device_id_wildcards;
> > diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> > index 3a4dbe7..2094842 100644
> > --- a/drivers/scsi/aacraid/linit.c
> > +++ b/drivers/scsi/aacraid/linit.c
> > @@ -1135,6 +1135,11 @@ static int aac_probe_one(struct pci_dev *pdev,
> const struct pci_device_id *id)
> >  	u64 dmamask;
> >  	extern int aac_sync_mode;
> >
> > +	if (aac_disable_device_id_wildcards &&
> > +		id->subvendor == PCI_ANY_ID &&
> > +		id->subdevice == PCI_ANY_ID)
> > +		return -ENODEV;
> > +
> >  	/*
> >  	 * Only series 7 needs freset.
> >  	 */

Regards,
Raghava Aditya

Re: [PATCH 08/10] aacraid: Disable device ID wildcard

[Christoph Hellwig]

On Thu, Dec 03, 2015 at 09:32:18PM +0000, Raghava Aditya Renukunta wrote:
> This will enable us to prevent aacraid from loading for PCI devices that match
> device ID wildcards. Enabling us to use say a new driver for future devices.

This looks like a bogus reason.  The same PCI ID should always be
compatible and mathed by the same driver.  Even if you add a new driver
to expose additional feature and break these semantics there is no point
to do a) reject them conditionally on a module option and b) do this
before said driver is merged.

RE: [PATCH 08/10] aacraid: Disable device ID wildcard

[Raghava Aditya Renukunta]

Hello Christoph,

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Friday, December 4, 2015 12:33 AM
> To: Raghava Aditya Renukunta
> Cc: Tomas Henzl; JBottomley@Parallels.com; linux-scsi@vger.kernel.org;
> Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 08/10] aacraid: Disable device ID wildcard
> 
> On Thu, Dec 03, 2015 at 09:32:18PM +0000, Raghava Aditya Renukunta wrote:
> > This will enable us to prevent aacraid from loading for PCI devices that
> match
> > device ID wildcards. Enabling us to use say a new driver for future devices.
> 
> This looks like a bogus reason.  The same PCI ID should always be
> compatible and mathed by the same driver.  Even if you add a new driver
> to expose additional feature and break these semantics there is no point
> to do a) reject them conditionally on a module option and b) do this
> before said driver is merged.

I have spoken with my team and  it does make more sense to submit
it when the new driver is merged. I will withdraw this patch.

Regards,
Raghava Aditya

[PATCH 09/10] aacraid: Fix character device re-initialization

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

During EEH PCI hotplug activity kernel unloads and loads the driver,
causing character device to be unregistered(aac_remove_one).When the
driver is loaded back using aac_probe_one the character device needs
to be registered again for the AIF management tools to work.

Fixed by adding code to register character device in aac_probe_one if
it is unregistered in aac_remove_one.

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/linit.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 2094842..7142578 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1123,6 +1123,13 @@ static void __aac_shutdown(struct aac_dev * aac)
 	else if (aac->max_msix > 1)
 		pci_disable_msix(aac->pdev);
 }
+static void aac_init_char(void)
+{
+	aac_cfg_major = register_chrdev(0, "aac", &aac_cfg_fops);
+	if (aac_cfg_major < 0) {
+		pr_err("aacraid: unable to register \"aac\" device.\n");
+	}
+}
 
 static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 {
@@ -1185,6 +1192,9 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	shost->max_cmd_len = 16;
 	shost->use_cmd_list = 1;
 
+	if (aac_cfg_major == -2)
+		aac_init_char();
+
 	aac = (struct aac_dev *)shost->hostdata;
 	aac->base_start = pci_resource_start(pdev, 0);
 	aac->scsi_host_ptr = shost;
@@ -1536,7 +1546,7 @@ static void aac_remove_one(struct pci_dev *pdev)
 	pci_disable_device(pdev);
 	if (list_empty(&aac_devices)) {
 		unregister_chrdev(aac_cfg_major, "aac");
-		aac_cfg_major = -1;
+		aac_cfg_major = -2;
 	}
 }
 
@@ -1697,11 +1707,8 @@ static int __init aac_init(void)
 	if (error < 0)
 		return error;
 
-	aac_cfg_major = register_chrdev( 0, "aac", &aac_cfg_fops);
-	if (aac_cfg_major < 0) {
-		printk(KERN_WARNING
-			"aacraid: unable to register \"aac\" device.\n");
-	}
+	aac_init_char();
+
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 09/10] aacraid: Fix character device re-initialization

[Johannes Thumshirn]

Hi Raghava,

On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> During EEH PCI hotplug activity kernel unloads and loads the driver,
> causing character device to be unregistered(aac_remove_one).When the
> driver is loaded back using aac_probe_one the character device needs
> to be registered again for the AIF management tools to work.
> 
> Fixed by adding code to register character device in aac_probe_one if
> it is unregistered in aac_remove_one.
> 
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/linit.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> index 2094842..7142578 100644
> --- a/drivers/scsi/aacraid/linit.c
> +++ b/drivers/scsi/aacraid/linit.c
> @@ -1123,6 +1123,13 @@ static void __aac_shutdown(struct aac_dev * aac)
>  	else if (aac->max_msix > 1)
>  		pci_disable_msix(aac->pdev);
>  }
> +static void aac_init_char(void)
> +{
> +	aac_cfg_major = register_chrdev(0, "aac", &aac_cfg_fops);
> +	if (aac_cfg_major < 0) {
> +		pr_err("aacraid: unable to register \"aac\" device.\n");
> +	}
> +}
>  
>  static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id
> *id)
>  {
> @@ -1185,6 +1192,9 @@ static int aac_probe_one(struct pci_dev *pdev, const
> struct pci_device_id *id)
>  	shost->max_cmd_len = 16;
>  	shost->use_cmd_list = 1;
>  
> +	if (aac_cfg_major == -2)
> +		aac_init_char();
> +
>  	aac = (struct aac_dev *)shost->hostdata;
>  	aac->base_start = pci_resource_start(pdev, 0);
>  	aac->scsi_host_ptr = shost;
> @@ -1536,7 +1546,7 @@ static void aac_remove_one(struct pci_dev *pdev)
>  	pci_disable_device(pdev);
>  	if (list_empty(&aac_devices)) {
>  		unregister_chrdev(aac_cfg_major, "aac");
> -		aac_cfg_major = -1;
> +		aac_cfg_major = -2;

Please add something like
#define AAC_CHARDEV_UNREGISTERED -1
#define AAC_CHARDEV_NEEDS_REINIT -2

so it's obvious what you're doing.

>  	}
>  }
>  
> @@ -1697,11 +1707,8 @@ static int __init aac_init(void)
>  	if (error < 0)
>  		return error;
>  
> -	aac_cfg_major = register_chrdev( 0, "aac", &aac_cfg_fops);
> -	if (aac_cfg_major < 0) {
> -		printk(KERN_WARNING
> -			"aacraid: unable to register \"aac\" device.\n");
> -	}
> +	aac_init_char();
> +
>  
>  	return 0;
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 09/10] aacraid: Fix character device re-initialization

[Raghava Aditya Renukunta]

Hello Johannes,

> -----Original Message-----
> From: Johannes Thumshirn [mailto:jthumshirn@suse.de]
> Sent: Wednesday, December 2, 2015 2:14 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 09/10] aacraid: Fix character device re-initialization
> 
> Hi Raghava,
> 
> On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> >
> > During EEH PCI hotplug activity kernel unloads and loads the driver,
> > causing character device to be unregistered(aac_remove_one).When the
> > driver is loaded back using aac_probe_one the character device needs
> > to be registered again for the AIF management tools to work.
> >
> > Fixed by adding code to register character device in aac_probe_one if
> > it is unregistered in aac_remove_one.
> >
> > Signed-off-by: Raghava Aditya Renukunta
> > <raghavaaditya.renukunta@pmcs.com>
> > ---
> >  drivers/scsi/aacraid/linit.c | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/scsi/aacraid/linit.c
> > b/drivers/scsi/aacraid/linit.c index 2094842..7142578 100644
> > --- a/drivers/scsi/aacraid/linit.c
> > +++ b/drivers/scsi/aacraid/linit.c
> > @@ -1123,6 +1123,13 @@ static void __aac_shutdown(struct aac_dev *
> aac)
> >  	else if (aac->max_msix > 1)
> >  		pci_disable_msix(aac->pdev);
> >  }
> > +static void aac_init_char(void)
> > +{
> > +	aac_cfg_major = register_chrdev(0, "aac", &aac_cfg_fops);
> > +	if (aac_cfg_major < 0) {
> > +		pr_err("aacraid: unable to register \"aac\" device.\n");
> > +	}
> > +}
> >
> >  static int aac_probe_one(struct pci_dev *pdev, const struct
> > pci_device_id
> > *id)
> >  {
> > @@ -1185,6 +1192,9 @@ static int aac_probe_one(struct pci_dev *pdev,
> > const struct pci_device_id *id)
> >  	shost->max_cmd_len = 16;
> >  	shost->use_cmd_list = 1;
> >
> > +	if (aac_cfg_major == -2)
> > +		aac_init_char();
> > +
> >  	aac = (struct aac_dev *)shost->hostdata;
> >  	aac->base_start = pci_resource_start(pdev, 0);
> >  	aac->scsi_host_ptr = shost;
> > @@ -1536,7 +1546,7 @@ static void aac_remove_one(struct pci_dev
> *pdev)
> >  	pci_disable_device(pdev);
> >  	if (list_empty(&aac_devices)) {
> >  		unregister_chrdev(aac_cfg_major, "aac");
> > -		aac_cfg_major = -1;
> > +		aac_cfg_major = -2;
> 
> Please add something like
> #define AAC_CHARDEV_UNREGISTERED -1
> #define AAC_CHARDEV_NEEDS_REINIT -2
> 
> so it's obvious what you're doing.

Thank you for pointing that out, I will make
the necessary changes in the next iteration.

> 
> >  	}
> >  }
> >
> > @@ -1697,11 +1707,8 @@ static int __init aac_init(void)
> >  	if (error < 0)
> >  		return error;
> >
> > -	aac_cfg_major = register_chrdev( 0, "aac", &aac_cfg_fops);
> > -	if (aac_cfg_major < 0) {
> > -		printk(KERN_WARNING
> > -			"aacraid: unable to register \"aac\" device.\n");
> > -	}
> > +	aac_init_char();
> > +
> >
> >  	return 0;
> >  }

Regards,
Raghava Aditya

[PATCH 09/10] aacraid: Fix character device re initialization

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

During EEH PCI hotplug activity kernel unloads and loads the driver,
causing character device to be unregistered(aac_remove_one).When the
driver is loaded back using aac_probe_one the character device needs
to be registered again for the AIF management tools to work.

Fixed by adding code to register character device in aac_probe_one if
it is unregistered in aac_remove_one.

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/linit.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 9076a95..e8c3606 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1123,6 +1123,13 @@ static void __aac_shutdown(struct aac_dev * aac)
 	else if (aac->max_msix > 1)
 		pci_disable_msix(aac->pdev);
 }
+static void aac_init_char(void)
+{
+	aac_cfg_major = register_chrdev(0, "aac", &aac_cfg_fops);
+	if (aac_cfg_major < 0) {
+		pr_err("aacraid: unable to register \"aac\" device.\n");
+	}
+}
 
 static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 {
@@ -1185,6 +1192,9 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	shost->max_cmd_len = 16;
 	shost->use_cmd_list = 1;
 
+	if (aac_cfg_major == -2)
+		aac_init_char();
+
 	aac = (struct aac_dev *)shost->hostdata;
 	aac->base_start = pci_resource_start(pdev, 0);
 	aac->scsi_host_ptr = shost;
@@ -1534,7 +1544,7 @@ static void aac_remove_one(struct pci_dev *pdev)
 	pci_disable_device(pdev);
 	if (list_empty(&aac_devices)) {
 		unregister_chrdev(aac_cfg_major, "aac");
-		aac_cfg_major = -1;
+		aac_cfg_major = -2;
 	}
 }
 
@@ -1695,11 +1705,8 @@ static int __init aac_init(void)
 	if (error < 0)
 		return error;
 
-	aac_cfg_major = register_chrdev( 0, "aac", &aac_cfg_fops);
-	if (aac_cfg_major < 0) {
-		printk(KERN_WARNING
-			"aacraid: unable to register \"aac\" device.\n");
-	}
+	aac_init_char();
+
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 09/10] aacraid: Fix character device re initialization

[Johannes Thumshirn]

Hi Raghava,
On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> During EEH PCI hotplug activity kernel unloads and loads the driver,
> causing character device to be unregistered(aac_remove_one).When the
> driver is loaded back using aac_probe_one the character device needs
> to be registered again for the AIF management tools to work.
> 
> Fixed by adding code to register character device in aac_probe_one if
> it is unregistered in aac_remove_one.
> 
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

This one is duplicate, isn't it?
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 09/10] aacraid: Fix character device re initialization

[Raghava Aditya Renukunta]

Hello Johannes,

> Hi Raghava,
> On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> >
> > During EEH PCI hotplug activity kernel unloads and loads the driver,
> > causing character device to be unregistered(aac_remove_one).When the
> > driver is loaded back using aac_probe_one the character device needs
> > to be registered again for the AIF management tools to work.
> >
> > Fixed by adding code to register character device in aac_probe_one if
> > it is unregistered in aac_remove_one.
> >
> > Signed-off-by: Raghava Aditya Renukunta
> > <raghavaaditya.renukunta@pmcs.com>
> 
> This one is duplicate, isn't it?

Yes this one is a duplicate.

[PATCH 10/10] aacraid: Update driver version

[Raghava Aditya Renukunta]

From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>

Updated diver version to 41052

Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
---
 drivers/scsi/aacraid/aacraid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 7708a2c..bbb94ec 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -62,7 +62,7 @@ enum {
 #define	PMC_GLOBAL_INT_BIT0		0x00000001
 
 #ifndef AAC_DRIVER_BUILD
-# define AAC_DRIVER_BUILD 41010
+# define AAC_DRIVER_BUILD 41052
 # define AAC_DRIVER_BRANCH "-ms"
 #endif
 #define MAXIMUM_NUM_CONTAINERS	32
-- 
1.9.1

Re: [PATCH 10/10] aacraid: Update driver version

[Johannes Thumshirn]

On Tue, 2015-12-01 at 04:39 -0800, Raghava Aditya Renukunta wrote:
> From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> 
> Updated diver version to 41052
> 
> Signed-off-by: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> ---
>  drivers/scsi/aacraid/aacraid.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
> index 7708a2c..bbb94ec 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -62,7 +62,7 @@ enum {
>  #define	PMC_GLOBAL_INT_BIT0		0x00000001
>  
>  #ifndef AAC_DRIVER_BUILD
> -# define AAC_DRIVER_BUILD 41010
> +# define AAC_DRIVER_BUILD 41052
>  # define AAC_DRIVER_BRANCH "-ms"
>  #endif
>  #define MAXIMUM_NUM_CONTAINERS	32

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html