Merge I2O patches from -mm

Merge I2O patches from -mm

[Warren Togami]

This is a request to please merge the I2O patches currently in Andrew 
Morton's -mm tree into the mainline kernel.  They resolve all known 
reported issues with I2O RAID devices.  If they can be included soon, it 
would be possible to implement and test direct installation before FC3 
Test2 freeze.

Also because Markus would never ask himself, I nominate Markus Lidel as 
the "maintainer" of the 2.6 generic I2O layer.  He has put a tremendous 
amount of work into improving an otherwise neglected part of the kernel. 
  Thanks to his efforts it is today usable and stable on multiple archs 
and all known supported cards.

Thank you,
Warren Togami
wtogami@redhat.com

Re: Merge I2O patches from -mm

[Andrew Morton]

Warren Togami <wtogami@redhat.com> wrote:
>
> This is a request to please merge the I2O patches currently in Andrew 
> Morton's -mm tree into the mainline kernel.  They resolve all known 
> reported issues with I2O RAID devices.  If they can be included soon, it 
> would be possible to implement and test direct installation before FC3 
> Test2 freeze.

We'll get it in when Linus returns.

> Also because Markus would never ask himself, I nominate Markus Lidel as 
> the "maintainer" of the 2.6 generic I2O layer.  He has put a tremendous 
> amount of work into improving an otherwise neglected part of the kernel. 

No problem with that, but we wouldn't want to go adding Markus to
./MAINTAINERS without his approval.

>   Thanks to his efforts it is today usable and stable on multiple archs 
> and all known supported cards.

yup.

Re: Merge I2O patches from -mm

[Markus Lidel]

Hi...

Andrew Morton wrote:
> Warren Togami <wtogami@redhat.com> wrote:
>>This is a request to please merge the I2O patches currently in Andrew 
>>Morton's -mm tree into the mainline kernel.  They resolve all known 
>>reported issues with I2O RAID devices.  If they can be included soon, it 
>>would be possible to implement and test direct installation before FC3 
>>Test2 freeze.
> We'll get it in when Linus returns.

Wowww, great news \:D/

>>Also because Markus would never ask himself, I nominate Markus Lidel as 
>>the "maintainer" of the 2.6 generic I2O layer.  He has put a tremendous 
>>amount of work into improving an otherwise neglected part of the kernel. 
> No problem with that, but we wouldn't want to go adding Markus to
> ./MAINTAINERS without his approval.

It would be a great honor for me to be the maintainer of the I2O subsystem.


Best regads,



Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: Merge I2O patches from -mm

[Christoph Hellwig]

On Sun, Aug 15, 2004 at 12:15:40AM -1000, Warren Togami wrote:
> This is a request to please merge the I2O patches currently in Andrew 
> Morton's -mm tree into the mainline kernel.  They resolve all known 
> reported issues with I2O RAID devices.  If they can be included soon, it 
> would be possible to implement and test direct installation before FC3 
> Test2 freeze.

I've looked over it and except for the i2o_scsi driver it looks sane.

Cosmetic fixups I'd like to see done befoee merging to mainline:

 - run the code through Lindent
 - stop the needless file renaming.  Splitting up i2o_core.c into multiple
   files is fine, but please don' rename the other drivers for the sake of
   it

Now to i2o_scsi:

 - the logic of "demand-allocating" Scsi_Hosts looks rather bad to me,
   life would be much simpler with a Scsi_Host per i2o device.
 - the slave_configure/i2o_scsi_probe_dev logical is quite horriblebut
   fortunately with the suggestion above it would just go away
 - the global list of hosts and wlaking it on exit is a very bad design,
   that's something the ->remove callback should do on per-device basis
 - the completely lack of SCSI EH in this driver scares me, does the firmware
   really handle all EH?

cosemtic stuff in here:

 - <asm/*.h> after <linux/*.h>.
 - please include scsi headers using <scsi/*.h> (after linux and asm headers)
 - please use the standard pr_Debug instead of DBG
 - please reorder the functions a little so you don't need forward-declarations

Re: Merge I2O patches from -mm

[Markus Lidel]

Hi...

Christoph Hellwig wrote:
> On Sun, Aug 15, 2004 at 12:15:40AM -1000, Warren Togami wrote:
>>This is a request to please merge the I2O patches currently in Andrew 
>>Morton's -mm tree into the mainline kernel.  They resolve all known 
>>reported issues with I2O RAID devices.  If they can be included soon, it 
>>would be possible to implement and test direct installation before FC3 
>>Test2 freeze.
> I've looked over it and except for the i2o_scsi driver it looks sane.
> Cosmetic fixups I'd like to see done befoee merging to mainline:
>  - run the code through Lindent

Okay, will do it...

>  - stop the needless file renaming.  Splitting up i2o_core.c into multiple
>    files is fine, but please don' rename the other drivers for the sake of
>    it

just wanted to be consistent with the other files, but it shouldn't be a 
problem to rename them to the original name...

> Now to i2o_scsi:
>  - the logic of "demand-allocating" Scsi_Hosts looks rather bad to me,
>    life would be much simpler with a Scsi_Host per i2o device.

But wouldn't it be a waste of resources to allocate a Scsi_Host 
structure for every I2O device? Note that the i2o_scsi "sees" all disks 
even if they are in a RAID array, so in most cases there are at least 3 
Scsi_Host adapters...

We also now know which disk is on which controller, this information is 
lost with your approach...

>  - the slave_configure/i2o_scsi_probe_dev logical is quite horriblebut
>    fortunately with the suggestion above it would just go away

Yep, i know that it would be better to extend scsi_add_device, so it's 
possible to pass a pointer to i2o_scsi_slave_alloc. This is only a 
workaround, which breaks as soon as things are done in parallel :-(

>  - the global list of hosts and wlaking it on exit is a very bad design,
>    that's something the ->remove callback should do on per-device basis

But what if the I2O device isn't removed?

>  - the completely lack of SCSI EH in this driver scares me, does the firmware
>    really handle all EH?

The i2o_scsi driver is not used to access the disks, it is only for 
monitoring. The i2o_block driver handles disk access. So if you reset 
the SCSI channel in the i2o_scsi driver, commands which are transfered 
by the i2o_block driver will be aborted (this is the reason, why the I2O 
subsystem didn't work for users, which compiled in i2o_scsi and 
i2o_block into the kernel)...

> cosemtic stuff in here:
>  - <asm/*.h> after <linux/*.h>.
>  - please include scsi headers using <scsi/*.h> (after linux and asm headers)
>  - please use the standard pr_Debug instead of DBG

This is what i searched for the whole time :-) Thanks for the hint!

>  - please reorder the functions a little so you don't need forward-declarations

most of the forward-declarations are not needed at all, should i remove 
unneeded completely?


Thanks for taking time to review my changes!


Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: Merge I2O patches from -mm

[Alan Cox]

On Maw, 2004-08-17 at 14:31, Markus Lidel wrote:
> > Now to i2o_scsi:
> >  - the logic of "demand-allocating" Scsi_Hosts looks rather bad to me,
> >    life would be much simpler with a Scsi_Host per i2o device.
> 
> But wouldn't it be a waste of resources to allocate a Scsi_Host 
> structure for every I2O device? Note that the i2o_scsi "sees" all disks 
> even if they are in a RAID array, so in most cases there are at least 3 
> Scsi_Host adapters...

Christoph the "I2O" device is a communication processor. You need to
preserve the real scsi busses in order to get sane results from scsi
tools. If EH is implemented you'll need this to do controlled resets
(although this gets quite umm 'interesting' if using i2o_block also)

You've got

	I2O comms processor
		Multiple scsi busses
			Multiple drives
		Block driver providing abstract volumes

Sometimes (as with other raid) bypassing the block driver is faster.

> >  - the completely lack of SCSI EH in this driver scares me, does the firmware
> >    really handle all EH?
> 
> The i2o_scsi driver is not used to access the disks, it is only for 
> monitoring. The i2o_block driver handles disk access. So if you reset 

(not always - the FC920 is way faster using i2o_scsi for the disk I/O)

> the SCSI channel in the i2o_scsi driver, commands which are transfered 
> by the i2o_block driver will be aborted (this is the reason, why the I2O 
> subsystem didn't work for users, which compiled in i2o_scsi and 
> i2o_block into the kernel)...

All error timeout handling is done by the controller. It does really
need minimal EH handlers simply to reissue failed timed out commands.

Re: Merge I2O patches from -mm

[Alan Cox]

On Maw, 2004-08-17 at 16:06, Christoph Hellwig wrote:
> Okay, then we'll have to rething the data structures a little more.
> I was under the impression the scsi busses were completely faked for
> the OS.

Nope. I can send you the i2o 1.5 docs if you would like but they are
fairly heavy reading, and a testimony to a neat hardware design and
concept destroyed by committee.

Re: Merge I2O patches from -mm

[Christoph Hellwig]

On Tue, Aug 17, 2004 at 03:00:59PM +0100, Alan Cox wrote:
> On Maw, 2004-08-17 at 14:31, Markus Lidel wrote:
> > > Now to i2o_scsi:
> > >  - the logic of "demand-allocating" Scsi_Hosts looks rather bad to me,
> > >    life would be much simpler with a Scsi_Host per i2o device.
> > 
> > But wouldn't it be a waste of resources to allocate a Scsi_Host 
> > structure for every I2O device? Note that the i2o_scsi "sees" all disks 
> > even if they are in a RAID array, so in most cases there are at least 3 
> > Scsi_Host adapters...
> 
> Christoph the "I2O" device is a communication processor. You need to
> preserve the real scsi busses in order to get sane results from scsi
> tools. If EH is implemented you'll need this to do controlled resets
> (although this gets quite umm 'interesting' if using i2o_block also)

Okay, then we'll have to rething the data structures a little more.
I was under the impression the scsi busses were completely faked for
the OS.

Re: Merge I2O patches from -mm

[Christoph Hellwig]

On Tue, Aug 17, 2004 at 03:31:18PM +0200, Markus Lidel wrote:
> > Now to i2o_scsi:
> >  - the logic of "demand-allocating" Scsi_Hosts looks rather bad to me,
> >    life would be much simpler with a Scsi_Host per i2o device.
> 
> But wouldn't it be a waste of resources to allocate a Scsi_Host 
> structure for every I2O device? Note that the i2o_scsi "sees" all disks 
> even if they are in a RAID array, so in most cases there are at least 3 
> Scsi_Host adapters...

I wouldn't wasted ressources but it seems Alan found another problem.

> We also now know which disk is on which controller, this information is 
> lost with your approach...

You could still keep that information in your data structure.  But what
do you actually need it for?

> >  - the slave_configure/i2o_scsi_probe_dev logical is quite horriblebut
> >    fortunately with the suggestion above it would just go away
> 
> Yep, i know that it would be better to extend scsi_add_device, so it's 
> possible to pass a pointer to i2o_scsi_slave_alloc. This is only a 
> workaround, which breaks as soon as things are done in parallel :-(

Just keep some lookup data structure so you can find the device data
by host/target/lun numbers.

> >  - the global list of hosts and wlaking it on exit is a very bad design,
> >    that's something the ->remove callback should do on per-device basis
> 
> But what if the I2O device isn't removed?

you're using the driver model, and that calls ->remove and every device
when the driver is unregistered.

> >  - please reorder the functions a little so you don't need forward-declarations
> 
> most of the forward-declarations are not needed at all, should i remove 
> unneeded completely?

Yes, please.

Okay, some brainstorming to get the data structures and lookup right:

What really seems to miss in your model is a callback to i2o_scsi
from the main i2o code when a new i2o_controller is found, if you implemented
that we'd allocate/deallocate the Scsi_Host in that callback and
->probe/->remove could be sure it'd always have it.

Anyway, I think we could live without that.

i2o_scsi_get_host would get inlined into i2o_scsi_probe.
i2o_scsi_remove basically needs a full rewrite, you need to find a way
to store a scsi_device ini i2o_dev and it becomes completely trivial.

Not sure about how to sanitize the scsi_devie probing logic
in i2o_scsi_probe yet.

Re: Merge I2O patches from -mm

[Christoph Hellwig]

The patch below adds a __scsi_add_device that can "preload" sdev->hostdata.


--- 1.127/drivers/scsi/scsi_scan.c	2004-06-25 13:56:28 +02:00
+++ edited/drivers/scsi/scsi_scan.c	2004-08-17 20:41:07 +02:00
@@ -200,7 +200,7 @@
  *     scsi_Device pointer, or NULL on failure.
  **/
 static struct scsi_device *scsi_alloc_sdev(struct Scsi_Host *shost,
-	       	uint channel, uint id, uint lun)
+	       	uint channel, uint id, uint lun, void *hostdata)
 {
 	struct scsi_device *sdev, *device;
 	unsigned long flags;
@@ -224,6 +224,8 @@
 	INIT_LIST_HEAD(&sdev->starved_entry);
 	spin_lock_init(&sdev->list_lock);
 
+	/* usually NULL and set by ->slave_alloc instead */
+	sdev->hostdata = hostdata;
 
 	/* if the device needs this changing, it may do so in the
 	 * slave_configure function */
@@ -697,7 +699,7 @@
  **/
 static int scsi_probe_and_add_lun(struct Scsi_Host *host,
 		uint channel, uint id, uint lun, int *bflagsp,
-		struct scsi_device **sdevp, int rescan)
+		struct scsi_device **sdevp, int rescan, void *hostdata)
 {
 	struct scsi_device *sdev;
 	struct scsi_request *sreq;
@@ -726,7 +728,7 @@
 		}
 	}
 
-	sdev = scsi_alloc_sdev(host, channel, id, lun);
+	sdev = scsi_alloc_sdev(host, channel, id, lun, hostdata);
 	if (!sdev)
 		goto out;
 	sreq = scsi_allocate_request(sdev, GFP_ATOMIC);
@@ -874,7 +876,7 @@
 	 */
 	for (lun = 1; lun < max_dev_lun; ++lun)
 		if ((scsi_probe_and_add_lun(shost, channel, id, lun,
-		      NULL, NULL, rescan) != SCSI_SCAN_LUN_PRESENT) &&
+		      NULL, NULL, rescan, NULL) != SCSI_SCAN_LUN_PRESENT) &&
 		    !sparse_lun)
 			return;
 }
@@ -1085,7 +1087,7 @@
 			int res;
 
 			res = scsi_probe_and_add_lun(sdev->host, sdev->channel,
-				sdev->id, lun, NULL, NULL, rescan);
+				sdev->id, lun, NULL, NULL, rescan, NULL);
 			if (res == SCSI_SCAN_NO_RESPONSE) {
 				/*
 				 * Got some results, but now none, abort.
@@ -1111,14 +1113,15 @@
 	return 0;
 }
 
-struct scsi_device *scsi_add_device(struct Scsi_Host *shost,
-				    uint channel, uint id, uint lun)
+struct scsi_device *__scsi_add_device(struct Scsi_Host *shost, uint channel,
+		uint id, uint lun, void *hostdata)
 {
 	struct scsi_device *sdev;
 	int res;
 
 	down(&shost->scan_mutex);
-	res = scsi_probe_and_add_lun(shost, channel, id, lun, NULL, &sdev, 1);
+	res = scsi_probe_and_add_lun(shost, channel, id, lun, NULL,
+				     &sdev, 1, hostdata);
 	if (res != SCSI_SCAN_LUN_PRESENT)
 		sdev = ERR_PTR(-ENODEV);
 	up(&shost->scan_mutex);
@@ -1178,7 +1181,7 @@
 		 * Scan for a specific host/chan/id/lun.
 		 */
 		scsi_probe_and_add_lun(shost, channel, id, lun, NULL, NULL,
-				       rescan);
+				       rescan, NULL);
 		return;
 	}
 
@@ -1187,7 +1190,7 @@
 	 * would not configure LUN 0 until all LUNs are scanned.
 	 */
 	res = scsi_probe_and_add_lun(shost, channel, id, 0, &bflags, &sdev,
-				     rescan);
+				     rescan, NULL);
 	if (res == SCSI_SCAN_LUN_PRESENT) {
 		if (scsi_report_lun_scan(sdev, bflags, rescan) != 0)
 			/*
@@ -1316,7 +1319,7 @@
 {
 	struct scsi_device *sdev;
 
-	sdev = scsi_alloc_sdev(shost, 0, shost->this_id, 0);
+	sdev = scsi_alloc_sdev(shost, 0, shost->this_id, 0, NULL);
 	if (sdev) {
 		sdev->borken = 0;
 	}
===== drivers/scsi/scsi_syms.c 1.49 vs edited =====
--- 1.49/drivers/scsi/scsi_syms.c	2004-06-27 00:40:24 +02:00
+++ edited/drivers/scsi/scsi_syms.c	2004-08-17 20:50:22 +02:00
@@ -73,7 +73,7 @@
 
 EXPORT_SYMBOL(scsi_io_completion);
 
-EXPORT_SYMBOL(scsi_add_device);
+EXPORT_SYMBOL(__scsi_add_device);
 EXPORT_SYMBOL(scsi_remove_device);
 EXPORT_SYMBOL(scsi_device_cancel);
 
--- 1.19/include/scsi/scsi_device.h	2004-07-07 18:24:13 +02:00
+++ edited/include/scsi/scsi_device.h	2004-08-17 20:42:18 +02:00
@@ -129,8 +129,10 @@
 #define transport_class_to_sdev(class_dev) \
 	container_of(class_dev, struct scsi_device, transport_classdev)
 
-extern struct scsi_device *scsi_add_device(struct Scsi_Host *,
-		uint, uint, uint);
+extern struct scsi_device *__scsi_add_device(struct Scsi_Host *,
+		uint, uint, uint, void *hostdata);
+#define scsi_add_device(host, channel, target, lun) \
+	__scsi_add_device(host, channel, target, lun, NULL)
 extern void scsi_remove_device(struct scsi_device *);
 extern int scsi_device_cancel(struct scsi_device *, int);
 

Re: Merge I2O patches from -mm

[Markus Lidel]

Hello,

Christoph Hellwig wrote:
> On Tue, Aug 17, 2004 at 03:31:18PM +0200, Markus Lidel wrote:
>>>Now to i2o_scsi:
>>> - the logic of "demand-allocating" Scsi_Hosts looks rather bad to me,
>>>   life would be much simpler with a Scsi_Host per i2o device.
>>But wouldn't it be a waste of resources to allocate a Scsi_Host 
>>structure for every I2O device? Note that the i2o_scsi "sees" all disks 
>>even if they are in a RAID array, so in most cases there are at least 3 
>>Scsi_Host adapters...
> I wouldn't wasted ressources but it seems Alan found another problem.

What problem do you mean?

>>We also now know which disk is on which controller, this information is 
>>lost with your approach...
> You could still keep that information in your data structure.  But what
> do you actually need it for?

Okay, it's not that it is crucial, but why should we throw this 
information over board? What will we gain, if we throw it over board, 
and use a single Scsi_Host for each I2O device?

>>> - the slave_configure/i2o_scsi_probe_dev logical is quite horriblebut
>>>   fortunately with the suggestion above it would just go away
>>Yep, i know that it would be better to extend scsi_add_device, so it's 
>>possible to pass a pointer to i2o_scsi_slave_alloc. This is only a 
>>workaround, which breaks as soon as things are done in parallel :-(
> Just keep some lookup data structure so you can find the device data
> by host/target/lun numbers.

We already had this, but we don't need it at all... This structure would 
take unnecessary resources, and what is the benefit? We don't need the 
mapping table after initialization anymore. IMHO it's better to add a 
parameter to scsi_add_device, which will be passed to scsi_slave_alloc 
afterwords, or do it the same way like the Scsi_Host structure...

>>> - the global list of hosts and wlaking it on exit is a very bad design,
>>>   that's something the ->remove callback should do on per-device basis
>>But what if the I2O device isn't removed?
> you're using the driver model, and that calls ->remove and every device
> when the driver is unregistered.

Okay, i don't remember why i added this, but i will try to get rid of it 
  if it is possible (i agree with you, that it is ugly)...

> Okay, some brainstorming to get the data structures and lookup right:
> 
> What really seems to miss in your model is a callback to i2o_scsi
> from the main i2o code when a new i2o_controller is found, if you implemented
> that we'd allocate/deallocate the Scsi_Host in that callback and
> ->probe/->remove could be sure it'd always have it.

Okay, sounds very good to me! This also solves the problem, when a new 
controller is added at runtime :-)

I'll look at the PCI driver to do it the same way...

> Anyway, I think we could live without that.
> i2o_scsi_get_host would get inlined into i2o_scsi_probe.
> i2o_scsi_remove basically needs a full rewrite, you need to find a way
> to store a scsi_device ini i2o_dev and it becomes completely trivial.

Okay, will look at it...

Thanks again for your help!



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: Merge I2O patches from -mm

[Markus Lidel]

Hello,

Christoph Hellwig wrote:
> The patch below adds a __scsi_add_device that can "preload" sdev->hostdata.

Thank you very much for changing it!

At the moment i only have a patch without the i2o_scsi changes, but that 
will follow ASAP...



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: Merge I2O patches from -mm

[Markus Lidel]

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

Hi...

Christoph Hellwig wrote:
 > What really seems to miss in your model is a callback to i2o_scsi
 > from the main i2o code when a new i2o_controller is found, if you
 > implemented
 > that we'd allocate/deallocate the Scsi_Host in that callback and
 > ->probe/->remove could be sure it'd always have it.
 > i2o_scsi_get_host would get inlined into i2o_scsi_probe.
 > i2o_scsi_remove basically needs a full rewrite, you need to find a way
 > to store a scsi_device ini i2o_dev and it becomes completely trivial.
 > Not sure about how to sanitize the scsi_devie probing logic
 > in i2o_scsi_probe yet.

Okay, patch i2o_scsi-cleanup.patch adds a notification facility to the 
i2o_driver, which notify if a controller is added or removed. The 
i2o_controller structure has now the ability to store per-driver data 
and the SCSI-OSM now takes advantage of this. So all ugly parts should 
be removed now :-)

If you have further things which should be changed, please let me know...



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

[-- Attachment #2: i2o_scsi-cleanup.patch --]
[-- Type: text/x-patch, Size: 15086 bytes --]

Index: include/linux/i2o.h
===================================================================
--- a/include/linux/i2o.h	(revision 111)
+++ b/include/linux/i2o.h	(revision 112)
@@ -34,6 +34,10 @@
 /* message queue empty */
 #define I2O_QUEUE_EMPTY		0xffffffff
 
+enum i2o_driver_notify {
+	I2O_DRIVER_NOTIFY_CONTROLLER_ADD = 0,
+	I2O_DRIVER_NOTIFY_CONTROLLER_REMOVE = 1,
+};
 
 /*
  *	Message structures
@@ -113,6 +117,9 @@
 
 	struct device_driver driver;
 
+	/* notification of changes */
+	void (*notify)(enum i2o_driver_notify, void *);
+
 	struct semaphore lock;
 };
 
@@ -201,6 +208,8 @@
 #endif
 	spinlock_t lock;			/* lock for controller
 						   configuration */
+
+	void *driver_data[I2O_MAX_DRIVERS];	/* storage for drivers */
 };
 
 /*
@@ -275,6 +284,7 @@
 extern u32 i2o_cntxt_list_add(struct i2o_controller *, void *);
 extern void *i2o_cntxt_list_get(struct i2o_controller *, u32);
 extern u32 i2o_cntxt_list_remove(struct i2o_controller *, void *);
+extern u32 i2o_cntxt_list_get_ptr(struct i2o_controller *, void *);
 
 static inline u32 i2o_ptr_low(void *ptr)
 {
@@ -301,6 +311,11 @@
 	return (u32)ptr;
 };
 
+static inline u32 i2o_cntxt_list_get_ptr(struct i2o_controller *c, void *ptr)
+{
+	return (u32)ptr;
+};
+
 static inline u32 i2o_ptr_low(void *ptr)
 {
 	return (u32)ptr;
@@ -316,6 +331,19 @@
 extern int i2o_driver_register(struct i2o_driver *);
 extern void i2o_driver_unregister(struct i2o_driver *);
 
+/**
+ *	i2o_driver_notify - Send notification to a single I2O drivers
+ *
+ *	Send notifications to a single registered driver.
+ */
+static inline void i2o_driver_notify(struct i2o_driver *drv,
+			enum i2o_driver_notify notify, void *data) {
+	if(drv->notify)
+			drv->notify(notify, data);
+}
+
+extern void i2o_driver_notify_all(enum i2o_driver_notify, void *);
+
 /* I2O device functions */
 extern int i2o_device_claim(struct i2o_device *);
 extern int i2o_device_claim_release(struct i2o_device *);
@@ -890,6 +918,7 @@
 #define I2O_TIMEOUT_RESET		30
 #define I2O_TIMEOUT_STATUS_GET		5
 #define I2O_TIMEOUT_LCT_GET		20
+#define I2O_TIMEOUT_SCSI_SCB_ABORT	240
 
 /* retries */
 #define I2O_HRT_GET_TRIES		3
Index: drivers/message/i2o/iop.c
===================================================================
--- a/drivers/message/i2o/iop.c	(revision 111)
+++ b/drivers/message/i2o/iop.c	(revision 112)
@@ -108,8 +108,8 @@
 #if BITS_PER_LONG == 64
 /**
  *      i2o_cntxt_list_add - Append a pointer to context list and return a id
+ *	@c: controller to which the context list belong
  *	@ptr: pointer to add to the context list
- *	@c: controller to which the context list belong
  *
  *	Because the context field in I2O is only 32-bit large, on 64-bit the
  *	pointer is to large to fit in the context field. The i2o_cntxt_list
@@ -117,7 +117,7 @@
  *
  *	Returns context id > 0 on success or 0 on failure.
  */
-u32 i2o_cntxt_list_add(struct i2o_controller * c, void *ptr)
+u32 i2o_cntxt_list_add(struct i2o_controller *c, void *ptr)
 {
 	struct i2o_context_list_element *entry;
 	unsigned long flags;
@@ -154,15 +154,15 @@
 
 /**
  *      i2o_cntxt_list_remove - Remove a pointer from the context list
+ *	@c: controller to which the context list belong
  *	@ptr: pointer which should be removed from the context list
- *	@c: controller to which the context list belong
  *
  *	Removes a previously added pointer from the context list and returns
  *	the matching context id.
  *
  *	Returns context id on succes or 0 on failure.
  */
-u32 i2o_cntxt_list_remove(struct i2o_controller * c, void *ptr)
+u32 i2o_cntxt_list_remove(struct i2o_controller *c, void *ptr)
 {
 	struct i2o_context_list_element *entry;
 	u32 context = 0;
@@ -189,9 +189,11 @@
 
 /**
  *      i2o_cntxt_list_get - Get a pointer from the context list and remove it
+ *	@c: controller to which the context list belong
  *	@context: context id to which the pointer belong
- *	@c: controller to which the context list belong
- *	returns pointer to the matching context id
+ *
+ *	Returns pointer to the matching context id on success or NULL on
+ *	failure.
  */
 void *i2o_cntxt_list_get(struct i2o_controller *c, u32 context)
 {
@@ -216,6 +218,37 @@
 
 	return ptr;
 };
+
+/**
+ *      i2o_cntxt_list_get_ptr - Get a context id from the context list
+ *	@c: controller to which the context list belong
+ *	@ptr: pointer to which the context id should be fetched
+ *
+ *	Returns context id which matches to the pointer on succes or 0 on
+ *	failure.
+ */
+u32 i2o_cntxt_list_get_ptr(struct i2o_controller * c, void *ptr)
+{
+	struct i2o_context_list_element *entry;
+	u32 context = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&c->context_list_lock, flags);
+	list_for_each_entry(entry, &c->context_list, list)
+	    if (entry->ptr == ptr) {
+		context = entry->context;
+		break;
+	}
+	spin_unlock_irqrestore(&c->context_list_lock, flags);
+
+	if (!context)
+		printk(KERN_WARNING "i2o: Could not find nonexistent ptr "
+		       "%p\n", ptr);
+
+	pr_debug("get context id from context list %p -> %d\n", ptr, context);
+
+	return context;
+};
 #endif
 
 /**
@@ -782,6 +815,8 @@
 
 	pr_debug("Deleting controller %s\n", c->name);
 
+	i2o_driver_notify_all(I2O_DRIVER_NOTIFY_CONTROLLER_REMOVE, c);
+
 	list_del(&c->list);
 
 	list_for_each_entry_safe(dev, tmp, &c->devices, list)
@@ -1098,6 +1133,8 @@
 
 	list_add(&c->list, &i2o_controllers);
 
+	i2o_driver_notify_all(I2O_DRIVER_NOTIFY_CONTROLLER_ADD, c);
+
 	printk(KERN_INFO "%s: Controller added\n", c->name);
 
 	return 0;
@@ -1209,6 +1246,7 @@
 EXPORT_SYMBOL(i2o_cntxt_list_add);
 EXPORT_SYMBOL(i2o_cntxt_list_get);
 EXPORT_SYMBOL(i2o_cntxt_list_remove);
+EXPORT_SYMBOL(i2o_cntxt_list_get_ptr);
 #endif
 EXPORT_SYMBOL(i2o_msg_get_wait);
 EXPORT_SYMBOL(i2o_msg_nop);
Index: drivers/message/i2o/i2o_scsi.c
===================================================================
--- a/drivers/message/i2o/i2o_scsi.c	(revision 111)
+++ b/drivers/message/i2o/i2o_scsi.c	(revision 112)
@@ -67,13 +67,12 @@
 
 #define VERSION_STRING        "Version 0.1.2"
 
+static struct i2o_driver i2o_scsi_driver;
+
 static int i2o_scsi_max_id = 16;
 static int i2o_scsi_max_lun = 8;
 
-static LIST_HEAD(i2o_scsi_hosts);
-
 struct i2o_scsi_host {
-	struct list_head list;	/* node in in i2o_scsi_hosts */
 	struct Scsi_Host *scsi_host;	/* pointer to the SCSI host */
 	struct i2o_controller *iop;	/* pointer to the I2O controller */
 	struct i2o_device *channel[0];	/* channel->i2o_dev mapping table */
@@ -81,19 +80,6 @@
 
 static struct scsi_host_template i2o_scsi_host_template;
 
-/*
- * This is only needed, because we can only set the hostdata after the device is
- * added to the scsi core. So we need this little workaround.
- */
-static DECLARE_MUTEX(i2o_scsi_probe_lock);
-static struct i2o_device *i2o_scsi_probe_dev = NULL;
-
-static int i2o_scsi_slave_alloc(struct scsi_device *sdp)
-{
-	sdp->hostdata = i2o_scsi_probe_dev;
-	return 0;
-};
-
 #define I2O_SCSI_CAN_QUEUE	4
 
 /* SCSI OSM class handling definition */
@@ -169,37 +155,11 @@
  *	is returned, otherwise the I2O controller is added to the SCSI
  *	core.
  *
- *	Returns pointer to the I2O SCSI host on success or negative error code
- *	on failure.
+ *	Returns pointer to the I2O SCSI host on success or NULL on failure.
  */
 static struct i2o_scsi_host *i2o_scsi_get_host(struct i2o_controller *c)
 {
-	struct i2o_scsi_host *i2o_shost;
-	int rc;
-
-	/* skip if already registered as I2O SCSI host */
-	list_for_each_entry(i2o_shost, &i2o_scsi_hosts, list)
-	    if (i2o_shost->iop == c)
-		return i2o_shost;
-
-	i2o_shost = i2o_scsi_host_alloc(c);
-	if (IS_ERR(i2o_shost)) {
-		printk(KERN_ERR "scsi-osm: Could not initialize SCSI host\n");
-		return i2o_shost;
-	}
-
-	rc = scsi_add_host(i2o_shost->scsi_host, &c->device);
-	if (rc) {
-		printk(KERN_ERR "scsi-osm: Could not add SCSI host\n");
-		scsi_host_put(i2o_shost->scsi_host);
-		return ERR_PTR(rc);
-	}
-
-	list_add(&i2o_shost->list, &i2o_scsi_hosts);
-	pr_debug("new I2O SCSI host added\n");
-
-	return i2o_shost;
-
+	return c->driver_data[i2o_scsi_driver.context];
 };
 
 /**
@@ -252,8 +212,8 @@
 	int i;
 
 	i2o_shost = i2o_scsi_get_host(c);
-	if (IS_ERR(i2o_shost))
-		return PTR_ERR(i2o_shost);
+	if (!i2o_shost)
+		return -EFAULT;
 
 	scsi_host = i2o_shost->scsi_host;
 
@@ -292,11 +252,8 @@
 		return -EFAULT;
 	}
 
-	down_interruptible(&i2o_scsi_probe_lock);
-	i2o_scsi_probe_dev = i2o_dev;
-	scsi_dev = scsi_add_device(i2o_shost->scsi_host, channel, id, lun);
-	i2o_scsi_probe_dev = NULL;
-	up(&i2o_scsi_probe_lock);
+	scsi_dev =
+	    __scsi_add_device(i2o_shost->scsi_host, channel, id, lun, i2o_dev);
 
 	if (!scsi_dev) {
 		printk(KERN_WARNING "scsi-osm: can not add SCSI device "
@@ -536,13 +493,67 @@
 				 cmd->request_bufflen, cmd->sc_data_direction);
 
 	return 1;
-}
+};
 
+/**
+ *	i2o_scsi_notify - Retrieve notifications of controller added or removed
+ *	@notify: the notification event which occurs
+ *	@data: pointer to additional data
+ *
+ *	If a I2O controller is added, we catch the notification to add a
+ *	corresponding Scsi_Host. On removal also remove the Scsi_Host.
+ */
+void i2o_scsi_notify(enum i2o_driver_notify notify, void *data)
+{
+	struct i2o_controller *c = data;
+	struct i2o_scsi_host *i2o_shost;
+	int rc;
+
+	switch (notify) {
+	case I2O_DRIVER_NOTIFY_CONTROLLER_ADD:
+		i2o_shost = i2o_scsi_host_alloc(c);
+		if (IS_ERR(i2o_shost)) {
+			printk(KERN_ERR "scsi-osm: Could not initialize"
+			       " SCSI host\n");
+			return;
+		}
+
+		rc = scsi_add_host(i2o_shost->scsi_host, &c->device);
+		if (rc) {
+			printk(KERN_ERR "scsi-osm: Could not add SCSI "
+			       "host\n");
+			scsi_host_put(i2o_shost->scsi_host);
+			return;
+		}
+
+		c->driver_data[i2o_scsi_driver.context] = i2o_shost;
+
+		pr_debug("new I2O SCSI host added\n");
+		break;
+
+	case I2O_DRIVER_NOTIFY_CONTROLLER_REMOVE:
+		i2o_shost = i2o_scsi_get_host(c);
+		if (!i2o_shost)
+			return;
+
+		c->driver_data[i2o_scsi_driver.context] = NULL;
+
+		scsi_remove_host(i2o_shost->scsi_host);
+		scsi_host_put(i2o_shost->scsi_host);
+		pr_debug("I2O SCSI host removed\n");
+		break;
+
+	default:
+		break;
+	}
+};
+
 /* SCSI OSM driver struct */
 static struct i2o_driver i2o_scsi_driver = {
 	.name = "scsi-osm",
 	.reply = i2o_scsi_reply,
 	.classes = i2o_scsi_class_id,
+	.notify = i2o_scsi_notify,
 	.driver = {
 		   .probe = i2o_scsi_probe,
 		   .remove = i2o_scsi_remove,
@@ -736,54 +747,47 @@
 	return 0;
 };
 
-#if 0
-FIXME
 /**
- *	i2o_scsi_abort	-	abort a running command
+ *	i2o_scsi_abort - abort a running command
  *	@SCpnt: command to abort
  *
  *	Ask the I2O controller to abort a command. This is an asynchrnous
- *	process and our callback handler will see the command complete
- *	with an aborted message if it succeeds.
+ *	process and our callback handler will see the command complete with an
+ *	aborted message if it succeeds.
  *
- *	Locks: no locks are held or needed
+ *	Returns 0 if the command is successfully aborted or negative error code
+ *	on failure.
  */
 int i2o_scsi_abort(struct scsi_cmnd *SCpnt)
 {
+	struct i2o_device *i2o_dev;
 	struct i2o_controller *c;
-	struct Scsi_Host *host;
-	struct i2o_scsi_host *hostdata;
-	u32 msg[5];
+	struct i2o_message *msg;
+	u32 m;
 	int tid;
 	int status = FAILED;
 
 	printk(KERN_WARNING "i2o_scsi: Aborting command block.\n");
 
-	host = SCpnt->device->host;
-	hostdata = (struct i2o_scsi_host *)host->hostdata;
-	tid = hostdata->task[SCpnt->device->id][SCpnt->device->lun];
-	if (tid == -1) {
-		printk(KERN_ERR "i2o_scsi: Impossible command to abort!\n");
-		return status;
-	}
-	c = hostdata->controller;
+	i2o_dev = SCpnt->device->hostdata;
+	c = i2o_dev->iop;
+	tid = i2o_dev->lct_data.tid;
 
-	spin_unlock_irq(host->host_lock);
+	m = i2o_msg_get_wait(c, &msg, I2O_TIMEOUT_MESSAGE_GET);
+	if (m == I2O_QUEUE_EMPTY)
+		return SCSI_MLQUEUE_HOST_BUSY;
 
-	msg[0] = FIVE_WORD_MSG_SIZE;
-	msg[1] = I2O_CMD_SCSI_ABORT << 24 | HOST_TID << 12 | tid;
-	msg[2] = scsi_context;
-	msg[3] = 0;
-	msg[4] = i2o_context_list_remove(SCpnt, c);
-	if (i2o_post_wait(c, msg, sizeof(msg), 240))
+	writel(FIVE_WORD_MSG_SIZE | SGL_OFFSET_0, &msg->u.head[0]);
+	writel(I2O_CMD_SCSI_ABORT << 24 | HOST_TID << 12 | tid,
+	       &msg->u.head[1]);
+	writel(i2o_cntxt_list_get_ptr(c, SCpnt), &msg->body[0]);
+
+	if (i2o_msg_post_wait(c, m, I2O_TIMEOUT_SCSI_SCB_ABORT))
 		status = SUCCESS;
 
-	spin_lock_irq(host->host_lock);
 	return status;
 }
 
-#endif
-
 /**
  *	i2o_scsi_bios_param	-	Invent disk geometry
  *	@sdev: scsi device
@@ -817,15 +821,12 @@
 	.name = "I2O SCSI Peripheral OSM",
 	.info = i2o_scsi_info,
 	.queuecommand = i2o_scsi_queuecommand,
-/*
-	.eh_abort_handler	= i2o_scsi_abort,
-*/
+	.eh_abort_handler = i2o_scsi_abort,
 	.bios_param = i2o_scsi_bios_param,
 	.can_queue = I2O_SCSI_CAN_QUEUE,
 	.sg_tablesize = 8,
 	.cmd_per_lun = 6,
 	.use_clustering = ENABLE_CLUSTERING,
-	.slave_alloc = i2o_scsi_slave_alloc,
 };
 
 /*
@@ -867,14 +868,6 @@
  */
 static void __exit i2o_scsi_exit(void)
 {
-	struct i2o_scsi_host *i2o_shost, *tmp;
-
-	/* Remove I2O SCSI hosts */
-	list_for_each_entry_safe(i2o_shost, tmp, &i2o_scsi_hosts, list) {
-		scsi_remove_host(i2o_shost->scsi_host);
-		scsi_host_put(i2o_shost->scsi_host);
-	}
-
 	/* Unregister I2O SCSI OSM from I2O core */
 	i2o_driver_unregister(&i2o_scsi_driver);
 };
Index: drivers/message/i2o/driver.c
===================================================================
--- a/drivers/message/i2o/driver.c	(revision 111)
+++ b/drivers/message/i2o/driver.c	(revision 112)
@@ -72,6 +72,7 @@
  */
 int i2o_driver_register(struct i2o_driver *drv)
 {
+	struct i2o_controller *c;
 	int i;
 	int rc = 0;
 	unsigned long flags;
@@ -108,6 +109,9 @@
 	spin_unlock_irqrestore(&i2o_drivers_lock, flags);
 
 	pr_debug("driver %s gets context id %d\n", drv->name, drv->context);
+	
+	list_for_each_entry(c, &i2o_controllers, list)
+		i2o_driver_notify(drv, I2O_DRIVER_NOTIFY_CONTROLLER_ADD, c);
 
 	rc = driver_register(&drv->driver);
 	if (rc)
@@ -125,12 +129,16 @@
  */
 void i2o_driver_unregister(struct i2o_driver *drv)
 {
+	struct i2o_controller *c;
 	unsigned long flags;
 
 	pr_debug("unregister driver %s\n", drv->name);
 
 	driver_unregister(&drv->driver);
 
+	list_for_each_entry(c, &i2o_controllers, list)
+		i2o_driver_notify(drv, I2O_DRIVER_NOTIFY_CONTROLLER_REMOVE, c);
+
 	spin_lock_irqsave(&i2o_drivers_lock, flags);
 	i2o_drivers[drv->context] = NULL;
 	spin_unlock_irqrestore(&i2o_drivers_lock, flags);
@@ -220,6 +228,23 @@
 }
 
 /**
+ *	i2o_driver_notify_all - Send notification to all I2O drivers
+ *
+ *	Send notifications to all registered drivers.
+ */
+void i2o_driver_notify_all(enum i2o_driver_notify evt, void *data) {
+	int i;
+	struct i2o_driver *drv;
+
+	for(i = 0; i < I2O_MAX_DRIVERS; i ++) {
+		drv = i2o_drivers[i];
+
+		if(drv)
+			i2o_driver_notify(drv, evt, data);
+	}
+}
+
+/**
  *	i2o_driver_init - initialize I2O drivers (OSMs)
  *
  *	Registers the I2O bus and allocate memory for the array of OSMs.
@@ -267,3 +292,4 @@
 
 EXPORT_SYMBOL(i2o_driver_register);
 EXPORT_SYMBOL(i2o_driver_unregister);
+EXPORT_SYMBOL(i2o_driver_notify_all);

Re: Merge I2O patches from -mm

[Christoph Hellwig]

On Thu, Aug 19, 2004 at 01:08:33AM +0200, Markus Lidel wrote:
> Okay, patch i2o_scsi-cleanup.patch adds a notification facility to the 
> i2o_driver, which notify if a controller is added or removed. The 
> i2o_controller structure has now the ability to store per-driver data 
> and the SCSI-OSM now takes advantage of this. So all ugly parts should 
> be removed now :-)
> 
> If you have further things which should be changed, please let me know...

Looks much better now, thanks.  But instead of the notify call please
add a controller_add and add controller_remove method, taking a typesafe
i2o_controller * instead of the multiplexer.

> 
> 
> 
> Best regards,
> 
> 
> Markus Lidel
> ------------------------------------------
> Markus Lidel (Senior IT Consultant)
> 
> Shadow Connect GmbH
> Carl-Reisch-Weg 12
> D-86381 Krumbach
> Germany
> 
> Phone:  +49 82 82/99 51-0
> Fax:    +49 82 82/99 51-11
> 
> E-Mail: Markus.Lidel@shadowconnect.com
> URL:    http://www.shadowconnect.com


---end quoted text---

Re: Merge I2O patches from -mm

[Markus Lidel]

Hi...

Christoph Hellwig wrote:
> On Thu, Aug 19, 2004 at 01:08:33AM +0200, Markus Lidel wrote:
>>Okay, patch i2o_scsi-cleanup.patch adds a notification facility to the 
>>i2o_driver, which notify if a controller is added or removed. The 
>>i2o_controller structure has now the ability to store per-driver data 
>>and the SCSI-OSM now takes advantage of this. So all ugly parts should 
>>be removed now :-)
>>If you have further things which should be changed, please let me know...
> Looks much better now, thanks.  But instead of the notify call please

Thanks!

> add a controller_add and add controller_remove method, taking a typesafe
> i2o_controller * instead of the multiplexer.

I had this before, but i want the notification also for I2O devices, 
because the driver model won't call probe functions for devices, which 
are already occupied by a other driver. This is not the best solution, 
if you have more then one drivers which could handle a device. This is 
the case in e.g. i2o_proc, which only want to display information, and 
is not a "real driver". So finally there will be controller_add, 
controller_remove, device_add, device_remove... and i thought it would 
be more generic, and i also don't have to add a function each time a new 
notification is needed :-)

Also i tried to implement the notification like the one already in the 
kernel, so i could exchange my notification facility with the already 
existing one (include/linux/notifier.h)...



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: Merge I2O patches from -mm

[Christoph Hellwig]

> > add a controller_add and add controller_remove method, taking a typesafe
> > i2o_controller * instead of the multiplexer.
> 
> I had this before, but i want the notification also for I2O devices, 
> because the driver model won't call probe functions for devices, which 
> are already occupied by a other driver.

Then please add more methods.  Multiplexer calls are an extremly bad idea.

> This is not the best solution, 
> if you have more then one drivers which could handle a device. This is 
> the case in e.g. i2o_proc, which only want to display information, and 
> is not a "real driver". So finally there will be controller_add, 
> controller_remove, device_add, device_remove... and i thought it would 
> be more generic, and i also don't have to add a function each time a new 
> notification is needed :-)

Yes, that's the whole point of this methods..

> Also i tried to implement the notification like the one already in the 
> kernel, so i could exchange my notification facility with the already 
> existing one (include/linux/notifier.h)...

linux/notifier.h is an bad example to follow and for many thing we're
moving slowly away from it (e.g. the shutdown notifications are now
exposed through the driver model)

Re: Merge I2O patches from -mm

[Christoph Hellwig]

On Thu, Aug 19, 2004 at 12:16:46PM +0200, Markus Lidel wrote:
> > Then please add more methods.  Multiplexer calls are an extremly bad idea.
> 
> Okay, i prefer type safety too, but i also don't like too many functions...

Please just add a function per thing you want to do.  That's how Linux APIs
work.

Re: Merge I2O patches from -mm

[Markus Lidel]

Hello,

Christoph Hellwig wrote:
>>>add a controller_add and add controller_remove method, taking a typesafe
>>>i2o_controller * instead of the multiplexer.
>>I had this before, but i want the notification also for I2O devices, 
>>because the driver model won't call probe functions for devices, which 
>>are already occupied by a other driver.
> Then please add more methods.  Multiplexer calls are an extremly bad idea.

Okay, i prefer type safety too, but i also don't like too many functions...

What do you think about that:

enum i2o_notify {
	ADD = 0;
	REMOVE = 1;
}

i2o_notify_controller(enum i2o_notify, struct i2o_controller *);
i2o_notify_device(enum i2o_notify, struct i2o_device *);



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: Merge I2O patches from -mm

[Markus Lidel]

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

Hello,

Christoph Hellwig wrote:

> On Thu, Aug 19, 2004 at 12:16:46PM +0200, Markus Lidel wrote:
>>>Then please add more methods.  Multiplexer calls are an extremly bad idea.
>>Okay, i prefer type safety too, but i also don't like too many functions...
> Please just add a function per thing you want to do.  That's how Linux APIs
> work.

I still liked the multiplexer approach more, because there where much 
less copy-and-paste code :-)

But is it okay for you now?



Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

[-- Attachment #2: i2o-remove_notify_multiplexer.patch --]
[-- Type: text/x-patch, Size: 11246 bytes --]

diff -ur a/drivers/message/i2o/device.c b/drivers/message/i2o/device.c
--- a/drivers/message/i2o/device.c	2004-08-20 12:17:00.000000000 -1000
+++ b/drivers/message/i2o/device.c	2004-08-20 14:33:36.972796000 -1000
@@ -239,6 +239,8 @@
 
 	class_device_register(&dev->classdev);
 
+	i2o_driver_notify_device_add_all(dev);
+
 	pr_debug("I2O device %s added\n", dev->device.bus_id);
 
 	return dev;
@@ -254,6 +256,7 @@
  */
 void i2o_device_remove(struct i2o_device *i2o_dev)
 {
+	i2o_driver_notify_device_remove_all(i2o_dev);
 	class_device_unregister(&i2o_dev->classdev);
 	list_del(&i2o_dev->list);
 	device_unregister(&i2o_dev->device);
diff -ur a/drivers/message/i2o/driver.c b/drivers/message/i2o/driver.c
--- a/drivers/message/i2o/driver.c	2004-08-20 12:32:13.000000000 -1000
+++ b/drivers/message/i2o/driver.c	2004-08-20 14:37:56.871285000 -1000
@@ -110,8 +110,14 @@
 
 	pr_debug("driver %s gets context id %d\n", drv->name, drv->context);
 	
-	list_for_each_entry(c, &i2o_controllers, list)
-		i2o_driver_notify(drv, I2O_DRIVER_NOTIFY_CONTROLLER_ADD, c);
+	list_for_each_entry(c, &i2o_controllers, list) {
+		struct i2o_device *i2o_dev;
+
+		i2o_driver_notify_controller_add(drv, c);
+		list_for_each_entry(i2o_dev, &c->devices, list)
+			i2o_driver_notify_device_add(drv, i2o_dev);
+	}
+
 
 	rc = driver_register(&drv->driver);
 	if (rc)
@@ -136,8 +142,14 @@
 
 	driver_unregister(&drv->driver);
 
-	list_for_each_entry(c, &i2o_controllers, list)
-		i2o_driver_notify(drv, I2O_DRIVER_NOTIFY_CONTROLLER_REMOVE, c);
+	list_for_each_entry(c, &i2o_controllers, list) {
+		struct i2o_device *i2o_dev;
+
+		list_for_each_entry(i2o_dev, &c->devices, list)
+			i2o_driver_notify_device_remove(drv, i2o_dev);
+
+		i2o_driver_notify_controller_remove(drv, c);
+	}
 
 	spin_lock_irqsave(&i2o_drivers_lock, flags);
 	i2o_drivers[drv->context] = NULL;
@@ -228,11 +240,68 @@
 }
 
 /**
- *	i2o_driver_notify_all - Send notification to all I2O drivers
+ *	i2o_driver_notify_controller_add_all - Send notify of added controller
+ *					       to all I2O drivers
+ *
+ *	Send notifications to all registered drivers that a new controller was
+ *	added.
+ */
+void i2o_driver_notify_controller_add_all(struct i2o_controller *c) {
+	int i;
+	struct i2o_driver *drv;
+
+	for(i = 0; i < I2O_MAX_DRIVERS; i ++) {
+		drv = i2o_drivers[i];
+
+		if(drv)
+			i2o_driver_notify_controller_add(drv, c);
+	}
+}
+
+/**
+ *	i2o_driver_notify_controller_remove_all - Send notify of removed
+ *						  controller to all I2O drivers
+ *
+ *	Send notifications to all registered drivers that a controller was
+ *	removed.
+ */
+void i2o_driver_notify_controller_remove_all(struct i2o_controller *c) {
+	int i;
+	struct i2o_driver *drv;
+
+	for(i = 0; i < I2O_MAX_DRIVERS; i ++) {
+		drv = i2o_drivers[i];
+
+		if(drv)
+			i2o_driver_notify_controller_remove(drv, c);
+	}
+}
+
+/**
+ *	i2o_driver_notify_device_add_all - Send notify of added device to all
+ *					   I2O drivers
+ *
+ *	Send notifications to all registered drivers that a device was added.
+ */
+void i2o_driver_notify_device_add_all(struct i2o_device *i2o_dev) {
+	int i;
+	struct i2o_driver *drv;
+
+	for(i = 0; i < I2O_MAX_DRIVERS; i ++) {
+		drv = i2o_drivers[i];
+
+		if(drv)
+			i2o_driver_notify_device_add(drv, i2o_dev);
+	}
+}
+
+/**
+ *	i2o_driver_notify_device_remove_all - Send notify of removed device to
+ *					      all I2O drivers
  *
- *	Send notifications to all registered drivers.
+ *	Send notifications to all registered drivers that a device was removed.
  */
-void i2o_driver_notify_all(enum i2o_driver_notify evt, void *data) {
+void i2o_driver_notify_device_remove_all(struct i2o_device *i2o_dev) {
 	int i;
 	struct i2o_driver *drv;
 
@@ -240,7 +309,7 @@
 		drv = i2o_drivers[i];
 
 		if(drv)
-			i2o_driver_notify(drv, evt, data);
+			i2o_driver_notify_device_remove(drv, i2o_dev);
 	}
 }
 
@@ -292,4 +361,7 @@
 
 EXPORT_SYMBOL(i2o_driver_register);
 EXPORT_SYMBOL(i2o_driver_unregister);
-EXPORT_SYMBOL(i2o_driver_notify_all);
+EXPORT_SYMBOL(i2o_driver_notify_controller_add_all);
+EXPORT_SYMBOL(i2o_driver_notify_controller_remove_all);
+EXPORT_SYMBOL(i2o_driver_notify_device_add_all);
+EXPORT_SYMBOL(i2o_driver_notify_device_remove_all);
diff -ur a/drivers/message/i2o/i2o_scsi.c b/drivers/message/i2o/i2o_scsi.c
--- a/drivers/message/i2o/i2o_scsi.c	2004-08-20 12:32:13.000000000 -1000
+++ b/drivers/message/i2o/i2o_scsi.c	2004-08-20 14:42:19.020433000 -1000
@@ -496,56 +496,58 @@
 };
 
 /**
- *	i2o_scsi_notify - Retrieve notifications of controller added or removed
- *	@notify: the notification event which occurs
- *	@data: pointer to additional data
+ *	i2o_scsi_notify_controller_add - Retrieve notifications of added
+ *					 controllers
+ *	@c: the controller which was added
  *
  *	If a I2O controller is added, we catch the notification to add a
- *	corresponding Scsi_Host. On removal also remove the Scsi_Host.
+ *	corresponding Scsi_Host.
  */
-void i2o_scsi_notify(enum i2o_driver_notify notify, void *data)
+void i2o_scsi_notify_controller_add(struct i2o_controller *c)
 {
-	struct i2o_controller *c = data;
 	struct i2o_scsi_host *i2o_shost;
 	int rc;
 
-	switch (notify) {
-	case I2O_DRIVER_NOTIFY_CONTROLLER_ADD:
-		i2o_shost = i2o_scsi_host_alloc(c);
-		if (IS_ERR(i2o_shost)) {
-			printk(KERN_ERR "scsi-osm: Could not initialize"
-			       " SCSI host\n");
-			return;
-		}
-
-		rc = scsi_add_host(i2o_shost->scsi_host, &c->device);
-		if (rc) {
-			printk(KERN_ERR "scsi-osm: Could not add SCSI "
-			       "host\n");
-			scsi_host_put(i2o_shost->scsi_host);
-			return;
-		}
-
-		c->driver_data[i2o_scsi_driver.context] = i2o_shost;
-
-		pr_debug("new I2O SCSI host added\n");
-		break;
+	i2o_shost = i2o_scsi_host_alloc(c);
+	if (IS_ERR(i2o_shost)) {
+		printk(KERN_ERR "scsi-osm: Could not initialize"
+		       " SCSI host\n");
+		return;
+	}
 
-	case I2O_DRIVER_NOTIFY_CONTROLLER_REMOVE:
-		i2o_shost = i2o_scsi_get_host(c);
-		if (!i2o_shost)
-			return;
+	rc = scsi_add_host(i2o_shost->scsi_host, &c->device);
+	if (rc) {
+		printk(KERN_ERR "scsi-osm: Could not add SCSI "
+		       "host\n");
+		scsi_host_put(i2o_shost->scsi_host);
+		return;
+	}
 
-		c->driver_data[i2o_scsi_driver.context] = NULL;
+	c->driver_data[i2o_scsi_driver.context] = i2o_shost;
 
-		scsi_remove_host(i2o_shost->scsi_host);
-		scsi_host_put(i2o_shost->scsi_host);
-		pr_debug("I2O SCSI host removed\n");
-		break;
+	pr_debug("new I2O SCSI host added\n");
+};
 
-	default:
-		break;
-	}
+/**
+ *	i2o_scsi_notify_controller_remove - Retrieve notifications of removed
+ *					    controllers
+ *	@c: the controller which was removed
+ *
+ *	If a I2O controller is removed, we catch the notification to remove the
+ *	corresponding Scsi_Host.
+ */
+void i2o_scsi_notify_controller_remove(struct i2o_controller *c)
+{
+	struct i2o_scsi_host *i2o_shost;
+	i2o_shost = i2o_scsi_get_host(c);
+	if (!i2o_shost)
+		return;
+
+	c->driver_data[i2o_scsi_driver.context] = NULL;
+
+	scsi_remove_host(i2o_shost->scsi_host);
+	scsi_host_put(i2o_shost->scsi_host);
+	pr_debug("I2O SCSI host removed\n");
 };
 
 /* SCSI OSM driver struct */
@@ -553,7 +555,8 @@
 	.name = "scsi-osm",
 	.reply = i2o_scsi_reply,
 	.classes = i2o_scsi_class_id,
-	.notify = i2o_scsi_notify,
+	.notify_controller_add = i2o_scsi_notify_controller_add,
+	.notify_controller_remove = i2o_scsi_notify_controller_remove,
 	.driver = {
 		   .probe = i2o_scsi_probe,
 		   .remove = i2o_scsi_remove,
diff -ur a/drivers/message/i2o/iop.c b/drivers/message/i2o/iop.c
--- a/drivers/message/i2o/iop.c	2004-08-20 12:32:13.000000000 -1000
+++ b/drivers/message/i2o/iop.c	2004-08-20 14:28:14.070884000 -1000
@@ -815,7 +815,7 @@
 
 	pr_debug("Deleting controller %s\n", c->name);
 
-	i2o_driver_notify_all(I2O_DRIVER_NOTIFY_CONTROLLER_REMOVE, c);
+	i2o_driver_notify_controller_remove_all(c);
 
 	list_del(&c->list);
 
@@ -1133,7 +1133,7 @@
 
 	list_add(&c->list, &i2o_controllers);
 
-	i2o_driver_notify_all(I2O_DRIVER_NOTIFY_CONTROLLER_ADD, c);
+	i2o_driver_notify_controller_add_all(c);
 
 	printk(KERN_INFO "%s: Controller added\n", c->name);
 
diff -ur a/include/linux/i2o.h b/include/linux/i2o.h
--- a/include/linux/i2o.h	2004-08-20 12:37:45.000000000 -1000
+++ b/include/linux/i2o.h	2004-08-20 14:37:07.000000000 -1000
@@ -33,11 +33,6 @@
 /* message queue empty */
 #define I2O_QUEUE_EMPTY		0xffffffff
 
-enum i2o_driver_notify {
-	I2O_DRIVER_NOTIFY_CONTROLLER_ADD = 0,
-	I2O_DRIVER_NOTIFY_CONTROLLER_REMOVE = 1,
-};
-
 /*
  *	Message structures
  */
@@ -115,7 +110,10 @@
 	struct device_driver driver;
 
 	/* notification of changes */
-	void (*notify) (enum i2o_driver_notify, void *);
+	void (*notify_controller_add) (struct i2o_controller *);
+	void (*notify_controller_remove) (struct i2o_controller *);
+	void (*notify_device_add) (struct i2o_device *);
+	void (*notify_device_remove) (struct i2o_device *);
 
 	struct semaphore lock;
 };
@@ -325,18 +323,61 @@
 extern void i2o_driver_unregister(struct i2o_driver *);
 
 /**
- *	i2o_driver_notify - Send notification to a single I2O drivers
+ *	i2o_driver_notify_controller_add - Send notification of added controller
+ *					   to a single I2O driver
  *
- *	Send notifications to a single registered driver.
+ *	Send notification of added controller to a single registered driver.
  */
-static inline void i2o_driver_notify(struct i2o_driver *drv,
-				     enum i2o_driver_notify notify, void *data)
+static inline void i2o_driver_notify_controller_add(struct i2o_driver *drv,
+						    struct i2o_controller *c)
 {
-	if (drv->notify)
-		drv->notify(notify, data);
-}
+	if (drv->notify_controller_add)
+		drv->notify_controller_add(c);
+};
+
+/**
+ *	i2o_driver_notify_controller_remove - Send notification of removed
+ *					      controller to a single I2O driver
+ *
+ *	Send notification of removed controller to a single registered driver.
+ */
+static inline void i2o_driver_notify_controller_remove(struct i2o_driver *drv,
+						       struct i2o_controller *c)
+{
+	if (drv->notify_controller_remove)
+		drv->notify_controller_remove(c);
+};
+
+/**
+ *	i2o_driver_notify_device_add - Send notification of added device to a
+ *				       single I2O driver
+ *
+ *	Send notification of added device to a single registered driver.
+ */
+static inline void i2o_driver_notify_device_add(struct i2o_driver *drv,
+						struct i2o_device *i2o_dev)
+{
+	if (drv->notify_device_add)
+		drv->notify_device_add(i2o_dev);
+};
+
+/**
+ *	i2o_driver_notify_device_remove - Send notification of removed device
+ *					  to a single I2O driver
+ *
+ *	Send notification of removed device to a single registered driver.
+ */
+static inline void i2o_driver_notify_device_remove(struct i2o_driver *drv,
+						   struct i2o_device *i2o_dev)
+{
+	if (drv->notify_device_remove)
+		drv->notify_device_remove(i2o_dev);
+};
 
-extern void i2o_driver_notify_all(enum i2o_driver_notify, void *);
+extern void i2o_driver_notify_controller_add_all(struct i2o_controller *);
+extern void i2o_driver_notify_controller_remove_all(struct i2o_controller *);
+extern void i2o_driver_notify_device_add_all(struct i2o_device *);
+extern void i2o_driver_notify_device_remove_all(struct i2o_device *);
 
 /* I2O device functions */
 extern int i2o_device_claim(struct i2o_device *);

Re: Merge I2O patches from -mm

[Christoph Hellwig]

On Thu, Aug 19, 2004 at 01:54:51PM +0200, Markus Lidel wrote:
> I still liked the multiplexer approach more, because there where much 
> less copy-and-paste code :-)
> 
> But is it okay for you now?

Yes, looks better.  mid-term we should look into some helper macros
to reduce the clutter and get locking right, but for now that patch
should do it.

Re: Merge I2O patches from -mm

[Markus Lidel]

Hello,

Christoph Hellwig wrote:
> Yes, looks better.  mid-term we should look into some helper macros

That's great!

> to reduce the clutter and get locking right, but for now that patch
> should do it.

Could you maybe send me some hints because i want it to be it in 
mainline kernel ASAP?

Thank you very much!


Best regards,


Markus Lidel
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: Merge I2O patches from -mm

[Christoph Hellwig]

On Tue, Aug 24, 2004 at 10:16:58AM +0200, Markus Lidel wrote:
> > to reduce the clutter and get locking right, but for now that patch
> > should do it.
> 
> Could you maybe send me some hints because i want it to be it in 
> mainline kernel ASAP?

I don't thjink it's require for mainline inclusion, the new driver
already is a definit improvenet.  I'll send you more review comments
when I get a little bit more time.

Re: Merge I2O patches from -mm

[Markus Lidel]

Hi...

Christoph Hellwig wrote:
> On Tue, Aug 24, 2004 at 10:16:58AM +0200, Markus Lidel wrote:
>>>to reduce the clutter and get locking right, but for now that patch
>>>should do it.
>>Could you maybe send me some hints because i want it to be it in 
>>mainline kernel ASAP?
> I don't thjink it's require for mainline inclusion, the new driver
> already is a definit improvenet.  I'll send you more review comments
> when I get a little bit more time.

Okay, thank you very much!

Bye...
------------------------------------------
Markus Lidel (Senior IT Consultant)

Shadow Connect GmbH
Carl-Reisch-Weg 12
D-86381 Krumbach
Germany

Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11

E-Mail: Markus.Lidel@shadowconnect.com
URL:    http://www.shadowconnect.com

Re: Merge I2O patches from -mm

[Warren Togami]

Can the current I2O stuff finally be merged now?  There seems to be no 
further objections for this round.

Warren Togami
wtogami@redhat.com