[PATCH]i2c-dev: add ioctls PROBE and REMOVE

[PATCH]i2c-dev: add ioctls PROBE and REMOVE

[Michael Lawnick]

From: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>

After system startup currently there is no way to add an i2c client to
subsystem. This may occur if the client sits on a subsystem that was
powerless at the beginning or is plugged in at runtime.
This patch adds ioctl to /dev/i2c-X to initiate a probe of an client and
to remove a client that may have gone.
Example:
fd=open("/dev/i2c-1",O_RDWR);
ioctl(fd,I2C_PROBE,"lm75,0x49");
close(fd);
will start probing of an lm75 compatible sensor at bus 1, id 0x49
ioctl(fd,I2C_REMOVE,0x49)
removes it again.

Signed-off-by: Michael Lawnick <ml.lawnick-Mmb7MZpHnFY@public.gmane.org>
---
Add ioctls I2C_PROBE and I2C_REMOVE to /dev/i2c-x

 drivers/i2c/i2c-dev.c   |   50
++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/i2c-dev.h |    2 +
 2 files changed, 51 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index c171988..82cbe7f
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -367,9 +367,22 @@ static noinline int i2cdev_ioctl_smbus(struct
i2c_client *client,
 	return res;
 }

+static int i2c_check_addr(struct device *dev, void *addrp)
+{
+	struct i2c_client	*client = i2c_verify_client(dev);
+	int			addr = *(int *)addrp;
+
+	if (client && client->addr == addr)
+		return -EBUSY;
+	return 0;
+}
+
 static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned
long arg)
 {
-	struct i2c_client *client = (struct i2c_client *)file->private_data;
+	struct i2c_client *bus_client, *client = (struct i2c_client
*)file->private_data;
+	struct i2c_board_info bInfo;
+	struct device *dev;
+	char   *colon, request[I2C_NAME_SIZE + 5];
 	unsigned long funcs;

 	dev_dbg(&client->adapter->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n",
@@ -424,6 +437,41 @@ static long i2cdev_ioctl(struct file *file,
unsigned int cmd, unsigned long arg)
 	case I2C_TIMEOUT:
 		client->adapter->timeout = arg;
 		break;
+	case I2C_PROBE:
+		memset(&bInfo, 0, sizeof(bInfo));
+		copy_from_user(request, (char *)arg, sizeof(request));
+
+		colon = strchr(request, ',');
+		if((!colon) || (colon - request > I2C_NAME_SIZE - 1))
+			return -EINVAL;
+
+		strncpy(bInfo.type, request, colon - request);
+		bInfo.type[I2C_NAME_SIZE - 1] = '\0';
+		bInfo.addr = simple_strtoul(colon + 1, NULL, 0);
+
+		bus_client = i2c_new_device(client->adapter, &bInfo);
+
+		if(!bus_client)
+			return -EIO;
+			
+		if(!bus_client->driver)
+		{
+			/* probe failed -> cleanup */
+			i2c_unregister_device(bus_client);
+			return -ENODEV;
+		}
+		break;
+	case I2C_REMOVE:
+		dev = device_find_child(&client->adapter->dev, &arg, i2c_check_addr);
+		if (!dev)
+			return -ENODEV;
+
+		bus_client = i2c_verify_client(dev);
+		if(!bus_client)
+			return -EIO;
+
+		i2c_unregister_device(bus_client);
+		break;
 	default:
 		/* NOTE:  returning a fault code here could cause trouble
 		 * in buggy userspace code.  Some old kernel bugs returned
diff --git a/include/linux/i2c-dev.h b/include/linux/i2c-dev.h
index 311315b..8b66638
--- a/include/linux/i2c-dev.h
+++ b/include/linux/i2c-dev.h
@@ -50,6 +50,8 @@
 #define I2C_PEC		0x0708	/* != 0 to use PEC with SMBus */
 #define I2C_SMBUS	0x0720	/* SMBus transfer */

+#define I2C_PROBE   0x0730  /* Probe for client "<name>,<addr>" */
+#define I2C_REMOVE  0x0731  /* Remove client <addr> */

 /* This is the structure as used in the I2C_SMBUS ioctl call */
 struct i2c_smbus_ioctl_data {

Re: [PATCH]i2c-dev: add ioctls PROBE and REMOVE

[Michael Lawnick]

Hi Jean,

could you give it a look?
If you at least ACK the interface, I could do the patch for the
documentation.

Regards,
Michael

Re: [PATCH]i2c-dev: add ioctls PROBE and REMOVE

[Jean Delvare]

Hi Michael,

On Fri, 27 Mar 2009 11:19:27 +0100, Michael Lawnick wrote:
> could you give it a look?
> If you at least ACK the interface, I could do the patch for the
> documentation.

Come on, which part of "this should be implemented in sysfs" did you
not understand? I'm a little tired of you trying to implement it using
_all_ other possible ways except the one we agreed on. If you need this
feature faster than I can implement it, please stick to the plan and
implement it using sysfs. If not, then just stay quiet and wait for it
to happen.

Thanks,
-- 
Jean Delvare

Re: [PATCH]i2c-dev: add ioctls PROBE and REMOVE

[Michael Lawnick]

Jean Delvare said the following:
> Hi Michael,
> 
> On Fri, 27 Mar 2009 11:19:27 +0100, Michael Lawnick wrote:
>> could you give it a look?
>> If you at least ACK the interface, I could do the patch for the
>> documentation.
> 
> Come on, which part of "this should be implemented in sysfs" did you
> not understand? I'm a little tired of you trying to implement it using
> _all_ other possible ways except the one we agreed on. If you need this
> feature faster than I can implement it, please stick to the plan and
> implement it using sysfs. If not, then just stay quiet and wait for it
> to happen.

Huh - don't bite me!

This step I wanted to discuss next:
AFAICS I2C subsystem doesn't implement any sysFs-entry itself. They are
all either from client or device-driver system. I not even dared to ask
for a comment to this before adding the feature on i2c-dev, a way that
does not change the visible interface.
I can't wait too long for implementation on sysFs. I'm ready to do it
now, if we agree on the place where to create the entries, naming and
interface.
What is your time line?
Naming IMHO should be probe/remove, parameters could be
"<client-name>,<id>" / "<id>", just as done in i2c-dev implementation,
if created per adapter.
But where to place?

-- 
*Kind* regards,
Michael

Re: [PATCH]i2c-dev: add ioctls PROBE and REMOVE

[Jean Delvare]

On Fri, 27 Mar 2009 14:13:32 +0100, Michael Lawnick wrote:
> Jean Delvare said the following:
> > Hi Michael,
> > 
> > On Fri, 27 Mar 2009 11:19:27 +0100, Michael Lawnick wrote:
> >> could you give it a look?
> >> If you at least ACK the interface, I could do the patch for the
> >> documentation.
> > 
> > Come on, which part of "this should be implemented in sysfs" did you
> > not understand? I'm a little tired of you trying to implement it using
> > _all_ other possible ways except the one we agreed on. If you need this
> > feature faster than I can implement it, please stick to the plan and
> > implement it using sysfs. If not, then just stay quiet and wait for it
> > to happen.
> 
> Huh - don't bite me!
> 
> This step I wanted to discuss next:
> AFAICS I2C subsystem doesn't implement any sysFs-entry itself. They are
> all either from client or device-driver system.

You didn't look carefully then. From i2c-core.c:

static ssize_t
show_adapter_name(struct device *dev, struct device_attribute *attr, char *buf)
{
	struct i2c_adapter *adap = to_i2c_adapter(dev);
	return sprintf(buf, "%s\n", adap->name);
}

static struct device_attribute i2c_adapter_attrs[] = {
	__ATTR(name, S_IRUGO, show_adapter_name, NULL),
	{ },
};

static struct class i2c_adapter_class = {
	.owner			= THIS_MODULE,
	.name			= "i2c-adapter",
	.dev_attrs		= i2c_adapter_attrs,
};

> I not even dared to ask
> for a comment to this before adding the feature on i2c-dev, a way that
> does not change the visible interface.

What do you think i2c-dev is, if not a visible interface? If we add a
new ioctl to the i2c-dev interface, we have to maintain it forever.
That's not to be done lightly just because you can't wait for you
actual need to be fulfilled by i2c-core.

> I can't wait too long for implementation on sysFs. I'm ready to do it
> now, if we agree on the place where to create the entries, naming and
> interface.

The place is very clearly /sys/class/i2c-adapter/i2c-*, next to the
"name" attribute I quoted above.

The naming and interface are to be discussed. Could be 2 write-only
files, add_device and delete_device, and the most basic syntax I can
think of would be:

echo lm75 0x49 > /sys/class/i2c-adapter/i2c-0/add_device
echo 0x49 > /sys/class/i2c-adapter/i2c-0/delete_device

add_device would optionally accept an irq value. As for delete_device,
I am wondering if we should require the chip name as well, for symmetry
with add_device and for safety reasons, or if the above is sufficient.

> What is your time line?

Getting rid of the legacy binding model has higher priority and is far
from being done. I also must take care of Rodolfo's multiplexer
support, which a lot of people have been asking for, and which might as
well be what you really need. The above interface comes next, I can't
give it higher priority as the same can already be achieved today using
module parameters for most drivers.

> Naming IMHO should be probe/remove, parameters could be

No, we can't use probe/remove as these are used for a completely
different meaning in the Linux driver model (and these _are_ misnomers
there, no question about that, but we have to live with it.) Not that
"probe" would be a good name anyway, as you are instantiating a device,
you are not probing for anything.

> "<client-name>,<id>" / "<id>", just as done in i2c-dev implementation,
> if created per adapter.

I'd go with a space instead of comma between parameters, for
consistency with the pci subsystem's "new_id" files.

And it's really <address>, NOT <id>, even though we use the address as
part of the device id internally.

> But where to place?

Answered above, that was the easy part.

-- 
Jean Delvare

Re: [PATCH]i2c-dev: add ioctls PROBE and REMOVE

[Jean Delvare]

On Fri, 27 Mar 2009 14:57:26 +0100, Jean Delvare wrote:
> On Fri, 27 Mar 2009 14:13:32 +0100, Michael Lawnick wrote:
> > What is your time line?
> 
> Getting rid of the legacy binding model has higher priority and is far
> from being done. I also must take care of Rodolfo's multiplexer
> support, which a lot of people have been asking for, and which might as
> well be what you really need. The above interface comes next, I can't
> give it higher priority as the same can already be achieved today using
> module parameters for most drivers.

BTW, if you don't want your problem to be overlooked, I strongly
suggest that you create yourself a wiki account on
http://i2c.wiki.kernel.org/, create a page describing the problem and
its proposed solution, and link to it from the "Work in progress"
section.

-- 
Jean Delvare

Re: [PATCH]i2c-dev: add ioctls PROBE and REMOVE

[Michael Lawnick]

Jean Delvare said the following:
> BTW, if you don't want your problem to be overlooked, I strongly
> suggest that you create yourself a wiki account on
> http://i2c.wiki.kernel.org/, create a page describing the problem and
> its proposed solution, and link to it from the "Work in progress"
> section.
> 
Ok, done.
<http://i2c.wiki.kernel.org/index.php?title=SysFs_entries_add_device/delete_device>

I made a patch, that uses i2c_new_device. But this won't use the detect
method of the client, so its possible to instantiate almost any code on
the given address :-(
Shouldn't we use i2c_detect_address() or something similar?

-- 
Kind regards,
Michael

Re: [PATCH]i2c-dev: add ioctls PROBE and REMOVE

[Jean Delvare]

Hi Michael,

Sorry for the late answer, I am pretty busy with the 2.6.30 merge
window.

On Thu, 02 Apr 2009 14:36:27 +0200, Michael Lawnick wrote:
> Jean Delvare said the following:
> > BTW, if you don't want your problem to be overlooked, I strongly
> > suggest that you create yourself a wiki account on
> > http://i2c.wiki.kernel.org/, create a page describing the problem and
> > its proposed solution, and link to it from the "Work in progress"
> > section.
> > 
> Ok, done.
> <http://i2c.wiki.kernel.org/index.php?title=SysFs_entries_add_device/delete_device>

Thank you.

> I made a patch, that uses i2c_new_device. But this won't use the detect
> method of the client, so its possible to instantiate almost any code on
> the given address :-(
> Shouldn't we use i2c_detect_address() or something similar?

No. The whole point of the sysfs entries you are adding is to let the
user instantiate I2C devices (at the device driver model level) for
physical devices which can _not_ be detected. For devices which can be
reliably detected, you would simply implement the .detect() method, set
the I2C class, and let i2c-core and the device driver take care of the
detection and device creation.

I hear you, "what about hot-plugged devices, they won't be detected"?
For one thing, these need to be behind a mux or gate, which we don't
support at the moment, so discussing this now doesn't make much sense.
But at any rate I don't think that the files you are adding in sysfs
are meant to address this particular issue. If we want to be able to
re-trigger detection on a given I2C segment (which currently only
happens at I2C adapter creation time and when a new driver is
registered) this would be done by a separate sysfs entry (say "detect",
write 1 to re-trigger all detections on a given segment). But really
I'd rather wait for Rodoflo's work to be merged first, depending on the
implementation there may or may not be a practical need for this.

Right now I am working hard at getting rid of the few remaining legacy
i2c device drivers. Then I can clean up i2c-core and then we can look
into Rodolfo's patches. I know it seems to take forever, but actually
we are making progress and will finally get there.

-- 
Jean Delvare

Re: [PATCH]i2c-dev: add ioctls PROBE and REMOVE

[Michael Lawnick]

Hi Jean,

although you are very busy, I hope you can spend a little more time.

Jean Delvare said the following:
> Hi Michael,
> 
> Sorry for the late answer, I am pretty busy with the 2.6.30 merge
> window.
> 
> ... The whole point of the sysfs entries you are adding is to let the
> user instantiate I2C devices (at the device driver model level) for
> physical devices which can _not_ be detected. 

I see, that's _your_ motivation. You want a user 'force' method.

...
> I hear you, "what about hot-plugged devices, they won't be detected"?
> For one thing, these need to be behind a mux or gate, which we don't
> support at the moment, so discussing this now doesn't make much sense.

No, sorry, here your are mistaken.
IMHO you recognize only a part of the problem: Even if separation is
done by a mux (what is not strictly necessary), it will be powered by
main system and its driver (i.e. adapters) loaded on boot. Still clients
may come up when system is already initialized and auto detection on the
sub buses has already completed.

> But at any rate I don't think that the files you are adding in sysfs
> are meant to address this particular issue.

Sorry, then you misunderstood me - it *is* this particular issue I need
to solve.
Please forget the thread about lm75a (vs. lm75), this was just a stone
on my way to a new running kernel. It took it, made a remark and threw
it aside. Nothing more. It is not related to the detect problem.

> If we want to be able to
> re-trigger detection on a given I2C segment (which currently only
> happens at I2C adapter creation time and when a new driver is
> registered) this would be done by a separate sysfs entry (say "detect",
> write 1 to re-trigger all detections on a given segment).

This solution will cause much useless traffic while the system is
already running and doing other hard work :-(

> But really
> I'd rather wait for Rodoflo's work to be merged first, depending on the
> implementation there may or may not be a practical need for this.

AFAICS has Rodolfo's work nothing to do with the problem above, as on
adapter creation an auto detection will (should) be triggered.

-- 
Michael Lawnick

Re: [PATCH]i2c-dev: add ioctls PROBE and REMOVE

[Jean Delvare]

Hi Michael,

On Tue, 07 Apr 2009 14:17:46 +0200, Michael Lawnick wrote:
> Hi Jean,
> 
> although you are very busy, I hope you can spend a little more time.
> 
> Jean Delvare said the following:
> > Hi Michael,
> > 
> > Sorry for the late answer, I am pretty busy with the 2.6.30 merge
> > window.
> > 
> > ... The whole point of the sysfs entries you are adding is to let the
> > user instantiate I2C devices (at the device driver model level) for
> > physical devices which can _not_ be detected. 
> 
> I see, that's _your_ motivation.

Yes it is. Admittedly, the feature you are asking for is not one I
think a lot of people will be interested in, it seems pretty borderline
as far as the I2C standard is concerned, therefore I am trying to see
what other widely needed feature could be implemented, which would
incidentally cover your uncommon need.

> You want a user 'force' method.

[Nitpicking mode] Instantiate, not force. You're really only creating
a device, and then letting any interested driver bind to it if it feels
like it. Just like platform data can do on embedded platforms, except
that it comes from user-space.

But yes, there is no verification (or detection) step involved. The i2c
subsystem takes your word, assumes you know what you're doing, and
simply does what you asked for.

> > I hear you, "what about hot-plugged devices, they won't be detected"?
> > For one thing, these need to be behind a mux or gate, which we don't
> > support at the moment, so discussing this now doesn't make much sense.
> 
> No, sorry, here your are mistaken.
> IMHO you recognize only a part of the problem: Even if separation is
> done by a mux (what is not strictly necessary),

It is necessary. "Simple" hot-plug is a violation of the I2C standard.
You need a way to guarantee that no I2C traffic will be on-going when
the new device is connected, and for this you need at least an I2C gate
to isolate the bus segment in question from all masters on the bus.

>                                                 it will be powered by
> main system and its driver (i.e. adapters) loaded on boot. Still clients
> may come up when system is already initialized and auto detection on the
> sub buses has already completed.

This is where I am unclear. It might make sense to remove (in the
device driver model sense of the term) i2c segments which are being
modified, and restore them afterwards. That is, I2C segments hanging
off a mux would have 3 possible states, active, inactive and disabled.
The change from disabled to active or inactive would trigger device
detection, just as the mere addition of an i2c adapter currently does.

I don't think this is what Rodoflo did, but this would address your
problem as far as I can see.

> > But at any rate I don't think that the files you are adding in sysfs
> > are meant to address this particular issue.
> 
> Sorry, then you misunderstood me - it *is* this particular issue I need
> to solve.
> Please forget the thread about lm75a (vs. lm75), this was just a stone
> on my way to a new running kernel. It took it, made a remark and threw
> it aside. Nothing more. It is not related to the detect problem.
> 
> > If we want to be able to
> > re-trigger detection on a given I2C segment (which currently only
> > happens at I2C adapter creation time and when a new driver is
> > registered) this would be done by a separate sysfs entry (say "detect",
> > write 1 to re-trigger all detections on a given segment).
> 
> This solution will cause much useless traffic while the system is
> already running and doing other hard work :-(

Much useless traffic? It will cause way less traffic than when adding a
new i2c adapter (or bus segment) in the first place. All devices which
are already bound will be excluded from this detection round. This
should be most of them as I understand it. Most probably, only the
newly added device will receive I2C traffic.

Sure, we could extend the syntax so that you can ask for just one
address on one bus segment to be probed (maybe even by just one
driver). But what's the point? If you already know all this, then the
add_device sysfs interface is doing the job. And remember that the
device you just connected might not even be reliably detectable at all!

> > But really
> > I'd rather wait for Rodoflo's work to be merged first, depending on the
> > implementation there may or may not be a practical need for this.
> 
> AFAICS has Rodolfo's work nothing to do with the problem above, as on
> adapter creation an auto detection will (should) be triggered.

See my point above... The detection could happen at creation time _or_
at activation time. In the latter case Rodoflo's work becomes totally
relevant.

-- 
Jean Delvare