[RFC][PATCH] epoll: allow EPOLLWAKEUP flag if PM_SLEEP is enabled

[RFC][PATCH] epoll: allow EPOLLWAKEUP flag if PM_SLEEP is enabled

[Amit Pundir]

I stumbled upon ENOMEM error from epoll_ctl() while bringing up Android-4.4 
on a device that does not yet support PM_SLEEP.

While looking into the problem, I found that ep_create_wakeup_source()
reports ENOMEM if wakeup_source_register() returns NULL.
ep_create_wakeup_source() assumes that NULL is only returned if we run
into ENOMEM but NULL is also returned when CONFIG_PM_SLEEP is disabled.

If CONFIG_PM_SLEEP is disabled, stripping the EPOLLWAKEUP flag seems to
be a reasonable solution here, allowing the call to succeed, while
dropping the wakeup logic.  While returning EINVAL might also be a good
solution, stripping the flag seems to follow the established behavior,
as is done when the process doesn't have sufficient capabilities to
block suspend.

I'd appreciate any thoughts or feedback!

Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
---
 fs/eventpoll.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 473e09d..7a83079 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1820,7 +1820,8 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, fd,
 		goto error_tgt_fput;
 
 	/* Check if EPOLLWAKEUP is allowed */
-	if ((epds.events & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND))
+	if ((epds.events & EPOLLWAKEUP) &&
+		(!capable(CAP_BLOCK_SUSPEND) || !IS_ENABLED(CONFIG_PM_SLEEP)))
 		epds.events &= ~EPOLLWAKEUP;
 
 	/*
-- 
1.7.10.4

Re: [RFC][PATCH] epoll: allow EPOLLWAKEUP flag if PM_SLEEP is enabled

[Rafael J. Wysocki]

On Monday, November 11, 2013 11:26:31 PM Amit Pundir wrote:
> I stumbled upon ENOMEM error from epoll_ctl() while bringing up Android-4.4 
> on a device that does not yet support PM_SLEEP.

Well, Android depends on PM_SLEEP which is a known fact.

> While looking into the problem, I found that ep_create_wakeup_source()
> reports ENOMEM if wakeup_source_register() returns NULL.
> ep_create_wakeup_source() assumes that NULL is only returned if we run
> into ENOMEM but NULL is also returned when CONFIG_PM_SLEEP is disabled.

So the error value should not be ENOMEM.

> If CONFIG_PM_SLEEP is disabled, stripping the EPOLLWAKEUP flag seems to
> be a reasonable solution here, allowing the call to succeed, while
> dropping the wakeup logic.  While returning EINVAL might also be a good
> solution, stripping the flag seems to follow the established behavior,
> as is done when the process doesn't have sufficient capabilities to
> block suspend.

Which is a different thing.

> I'd appreciate any thoughts or feedback!
>
> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
> ---
>  fs/eventpoll.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 473e09d..7a83079 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -1820,7 +1820,8 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, fd,
>  		goto error_tgt_fput;
>  
>  	/* Check if EPOLLWAKEUP is allowed */
> -	if ((epds.events & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND))
> +	if ((epds.events & EPOLLWAKEUP) &&
> +		(!capable(CAP_BLOCK_SUSPEND) || !IS_ENABLED(CONFIG_PM_SLEEP)))

I don't particularly like mixing the checking of CONFIG_ symbols with code like
this.

I'd create a static inline function taking a pointer to epds.events that would
do the

	if ((epds.events & EPOLLWAKEUP) && !capable(CAP_BLOCK_SUSPEND))
 		epds.events &= ~EPOLLWAKEUP;

for CONFIG_PM_SLEEP set and

  		epds.events &= ~EPOLLWAKEUP;

unconditionally for CONFIG_PM_SLEEP unset.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC][PATCH] epoll: allow EPOLLWAKEUP flag if PM_SLEEP is enabled

[John Stultz]

On Mon, Nov 11, 2013 at 1:22 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, November 11, 2013 11:26:31 PM Amit Pundir wrote:
>> While looking into the problem, I found that ep_create_wakeup_source()
>> reports ENOMEM if wakeup_source_register() returns NULL.
>> ep_create_wakeup_source() assumes that NULL is only returned if we run
>> into ENOMEM but NULL is also returned when CONFIG_PM_SLEEP is disabled.
>
> So the error value should not be ENOMEM.

Right, ENOMEM is clearly wrong. I think its just not clear what the
right thing to do is.


>> If CONFIG_PM_SLEEP is disabled, stripping the EPOLLWAKEUP flag seems to
>> be a reasonable solution here, allowing the call to succeed, while
>> dropping the wakeup logic.  While returning EINVAL might also be a good
>> solution, stripping the flag seems to follow the established behavior,
>> as is done when the process doesn't have sufficient capabilities to
>> block suspend.
>
> Which is a different thing.

Well, in the case of the process not having sufficient capabilities,
-EPERM seems like a reasonable return value.  But instead the wakeup
flag is silently dropped.  That's why Amit is asking if dropping the
wakeup flag is also the right approach for the !PM_SLEEP case instead
of returning EINVAL.

Dropping the wakeup flag is the easier solution, since we don't have
to then go through all the userspace code and have it handle the error
and resubmit without the wakeup flag.  I suspect this was the same
consideration for the decision in the insufficient capabilities case,
but I don't really know.

thanks
-john

Re: [RFC][PATCH] epoll: allow EPOLLWAKEUP flag if PM_SLEEP is enabled

[Rafael J. Wysocki]

On Tuesday, November 12, 2013 11:24:05 AM John Stultz wrote:
> On Mon, Nov 11, 2013 at 1:22 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Monday, November 11, 2013 11:26:31 PM Amit Pundir wrote:
> >> While looking into the problem, I found that ep_create_wakeup_source()
> >> reports ENOMEM if wakeup_source_register() returns NULL.
> >> ep_create_wakeup_source() assumes that NULL is only returned if we run
> >> into ENOMEM but NULL is also returned when CONFIG_PM_SLEEP is disabled.
> >
> > So the error value should not be ENOMEM.
> 
> Right, ENOMEM is clearly wrong. I think its just not clear what the
> right thing to do is.
> 
> 
> >> If CONFIG_PM_SLEEP is disabled, stripping the EPOLLWAKEUP flag seems to
> >> be a reasonable solution here, allowing the call to succeed, while
> >> dropping the wakeup logic.  While returning EINVAL might also be a good
> >> solution, stripping the flag seems to follow the established behavior,
> >> as is done when the process doesn't have sufficient capabilities to
> >> block suspend.
> >
> > Which is a different thing.
> 
> Well, in the case of the process not having sufficient capabilities,
> -EPERM seems like a reasonable return value.  But instead the wakeup
> flag is silently dropped.  That's why Amit is asking if dropping the
> wakeup flag is also the right approach for the !PM_SLEEP case instead
> of returning EINVAL.
> 
> Dropping the wakeup flag is the easier solution, since we don't have
> to then go through all the userspace code and have it handle the error
> and resubmit without the wakeup flag.  I suspect this was the same
> consideration for the decision in the insufficient capabilities case,
> but I don't really know.

Yes, it was.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.