i2c bus multiplexing (Version 2).

When we are removing an adapter we decide to ignore any active client detaching faults for two reasons:

1) one (or more) active client may be switched off, so it cannot replay to the "adapter removed" event.

2) it shouldn't happen, and even if it happens it may be due a bus fault which can be resolved by resetting the adapter (most of them can be resetted simply by rmmod and then insmod the module again).

Quoted 10 lines · Rodolfo Giometti

When we are removing an adapter we decide to ignore any active client detaching faults for two reasons:

1) one (or more) active client may be switched off, so it cannot replay to the "adapter removed" event.

2) it shouldn't happen, and even if it happens it may be due a bus fault which can be resolved by resetting the adapter (most of them can be resetted simply by rmmod and then insmod the module again).

This bothers me. If the client can't detach, it can't; there may be all kinds of chaos introduced by trying to fake success in such cases. Like resources that suddenly just vanish, leaving breakage in their wake.

If this is a shutdown() path, where it's basically just a polite driver notification, that's one thing.

But I2C gets used as a system control bus, so this sort of "ignore the errors" stuff worries me a lot.

Quoted 20 lines · David Brownell

Quoted 10 lines · Rodolfo Giometti

When we are removing an adapter we decide to ignore any active client detaching faults for two reasons:

1) one (or more) active client may be switched off, so it cannot replay to the "adapter removed" event.

2) it shouldn't happen, and even if it happens it may be due a bus fault which can be resolved by resetting the adapter (most of them can be resetted simply by rmmod and then insmod the module again).

This bothers me. If the client can't detach, it can't; there may be all kinds of chaos introduced by trying to fake success in such cases. Like resources that suddenly just vanish, leaving breakage in their wake.

If this is a shutdown() path, where it's basically just a polite driver notification, that's one thing.

Quoted 2 lines

But I2C gets used as a system control bus, so this sort of "ignore the errors" stuff worries me a lot.

If so I suppose we should take in account the adapter->client_unregister() return value also, which is currently ignored in i2c_unregister_device() function...

However, since I2c is a bus it could be possible that a slave device stops functioning (or the user decides to turn it off), so aborting the adapter removal in these cases can be not right... moreover if the adapter should be logically separated from the i2c clients I suppose is better allowing the user to remove (and eventually replace) it and then resolving i2c client's stale states into the relative driver.

I suppose this is the same behaviour of USB bus: we can remove an USB host adapter independently from USB devices.

Taking the old, deprecated, notoriously broken i2c model as an example doesn't necessarily serve your cause ;)

The big difference, I presume, is that USB is never a system bus. But even for USB, devices can't exist without the host. If you kill the host and resurrect it later, you also kill the devices and resurrect them later.

The debate about drivers failing device removal is an old one, not specific to i2c. My opinion is that .remove() should succeed as much as possible. It should really only fail if the problem is so serious that the system's state would otherwise become a problem (e.g. freeing memory which is still referenced.) This should be a rare case.

Quoted 3 lines · Jean Delvare

Taking the old, deprecated, notoriously broken i2c model as an example doesn't necessarily serve your cause ;)

I see, but till this part is in we should consider it, shouldn't we? Eheheh :D

Quoted 4 lines

The big difference, I presume, is that USB is never a system bus. But even for USB, devices can't exist without the host. If you kill the host and resurrect it later, you also kill the devices and resurrect them later.

But this can be done with i2c devices too! Let's think about leds drivers with uses an i2c GPIOs expander, they can exist without any adapter, even if they are not functional of course. :)

Quoted 5 lines

The debate about drivers failing device removal is an old one, not specific to i2c. My opinion is that .remove() should succeed as much as possible. It should really only fail if the problem is so serious that the system's state would otherwise become a problem (e.g. freeing memory which is still referenced.) This should be a rare case.

I think that i2c clients devices should be really separated by adapter ones, that is we should be free to add and remove any adapter without considering the clients state. Then each clients device drivers should free each allocated resource they use and the adapters shouldn't know about it.

Doing like this will semply the code a lot and it allows i2c core to be more flexible.

No. As I've said before, ideally I wanted legacy binding style to die _before_ we add support for multiplexing. But if we don't want to wait, I still want legacy binding and multiplexing to be mutually exclusive.

Quoted 8 lines

Quoted 4 lines

The big difference, I presume, is that USB is never a system bus. But even for USB, devices can't exist without the host. If you kill the host and resurrect it later, you also kill the devices and resurrect them later.

But this can be done with i2c devices too! Let's think about leds drivers with uses an i2c GPIOs expander, they can exist without any adapter, even if they are not functional of course. :)

That not the question. We all agree that some i2c chip drivers don't need to be able to prevent the removal of the underlying bus driver. The open point is whether some i2c chip drivers do. Honestly, I don't know, as such cases would happen on embedded systems, which are out of my realm. But maybe David has examples in mind.

Quoted 6 lines

Quoted 5 lines

The debate about drivers failing device removal is an old one, not specific to i2c. My opinion is that .remove() should succeed as much as possible. It should really only fail if the problem is so serious that the system's state would otherwise become a problem (e.g. freeing memory which is still referenced.) This should be a rare case.

Quoted 5 lines

I think that i2c clients devices should be really separated by adapter ones, that is we should be free to add and remove any adapter without considering the clients state. Then each clients device drivers should free each allocated resource they use and the adapters shouldn't know about it.

How does your description differ from the current implementation? I don't follow you.

Quoted 2 lines

Doing like this will semply the code a lot and it allows i2c core to be more flexible.

Patches welcome... but I have no clue what you want to do at this point.

Quoted 5 lines · Jean Delvare

That not the question. We all agree that some i2c chip drivers don't need to be able to prevent the removal of the underlying bus driver. The open point is whether some i2c chip drivers do. Honestly, I don't know, as such cases would happen on embedded systems, which are out of my realm. But maybe David has examples in mind.

I see. But in this case we should dig into the code and verify what each i2c drivers wish to do during driver removal and eventually fix it, is that right?

Quoted 3 lines

(...)
How does your description differ from the current implementation? I don't follow you.

Currently an i2c driver may block adpater removal procedure, my implementation, instead, considers i2c adapter completely separated from i2c clients so that no one can't interfere with adapter insertion/removal code.

Quoted 1 line

Patches welcome... but I have no clue what you want to do at this point.

If you agree I can provide a branch tree from current vanilla and describe my solution into linux-i2c wiki, so users may test it and propose enhancements without touching current stable code.

Quoted 4 lines · Rodolfo Giometti

If you agree I can provide a branch tree from current vanilla and describe my solution into linux-i2c wiki, so users may test it and propose enhancements without touching current stable code.

Well, you can always do that if you want, but please first clearly define the practical problem you are trying to solve. We are not going to change the i2c core without a good reason.

Quoted 3 lines · Jean Delvare

Well, you can always do that if you want, but please first clearly define the practical problem you are trying to solve. We are not going to change the i2c core without a good reason.

I created a new page here:

http://i2c.wiki.kernel.org/index.php/I2C_bus_multiplexing

Please, take a look and report everything should be fixed. :)

Hi Rodolfo,

Sorry for the late answer, I missed this post in the middle of the thread.

Quoted 8 lines · Jean Delvare

Quoted 3 lines

Well, you can always do that if you want, but please first clearly define the practical problem you are trying to solve. We are not going to change the i2c core without a good reason.

I created a new page here:

http://i2c.wiki.kernel.org/index.php/I2C_bus_multiplexing

Thanks for doing this!

Quoted 1 line

Please, take a look and report everything should be fixed. :)

Random comments:

* I really would like to get rid of the "virtual" term that has been repeatedly abused. Bus segments behind multiplexers are very real.

* I am surprised that you use the term "trunk" for all segments, be they in front of or behind multiplexers. When I think of a multiplexer, I designate the segment between the controller and the multiplexer as the trunk, and the segments behind the multiplexer as branches.

Other than that I am rather happy with your page, which explains the problem and the proposed solution clearly.

Quoted 2 lines · Jean Delvare

* I really would like to get rid of the "virtual" term that has been repeatedly abused. Bus segments behind multiplexers are very real.

Actually I use the term "virtual" regarding to the adapters not to the bus segments. In fact the new devices created are not "real" adapters but "virtual" ones.

Hi Rodolfo,

Sorry for the late answer, I missed this post in the middle of the thread.

Quoted 8 lines · Rodolfo Giometti

Quoted 3 lines · Jean Delvare

Well, you can always do that if you want, but please first clearly define the practical problem you are trying to solve. We are not going to change the i2c core without a good reason.

I created a new page here:

http://i2c.wiki.kernel.org/index.php/I2C_bus_multiplexing

Thanks for doing this!

Quoted 1 line

Please, take a look and report everything should be fixed. :)

Random comments:

* I really would like to get rid of the "virtual" term that has been repeatedly abused. Bus segments behind multiplexers are very real.

* I am surprised that you use the term "trunk" for all segments, be they in front of or behind multiplexers. When I think of a multiplexer, I designate the segment between the controller and the multiplexer as the trunk, and the segments behind the multiplexer as branches.

Quoted 5 lines · Jean Delvare

Random comments:

* I really would like to get rid of the "virtual" term that has been repeatedly abused. Bus segments behind multiplexers are very real.

Quoted 5 lines

* I am surprised that you use the term "trunk" for all segments, be they in front of or behind multiplexers. When I think of a multiplexer, I designate the segment between the controller and the multiplexer as the trunk, and the segments behind the multiplexer as branches.

Both fixed. Please review the page again. :)

By using rwsem we can easily manage recursive calls of i2c_scan_static_board_info() function without breaking the locking.

Signed-off-by Rodolfo Giometti <[email protected]>

---
 drivers/i2c/i2c-boardinfo.c |    6 +++---
 drivers/i2c/i2c-core.c      |    4 ++--
 drivers/i2c/i2c-core.h      |    2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

drivers/i2c/i2c-boardinfo.c +3 -3

index ffb35f0..981cdd8 100644
@@ -25,7 +25,7 @@
 /* These symbols are exported ONLY FOR the i2c core.
  * No other users will be supported.
  */
-DEFINE_MUTEX(__i2c_board_lock);
+DECLARE_RWSEM(__i2c_board_lock);
 EXPORT_SYMBOL_GPL(__i2c_board_lock);
 
 LIST_HEAD(__i2c_board_list);

@@ -63,7 +63,7 @@ i2c_register_board_info(int busnum,
 {
 	int status;
 
-	mutex_lock(&__i2c_board_lock);
+	down_write(&__i2c_board_lock);
 
 	/* dynamic bus numbers will be assigned after the last static one */
 	if (busnum >= __i2c_first_dynamic_bus_num)

@@ -84,7 +84,7 @@ i2c_register_board_info(int busnum,
 		list_add_tail(&devinfo->list, &__i2c_board_list);
 	}
 
-	mutex_unlock(&__i2c_board_lock);
+	up_write(&__i2c_board_lock);
 
 	return status;
 }

drivers/i2c/i2c-core.c +2 -2

index 3454878..74b89a9 100644
@@ -408,7 +408,7 @@ static void i2c_scan_static_board_info(struct i2c_adapter *adapter)
 {
 	struct i2c_devinfo	*devinfo;
 
-	mutex_lock(&__i2c_board_lock);
+	down_read(&__i2c_board_lock);
 	list_for_each_entry(devinfo, &__i2c_board_list, list) {
 		if (devinfo->busnum == adapter->nr
 				&& !i2c_new_device(adapter,

@@ -417,7 +417,7 @@ static void i2c_scan_static_board_info(struct i2c_adapter *adapter)
 				i2c_adapter_id(adapter),
 				devinfo->board_info.addr);
 	}
-	mutex_unlock(&__i2c_board_lock);
+	up_read(&__i2c_board_lock);
 }
 
 static int i2c_do_add_adapter(struct device_driver *d, void *data)

drivers/i2c/i2c-core.h +0 -0

index cd5bff8..e6af064 100644

This patch set adds to the I2C core the ability to handle multiplexed I2C bus topologies by presenting each multiplexed segment as a I2C adapter.

Quoted 4 lines

This patch set adds to the I2C core the ability to handle multiplexed I2C bus topologies by presenting each multiplexed segment as a I2C adapter.

Hi Rodolfo,

Thanks for this patch. I learned about this patch through linux-media mailing list.

1)I have a sensor board header that uses pca9543a i2c switch which is addressed by this patch. In my board, I have another i2c chip that share the same i2c address with the sensor. Currently I cannot have both these drivers co-exists due to this. Please let me know if this can work once I use this patch.

2)Which kernel version you use for this patch?

3) Where can I download your patch so that I can give it a shot. Is there any platform data I need to setup in my arch/ folder to use this patch?

4) When do you think this will be merged to mainline kernel?