For today there is no way to use i3c devices from user space and
the introduction of such API will help developers during the i3c device
or i3c host controllers development.
The i3cdev module is highly based on i2c-dev and yet I tried to address
the concerns raised in [1].
NOTES:
- The i3cdev dynamically request an unused major number.
- The i3c devices are dynamically exposed/removed from dev/ folder based
on if they have a device driver bound to it.
- For now, the module exposes i3c devices without device driver on
dev/i3c-<bus>-<pid>, but we can change the path to
dev/bus/i3c/<bus>-<pid> or dev/i3c/<bus>-<pid>.
- As in the i2c subsystem, here it is exposed the i3c_priv_xfer to
userspace. I tried to use a dedicated structure as in spidev but I don't
see any obvious advantage.
- Since the i3c API only exposes i3c_priv_xfer to devices, for now, the
module just makes use of one ioctl(). This can change in the future with
the introduction hdr commands or by the need of exposing some CCC
commands to the device API (private contract between master-slave).
Regarding the i3c device info, some information is already available
through sysfs. We can add more device attributes to expose more
information or add a dedicated ioctl() request for that purpose or both.
- Similar to i2c, I have also created a tool that you can find in [2]
for testing purposes. If you have some time available I would appreciate
your feedback about it as well.
[1] https://lkml.org/lkml/2018/11/15/853
[2] https://github.com/vitor-soares-snps/i3c-tools.git
Changes in v2:
Use IDR api for minor numbering
Modify ioctl struct
Fix SPDX license
Vitor Soares (4):
i3c: master: export i3c_masterdev_type
i3c: master: export i3c_bus_type symbol
i3c: master: add i3c_for_each_dev helper
i3c: add i3cdev module to expose i3c dev in /dev
drivers/i3c/Kconfig | 15 ++
drivers/i3c/Makefile | 1 +
drivers/i3c/i3cdev.c | 429 ++++++++++++++++++++++++++++++++++++++++
drivers/i3c/internals.h | 2 +
drivers/i3c/master.c | 16 +-
include/uapi/linux/i3c/i3cdev.h | 38 ++++
6 files changed, 500 insertions(+), 1 deletion(-)
create mode 100644 drivers/i3c/i3cdev.c
create mode 100644 include/uapi/linux/i3c/i3cdev.h
--
2.7.4
Introduce i3c_for_each_dev(), an i3c device iterator for use by i3cdev.
Signed-off-by: Vitor Soares <[email protected]>
---
drivers/i3c/internals.h | 1 +
drivers/i3c/master.c | 12 ++++++++++++
2 files changed, 13 insertions(+)
diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index bc062e8..a6deedf 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -24,4 +24,5 @@ int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
const struct i3c_ibi_setup *req);
void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev);
+int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *));
#endif /* I3C_INTERNAL_H */
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 21c4372..8e22da2 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2640,6 +2640,18 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
dev->ibi = NULL;
}
+int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *))
+{
+ int res;
+
+ mutex_lock(&i3c_core_lock);
+ res = bus_for_each_dev(&i3c_bus_type, NULL, data, fn);
+ mutex_unlock(&i3c_core_lock);
+
+ return res;
+}
+EXPORT_SYMBOL_GPL(i3c_for_each_dev);
+
static int __init i3c_init(void)
{
return bus_register(&i3c_bus_type);
--
2.7.4
Export i3c_bus_type symbol so i3cdev can register a notifier chain
for i3c bus.
Signed-off-by: Vitor Soares <[email protected]>
---
drivers/i3c/master.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 8a0ba34..21c4372 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -321,6 +321,7 @@ struct bus_type i3c_bus_type = {
.probe = i3c_device_probe,
.remove = i3c_device_remove,
};
+EXPORT_SYMBOL_GPL(i3c_bus_type);
static enum i3c_addr_slot_status
i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
--
2.7.4
Exporte i3c_masterdev_type so i3cdev module can verify if an i3c device
is a master.
Signed-off-by: Vitor Soares <[email protected]>
---
drivers/i3c/internals.h | 1 +
drivers/i3c/master.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 86b7b44..bc062e8 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -11,6 +11,7 @@
#include <linux/i3c/master.h>
extern struct bus_type i3c_bus_type;
+extern const struct device_type i3c_masterdev_type;
void i3c_bus_normaluse_lock(struct i3c_bus *bus);
void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 7f8f896..8a0ba34 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -523,9 +523,10 @@ static void i3c_masterdev_release(struct device *dev)
of_node_put(dev->of_node);
}
-static const struct device_type i3c_masterdev_type = {
+const struct device_type i3c_masterdev_type = {
.groups = i3c_masterdev_groups,
};
+EXPORT_SYMBOL_GPL(i3c_masterdev_type);
static int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
unsigned long max_i2c_scl_rate)
--
2.7.4
Hello Vitor,
Sorry for taking so long to reply, and thanks for working on that topic.
On Wed, 29 Jan 2020 13:17:31 +0100
Vitor Soares <[email protected]> wrote:
> For today there is no way to use i3c devices from user space and
> the introduction of such API will help developers during the i3c device
> or i3c host controllers development.
>
> The i3cdev module is highly based on i2c-dev and yet I tried to address
> the concerns raised in [1].
>
> NOTES:
> - The i3cdev dynamically request an unused major number.
>
> - The i3c devices are dynamically exposed/removed from dev/ folder based
> on if they have a device driver bound to it.
May I ask why you need to automatically bind devices to the i3cdev
driver when they don't have a driver matching the device id
loaded/compiled-in? If we get the i3c subsystem to generate proper
uevents we should be able to load the i3cdev module and bind the device
to this driver using a udev rule.
Regards,
Boris
On Wed, 29 Jan 2020 13:17:32 +0100
Vitor Soares <[email protected]> wrote:
> Exporte i3c_masterdev_type so i3cdev module can verify if an i3c device
> is a master.
>
> Signed-off-by: Vitor Soares <[email protected]>
> ---
> drivers/i3c/internals.h | 1 +
> drivers/i3c/master.c | 3 ++-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 86b7b44..bc062e8 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -11,6 +11,7 @@
> #include <linux/i3c/master.h>
>
> extern struct bus_type i3c_bus_type;
> +extern const struct device_type i3c_masterdev_type;
>
> void i3c_bus_normaluse_lock(struct i3c_bus *bus);
> void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 7f8f896..8a0ba34 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -523,9 +523,10 @@ static void i3c_masterdev_release(struct device *dev)
> of_node_put(dev->of_node);
> }
>
> -static const struct device_type i3c_masterdev_type = {
> +const struct device_type i3c_masterdev_type = {
> .groups = i3c_masterdev_groups,
> };
> +EXPORT_SYMBOL_GPL(i3c_masterdev_type);
No need to export the symbol, removing the static and adding the
definition to internal.h should work just fine (i3c.o contains
both master.o and device.o).
>
> static int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
> unsigned long max_i2c_scl_rate)
On Mon, 17 Feb 2020 15:56:23 +0100
Boris Brezillon <[email protected]> wrote:
> On Wed, 29 Jan 2020 13:17:32 +0100
> Vitor Soares <[email protected]> wrote:
>
> > Exporte i3c_masterdev_type so i3cdev module can verify if an i3c device
> > is a master.
> >
> > Signed-off-by: Vitor Soares <[email protected]>
> > ---
> > drivers/i3c/internals.h | 1 +
> > drivers/i3c/master.c | 3 ++-
> > 2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> > index 86b7b44..bc062e8 100644
> > --- a/drivers/i3c/internals.h
> > +++ b/drivers/i3c/internals.h
> > @@ -11,6 +11,7 @@
> > #include <linux/i3c/master.h>
> >
> > extern struct bus_type i3c_bus_type;
> > +extern const struct device_type i3c_masterdev_type;
> >
> > void i3c_bus_normaluse_lock(struct i3c_bus *bus);
> > void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 7f8f896..8a0ba34 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -523,9 +523,10 @@ static void i3c_masterdev_release(struct device *dev)
> > of_node_put(dev->of_node);
> > }
> >
> > -static const struct device_type i3c_masterdev_type = {
> > +const struct device_type i3c_masterdev_type = {
> > .groups = i3c_masterdev_groups,
> > };
> > +EXPORT_SYMBOL_GPL(i3c_masterdev_type);
>
> No need to export the symbol, removing the static and adding the
> definition to internal.h should work just fine (i3c.o contains
> both master.o and device.o).
Hm, my bad. Looks like i3cdev is a separate module/driver. If that's
the case, it should not have direct access to internals.h. I see 2
options here:
1/ make the i3cdev logic part of the core
2/ provide helpers to find devices by type
But maybe none of that is needed if you let userspace bind i3c devices
to the i3cdev driver.
>
> >
> > static int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
> > unsigned long max_i2c_scl_rate)
>
On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
<[email protected]> wrote:
> Sorry for taking so long to reply, and thanks for working on that topic.
>
> On Wed, 29 Jan 2020 13:17:31 +0100
> Vitor Soares <[email protected]> wrote:
>
> > For today there is no way to use i3c devices from user space and
> > the introduction of such API will help developers during the i3c device
> > or i3c host controllers development.
> >
> > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > the concerns raised in [1].
> >
> > NOTES:
> > - The i3cdev dynamically request an unused major number.
> >
> > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > on if they have a device driver bound to it.
>
> May I ask why you need to automatically bind devices to the i3cdev
> driver when they don't have a driver matching the device id
> loaded/compiled-in? If we get the i3c subsystem to generate proper
> uevents we should be able to load the i3cdev module and bind the device
> to this driver using a udev rule.
I think that would require manual configuration to ensure that the correct
set of devices get bound to either the userspace driver or an in-kernel
driver. The method from the current patch series is more complicated,
but it means that any device can be accessed by the user space driver
as long as it's not already owned by a kernel driver.
Arnd
Hi Boris,
From: Boris Brezillon <[email protected]>
Date: Mon, Feb 17, 2020 at 14:51:41
> Hello Vitor,
>
> Sorry for taking so long to reply, and thanks for working on that topic.
>
> On Wed, 29 Jan 2020 13:17:31 +0100
> Vitor Soares <[email protected]> wrote:
>
> > For today there is no way to use i3c devices from user space and
> > the introduction of such API will help developers during the i3c device
> > or i3c host controllers development.
> >
> > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > the concerns raised in [1].
> >
> > NOTES:
> > - The i3cdev dynamically request an unused major number.
> >
> > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > on if they have a device driver bound to it.
>
> May I ask why you need to automatically bind devices to the i3cdev
> driver when they don't have a driver matching the device id
> loaded/compiled-in? If we get the i3c subsystem to generate proper
> uevents we should be able to load the i3cdev module and bind the device
> to this driver using a udev rule.
My idea was to expose every device to user-space by default so we can
test them without a driver (more or less the i2c use case) but as we
agreed during the i3c subsystem only expose devices that doesn't have
device driver.
I considered to have a uevent but to expose the devices by default it
would required something generic, what I didn't figure out and tend to
follow the i2c-dev module.
With this current approach even if a device has a driver we can unbind it
through the Sysfs and have access from user space which I found useful
for debug.
>
> Regards,
>
> Boris
On Mon, 17 Feb 2020 16:06:45 +0100
Arnd Bergmann <[email protected]> wrote:
> On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> <[email protected]> wrote:
> > Sorry for taking so long to reply, and thanks for working on that topic.
> >
> > On Wed, 29 Jan 2020 13:17:31 +0100
> > Vitor Soares <[email protected]> wrote:
> >
> > > For today there is no way to use i3c devices from user space and
> > > the introduction of such API will help developers during the i3c device
> > > or i3c host controllers development.
> > >
> > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > the concerns raised in [1].
> > >
> > > NOTES:
> > > - The i3cdev dynamically request an unused major number.
> > >
> > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > on if they have a device driver bound to it.
> >
> > May I ask why you need to automatically bind devices to the i3cdev
> > driver when they don't have a driver matching the device id
> > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > uevents we should be able to load the i3cdev module and bind the device
> > to this driver using a udev rule.
>
> I think that would require manual configuration to ensure that the correct
> set of devices get bound to either the userspace driver or an in-kernel
> driver.
Hm, isn't that what udev is supposed to do anyway? Remember that
I3C devices expose a manufacturer and part-id (which are similar to the
USB vendor and product ids), so deciding when an I3C device should be
bound to the i3cdev driver should be fairly easy, and that's a
per-device decision anyway.
> The method from the current patch series is more complicated,
> but it means that any device can be accessed by the user space driver
> as long as it's not already owned by a kernel driver.
Well, I'm more worried about the extra churn this auto-binding logic
might create for the common 'on-demand driver loading' use case. At
first, there's no driver matching a specific device, but userspace
might load one based on the uevents it receives. With the current
approach, that means we'd first have to unbind the device before
loading the driver. AFAICT, no other subsystem does that.
On Mon, 17 Feb 2020 15:32:55 +0000
Vitor Soares <[email protected]> wrote:
> Hi Boris,
>
> From: Boris Brezillon <[email protected]>
> Date: Mon, Feb 17, 2020 at 14:51:41
>
> > Hello Vitor,
> >
> > Sorry for taking so long to reply, and thanks for working on that topic.
> >
> > On Wed, 29 Jan 2020 13:17:31 +0100
> > Vitor Soares <[email protected]> wrote:
> >
> > > For today there is no way to use i3c devices from user space and
> > > the introduction of such API will help developers during the i3c device
> > > or i3c host controllers development.
> > >
> > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > the concerns raised in [1].
> > >
> > > NOTES:
> > > - The i3cdev dynamically request an unused major number.
> > >
> > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > on if they have a device driver bound to it.
> >
> > May I ask why you need to automatically bind devices to the i3cdev
> > driver when they don't have a driver matching the device id
> > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > uevents we should be able to load the i3cdev module and bind the device
> > to this driver using a udev rule.
>
> My idea was to expose every device to user-space by default so we can
> test them without a driver (more or less the i2c use case) but as we
> agreed during the i3c subsystem only expose devices that doesn't have
> device driver.
Those that do not have a driver *yet*. What if i3cdev is compiled-in
and other I3C drivers are enabled as modules? When the device is
discovered at boot time is will be automatically bound to the i3cdev
driver since no matching drivers are available at that point. But
the end user probably expects this device to be attached to the in
kernel driver.
> I considered to have a uevent but to expose the devices by default it
> would required something generic, what I didn't figure out and tend to
> follow the i2c-dev module.
Well, I3C and I2C/SPI are quite different in this regard. I2C dev
exposes the whole bus, and SPI devs don't have a standard way to
uniquely identify the device connected on the bus (unless it has a
dedicated compatible for DT-based boards). In that case it might make
sense to auto-bind all orphan devs to the default spidev driver, though
I'm not entirely sure it's really necessary since that's probably a
per-board decision, and having a udev rule matching the bus/CS would
make sense too.
>
> With this current approach even if a device has a driver we can unbind it
> through the Sysfs and have access from user space which I found useful
> for debug.
That's my point. We're making the default 'on-demand driver loading'
use case more complicated to ease the less common 'userspace driver'
use case.
Hi,
From: Boris Brezillon <[email protected]>
Date: Mon, Feb 17, 2020 at 15:36:22
> On Mon, 17 Feb 2020 16:06:45 +0100
> Arnd Bergmann <[email protected]> wrote:
>
> > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > <[email protected]> wrote:
> > > Sorry for taking so long to reply, and thanks for working on that topic.
> > >
> > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > Vitor Soares <[email protected]> wrote:
> > >
> > > > For today there is no way to use i3c devices from user space and
> > > > the introduction of such API will help developers during the i3c device
> > > > or i3c host controllers development.
> > > >
> > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > the concerns raised in [1].
> > > >
> > > > NOTES:
> > > > - The i3cdev dynamically request an unused major number.
> > > >
> > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > > on if they have a device driver bound to it.
> > >
> > > May I ask why you need to automatically bind devices to the i3cdev
> > > driver when they don't have a driver matching the device id
> > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > uevents we should be able to load the i3cdev module and bind the device
> > > to this driver using a udev rule.
> >
> > I think that would require manual configuration to ensure that the correct
> > set of devices get bound to either the userspace driver or an in-kernel
> > driver.
>
> Hm, isn't that what udev is supposed to do anyway? Remember that
> I3C devices expose a manufacturer and part-id (which are similar to the
> USB vendor and product ids), so deciding when an I3C device should be
> bound to the i3cdev driver should be fairly easy, and that's a
> per-device decision anyway.
>
> > The method from the current patch series is more complicated,
> > but it means that any device can be accessed by the user space driver
> > as long as it's not already owned by a kernel driver.
>
> Well, I'm more worried about the extra churn this auto-binding logic
> might create for the common 'on-demand driver loading' use case. At
> first, there's no driver matching a specific device, but userspace
> might load one based on the uevents it receives. With the current
> approach, that means we'd first have to unbind the device before
> loading the driver. AFAICT, no other subsystem does that.
I'm about to finish v3 (today or tomorrow) and I think I fixed all
concerns rise during v2. I would like you to see that version before any
change.
Best regards,
Vitor Soares
On Mon, Feb 17, 2020 at 03:55:08PM +0000, Vitor Soares wrote:
> Hi,
>
> From: Boris Brezillon <[email protected]>
> Date: Mon, Feb 17, 2020 at 15:36:22
>
> > On Mon, 17 Feb 2020 16:06:45 +0100
> > Arnd Bergmann <[email protected]> wrote:
> >
> > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > <[email protected]> wrote:
> > > > Sorry for taking so long to reply, and thanks for working on that topic.
> > > >
> > > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > > Vitor Soares <[email protected]> wrote:
> > > >
> > > > > For today there is no way to use i3c devices from user space and
> > > > > the introduction of such API will help developers during the i3c device
> > > > > or i3c host controllers development.
> > > > >
> > > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > > the concerns raised in [1].
> > > > >
> > > > > NOTES:
> > > > > - The i3cdev dynamically request an unused major number.
> > > > >
> > > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > > > on if they have a device driver bound to it.
> > > >
> > > > May I ask why you need to automatically bind devices to the i3cdev
> > > > driver when they don't have a driver matching the device id
> > > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > > uevents we should be able to load the i3cdev module and bind the device
> > > > to this driver using a udev rule.
> > >
> > > I think that would require manual configuration to ensure that the correct
> > > set of devices get bound to either the userspace driver or an in-kernel
> > > driver.
> >
> > Hm, isn't that what udev is supposed to do anyway? Remember that
> > I3C devices expose a manufacturer and part-id (which are similar to the
> > USB vendor and product ids), so deciding when an I3C device should be
> > bound to the i3cdev driver should be fairly easy, and that's a
> > per-device decision anyway.
> >
> > > The method from the current patch series is more complicated,
> > > but it means that any device can be accessed by the user space driver
> > > as long as it's not already owned by a kernel driver.
> >
> > Well, I'm more worried about the extra churn this auto-binding logic
> > might create for the common 'on-demand driver loading' use case. At
> > first, there's no driver matching a specific device, but userspace
> > might load one based on the uevents it receives. With the current
> > approach, that means we'd first have to unbind the device before
> > loading the driver. AFAICT, no other subsystem does that.
>
> I'm about to finish v3 (today or tomorrow) and I think I fixed all
> concerns rise during v2. I would like you to see that version before any
> change.
Why are there so many "RFC" series here? I treat "RFC" as "I don't
really like this patch but I'm throwing it out there for others to look
at if they care". No RFC should ever go on beyond a v1 as obviously you
think this is good enough by now, right?
Also, I almost never review RFC patches, we have enough "real" patches
to review as it is :)
thanks,
greg k-h
From: gregkh <[email protected]>
Date: Mon, Feb 17, 2020 at 16:03:45
> On Mon, Feb 17, 2020 at 03:55:08PM +0000, Vitor Soares wrote:
> > Hi,
> >
> > From: Boris Brezillon <[email protected]>
> > Date: Mon, Feb 17, 2020 at 15:36:22
> >
> > > On Mon, 17 Feb 2020 16:06:45 +0100
> > > Arnd Bergmann <[email protected]> wrote:
> > >
> > > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > > <[email protected]> wrote:
> > > > > Sorry for taking so long to reply, and thanks for working on that topic.
> > > > >
> > > > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > > > Vitor Soares <[email protected]> wrote:
> > > > >
> > > > > > For today there is no way to use i3c devices from user space and
> > > > > > the introduction of such API will help developers during the i3c device
> > > > > > or i3c host controllers development.
> > > > > >
> > > > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > > > the concerns raised in [1].
> > > > > >
> > > > > > NOTES:
> > > > > > - The i3cdev dynamically request an unused major number.
> > > > > >
> > > > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > > > > on if they have a device driver bound to it.
> > > > >
> > > > > May I ask why you need to automatically bind devices to the i3cdev
> > > > > driver when they don't have a driver matching the device id
> > > > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > > > uevents we should be able to load the i3cdev module and bind the device
> > > > > to this driver using a udev rule.
> > > >
> > > > I think that would require manual configuration to ensure that the correct
> > > > set of devices get bound to either the userspace driver or an in-kernel
> > > > driver.
> > >
> > > Hm, isn't that what udev is supposed to do anyway? Remember that
> > > I3C devices expose a manufacturer and part-id (which are similar to the
> > > USB vendor and product ids), so deciding when an I3C device should be
> > > bound to the i3cdev driver should be fairly easy, and that's a
> > > per-device decision anyway.
> > >
> > > > The method from the current patch series is more complicated,
> > > > but it means that any device can be accessed by the user space driver
> > > > as long as it's not already owned by a kernel driver.
> > >
> > > Well, I'm more worried about the extra churn this auto-binding logic
> > > might create for the common 'on-demand driver loading' use case. At
> > > first, there's no driver matching a specific device, but userspace
> > > might load one based on the uevents it receives. With the current
> > > approach, that means we'd first have to unbind the device before
> > > loading the driver. AFAICT, no other subsystem does that.
> >
> > I'm about to finish v3 (today or tomorrow) and I think I fixed all
> > concerns rise during v2. I would like you to see that version before any
> > change.
>
> Why are there so many "RFC" series here? I treat "RFC" as "I don't
> really like this patch but I'm throwing it out there for others to look
> at if they care". No RFC should ever go on beyond a v1
My bad ☹.
> as obviously you
> think this is good enough by now, right?
Yes and I really appreciate the feedback that leads me to this v3.
>
> Also, I almost never review RFC patches, we have enough "real" patches
> to review as it is :)
>
> thanks,
>
> greg k-h
Thanks for your advice.
Best regards,
Vitor Soares
On Mon, Feb 17, 2020 at 4:36 PM Boris Brezillon
<[email protected]> wrote:
> On Mon, 17 Feb 2020 16:06:45 +0100 Arnd Bergmann <[email protected]> wrote:
> > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > <[email protected]> wrote:
> > > Sorry for taking so long to reply, and thanks for working on that topic.
> > >
> > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > Vitor Soares <[email protected]> wrote:
> > >
> > > > For today there is no way to use i3c devices from user space and
> > > > the introduction of such API will help developers during the i3c device
> > > > or i3c host controllers development.
> > > >
> > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > the concerns raised in [1].
> > > >
> > > > NOTES:
> > > > - The i3cdev dynamically request an unused major number.
> > > >
> > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > > on if they have a device driver bound to it.
> > >
> > > May I ask why you need to automatically bind devices to the i3cdev
> > > driver when they don't have a driver matching the device id
> > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > uevents we should be able to load the i3cdev module and bind the device
> > > to this driver using a udev rule.
> >
> > I think that would require manual configuration to ensure that the correct
> > set of devices get bound to either the userspace driver or an in-kernel
> > driver.
>
> Hm, isn't that what udev is supposed to do anyway? Remember that
> I3C devices expose a manufacturer and part-id (which are similar to the
> USB vendor and product ids), so deciding when an I3C device should be
> bound to the i3cdev driver should be fairly easy, and that's a
> per-device decision anyway.
>
> > The method from the current patch series is more complicated,
> > but it means that any device can be accessed by the user space driver
> > as long as it's not already owned by a kernel driver.
>
> Well, I'm more worried about the extra churn this auto-binding logic
> might create for the common 'on-demand driver loading' use case. At
> first, there's no driver matching a specific device, but userspace
> might load one based on the uevents it receives. With the current
> approach, that means we'd first have to unbind the device before
> loading the driver. AFAICT, no other subsystem does that.
As I understand it, this is handled by the patches: when a new device
shows up, this triggers the creation of the userspace interface and
also the event that leads to the kernel driver to get loaded. If there
is a kernel driver for the device, that should still load and bind to the
device, at which point the user space interface will go away again.
This may waste CPU cycles for first creating and then destroying
the user space interface, but I don't see how it requires extra work.
If it does require manual configuration or unbinding, that would
indeed be a bad design.
Arnd
On Mon, 17 Feb 2020 15:55:08 +0000
Vitor Soares <[email protected]> wrote:
> Hi,
>
> From: Boris Brezillon <[email protected]>
> Date: Mon, Feb 17, 2020 at 15:36:22
>
> > On Mon, 17 Feb 2020 16:06:45 +0100
> > Arnd Bergmann <[email protected]> wrote:
> >
> > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > <[email protected]> wrote:
> > > > Sorry for taking so long to reply, and thanks for working on that topic.
> > > >
> > > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > > Vitor Soares <[email protected]> wrote:
> > > >
> > > > > For today there is no way to use i3c devices from user space and
> > > > > the introduction of such API will help developers during the i3c device
> > > > > or i3c host controllers development.
> > > > >
> > > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > > the concerns raised in [1].
> > > > >
> > > > > NOTES:
> > > > > - The i3cdev dynamically request an unused major number.
> > > > >
> > > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > > > on if they have a device driver bound to it.
> > > >
> > > > May I ask why you need to automatically bind devices to the i3cdev
> > > > driver when they don't have a driver matching the device id
> > > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > > uevents we should be able to load the i3cdev module and bind the device
> > > > to this driver using a udev rule.
> > >
> > > I think that would require manual configuration to ensure that the correct
> > > set of devices get bound to either the userspace driver or an in-kernel
> > > driver.
> >
> > Hm, isn't that what udev is supposed to do anyway? Remember that
> > I3C devices expose a manufacturer and part-id (which are similar to the
> > USB vendor and product ids), so deciding when an I3C device should be
> > bound to the i3cdev driver should be fairly easy, and that's a
> > per-device decision anyway.
> >
> > > The method from the current patch series is more complicated,
> > > but it means that any device can be accessed by the user space driver
> > > as long as it's not already owned by a kernel driver.
> >
> > Well, I'm more worried about the extra churn this auto-binding logic
> > might create for the common 'on-demand driver loading' use case. At
> > first, there's no driver matching a specific device, but userspace
> > might load one based on the uevents it receives. With the current
> > approach, that means we'd first have to unbind the device before
> > loading the driver. AFAICT, no other subsystem does that.
Okay, I have clearly not read the code carefully enough. I thought you
were declaring a new i3c_device_driver and were manually binding all
orphan devices to this driver. Looks like the solution is more subtle
than that, and i3cdevs are actually subdevices that are automatically
created/removed when the I3C device is unbound/bound. That means the
'on-demand driver loading' logic is not impacted by this new layer. I'm
still not convinced this is needed (I expect i3cdev to be used mostly
for experiment, and in that case, having a udev rule, or manually
binding the device to the i3cdev driver shouldn't be a problem). I'm
also not sure what happens if the device is still used when
i3cdev_detach() is called, can transfers still be done after the device
is attached to its in-kernel driver?
On Mon, Feb 17, 2020 at 5:23 PM Boris Brezillon
<[email protected]> wrote:
> On Mon, 17 Feb 2020 15:55:08 +0000 Vitor Soares <[email protected]> wrote:
>
> Okay, I have clearly not read the code carefully enough. I thought you
> were declaring a new i3c_device_driver and were manually binding all
> orphan devices to this driver. Looks like the solution is more subtle
> than that, and i3cdevs are actually subdevices that are automatically
> created/removed when the I3C device is unbound/bound. That means the
> 'on-demand driver loading' logic is not impacted by this new layer. I'm
> still not convinced this is needed (I expect i3cdev to be used mostly
> for experiment, and in that case, having a udev rule, or manually
> binding the device to the i3cdev driver shouldn't be a problem).
I'm fairly sure it's not needed, other approaches could be used to
provide user space access, but it's not clear if any other way is
better either. It also took me a while to figure out what is going on
when I read the code.
One thought that I had was that this could be better integrated into
the core, with user space being there implicitly through sysfs rather
than a /dev file.
> I'm also not sure what happens if the device is still used when
> i3cdev_detach() is called, can transfers still be done after the device
> is attached to its in-kernel driver?
I think this is still an open issue that I also pointed out. The driver
binding/unbinding and user space access definitely needs to
be properly serialized, whichever method is used to implement the
user access.
Arnd
On Mon, 17 Feb 2020 17:19:57 +0100
Arnd Bergmann <[email protected]> wrote:
> On Mon, Feb 17, 2020 at 4:36 PM Boris Brezillon
> <[email protected]> wrote:
> > On Mon, 17 Feb 2020 16:06:45 +0100 Arnd Bergmann <[email protected]> wrote:
> > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > <[email protected]> wrote:
> > > > Sorry for taking so long to reply, and thanks for working on that topic.
> > > >
> > > > On Wed, 29 Jan 2020 13:17:31 +0100
> > > > Vitor Soares <[email protected]> wrote:
> > > >
> > > > > For today there is no way to use i3c devices from user space and
> > > > > the introduction of such API will help developers during the i3c device
> > > > > or i3c host controllers development.
> > > > >
> > > > > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > > > > the concerns raised in [1].
> > > > >
> > > > > NOTES:
> > > > > - The i3cdev dynamically request an unused major number.
> > > > >
> > > > > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > > > > on if they have a device driver bound to it.
> > > >
> > > > May I ask why you need to automatically bind devices to the i3cdev
> > > > driver when they don't have a driver matching the device id
> > > > loaded/compiled-in? If we get the i3c subsystem to generate proper
> > > > uevents we should be able to load the i3cdev module and bind the device
> > > > to this driver using a udev rule.
> > >
> > > I think that would require manual configuration to ensure that the correct
> > > set of devices get bound to either the userspace driver or an in-kernel
> > > driver.
> >
> > Hm, isn't that what udev is supposed to do anyway? Remember that
> > I3C devices expose a manufacturer and part-id (which are similar to the
> > USB vendor and product ids), so deciding when an I3C device should be
> > bound to the i3cdev driver should be fairly easy, and that's a
> > per-device decision anyway.
> >
> > > The method from the current patch series is more complicated,
> > > but it means that any device can be accessed by the user space driver
> > > as long as it's not already owned by a kernel driver.
> >
> > Well, I'm more worried about the extra churn this auto-binding logic
> > might create for the common 'on-demand driver loading' use case. At
> > first, there's no driver matching a specific device, but userspace
> > might load one based on the uevents it receives. With the current
> > approach, that means we'd first have to unbind the device before
> > loading the driver. AFAICT, no other subsystem does that.
>
> As I understand it, this is handled by the patches: when a new device
> shows up, this triggers the creation of the userspace interface and
> also the event that leads to the kernel driver to get loaded. If there
> is a kernel driver for the device, that should still load and bind to the
> device, at which point the user space interface will go away again.
Yep, that's what I figured after having a closer look at the code.
>
> This may waste CPU cycles for first creating and then destroying
> the user space interface, but I don't see how it requires extra work.
> If it does require manual configuration or unbinding, that would
> indeed be a bad design.
To be honest, I had something less invasive in mind. Something closer
to what spidev provides (a driver that can expose I3C devices to
userspace when explicitly requested). I see now that the USB subsystem
does something similar to what's done here, but I'm wondering if it's
really worth it in the I3C case. As I said in my previous reply, I
expect i3cdev to be used when experimenting or when kernel-space driver
is not an option (licensing/security issues).
On Mon, 17 Feb 2020 17:31:08 +0100
Arnd Bergmann <[email protected]> wrote:
> On Mon, Feb 17, 2020 at 5:23 PM Boris Brezillon
> <[email protected]> wrote:
> > On Mon, 17 Feb 2020 15:55:08 +0000 Vitor Soares <[email protected]> wrote:
> >
> > Okay, I have clearly not read the code carefully enough. I thought you
> > were declaring a new i3c_device_driver and were manually binding all
> > orphan devices to this driver. Looks like the solution is more subtle
> > than that, and i3cdevs are actually subdevices that are automatically
> > created/removed when the I3C device is unbound/bound. That means the
> > 'on-demand driver loading' logic is not impacted by this new layer. I'm
> > still not convinced this is needed (I expect i3cdev to be used mostly
> > for experiment, and in that case, having a udev rule, or manually
> > binding the device to the i3cdev driver shouldn't be a problem).
>
> I'm fairly sure it's not needed, other approaches could be used to
> provide user space access, but it's not clear if any other way is
> better either. It also took me a while to figure out what is going on
> when I read the code.
>
> One thought that I had was that this could be better integrated into
> the core, with user space being there implicitly through sysfs rather
> than a /dev file.
Hm, doing I3C transfers through sysfs sounds a bit odd. sysfs entries
are most of the time exposing human readable/writable stuff (plain text
data, that is).
>
> > I'm also not sure what happens if the device is still used when
> > i3cdev_detach() is called, can transfers still be done after the device
> > is attached to its in-kernel driver?
>
> I think this is still an open issue that I also pointed out. The driver
> binding/unbinding and user space access definitely needs to
> be properly serialized, whichever method is used to implement the
> user access.
Well, going for the spidev approach would solve that and make the
whole implementation more straightforward and less invasive (no
notifier, no need to expose internal device types to this i3cdev
driver, no concurrency issues, ...). We can always revisit this solution
if it proves to be to limited and we feel the need for a
kernel-driven i3cdev auto-binding logic, but I doubt we'll ever be
in that position since udev can handle that for us, and if we start
having actual userspace drivers (which is not the case yet), I expect
distros to add the relevant udev rules to take care of this auto-bind.
On Mon, 17 Feb 2020 15:51:41 +0100
Boris Brezillon <[email protected]> wrote:
> Hello Vitor,
>
> Sorry for taking so long to reply, and thanks for working on that topic.
>
> On Wed, 29 Jan 2020 13:17:31 +0100
> Vitor Soares <[email protected]> wrote:
>
> > For today there is no way to use i3c devices from user space and
> > the introduction of such API will help developers during the i3c device
> > or i3c host controllers development.
> >
> > The i3cdev module is highly based on i2c-dev and yet I tried to address
> > the concerns raised in [1].
> >
> > NOTES:
> > - The i3cdev dynamically request an unused major number.
> >
> > - The i3c devices are dynamically exposed/removed from dev/ folder based
> > on if they have a device driver bound to it.
>
> May I ask why you need to automatically bind devices to the i3cdev
> driver when they don't have a driver matching the device id
> loaded/compiled-in? If we get the i3c subsystem to generate proper
> uevents we should be able to load the i3cdev module and bind the device
> to this driver using a udev rule.
Hm, looks like we already send a proper MODALIAS [1], so we can actually
implement this auto-bind to i3cdev in udev (fall back on i3cdev
load+bind if no matching module is found).
[1]https://elixir.bootlin.com/linux/latest/source/drivers/i3c/master.c#L269