Re: [RFC v3 01/12] rtw88: main files

[Kalle Valo <kvalo@codeaurora.org>]

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