DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A3B862145D
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org C42B260386
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     Johannes Berg <johannes@sipsolutions.net>,
        "Larry.Finger\@lwfinger.net" <Larry.Finger@lwfinger.net>,
        Pkshih <pkshih@realtek.com>, Andy Huang <tehuang@realtek.com>,
        "sgruszka\@redhat.com" <sgruszka@redhat.com>,
        "linux-wireless\@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [RFC v3 01/12] rtw88: main files
References: <1538565659-29530-1-git-send-email-yhchuang@realtek.com>
        <1538565659-29530-2-git-send-email-yhchuang@realtek.com>
        <201810081447.w98ElQfu018110@rtits1.realtek.com.tw>
        <F7CD281DE3E379468C6D07993EA72F84D172DCB6@RTITMBSVM01.realtek.com.tw>
        <87lg7120xq.fsf@kamboji.qca.qualcomm.com>
        <F7CD281DE3E379468C6D07993EA72F84D1734F60@RTITMBSVM01.realtek.com.tw>
Date:   Thu, 15 Nov 2018 16:18:21 +0200
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D1734F60@RTITMBSVM01.realtek.com.tw>
        (Tony Chuang's message of "Mon, 22 Oct 2018 03:40:27 +0000")
Message-ID: <87zhuaa0gi.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Tony Chuang <yhchuang@realtek.com> writes:

>> -----Original Message-----
>> From: Kalle Valo [mailto:kvalo@codeaurora.org]
>> Sent: Sunday, October 14, 2018 1:48 AM
>> To: Tony Chuang
>> Cc: Johannes Berg; Larry.Finger@lwfinger.net; Pkshih; Andy Huang;
>> sgruszka@redhat.com; linux-wireless@vger.kernel.org
>> Subject: Re: [RFC v3 01/12] rtw88: main files
>> 
>> Tony Chuang <yhchuang@realtek.com> writes:
>> 
>> >> > +static void rtw_watch_dog_work(struct work_struct *work)
>> >> > +{
>> >> > +	struct rtw_dev *rtwdev = container_of(work, struct rtw_dev,
>> >> > +					      watch_dog_work.work);
>> >> > +	struct rtw_vif *rtwvif;
>> >> > +
>> >> > +	if (!rtw_flag_check(rtwdev, RTW_FLAG_RUNNING))
>> >> > +		return;
>> >> > +
>> >> > +	ieee80211_queue_delayed_work(rtwdev->hw,
>> >> &rtwdev->watch_dog_work,
>> >> > +				     RTW_WATCH_DOG_DELAY_TIME);
>> >>
>> >> You're aware of the power cost of waking up every 2 seconds? That's a
>> >> really bad idea, in general, at the very least you should use a more
>> >> power efficient scheduling here to combine with other wakeups
>> >> (round_jiffies_relative, or so).
>> >
>> > Yeah I knew it, but so far we can only work like this...
>> > Will use round_jiffies_relative to combine the CPU wakeups.
>> 
>> Can you elaborate more why this horrible timer is needed? And it
>> definitely needs a comment in the code explaining the reason.
>> 
>
>
> The watchdog timer is required for our devices to enhance the performance.
> It does a lot of tx/rx statistics processing for the hardware.
> Those information process routines help the devices to adapt to the environment.
>
> However, status polling every two seconds is not a good solution.
> But it makes drive simpler to be implemented.
>
> We will try to change it to interrupt mode.
> But it will take a lot of time to work on it.
> So, before it's done, I think we can leave the timer here.

Yeah, interrupt mode sounds like a much better idea. But if you have to
keep two second polling at least add a proper comment to the code
explaining what you said above.

-- 
Kalle Valo