Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH net-next 3/6] net: ipa: re-enable transmit in PM WQ
 context
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     davem@davemloft.net, bjorn.andersson@linaro.org,
        evgreen@chromium.org, cpratapa@codeaurora.org,
        subashab@codeaurora.org, elder@kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20210812195035.2816276-1-elder@linaro.org>
 <20210812195035.2816276-4-elder@linaro.org>
 <20210813174406.5e7fc350@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
From:   Alex Elder <elder@linaro.org>
Message-ID: <1e17b9a9-4f04-bae7-b113-26c1944abbfc@linaro.org>
Date:   Fri, 13 Aug 2021 21:32:30 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210813174406.5e7fc350@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/13/21 7:44 PM, Jakub Kicinski wrote:
> On Thu, 12 Aug 2021 14:50:32 -0500 Alex Elder wrote:
>> +/**
>> + * ipa_modem_wake_queue_work() - enable modem netdev queue
>> + * @work:	Work structure
>> + *
>> + * Re-enable transmit on the modem network device.  This is called
>> + * in (power management) work queue context, scheduled when resuming
>> + * the modem.
>> + */
>> +static void ipa_modem_wake_queue_work(struct work_struct *work)
>> +{
>> +	struct ipa_priv *priv = container_of(work, struct ipa_priv, work);
>> +
>> +	netif_wake_queue(priv->ipa->modem_netdev);
>> +}
>> +
>>  /** ipa_modem_resume() - resume callback for runtime_pm
>>   * @dev: pointer to device
>>   *
>> @@ -205,7 +226,8 @@ void ipa_modem_resume(struct net_device *netdev)
>>  	ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
>>  	ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
>>  
>> -	netif_wake_queue(netdev);
>> +	/* Arrange for the TX queue to be restarted */
>> +	(void)queue_pm_work(&priv->work);
>>  }
> 
> Why move the wake call to a work queue, tho? It's okay to call it 
> from any context.

The issue isn't about the context in which is run (well, not
really, not in the sense you're talking about).

The issue has to do with the PM ->runtime_resume function
running concurrent with the network ->start_xmit function.

We need the hardware powered in ipa_start_xmit().  So we
call pm_runtime_get(), which will not block and which will
indicate in its return value whether power:  is active
(return is 1); will be active once the resume underway
completes (return is -EINPROGRESS); will be active once
suspend underway and a delayed resume completes (return
is 0); or will be active once the newly-scheduled resume
completes (return is 0, scheduled on PM work queue).
We don't expect any other error, but if we get one we
drop the packet.

If the return value is 1, power is active and we transmit
the packet.  If the return value indicates power is not
active, but will be, we stop the TX queue.  No other packets
should be passed to ->start_xmit until TX is started again.

We wish to restart the TX queue when the ipa_runtime_resume()
completes. Here is the call path:

    ipa_runtime_resume()	This is the ->runtime_resume PM op

      ipa_endpoint_resume()

        ipa_modem_resume()

          netif_wake_queue()	Without this patch


The instant netif_wake_queue() is called, we start getting
calls to ipa_start_xmit(), which again attempts to transmit
the SKB that caused the queue to be stopped.  And there is a
good chance that when that is called, the ipa_runtime_resume()
PM callback is still executing, and not complete.  In that case,
we'll *again* get an -EINPROGRESS back from pm_runtime_get() in
ipa_start_xmit(), and we stop the TX queue again.  Basically,
we're stuck.

All we need is for the TX queue to be started *after* the
PM ->runtime_resume callback completes and marks the the
PM runtime status ACTIVE.  Scheduling this on the PM
workqueue ensures this will happen then, if we happen
to be running ipa_runtime_resume() via that workqueue.
If not, there's a bit of a race but it should resolve
(but I think here lies the specific race you mentioned
in the other message).

I'm open to other suggestions, but my hope was to at least
explain why I did it this way.  I'll think about it over
the weekend and will send a new version of the series when
I come up with a solution.

Thank you very much for the review.

					-Alex