Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Mon, Oct 05, 2020 at 01:58:19PM +0200, Thierry Reding wrote: > On Mon, Oct 05, 2020 at 01:17:38PM +0200, Greg Kroah-Hartman wrote: > > On Mon, Oct 05, 2020 at 01:08:19PM +0200, Thierry Reding wrote: > > > On Mon, Oct 05, 2020 at 12:40:23PM +0200, Greg Kroah-Hartman wrote: > > > > On Mon, Oct 05, 2020 at 12:17:21PM +0200, Thierry Reding wrote: > > > > > On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote: > > > > > > On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote: > > > > > > > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman > > > > > > > wrote: > > > > > > > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote: > > > > > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König > > > > > > > > > wrote: > > > > > > > > > > Hello, > > > > > > > > > > > > > > > > > > > > I added Greg Kroah-Hartman who I discussed this with via > > > > > > > > > > irc a bit to > > > > > > > > > > Cc:. > > > > > > > > > > > > > > > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel > > > > > > > > > > wrote: > > > > > > > > > > > thank you for your review! > > > > > > > > > > > > > > > > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe > > > > > > > > > > > Kleine-König wrote: > > > > > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, > > > > > > > > > > > > poesc...@lemonage.de wrote: > > > > > > > > > > > > > From: Lars Poeschel > > > > > > > > > > > > > > > > > > > > > > > > > > This adds a class to exported pwm devices. > > > > > > > > > > > > > Exporting a pwm through sysfs did not yield udev > > > > > > > > > > > > > events. The > > > > > > > > > > > > > > > > > > > > > > > > I wonder what is your use-case here. This for sure also > > > > > > > > > > > > has a place to > > > > > > > > > > > > be mentioned in the commit log. I suspect there is a > > > > > > > > > > > > better way to > > > > > > > > > > > > accomplish you way. > > > > > > > > > > > > > > > > > > > > > > Use-case is to be able to use a pwm from a non-root > > > > > > > > > > > userspace process. > > > > > > > > > > > I use udev rules to adjust permissions. > > > > > > > > > > > > > > > > > > > > Hmm, how do you trigger the export? Without being aware of > > > > > > > > > > all the > > > > > > > > > > details in the sysfs code I would expect that the exported > > > > > > > > > > stuff is > > > > > > > > > > available instantly once the write used to export the PWM > > > > > > > > > > is completed. > > > > > > > > > > So changing the permissions can be done directly after > > > > > > > > > > triggering the > > > > > > > > > > export in the same process. > > > > > > > > > > > > > > > > > > The export is triggered through the userspace process itself. > > > > > > > > > Why can it > > > > > > > > > do this ? Because there is another udev rule, that changes > > > > > > > > > permissions > > > > > > > > > when a pwmchip appears. > > > > > > > > > Then I'd like to have the second udev rule, that changes > > > > > > > > > permissions on > > > > > > > > > the freshly exported pwm. The userspace process can't do this. > > > > > > > > > You are right I could propably do everything from within > > > > > > > > > udev: If a > > > > > > > > > pwmchip appears, export certain pwms and right away change > > > > > > > > > their > > > > > > > > > permissions. It does not also not feel right. It'd require > > > > > > > > > knowledge > > > > > > > > > from the userspace application to be mapped to udev. > > > > > > > > > > > > > > > > The way the kernel code is now, yes, you will not have any way > > > > > > > > to > > > > > > > > trigger it by userspace as the kernel is creating a "raw" > > > > > > > > struct device > > > > > > > > that isn't assigned to anything. That is what needs to be > > > > > > > > fixed here. > > > > > > > > > > > > > > > > > > Out of interest: What do you use the pwm for? Isn't there a > > > > > > > > > > suitable > > > > > > > > > > kernel driver that can do the required stuff? Compared to > > > > > > > > > > the kernel-API > > > > > > > > > > the sysfs interface isn't atomic. Is this an annoyance? > > > > > > > > > > > > > > > > > > Use-case is generating a voltage from the pwm. This voltage > > > > > > > > > is used to > > > > > > > > > signal different states and does not change very often. This > > > > > > > > > is > > > > > > > > > absolutely not annoying that this is not atomic. We just > > > > > > > > > change the duty > > > > > > > > > cycle on the fly. Everything else is configured one time at > > > > > > > > > startup. > > > > > > > > > I'd call what I need pwm-dac. I could not find a ready to use > > > > > > > > > driver. > > > > > > > > > Maybe I could misuse some kernel driver for this. Maybe I > > > > > > > > > could use > > > > > > > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator > > > > > > > > > could work, > > > > > > > > > there is even a userspace facing part for this, but
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Mon, Oct 05, 2020 at 01:17:38PM +0200, Greg Kroah-Hartman wrote: > On Mon, Oct 05, 2020 at 01:08:19PM +0200, Thierry Reding wrote: > > On Mon, Oct 05, 2020 at 12:40:23PM +0200, Greg Kroah-Hartman wrote: > > > On Mon, Oct 05, 2020 at 12:17:21PM +0200, Thierry Reding wrote: > > > > On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote: > > > > > On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote: > > > > > > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote: > > > > > > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote: > > > > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König > > > > > > > > wrote: > > > > > > > > > Hello, > > > > > > > > > > > > > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc > > > > > > > > > a bit to > > > > > > > > > Cc:. > > > > > > > > > > > > > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote: > > > > > > > > > > thank you for your review! > > > > > > > > > > > > > > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König > > > > > > > > > > wrote: > > > > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, > > > > > > > > > > > poesc...@lemonage.de wrote: > > > > > > > > > > > > From: Lars Poeschel > > > > > > > > > > > > > > > > > > > > > > > > This adds a class to exported pwm devices. > > > > > > > > > > > > Exporting a pwm through sysfs did not yield udev > > > > > > > > > > > > events. The > > > > > > > > > > > > > > > > > > > > > > I wonder what is your use-case here. This for sure also > > > > > > > > > > > has a place to > > > > > > > > > > > be mentioned in the commit log. I suspect there is a > > > > > > > > > > > better way to > > > > > > > > > > > accomplish you way. > > > > > > > > > > > > > > > > > > > > Use-case is to be able to use a pwm from a non-root > > > > > > > > > > userspace process. > > > > > > > > > > I use udev rules to adjust permissions. > > > > > > > > > > > > > > > > > > Hmm, how do you trigger the export? Without being aware of > > > > > > > > > all the > > > > > > > > > details in the sysfs code I would expect that the exported > > > > > > > > > stuff is > > > > > > > > > available instantly once the write used to export the PWM is > > > > > > > > > completed. > > > > > > > > > So changing the permissions can be done directly after > > > > > > > > > triggering the > > > > > > > > > export in the same process. > > > > > > > > > > > > > > > > The export is triggered through the userspace process itself. > > > > > > > > Why can it > > > > > > > > do this ? Because there is another udev rule, that changes > > > > > > > > permissions > > > > > > > > when a pwmchip appears. > > > > > > > > Then I'd like to have the second udev rule, that changes > > > > > > > > permissions on > > > > > > > > the freshly exported pwm. The userspace process can't do this. > > > > > > > > You are right I could propably do everything from within udev: > > > > > > > > If a > > > > > > > > pwmchip appears, export certain pwms and right away change their > > > > > > > > permissions. It does not also not feel right. It'd require > > > > > > > > knowledge > > > > > > > > from the userspace application to be mapped to udev. > > > > > > > > > > > > > > The way the kernel code is now, yes, you will not have any way to > > > > > > > trigger it by userspace as the kernel is creating a "raw" struct > > > > > > > device > > > > > > > that isn't assigned to anything. That is what needs to be fixed > > > > > > > here. > > > > > > > > > > > > > > > > Out of interest: What do you use the pwm for? Isn't there a > > > > > > > > > suitable > > > > > > > > > kernel driver that can do the required stuff? Compared to the > > > > > > > > > kernel-API > > > > > > > > > the sysfs interface isn't atomic. Is this an annoyance? > > > > > > > > > > > > > > > > Use-case is generating a voltage from the pwm. This voltage is > > > > > > > > used to > > > > > > > > signal different states and does not change very often. This is > > > > > > > > absolutely not annoying that this is not atomic. We just change > > > > > > > > the duty > > > > > > > > cycle on the fly. Everything else is configured one time at > > > > > > > > startup. > > > > > > > > I'd call what I need pwm-dac. I could not find a ready to use > > > > > > > > driver. > > > > > > > > Maybe I could misuse some kernel driver for this. Maybe I could > > > > > > > > use > > > > > > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator > > > > > > > > could work, > > > > > > > > there is even a userspace facing part for this, but this is not > > > > > > > > devicetree ready. > > > > > > > > ...and the worst, please don't blame me: The application is > > > > > > > > java, so > > > > > > > > ioctl is a problem. > > > > > > > > > > > > > > I thought java could do ioctls, otherwise how would it ever be > > > > > > > able to > > > > > > > talk to serial ports? >
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Mon, Oct 05, 2020 at 01:08:19PM +0200, Thierry Reding wrote: > On Mon, Oct 05, 2020 at 12:40:23PM +0200, Greg Kroah-Hartman wrote: > > On Mon, Oct 05, 2020 at 12:17:21PM +0200, Thierry Reding wrote: > > > On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote: > > > > On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote: > > > > > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote: > > > > > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote: > > > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote: > > > > > > > > Hello, > > > > > > > > > > > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a > > > > > > > > bit to > > > > > > > > Cc:. > > > > > > > > > > > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote: > > > > > > > > > thank you for your review! > > > > > > > > > > > > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König > > > > > > > > > wrote: > > > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, > > > > > > > > > > poesc...@lemonage.de wrote: > > > > > > > > > > > From: Lars Poeschel > > > > > > > > > > > > > > > > > > > > > > This adds a class to exported pwm devices. > > > > > > > > > > > Exporting a pwm through sysfs did not yield udev events. > > > > > > > > > > > The > > > > > > > > > > > > > > > > > > > > I wonder what is your use-case here. This for sure also has > > > > > > > > > > a place to > > > > > > > > > > be mentioned in the commit log. I suspect there is a better > > > > > > > > > > way to > > > > > > > > > > accomplish you way. > > > > > > > > > > > > > > > > > > Use-case is to be able to use a pwm from a non-root userspace > > > > > > > > > process. > > > > > > > > > I use udev rules to adjust permissions. > > > > > > > > > > > > > > > > Hmm, how do you trigger the export? Without being aware of all > > > > > > > > the > > > > > > > > details in the sysfs code I would expect that the exported > > > > > > > > stuff is > > > > > > > > available instantly once the write used to export the PWM is > > > > > > > > completed. > > > > > > > > So changing the permissions can be done directly after > > > > > > > > triggering the > > > > > > > > export in the same process. > > > > > > > > > > > > > > The export is triggered through the userspace process itself. Why > > > > > > > can it > > > > > > > do this ? Because there is another udev rule, that changes > > > > > > > permissions > > > > > > > when a pwmchip appears. > > > > > > > Then I'd like to have the second udev rule, that changes > > > > > > > permissions on > > > > > > > the freshly exported pwm. The userspace process can't do this. > > > > > > > You are right I could propably do everything from within udev: If > > > > > > > a > > > > > > > pwmchip appears, export certain pwms and right away change their > > > > > > > permissions. It does not also not feel right. It'd require > > > > > > > knowledge > > > > > > > from the userspace application to be mapped to udev. > > > > > > > > > > > > The way the kernel code is now, yes, you will not have any way to > > > > > > trigger it by userspace as the kernel is creating a "raw" struct > > > > > > device > > > > > > that isn't assigned to anything. That is what needs to be fixed > > > > > > here. > > > > > > > > > > > > > > Out of interest: What do you use the pwm for? Isn't there a > > > > > > > > suitable > > > > > > > > kernel driver that can do the required stuff? Compared to the > > > > > > > > kernel-API > > > > > > > > the sysfs interface isn't atomic. Is this an annoyance? > > > > > > > > > > > > > > Use-case is generating a voltage from the pwm. This voltage is > > > > > > > used to > > > > > > > signal different states and does not change very often. This is > > > > > > > absolutely not annoying that this is not atomic. We just change > > > > > > > the duty > > > > > > > cycle on the fly. Everything else is configured one time at > > > > > > > startup. > > > > > > > I'd call what I need pwm-dac. I could not find a ready to use > > > > > > > driver. > > > > > > > Maybe I could misuse some kernel driver for this. Maybe I could > > > > > > > use > > > > > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator > > > > > > > could work, > > > > > > > there is even a userspace facing part for this, but this is not > > > > > > > devicetree ready. > > > > > > > ...and the worst, please don't blame me: The application is java, > > > > > > > so > > > > > > > ioctl is a problem. > > > > > > > > > > > > I thought java could do ioctls, otherwise how would it ever be able > > > > > > to > > > > > > talk to serial ports? > > > > > > > > > > > > Anyway, this needs to be fixed in the kernel... > > > > > > > > > > If atomicity was a problem, we could potentially add a mechanism to > > > > > the > > > > > sysfs interface to enable that. I don't see a good way of doing that > > > > > in > > > > >
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Mon, Oct 05, 2020 at 12:40:23PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 05, 2020 at 12:17:21PM +0200, Thierry Reding wrote:
> > On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> > > > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > > Hello,
> > > > > > >
> > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a
> > > > > > > bit to
> > > > > > > Cc:.
> > > > > > >
> > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > > thank you for your review!
> > > > > > > >
> > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König
> > > > > > > > wrote:
> > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200,
> > > > > > > > > poesc...@lemonage.de wrote:
> > > > > > > > > > From: Lars Poeschel
> > > > > > > > > >
> > > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > > >
> > > > > > > > > I wonder what is your use-case here. This for sure also has a
> > > > > > > > > place to
> > > > > > > > > be mentioned in the commit log. I suspect there is a better
> > > > > > > > > way to
> > > > > > > > > accomplish you way.
> > > > > > > >
> > > > > > > > Use-case is to be able to use a pwm from a non-root userspace
> > > > > > > > process.
> > > > > > > > I use udev rules to adjust permissions.
> > > > > > >
> > > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > > details in the sysfs code I would expect that the exported stuff
> > > > > > > is
> > > > > > > available instantly once the write used to export the PWM is
> > > > > > > completed.
> > > > > > > So changing the permissions can be done directly after triggering
> > > > > > > the
> > > > > > > export in the same process.
> > > > > >
> > > > > > The export is triggered through the userspace process itself. Why
> > > > > > can it
> > > > > > do this ? Because there is another udev rule, that changes
> > > > > > permissions
> > > > > > when a pwmchip appears.
> > > > > > Then I'd like to have the second udev rule, that changes
> > > > > > permissions on
> > > > > > the freshly exported pwm. The userspace process can't do this.
> > > > > > You are right I could propably do everything from within udev: If a
> > > > > > pwmchip appears, export certain pwms and right away change their
> > > > > > permissions. It does not also not feel right. It'd require knowledge
> > > > > > from the userspace application to be mapped to udev.
> > > > >
> > > > > The way the kernel code is now, yes, you will not have any way to
> > > > > trigger it by userspace as the kernel is creating a "raw" struct
> > > > > device
> > > > > that isn't assigned to anything. That is what needs to be fixed here.
> > > > >
> > > > > > > Out of interest: What do you use the pwm for? Isn't there a
> > > > > > > suitable
> > > > > > > kernel driver that can do the required stuff? Compared to the
> > > > > > > kernel-API
> > > > > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > > > >
> > > > > > Use-case is generating a voltage from the pwm. This voltage is used
> > > > > > to
> > > > > > signal different states and does not change very often. This is
> > > > > > absolutely not annoying that this is not atomic. We just change the
> > > > > > duty
> > > > > > cycle on the fly. Everything else is configured one time at startup.
> > > > > > I'd call what I need pwm-dac. I could not find a ready to use
> > > > > > driver.
> > > > > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could
> > > > > > work,
> > > > > > there is even a userspace facing part for this, but this is not
> > > > > > devicetree ready.
> > > > > > ...and the worst, please don't blame me: The application is java, so
> > > > > > ioctl is a problem.
> > > > >
> > > > > I thought java could do ioctls, otherwise how would it ever be able to
> > > > > talk to serial ports?
> > > > >
> > > > > Anyway, this needs to be fixed in the kernel...
> > > >
> > > > If atomicity was a problem, we could potentially add a mechanism to the
> > > > sysfs interface to enable that. I don't see a good way of doing that in
> > > > a single file, since that works against how sysfs is designed. But one
> > > > thing I could imagine is adding a file ("lock", or whatever you want to
> > > > call it) that you can use for atomic fencing:
> > > >
> > > > $ echo 1 > lock # locks the hardware state
> > > > $ echo 100 > period
> > > > $ echo 50 > duty_cycle
> > > > $ echo 0 > lock # flushes state to hardware
>
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > Hello,
> > > >
> > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > Cc:.
> > > >
> > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > thank you for your review!
> > > > >
> > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de
> > > > > > wrote:
> > > > > > > From: Lars Poeschel
> > > > > > >
> > > > > > > This adds a class to exported pwm devices.
> > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > >
> > > > > > I wonder what is your use-case here. This for sure also has a place
> > > > > > to
> > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > accomplish you way.
> > > > >
> > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > I use udev rules to adjust permissions.
> > > >
> > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > details in the sysfs code I would expect that the exported stuff is
> > > > available instantly once the write used to export the PWM is completed.
> > > > So changing the permissions can be done directly after triggering the
> > > > export in the same process.
> > >
> > > The export is triggered through the userspace process itself. Why can it
> > > do this ? Because there is another udev rule, that changes permissions
> > > when a pwmchip appears.
> > > Then I'd like to have the second udev rule, that changes permissions on
> > > the freshly exported pwm. The userspace process can't do this.
> > > You are right I could propably do everything from within udev: If a
> > > pwmchip appears, export certain pwms and right away change their
> > > permissions. It does not also not feel right. It'd require knowledge
> > > from the userspace application to be mapped to udev.
> >
> > The way the kernel code is now, yes, you will not have any way to
> > trigger it by userspace as the kernel is creating a "raw" struct device
> > that isn't assigned to anything. That is what needs to be fixed here.
> >
> > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > > the sysfs interface isn't atomic. Is this an annoyance?
> > >
> > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > signal different states and does not change very often. This is
> > > absolutely not annoying that this is not atomic. We just change the duty
> > > cycle on the fly. Everything else is configured one time at startup.
> > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > there is even a userspace facing part for this, but this is not
> > > devicetree ready.
> > > ...and the worst, please don't blame me: The application is java, so
> > > ioctl is a problem.
> >
> > I thought java could do ioctls, otherwise how would it ever be able to
> > talk to serial ports?
> >
> > Anyway, this needs to be fixed in the kernel...
>
> If atomicity was a problem, we could potentially add a mechanism to the
> sysfs interface to enable that. I don't see a good way of doing that in
> a single file, since that works against how sysfs is designed. But one
> thing I could imagine is adding a file ("lock", or whatever you want to
> call it) that you can use for atomic fencing:
>
> $ echo 1 > lock # locks the hardware state
> $ echo 100 > period
> $ echo 50 > duty_cycle
> $ echo 0 > lock # flushes state to hardware
>
> But it sounds like that's not even a big issue.
For my use case atomicity is absolutely not a problem, but of course
things should be solved in a generic way.
> However, given the use-case description, it sounds to me like
> pwm-regulator would be a better candidate to solve this, because it's
> purpose is literally to generate a voltage using a PWM. There is a
> device tree binding for pwm-regulator, so this should work there as
> well.
>
> Lars, what exactly are the problems that you're running into when trying
> to use pwm-regulator with device tree?
The problem is not to use pwm-regulator from device tree. But I have no
userspace facing part then. pwm-regulator is for kernel drivers only (as
far as I can see). I can not do anything with it from userspace.
I found that "userspace-consumer" solves this, but this thing does not
have a device tree binding. I could add this of course, but pwm sysfs
was there ready
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Mon, Oct 05, 2020 at 12:17:21PM +0200, Thierry Reding wrote:
> On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> > > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > Hello,
> > > > > >
> > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit
> > > > > > to
> > > > > > Cc:.
> > > > > >
> > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > thank you for your review!
> > > > > > >
> > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de
> > > > > > > > wrote:
> > > > > > > > > From: Lars Poeschel
> > > > > > > > >
> > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > >
> > > > > > > > I wonder what is your use-case here. This for sure also has a
> > > > > > > > place to
> > > > > > > > be mentioned in the commit log. I suspect there is a better way
> > > > > > > > to
> > > > > > > > accomplish you way.
> > > > > > >
> > > > > > > Use-case is to be able to use a pwm from a non-root userspace
> > > > > > > process.
> > > > > > > I use udev rules to adjust permissions.
> > > > > >
> > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > > available instantly once the write used to export the PWM is
> > > > > > completed.
> > > > > > So changing the permissions can be done directly after triggering
> > > > > > the
> > > > > > export in the same process.
> > > > >
> > > > > The export is triggered through the userspace process itself. Why can
> > > > > it
> > > > > do this ? Because there is another udev rule, that changes permissions
> > > > > when a pwmchip appears.
> > > > > Then I'd like to have the second udev rule, that changes permissions
> > > > > on
> > > > > the freshly exported pwm. The userspace process can't do this.
> > > > > You are right I could propably do everything from within udev: If a
> > > > > pwmchip appears, export certain pwms and right away change their
> > > > > permissions. It does not also not feel right. It'd require knowledge
> > > > > from the userspace application to be mapped to udev.
> > > >
> > > > The way the kernel code is now, yes, you will not have any way to
> > > > trigger it by userspace as the kernel is creating a "raw" struct device
> > > > that isn't assigned to anything. That is what needs to be fixed here.
> > > >
> > > > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > > > kernel driver that can do the required stuff? Compared to the
> > > > > > kernel-API
> > > > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > > >
> > > > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > > > signal different states and does not change very often. This is
> > > > > absolutely not annoying that this is not atomic. We just change the
> > > > > duty
> > > > > cycle on the fly. Everything else is configured one time at startup.
> > > > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could
> > > > > work,
> > > > > there is even a userspace facing part for this, but this is not
> > > > > devicetree ready.
> > > > > ...and the worst, please don't blame me: The application is java, so
> > > > > ioctl is a problem.
> > > >
> > > > I thought java could do ioctls, otherwise how would it ever be able to
> > > > talk to serial ports?
> > > >
> > > > Anyway, this needs to be fixed in the kernel...
> > >
> > > If atomicity was a problem, we could potentially add a mechanism to the
> > > sysfs interface to enable that. I don't see a good way of doing that in
> > > a single file, since that works against how sysfs is designed. But one
> > > thing I could imagine is adding a file ("lock", or whatever you want to
> > > call it) that you can use for atomic fencing:
> > >
> > > $ echo 1 > lock # locks the hardware state
> > > $ echo 100 > period
> > > $ echo 50 > duty_cycle
> > > $ echo 0 > lock # flushes state to hardware
> > >
> > > But it sounds like that's not even a big issue.
> >
> > That is exactly what configfs was designed for :)
>
> Interesting... for some reason I had never thought about configfs in
> this context. But it does indeed sound like it could solve this problem
> in a better way.
>
> My memory is a bit sketchy, but I think for USB device controllers this
> works
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Mon, Oct 05, 2020 at 11:45:30AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > Hello,
> > > > >
> > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > Cc:.
> > > > >
> > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > thank you for your review!
> > > > > >
> > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de
> > > > > > > wrote:
> > > > > > > > From: Lars Poeschel
> > > > > > > >
> > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > >
> > > > > > > I wonder what is your use-case here. This for sure also has a
> > > > > > > place to
> > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > accomplish you way.
> > > > > >
> > > > > > Use-case is to be able to use a pwm from a non-root userspace
> > > > > > process.
> > > > > > I use udev rules to adjust permissions.
> > > > >
> > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > available instantly once the write used to export the PWM is
> > > > > completed.
> > > > > So changing the permissions can be done directly after triggering the
> > > > > export in the same process.
> > > >
> > > > The export is triggered through the userspace process itself. Why can it
> > > > do this ? Because there is another udev rule, that changes permissions
> > > > when a pwmchip appears.
> > > > Then I'd like to have the second udev rule, that changes permissions on
> > > > the freshly exported pwm. The userspace process can't do this.
> > > > You are right I could propably do everything from within udev: If a
> > > > pwmchip appears, export certain pwms and right away change their
> > > > permissions. It does not also not feel right. It'd require knowledge
> > > > from the userspace application to be mapped to udev.
> > >
> > > The way the kernel code is now, yes, you will not have any way to
> > > trigger it by userspace as the kernel is creating a "raw" struct device
> > > that isn't assigned to anything. That is what needs to be fixed here.
> > >
> > > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > > kernel driver that can do the required stuff? Compared to the
> > > > > kernel-API
> > > > > the sysfs interface isn't atomic. Is this an annoyance?
> > > >
> > > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > > signal different states and does not change very often. This is
> > > > absolutely not annoying that this is not atomic. We just change the duty
> > > > cycle on the fly. Everything else is configured one time at startup.
> > > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > > there is even a userspace facing part for this, but this is not
> > > > devicetree ready.
> > > > ...and the worst, please don't blame me: The application is java, so
> > > > ioctl is a problem.
> > >
> > > I thought java could do ioctls, otherwise how would it ever be able to
> > > talk to serial ports?
> > >
> > > Anyway, this needs to be fixed in the kernel...
> >
> > If atomicity was a problem, we could potentially add a mechanism to the
> > sysfs interface to enable that. I don't see a good way of doing that in
> > a single file, since that works against how sysfs is designed. But one
> > thing I could imagine is adding a file ("lock", or whatever you want to
> > call it) that you can use for atomic fencing:
> >
> > $ echo 1 > lock # locks the hardware state
> > $ echo 100 > period
> > $ echo 50 > duty_cycle
> > $ echo 0 > lock # flushes state to hardware
> >
> > But it sounds like that's not even a big issue.
>
> That is exactly what configfs was designed for :)
Interesting... for some reason I had never thought about configfs in
this context. But it does indeed sound like it could solve this problem
in a better way.
My memory is a bit sketchy, but I think for USB device controllers this
works by exposing each controller in configfs and then configuring
things like endpoints within the controller's directory. So I wonder if
instead of requesting PWMs via sysfs, we should rather expose them via
configfs items.
Something like:
# mkdir /configfs/7000a000.pwm/0
could then be the equivalent of exporting PWM
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Mon, Oct 05, 2020 at 11:30:16AM +0200, Thierry Reding wrote:
> On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > Hello,
> > > >
> > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > Cc:.
> > > >
> > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > thank you for your review!
> > > > >
> > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de
> > > > > > wrote:
> > > > > > > From: Lars Poeschel
> > > > > > >
> > > > > > > This adds a class to exported pwm devices.
> > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > >
> > > > > > I wonder what is your use-case here. This for sure also has a place
> > > > > > to
> > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > accomplish you way.
> > > > >
> > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > I use udev rules to adjust permissions.
> > > >
> > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > details in the sysfs code I would expect that the exported stuff is
> > > > available instantly once the write used to export the PWM is completed.
> > > > So changing the permissions can be done directly after triggering the
> > > > export in the same process.
> > >
> > > The export is triggered through the userspace process itself. Why can it
> > > do this ? Because there is another udev rule, that changes permissions
> > > when a pwmchip appears.
> > > Then I'd like to have the second udev rule, that changes permissions on
> > > the freshly exported pwm. The userspace process can't do this.
> > > You are right I could propably do everything from within udev: If a
> > > pwmchip appears, export certain pwms and right away change their
> > > permissions. It does not also not feel right. It'd require knowledge
> > > from the userspace application to be mapped to udev.
> >
> > The way the kernel code is now, yes, you will not have any way to
> > trigger it by userspace as the kernel is creating a "raw" struct device
> > that isn't assigned to anything. That is what needs to be fixed here.
> >
> > > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > > the sysfs interface isn't atomic. Is this an annoyance?
> > >
> > > Use-case is generating a voltage from the pwm. This voltage is used to
> > > signal different states and does not change very often. This is
> > > absolutely not annoying that this is not atomic. We just change the duty
> > > cycle on the fly. Everything else is configured one time at startup.
> > > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > > Maybe I could misuse some kernel driver for this. Maybe I could use
> > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > > there is even a userspace facing part for this, but this is not
> > > devicetree ready.
> > > ...and the worst, please don't blame me: The application is java, so
> > > ioctl is a problem.
> >
> > I thought java could do ioctls, otherwise how would it ever be able to
> > talk to serial ports?
> >
> > Anyway, this needs to be fixed in the kernel...
>
> If atomicity was a problem, we could potentially add a mechanism to the
> sysfs interface to enable that. I don't see a good way of doing that in
> a single file, since that works against how sysfs is designed. But one
> thing I could imagine is adding a file ("lock", or whatever you want to
> call it) that you can use for atomic fencing:
>
> $ echo 1 > lock # locks the hardware state
> $ echo 100 > period
> $ echo 50 > duty_cycle
> $ echo 0 > lock # flushes state to hardware
>
> But it sounds like that's not even a big issue.
That is exactly what configfs was designed for :)
thanks,
greg k-h
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote:
> > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > >
> > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > Cc:.
> > >
> > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > thank you for your review!
> > > >
> > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de wrote:
> > > > > > From: Lars Poeschel
> > > > > >
> > > > > > This adds a class to exported pwm devices.
> > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > >
> > > > > I wonder what is your use-case here. This for sure also has a place to
> > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > accomplish you way.
> > > >
> > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > I use udev rules to adjust permissions.
> > >
> > > Hmm, how do you trigger the export? Without being aware of all the
> > > details in the sysfs code I would expect that the exported stuff is
> > > available instantly once the write used to export the PWM is completed.
> > > So changing the permissions can be done directly after triggering the
> > > export in the same process.
> >
> > The export is triggered through the userspace process itself. Why can it
> > do this ? Because there is another udev rule, that changes permissions
> > when a pwmchip appears.
> > Then I'd like to have the second udev rule, that changes permissions on
> > the freshly exported pwm. The userspace process can't do this.
> > You are right I could propably do everything from within udev: If a
> > pwmchip appears, export certain pwms and right away change their
> > permissions. It does not also not feel right. It'd require knowledge
> > from the userspace application to be mapped to udev.
>
> The way the kernel code is now, yes, you will not have any way to
> trigger it by userspace as the kernel is creating a "raw" struct device
> that isn't assigned to anything. That is what needs to be fixed here.
>
> > > Out of interest: What do you use the pwm for? Isn't there a suitable
> > > kernel driver that can do the required stuff? Compared to the kernel-API
> > > the sysfs interface isn't atomic. Is this an annoyance?
> >
> > Use-case is generating a voltage from the pwm. This voltage is used to
> > signal different states and does not change very often. This is
> > absolutely not annoying that this is not atomic. We just change the duty
> > cycle on the fly. Everything else is configured one time at startup.
> > I'd call what I need pwm-dac. I could not find a ready to use driver.
> > Maybe I could misuse some kernel driver for this. Maybe I could use
> > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work,
> > there is even a userspace facing part for this, but this is not
> > devicetree ready.
> > ...and the worst, please don't blame me: The application is java, so
> > ioctl is a problem.
>
> I thought java could do ioctls, otherwise how would it ever be able to
> talk to serial ports?
>
> Anyway, this needs to be fixed in the kernel...
If atomicity was a problem, we could potentially add a mechanism to the
sysfs interface to enable that. I don't see a good way of doing that in
a single file, since that works against how sysfs is designed. But one
thing I could imagine is adding a file ("lock", or whatever you want to
call it) that you can use for atomic fencing:
$ echo 1 > lock # locks the hardware state
$ echo 100 > period
$ echo 50 > duty_cycle
$ echo 0 > lock # flushes state to hardware
But it sounds like that's not even a big issue.
However, given the use-case description, it sounds to me like
pwm-regulator would be a better candidate to solve this, because it's
purpose is literally to generate a voltage using a PWM. There is a
device tree binding for pwm-regulator, so this should work there as
well.
Lars, what exactly are the problems that you're running into when trying
to use pwm-regulator with device tree?
Thierry
signature.asc
Description: PGP signature
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Thu, Oct 01, 2020 at 03:50:09PM +0200, Lars Poeschel wrote: > On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote: > > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote: > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote: > > > > Hello, > > > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to > > > > Cc:. > > > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote: > > > > > thank you for your review! > > > > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote: > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de > > > > > > wrote: > > > > > > > From: Lars Poeschel > > > > > > > > > > > > > > This adds a class to exported pwm devices. > > > > > > > Exporting a pwm through sysfs did not yield udev events. The > > > > > > > > > > > > I wonder what is your use-case here. This for sure also has a place > > > > > > to > > > > > > be mentioned in the commit log. I suspect there is a better way to > > > > > > accomplish you way. > > > > > > > > > > Use-case is to be able to use a pwm from a non-root userspace process. > > > > > I use udev rules to adjust permissions. > > > > > > > > Hmm, how do you trigger the export? Without being aware of all the > > > > details in the sysfs code I would expect that the exported stuff is > > > > available instantly once the write used to export the PWM is completed. > > > > So changing the permissions can be done directly after triggering the > > > > export in the same process. > > > > > > The export is triggered through the userspace process itself. Why can it > > > do this ? Because there is another udev rule, that changes permissions > > > when a pwmchip appears. > > > Then I'd like to have the second udev rule, that changes permissions on > > > the freshly exported pwm. The userspace process can't do this. > > > You are right I could propably do everything from within udev: If a > > > pwmchip appears, export certain pwms and right away change their > > > permissions. It does not also not feel right. It'd require knowledge > > > from the userspace application to be mapped to udev. > > > > The way the kernel code is now, yes, you will not have any way to > > trigger it by userspace as the kernel is creating a "raw" struct device > > that isn't assigned to anything. That is what needs to be fixed here. > > I did a first try with our approach. > I set the class of the child to its parent class. This does work and > create the directories right under /sys/pwm but because the child now > also inherits the dev_groups from the parent its directory also contain > export, unexport and npwm files, that don't apply for pwm's as soon a I > register the device to driver core. > > Did we miss something or is there a way to avoid that ? I had a look at > device_add and saw that as soon as a class it set it's dev_groups get > exported through device_add_attrs. Ah, you need to tweak that group to only show up for a specific "type" of device. There is a is_visable callback for a group that should be used for this, and you can check the type of device being affected here, try messing with that. And make sure you set a new type for the new devices you are creating. I know that's vague, if you need more help I can work on it next week. > > > > Out of interest: What do you use the pwm for? Isn't there a suitable > > > > kernel driver that can do the required stuff? Compared to the kernel-API > > > > the sysfs interface isn't atomic. Is this an annoyance? > > > > > > Use-case is generating a voltage from the pwm. This voltage is used to > > > signal different states and does not change very often. This is > > > absolutely not annoying that this is not atomic. We just change the duty > > > cycle on the fly. Everything else is configured one time at startup. > > > I'd call what I need pwm-dac. I could not find a ready to use driver. > > > Maybe I could misuse some kernel driver for this. Maybe I could use > > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work, > > > there is even a userspace facing part for this, but this is not > > > devicetree ready. > > > ...and the worst, please don't blame me: The application is java, so > > > ioctl is a problem. > > > > I thought java could do ioctls, otherwise how would it ever be able to > > talk to serial ports? > > It is not impossible but it is horrible. java itself can access the > ports through normal file I/O, but it can not set control lines or > baudrate or anything. You need some C-code for this, that is not part of > the java vm itself and has to be called through something called JNI - > java native interface. Ah, ok, yeah, that's not ok, sorry to hear you are stuck with Java :( greg k-h
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Thu, Oct 01, 2020 at 01:24:49PM +0200, Greg Kroah-Hartman wrote: > On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote: > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote: > > > Hello, > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to > > > Cc:. > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote: > > > > thank you for your review! > > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote: > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de wrote: > > > > > > From: Lars Poeschel > > > > > > > > > > > > This adds a class to exported pwm devices. > > > > > > Exporting a pwm through sysfs did not yield udev events. The > > > > > > > > > > I wonder what is your use-case here. This for sure also has a place to > > > > > be mentioned in the commit log. I suspect there is a better way to > > > > > accomplish you way. > > > > > > > > Use-case is to be able to use a pwm from a non-root userspace process. > > > > I use udev rules to adjust permissions. > > > > > > Hmm, how do you trigger the export? Without being aware of all the > > > details in the sysfs code I would expect that the exported stuff is > > > available instantly once the write used to export the PWM is completed. > > > So changing the permissions can be done directly after triggering the > > > export in the same process. > > > > The export is triggered through the userspace process itself. Why can it > > do this ? Because there is another udev rule, that changes permissions > > when a pwmchip appears. > > Then I'd like to have the second udev rule, that changes permissions on > > the freshly exported pwm. The userspace process can't do this. > > You are right I could propably do everything from within udev: If a > > pwmchip appears, export certain pwms and right away change their > > permissions. It does not also not feel right. It'd require knowledge > > from the userspace application to be mapped to udev. > > The way the kernel code is now, yes, you will not have any way to > trigger it by userspace as the kernel is creating a "raw" struct device > that isn't assigned to anything. That is what needs to be fixed here. I did a first try with our approach. I set the class of the child to its parent class. This does work and create the directories right under /sys/pwm but because the child now also inherits the dev_groups from the parent its directory also contain export, unexport and npwm files, that don't apply for pwm's as soon a I register the device to driver core. Did we miss something or is there a way to avoid that ? I had a look at device_add and saw that as soon as a class it set it's dev_groups get exported through device_add_attrs. > > > Out of interest: What do you use the pwm for? Isn't there a suitable > > > kernel driver that can do the required stuff? Compared to the kernel-API > > > the sysfs interface isn't atomic. Is this an annoyance? > > > > Use-case is generating a voltage from the pwm. This voltage is used to > > signal different states and does not change very often. This is > > absolutely not annoying that this is not atomic. We just change the duty > > cycle on the fly. Everything else is configured one time at startup. > > I'd call what I need pwm-dac. I could not find a ready to use driver. > > Maybe I could misuse some kernel driver for this. Maybe I could use > > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work, > > there is even a userspace facing part for this, but this is not > > devicetree ready. > > ...and the worst, please don't blame me: The application is java, so > > ioctl is a problem. > > I thought java could do ioctls, otherwise how would it ever be able to > talk to serial ports? It is not impossible but it is horrible. java itself can access the ports through normal file I/O, but it can not set control lines or baudrate or anything. You need some C-code for this, that is not part of the java vm itself and has to be called through something called JNI - java native interface. Regards, Lars
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Thu, Oct 01, 2020 at 11:05:31AM +0200, Lars Poeschel wrote: > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote: > > Hello, > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to > > Cc:. > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote: > > > thank you for your review! > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote: > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de wrote: > > > > > From: Lars Poeschel > > > > > > > > > > This adds a class to exported pwm devices. > > > > > Exporting a pwm through sysfs did not yield udev events. The > > > > > > > > I wonder what is your use-case here. This for sure also has a place to > > > > be mentioned in the commit log. I suspect there is a better way to > > > > accomplish you way. > > > > > > Use-case is to be able to use a pwm from a non-root userspace process. > > > I use udev rules to adjust permissions. > > > > Hmm, how do you trigger the export? Without being aware of all the > > details in the sysfs code I would expect that the exported stuff is > > available instantly once the write used to export the PWM is completed. > > So changing the permissions can be done directly after triggering the > > export in the same process. > > The export is triggered through the userspace process itself. Why can it > do this ? Because there is another udev rule, that changes permissions > when a pwmchip appears. > Then I'd like to have the second udev rule, that changes permissions on > the freshly exported pwm. The userspace process can't do this. > You are right I could propably do everything from within udev: If a > pwmchip appears, export certain pwms and right away change their > permissions. It does not also not feel right. It'd require knowledge > from the userspace application to be mapped to udev. The way the kernel code is now, yes, you will not have any way to trigger it by userspace as the kernel is creating a "raw" struct device that isn't assigned to anything. That is what needs to be fixed here. > > Out of interest: What do you use the pwm for? Isn't there a suitable > > kernel driver that can do the required stuff? Compared to the kernel-API > > the sysfs interface isn't atomic. Is this an annoyance? > > Use-case is generating a voltage from the pwm. This voltage is used to > signal different states and does not change very often. This is > absolutely not annoying that this is not atomic. We just change the duty > cycle on the fly. Everything else is configured one time at startup. > I'd call what I need pwm-dac. I could not find a ready to use driver. > Maybe I could misuse some kernel driver for this. Maybe I could use > pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work, > there is even a userspace facing part for this, but this is not > devicetree ready. > ...and the worst, please don't blame me: The application is java, so > ioctl is a problem. I thought java could do ioctls, otherwise how would it ever be able to talk to serial ports? Anyway, this needs to be fixed in the kernel... thanks, greg k-h
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote: > Hello, > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to > Cc:. > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote: > > thank you for your review! > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote: > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de wrote: > > > > From: Lars Poeschel > > > > > > > > This adds a class to exported pwm devices. > > > > Exporting a pwm through sysfs did not yield udev events. The > > > > > > I wonder what is your use-case here. This for sure also has a place to > > > be mentioned in the commit log. I suspect there is a better way to > > > accomplish you way. > > > > Use-case is to be able to use a pwm from a non-root userspace process. > > I use udev rules to adjust permissions. > > Hmm, how do you trigger the export? Without being aware of all the > details in the sysfs code I would expect that the exported stuff is > available instantly once the write used to export the PWM is completed. > So changing the permissions can be done directly after triggering the > export in the same process. The export is triggered through the userspace process itself. Why can it do this ? Because there is another udev rule, that changes permissions when a pwmchip appears. Then I'd like to have the second udev rule, that changes permissions on the freshly exported pwm. The userspace process can't do this. You are right I could propably do everything from within udev: If a pwmchip appears, export certain pwms and right away change their permissions. It does not also not feel right. It'd require knowledge from the userspace application to be mapped to udev. > Out of interest: What do you use the pwm for? Isn't there a suitable > kernel driver that can do the required stuff? Compared to the kernel-API > the sysfs interface isn't atomic. Is this an annoyance? Use-case is generating a voltage from the pwm. This voltage is used to signal different states and does not change very often. This is absolutely not annoying that this is not atomic. We just change the duty cycle on the fly. Everything else is configured one time at startup. I'd call what I need pwm-dac. I could not find a ready to use driver. Maybe I could misuse some kernel driver for this. Maybe I could use pwm-led or pwm-brightness or pwm-fan. Propably pwm-regulator could work, there is even a userspace facing part for this, but this is not devicetree ready. ...and the worst, please don't blame me: The application is java, so ioctl is a problem. So, sysfs was quite a good choice for me. Regards, Lars
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Wed, Sep 30, 2020 at 05:03:02PM +0200, Uwe Kleine-König wrote:
> On Wed, Sep 30, 2020 at 04:13:52PM +0200, Lars Poeschel wrote:
> > On Wed, Sep 30, 2020 at 01:51:06PM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 30, 2020 at 01:27:20PM +0200, Lars Poeschel wrote:
> > > > On Wed, Sep 30, 2020 at 12:52:38PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> > > > > > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > > > Hello,
> > > > > > > >
> > > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a
> > > > > > > > bit to
> > > > > > > > Cc:.
> > > > > > > >
> > > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > > > thank you for your review!
> > > > > > > > >
> > > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König
> > > > > > > > > wrote:
> > > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200,
> > > > > > > > > > poesc...@lemonage.de wrote:
> > > > > > > > > > > From: Lars Poeschel
> > > > > > > > > > >
> > > > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > > > Exporting a pwm through sysfs did not yield udev events.
> > > > > > > > > > > The
> > > > > > > > > >
> > > > > > > > > > I wonder what is your use-case here. This for sure also has
> > > > > > > > > > a place to
> > > > > > > > > > be mentioned in the commit log. I suspect there is a better
> > > > > > > > > > way to
> > > > > > > > > > accomplish you way.
> > > > > > > > >
> > > > > > > > > Use-case is to be able to use a pwm from a non-root userspace
> > > > > > > > > process.
> > > > > > > > > I use udev rules to adjust permissions.
> > > > > > > >
> > > > > > > > Hmm, how do you trigger the export? Without being aware of all
> > > > > > > > the
> > > > > > > > details in the sysfs code I would expect that the exported
> > > > > > > > stuff is
> > > > > > > > available instantly once the write used to export the PWM is
> > > > > > > > completed.
> > > > > > > > So changing the permissions can be done directly after
> > > > > > > > triggering the
> > > > > > > > export in the same process.
> > > > > > >
> > > > > > > It looks like userspace wants to see when a pwmX device shows up,
> > > > > > > right?
> > > > > > >
> > > > > > > And it's not because those devices do not belong to any class or
> > > > > > > bus, so
> > > > > > > they are just "floating" out there (they might show up under
> > > > > > > /sys/bus/virtual, if you set things up right, which I don't think
> > > > > > > is
> > > > > > > happening here...)
> > > > > > >
> > > > > > > So yes, you need to create a class, or assign this to a bus,
> > > > > > > which is
> > > > > > > fine, but it looks like no one is doing that. Don't create new
> > > > > > > classes
> > > > > > > dynamically, but rather, just assign this to the existing pwm
> > > > > > > class.
> > > > > > > What's wrong with that? I saw an older patch that did that, what
> > > > > > > did
> > > > > > > that break?
> > > > > >
> > > > > > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did
> > > > > > you
> > > > > > read the reverting commit's log message? (i.e.
> > > > > > c289d6625237aa785b484b4e94c23b3b91ea7e60)
> > > > > >
> > > > > > I guess the breakage is that the resulting name then is:
> > > > > >
> > > > > > "pwm%d", pwm->id
> > > > > >
> > > > > > where pwm->id is a number unique to the pwmchip. So doing
> > > > > >
> > > > > > echo 0 > pwmchip1/export
> > > > > > echo 0 > pwmchip2/export
> > > > > >
> > > > > > breaks because both want to create pwm0 in the class directory.
> > > > >
> > > > > Ah, that makes more sense why that didn't work.
> > > > >
> > > > > Ok, can the "name" of the new export chip be changed? Is that
> > > > > hard-coded somewhere in userspace tools already? Depending on that,
> > > > > the
> > > > > solution for this will change...
> > > >
> > > > I know that back then, when sysfs for pwm was created, Thierry didn't
> > > > want to have one global namespace like gpio sysfs has. What you ask for
> > > > is something like:
> > > > pwm-{chipnumber}-{pwmnumber}
> > > > Right ? Can that be considered non-global ?
> > >
> > > Yes, and that's just "global" for the pwm class namespace.
> > >
> > > > Thierry's mail from back then is here:
> > > > https://lore.kernel.org/lkml/20130408081745.ga21...@avionic-0098.mockup.avionic-design.de/
> > > >
> > > > A short search on github I found this:
> > > > https://github.com/vsergeev/c-periphery/blob/d34077d7ee45fa7d1947cc0174919452fac31597/src/pwm.c#L74
> > > >
> > > > Seems to match your hardcoded criteria ?
> > >
> > > Yes, ugh :(
> > >
> > > Ok, now I see why the "lots of pwm classes!" patch was proposed.
> > >
> > > And maybe that's really the only way forward here, as the chip
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Wed, Sep 30, 2020 at 04:13:52PM +0200, Lars Poeschel wrote:
> On Wed, Sep 30, 2020 at 01:51:06PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 30, 2020 at 01:27:20PM +0200, Lars Poeschel wrote:
> > > On Wed, Sep 30, 2020 at 12:52:38PM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> > > > > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > > Hello,
> > > > > > >
> > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a
> > > > > > > bit to
> > > > > > > Cc:.
> > > > > > >
> > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > > thank you for your review!
> > > > > > > >
> > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König
> > > > > > > > wrote:
> > > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200,
> > > > > > > > > poesc...@lemonage.de wrote:
> > > > > > > > > > From: Lars Poeschel
> > > > > > > > > >
> > > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > > >
> > > > > > > > > I wonder what is your use-case here. This for sure also has a
> > > > > > > > > place to
> > > > > > > > > be mentioned in the commit log. I suspect there is a better
> > > > > > > > > way to
> > > > > > > > > accomplish you way.
> > > > > > > >
> > > > > > > > Use-case is to be able to use a pwm from a non-root userspace
> > > > > > > > process.
> > > > > > > > I use udev rules to adjust permissions.
> > > > > > >
> > > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > > details in the sysfs code I would expect that the exported stuff
> > > > > > > is
> > > > > > > available instantly once the write used to export the PWM is
> > > > > > > completed.
> > > > > > > So changing the permissions can be done directly after triggering
> > > > > > > the
> > > > > > > export in the same process.
> > > > > >
> > > > > > It looks like userspace wants to see when a pwmX device shows up,
> > > > > > right?
> > > > > >
> > > > > > And it's not because those devices do not belong to any class or
> > > > > > bus, so
> > > > > > they are just "floating" out there (they might show up under
> > > > > > /sys/bus/virtual, if you set things up right, which I don't think is
> > > > > > happening here...)
> > > > > >
> > > > > > So yes, you need to create a class, or assign this to a bus, which
> > > > > > is
> > > > > > fine, but it looks like no one is doing that. Don't create new
> > > > > > classes
> > > > > > dynamically, but rather, just assign this to the existing pwm class.
> > > > > > What's wrong with that? I saw an older patch that did that, what
> > > > > > did
> > > > > > that break?
> > > > >
> > > > > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
> > > > > read the reverting commit's log message? (i.e.
> > > > > c289d6625237aa785b484b4e94c23b3b91ea7e60)
> > > > >
> > > > > I guess the breakage is that the resulting name then is:
> > > > >
> > > > > "pwm%d", pwm->id
> > > > >
> > > > > where pwm->id is a number unique to the pwmchip. So doing
> > > > >
> > > > > echo 0 > pwmchip1/export
> > > > > echo 0 > pwmchip2/export
> > > > >
> > > > > breaks because both want to create pwm0 in the class directory.
> > > >
> > > > Ah, that makes more sense why that didn't work.
> > > >
> > > > Ok, can the "name" of the new export chip be changed? Is that
> > > > hard-coded somewhere in userspace tools already? Depending on that, the
> > > > solution for this will change...
> > >
> > > I know that back then, when sysfs for pwm was created, Thierry didn't
> > > want to have one global namespace like gpio sysfs has. What you ask for
> > > is something like:
> > > pwm-{chipnumber}-{pwmnumber}
> > > Right ? Can that be considered non-global ?
> >
> > Yes, and that's just "global" for the pwm class namespace.
> >
> > > Thierry's mail from back then is here:
> > > https://lore.kernel.org/lkml/20130408081745.ga21...@avionic-0098.mockup.avionic-design.de/
> > >
> > > A short search on github I found this:
> > > https://github.com/vsergeev/c-periphery/blob/d34077d7ee45fa7d1947cc0174919452fac31597/src/pwm.c#L74
> > >
> > > Seems to match your hardcoded criteria ?
> >
> > Yes, ugh :(
> >
> > Ok, now I see why the "lots of pwm classes!" patch was proposed.
> >
> > And maybe that's really the only way forward here, as the chip namespace
> > is the only unique thing.
> >
> > But wow, it feels wrong...
>
> Would the following feel better:
> * use the new naming scheme you proposed for pwm's :
> pwm-{chipnumber}-{pwmnumber}
> * assign the normal pwm class to the exported pwm devices. That lets
> them appear in the global /sys/class/pwm directory as e.g. pwm-0-0
> * maintain backward
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Wed, Sep 30, 2020 at 01:51:06PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 30, 2020 at 01:27:20PM +0200, Lars Poeschel wrote:
> > On Wed, Sep 30, 2020 at 12:52:38PM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> > > > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > > Hello,
> > > > > >
> > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit
> > > > > > to
> > > > > > Cc:.
> > > > > >
> > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > > thank you for your review!
> > > > > > >
> > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de
> > > > > > > > wrote:
> > > > > > > > > From: Lars Poeschel
> > > > > > > > >
> > > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > > >
> > > > > > > > I wonder what is your use-case here. This for sure also has a
> > > > > > > > place to
> > > > > > > > be mentioned in the commit log. I suspect there is a better way
> > > > > > > > to
> > > > > > > > accomplish you way.
> > > > > > >
> > > > > > > Use-case is to be able to use a pwm from a non-root userspace
> > > > > > > process.
> > > > > > > I use udev rules to adjust permissions.
> > > > > >
> > > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > > available instantly once the write used to export the PWM is
> > > > > > completed.
> > > > > > So changing the permissions can be done directly after triggering
> > > > > > the
> > > > > > export in the same process.
> > > > >
> > > > > It looks like userspace wants to see when a pwmX device shows up,
> > > > > right?
> > > > >
> > > > > And it's not because those devices do not belong to any class or bus,
> > > > > so
> > > > > they are just "floating" out there (they might show up under
> > > > > /sys/bus/virtual, if you set things up right, which I don't think is
> > > > > happening here...)
> > > > >
> > > > > So yes, you need to create a class, or assign this to a bus, which is
> > > > > fine, but it looks like no one is doing that. Don't create new
> > > > > classes
> > > > > dynamically, but rather, just assign this to the existing pwm class.
> > > > > What's wrong with that? I saw an older patch that did that, what did
> > > > > that break?
> > > >
> > > > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
> > > > read the reverting commit's log message? (i.e.
> > > > c289d6625237aa785b484b4e94c23b3b91ea7e60)
> > > >
> > > > I guess the breakage is that the resulting name then is:
> > > >
> > > > "pwm%d", pwm->id
> > > >
> > > > where pwm->id is a number unique to the pwmchip. So doing
> > > >
> > > > echo 0 > pwmchip1/export
> > > > echo 0 > pwmchip2/export
> > > >
> > > > breaks because both want to create pwm0 in the class directory.
> > >
> > > Ah, that makes more sense why that didn't work.
> > >
> > > Ok, can the "name" of the new export chip be changed? Is that
> > > hard-coded somewhere in userspace tools already? Depending on that, the
> > > solution for this will change...
> >
> > I know that back then, when sysfs for pwm was created, Thierry didn't
> > want to have one global namespace like gpio sysfs has. What you ask for
> > is something like:
> > pwm-{chipnumber}-{pwmnumber}
> > Right ? Can that be considered non-global ?
>
> Yes, and that's just "global" for the pwm class namespace.
>
> > Thierry's mail from back then is here:
> > https://lore.kernel.org/lkml/20130408081745.ga21...@avionic-0098.mockup.avionic-design.de/
> >
> > A short search on github I found this:
> > https://github.com/vsergeev/c-periphery/blob/d34077d7ee45fa7d1947cc0174919452fac31597/src/pwm.c#L74
> >
> > Seems to match your hardcoded criteria ?
>
> Yes, ugh :(
>
> Ok, now I see why the "lots of pwm classes!" patch was proposed.
>
> And maybe that's really the only way forward here, as the chip namespace
> is the only unique thing.
>
> But wow, it feels wrong...
Would the following feel better:
* use the new naming scheme you proposed for pwm's :
pwm-{chipnumber}-{pwmnumber}
* assign the normal pwm class to the exported pwm devices. That lets
them appear in the global /sys/class/pwm directory as e.g. pwm-0-0
* maintain backward compatibility through symlinks e.g.:
pwmchip0/pwm0 -> ../pwm-0-0
Or does this feel even more horrible ?
I may miss some subtleties.
Regards,
Lars
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Wed, Sep 30, 2020 at 01:27:20PM +0200, Lars Poeschel wrote:
> On Wed, Sep 30, 2020 at 12:52:38PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> > > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > > Hello,
> > > > >
> > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > > Cc:.
> > > > >
> > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > > thank you for your review!
> > > > > >
> > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de
> > > > > > > wrote:
> > > > > > > > From: Lars Poeschel
> > > > > > > >
> > > > > > > > This adds a class to exported pwm devices.
> > > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > > >
> > > > > > > I wonder what is your use-case here. This for sure also has a
> > > > > > > place to
> > > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > > accomplish you way.
> > > > > >
> > > > > > Use-case is to be able to use a pwm from a non-root userspace
> > > > > > process.
> > > > > > I use udev rules to adjust permissions.
> > > > >
> > > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > > details in the sysfs code I would expect that the exported stuff is
> > > > > available instantly once the write used to export the PWM is
> > > > > completed.
> > > > > So changing the permissions can be done directly after triggering the
> > > > > export in the same process.
> > > >
> > > > It looks like userspace wants to see when a pwmX device shows up, right?
> > > >
> > > > And it's not because those devices do not belong to any class or bus, so
> > > > they are just "floating" out there (they might show up under
> > > > /sys/bus/virtual, if you set things up right, which I don't think is
> > > > happening here...)
> > > >
> > > > So yes, you need to create a class, or assign this to a bus, which is
> > > > fine, but it looks like no one is doing that. Don't create new classes
> > > > dynamically, but rather, just assign this to the existing pwm class.
> > > > What's wrong with that? I saw an older patch that did that, what did
> > > > that break?
> > >
> > > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
> > > read the reverting commit's log message? (i.e.
> > > c289d6625237aa785b484b4e94c23b3b91ea7e60)
> > >
> > > I guess the breakage is that the resulting name then is:
> > >
> > > "pwm%d", pwm->id
> > >
> > > where pwm->id is a number unique to the pwmchip. So doing
> > >
> > > echo 0 > pwmchip1/export
> > > echo 0 > pwmchip2/export
> > >
> > > breaks because both want to create pwm0 in the class directory.
> >
> > Ah, that makes more sense why that didn't work.
> >
> > Ok, can the "name" of the new export chip be changed? Is that
> > hard-coded somewhere in userspace tools already? Depending on that, the
> > solution for this will change...
>
> I know that back then, when sysfs for pwm was created, Thierry didn't
> want to have one global namespace like gpio sysfs has. What you ask for
> is something like:
> pwm-{chipnumber}-{pwmnumber}
> Right ? Can that be considered non-global ?
Yes, and that's just "global" for the pwm class namespace.
> Thierry's mail from back then is here:
> https://lore.kernel.org/lkml/20130408081745.ga21...@avionic-0098.mockup.avionic-design.de/
>
> A short search on github I found this:
> https://github.com/vsergeev/c-periphery/blob/d34077d7ee45fa7d1947cc0174919452fac31597/src/pwm.c#L74
>
> Seems to match your hardcoded criteria ?
Yes, ugh :(
Ok, now I see why the "lots of pwm classes!" patch was proposed.
And maybe that's really the only way forward here, as the chip namespace
is the only unique thing.
But wow, it feels wrong...
greg k-h
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Wed, Sep 30, 2020 at 12:52:38PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote:
> > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote:
> > > > Hello,
> > > >
> > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to
> > > > Cc:.
> > > >
> > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote:
> > > > > thank you for your review!
> > > > >
> > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de
> > > > > > wrote:
> > > > > > > From: Lars Poeschel
> > > > > > >
> > > > > > > This adds a class to exported pwm devices.
> > > > > > > Exporting a pwm through sysfs did not yield udev events. The
> > > > > >
> > > > > > I wonder what is your use-case here. This for sure also has a place
> > > > > > to
> > > > > > be mentioned in the commit log. I suspect there is a better way to
> > > > > > accomplish you way.
> > > > >
> > > > > Use-case is to be able to use a pwm from a non-root userspace process.
> > > > > I use udev rules to adjust permissions.
> > > >
> > > > Hmm, how do you trigger the export? Without being aware of all the
> > > > details in the sysfs code I would expect that the exported stuff is
> > > > available instantly once the write used to export the PWM is completed.
> > > > So changing the permissions can be done directly after triggering the
> > > > export in the same process.
> > >
> > > It looks like userspace wants to see when a pwmX device shows up, right?
> > >
> > > And it's not because those devices do not belong to any class or bus, so
> > > they are just "floating" out there (they might show up under
> > > /sys/bus/virtual, if you set things up right, which I don't think is
> > > happening here...)
> > >
> > > So yes, you need to create a class, or assign this to a bus, which is
> > > fine, but it looks like no one is doing that. Don't create new classes
> > > dynamically, but rather, just assign this to the existing pwm class.
> > > What's wrong with that? I saw an older patch that did that, what did
> > > that break?
> >
> > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you
> > read the reverting commit's log message? (i.e.
> > c289d6625237aa785b484b4e94c23b3b91ea7e60)
> >
> > I guess the breakage is that the resulting name then is:
> >
> > "pwm%d", pwm->id
> >
> > where pwm->id is a number unique to the pwmchip. So doing
> >
> > echo 0 > pwmchip1/export
> > echo 0 > pwmchip2/export
> >
> > breaks because both want to create pwm0 in the class directory.
>
> Ah, that makes more sense why that didn't work.
>
> Ok, can the "name" of the new export chip be changed? Is that
> hard-coded somewhere in userspace tools already? Depending on that, the
> solution for this will change...
I know that back then, when sysfs for pwm was created, Thierry didn't
want to have one global namespace like gpio sysfs has. What you ask for
is something like:
pwm-{chipnumber}-{pwmnumber}
Right ? Can that be considered non-global ?
Thierry's mail from back then is here:
https://lore.kernel.org/lkml/20130408081745.ga21...@avionic-0098.mockup.avionic-design.de/
A short search on github I found this:
https://github.com/vsergeev/c-periphery/blob/d34077d7ee45fa7d1947cc0174919452fac31597/src/pwm.c#L74
Seems to match your hardcoded criteria ?
Regards,
Lars
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Wed, Sep 30, 2020 at 12:01:26PM +0200, Uwe Kleine-König wrote: > On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote: > > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote: > > > Hello, > > > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to > > > Cc:. > > > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote: > > > > thank you for your review! > > > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote: > > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de wrote: > > > > > > From: Lars Poeschel > > > > > > > > > > > > This adds a class to exported pwm devices. > > > > > > Exporting a pwm through sysfs did not yield udev events. The > > > > > > > > > > I wonder what is your use-case here. This for sure also has a place to > > > > > be mentioned in the commit log. I suspect there is a better way to > > > > > accomplish you way. > > > > > > > > Use-case is to be able to use a pwm from a non-root userspace process. > > > > I use udev rules to adjust permissions. > > > > > > Hmm, how do you trigger the export? Without being aware of all the > > > details in the sysfs code I would expect that the exported stuff is > > > available instantly once the write used to export the PWM is completed. > > > So changing the permissions can be done directly after triggering the > > > export in the same process. > > > > It looks like userspace wants to see when a pwmX device shows up, right? > > > > And it's not because those devices do not belong to any class or bus, so > > they are just "floating" out there (they might show up under > > /sys/bus/virtual, if you set things up right, which I don't think is > > happening here...) > > > > So yes, you need to create a class, or assign this to a bus, which is > > fine, but it looks like no one is doing that. Don't create new classes > > dynamically, but rather, just assign this to the existing pwm class. > > What's wrong with that? I saw an older patch that did that, what did > > that break? > > Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you > read the reverting commit's log message? (i.e. > c289d6625237aa785b484b4e94c23b3b91ea7e60) > > I guess the breakage is that the resulting name then is: > > "pwm%d", pwm->id > > where pwm->id is a number unique to the pwmchip. So doing > > echo 0 > pwmchip1/export > echo 0 > pwmchip2/export > > breaks because both want to create pwm0 in the class directory. Ah, that makes more sense why that didn't work. Ok, can the "name" of the new export chip be changed? Is that hard-coded somewhere in userspace tools already? Depending on that, the solution for this will change... thanks, greg k-h
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Wed, Sep 30, 2020 at 11:52:04AM +0200, Greg Kroah-Hartman wrote: > On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote: > > Hello, > > > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to > > Cc:. > > > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote: > > > thank you for your review! > > > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote: > > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de wrote: > > > > > From: Lars Poeschel > > > > > > > > > > This adds a class to exported pwm devices. > > > > > Exporting a pwm through sysfs did not yield udev events. The > > > > > > > > I wonder what is your use-case here. This for sure also has a place to > > > > be mentioned in the commit log. I suspect there is a better way to > > > > accomplish you way. > > > > > > Use-case is to be able to use a pwm from a non-root userspace process. > > > I use udev rules to adjust permissions. > > > > Hmm, how do you trigger the export? Without being aware of all the > > details in the sysfs code I would expect that the exported stuff is > > available instantly once the write used to export the PWM is completed. > > So changing the permissions can be done directly after triggering the > > export in the same process. > > It looks like userspace wants to see when a pwmX device shows up, right? > > And it's not because those devices do not belong to any class or bus, so > they are just "floating" out there (they might show up under > /sys/bus/virtual, if you set things up right, which I don't think is > happening here...) > > So yes, you need to create a class, or assign this to a bus, which is > fine, but it looks like no one is doing that. Don't create new classes > dynamically, but rather, just assign this to the existing pwm class. > What's wrong with that? I saw an older patch that did that, what did > that break? Are you refering to 7e5d1fd75c3dde9fc10c4472b9368089d1b81d00? Did you read the reverting commit's log message? (i.e. c289d6625237aa785b484b4e94c23b3b91ea7e60) I guess the breakage is that the resulting name then is: "pwm%d", pwm->id where pwm->id is a number unique to the pwmchip. So doing echo 0 > pwmchip1/export echo 0 > pwmchip2/export breaks because both want to create pwm0 in the class directory. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH] pwm: sysfs: Set class on pwm devices
On Wed, Sep 30, 2020 at 11:41:46AM +0200, Uwe Kleine-König wrote: > Hello, > > I added Greg Kroah-Hartman who I discussed this with via irc a bit to > Cc:. > > On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote: > > thank you for your review! > > > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote: > > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de wrote: > > > > From: Lars Poeschel > > > > > > > > This adds a class to exported pwm devices. > > > > Exporting a pwm through sysfs did not yield udev events. The > > > > > > I wonder what is your use-case here. This for sure also has a place to > > > be mentioned in the commit log. I suspect there is a better way to > > > accomplish you way. > > > > Use-case is to be able to use a pwm from a non-root userspace process. > > I use udev rules to adjust permissions. > > Hmm, how do you trigger the export? Without being aware of all the > details in the sysfs code I would expect that the exported stuff is > available instantly once the write used to export the PWM is completed. > So changing the permissions can be done directly after triggering the > export in the same process. It looks like userspace wants to see when a pwmX device shows up, right? And it's not because those devices do not belong to any class or bus, so they are just "floating" out there (they might show up under /sys/bus/virtual, if you set things up right, which I don't think is happening here...) So yes, you need to create a class, or assign this to a bus, which is fine, but it looks like no one is doing that. Don't create new classes dynamically, but rather, just assign this to the existing pwm class. What's wrong with that? I saw an older patch that did that, what did that break? thanks, greg k-h
Re: [PATCH] pwm: sysfs: Set class on pwm devices
Hello, I added Greg Kroah-Hartman who I discussed this with via irc a bit to Cc:. On Wed, Sep 30, 2020 at 11:20:56AM +0200, Lars Poeschel wrote: > thank you for your review! > > On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote: > > On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de wrote: > > > From: Lars Poeschel > > > > > > This adds a class to exported pwm devices. > > > Exporting a pwm through sysfs did not yield udev events. The > > > > I wonder what is your use-case here. This for sure also has a place to > > be mentioned in the commit log. I suspect there is a better way to > > accomplish you way. > > Use-case is to be able to use a pwm from a non-root userspace process. > I use udev rules to adjust permissions. Hmm, how do you trigger the export? Without being aware of all the details in the sysfs code I would expect that the exported stuff is available instantly once the write used to export the PWM is completed. So changing the permissions can be done directly after triggering the export in the same process. Out of interest: What do you use the pwm for? Isn't there a suitable kernel driver that can do the required stuff? Compared to the kernel-API the sysfs interface isn't atomic. Is this an annoyance? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH] pwm: sysfs: Set class on pwm devices
Hi Uwe,
thank you for your review!
On Wed, Sep 30, 2020 at 08:57:26AM +0200, Uwe Kleine-König wrote:
> Hello Lars,
>
> On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de wrote:
> > From: Lars Poeschel
> >
> > This adds a class to exported pwm devices.
> > Exporting a pwm through sysfs did not yield udev events. The
>
> I wonder what is your use-case here. This for sure also has a place to
> be mentioned in the commit log. I suspect there is a better way to
> accomplish you way.
Use-case is to be able to use a pwm from a non-root userspace process.
I use udev rules to adjust permissions.
> > dev_uevent_filter function does filter-out devices without a bus or
> > class.
> > This was already addressed in commit
> > commit 7e5d1fd75c3d ("pwm: Set class for exported channels in sysfs")
> > but this did cause problems and the commit got reverted with
> > commit c289d6625237 ("Revert "pwm: Set class for exported channels in
> > sysfs"")
> >
> > pwm's have to be local to its pwmchip, so we create an individual class
> > for each pwmchip
>
> This sounds conceptually wrong.
I suspect you mean the second part is wrong and we agree on the first
part, that pwm's have to be local to its pwmchip.
There seems to be a need for this as 7e5d1fd75c3d tried this already.
Problem with this approach was, that the pwms where not local to their
pwmchip and if you then have the same pwm number exported from different
pwmchips this did collide.
Ok, now the uevent_ops filter function blocks the uevent I want to see
based on if device has a bus or a class set.
Can you recommend a better solution ?
Write a different filter function for this case ?
> > and assign this class to the pwm belonging to the
> > pwmchip. This does better map to structure that is also visible in
> > sysfs.
> >
> > Signed-off-by: Lars Poeschel
> > ---
> > drivers/pwm/sysfs.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> > index 449dbc0f49ed..e2dfbc335366 100644
> > --- a/drivers/pwm/sysfs.c
> > +++ b/drivers/pwm/sysfs.c
> > @@ -259,7 +259,7 @@ static int pwm_export_child(struct device *parent,
> > struct pwm_device *pwm)
> > export->child.release = pwm_export_release;
> > export->child.parent = parent;
> > export->child.devt = MKDEV(0, 0);
> > - export->child.groups = pwm_groups;
> > + export->child.class = parent->class;
> > dev_set_name(>child, "pwm%u", pwm->hwpwm);
> >
> > ret = device_register(>child);
> > @@ -499,6 +499,9 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
> > dev_warn(chip->dev,
> > "device_create failed for pwm_chip sysfs export\n");
> > }
> > +
> > + parent->class = class_create(THIS_MODULE, parent->kobj.name);
>
> This needs error handling.
If this concept has a chance after discussion, I will change this.
Thanks again,
Lars
Re: [PATCH] pwm: sysfs: Set class on pwm devices
Hello Lars,
On Tue, Sep 29, 2020 at 02:19:53PM +0200, poesc...@lemonage.de wrote:
> From: Lars Poeschel
>
> This adds a class to exported pwm devices.
> Exporting a pwm through sysfs did not yield udev events. The
I wonder what is your use-case here. This for sure also has a place to
be mentioned in the commit log. I suspect there is a better way to
accomplish you way.
> dev_uevent_filter function does filter-out devices without a bus or
> class.
> This was already addressed in commit
> commit 7e5d1fd75c3d ("pwm: Set class for exported channels in sysfs")
> but this did cause problems and the commit got reverted with
> commit c289d6625237 ("Revert "pwm: Set class for exported channels in
> sysfs"")
>
> pwm's have to be local to its pwmchip, so we create an individual class
> for each pwmchip
This sounds conceptually wrong.
> and assign this class to the pwm belonging to the
> pwmchip. This does better map to structure that is also visible in
> sysfs.
>
> Signed-off-by: Lars Poeschel
> ---
> drivers/pwm/sysfs.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 449dbc0f49ed..e2dfbc335366 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -259,7 +259,7 @@ static int pwm_export_child(struct device *parent, struct
> pwm_device *pwm)
> export->child.release = pwm_export_release;
> export->child.parent = parent;
> export->child.devt = MKDEV(0, 0);
> - export->child.groups = pwm_groups;
> + export->child.class = parent->class;
> dev_set_name(>child, "pwm%u", pwm->hwpwm);
>
> ret = device_register(>child);
> @@ -499,6 +499,9 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
> dev_warn(chip->dev,
>"device_create failed for pwm_chip sysfs export\n");
> }
> +
> + parent->class = class_create(THIS_MODULE, parent->kobj.name);
This needs error handling.
> + parent->class->dev_groups = pwm_groups;
> }
>
> void pwmchip_sysfs_unexport(struct pwm_chip *chip)
> @@ -518,6 +521,7 @@ void pwmchip_sysfs_unexport(struct pwm_chip *chip)
> pwm_unexport_child(parent, pwm);
> }
>
> + class_destroy(parent->class);
> put_device(parent);
> device_unregister(parent);
> }
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
[PATCH] pwm: sysfs: Set class on pwm devices
From: Lars Poeschel
This adds a class to exported pwm devices.
Exporting a pwm through sysfs did not yield udev events. The
dev_uevent_filter function does filter-out devices without a bus or
class.
This was already addressed in commit
commit 7e5d1fd75c3d ("pwm: Set class for exported channels in sysfs")
but this did cause problems and the commit got reverted with
commit c289d6625237 ("Revert "pwm: Set class for exported channels in
sysfs"")
pwm's have to be local to its pwmchip, so we create an individual class
for each pwmchip and assign this class to the pwm belonging to the
pwmchip. This does better map to structure that is also visible in
sysfs.
Signed-off-by: Lars Poeschel
---
drivers/pwm/sysfs.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 449dbc0f49ed..e2dfbc335366 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -259,7 +259,7 @@ static int pwm_export_child(struct device *parent, struct
pwm_device *pwm)
export->child.release = pwm_export_release;
export->child.parent = parent;
export->child.devt = MKDEV(0, 0);
- export->child.groups = pwm_groups;
+ export->child.class = parent->class;
dev_set_name(>child, "pwm%u", pwm->hwpwm);
ret = device_register(>child);
@@ -499,6 +499,9 @@ void pwmchip_sysfs_export(struct pwm_chip *chip)
dev_warn(chip->dev,
"device_create failed for pwm_chip sysfs export\n");
}
+
+ parent->class = class_create(THIS_MODULE, parent->kobj.name);
+ parent->class->dev_groups = pwm_groups;
}
void pwmchip_sysfs_unexport(struct pwm_chip *chip)
@@ -518,6 +521,7 @@ void pwmchip_sysfs_unexport(struct pwm_chip *chip)
pwm_unexport_child(parent, pwm);
}
+ class_destroy(parent->class);
put_device(parent);
device_unregister(parent);
}
--
2.28.0
