From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH net] ipv4: igmp: hold wakelock to prevent delayed reports Date: Fri, 1 Jun 2018 07:45:16 -0700 Message-ID: <8661f50b-876f-50d8-f1a5-61b1dabc5398@gmail.com> References: <20180601140532.GA13253@tejaswit-linux.qualcomm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: Andrew Lunn To: Tejaswi Tanikella , netdev@vger.kernel.org Return-path: Received: from mail-qk0-f195.google.com ([209.85.220.195]:34827 "EHLO mail-qk0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752208AbeFAOpV (ORCPT ); Fri, 1 Jun 2018 10:45:21 -0400 Received: by mail-qk0-f195.google.com with SMTP id d130-v6so14653036qkc.2 for ; Fri, 01 Jun 2018 07:45:21 -0700 (PDT) In-Reply-To: <20180601140532.GA13253@tejaswit-linux.qualcomm.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 06/01/2018 07:05 AM, Tejaswi Tanikella wrote: > On receiving a IGMPv2/v3 query, based on max_delay set in the header a > timer is started to send out a response after a random time within > max_delay. If the system then moves into suspend state, Report is > delayed until system wakes up. > > In one reported scenario, on arm64 devices, max_delay was set to 10s, > Reports were consistantly delayed if the timer is scheduled after 5 plus > seconds. > > Hold wakelock while starting the timer to prevent moving into suspend > state. I suppose this looks fine, but are not you going to be playing whack-a-mole through the network stack wherever there are similar patterns? Is not a better solution to move this to in_dev_get()/in_dev_put() where you could create a proper wakelock per network device? For instance, I could imagine ARP suffering from the same short comings... There is one thing that needs fixing though, see below. > > Signed-off-by: Tejaswi Tanikella > --- > include/linux/igmp.h | 1 +b > net/ipv4/igmp.c | 20 ++++++++++++++++++-- > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/include/linux/igmp.h b/include/linux/igmp.h > index f823185..9be1c58 100644 > --- a/include/linux/igmp.h > +++ b/include/linux/igmp.h > @@ -84,6 +84,7 @@ struct ip_mc_list { > }; > struct ip_mc_list __rcu *next_hash; > struct timer_list timer; > + struct wakeup_source *wakeup_src; Since you are using this only when CONFIG_IP_MULTICAST is defined, you might was well save a few bytes by making this enclosed within an ifdef CONFIG_IP_MULTICAST here as well? [snip] > @@ -1415,6 +1429,8 @@ void ip_mc_inc_group(struct in_device *in_dev, __be32 addr) > #ifdef CONFIG_IP_MULTICAST > timer_setup(&im->timer, igmp_timer_expire, 0); > im->unsolicit_count = net->ipv4.sysctl_igmp_qrv; > + im->wakeup_src = wakeup_source_create("igmp_wakeup_source"); Missing error checking, wakeup_source_create() can return NULL here. -- Florian