LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alex Elder <elder@linaro.org>
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
Subject: Re: [PATCH net-next 3/6] net: ipa: re-enable transmit in PM WQ context
Date: Fri, 13 Aug 2021 21:32:30 -0500	[thread overview]
Message-ID: <1e17b9a9-4f04-bae7-b113-26c1944abbfc@linaro.org> (raw)
In-Reply-To: <20210813174406.5e7fc350@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

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

  reply	other threads:[~2021-08-14  2:32 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-12 19:50 [PATCH net-next 0/6] net: ipa: last things before PM conversion Alex Elder
2021-08-12 19:50 ` [PATCH net-next 1/6] net: ipa: enable wakeup in ipa_power_setup() Alex Elder
2021-08-12 19:50 ` [PATCH net-next 2/6] net: ipa: distinguish system from runtime suspend Alex Elder
2021-08-12 19:50 ` [PATCH net-next 3/6] net: ipa: re-enable transmit in PM WQ context Alex Elder
2021-08-14  0:44   ` Jakub Kicinski
2021-08-14  2:32     ` Alex Elder [this message]
2021-08-12 19:50 ` [PATCH net-next 4/6] net: ipa: ensure hardware has power in ipa_start_xmit() Alex Elder
2021-08-14  0:46   ` Jakub Kicinski
2021-08-14  2:25     ` Alex Elder
2021-08-16 14:15       ` Jakub Kicinski
2021-08-16 14:20         ` Alex Elder
2021-08-16 17:56           ` Alex Elder
2021-08-16 20:19             ` Jakub Kicinski
2021-08-12 19:50 ` [PATCH net-next 5/6] net: ipa: don't stop TX on suspend Alex Elder
2021-08-12 19:50 ` [PATCH net-next 6/6] net: ipa: don't hold clock reference while netdev open Alex Elder
2021-08-14 13:20 ` [PATCH net-next 0/6] net: ipa: last things before PM conversion patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1e17b9a9-4f04-bae7-b113-26c1944abbfc@linaro.org \
    --to=elder@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=cpratapa@codeaurora.org \
    --cc=davem@davemloft.net \
    --cc=elder@kernel.org \
    --cc=evgreen@chromium.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=subashab@codeaurora.org \
    --subject='Re: [PATCH net-next 3/6] net: ipa: re-enable transmit in PM WQ context' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).