Tony Chuang <yhchuang@xxxxxxxxxxx> writes: >> -----Original Message----- >> From: Kalle Valo [mailto:kvalo@xxxxxxxxxxxxxx] >> Sent: Sunday, October 14, 2018 1:48 AM >> To: Tony Chuang >> Cc: Johannes Berg; Larry.Finger@xxxxxxxxxxxx; Pkshih; Andy Huang; >> sgruszka@xxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx >> Subject: Re: [RFC v3 01/12] rtw88: main files >> >> Tony Chuang <yhchuang@xxxxxxxxxxx> 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
