Re: Proper suspend/resume flow

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Thu, Mar 27, 2014 at 12:10:33PM +0200, Claudiu Manoil wrote:
> On 3/26/2014 1:58 PM, Russell King - ARM Linux wrote:
>> I think the right solution for a driver which does all its processing in
>> the NAPI poll function would be:
>>
>> 	if (netif_running(ndev)) {
>> 		napi_disable(napi);
>> 		netif_device_detach(ndev);
>> 		disable_hardware();
>> 	}
>>
>> and upon resume, the reverse order.  The theory being:
>>
>> - napi_disable() is to stop the driver processing its rings and possibly
>>    waking up the transmit queue(s) after the following netif_device_detach().
>> - netif_device_detach() to stop new packets being queued to the device.
>
>
> There's a risk to disabling the NAPI before stopping the transmission
> (i.e. stopping the Tx queues).  The net stack might continue to enqueue
> Tx packets and, if the Tx BD rings get full, the driver's start_xmit
> will return TX_BUSY which leads to netdev watchdog triggering Tx
> timeout (and the ndo_tx_timeout hook from the driver will re-enable the
> Tx queues).

Part of netif_device_detach()'s work is to stop the transmit queues in a
similar manner to the driver reporting "ring full", but before doing so,
it also marks the device not present.

The watchdog is conditional upon netif_device_present() returning true,
but as the device has been marked not-present, this test will fail.

When the device is resumed, and netif_device_attach() is called,
__netdev_watchdog_up() will also be called which reschedules the watchdog
for a new timeout grace period.

So I don't think that's an issue - I think we're safe from the watchdog
triggering.

> Maybe it's unlikely to get the BD rings full during suspend, but there's
> another case too - BQL (if the driver supports BQL).  BQL needs a
> confirmation for each xmitted byte, and the confirmation comes from
> Tx completion processing from NAPI context (see
> netdev_tx_completed_queue()).  If the confirmation is delayed too much,
> BQL blocks the Tx queues to trigger the same netdev Tx timeout watchdog
> mechanism.

One of the things FEC does when it resumes is to clean the transmit queue,
which with BQL ends up resetting the BQL counts (via netdev_reset_queue()).
Maybe that's something which should happen on the suspend path to clean
everything up in preparation?

> I think there's a more general problem to this: how to properly stop
> the Tx traffic from the driver.  And I think an approach to solve it
> would be to stop the Tx queues (i.e. netif_tx_stop_all_queues()) before
> disabling the NAPI processing, however the driver will need to use a
> special state flag (like "DOWN" or "RESETTING") to prevent waking up
> the Tx queues from NAPI while the Tx traffic is being stopped.
> There are some drivers implementing such "synchronization" flags to
> prevent the Tx congestion mechanism to wake up the Tx queues while the
> device is resetting or brought down.  But there's no generic
> implementation for this (there are differences in implementation from
> driver to driver).

If what I've said above is correct, then I don't think any of that is
necessary, and the sequence I mentioned in my initial email should work
fine.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.