Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make pwm sysfs work for non-root users #1983

Closed
gohai opened this issue May 1, 2017 · 37 comments
Closed

Make pwm sysfs work for non-root users #1983

gohai opened this issue May 1, 2017 · 37 comments
Labels
Waiting for external input Waiting for a comment from the originator of the issue, or a collaborator.

Comments

@gohai
Copy link
Contributor

gohai commented May 1, 2017

I sent a one-liner patch to the linux-pwm mailing list, which makes exporting a channel (with echo 0 > /sys/class/pwm/pwmchip0/export) send a udev event.

With this patch, and the following addition to /etc/udev/rules.d/99-com.rules, it is finally possible to use the pwm functionality with the default pi user, instead of having to use sudo (which is not an option for Processing e.g.):

SUBSYSTEM=="pwm*", PROGRAM="/bin/sh -c '\
        chown -R root:gpio /sys/class/pwm && chmod -R 770 /sys/class/pwm;\
        chown -R root:gpio /sys/devices/platform/soc/*.pwm/pwm/pwmchip* && chmod -R 770 /sys/devices/platform/soc/*.pwm/pwm/pwmchip*\
'"

Would you mind giving these changes a try, and consider them for inclusion with Raspbian? I am the author of Processing's Hardware I/O library - this would enable users to use hardware PWM from within Processing on their Raspberry Pi computers.

Let me know if I should send a PR for the kernel part of this - I'll of course also be eying any discussion that happens upstream regarding my patch submission.

@pelwell
Copy link
Contributor

pelwell commented May 1, 2017

I would expect your patch to be quickly accepted - let us know when it is and we will back-port it to the current rpi-
branches and merge your udev rule into Raspbian.

@JamesH65
Copy link
Contributor

@gohai Do you know if this was accepted? If so we can cherry pick it's commit.

@JamesH65 JamesH65 added the Waiting for external input Waiting for a comment from the originator of the issue, or a collaborator. label May 19, 2017
@gohai
Copy link
Contributor Author

gohai commented May 21, 2017

@JamesH65 I've submitted it to linux-pwm, in the proper format, and with the right folks in CC, but haven't heard back so far.

@pelwell
Copy link
Contributor

pelwell commented May 21, 2017

I think they're quite busy with the 4.12 release candidates - I also have patches waiting for review.

@gohai
Copy link
Contributor Author

gohai commented May 21, 2017

@pelwell Thanks for the heads-up. I'll keep an eye on the mailing list and will report here once it is in some tree...

@lategoodbye
Copy link
Contributor

lategoodbye commented May 30, 2017

@gohai I think your upstream patch got lost. I suggest to resend the patch and also add linux-arm-kernel (moderated list) to CC in order to increase the chance of an reviewer. Another approach is to ask about this topic on kernelnewbies mailing list or IRC, where you get a much fast reply.

@gohai
Copy link
Contributor Author

gohai commented Jun 2, 2017

@lategoodbye Just re-sent it, with CC to linux-arm-kernel and linux-rpi-kernel for what it's worth. I dropped by on kernelnewbies IRC, there the suggestion was to just be more insistent (I was told the submission looks fine).

@lategoodbye
Copy link
Contributor

I've seen Thierry on the mailing list, so maybe you should send a gentle ping. Otherwise the merge window is still open.

@gohai
Copy link
Contributor Author

gohai commented Jul 11, 2017

@lategoodbye I've sent it three times already - anyone, feel free to pick this up yourself, but I am wholeheartedly discouraged from further attempting to make a contribution at this point...

@JamesH65
Copy link
Contributor

@gohai @pelwell Was there any progress on this?

@pelwell
Copy link
Contributor

pelwell commented Sep 13, 2017

I can see that the patch got no response upstream, which is very disappointing. @gohai - give it one more try, wait a week, and if you get no response I'll apply it downstream and maintain it myself.

@lategoodbye
Copy link
Contributor

As long as the merge window is open, any attempt would be pointless. We must wait until 4.14rc-1 is released.

@pelwell
Copy link
Contributor

pelwell commented Sep 13, 2017

Why pointless? Do all reviewers become mute during the merge window? An indication of whether the submitter is on the right lines or doomed to failure isn't a lot to ask for.

@lategoodbye
Copy link
Contributor

I think there was enough time to review, so it's all about merging. Some maintainers ignore patches within the merge window.

@pelwell
Copy link
Contributor

pelwell commented Sep 13, 2017

It is unreasonable to expect submitters to know about merge windows. Since the original submission, 4.13 gone mainstream and now we are waiting for 4.14 rc1.

@thierryreding Should @gohai resubmit his patch or is it already in the pipeline somewhere?

@larsks
Copy link
Contributor

larsks commented Sep 26, 2017

For folks who find themselves here via google, note that the udev rules suggested in this issue only solve half the problem: while they will result in correct ownership and permissions on /sys/class/pwm and friends, they will not actually apply to the new sysfs entries created when you export a pwm (via for example echo 1 > /sys/class/pwm/pwmchip0/export). Those files will still be owned by root:root .

@gohai
Copy link
Contributor Author

gohai commented Sep 26, 2017

@larsks The necessary kernel patch is linked in the first sentence of the initial message in this issue.

@thierryreding
Copy link
Contributor

@gohai best to resubmit this patch. I don't remember seeing it before and I can't find anything that looks related in my inbox. Maybe it was lost along the way? Sorry.

@gohai
Copy link
Contributor Author

gohai commented Sep 26, 2017

@thierryreding I just sent it again, subject should be [RESEND][PATCH] pwm: Set class for exported channels in sysfs.

Might have not made it onto the mailing lists this time, because after 45 minutes I still couldn't get git send-email work with my GMail address. Oh well...

@pelwell
Copy link
Contributor

pelwell commented Sep 26, 2017

I just received it.

@gohai
Copy link
Contributor Author

gohai commented Oct 8, 2017

@thierryreding Have you received my patch? The resend made it to linux-pwm...

@gohai
Copy link
Contributor Author

gohai commented Nov 26, 2017

Pinging @thierryreding

@JamesH65
Copy link
Contributor

JamesH65 commented Dec 4, 2017

Would be good to get some sort of closure on this one.... @thierryreding @gohai @pelwell

@pelwell
Copy link
Contributor

pelwell commented Dec 4, 2017

There's shortage of time, and then there's obstruction.

@gohai If you submit a PR against 4.9 (and 4.14 if it takes more than a simple cherry-pick) I'll apply it downstream.

@6by9
Copy link
Contributor

6by9 commented Dec 5, 2017

Response on the mailing list that it has been applied to for-next - http://lists.infradead.org/pipermail/linux-rpi-kernel/2017-December/007193.html

@pelwell
Copy link
Contributor

pelwell commented Dec 5, 2017

I've cherry-picked - just compile-testing now.

@gohai
Copy link
Contributor Author

gohai commented Dec 5, 2017

@6by9 @pelwell @lategoodbye Fantastic. Thanks everyone for your support!

@pelwell
Copy link
Contributor

pelwell commented Dec 5, 2017

All merged now - thanks everyone.

@pelwell pelwell closed this as completed Dec 5, 2017
@tarikgraba
Copy link

@pelwell The patch seems to cause some issues and was marked to be reverted
https://www.spinics.net/lists/linux-pwm/msg07577.html
So maybe the issue is still here

@pelwell
Copy link
Contributor

pelwell commented Jan 18, 2019

You're right - it was reverted (c289d66) and replaced (552c02e) in 4.20. I've backported those commits to rpi-4.14.y and rpi-4.19.y.

Thanks for the pointer.

@tarikgraba
Copy link

👍

pgrawehr added a commit to pgrawehr/iot that referenced this issue Nov 16, 2019
This file was suggested here and a corresponding
linux kernel patch was implemented to make it work:
raspberrypi/linux#1983

Copy this file to /etc/udev/rules.d and reboot.
pgrawehr added a commit to pgrawehr/iot that referenced this issue Nov 16, 2019
This file was suggested here and a corresponding
linux kernel patch was implemented to make it work:
raspberrypi/linux#1983

Copy this file to /etc/udev/rules.d and reboot.
@pgrawehr
Copy link

@gohai @pelwell I've tested this and it works, but do you know why the proposed rule change is not part of the default Raspbian installation? The gpio pins can meanwhile be accessed without root rights, so PWM usage should be possible as well, or is there any additional security risk involved in doing so?

@pelwell
Copy link
Contributor

pelwell commented Nov 21, 2019

Probably because nobody explicitly asked for it in the right place - try submitting a Pull Request to the raspberrypi-sys-mods repo.

@XECDesign
Copy link
Contributor

Request noted and I've put it on my todo list.

@pgrawehr
Copy link

Cool.
Note that while the suggested script works, it has a downside: It requires the client to introduce a sleep or a retry between writing the export file and opening the pwm0 subdir afterwards. Not sure whether it is possible to avoid that.

@pelwell
Copy link
Contributor

pelwell commented Nov 21, 2019

That's down to the asynchronous nature of sysfs, and is essentially unavoidable. The problem was solved for GPIOs by deprecating the sysfs interfacing and providing a much better ioctl-based interface (gpiolib) instead.

@pgrawehr
Copy link

:( I had expected something like that. If this is a known issue then, the workaround will hopefully be acceptable on the client side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting for external input Waiting for a comment from the originator of the issue, or a collaborator.
Projects
None yet
Development

No branches or pull requests

10 participants