| Message ID | cover.1582069402.git.vitor.soares@synopsys.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce i3c device userspace interface | expand |
Hi Boris, From: Vitor Soares <vitor.soares@synopsys.com> Date: Wed, Feb 19, 2020 at 00:20:38 > 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/bus/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 v3: > Use the xfer_lock to prevent device detach during ioctl call > Expose i3cdev under /dev/bus/i3c/ folder like usb does > Change NOTIFY_BOUND to NOTIFY_BIND, this allows the device detach occur > before driver->probe call > Avoid use of IS_ERR_OR_NULL > Use u64_to_user_ptr instead of (void __user *)(uintptr_t) cast > Allocate k_xfer and data_ptrs at once and eliminate double allocation > check > Pass i3cdev to dev->driver_data > Make all minors available > Add API documentation > > Changes in v2: > Use IDR api for minor numbering > Modify ioctl struct > Fix SPDX license > > Vitor Soares (5): > 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 > userspace-api: add i3cdev documentation > > Documentation/userspace-api/i3c/i3cdev.rst | 116 ++++++++ > 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 +++ > 7 files changed, 616 insertions(+), 1 deletion(-) > create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst > create mode 100644 drivers/i3c/i3cdev.c > create mode 100644 include/uapi/linux/i3c/i3cdev.h > > -- > 2.7.4 I want to make you know that none of your previous comments was ignored and I would like to start the discussion from this point. Best regards, Vitor Soares
On Wed, 19 Feb 2020 00:39:31 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > Hi Boris, > > From: Vitor Soares <vitor.soares@synopsys.com> > Date: Wed, Feb 19, 2020 at 00:20:38 > > > 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/bus/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 v3: > > Use the xfer_lock to prevent device detach during ioctl call > > Expose i3cdev under /dev/bus/i3c/ folder like usb does > > Change NOTIFY_BOUND to NOTIFY_BIND, this allows the device detach occur > > before driver->probe call > > Avoid use of IS_ERR_OR_NULL > > Use u64_to_user_ptr instead of (void __user *)(uintptr_t) cast > > Allocate k_xfer and data_ptrs at once and eliminate double allocation > > check > > Pass i3cdev to dev->driver_data > > Make all minors available > > Add API documentation > > > > Changes in v2: > > Use IDR api for minor numbering > > Modify ioctl struct > > Fix SPDX license > > > > Vitor Soares (5): > > 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 > > userspace-api: add i3cdev documentation > > > > Documentation/userspace-api/i3c/i3cdev.rst | 116 ++++++++ > > 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 +++ > > 7 files changed, 616 insertions(+), 1 deletion(-) > > create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst > > create mode 100644 drivers/i3c/i3cdev.c > > create mode 100644 include/uapi/linux/i3c/i3cdev.h > > > > -- > > 2.7.4 > > I want to make you know that none of your previous comments was ignored > and I would like to start the discussion from this point. Sure, np. I'll probably wait for a v4 exploring the option I proposed then.
Hi Boris, From: Boris Brezillon <boris.brezillon@collabora.com> Date: Wed, Feb 19, 2020 at 08:16:58 > On Wed, 19 Feb 2020 00:39:31 +0000 > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > Hi Boris, > > > > From: Vitor Soares <vitor.soares@synopsys.com> > > Date: Wed, Feb 19, 2020 at 00:20:38 > > > > > 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/bus/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://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2018_11_15_853&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=1N2OHEqhXNvZn7HIjf_EmPX5N9wwBWLsUakJ9a1zno0&s=RJ3Lr0SIvJYiKxLCFaV0_pNArfUJj7TxsJecNgYbTQA&e= > > > [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_vitor-2Dsoares-2Dsnps_i3c-2Dtools.git&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=1N2OHEqhXNvZn7HIjf_EmPX5N9wwBWLsUakJ9a1zno0&s=q4Oi7UD-dZ1YMlTN9prqycS1G1QvEUuBFnsABMGRuBk&e= > > > > > > Changes in v3: > > > Use the xfer_lock to prevent device detach during ioctl call > > > Expose i3cdev under /dev/bus/i3c/ folder like usb does > > > Change NOTIFY_BOUND to NOTIFY_BIND, this allows the device detach occur > > > before driver->probe call > > > Avoid use of IS_ERR_OR_NULL > > > Use u64_to_user_ptr instead of (void __user *)(uintptr_t) cast > > > Allocate k_xfer and data_ptrs at once and eliminate double allocation > > > check > > > Pass i3cdev to dev->driver_data > > > Make all minors available > > > Add API documentation > > > > > > Changes in v2: > > > Use IDR api for minor numbering > > > Modify ioctl struct > > > Fix SPDX license > > > > > > Vitor Soares (5): > > > 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 > > > userspace-api: add i3cdev documentation > > > > > > Documentation/userspace-api/i3c/i3cdev.rst | 116 ++++++++ > > > 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 +++ > > > 7 files changed, 616 insertions(+), 1 deletion(-) > > > create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst > > > create mode 100644 drivers/i3c/i3cdev.c > > > create mode 100644 include/uapi/linux/i3c/i3cdev.h > > > > > > -- > > > 2.7.4 > > > > I want to make you know that none of your previous comments was ignored > > and I would like to start the discussion from this point. > > Sure, np. I'll probably wait for a v4 exploring the option I proposed > then. I would like to check with you: - How can we prioritize the device driver over the i3cdev driver if the driver is loaded after i3cdev? Currently, this is done automatically without any command, and for me, this is a requirement. - For the ioctl command structure, there is no rule about the way I did or what you proposed, both are currently used in the kernel. For me it is one more structure to deal with, can you point the advantages of your purpose? - Regarding the ioctl codes, I tried to use those after I2C. Best regards, Vitor Soares
On Fri, 21 Feb 2020 17:08:17 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > Hi Boris, > > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Wed, Feb 19, 2020 at 08:16:58 > > > On Wed, 19 Feb 2020 00:39:31 +0000 > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > Hi Boris, > > > > > > From: Vitor Soares <vitor.soares@synopsys.com> > > > Date: Wed, Feb 19, 2020 at 00:20:38 > > > > > > > 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/bus/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://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2018_11_15_853&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=1N2OHEqhXNvZn7HIjf_EmPX5N9wwBWLsUakJ9a1zno0&s=RJ3Lr0SIvJYiKxLCFaV0_pNArfUJj7TxsJecNgYbTQA&e= > > > > [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_vitor-2Dsoares-2Dsnps_i3c-2Dtools.git&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=1N2OHEqhXNvZn7HIjf_EmPX5N9wwBWLsUakJ9a1zno0&s=q4Oi7UD-dZ1YMlTN9prqycS1G1QvEUuBFnsABMGRuBk&e= > > > > > > > > Changes in v3: > > > > Use the xfer_lock to prevent device detach during ioctl call > > > > Expose i3cdev under /dev/bus/i3c/ folder like usb does > > > > Change NOTIFY_BOUND to NOTIFY_BIND, this allows the device detach occur > > > > before driver->probe call > > > > Avoid use of IS_ERR_OR_NULL > > > > Use u64_to_user_ptr instead of (void __user *)(uintptr_t) cast > > > > Allocate k_xfer and data_ptrs at once and eliminate double allocation > > > > check > > > > Pass i3cdev to dev->driver_data > > > > Make all minors available > > > > Add API documentation > > > > > > > > Changes in v2: > > > > Use IDR api for minor numbering > > > > Modify ioctl struct > > > > Fix SPDX license > > > > > > > > Vitor Soares (5): > > > > 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 > > > > userspace-api: add i3cdev documentation > > > > > > > > Documentation/userspace-api/i3c/i3cdev.rst | 116 ++++++++ > > > > 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 +++ > > > > 7 files changed, 616 insertions(+), 1 deletion(-) > > > > create mode 100644 Documentation/userspace-api/i3c/i3cdev.rst > > > > create mode 100644 drivers/i3c/i3cdev.c > > > > create mode 100644 include/uapi/linux/i3c/i3cdev.h > > > > > > > > -- > > > > 2.7.4 > > > > > > I want to make you know that none of your previous comments was ignored > > > and I would like to start the discussion from this point. > > > > Sure, np. I'll probably wait for a v4 exploring the option I proposed > > then. > > I would like to check with you: > - How can we prioritize the device driver over the i3cdev driver if the > driver is loaded after i3cdev? Currently, this is done automatically > without any command, and for me, this is a requirement. No devs would be bound to the i3cdev driver by default, it would have to be done explicitly through a sysfs knob. Which makes me realize we can't use the generic bind knob since it doesn't let the subsystem know that it's a manual bind. I thought there was a way to distinguish between manual and auto-bind. > - For the ioctl command structure, there is no rule about the way I did > or what you proposed, both are currently used in the kernel. For me it is > one more structure to deal with, can you point the advantages of your > purpose? I don't have a strong opinion on that one, though I find it a bit easier to follow when the number of xfers is encoded in a separate struct rather than extracted from the data size passed through the cmd argument. > - Regarding the ioctl codes, I tried to use those after I2C. Why start from 0x30? It doesn't make sense to me. Just because you base your code on something that already exists doesn't mean you have to copy all of it.
Hi Boris, From: Boris Brezillon <boris.brezillon@collabora.com> Date: Fri, Feb 21, 2020 at 17:41:16 > > > > > > > > I want to make you know that none of your previous comments was ignored > > > > and I would like to start the discussion from this point. > > > > > > Sure, np. I'll probably wait for a v4 exploring the option I proposed > > > then. > > > > I would like to check with you: > > - How can we prioritize the device driver over the i3cdev driver if the > > driver is loaded after i3cdev? Currently, this is done automatically > > without any command, and for me, this is a requirement. > > No devs would be bound to the i3cdev driver by default, it would have > to be done explicitly through a sysfs knob. Which makes me realize > we can't use the generic bind knob since it doesn't let the subsystem > know that it's a manual bind. I thought there was a way to distinguish > between manual and auto-bind. > > > - For the ioctl command structure, there is no rule about the way I did > > or what you proposed, both are currently used in the kernel. For me it is > > one more structure to deal with, can you point the advantages of your > > purpose? > > I don't have a strong opinion on that one, though I find it a bit > easier to follow when the number of xfers is encoded in a separate > struct rather than extracted from the data size passed through the cmd > argument. I will change it then. Do you have any suggestion for the naming to keep it short? > > > - Regarding the ioctl codes, I tried to use those after I2C. > > Why start from 0x30? It doesn't make sense to me. Just because you base > your code on something that already exists doesn't mean you have to > copy all of it. I might be wrong but last I2C command is 0x20 and I tried to let some free space in case they need. Also I think that make sense I2C and I3C share the same 'magic number'. BTW, in ioctl-numbers documentation there is no reference for code 0x07. Best regards, Vitor Soares
On Mon, 24 Feb 2020 10:53:25 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > Hi Boris, > > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Fri, Feb 21, 2020 at 17:41:16 > > > > > > > > > > > I want to make you know that none of your previous comments was ignored > > > > > and I would like to start the discussion from this point. > > > > > > > > Sure, np. I'll probably wait for a v4 exploring the option I proposed > > > > then. > > > > > > I would like to check with you: > > > - How can we prioritize the device driver over the i3cdev driver if the > > > driver is loaded after i3cdev? Currently, this is done automatically > > > without any command, and for me, this is a requirement. > > > > No devs would be bound to the i3cdev driver by default, it would have > > to be done explicitly through a sysfs knob. Which makes me realize > > we can't use the generic bind knob since it doesn't let the subsystem > > know that it's a manual bind. I thought there was a way to distinguish > > between manual and auto-bind. > > > > > - For the ioctl command structure, there is no rule about the way I did > > > or what you proposed, both are currently used in the kernel. For me it is > > > one more structure to deal with, can you point the advantages of your > > > purpose? > > > > I don't have a strong opinion on that one, though I find it a bit > > easier to follow when the number of xfers is encoded in a separate > > struct rather than extracted from the data size passed through the cmd > > argument. > > I will change it then. Do you have any suggestion for the naming to keep > it short? I named it i3cdev_priv_xfers in the patch I sent, but you can pick a different name if you don't like this one. > > > > > > - Regarding the ioctl codes, I tried to use those after I2C. > > > > Why start from 0x30? It doesn't make sense to me. Just because you base > > your code on something that already exists doesn't mean you have to > > copy all of it. > > I might be wrong but last I2C command is 0x20 and I tried to let some > free space in case they need. > Also I think that make sense I2C and I3C share the same 'magic number'. Hm, I'm not sure that's a good idea. The set of ioctls for I2C and I3C are likely to be completely different, so I'd recommend using a separate namespace (AKA ioctl magic number). Wolfram, any opinion? > > BTW, in ioctl-numbers documentation there is no reference for code 0x07. Indeed, looks like it's not documented.