Skip to contentSkip to conversation

Merge I2O patches from -mm

DISCUSSION Markus Lidel 3 participants 12 messages Aug 18, 2004 – Aug 28, 2004

lore
Share & tools
lore

Markus Lidel

author
#0 lore raw

Hi...

Quoted 10 lines · Christoph Hellwig

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: [email protected]
URL:    http://www.shadowconnect.com
include/linux/i2o.h +29 -0
Index: include/linux/i2o.h
===================================================================
@@ -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
drivers/message/i2o/iop.c +44 -6

107 lines not on this page · raw message

drivers/message/i2o/i2o_scsi.c +84 -91

268 lines not on this page · raw message

drivers/message/i2o/driver.c +26 -0
Index: drivers/message/i2o/driver.c
===================================================================
@@ -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);
#1 lore raw
Quoted 7 lines · Markus Lidel

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.

Quoted 15 lines

Best regards,

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

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

Quoted 5 lines
Phone:  +49 82 82/99 51-0
Fax:    +49 82 82/99 51-11
E-Mail: [email protected]
URL:    http://www.shadowconnect.com

---end quoted text---

author
#2 lore raw

Hi...

Quoted 4 lines · Christoph Hellwig
Quoted 2 lines · Markus Lidel

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!

Quoted 2 lines

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: [email protected]
URL:    http://www.shadowconnect.com
#3 lore raw
Quoted 6 lines
Quoted 2 lines

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.

Quoted 7 lines

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..

Quoted 3 lines

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)

author
#4 lore raw

Hello,

Quoted 4 lines · Christoph Hellwig
Quoted 3 lines
Quoted 2 lines

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: [email protected]
URL:    http://www.shadowconnect.com
#5 lore raw
Quoted 3 lines · Markus Lidel
Quoted 1 line

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.

author
#6 lore raw

Hello,

Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Markus Lidel
Quoted 1 line

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: [email protected]
URL:    http://www.shadowconnect.com
drivers/message/i2o/device.c +3 -0
diff -ur a/drivers/message/i2o/device.c b/drivers/message/i2o/device.c
@@ -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);
drivers/message/i2o/driver.c +81 -9

119 lines not on this page · raw message

drivers/message/i2o/i2o_scsi.c +43 -40

106 lines not on this page · raw message

drivers/message/i2o/iop.c +2 -2
diff -ur a/drivers/message/i2o/iop.c b/drivers/message/i2o/iop.c
@@ -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);
include/linux/i2o.h +55 -14

91 lines not on this page · raw message

#7 lore raw
Quoted 4 lines · Markus Lidel

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.

author
#8 lore raw

Hello,

Quoted 1 line · Christoph Hellwig

Yes, looks better. mid-term we should look into some helper macros

That's great!

Quoted 2 lines

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: [email protected]
URL:    http://www.shadowconnect.com
#9 lore raw
Quoted 5 lines · Markus Lidel
Quoted 2 lines

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.

author
#10 lore raw

Hi...

Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Markus Lidel
Quoted 2 lines

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: [email protected]
URL:    http://www.shadowconnect.com