On Fri, Jan 29, 2021 at 3:29 PM Alex Elder <el...@linaro.org> wrote: > > The channel stop and suspend paths both call __gsi_channel_stop(), > which quiesces channel activity, disables NAPI, and (on other than > SDM845) stops the channel. Similarly, the start and resume paths > share __gsi_channel_start(), which starts the channel and re-enables > NAPI again. > > Disabling NAPI should be done when stopping a channel, but this > should *not* be done when suspending. It's not necessary in the > suspend path anyway, because the stopped channel (or suspended > endpoint on SDM845) will not cause interrupts to schedule NAPI, > and gsi_channel_trans_quiesce() won't return until there are no > more transactions to process in the NAPI polling loop.
But why is it incorrect to do so? >From a quick look, virtio-net disables on both remove and freeze, for instance. > Instead, enable NAPI in gsi_channel_start(), when the completion > interrupt is first enabled. Disable it again in gsi_channel_stop(), > when finally disabling the interrupt. > > Add a call to napi_synchronize() to __gsi_channel_stop(), to ensure > NAPI polling is done before moving on. > > Signed-off-by: Alex Elder <el...@linaro.org> > --- = > @@ -894,12 +894,16 @@ int gsi_channel_start(struct gsi *gsi, u32 channel_id) > struct gsi_channel *channel = &gsi->channel[channel_id]; > int ret; > > - /* Enable the completion interrupt */ > + /* Enable NAPI and the completion interrupt */ > + napi_enable(&channel->napi); > gsi_irq_ieob_enable_one(gsi, channel->evt_ring_id); > > ret = __gsi_channel_start(channel, true); > - if (ret) > - gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id); > + if (!ret) > + return 0; > + > + gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id); > + napi_disable(&channel->napi); > > return ret; > } subjective, but easier to parse when the normal control flow is linear and the error path takes a branch (or goto, if reused).
