From: Alex Elder <hidden> Date: 2021-01-21 11:56:33
Version 1 of this series inadvertently dropped the "static" that
limits the scope of gsi_channel_update(). Version 2 fixes this
(in patch 3).
While reviewing the IPA NAPI polling code in detail I found two
problems. This series fixes those, and implements a few other
improvements to this part of the code.
The first two patches are minor bug fixes that avoid extra passes
through the poll function. The third simplifies code inside the
polling loop a bit.
The last two update how interrupts are disabled; previously it was
possible for another I/O completion condition to be recorded before
NAPI got scheduled.
-Alex
Alex Elder (5):
net: ipa: count actual work done in gsi_channel_poll()
net: ipa: heed napi_complete() return value
net: ipa: have gsi_channel_update() return a value
net: ipa: repurpose gsi_irq_ieob_disable()
net: ipa: disable IEOB interrupts before clearing
drivers/net/ipa/gsi.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)
--
2.20.1
From: Alex Elder <hidden> Date: 2021-01-21 11:50:47
There is an off-by-one problem in gsi_channel_poll(). The count of
transactions completed is incremented each time through the loop
*before* determining whether there is any more work to do. As a
result, if we exit the loop early the counter its value is one more
than the number of transactions actually processed.
Instead, increment the count after processing, to ensure it reflects
the number of processed transactions. The result is more naturally
described as a for loop rather than a while loop, so change that.
Signed-off-by: Alex Elder <redacted>
---
drivers/net/ipa/gsi.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
From: Alex Elder <hidden> Date: 2021-01-21 11:52:07
Have gsi_channel_update() return the first transaction in the
updated completed transaction list, or NULL if no new transactions
have been added.
Signed-off-by: Alex Elder <redacted>
---
v2: Do not drop the static keyword that limits the function scope.
drivers/net/ipa/gsi.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
@@ -1452,7 +1452,7 @@ void gsi_channel_doorbell(struct gsi_channel *channel)}/* Consult hardware, move any newly completed transactions to completed list */-staticvoidgsi_channel_update(structgsi_channel*channel)+staticstructgsi_trans*gsi_channel_update(structgsi_channel*channel){u32evt_ring_id=channel->evt_ring_id;structgsi*gsi=channel->gsi;
@@ -1471,7 +1471,7 @@ static void gsi_channel_update(struct gsi_channel *channel)offset=GSI_EV_CH_E_CNTXT_4_OFFSET(evt_ring_id);index=gsi_ring_index(ring,ioread32(gsi->virt+offset));if(index==ring->index%ring->count)-return;+returnNULL;/* Get the transaction for the latest completed event. Take a*referencetokeepitfromcompletingbeforewegivetheevents
@@ -1516,11 +1518,8 @@ static struct gsi_trans *gsi_channel_poll_one(struct gsi_channel *channel)/* Get the first transaction from the completed list */trans=gsi_channel_trans_complete(channel);-if(!trans){-/* List is empty; see if there's more to do */-gsi_channel_update(channel);-trans=gsi_channel_trans_complete(channel);-}+if(!trans)/* List is empty; see if there's more to do */+trans=gsi_channel_update(channel);if(trans)gsi_trans_move_polled(trans);
From: Alex Elder <hidden> Date: 2021-01-21 11:53:59
Currently in gsi_isr_ieob(), event ring IEOB interrupts are disabled
one at a time. The loop disables the IEOB interrupt for all event
rings represented in the event mask. Instead, just disable them all
at once.
Disable them all *before* clearing the interrupt condition. This
guarantees we'll schedule NAPI for each event once, before another
IEOB interrupt could be signaled.
Signed-off-by: Alex Elder <redacted>
---
drivers/net/ipa/gsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Alex Elder <hidden> Date: 2021-01-21 11:55:17
Pay attention to the return value of napi_complete(), completing
polling only if it returns true.
Just use napi rather than &channel->napi as the argument passed to
napi_complete().
Signed-off-by: Alex Elder <redacted>
---
drivers/net/ipa/gsi.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: Alex Elder <hidden> Date: 2021-01-21 11:56:05
Rename gsi_irq_ieob_disable() to be gsi_irq_ieob_disable_one().
Introduce a new function gsi_irq_ieob_disable() that takes a mask of
events to disable rather than a single event id. This will be used
in the next patch.
Rename gsi_irq_ieob_enable() to be gsi_irq_ieob_enable_one() to be
consistent.
Signed-off-by: Alex Elder <redacted>
---
drivers/net/ipa/gsi.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
@@ -286,11 +286,11 @@ static void gsi_irq_ieob_enable(struct gsi *gsi, u32 evt_ring_id)gsi_irq_type_enable(gsi,GSI_IEOB);}-staticvoidgsi_irq_ieob_disable(structgsi*gsi,u32evt_ring_id)+staticvoidgsi_irq_ieob_disable(structgsi*gsi,u32event_mask){u32val;-gsi->ieob_enabled_bitmap&=~BIT(evt_ring_id);+gsi->ieob_enabled_bitmap&=~event_mask;/* Disable the interrupt type if this was the last enabled channel */if(!gsi->ieob_enabled_bitmap)
@@ -766,13 +771,13 @@ static void gsi_channel_freeze(struct gsi_channel *channel)napi_disable(&channel->napi);-gsi_irq_ieob_disable(channel->gsi,channel->evt_ring_id);+gsi_irq_ieob_disable_one(channel->gsi,channel->evt_ring_id);}/* Allow transactions to be used on the channel again. */staticvoidgsi_channel_thaw(structgsi_channel*channel){-gsi_irq_ieob_enable(channel->gsi,channel->evt_ring_id);+gsi_irq_ieob_enable_one(channel->gsi,channel->evt_ring_id);napi_enable(&channel->napi);}
Hello:
This series was applied to netdev/net-next.git (refs/heads/master):
On Thu, 21 Jan 2021 05:48:16 -0600 you wrote:
Version 1 of this series inadvertently dropped the "static" that
limits the scope of gsi_channel_update(). Version 2 fixes this
(in patch 3).
While reviewing the IPA NAPI polling code in detail I found two
problems. This series fixes those, and implements a few other
improvements to this part of the code.
[...]