Re: [PATCH 4/7] aacraid: MSI-x support

[Hannes Reinecke <hare@suse.de>]

On 03/17/2015 04:27 PM, Achim Leubner wrote:
> Reviewed-by: Achim Leubner <Achim.Leubner@pmcs.com>
> 
> 
> -----Original Message-----
> From: Mahesh Rajashekhara 
> Sent: Wednesday, March 4, 2015 9:39 AM
> To: JBottomley@Parallels.com; linux-scsi@vger.kernel.org
> Cc: aacraid@pmc-sierra.com; Harry Yang; Achim Leubner; Rajinikanth Pandurangan; Rich Bono; Mahesh Rajashekhara
> Subject: [PATCH 4/7] aacraid: MSI-x support
> 
> Add MSI-x interrupt mode support.
> 
> Signed-off-by: Mahesh Rajashekhara <Mahesh.Rajashekhara@pmcs.com>
> ---
>  drivers/scsi/aacraid/aachba.c   |    6 +-
>  drivers/scsi/aacraid/aacraid.h  |   79 ++++++++-
>  drivers/scsi/aacraid/comminit.c |   86 +++++++++-
>  drivers/scsi/aacraid/commsup.c  |   19 ++-
>  drivers/scsi/aacraid/dpcsup.c   |    9 +-
>  drivers/scsi/aacraid/linit.c    |   18 ++-
>  drivers/scsi/aacraid/src.c      |  365 +++++++++++++++++++++++++++++----------
>  7 files changed, 473 insertions(+), 109 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c index 0819644..eb524e6 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -473,7 +473,7 @@ static void get_container_name_callback(void *context, struct fib * fibptr)
>  	if ((le32_to_cpu(get_name_reply->status) == CT_OK)
>  	 && (get_name_reply->data[0] != '\0')) {
>  		char *sp = get_name_reply->data;
> -		sp[sizeof(((struct aac_get_name_resp *)NULL)->data)-1] = '\0';
> +		sp[sizeof(((struct aac_get_name_resp *)NULL)->data)] = '\0';
>  		while (*sp == ' ')
>  			++sp;
>  		if (*sp) {
> @@ -613,7 +613,9 @@ static void _aac_probe_container1(void * context, struct fib * fibptr)
>  	int status;
>  
>  	dresp = (struct aac_mount *) fib_data(fibptr);
> -	dresp->mnt[0].capacityhigh = 0;
> +	if (!(fibptr->dev->supplement_adapter_info.SupportedOptions2 &
> +	    AAC_OPTION_VARIABLE_BLOCK_SIZE))
> +		dresp->mnt[0].capacityhigh = 0;
>  	if ((le32_to_cpu(dresp->status) != ST_OK) ||
>  	    (le32_to_cpu(dresp->mnt[0].vol) != CT_NONE)) {
>  		_aac_probe_container2(context, fibptr); diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h index 93579f3..c162a65 100644
> --- a/drivers/scsi/aacraid/aacraid.h
> +++ b/drivers/scsi/aacraid/aacraid.h
> @@ -6,11 +6,61 @@
>  #define nblank(x) _nblank(x)[0]
>  
>  #include <linux/interrupt.h>
> +#include <linux/pci.h>
>  
>  /*------------------------------------------------------------------------------
>   *              D E F I N E S
>   *----------------------------------------------------------------------------*/
>  
> +#define AAC_MAX_MSIX		32	/* vectors */
> +#define AAC_PCI_MSI_ENABLE	0x8000
> +
> +enum {
> +	AAC_ENABLE_INTERRUPT	= 0x0,
> +	AAC_DISABLE_INTERRUPT,
> +	AAC_ENABLE_MSIX,
> +	AAC_DISABLE_MSIX,
> +	AAC_CLEAR_AIF_BIT,
> +	AAC_CLEAR_SYNC_BIT,
> +	AAC_ENABLE_INTX
> +};
> +
> +#define AAC_INT_MODE_INTX		(1<<0)
> +#define AAC_INT_MODE_MSI		(1<<1)
> +#define AAC_INT_MODE_AIF		(1<<2)
> +#define AAC_INT_MODE_SYNC		(1<<3)
> +
> +#define AAC_INT_ENABLE_TYPE1_INTX	0xfffffffb
> +#define AAC_INT_ENABLE_TYPE1_MSIX	0xfffffffa
> +#define AAC_INT_DISABLE_ALL		0xffffffff
> +
> +/* Bit definitions in IOA->Host Interrupt Register */
> +#define PMC_TRANSITION_TO_OPERATIONAL	(0x80000000 >> 0)
> +#define PMC_IOARCB_TRANSFER_FAILED	(0x80000000 >> 3)
> +#define PMC_IOA_UNIT_CHECK		(0x80000000 >> 4)
> +#define PMC_NO_HOST_RRQ_FOR_CMD_RESPONSE (0x80000000 >> 5)
> +#define PMC_CRITICAL_IOA_OP_IN_PROGRESS	(0x80000000 >> 6)
> +#define PMC_IOARRIN_LOST		(0x80000000 >> 27)
> +#define PMC_SYSTEM_BUS_MMIO_ERROR	(0x80000000 >> 28)
> +#define PMC_IOA_PROCESSOR_IN_ERROR_STATE (0x80000000 >> 29)
> +#define PMC_HOST_RRQ_VALID		(0x80000000 >> 30)
> +#define PMC_OPERATIONAL_STATUS		(0x80000000 >> 0)
> +#define PMC_ALLOW_MSIX_VECTOR0		(0x80000000 >> 31)
> +
How positively curious.
Typically you define a git mask by shifting _left_, not right.

> +#define PMC_IOA_ERROR_INTERRUPTS	(PMC_IOARCB_TRANSFER_FAILED | \
> +					 PMC_IOA_UNIT_CHECK | \
> +					 PMC_NO_HOST_RRQ_FOR_CMD_RESPONSE | \
> +					 PMC_IOARRIN_LOST | \
> +					 PMC_SYSTEM_BUS_MMIO_ERROR | \
> +					 PMC_IOA_PROCESSOR_IN_ERROR_STATE)
> +
> +#define PMC_ALL_INTERRUPT_BITS		(PMC_IOA_ERROR_INTERRUPTS | \
> +					 PMC_HOST_RRQ_VALID | \
> +					 PMC_TRANSITION_TO_OPERATIONAL | \
> +					 PMC_ALLOW_MSIX_VECTOR0)
> +#define	PMC_GLOBAL_INT_BIT2		0x00000004
> +#define	PMC_GLOBAL_INT_BIT0		0x00000001
> +
>  #ifndef AAC_DRIVER_BUILD
>  # define AAC_DRIVER_BUILD 30300
>  # define AAC_DRIVER_BRANCH "-ms"
> @@ -36,6 +86,7 @@
>  #define CONTAINER_TO_ID(cont)		(cont)
>  #define CONTAINER_TO_LUN(cont)		(0)
>  
> +#define PMC_DEVICE_S6	0x28b
>  #define PMC_DEVICE_S7	0x28c
>  #define PMC_DEVICE_S8	0x28d
>  #define PMC_DEVICE_S9	0x28f
> @@ -434,7 +485,7 @@ enum fib_xfer_state {  struct aac_init  {
>  	__le32	InitStructRevision;
> -	__le32	MiniPortRevision;
> +	__le32	NoOfMSIXVectors;
>  	__le32	fsrev;
>  	__le32	CommHeaderAddress;
>  	__le32	FastIoCommAreaAddress;
> @@ -755,7 +806,8 @@ struct rkt_registers {
>  
>  struct src_mu_registers {
>  				/*	PCI*| Name */
> -	__le32	reserved0[8];	/*	00h | Reserved */
> +	__le32	reserved0[6];	/*	00h | Reserved */
> +	__le32	IOAR[2];	/*	18h | IOA->host interrupt register */
>  	__le32	IDR;		/*	20h | Inbound Doorbell Register */
>  	__le32	IISR;		/*	24h | Inbound Int. Status Register */
>  	__le32	reserved1[3];	/*	28h | Reserved */
> @@ -767,17 +819,18 @@ struct src_mu_registers {
>  	__le32	OMR;		/*	bch | Outbound Message Register */
>  	__le32	IQ_L;		/*  c0h | Inbound Queue (Low address) */
>  	__le32	IQ_H;		/*  c4h | Inbound Queue (High address) */
> +	__le32	ODR_MSI;	/*  c8h | MSI register for sync./AIF */
>  };
>  
>  struct src_registers {
> -	struct src_mu_registers MUnit;	/* 00h - c7h */
> +	struct src_mu_registers MUnit;	/* 00h - cbh */
>  	union {
>  		struct {
> -			__le32 reserved1[130790];	/* c8h - 7fc5fh */
> +			__le32 reserved1[130789];	/* cch - 7fc5fh */
>  			struct src_inbound IndexRegs;	/* 7fc60h */
>  		} tupelo;
>  		struct {
> -			__le32 reserved1[974];		/* c8h - fffh */
> +			__le32 reserved1[973];		/* cch - fffh */
>  			struct src_inbound IndexRegs;	/* 1000h */
>  		} denali;
>  	} u;
> @@ -1028,6 +1081,11 @@ struct aac_bus_info_response {
>  #define AAC_OPT_NEW_COMM_TYPE3		cpu_to_le32(1<<30)
>  #define AAC_OPT_NEW_COMM_TYPE4		cpu_to_le32(1<<31)
>  
> +/* MSIX context */
> +struct aac_msix_ctx {
> +	int		vector_no;
> +	struct aac_dev	*dev;
> +};
>  
>  struct aac_dev
>  {
> @@ -1083,8 +1141,9 @@ struct aac_dev
>  						 * if AAC_COMM_MESSAGE_TYPE1 */
>  
>  	dma_addr_t		host_rrq_pa;	/* phys. address */
> -	u32			host_rrq_idx;	/* index into rrq buffer */
> -
> +	u32			host_rrq_idx[AAC_MAX_MSIX];	/* index into rrq buffer */
> +	atomic_t		rrq_outstanding[AAC_MAX_MSIX];
> +	u32			fibs_pushed_no;
>  	struct pci_dev		*pdev;		/* Our PCI interface */
>  	void *			printfbuf;	/* pointer to buffer used for printf's from the adapter */
>  	void *			comm_addr;	/* Base address of Comm area */
> @@ -1153,6 +1212,11 @@ struct aac_dev
>  	int			sync_mode;
>  	struct fib		*sync_fib;
>  	struct list_head	sync_fib_list;
> +	u32			max_msix;	/* max. MSI-X vectors */
> +	u32			vector_cap;	/* MSI-X vector capab.*/
> +	int			msi_enabled;	/* MSI/MSI-X enabled */
> +	struct msix_entry	msixentry[AAC_MAX_MSIX];
> +	struct aac_msix_ctx	aac_msix[AAC_MAX_MSIX]; /* context */
>  };
>  
>  #define aac_adapter_interrupt(dev) \
> @@ -2035,6 +2099,7 @@ void aac_consumer_free(struct aac_dev * dev, struct aac_queue * q, u32 qnum);  int aac_fib_complete(struct fib * context);  #define fib_data(fibctx) ((void *)(fibctx)->hw_fib_va->data)  struct aac_dev *aac_init_adapter(struct aac_dev *dev);
> +void aac_src_access_devreg(struct aac_dev *dev, int mode);
>  int aac_get_config_status(struct aac_dev *dev, int commit_flag);  int aac_get_containers(struct aac_dev *dev);  int aac_scsi_cmd(struct scsi_cmnd *cmd); diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c index 177b094..29c35c8 100644
> --- a/drivers/scsi/aacraid/comminit.c
> +++ b/drivers/scsi/aacraid/comminit.c
> @@ -43,6 +43,8 @@
>  
>  #include "aacraid.h"
>  
> +static void aac_define_int_mode(struct aac_dev *dev);
> +
>  struct aac_common aac_config = {
>  	.irq_mod = 1
>  };
> @@ -91,7 +93,7 @@ static int aac_alloc_comm(struct aac_dev *dev, void **commaddr, unsigned long co
>  	init->InitStructRevision = cpu_to_le32(ADAPTER_INIT_STRUCT_REVISION);
>  	if (dev->max_fib_size != sizeof(struct hw_fib))
>  		init->InitStructRevision = cpu_to_le32(ADAPTER_INIT_STRUCT_REVISION_4);
> -	init->MiniPortRevision = cpu_to_le32(Sa_MINIPORT_REVISION);
> +	init->NoOfMSIXVectors = cpu_to_le32(Sa_MINIPORT_REVISION);
>  	init->fsrev = cpu_to_le32(dev->fsrev);
>  
>  	/*
Now that you switched the field definition from 'MiniPortRevision'
to 'NoOfMSIXVectors', shouldn't you rather introduce a
'Sa_MSIXVECTORS' here instead of using the old definitions?

> @@ -140,7 +142,7 @@ static int aac_alloc_comm(struct aac_dev *dev, void **commaddr, unsigned long co
>  			INITFLAGS_NEW_COMM_TYPE2_SUPPORTED | INITFLAGS_FAST_JBOD_SUPPORTED);
>  		init->HostRRQ_AddrHigh = cpu_to_le32((u32)((u64)dev->host_rrq_pa >> 32));
>  		init->HostRRQ_AddrLow = cpu_to_le32((u32)(dev->host_rrq_pa & 0xffffffff));
> -		init->MiniPortRevision = cpu_to_le32(0L);		/* number of MSI-X */
> +		init->NoOfMSIXVectors = cpu_to_le32(dev->max_msix);	/* number of MSI-X */
>  		dprintk((KERN_WARNING"aacraid: New Comm Interface type2 enabled\n"));
>  	}
>  
> @@ -228,6 +230,11 @@ int aac_send_shutdown(struct aac_dev * dev)
>  	/* FIB should be freed only after getting the response from the F/W */
>  	if (status != -ERESTARTSYS)
>  		aac_fib_free(fibctx);
> +	if ((dev->pdev->device == PMC_DEVICE_S7 ||
> +	     dev->pdev->device == PMC_DEVICE_S8 ||
> +	     dev->pdev->device == PMC_DEVICE_S9) &&
> +	     dev->msi_enabled)
> +		aac_src_access_devreg(dev, AAC_ENABLE_INTX);
>  	return status;
>  }
>  
> @@ -388,6 +395,8 @@ struct aac_dev *aac_init_adapter(struct aac_dev *dev)
>  			}
>  		}
>  	}
> +	dev->max_msix = 0;
> +	dev->msi_enabled = 0;
>  	if ((!aac_adapter_sync_cmd(dev, GET_COMM_PREFERRED_SETTINGS,
>  	  0, 0, 0, 0, 0, 0,
>  	  status+0, status+1, status+2, status+3, status+4)) @@ -461,6 +470,11 @@ struct aac_dev *aac_init_adapter(struct aac_dev *dev)
>  	if (host->can_queue > AAC_NUM_IO_FIB)
>  		host->can_queue = AAC_NUM_IO_FIB;
>  
> +	if (dev->pdev->device == PMC_DEVICE_S6 ||
> +	    dev->pdev->device == PMC_DEVICE_S7 ||
> +	    dev->pdev->device == PMC_DEVICE_S8 ||
> +	    dev->pdev->device == PMC_DEVICE_S9)
> +		aac_define_int_mode(dev);
>  	/*
>  	 *	Ok now init the communication subsystem
>  	 */
> @@ -489,4 +503,70 @@ struct aac_dev *aac_init_adapter(struct aac_dev *dev)
>  	return dev;
>  }
>  
> -    
> +static void aac_define_int_mode(struct aac_dev *dev) {
> +
> +	int i, msi_count;
> +
> +	/* max. vectors from GET_COMM_PREFERRED_SETTINGS */
> +	if (dev->max_msix == 0 ||
> +	    dev->pdev->device == PMC_DEVICE_S6 ||
> +	    dev->sync_mode) {
> +		dev->max_msix = 1;
> +		dev->vector_cap = dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB;
> +		return;
> +	}
> +
> +	msi_count = min(dev->max_msix,
> +		(unsigned int)num_online_cpus());
> +
> +	dev->max_msix = msi_count;
> +
> +	if (msi_count > AAC_MAX_MSIX)
> +		msi_count = AAC_MAX_MSIX;
> +
> +	for (i = 0; i < msi_count; i++)
> +		dev->msixentry[i].entry = i;
> +
> +	if (msi_count > 1 && pci_find_capability(dev->pdev, PCI_CAP_ID_MSIX)) 
> +{
> +
Braces should be at the same line as the condition.

> +		i = pci_enable_msix(dev->pdev, dev->msixentry, msi_count);
> +		 /* Check how many MSIX vectors are allocated */
> +		if (i >= 0) {
> +			dev->msi_enabled = 1;
> +			if (i) {
> +				msi_count = i;
> +				if (pci_enable_msix(dev->pdev, dev->msixentry, msi_count)) {
> +					dev->msi_enabled = 0;
> +					printk(KERN_ERR "%s%d: MSIX not supported!! Will try MSI 0x%x.\n",
> +							dev->name, dev->id, i);
> +				}
> +			}
> +		} else {
> +			dev->msi_enabled = 0;
> +			printk(KERN_ERR "%s%d: MSIX not supported!! Will try MSI 0x%x.\n",
> +					dev->name, dev->id, i);
> +		}
> +	}
> +
> +	if (!dev->msi_enabled) {
> +		msi_count = 1;
> +		i = !pci_enable_msi(dev->pdev);
> +

Yikes. Please don't do that; use (!i) in the condition instead.

> +		if (i) {
> +			dev->msi_enabled = 1;
> +			dev->msi = 1;
> +		} else {
> +			printk(KERN_ERR "%s%d: MSI not supported!! Will try INTx 0x%x.\n",
> +					dev->name, dev->id, i);
> +		}
> +	}
> +
> +	if (!dev->msi_enabled)
> +		dev->max_msix = msi_count = 1;
> +	else {
> +		if (dev->max_msix > msi_count)
> +			dev->max_msix = msi_count;
> +	}
> +	dev->vector_cap = (dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB) / 
> +msi_count; }
Closing braces should be on an individual line.

Also here: please remember to run checkpatch before submitting.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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