[RFC,v2,0/4] Introduce i3c device userspace interface

Message

Vitor Soares Jan. 29, 2020, 12:17 p.m. UTC

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

Comments

Boris Brezillon Feb. 17, 2020, 2:51 p.m. UTC | #1

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 <[e-mail]> 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

Arnd Bergmann Feb. 17, 2020, 3:06 p.m. UTC | #2

On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
<[e-mail]> 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 <[e-mail]> 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

Vitor Soares Feb. 17, 2020, 3:32 p.m. UTC | #3

Hi Boris,

From: Boris Brezillon <[e-mail]>
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 <[e-mail]> 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

Boris Brezillon Feb. 17, 2020, 3:36 p.m. UTC | #4

On Mon, 17 Feb 2020 16:06:45 +0100
Arnd Bergmann <[e-mail]> wrote:

> On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> <[e-mail]> 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 <[e-mail]> 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.

Boris Brezillon Feb. 17, 2020, 3:52 p.m. UTC | #5

On Mon, 17 Feb 2020 15:32:55 +0000
Vitor Soares <[e-mail]> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <[e-mail]>
> 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 <[e-mail]> 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.

Vitor Soares Feb. 17, 2020, 3:55 p.m. UTC | #6

Hi,

From: Boris Brezillon <[e-mail]>
Date: Mon, Feb 17, 2020 at 15:36:22

> On Mon, 17 Feb 2020 16:06:45 +0100
> Arnd Bergmann <[e-mail]> wrote:
> 
> > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > <[e-mail]> 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 <[e-mail]> 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

Greg Kroah-Hartman Feb. 17, 2020, 4:03 p.m. UTC | #7

On Mon, Feb 17, 2020 at 03:55:08PM +0000, Vitor Soares wrote:
> Hi,
> 
> From: Boris Brezillon <[e-mail]>
> Date: Mon, Feb 17, 2020 at 15:36:22
> 
> > On Mon, 17 Feb 2020 16:06:45 +0100
> > Arnd Bergmann <[e-mail]> wrote:
> > 
> > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > <[e-mail]> 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 <[e-mail]> 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

Vitor Soares Feb. 17, 2020, 4:12 p.m. UTC | #8

From: gregkh <[e-mail]>
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 <[e-mail]>
> > Date: Mon, Feb 17, 2020 at 15:36:22
> > 
> > > On Mon, 17 Feb 2020 16:06:45 +0100
> > > Arnd Bergmann <[e-mail]> wrote:
> > > 
> > > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > > <[e-mail]> 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 <[e-mail]> 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

Arnd Bergmann Feb. 17, 2020, 4:19 p.m. UTC | #9

On Mon, Feb 17, 2020 at 4:36 PM Boris Brezillon
<[e-mail]> wrote:
> On Mon, 17 Feb 2020 16:06:45 +0100 Arnd Bergmann <[e-mail]> wrote:
> > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > <[e-mail]> 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 <[e-mail]> 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

Boris Brezillon Feb. 17, 2020, 4:23 p.m. UTC | #10

On Mon, 17 Feb 2020 15:55:08 +0000
Vitor Soares <[e-mail]> wrote:

> Hi,
> 
> From: Boris Brezillon <[e-mail]>
> Date: Mon, Feb 17, 2020 at 15:36:22
> 
> > On Mon, 17 Feb 2020 16:06:45 +0100
> > Arnd Bergmann <[e-mail]> wrote:
> >   
> > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > <[e-mail]> 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 <[e-mail]> 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?

Arnd Bergmann Feb. 17, 2020, 4:31 p.m. UTC | #11

On Mon, Feb 17, 2020 at 5:23 PM Boris Brezillon
<[e-mail]> wrote:
> On Mon, 17 Feb 2020 15:55:08 +0000 Vitor Soares <[e-mail]> 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

Boris Brezillon Feb. 17, 2020, 4:34 p.m. UTC | #12

On Mon, 17 Feb 2020 17:19:57 +0100
Arnd Bergmann <[e-mail]> wrote:

> On Mon, Feb 17, 2020 at 4:36 PM Boris Brezillon
> <[e-mail]> wrote:
> > On Mon, 17 Feb 2020 16:06:45 +0100 Arnd Bergmann <[e-mail]> wrote:  
> > > On Mon, Feb 17, 2020 at 3:51 PM Boris Brezillon
> > > <[e-mail]> 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 <[e-mail]> 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).

Boris Brezillon Feb. 17, 2020, 5:06 p.m. UTC | #13

On Mon, 17 Feb 2020 17:31:08 +0100
Arnd Bergmann <[e-mail]> wrote:

> On Mon, Feb 17, 2020 at 5:23 PM Boris Brezillon
> <[e-mail]> wrote:
> > On Mon, 17 Feb 2020 15:55:08 +0000 Vitor Soares <[e-mail]> 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.

Boris Brezillon Feb. 17, 2020, 5:37 p.m. UTC | #14

On Mon, 17 Feb 2020 15:51:41 +0100
Boris Brezillon <[e-mail]> 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 <[e-mail]> 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