RE: [Patch] scsi: megaraid_{mm,mbox}: init fix for kdump

From: Patro, Sumant
Date: Tue Jan 02 2007 - 21:55:43 EST



Thanks for the review.
I will resubmit the patch.

Regards,

Sumant

-----Original Message-----
From: Randy Dunlap [mailto:rdunlap@xxxxxxxxxxxx]
Sent: Friday, December 29, 2006 1:38 PM
To: Patro, Sumant
Cc: James.Bottomley@xxxxxxxxxxxx; akpm@xxxxxxxx;
linux-scsi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Kolli, Neela;
Yang, Bo; Patro, Sumant
Subject: Re: [Patch] scsi: megaraid_{mm,mbox}: init fix for kdump

On Fri, 29 Dec 2006 08:02:17 -0800 Sumant Patro wrote:

See Documentation/SubmittingPatches:
Please include output of "diffstat -p1 -w70" so that we can easily see
the scope of the changes.

and see Documentation/CodingStyle for comments below:


> diff -uprN linux-2.6.orig/drivers/scsi/megaraid/megaraid_mbox.c
> linux-2.6.new/drivers/scsi/megaraid/megaraid_mbox.c
> --- linux-2.6.orig/drivers/scsi/megaraid/megaraid_mbox.c 2006-12-28
> 09:56:04.000000000 -0800
> +++ linux-2.6.new/drivers/scsi/megaraid/megaraid_mbox.c 2006-12-29
> +++ 05:31:48.000000000 -0800
> @@ -779,6 +780,22 @@ megaraid_init_mbox(adapter_t *adapter)
> goto out_release_regions;
> }
>
> + // initialize the mutual exclusion lock for the mailbox
> + spin_lock_init(&raid_dev->mailbox_lock);

Linux uses /*...*/ C89-style comments, not // C99 comments.

> + // allocate memory required for commands
> + if (megaraid_alloc_cmd_packets(adapter) != 0) {
> + goto out_iounmap;
> + }
> +
> + /*
> + * Issue SYNC cmd to flush the pending cmds in the adapter
> + * and initialize its internal state
> + */
> +
> + if (megaraid_mbox_fire_sync_cmd(adapter))
> + con_log(CL_ANN, ("megaraid: sync cmd failed\n"));
> +

> // Product info
> if (megaraid_mbox_product_info(adapter) != 0) {
> - goto out_alloc_cmds;
> + goto out_free_irq;

Don't uses {} braces around 1-statement "blocks".

> @@ -875,7 +883,7 @@ megaraid_init_mbox(adapter_t *adapter)
> * accessed
> */
> if (megaraid_sysfs_alloc_resources(adapter) != 0) {
> - goto out_alloc_cmds;
> + goto out_free_irq;

Ditto.

> }
>
> // Set the DMA mask to 64-bit. All supported controllers as
capable
> of @@ -3380,6 +3388,86 @@ megaraid_mbox_flush_cache(adapter_t *ada
>
>
> /**
> + * megaraid_mbox_fire_sync_cmd - fire the sync cmd
> + * @param adapter : soft state for the controller
> + */
> +static int
> +megaraid_mbox_fire_sync_cmd(adapter_t *adapter) {
> + mbox_t *mbox;
> + uint8_t raw_mbox[sizeof(mbox_t)];
> + mraid_device_t *raid_dev = ADAP2RAIDDEV(adapter);
> + mbox64_t *mbox64;
> + uint8_t status = 0;
> + int i;
> + uint32_t dword;
> +
> + mbox = (mbox_t *)raw_mbox;
> +
> + memset((caddr_t)raw_mbox, 0, sizeof(mbox_t));
> +
> + raw_mbox[0] = 0xFF;
> +
> + mbox64 = raid_dev->mbox64;
> + mbox = raid_dev->mbox;
> +
> + /*
> + * Wait until mailbox is free
> + */
> + if (megaraid_busywait_mbox(raid_dev) != 0) {
> + status = 1;
> + goto blocked_mailbox;
> + }
> +
> + /*
> + * Copy mailbox data into host structure
> + */
> + memcpy((caddr_t)mbox, (caddr_t)raw_mbox, 16);
> + mbox->cmdid = 0xFE;
> + mbox->busy = 1;
> + mbox->poll = 0;
> + mbox->ack = 0;
> + mbox->numstatus = 0;
> + mbox->status = 0;
> +
> + wmb();
> + WRINDOOR(raid_dev, raid_dev->mbox_dma | 0x1);
> +
> + // wait for maximum 1 min for status to post.
> + // If the Firmware SUPPORTS the ABOVE COMMAND,
> + // mbox->cmd will be set to 0
> + // else
> + // the firmware will reject the command with
> + // mbox->numstatus set to 1

Don't use // comment style. Also, for multi-line comments in Linux,
please use this preferred style:

/*
* This is the preferred style for multi-line
* comments in the Linux kernel source code.
* Please use it consistently.
*
* Description: A column of asterisks on the left side,
* with beginning and ending almost-blank lines.
*/

Thanks.
---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/