RE: [PATCH] cpqarray update

RE: [PATCH] cpqarray update

[Wiran, Francis]

> You need to check the return value of pci_module_init() for errors.
No, because the return value is determined from number of ctrls found,
and not from function return.

int __init cpqarray_init(void)
{
...
	pci_module_init(&cpqarray_pci_driver);
	cpqarray_eisa_detect();

	for(i=0;i<MAX_CTLR;i++) {
		if(hba[i] != NULL)
			num_ctlrs_reg++
	}

	return (num_ctlrs_reg);
}

int __init cpqarray_init_module(void)
{
	if (cpqarray_init() == 0)
		return -ENODEV;
	return 0;
}

regards,
-francis-



> -----Original Message-----
> From: Jeff Garzik [mailto:jgarzik@pobox.com] 
> Sent: Monday, January 26, 2004 2:15 PM
> To: Linux Kernel Mailing List; Wiran, Francis
> Subject: Re: [PATCH] cpqarray update
> 
> 
> Linux Kernel Mailing List wrote:
> > ChangeSet 1.1288, 2004/01/26 16:58:21-02:00, francis.wiran@hp.com
> > 
> > 	[PATCH] cpqarray update
> > 	
> > 	Yes, we should. I usually ask Mike Miller in our group 
> to send my
> > 	patches since he's been doing that and more known in 
> the community, but
> > 	since you already got a hold of it ...yes, please :)
> > 	
> > 	CHANGELOG:
> > 	
> > 	   * Fix for eisa card not detecting partitions properly
> > 	   * Use pci_module_init instead of pci_register_device 
> to handle hotplug
> > 	     scenario and unregister if the driver can't find 
> pci controller
> > 	   * Add BLKSSZGET ioctl
> [...]
> > @@ -616,7 +623,7 @@
> >  
> >  	/* detect controllers */
> >  	printk(DRIVER_NAME "\n");
> > -	pci_register_driver(&cpqarray_pci_driver);
> > +	pci_module_init(&cpqarray_pci_driver);
> >  	cpqarray_eisa_detect();
> >  
> >  	for(i=0; i< MAX_CTLR; i++) {
> 
> You need to check the return value of pci_module_init() for errors.
> 
> 	Jeff
> 
> 
> 
> 

Re: [PATCH] cpqarray update

[Jeff Garzik]

Wiran, Francis wrote:
>>You need to check the return value of pci_module_init() for errors.
> 
> No, because the return value is determined from number of ctrls found,
> and not from function return.
> 
> int __init cpqarray_init(void)
> {
> ...
> 	pci_module_init(&cpqarray_pci_driver);
> 	cpqarray_eisa_detect();
> 
> 	for(i=0;i<MAX_CTLR;i++) {
> 		if(hba[i] != NULL)
> 			num_ctlrs_reg++
> 	}
> 
> 	return (num_ctlrs_reg);
> }
> 
> int __init cpqarray_init_module(void)
> {
> 	if (cpqarray_init() == 0)
> 		return -ENODEV;
> 	return 0;
> }


Nope, this needs to be turned inside out.  The proper PCI driver looks like

static int __init cp_init (void)
{
         return pci_module_init (&cp_driver);
}

static void __exit cp_exit (void)
{
         pci_unregister_driver (&cp_driver);
}

We already handle the cases you describe.  The cpqarray code -breaks- 
the API design by doing it this way.

cpqarray does not fully support the pci_ids features and hotplug without 
this.

	Jeff

RE: [PATCH] cpqarray update

[Wiran, Francis]

Ok. I'll remember that when we update the 2.6 driver (currently the
driver packaged in 2.6.1 is version 2.4.5 ... pretty old).

Thanks
-fw-

> -----Original Message-----
> From: Jeff Garzik [mailto:jgarzik@pobox.com] 
> Sent: Thursday, January 29, 2004 2:15 PM
> To: Wiran, Francis
> Cc: Greg KH; Hollis Blanchard; Marcelo Tosatti; Linux Kernel 
> Mailing List
> Subject: Re: [PATCH] cpqarray update
> 
> 
> Wiran, Francis wrote:
> > Ok. I'll make a patch when the behaviour changes, as per Greg's:
> > 
> > 
> >>Yeah, I don't really like it either, but figured it was a 
> 2.7 task to
> >>clean it up properly.
> > 
> > 
> > At 2.6.1, it still returns count.
> 
> Look closer:
> 2.4 behavior:  returns count.
> 
> 2.6 behavior:  returns negative value on error, or returns 1 when no
> controllers found, or returns number of controllers found.
> 
> 	Jeff
> 
> 
> 
> 

Re: [PATCH] cpqarray update

[Jeff Garzik]

Wiran, Francis wrote:
> Ok. I'll make a patch when the behaviour changes, as per Greg's:
> 
> 
>>Yeah, I don't really like it either, but figured it was a 2.7 task to
>>clean it up properly.
> 
> 
> At 2.6.1, it still returns count.

Look closer:
2.4 behavior:  returns count.

2.6 behavior:  returns negative value on error, or returns 1 when no
controllers found, or returns number of controllers found.

	Jeff

RE: [PATCH] cpqarray update

[Wiran, Francis]

Ok. I'll make a patch when the behaviour changes, as per Greg's:

>Yeah, I don't really like it either, but figured it was a 2.7 task to
>clean it up properly.

At 2.6.1, it still returns count.


> -----Original Message-----
> From: Jeff Garzik [mailto:jgarzik@pobox.com] 
> Sent: Thursday, January 29, 2004 10:57 AM
> To: Wiran, Francis
> Cc: Greg KH; Hollis Blanchard; Marcelo Tosatti; Linux Kernel 
> Mailing List
> Subject: Re: [PATCH] cpqarray update
> 
> 
> Wiran, Francis wrote:
> > check for negative value? That's odd. The 
> pci_register_driver() in my
> > copy of 2.4.24 kernel (drivers/pci/pci.c) looks something like this:
> > 
> > {
> > 	count = 0;
> > 
> > 	for ....
> > 		count += foo();
> > 
> > 	return count;
> > }
> > 
> > 
> > Or will it change in the future? The patch that I sent was 
> based on what
> > is in the current kernel.
> 
> 
> Correct, Greg was referring to 2.6.x behavior of 
> pci_register_driver(), 
> which changed from 2.4.x.
> 
> 	Jeff
> 
> 
> 
> 

Re: [PATCH] cpqarray update

[Jeff Garzik]

Wiran, Francis wrote:
> check for negative value? That's odd. The pci_register_driver() in my
> copy of 2.4.24 kernel (drivers/pci/pci.c) looks something like this:
> 
> {
> 	count = 0;
> 
> 	for ....
> 		count += foo();
> 
> 	return count;
> }
> 
> 
> Or will it change in the future? The patch that I sent was based on what
> is in the current kernel.


Correct, Greg was referring to 2.6.x behavior of pci_register_driver(), 
which changed from 2.4.x.

	Jeff

RE: [PATCH] cpqarray update

[Wiran, Francis]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com] 
> Sent: Wednesday, January 28, 2004 5:29 PM
> To: Wiran, Francis
> Cc: Hollis Blanchard; Marcelo Tosatti; Jeff Garzik; Linux 
> Kernel Mailing List
> Subject: Re: [PATCH] cpqarray update
> 
> 
> On Wed, Jan 28, 2004 at 05:10:29PM -0600, Wiran, Francis wrote:
> > 
> > Ok. Here's the patch for that. At least until 
> vio_module_init comes :)
> 
> Heh, you didn't actually try that patch, did you?
> 
> (hint, you need to check for a negative value...)
> 
> greg k-h
> 

check for negative value? That's odd. The pci_register_driver() in my
copy of 2.4.24 kernel (drivers/pci/pci.c) looks something like this:

{
	count = 0;

	for ....
		count += foo();

	return count;
}


Or will it change in the future? The patch that I sent was based on what
is in the current kernel.




Just for clarification:
I sent two patches yesterday (I should've numbered them, sorry I
forgot), since they address different issues and the driver registration
issue is being discussed right now.
First patch was cpqarray_eisa_detect_fix.patch, second was
cpqarray_pci_unregister.patch

Re: [PATCH] cpqarray update

[Greg KH]

On Wed, Jan 28, 2004 at 06:44:32PM -0500, Jeff Garzik wrote:
> Jeff Garzik wrote:
> >Actually I disagree with GregKH on this.
> >
> >The register/unregister functions need to be returning error codes, 
> >_not_ the count of interfaces registered.  It is trivial to count the 
> >registered interfaces in the driver itself, but IMO far more important 
> >to propagate fatal errors back to the original caller.
> 
> Nevermind, this got fixed.  I'm still worried about the '1' return 
> value, though, for zero controllers found.

Yeah, I don't really like it either, but figured it was a 2.7 task to
clean it up properly.

thanks,

greg k-h

Re: [PATCH] cpqarray update

[Jeff Garzik]

Jeff Garzik wrote:
> Actually I disagree with GregKH on this.
> 
> The register/unregister functions need to be returning error codes, 
> _not_ the count of interfaces registered.  It is trivial to count the 
> registered interfaces in the driver itself, but IMO far more important 
> to propagate fatal errors back to the original caller.

Nevermind, this got fixed.  I'm still worried about the '1' return 
value, though, for zero controllers found.

	Jeff

Re: [PATCH] cpqarray update

[Jeff Garzik]

Hollis Blanchard wrote:
> I'm defining a new bus and had copied pci_module_init() to 
> vio_module_init(). Here's what Greg KH had to say about that:
> 
>> Eeek!  I want to fix that code in pci_module_init() so it doesn't do
>> this at all.  Please don't copy that horrible function.  Just register
>> the driver with a call to vio_register_driver() and drop the whole
>> vio_module_init() completly.  I'll be doing that for pci soon, and
>> there's no reason you want to duplicate this broken logic (you always
>> want your module probe to succeed, for lots of reasons...)


Actually I disagree with GregKH on this.

The register/unregister functions need to be returning error codes, 
_not_ the count of interfaces registered.  It is trivial to count the 
registered interfaces in the driver itself, but IMO far more important 
to propagate fatal errors back to the original caller.

This is what pci_module_init() does... returns an error if an error 
occurred, zero if not.  Further, use of pci_module_init() makes drivers 
future-proof for a time when the API can better return fatal errors that 
occur during the registration process.

As it stands now, pci_register_driver() -can- call functions which can 
fail internally (see what driver_register calls...), unrelated to the 
driver, and the driver will never ever know this.

That is an ugly flaw in most current foo_register_driver() APIs.  Errors 
are silently lost :(

	Jeff

Re: [PATCH] cpqarray update

[Greg KH]

On Wed, Jan 28, 2004 at 05:10:29PM -0600, Wiran, Francis wrote:
> 
> Ok. Here's the patch for that. At least until vio_module_init comes :)

Heh, you didn't actually try that patch, did you?

(hint, you need to check for a negative value...)

greg k-h

RE: [PATCH] cpqarray update

[Wiran, Francis]

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


Ok. Here's the patch for that. At least until vio_module_init comes :)

thanks
-francis-



> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com] 
> Sent: Wednesday, January 28, 2004 11:41 AM
> To: Hollis Blanchard
> Cc: Jeff Garzik; Linux Kernel Mailing List; Wiran, Francis
> Subject: Re: [PATCH] cpqarray update
> 
>...
> Well, changing it back to pci_register_driver() and actually checking
> the return value would be a good idea :)
> 
> thanks,
> 
> greg k-h
> 

[-- Attachment #2: cpqarray_pci_unregister.patch --]
[-- Type: application/octet-stream, Size: 638 bytes --]


   * Checks the rc of pci_register_driver and unregister when necessary


 drivers/block/cpqarray.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

--- linux-2.4.24/drivers/block/cpqarray.c~cpqarray_pci_unregister	Wed Jan 28 16:56:39 2004
+++ linux-2.4.24-root/drivers/block/cpqarray.c	Wed Jan 28 17:01:10 2004
@@ -623,7 +623,8 @@ int __init cpqarray_init(void)
 
 	/* detect controllers */
 	printk(DRIVER_NAME "\n");
-	pci_register_driver(&cpqarray_pci_driver);
+	if (!pci_register_driver(&cpqarray_pci_driver))
+		pci_unregister_driver(&cpqarray_pci_driver);
 	cpqarray_eisa_detect();
 
 	for(i=0; i< MAX_CTLR; i++) {

_

RE: [PATCH] cpqarray update

[Wiran, Francis]

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


Ok, here's the new patch without that hunk. 



> -----Original Message-----
> From: Hollis Blanchard [mailto:hollisb@us.ibm.com] 
> Sent: Wednesday, January 28, 2004 9:46 AM
> To: Jeff Garzik
> Cc: Linux Kernel Mailing List; Wiran, Francis; Greg KH
> Subject: Re: [PATCH] cpqarray update
> 
> 
> On Jan 26, 2004, at 2:15 PM, Jeff Garzik wrote:
> 
> > Linux Kernel Mailing List wrote:
> >> ChangeSet 1.1288, 2004/01/26 16:58:21-02:00, francis.wiran@hp.com
> >> @@ -616,7 +623,7 @@
> >>   	/* detect controllers */
> >>  	printk(DRIVER_NAME "\n");
> >> -	pci_register_driver(&cpqarray_pci_driver);
> >> +	pci_module_init(&cpqarray_pci_driver);
> >>  	cpqarray_eisa_detect();
> >>   	for(i=0; i< MAX_CTLR; i++) {
> >
> > You need to check the return value of pci_module_init() for errors.
> 
> I'm defining a new bus and had copied pci_module_init() to 
> vio_module_init(). Here's what Greg KH had to say about that:
> > Eeek!  I want to fix that code in pci_module_init() so it doesn't do
> > this at all.  Please don't copy that horrible function.  
> Just register
> > the driver with a call to vio_register_driver() and drop the whole
> > vio_module_init() completly.  I'll be doing that for pci soon, and
> > there's no reason you want to duplicate this broken logic 
> (you always
> > want your module probe to succeed, for lots of reasons...)
> 
> So there's no need for the quoted patch hunk at all.
> 
> -- 
> Hollis Blanchard
> IBM Linux Technology Center
> 
> 

[-- Attachment #2: cpqarray_eisa_detect_fix.patch --]
[-- Type: application/octet-stream, Size: 8813 bytes --]


   * Fix for eisa card not detecting partitions properly
   * Adds BLKSSZGET ioctl


 drivers/block/cpqarray.c |  213 ++++++++++++++++++++++++-----------------------
 1 files changed, 113 insertions(+), 100 deletions(-)

--- linux-2.4.24/drivers/block/cpqarray.c~cpqarray_eisa_detect_fix	Fri Jan 16 13:06:34 2004
+++ linux-2.4.24-root/drivers/block/cpqarray.c	Wed Jan 28 16:42:41 2004
@@ -41,13 +41,13 @@
 
 #define SMART2_DRIVER_VERSION(maj,min,submin) ((maj<<16)|(min<<8)|(submin))
 
-#define DRIVER_NAME "Compaq SMART2 Driver (v 2.4.25)"
-#define DRIVER_VERSION SMART2_DRIVER_VERSION(2,4,25)
+#define DRIVER_NAME "Compaq SMART2 Driver (v 2.4.28)"
+#define DRIVER_VERSION SMART2_DRIVER_VERSION(2,4,28)
 
 /* Embedded module documentation macros - see modules.h */
 /* Original author Chris Frantz - Compaq Computer Corporation */
 MODULE_AUTHOR("Compaq Computer Corporation");
-MODULE_DESCRIPTION("Driver for Compaq Smart2 Array Controllers version 2.4.25");
+MODULE_DESCRIPTION("Driver for Compaq Smart2 Array Controllers version 2.4.28");
 MODULE_LICENSE("GPL");
 
 #define MAJOR_NR COMPAQ_SMART2_MAJOR
@@ -180,6 +180,7 @@ static int revalidate_allvol(kdev_t dev)
 
 static int deregister_disk(int ctlr, int logvol);
 static int register_new_disk(int cltr,int logvol);
+static int cpqarray_register_ctlr(int ctlr, int type);
 
 #ifdef CONFIG_PROC_FS
 static void ida_procinit(int i);
@@ -470,10 +471,111 @@ static int cpq_merge_requests_fn(request
 	return 1;
 }
 
+static int cpqarray_register_ctlr(int ctlr, int type)
+{
+	request_queue_t *q;
+	int j;
+	
+	/*
+	 * register block devices
+	 * Find disks and fill in structs
+	 * Get an interrupt, set the Q depth and get into /proc
+	 */
+
+	/* If this successful it should insure that we are the only */
+	/* instance of the driver for this card */
+	if (register_blkdev(MAJOR_NR+ctlr, hba[ctlr]->devname, &ida_fops)) {
+		printk(KERN_ERR "cpqarray: Unable to get major number %d\n", MAJOR_NR+ctlr);
+		goto err_out;
+	}
+
+	hba[ctlr]->access.set_intr_mask(hba[ctlr], 0);
+	if (request_irq(hba[ctlr]->intr, do_ida_intr,
+		SA_INTERRUPT|SA_SHIRQ|SA_SAMPLE_RANDOM,
+		hba[ctlr]->devname, hba[ctlr])) {
+		printk(KERN_ERR "cpqarray: Unable to get irq %d for %s\n",
+			hba[ctlr]->intr, hba[ctlr]->devname);
+		unregister_blkdev(MAJOR_NR+ctlr, hba[ctlr]->devname);
+		goto err_out;
+	}
+	hba[ctlr]->cmd_pool = (cmdlist_t *)pci_alloc_consistent(
+		hba[ctlr]->pci_dev, NR_CMDS * sizeof(cmdlist_t),
+		&(hba[ctlr]->cmd_pool_dhandle));
+	hba[ctlr]->cmd_pool_bits = (__u32*)kmalloc(
+		((NR_CMDS+31)/32)*sizeof(__u32), GFP_KERNEL);
+
+	if (hba[ctlr]->cmd_pool_bits == NULL || hba[ctlr]->cmd_pool == NULL) {
+		if (hba[ctlr]->cmd_pool_bits)
+			kfree(hba[ctlr]->cmd_pool_bits);
+		if (hba[ctlr]->cmd_pool)
+			pci_free_consistent(hba[ctlr]->pci_dev,
+				NR_CMDS * sizeof(cmdlist_t),
+				hba[ctlr]->cmd_pool,
+				hba[ctlr]->cmd_pool_dhandle);
+
+		free_irq(hba[ctlr]->intr, hba[ctlr]);
+		unregister_blkdev(MAJOR_NR+ctlr, hba[ctlr]->devname);
+		printk( KERN_ERR "cpqarray: out of memory");
+		goto err_out;
+	}
+	memset(hba[ctlr]->cmd_pool, 0, NR_CMDS * sizeof(cmdlist_t));
+	memset(hba[ctlr]->cmd_pool_bits, 0, ((NR_CMDS+31)/32)*sizeof(__u32));
+	printk(KERN_INFO "cpqarray: Finding drives on %s", hba[ctlr]->devname);
+	getgeometry(ctlr);
+	start_fwbk(ctlr);
+
+	hba[ctlr]->access.set_intr_mask(hba[ctlr], FIFO_NOT_EMPTY);
+
+	ida_procinit(ctlr);
+
+	q = BLK_DEFAULT_QUEUE(MAJOR_NR + ctlr);
+	q->queuedata = hba[ctlr];
+	blk_init_queue(q, do_ida_request);
+	if (type)
+		blk_queue_bounce_limit(q, hba[ctlr]->pci_dev->dma_mask);
+	blk_queue_headactive(q, 0);
+	blksize_size[MAJOR_NR+ctlr] = hba[ctlr]->blocksizes;
+	hardsect_size[MAJOR_NR+ctlr] = hba[ctlr]->hardsizes;
+	read_ahead[MAJOR_NR+ctlr] = READ_AHEAD;
+
+	q->back_merge_fn = cpq_back_merge_fn;
+	q->front_merge_fn = cpq_front_merge_fn;
+	q->merge_requests_fn = cpq_merge_requests_fn;
+
+	hba[ctlr]->gendisk.major = MAJOR_NR + ctlr;
+	hba[ctlr]->gendisk.major_name = "ida";
+	hba[ctlr]->gendisk.minor_shift = NWD_SHIFT;
+	hba[ctlr]->gendisk.max_p = IDA_MAX_PART;
+	hba[ctlr]->gendisk.part = hba[ctlr]->hd;
+	hba[ctlr]->gendisk.sizes = hba[ctlr]->sizes;
+	hba[ctlr]->gendisk.nr_real = hba[ctlr]->highest_lun+1;
+	hba[ctlr]->gendisk.fops = &ida_fops;
+
+	/* Get on the disk list */
+	add_gendisk(&(hba[ctlr]->gendisk));
+
+	init_timer(&hba[ctlr]->timer);
+	hba[ctlr]->timer.expires = jiffies + IDA_TIMER;
+	hba[ctlr]->timer.data = (unsigned long)hba[ctlr];
+	hba[ctlr]->timer.function = ida_timer;
+	add_timer(&hba[ctlr]->timer);
+
+	ida_geninit(ctlr);
+	for(j=0; j<NWD; j++)
+		register_disk(&(hba[ctlr]->gendisk), MKDEV(MAJOR_NR+ctlr,j<<4),
+			IDA_MAX_PART, &ida_fops, hba[ctlr]->drv[j].nr_blks);
+	return(ctlr);
+
+err_out:
+	release_io_mem(hba[ctlr]);
+	free_hba(ctlr);
+	return (-1);
+}
+
+
 static int __init cpqarray_init_one( struct pci_dev *pdev,
 	const struct pci_device_id *ent)
 {
-	request_queue_t *q;
         int i,j;
 
 
@@ -500,102 +602,7 @@ static int __init cpqarray_init_one( str
 		return (-1);
 	}
 			
-	/* 
-	 * register block devices
-	 * Find disks and fill in structs
-	 * Get an interrupt, set the Q depth and get into /proc
-	 */
-	
-	/* If this successful it should insure that we are the only */
-	/* instance of the driver */	
-	if (register_blkdev(MAJOR_NR+i, hba[i]->devname, &ida_fops)) {
-        	printk(KERN_ERR "cpqarray: Unable to get major number %d for ida\n",
-                                MAJOR_NR+i);
-                release_io_mem(hba[i]);
-                free_hba(i);
-                return (-1);
-        }
-
-	
-	hba[i]->access.set_intr_mask(hba[i], 0);
-	if (request_irq(hba[i]->intr, do_ida_intr,
-		SA_INTERRUPT | SA_SHIRQ | SA_SAMPLE_RANDOM, 
-				hba[i]->devname, hba[i])) {
-
-		printk(KERN_ERR "cpqarray: Unable to get irq %d for %s\n", 
-				hba[i]->intr, hba[i]->devname);
-		unregister_blkdev(MAJOR_NR+i, hba[i]->devname);
-		release_io_mem(hba[i]);
-                free_hba(i);
-                return (-1);
-	}
-	hba[i]->cmd_pool = (cmdlist_t *)pci_alloc_consistent(
-	hba[i]->pci_dev, NR_CMDS * sizeof(cmdlist_t), 
-				&(hba[i]->cmd_pool_dhandle));
-	hba[i]->cmd_pool_bits = (__u32*)kmalloc(
-				((NR_CMDS+31)/32)*sizeof(__u32), GFP_KERNEL);
-		
-	if(hba[i]->cmd_pool_bits == NULL || hba[i]->cmd_pool == NULL) {
-		if(hba[i]->cmd_pool_bits)
-			kfree(hba[i]->cmd_pool_bits);
-		if(hba[i]->cmd_pool)
-			pci_free_consistent(hba[i]->pci_dev, 
-				NR_CMDS * sizeof(cmdlist_t), 
-				hba[i]->cmd_pool, hba[i]->cmd_pool_dhandle);
-		free_irq(hba[i]->intr, hba[i]);
-		unregister_blkdev(MAJOR_NR+i, hba[i]->devname);
-                printk( KERN_ERR "cpqarray: out of memory");
-		release_io_mem(hba[i]);
-                free_hba(i);
-                return (-1);
-	}
-	memset(hba[i]->cmd_pool, 0, NR_CMDS * sizeof(cmdlist_t));
-	memset(hba[i]->cmd_pool_bits, 0, ((NR_CMDS+31)/32)*sizeof(__u32));
-	printk(KERN_INFO "cpqarray: Finding drives on %s", hba[i]->devname);
-	getgeometry(i);
-	start_fwbk(i); 
-
-	hba[i]->access.set_intr_mask(hba[i], FIFO_NOT_EMPTY);
-
-	ida_procinit(i);
-
-	q = BLK_DEFAULT_QUEUE(MAJOR_NR + i);
-	q->queuedata = hba[i];
-	blk_init_queue(q, do_ida_request);
-	blk_queue_bounce_limit(q, hba[i]->pci_dev->dma_mask);
-	blk_queue_headactive(q, 0);
-	blksize_size[MAJOR_NR+i] = hba[i]->blocksizes;
-	hardsect_size[MAJOR_NR+i] = hba[i]->hardsizes;
-	read_ahead[MAJOR_NR+i] = READ_AHEAD;
-
-	q->back_merge_fn = cpq_back_merge_fn;
-	q->front_merge_fn = cpq_front_merge_fn;
-	q->merge_requests_fn = cpq_merge_requests_fn;
-
-	hba[i]->gendisk.major = MAJOR_NR + i;
-	hba[i]->gendisk.major_name = "ida";
-	hba[i]->gendisk.minor_shift = NWD_SHIFT;
-	hba[i]->gendisk.max_p = IDA_MAX_PART;
-	hba[i]->gendisk.part = hba[i]->hd;
-	hba[i]->gendisk.sizes = hba[i]->sizes;
-	hba[i]->gendisk.nr_real = hba[i]->highest_lun+1; 
-	hba[i]->gendisk.fops = &ida_fops;
-	
-		/* Get on the disk list */
-	add_gendisk(&(hba[i]->gendisk));
-
-	init_timer(&hba[i]->timer);
-	hba[i]->timer.expires = jiffies + IDA_TIMER;
-	hba[i]->timer.data = (unsigned long)hba[i];
-	hba[i]->timer.function = ida_timer;
-	add_timer(&hba[i]->timer);
-
-	ida_geninit(i);
-	for(j=0; j<NWD; j++)	
-		register_disk(&(hba[i]->gendisk), MKDEV(MAJOR_NR+i,j<<4),
-			IDA_MAX_PART, &ida_fops, hba[i]->drv[j].nr_blks);
-
-	return(i);
+	return (cpqarray_register_ctlr(i, 1));
 }
 static struct pci_driver cpqarray_pci_driver = {
         name:   "cpqarray",
@@ -868,6 +875,11 @@ DBGINFO(
 
 		num_ctlr++;
 		i++;
+
+		if (cpqarray_register_ctlr(ctlr, 0) == -1)
+			printk(KERN_WARNING 
+				"cpqarray%d: Can't register EISA controller\n",
+				ctlr);
 	}
 
 	return num_ctlr;
@@ -1377,6 +1389,7 @@ static int ida_ioctl(struct inode *inode
 	case BLKFLSBUF:
 	case BLKBSZSET:
 	case BLKBSZGET:
+	case BLKSSZGET:
 	case BLKROSET:
 	case BLKROGET:
 	case BLKRASET:

_

Re: [PATCH] cpqarray update

[Greg KH]

On Wed, Jan 28, 2004 at 09:46:06AM -0600, Hollis Blanchard wrote:
> On Jan 26, 2004, at 2:15 PM, Jeff Garzik wrote:
> 
> >Linux Kernel Mailing List wrote:
> >>ChangeSet 1.1288, 2004/01/26 16:58:21-02:00, francis.wiran@hp.com
> >>@@ -616,7 +623,7 @@
> >>  	/* detect controllers */
> >> 	printk(DRIVER_NAME "\n");
> >>-	pci_register_driver(&cpqarray_pci_driver);
> >>+	pci_module_init(&cpqarray_pci_driver);
> >> 	cpqarray_eisa_detect();
> >>  	for(i=0; i< MAX_CTLR; i++) {
> >
> >You need to check the return value of pci_module_init() for errors.
> 
> I'm defining a new bus and had copied pci_module_init() to 
> vio_module_init(). Here's what Greg KH had to say about that:
> >Eeek!  I want to fix that code in pci_module_init() so it doesn't do
> >this at all.  Please don't copy that horrible function.  Just register
> >the driver with a call to vio_register_driver() and drop the whole
> >vio_module_init() completly.  I'll be doing that for pci soon, and
> >there's no reason you want to duplicate this broken logic (you always
> >want your module probe to succeed, for lots of reasons...)
> 
> So there's no need for the quoted patch hunk at all.

Well, changing it back to pci_register_driver() and actually checking
the return value would be a good idea :)

thanks,

greg k-h

Re: [PATCH] cpqarray update

[Hollis Blanchard]

On Jan 26, 2004, at 2:15 PM, Jeff Garzik wrote:

> Linux Kernel Mailing List wrote:
>> ChangeSet 1.1288, 2004/01/26 16:58:21-02:00, francis.wiran@hp.com
>> @@ -616,7 +623,7 @@
>>   	/* detect controllers */
>>  	printk(DRIVER_NAME "\n");
>> -	pci_register_driver(&cpqarray_pci_driver);
>> +	pci_module_init(&cpqarray_pci_driver);
>>  	cpqarray_eisa_detect();
>>   	for(i=0; i< MAX_CTLR; i++) {
>
> You need to check the return value of pci_module_init() for errors.

I'm defining a new bus and had copied pci_module_init() to 
vio_module_init(). Here's what Greg KH had to say about that:
> Eeek!  I want to fix that code in pci_module_init() so it doesn't do
> this at all.  Please don't copy that horrible function.  Just register
> the driver with a call to vio_register_driver() and drop the whole
> vio_module_init() completly.  I'll be doing that for pci soon, and
> there's no reason you want to duplicate this broken logic (you always
> want your module probe to succeed, for lots of reasons...)

So there's no need for the quoted patch hunk at all.

-- 
Hollis Blanchard
IBM Linux Technology Center

RE: [PATCH] cpqarray update

[Wiran, Francis]

Then again, maybe leaving that part alone would be ok too? i.e: keep
using pci_module_init() ... Any opinions?

thanks
-francis-

> -----Original Message-----
> From: Jeff Garzik [mailto:jgarzik@pobox.com] 
> Sent: Monday, January 26, 2004 7:52 PM
> To: Wiran, Francis
> Cc: Linux Kernel Mailing List
> Subject: Re: [PATCH] cpqarray update
> 
> 
> Wiran, Francis wrote:
> >>You need to check the return value of pci_module_init() for errors.
> > 
> > No, because the return value is determined from number of 
> ctrls found,
> > and not from function return.
> > 
> > int __init cpqarray_init(void)
> > {
> > ...
> > 	pci_module_init(&cpqarray_pci_driver);
> > 	cpqarray_eisa_detect();
> > 
> > 	for(i=0;i<MAX_CTLR;i++) {
> > 		if(hba[i] != NULL)
> > 			num_ctlrs_reg++
> > 	}
> > 
> > 	return (num_ctlrs_reg);
> > }
> > 
> > int __init cpqarray_init_module(void)
> > {
> > 	if (cpqarray_init() == 0)
> > 		return -ENODEV;
> > 	return 0;
> > }
> 
> 
> Nope, this needs to be turned inside out.  The proper PCI 
> driver looks like
> 
> static int __init cp_init (void)
> {
>          return pci_module_init (&cp_driver);
> }
> 
> static void __exit cp_exit (void)
> {
>          pci_unregister_driver (&cp_driver);
> }
> 
> We already handle the cases you describe.  The cpqarray code -breaks- 
> the API design by doing it this way.
> 
> cpqarray does not fully support the pci_ids features and 
> hotplug without 
> this.
> 
> 	Jeff
> 
> 
> 
> 

RE: [PATCH] cpqarray update

[Wiran, Francis]

Hmm... cpqarray supports both pci and eisa ctrls.

What would happen if there are no pci ctrl detected, but there is one
eisa ctrl detected? What do we return in cpqarray_init()? The return
code of pci_module_init is either 0 or -ENODEV. What happen if
cpqarray_init returns -ENODEV? Would the driver stay loaded? How about
0, or positive number?


thanks
-francis-

> -----Original Message-----
> From: Jeff Garzik [mailto:jgarzik@pobox.com] 
> Sent: Monday, January 26, 2004 7:52 PM
> To: Wiran, Francis
> Cc: Linux Kernel Mailing List
> Subject: Re: [PATCH] cpqarray update
> 
> 
> Wiran, Francis wrote:
> >>You need to check the return value of pci_module_init() for errors.
> > 
> > No, because the return value is determined from number of 
> ctrls found,
> > and not from function return.
> > 
> > int __init cpqarray_init(void)
> > {
> > ...
> > 	pci_module_init(&cpqarray_pci_driver);
> > 	cpqarray_eisa_detect();
> > 
> > 	for(i=0;i<MAX_CTLR;i++) {
> > 		if(hba[i] != NULL)
> > 			num_ctlrs_reg++
> > 	}
> > 
> > 	return (num_ctlrs_reg);
> > }
> > 
> > int __init cpqarray_init_module(void)
> > {
> > 	if (cpqarray_init() == 0)
> > 		return -ENODEV;
> > 	return 0;
> > }
> 
> 
> Nope, this needs to be turned inside out.  The proper PCI 
> driver looks like
> 
> static int __init cp_init (void)
> {
>          return pci_module_init (&cp_driver);
> }
> 
> static void __exit cp_exit (void)
> {
>          pci_unregister_driver (&cp_driver);
> }
> 
> We already handle the cases you describe.  The cpqarray code -breaks- 
> the API design by doing it this way.
> 
> cpqarray does not fully support the pci_ids features and 
> hotplug without 
> this.
> 
> 	Jeff
> 
> 
> 
> 

Re: [PATCH] cpqarray update

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> ChangeSet 1.1288, 2004/01/26 16:58:21-02:00, francis.wiran@hp.com
> 
> 	[PATCH] cpqarray update
> 	
> 	Yes, we should. I usually ask Mike Miller in our group to send my
> 	patches since he's been doing that and more known in the community, but
> 	since you already got a hold of it ...yes, please :)
> 	
> 	CHANGELOG:
> 	
> 	   * Fix for eisa card not detecting partitions properly
> 	   * Use pci_module_init instead of pci_register_device to handle hotplug
> 	     scenario and unregister if the driver can't find pci controller
> 	   * Add BLKSSZGET ioctl
[...]
> @@ -616,7 +623,7 @@
>  
>  	/* detect controllers */
>  	printk(DRIVER_NAME "\n");
> -	pci_register_driver(&cpqarray_pci_driver);
> +	pci_module_init(&cpqarray_pci_driver);
>  	cpqarray_eisa_detect();
>  
>  	for(i=0; i< MAX_CTLR; i++) {

You need to check the return value of pci_module_init() for errors.

	Jeff