Message ID | 20171123183914.71769-1-andriy.shevchenko@linux.intel.com |
---|---|
State | Not Applicable |
Headers | show |
Series | [v1] busybox: Fix rtcwake to use /dev/rtc0 properly | expand |
Hi Andy, On 23-11-17 19:39, Andy Shevchenko wrote: > rtcwake from busybox has failed in case the /dev/rtc is a symlink > (which is default case for udev enabled systems) due to wrong pathname > used for a sysfs wakeup attribute. In Buildroot, we don't accept "feature patches" for packages. We try to limit to patches that fix the build or complete breakage, sometimes also to make it work together with other packages. I think this patch doesn't fall in that category. For sure, you should first send the patch upstream. Particularly in the case of busybox, Denys often proposes improved patches. If it gets accepted upstream, then we can consider including it in Buildroot as well. Regards, Arnout > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > ...bb-Try-dev-rtc0-first-followed-by-dev-rtc.patch | 45 ++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > create mode 100644 package/busybox/0006-libbb-Try-dev-rtc0-first-followed-by-dev-rtc.patch > > diff --git a/package/busybox/0006-libbb-Try-dev-rtc0-first-followed-by-dev-rtc.patch b/package/busybox/0006-libbb-Try-dev-rtc0-first-followed-by-dev-rtc.patch > new file mode 100644 > index 0000000000..0702c3c065 > --- /dev/null > +++ b/package/busybox/0006-libbb-Try-dev-rtc0-first-followed-by-dev-rtc.patch > @@ -0,0 +1,45 @@ > +From b59088ab14fb9c336414e2c9b27a9f5b7447ce5b Mon Sep 17 00:00:00 2001 > +From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > +Date: Thu, 23 Nov 2017 19:33:08 +0200 > +Subject: [PATCH v1] libbb: Try /dev/rtc0 first followed by /dev/rtc > + > +In case we run > + > +% rtcwake -s5 -mmem > + > +in udev environment we will get an error: > + > +rtcwake: /dev/rtc not enabled for wakeup events > + > +because /dev/rtc is a symlink to /dev/rtc0 in Buildroot and other Linux > +distributions (like Fedora) and make_wakeup() obviously fails. > + > +As a quick fix just reorder probe of device nodes to try /dev/rtc0 > +first. > + > +The proper solution, of course much expensive by code footprint, > +is to resolve /dev/rtc to a proper device node and then > +to follow sysfs attributes. > + > +Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > +--- > + libbb/rtc.c | 2 +- > + 1 file changed, 1 insertion(+), 1 deletion(-) > + > +diff --git a/libbb/rtc.c b/libbb/rtc.c > +index c4117ba34..e087a8fd6 100644 > +--- a/libbb/rtc.c > ++++ b/libbb/rtc.c > +@@ -64,8 +64,8 @@ int FAST_FUNC rtc_xopen(const char **default_rtc, int flags) > + { > + int rtc; > + const char *name = > +- "/dev/rtc""\0" > + "/dev/rtc0""\0" > ++ "/dev/rtc""\0" > + "/dev/misc/rtc""\0"; > + > + if (!*default_rtc) > +-- > +2.15.0 > + >
On Thu, 2017-11-23 at 23:46 +0100, Arnout Vandecappelle wrote: > Hi Andy, > > On 23-11-17 19:39, Andy Shevchenko wrote: > > rtcwake from busybox has failed in case the /dev/rtc is a symlink > > (which is default case for udev enabled systems) due to wrong > > pathname > > used for a sysfs wakeup attribute. > > In Buildroot, we don't accept "feature patches" for packages. To be honest it's not a feature patch at all. It fixes (okay, workarounds) obvious bug in rtcwake logic. Easy to reproduce. 100% reproducible. > We try to limit > to patches that fix the build or complete breakage, sometimes also to > make it > work together with other packages. I think this patch doesn't fall in > that category. Whatever, your choice at the end. > > For sure, you should first send the patch upstream. Are you sure I didn't? The policy of Busybox mailing list is to reject (I'm not subscriber and after a such policy would not like to be one). Happy contribution! > Particularly in the case of > busybox, Denys often proposes improved patches. If it gets accepted > upstream, > then we can consider including it in Buildroot as well. Good luck! I'm done with it. If Denys is caring about project he will take the series (there are more patches than just one) from his private mailbox (Cc was there as well).
Hi Andy, Sorry, I should have started my mail with: Thank you very much for your contributation. I'm afraid, however, we will not accept it as is in Buildroot. On 24-11-17 14:27, Andy Shevchenko wrote: > On Thu, 2017-11-23 at 23:46 +0100, Arnout Vandecappelle wrote: >> Hi Andy, >> >> On 23-11-17 19:39, Andy Shevchenko wrote: >>> rtcwake from busybox has failed in case the /dev/rtc is a symlink >>> (which is default case for udev enabled systems) due to wrong >>> pathname >>> used for a sysfs wakeup attribute. >> >> In Buildroot, we don't accept "feature patches" for packages. > > To be honest it's not a feature patch at all. It fixes (okay, > workarounds) obvious bug in rtcwake logic. Easy to reproduce. 100% > reproducible Yes, that's why I continued with: >> We try to limit >> to patches that fix the build or complete breakage, sometimes also to ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ For other bugs, we will happily include upstream patches. But patches that are not yet upstream are dangerous: how can we be sure that it doesn't break things? In this particular case, you patch *will* actually break things for a who does have a working /dev/rtc but it is something different than /dev/rtc0. We want upstream to take the responsibility for that. >> make it >> work together with other packages. I think this patch doesn't fall in >> that category. > > Whatever, your choice at the end. > >> >> For sure, you should first send the patch upstream. > > Are you sure I didn't? I checked the mailing list, it wasn't there. I checked google, and found nothing. > The policy of Busybox mailing list is to reject (I'm not subscriber and Buildroot mailing list has the same policy (well, it goes into a moderation queue but that is filled up with so much spam that it's unlikely it ever makes it to the list). If you have a solution to bar spam from mailing lists, please share it! > after a such policy would not like to be one). Happy contribution! > >> Particularly in the case of >> busybox, Denys often proposes improved patches. If it gets accepted >> upstream, >> then we can consider including it in Buildroot as well. > > Good luck! > > I'm done with it. If Denys is caring about project he will take the > series (there are more patches than just one) from his private mailbox > (Cc was there as well). If/when he does, feel free to resubmit with a reference to the upstream commit. Thanks! Regards, Arnout
On Sat, 2017-11-25 at 18:40 +0100, Arnout Vandecappelle wrote: > Hi Andy, > > Sorry, I should have started my mail with: > > Thank you very much for your contributation. I'm afraid, however, we > will not > accept it as is in Buildroot. Much better :-) > > > In Buildroot, we don't accept "feature patches" for packages. > > > > To be honest it's not a feature patch at all. It fixes (okay, > > workarounds) obvious bug in rtcwake logic. Easy to reproduce. 100% > > reproducible > > Yes, that's why I continued with: > > > > We try to limit > > > to patches that fix the build or complete breakage, sometimes also > > > to > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > For other bugs, we will happily include upstream patches. But patches > that are > not yet upstream are dangerous: how can we be sure that it doesn't > break things? > In this particular case, you patch *will* actually break things for a > who does > have a working /dev/rtc but it is something different than /dev/rtc0. > We want > upstream to take the responsibility for that. Of course. Btw, how many use cases where people use rtcwake from busybox vs. util- linux? I doubt there are many. > > > For sure, you should first send the patch upstream. > > > > Are you sure I didn't? > > I checked the mailing list, it wasn't there. I checked google, and > found nothing. > > > The policy of Busybox mailing list is to reject (I'm not subscriber > > and > > Buildroot mailing list has the same policy (well, it goes into a > moderation > queue but that is filled up with so much spam that it's unlikely it > ever makes > it to the list). Exactly! Not reject, but put in to moderation queue. Feel the difference! > If you have a solution to bar spam from mailing lists, please > share it! See above. > > after a such policy would not like to be one). Happy contribution! > > > > > Particularly in the case of > > > busybox, Denys often proposes improved patches. If it gets > > > accepted > > > upstream, > > > then we can consider including it in Buildroot as well. > > > > Good luck! > > > > I'm done with it. If Denys is caring about project he will take the > > series (there are more patches than just one) from his private > > mailbox > > (Cc was there as well). > > If/when he does, feel free to resubmit with a reference to the > upstream commit. OK.
diff --git a/package/busybox/0006-libbb-Try-dev-rtc0-first-followed-by-dev-rtc.patch b/package/busybox/0006-libbb-Try-dev-rtc0-first-followed-by-dev-rtc.patch new file mode 100644 index 0000000000..0702c3c065 --- /dev/null +++ b/package/busybox/0006-libbb-Try-dev-rtc0-first-followed-by-dev-rtc.patch @@ -0,0 +1,45 @@ +From b59088ab14fb9c336414e2c9b27a9f5b7447ce5b Mon Sep 17 00:00:00 2001 +From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> +Date: Thu, 23 Nov 2017 19:33:08 +0200 +Subject: [PATCH v1] libbb: Try /dev/rtc0 first followed by /dev/rtc + +In case we run + +% rtcwake -s5 -mmem + +in udev environment we will get an error: + +rtcwake: /dev/rtc not enabled for wakeup events + +because /dev/rtc is a symlink to /dev/rtc0 in Buildroot and other Linux +distributions (like Fedora) and make_wakeup() obviously fails. + +As a quick fix just reorder probe of device nodes to try /dev/rtc0 +first. + +The proper solution, of course much expensive by code footprint, +is to resolve /dev/rtc to a proper device node and then +to follow sysfs attributes. + +Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> +--- + libbb/rtc.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/libbb/rtc.c b/libbb/rtc.c +index c4117ba34..e087a8fd6 100644 +--- a/libbb/rtc.c ++++ b/libbb/rtc.c +@@ -64,8 +64,8 @@ int FAST_FUNC rtc_xopen(const char **default_rtc, int flags) + { + int rtc; + const char *name = +- "/dev/rtc""\0" + "/dev/rtc0""\0" ++ "/dev/rtc""\0" + "/dev/misc/rtc""\0"; + + if (!*default_rtc) +-- +2.15.0 +
rtcwake from busybox has failed in case the /dev/rtc is a symlink (which is default case for udev enabled systems) due to wrong pathname used for a sysfs wakeup attribute. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- ...bb-Try-dev-rtc0-first-followed-by-dev-rtc.patch | 45 ++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 package/busybox/0006-libbb-Try-dev-rtc0-first-followed-by-dev-rtc.patch