On Wed, Jun 02, 2010 at 12:58:06PM -0700, [email protected] wrote:
>
> The patch titled
> cciss: add performant mode support for Stars/Sirius
> has been added to the -mm tree. Its filename is
> cciss-add-performant-mode-support-for-stars-sirius.patch
>
[...snip...] see my comments below.
>
> ------------------------------------------------------
> Subject: cciss: add performant mode support for Stars/Sirius
> From: Mike Miller <[email protected]>
>
> Add a mode of controller operation called Performant Mode. Even though
> cciss has been deprecated in favor of hpsa there are new controllers due
> out next year that HP must support in older vendor distros. Vendors
> require all fixes/features be upstream. These new controllers support
> only 16 commands in simple mode but support up to 1024 in performant mode.
> This requires us to add this support at this late date.
>
> The performant mode transport minimizes host PCI accesses by performinf
> many completions per read. PCI writes are posted so the host can write
> then immediately get off the bus not waiting for the writwe to complete to
> the target. In the context of performant mode the host read out to a
> controller pulls all posted writes into host memory ensuring the reply
> queue is coherent.
>
> Signed-off-by: Mike Miller <[email protected]>
> Cc: Stephen M. Cameron <[email protected]>
> Cc: Jens Axboe <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> drivers/block/cciss.c | 267 ++++++++++++++++++++++++++++-------
> drivers/block/cciss.h | 109 +++++++++++++-
> drivers/block/cciss_cmd.h | 34 +++-
> drivers/block/cciss_scsi.c | 6
> 4 files changed, 353 insertions(+), 63 deletions(-)
>
> diff -puN drivers/block/cciss.c~cciss-add-performant-mode-support-for-stars-sirius drivers/block/cciss.c
> --- a/drivers/block/cciss.c~cciss-add-performant-mode-support-for-stars-sirius
> +++ a/drivers/block/cciss.c
> @@ -206,6 +206,11 @@ static void cciss_device_release(struct
> static void cciss_free_gendisk(ctlr_info_t *h, int drv_index);
> static void cciss_free_drive_info(ctlr_info_t *h, int drv_index);
>
[...snip...]
> + set_performant_mode(h, c);
> spin_lock_irqsave(&h->lock, flags);
> addQ(&h->reqQ, c);
> h->Qdepth++;
> @@ -350,6 +366,28 @@ static const char *raid_label[] = { "0",
>
> #ifdef CONFIG_PROC_FS
>
> +static inline u32 next_command(ctlr_info_t *h)
> +{
"next_command(...)" should not be inside "#ifdef CONFIG_PROC_FS"
[... snip...]
> dma_prefetch |= 0x8000;
> @@ -3944,38 +4140,8 @@ static int __devinit cciss_pci_init(ctlr
> }
>
> #ifdef CCISS_DEBUG
> return 0;
[...snip...]
>
> err_out_free_res:
> @@ -3984,6 +4150,7 @@ err_out_free_res:
> * Smart Array controllers that pci_enable_device does not undo
> */
> pci_release_regions(pdev);
> + cciss_put_controller_into_performant_mode(c);
> return err;
^^^ This is calling cciss_put_controller_into_performant_mode() in
the error code path of cciss_pci_init(), and not in the main initialization
code path. That cannot be correct.
[...snip...]
>
> static void __devexit cciss_remove_one(struct pci_dev *pdev)
> @@ -4575,7 +4741,6 @@ static int __init cciss_init(void)
> * array of them, the size must be a multiple of 8 bytes.
^^^ "8 bytes" is no longer correct in the above comment,
should be COMMANDLIST_ALIGNMENT (which is 32 now).
> */
> BUILD_BUG_ON(sizeof(CommandList_struct) % COMMANDLIST_ALIGNMENT);
> -
> printk(KERN_INFO DRIVER_NAME "\n");
>
[...snip...]
> @@ -173,12 +174,15 @@ typedef struct _SGDescriptor_struct {
> * PAD_64 can be adjusted independently as needed for 32-bit
> * and 64-bits systems.
> */
> -#define COMMANDLIST_ALIGNMENT (8)
> +#define COMMANDLIST_ALIGNMENT (32)
> #define IS_64_BIT ((sizeof(long) - 4)/4)
> #define IS_32_BIT (!IS_64_BIT)
> -#define PAD_32 (0)
> +#define PAD_32 (32)
^^^ if it's supposed to be aligned at 32 bit boundary
(which it is) then PAD_32 should be 0, not 32, if 32 works.
[...snip...]
> @@ -213,6 +213,8 @@ scsi_cmd_stack_setup(int ctlr, struct cc
>
> /* Check alignment, see cciss_cmd.h near CommandList_struct def. */
> BUILD_BUG_ON((sizeof(*stk->pool) % COMMANDLIST_ALIGNMENT) != 0);
> + /* printk(KERN_WARNING "cciss_scsi.c: 0x%08x 0x%08x 0x%08x\n",
> + 0xdeadbeef, sizeof(*stk->pool), 0xbeefdead); */
^^^ this looks like debug code left in by mistake.
> /* pci_alloc_consistent guarantees 32-bit DMA address will be used */
> stk->pool = (struct cciss_scsi_cmd_stack_elem_t *)
> pci_alloc_consistent(hba[ctlr]->pdev, size, &stk->cmd_pool_handle);
> _
>
-- steve