Re: [PATCH rtw-next v3] wifi: rtw88: wake the TX queues after ieee80211_restart_hw()

[Abdurrahman Karadag <abdurrahmankaradag19@gmail.com>]

Hi Luka,

Thank you for reading it this closely.

> The same callback runs with IEEE80211_RECONFIG_TYPE_SUSPEND after a plain
> resume, and the wake is skipped there. [...] Is that reachable, or is it
> released somewhere I missed?

It is reachable, and you did not miss anything. I traced it after your
mail:

  __ieee80211_suspend() stops every queue with
  IEEE80211_QUEUE_STOP_REASON_SUSPEND, then calls
  ieee80211_stop_device(), which is drv_stop() -> rtw_ops_stop() ->
  rtw_core_stop() -> rtw_power_off() -> rtw_hci_stop() ->
  rtw_pci_stop() -> rtw_pci_dma_release() -> rtw_pci_reset_trx_ring().
  So yes, the ring is rewound on the way down, exactly as in the restart
  case.

  On resume ieee80211_reconfig() runs with suspended set, so the
  RECONFIG_TYPE_RESTART call is skipped - its condition is
  "!suspended || reconfig_due_to_wowlan" - and the SUSPEND one is made
  instead. The only wake it does by itself is
  ieee80211_wake_queues_by_reason(..., SUSPEND, ...). Nothing releases
  the DRIVER reason.

So a DRIVER stop that is still set when the system suspends survives the
resume, and v3 does not cover it.

One thing that narrows it in practice. After the SUSPEND stop nothing
new is submitted, and the interrupts stay live until rtw_pci_stop()
disables them - which it does before rtw_pci_dma_release(), with a
synchronize_irq() in between. So on a healthy ring the outstanding
descriptors complete in that window, rtw_pci_tx_isr() runs and clears
both ring->queue_stopped and the DRIVER reason on its own. It only
survives when the hardware has already stopped consuming descriptors -
the same precondition as the case this patch is about. That makes it
narrow, not unreachable, and I would rather it were handled than argued
away.

It also bears on the flow Ping-Ke sketched in the v2 thread: clearing
ring->queue_stopped in rtw_pci_dma_release() and waking from
rtw_reconfig_complete(). His "(but not call ieee80211_wake_queues() if
restarting)" reads two ways to me - either the reset gets a wake beside
it for the callers that are not a restart, or the reset stands alone
and the wake lives only in step 3 - and your case turns on which he
meant. The suspend path reaches rtw_pci_dma_release() through
rtw_ops_stop() as well, so a wake there would cover it; a wake only in
step 3 would not, since a resume gets RECONFIG_TYPE_SUSPEND. I have
asked him in that thread, with your point attached.

> ieee80211_wake_queues() releases every queue with the driver reason, not
> just the one the ring-full path stopped, and the offload scan and PCIe
> error paths use that reason as well. I think the first sentence reads
> narrower than it is.

You are right, and that is my fault - v2 said this and I cut it when I
trimmed the message. rtw_hw_scan_start(), rtw_hw_scan_chan_switch() and
rtw_pci_io_err_detected() all stop queues with the same non-refcounted
DRIVER reason, so this wake cannot tell them apart. I will put it back in
the next version, along the lines of:

  Wake every queue holding IEEE80211_QUEUE_STOP_REASON_DRIVER. mac80211
  keeps one non-refcounted slot per queue for that reason, which the
  offload scan and PCIe error paths share, so this cannot be narrowed to
  the queue the ring-full path stopped.

I am holding the next version until Ping-Ke comes back with the flow he
wants, so your two points will land in the same respin.

Best regards,
Abdurrahman