Skip to contentSkip to conversation

i2c: Keep client->driver and client->dev.driver in sync

PATCH In review Jean Delvare 2 participants 4 messages Mar 8, 2008 – Mar 9, 2008 3 messages not in the archive

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 i2c: Keep client->driver and client->dev.driver in sync

From: Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]>

Ensures that client->driver is set to NULL if the probe() returns an error (this keeps client->driver and client->dev.driver in sync).

---
Hans, I've updated your patch a bit to make i2c_device_probe() look more like i2c_device_remove(); hope you don't mind.

David, this fix looks correct to me, I'll send it to Linus quickly if you have no objection.

 drivers/i2c/i2c-core.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/i2c/i2c-core.c +5 -2
@@ -90,12 +90,15 @@ static int i2c_device_probe(struct devic
 {
 	struct i2c_client	*client = to_i2c_client(dev);
 	struct i2c_driver	*driver = to_i2c_driver(dev->driver);
+	int status;
 
 	if (!driver->probe)
 		return -ENODEV;
-	client->driver = driver;
 	dev_dbg(dev, "probe\n");
-	return driver->probe(client);
+	status = driver->probe(client);
+	if (status == 0)
+		client->driver = driver;
+	return status;
 }
 
 static int i2c_device_remove(struct device *dev)
Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw
Quoted 5 lines · Jean Delvare

From: Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]>

Ensures that client->driver is set to NULL if the probe() returns an error (this keeps client->driver and client->dev.driver in sync).

Quoted 8 lines

---
Hans, I've updated your patch a bit to make i2c_device_probe() look more like i2c_device_remove(); hope you don't mind.

David, this fix looks correct to me, I'll send it to Linus quickly if you have no objection.

The only issue I have is that code which the driver probe() calls may expect client->driver to be set, but it won't be...

While I'd rather see client->driver vanish, in this case I'd just suggest leaving the early assignment, and nulling it out on error (instead of assigning it on success).

Quoted 28 lines
 drivers/i2c/i2c-core.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/i2c/i2c-core.c +5 -2
@@ -90,12 +90,15 @@ static int i2c_device_probe(struct devic
 {
 	struct i2c_client	*client = to_i2c_client(dev);
 	struct i2c_driver	*driver = to_i2c_driver(dev->driver);
+	int status;
 
 	if (!driver->probe)
 		return -ENODEV;
-	client->driver = driver;
 	dev_dbg(dev, "probe\n");
-	return driver->probe(client);
+	status = driver->probe(client);
+	if (status == 0)
+		client->driver = driver;
+	return status;
 }
 
 static int i2c_device_remove(struct device *dev)
Signature · 2 lines
-- 
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): 200803081312.36524.david-b-yBeKhBN/[email protected]

#4 lore raw

Hi David,

Quoted 6 lines · David Brownell

The only issue I have is that code which the driver probe() calls may expect client->driver to be set, but it won't be...

While I'd rather see client->driver vanish, in this case I'd just suggest leaving the early assignment, and nulling it out on error (instead of assigning it on success).

Oh well, that's exactly what Hans' patch did originally, I changed it to make the code more simple, but if you think it is better that way, I'll revert this change of mine. Here you go:

From: Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]>

Ensure that client->driver is set to NULL if the probe() returns an error (this keeps client->driver and client->dev.driver in sync).

---
 drivers/i2c/i2c-core.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/i2c/i2c-core.c +5 -1
@@ -90,12 +90,16 @@ static int i2c_device_probe(struct devic
 {
 	struct i2c_client	*client = to_i2c_client(dev);
 	struct i2c_driver	*driver = to_i2c_driver(dev->driver);
+	int status;
 
 	if (!driver->probe)
 		return -ENODEV;
 	client->driver = driver;
 	dev_dbg(dev, "probe\n");
-	return driver->probe(client);
+	status = driver->probe(client);
+	if (status)
+		client->driver = NULL;
+	return status;
 }
 
 static int i2c_device_remove(struct device *dev)
Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

#6 lore raw
Quoted 6 lines · Jean Delvare

Hi David,

Quoted 3 lines · David Brownell

The only issue I have is that code which the driver probe() calls may expect client->driver to be set, but it won't be...

Quoted 7 lines
Quoted 3 lines

While I'd rather see client->driver vanish, in this case I'd just suggest leaving the early assignment, and nulling it out on error (instead of assigning it on success).

Oh well, that's exactly what Hans' patch did originally, I changed it to make the code more simple, but if you think it is better that way, I'll revert this change of mine. Here you go:

Yeah, this has much less chance to break something inadvertently.

- Dave

Quoted 37 lines

From: Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/[email protected]>

Ensure that client->driver is set to NULL if the probe() returns an error (this keeps client->driver and client->dev.driver in sync).

---
 drivers/i2c/i2c-core.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/i2c/i2c-core.c +5 -1
@@ -90,12 +90,16 @@ static int i2c_device_probe(struct devic
 {
 	struct i2c_client	*client = to_i2c_client(dev);
 	struct i2c_driver	*driver = to_i2c_driver(dev->driver);
+	int status;
 
 	if (!driver->probe)
 		return -ENODEV;
 	client->driver = driver;
 	dev_dbg(dev, "probe\n");
-	return driver->probe(client);
+	status = driver->probe(client);
+	if (status)
+		client->driver = NULL;
+	return status;
 }
 
 static int i2c_device_remove(struct device *dev)
Signature · 2 lines
-- 
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c