[ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[Mukker, Atul]

New megaraid driver 2.00.3 is now available at
ftp://ftp.lsil.com/pub/linux-megaraid For this driver, a patch is also
available for 2.5.67 kernel.

Regards
Atul Mukker
Storage Systems
LSI Logic Corporation
U.S.A.

Re: [ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[Christoph Hellwig]

On Wed, Apr 16, 2003 at 04:34:22PM -0400, Mukker, Atul wrote:
> New megaraid driver 2.00.3 is now available at
> ftp://ftp.lsil.com/pub/linux-megaraid For this driver, a patch is also
> available for 2.5.67 kernel.

Hmm, why are you patching and announcing 2.00.3 here if 2.00.4beta is
already out?

Anyway, here's some comments for the driver:


static int hba_count;
static adapter_t *hba_soft_state[MAX_CONTROLLERS];

    a) please avoid typedefs for structs.  Also adapter_t is a bit
    too generic.  What about struct mega_adapter instead?
    b) please don't use such global arrays but always use the
    ->hostdata field.

static struct proc_dir_entry *mega_proc_dir_entry;

static struct notifier_block mega_notifier = {
	.notifier_call = megaraid_reboot_notify
};

    Do you really need this?  Why can you use
    struct device_driver->shutdown?


/*
 * megaraid_validate_parms()
 *
 * Validate that any module parms passed in
 * have proper values.
 */
static void
megaraid_validate_parms(void)
{
	if( (max_cmd_per_lun <= 0) || (max_cmd_per_lun > MAX_CMD_PER_LUN) )
		max_cmd_per_lun = MAX_CMD_PER_LUN;
	if( max_mbox_busy_wait > MBOX_BUSY_WAIT )
		max_mbox_busy_wait = MBOX_BUSY_WAIT;
}

    Please run the whole driver through scripts/Lindent


/**
 * megaraid_detect()
 * @host_template - Our soft state maintained by mid-layer
 *
 * the detect entry point for the mid-layer.
 * We scan the PCI bus for our controllers and start them.
 *
 * Note: PCI_DEVICE_ID_PERC4_DI below represents the PERC4/Di class of
 * products. All of them share the same vendor id, device id, and subsystem
 * vendor id but different subsystem ids. As of now, driver does not use the
 * subsystem id.
 */
static int
megaraid_detect(Scsi_Host_Template *host_template)

    Please get rid of ->detect and use the new-style pci API +
    scsi_add_host instead.

	host_template->proc_name = "megaraid";

    CAn be initialized at compile-time.

	printk(KERN_NOTICE "megaraid: " MEGARAID_VERSION);

	megaraid_validate_parms();

	memset(mega_hbas, 0, sizeof (mega_hbas));

	hba_count = 0;

    These are in .bss

	}

	return;

}
    This is superflous.

/*
 * megaraid_proc_info()
 *
 * Returns data to be displayed in /proc/scsi/megaraid/X
 */
static int
megaraid_proc_info(char *buffer, char **start, off_t offset, int length,
		int host_no, int inout)
{
	*start = buffer;
	return 0;
}

    Just don't implement this method instead?

volatile static int internal_done_flag = 0;
volatile static int internal_done_errcode = 0;


static DECLARE_WAIT_QUEUE_HEAD (internal_wait);

static void internal_done (Scsi_Cmnd *cmd)
{
	internal_done_errcode = cmd->result;
	internal_done_flag++;
	wake_up (&internal_wait);
}

/* shouldn't be used, but included for completeness */

static int
megaraid_command (Scsi_Cmnd *cmd)
{
	internal_done_flag = 0;

	/* Queue command, and wait until it has completed */
	megaraid_queue (cmd, internal_done);

	while (!internal_done_flag)
		interruptible_sleep_on (&internal_wait);

	return internal_done_errcode;
}

    This looks horribly racy.


				return TRUE;
			}
		}
	}

	return FALSE;
}

    Please don't use TRUE/FALSE but 1/0 directly.

static int
megadev_open (struct inode *inode, struct file *filep)
{
	/*
	 * Only allow superuser to access private ioctl interface
	 */
	if( !capable(CAP_SYS_ADMIN) ) return -EACCES;

	if (!try_module_get(THIS_MODULE)) {
		return -ENXIO;
	}

   This is broken as hell (and I fixed it in the old megraid driver
   ages ago!)  Just set ->owner in the file_operation.

	/*
	 * Wait till all the issued commands are complete and there are no
	 * commands in the pending queue
	 */
	while( atomic_read(&adapter->pend_cmds) > 0 ||
			!list_empty(&adapter->pending_list) ) {

		sleep_on_timeout( &wq, 1*HZ );	/* sleep for 1s */
	}

  Again racy.  You need to opencode this (similar to wait_event, maybe
  just add wait_even_timeout).

	if (adapter->read_ldidmap) {
		struct list_head *pos;
		list_for_each(pos, &adapter->pending_list) {
			scb = list_entry(pos, scb_t, list);

  list_for_each_entry()?

/**
 * mega_allocate_inquiry()
 * @dma_handle - handle returned for dma address
 * @pdev - handle to pci device
 *
 * allocates memory for inquiry structure
 */
static inline caddr_t
mega_allocate_inquiry(dma_addr_t *dma_handle, struct pci_dev *pdev)
{
	return pci_alloc_consistent(pdev, sizeof(mega_inquiry3), dma_handle);
}


static inline void
mega_free_inquiry(caddr_t inquiry, dma_addr_t dma_handle, struct pci_dev *pdev)
{
	pci_free_consistent(pdev, sizeof(mega_inquiry3), inquiry, dma_handle);
}

Do you really need a wrapper for those?



	/*
	 * For all internal commands, the buffer must be allocated in <4GB
	 * address range
	 */
	if( make_local_pdev(adapter, &pdev) != 0 ) return -1;

    So use dma_alloc_coherent with a GFP_KERNEL arg instead of such ugly hacks.

	/*
	 * Wait till this command finishes. Do not use
	 * wait_event_interruptible(). It causes panic if CTRL-C is hit when
	 * dumping e.g., physical disk information through /proc interface.
	 */
#if 0
	wait_event_interruptible(adapter->int_waitq, scmd->state);
#endif
	wait_event(adapter->int_waitq, scmd->state);

    Maybe you need to actually look at wait_event_interruptible and
    handle it's return value? :)


static Scsi_Host_Template driver_template = MEGARAID;

    Get rid of the ugly macro and add the members right here.

While we're at it:  Please also get rid of all those prototypes in
megaraid.h

Re: [ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[Alan Cox]

>     a) please avoid typedefs for structs.  Also adapter_t is a bit
>     too generic.  What about struct mega_adapter instead?
>     b) please don't use such global arrays but always use the
>     ->hostdata field.

a) megaraid always used a set of basic typedefs. Seems to be your personal
view not general comment

b) Probably right

> static struct notifier_block mega_notifier = {
> 	.notifier_call = megaraid_reboot_notify
> };
> 
>     Do you really need this?  Why can you use
>     struct device_driver->shutdown?

Not on 2.2

> 	if( max_mbox_busy_wait > MBOX_BUSY_WAIT )
> 		max_mbox_busy_wait = MBOX_BUSY_WAIT;
> }
> 
>     Please run the whole driver through scripts/Lindent

Looks fine to me anyway

> megaraid_detect(Scsi_Host_Template *host_template)
> 
>     Please get rid of ->detect and use the new-style pci API +
>     scsi_add_host instead.

Doesnt work on 2.4

> static void internal_done (Scsi_Cmnd *cmd)
> {
> 	internal_done_errcode = cmd->result;
> 	internal_done_flag++;
> 	wake_up (&internal_wait);
> }
> 
> /* shouldn't be used, but included for completeness */

>     This looks horribly racy.

Its ok on older kernels 2.2/2.4 wont afaik ever use it.

> 	return FALSE;
> 
>     Please don't use TRUE/FALSE but 1/0 directly.

TRUE/FALSE is IMHO perfectly clear

> 	if (!try_module_get(THIS_MODULE)) {
> 		return -ENXIO;
> 	}
> 
>    This is broken as hell (and I fixed it in the old megraid driver
>    ages ago!)  Just set ->owner in the file_operation.

Definitely wants fixing
 
> 	while( atomic_read(&adapter->pend_cmds) > 0 ||
> 			!list_empty(&adapter->pending_list) ) {
> 
> 		sleep_on_timeout( &wq, 1*HZ );	/* sleep for 1s */
> 	}
> 
>   Again racy.  You need to opencode this (similar to wait_event, maybe
>   just add wait_even_timeout).

Actually the timeout means the worst that occurs is you wait a 
second, and the code is portable to old 2.2/2.4

> static Scsi_Host_Template driver_template = MEGARAID;
> 
>     Get rid of the ugly macro and add the members right here.
> 
> While we're at it:  Please also get rid of all those prototypes in
> megaraid.h

2.2

Re: [ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[Christoph Hellwig]

On Thu, Apr 17, 2003 at 08:52:16AM -0400, Alan Cox wrote:
> a) megaraid always used a set of basic typedefs. Seems to be your personal
> view not general comment

At least Linus seems to share my opinion :)

> >     Do you really need this?  Why can you use
> >     struct device_driver->shutdown?
> 
> Not on 2.2

This is a 2.5-only driver (in fact a 2.5 only patch against a 2.4 driver)

> > 	return FALSE;
> > 
> >     Please don't use TRUE/FALSE but 1/0 directly.
> 
> TRUE/FALSE is IMHO perfectly clear

It's clear, but Linux kernel code avoids it usually.  I'm
currently getting rid of it in the scsi headers so it's a good idea
to avoid it in new drivers.

Re: [ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[Alan Cox]

> On Thu, Apr 17, 2003 at 08:52:16AM -0400, Alan Cox wrote:
> > >     Do you really need this?  Why can you use
> > >     struct device_driver->shutdown?
> > 
> > Not on 2.2
> 
> This is a 2.5-only driver (in fact a 2.5 only patch against a 2.4 driver)

2.0 exists for 2.4 as well. I dont know about for 2.2

Re: [ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[Christoph Hellwig]

On Thu, Apr 17, 2003 at 09:23:34AM -0400, Alan Cox wrote:
> > This is a 2.5-only driver (in fact a 2.5 only patch against a 2.4 driver)
> 
> 2.0 exists for 2.4 as well. I dont know about for 2.2

Is my English that bad? :)

Currently the megaraid2 driver is distributed as a tarball that works _only_
with Linux 2.4 (and it looks like only with rather recent 2.4 versions), and
there is a patch against it to make it work with 2.5 and only 2.5.

Re: [ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[James Bottomley]

On Thu, 2003-04-17 at 07:38, Christoph Hellwig wrote:
> On Wed, Apr 16, 2003 at 04:34:22PM -0400, Mukker, Atul wrote:
> > New megaraid driver 2.00.3 is now available at
> > ftp://ftp.lsil.com/pub/linux-megaraid For this driver, a patch is also
> > available for 2.5.67 kernel.
[...]

I'm not inclined to force style changes in any drivers with active
maintainers on the grounds that we already have much worse offenders in
the tree (and style changes do make merging a pain).

I counted one serious issue:

The try_module_get problem

One future compatibility issue:

->detect moved to scsi_add_host (for the device model).

One issue that would improve the driver internally:

move hba_soft_state array to dynamic ->hostdata

plus a list of other more style related issues.

I'll fix up the first one, Atul, can you take the other two for a future
patch (the others Christoph mentions might be nice, too...)?

James

Re: [ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 170 bytes --]

OK, I think the attached patch fixes the try_module_get problem and also
sweeps up a compile warning issue I ran into on pa-risc.

Does this look OK to everyone?

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 1715 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1044  -> 1.1046 
#	drivers/scsi/megaraid.c	1.38    -> 1.40   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/04/17	jejb@raven.il.steeleye.com	1.1045
# Fix megaraid compile warnings
# --------------------------------------------
# 03/04/17	jejb@raven.il.steeleye.com	1.1046
# Fix megaraid module ownership
# 
# Move to using the .owner field of fops
# --------------------------------------------
#
diff -Nru a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
--- a/drivers/scsi/megaraid.c	Thu Apr 17 12:23:35 2003
+++ b/drivers/scsi/megaraid.c	Thu Apr 17 12:23:35 2003
@@ -34,6 +34,7 @@
 #include <linux/fs.h>
 #include <linux/blk.h>
 #include <asm/uaccess.h>
+#include <asm/io.h>
 #include <linux/delay.h>
 #include <linux/reboot.h>
 #include <linux/module.h>
@@ -87,9 +88,9 @@
  * The File Operations structure for the serial/ioctl interface of the driver
  */
 static struct file_operations megadev_fops = {
+	.owner		= THIS_MODULE,
 	.ioctl		= megadev_ioctl,
 	.open		= megadev_open,
-	.release	= megadev_close,
 };
 
 /*
@@ -4039,9 +4040,6 @@
 	 */
 	if( !capable(CAP_SYS_ADMIN) ) return -EACCES;
 
-	if (!try_module_get(THIS_MODULE)) {
-		return -ENXIO;
-	}
 	return 0;
 }
 
@@ -4635,14 +4633,6 @@
 		}
 	}
 
-	return 0;
-}
-
-
-static int
-megadev_close (struct inode *inode, struct file *filep)
-{
-	module_put(THIS_MODULE);
 	return 0;
 }
 

RE: [ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[Mukker, Atul]

I have posted the 2.00.5 driver on ftp.lsil.com/pub/linux-megaraid I have
tried to address most of your concerns in this driver except for the
following

>     b) please don't use such global arrays but always use the
>     ->hostdata field

The driver needs a global access to all adapters since it implements a
private ioctl node for applications . The applications use a single node for
all controllers


The patch for 2.5.6x is not available yet but I will announce that pretty
soon.

Thanks
-Atul Mukker <atulm@lsil.com>

> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Thursday, April 17, 2003 8:38 AM
> To: Mukker, Atul
> Cc: 'alan@redhat.com'; 'James.Bottomley@steeleye.com';
> 'linux-kernel@vger.kernel.org'; 'linux-scsi@vger.kernel.org';
> 'linux-megaraid-devel@dell.com'; 'linux-megaraid-announce@dell.com'
> Subject: Re: [ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and
> 2.5.67 kernels
> 
> 
> On Wed, Apr 16, 2003 at 04:34:22PM -0400, Mukker, Atul wrote:
> > New megaraid driver 2.00.3 is now available at
> > ftp://ftp.lsil.com/pub/linux-megaraid For this driver, a 
> patch is also
> > available for 2.5.67 kernel.
> 
> Hmm, why are you patching and announcing 2.00.3 here if 2.00.4beta is
> already out?
> 
> Anyway, here's some comments for the driver:
> 
> 
> static int hba_count;
> static adapter_t *hba_soft_state[MAX_CONTROLLERS];
> 
>     a) please avoid typedefs for structs.  Also adapter_t is a bit
>     too generic.  What about struct mega_adapter instead?
>     b) please don't use such global arrays but always use the
>     ->hostdata field.
> 
> static struct proc_dir_entry *mega_proc_dir_entry;
> 
> static struct notifier_block mega_notifier = {
> 	.notifier_call = megaraid_reboot_notify
> };
> 
>     Do you really need this?  Why can you use
>     struct device_driver->shutdown?
> 
> 
> /*
>  * megaraid_validate_parms()
>  *
>  * Validate that any module parms passed in
>  * have proper values.
>  */
> static void
> megaraid_validate_parms(void)
> {
> 	if( (max_cmd_per_lun <= 0) || (max_cmd_per_lun > 
> MAX_CMD_PER_LUN) )
> 		max_cmd_per_lun = MAX_CMD_PER_LUN;
> 	if( max_mbox_busy_wait > MBOX_BUSY_WAIT )
> 		max_mbox_busy_wait = MBOX_BUSY_WAIT;
> }
> 
>     Please run the whole driver through scripts/Lindent
> 
> 
> /**
>  * megaraid_detect()
>  * @host_template - Our soft state maintained by mid-layer
>  *
>  * the detect entry point for the mid-layer.
>  * We scan the PCI bus for our controllers and start them.
>  *
>  * Note: PCI_DEVICE_ID_PERC4_DI below represents the PERC4/Di class of
>  * products. All of them share the same vendor id, device id, 
> and subsystem
>  * vendor id but different subsystem ids. As of now, driver 
> does not use the
>  * subsystem id.
>  */
> static int
> megaraid_detect(Scsi_Host_Template *host_template)
> 
>     Please get rid of ->detect and use the new-style pci API +
>     scsi_add_host instead.
> 
> 	host_template->proc_name = "megaraid";
> 
>     CAn be initialized at compile-time.
> 
> 	printk(KERN_NOTICE "megaraid: " MEGARAID_VERSION);
> 
> 	megaraid_validate_parms();
> 
> 	memset(mega_hbas, 0, sizeof (mega_hbas));
> 
> 	hba_count = 0;
> 
>     These are in .bss
> 
> 	}
> 
> 	return;
> 
> }
>     This is superflous.
> 
> /*
>  * megaraid_proc_info()
>  *
>  * Returns data to be displayed in /proc/scsi/megaraid/X
>  */
> static int
> megaraid_proc_info(char *buffer, char **start, off_t offset, 
> int length,
> 		int host_no, int inout)
> {
> 	*start = buffer;
> 	return 0;
> }
> 
>     Just don't implement this method instead?
> 
> volatile static int internal_done_flag = 0;
> volatile static int internal_done_errcode = 0;
> 
> 
> static DECLARE_WAIT_QUEUE_HEAD (internal_wait);
> 
> static void internal_done (Scsi_Cmnd *cmd)
> {
> 	internal_done_errcode = cmd->result;
> 	internal_done_flag++;
> 	wake_up (&internal_wait);
> }
> 
> /* shouldn't be used, but included for completeness */
> 
> static int
> megaraid_command (Scsi_Cmnd *cmd)
> {
> 	internal_done_flag = 0;
> 
> 	/* Queue command, and wait until it has completed */
> 	megaraid_queue (cmd, internal_done);
> 
> 	while (!internal_done_flag)
> 		interruptible_sleep_on (&internal_wait);
> 
> 	return internal_done_errcode;
> }
> 
>     This looks horribly racy.
> 
> 
> 				return TRUE;
> 			}
> 		}
> 	}
> 
> 	return FALSE;
> }
> 
>     Please don't use TRUE/FALSE but 1/0 directly.
> 
> static int
> megadev_open (struct inode *inode, struct file *filep)
> {
> 	/*
> 	 * Only allow superuser to access private ioctl interface
> 	 */
> 	if( !capable(CAP_SYS_ADMIN) ) return -EACCES;
> 
> 	if (!try_module_get(THIS_MODULE)) {
> 		return -ENXIO;
> 	}
> 
>    This is broken as hell (and I fixed it in the old megraid driver
>    ages ago!)  Just set ->owner in the file_operation.
> 
> 	/*
> 	 * Wait till all the issued commands are complete and 
> there are no
> 	 * commands in the pending queue
> 	 */
> 	while( atomic_read(&adapter->pend_cmds) > 0 ||
> 			!list_empty(&adapter->pending_list) ) {
> 
> 		sleep_on_timeout( &wq, 1*HZ );	/* sleep for 1s */
> 	}
> 
>   Again racy.  You need to opencode this (similar to wait_event, maybe
>   just add wait_even_timeout).
> 
> 	if (adapter->read_ldidmap) {
> 		struct list_head *pos;
> 		list_for_each(pos, &adapter->pending_list) {
> 			scb = list_entry(pos, scb_t, list);
> 
>   list_for_each_entry()?
> 
> /**
>  * mega_allocate_inquiry()
>  * @dma_handle - handle returned for dma address
>  * @pdev - handle to pci device
>  *
>  * allocates memory for inquiry structure
>  */
> static inline caddr_t
> mega_allocate_inquiry(dma_addr_t *dma_handle, struct pci_dev *pdev)
> {
> 	return pci_alloc_consistent(pdev, 
> sizeof(mega_inquiry3), dma_handle);
> }
> 
> 
> static inline void
> mega_free_inquiry(caddr_t inquiry, dma_addr_t dma_handle, 
> struct pci_dev *pdev)
> {
> 	pci_free_consistent(pdev, sizeof(mega_inquiry3), 
> inquiry, dma_handle);
> }
> 
> Do you really need a wrapper for those?
> 
> 
> 
> 	/*
> 	 * For all internal commands, the buffer must be 
> allocated in <4GB
> 	 * address range
> 	 */
> 	if( make_local_pdev(adapter, &pdev) != 0 ) return -1;
> 
>     So use dma_alloc_coherent with a GFP_KERNEL arg instead 
> of such ugly hacks.
> 
> 	/*
> 	 * Wait till this command finishes. Do not use
> 	 * wait_event_interruptible(). It causes panic if 
> CTRL-C is hit when
> 	 * dumping e.g., physical disk information through 
> /proc interface.
> 	 */
> #if 0
> 	wait_event_interruptible(adapter->int_waitq, scmd->state);
> #endif
> 	wait_event(adapter->int_waitq, scmd->state);
> 
>     Maybe you need to actually look at wait_event_interruptible and
>     handle it's return value? :)
> 
> 
> static Scsi_Host_Template driver_template = MEGARAID;
> 
>     Get rid of the ugly macro and add the members right here.
> 
> While we're at it:  Please also get rid of all those prototypes in
> megaraid.h
> -
> 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: [ANNOUNCE]: version 2.00.3 megaraid driver for 2.4.x and 2.5.67 kernels

[Mukker, Atul]

> The patch for 2.5.6x is not available yet but I will announce 
> that pretty soon.

The patch for kernel 2.5.6[78] for driver 2.00.5 is now available at

ftp://ftp.lsil.com/pub/linux-megaraid/drivers/version-2.00.5/

Regards
-Atul Mukker <atulm@lsil.com>