| Message ID | 20210203152855.11866-2-elder@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipa: a mix of small improvements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 86 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Wed, 3 Feb 2021 09:28:49 -0600 Alex Elder wrote: > Make __gsi_channel_start() and __gsi_channel_stop() more structurally > and semantically similar to each other: > - Restructure __gsi_channel_start() to always return at the end of > the function, similar to the way __gsi_channel_stop() does. > - Move the mutex calls out of gsi_channel_stop_retry() and into > __gsi_channel_stop(). > > Restructure gsi_channel_stop() to always return at the end of the > function, like gsi_channel_start() does. > > Signed-off-by: Alex Elder <elder@linaro.org> > --- > drivers/net/ipa/gsi.c | 45 +++++++++++++++++++++++-------------------- > 1 file changed, 24 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c > index 53640447bf123..2671b76ebcfe3 100644 > --- a/drivers/net/ipa/gsi.c > +++ b/drivers/net/ipa/gsi.c > @@ -873,17 +873,17 @@ static void gsi_channel_deprogram(struct gsi_channel *channel) > > static int __gsi_channel_start(struct gsi_channel *channel, bool start) > { > - struct gsi *gsi = channel->gsi; > - int ret; > + int ret = 0; > > - if (!start) > - return 0; > + if (start) { > + struct gsi *gsi = channel->gsi; > > - mutex_lock(&gsi->mutex); > + mutex_lock(&gsi->mutex); > > - ret = gsi_channel_start_command(channel); > + ret = gsi_channel_start_command(channel); > > - mutex_unlock(&gsi->mutex); > + mutex_unlock(&gsi->mutex); > + } nit: I thought just recently Willem pointed out that keeping main flow unindented is considered good style, maybe it doesn't apply here perfectly, but I'd think it still applies. Why have the entire body of the function indented? > return ret; > } > @@ -910,11 +910,8 @@ int gsi_channel_start(struct gsi *gsi, u32 channel_id) > static int gsi_channel_stop_retry(struct gsi_channel *channel) > { > u32 retries = GSI_CHANNEL_STOP_RETRIES; > - struct gsi *gsi = channel->gsi; > int ret; > > - mutex_lock(&gsi->mutex); > - > do { > ret = gsi_channel_stop_command(channel); > if (ret != -EAGAIN) > @@ -922,19 +919,26 @@ static int gsi_channel_stop_retry(struct gsi_channel *channel) > usleep_range(3 * USEC_PER_MSEC, 5 * USEC_PER_MSEC); > } while (retries--); > > - mutex_unlock(&gsi->mutex); > - > return ret; > } > > static int __gsi_channel_stop(struct gsi_channel *channel, bool stop) > { > - int ret; > + int ret = 0; > > /* Wait for any underway transactions to complete before stopping. */ > gsi_channel_trans_quiesce(channel); > > - ret = stop ? gsi_channel_stop_retry(channel) : 0; > + if (stop) { > + struct gsi *gsi = channel->gsi; > + > + mutex_lock(&gsi->mutex); > + > + ret = gsi_channel_stop_retry(channel); > + > + mutex_unlock(&gsi->mutex); > + } > + > /* Finally, ensure NAPI polling has finished. */ > if (!ret) > napi_synchronize(&channel->napi); > @@ -948,15 +952,14 @@ int gsi_channel_stop(struct gsi *gsi, u32 channel_id) > struct gsi_channel *channel = &gsi->channel[channel_id]; > int ret; > > - /* Only disable the completion interrupt if stop is successful */ > ret = __gsi_channel_stop(channel, true); > - if (ret) > - return ret; > + if (ret) { This inverts the logic, right? Is it intentional? > + /* Disable the completion interrupt and NAPI if successful */ > + gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id); > + napi_disable(&channel->napi); > + } > > - gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id); > - napi_disable(&channel->napi); > - > - return 0; > + return ret; > } > > /* Reset and reconfigure a channel, (possibly) enabling the doorbell engine */
On 2/4/21 10:50 PM, Jakub Kicinski wrote: > On Wed, 3 Feb 2021 09:28:49 -0600 Alex Elder wrote: >> Make __gsi_channel_start() and __gsi_channel_stop() more structurally >> and semantically similar to each other: >> - Restructure __gsi_channel_start() to always return at the end of >> the function, similar to the way __gsi_channel_stop() does. >> - Move the mutex calls out of gsi_channel_stop_retry() and into >> __gsi_channel_stop(). >> >> Restructure gsi_channel_stop() to always return at the end of the >> function, like gsi_channel_start() does. >> >> Signed-off-by: Alex Elder <elder@linaro.org> >> --- >> drivers/net/ipa/gsi.c | 45 +++++++++++++++++++++++-------------------- >> 1 file changed, 24 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c >> index 53640447bf123..2671b76ebcfe3 100644 >> --- a/drivers/net/ipa/gsi.c >> +++ b/drivers/net/ipa/gsi.c >> @@ -873,17 +873,17 @@ static void gsi_channel_deprogram(struct gsi_channel *channel) >> >> static int __gsi_channel_start(struct gsi_channel *channel, bool start) >> { >> - struct gsi *gsi = channel->gsi; >> - int ret; >> + int ret = 0; >> >> - if (!start) >> - return 0; >> + if (start) { >> + struct gsi *gsi = channel->gsi; >> >> - mutex_lock(&gsi->mutex); >> + mutex_lock(&gsi->mutex); >> >> - ret = gsi_channel_start_command(channel); >> + ret = gsi_channel_start_command(channel); >> >> - mutex_unlock(&gsi->mutex); >> + mutex_unlock(&gsi->mutex); >> + } > > nit: I thought just recently Willem pointed out that keeping main flow > unindented is considered good style, maybe it doesn't apply here > perfectly, but I'd think it still applies. Why have the entire > body of the function indented? I *like* keeping the main flow un-indented (the way it was). It's a little funny, because one of my motivations for doing it this way was how I interpreted the comment from Willem (and echoed by you). He said, "...easier to parse when the normal control flow is linear and the error path takes a branch (or goto, if reused)." And now that I read it again, I see that's what he was saying. But the way I interpreted it was "don't return early for the success case," because that's what the code in question that elicited that comment was doing. In any case I concur with your comment and prefer the code the other way. I will post v2 that will fix this, both here and in __gsi_channel_start(). Thanks. -Alex > >> return ret; >> } >> @@ -910,11 +910,8 @@ int gsi_channel_start(struct gsi *gsi, u32 channel_id) >> static int gsi_channel_stop_retry(struct gsi_channel *channel) >> { >> u32 retries = GSI_CHANNEL_STOP_RETRIES; >> - struct gsi *gsi = channel->gsi; >> int ret; >> >> - mutex_lock(&gsi->mutex); >> - >> do { >> ret = gsi_channel_stop_command(channel); >> if (ret != -EAGAIN) >> @@ -922,19 +919,26 @@ static int gsi_channel_stop_retry(struct gsi_channel *channel) >> usleep_range(3 * USEC_PER_MSEC, 5 * USEC_PER_MSEC); >> } while (retries--); >> >> - mutex_unlock(&gsi->mutex); >> - >> return ret; >> } >> >> static int __gsi_channel_stop(struct gsi_channel *channel, bool stop) >> { >> - int ret; >> + int ret = 0; >> >> /* Wait for any underway transactions to complete before stopping. */ >> gsi_channel_trans_quiesce(channel); >> >> - ret = stop ? gsi_channel_stop_retry(channel) : 0; >> + if (stop) { >> + struct gsi *gsi = channel->gsi; >> + >> + mutex_lock(&gsi->mutex); >> + >> + ret = gsi_channel_stop_retry(channel); >> + >> + mutex_unlock(&gsi->mutex); >> + } >> + >> /* Finally, ensure NAPI polling has finished. */ >> if (!ret) >> napi_synchronize(&channel->napi); >> @@ -948,15 +952,14 @@ int gsi_channel_stop(struct gsi *gsi, u32 channel_id) >> struct gsi_channel *channel = &gsi->channel[channel_id]; >> int ret; >> >> - /* Only disable the completion interrupt if stop is successful */ >> ret = __gsi_channel_stop(channel, true); >> - if (ret) >> - return ret; >> + if (ret) { > > This inverts the logic, right? Is it intentional? > >> + /* Disable the completion interrupt and NAPI if successful */ >> + gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id); >> + napi_disable(&channel->napi); >> + } >> >> - gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id); >> - napi_disable(&channel->napi); >> - >> - return 0; >> + return ret; >> } >> >> /* Reset and reconfigure a channel, (possibly) enabling the doorbell engine */ >
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index 53640447bf123..2671b76ebcfe3 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -873,17 +873,17 @@ static void gsi_channel_deprogram(struct gsi_channel *channel) static int __gsi_channel_start(struct gsi_channel *channel, bool start) { - struct gsi *gsi = channel->gsi; - int ret; + int ret = 0; - if (!start) - return 0; + if (start) { + struct gsi *gsi = channel->gsi; - mutex_lock(&gsi->mutex); + mutex_lock(&gsi->mutex); - ret = gsi_channel_start_command(channel); + ret = gsi_channel_start_command(channel); - mutex_unlock(&gsi->mutex); + mutex_unlock(&gsi->mutex); + } return ret; } @@ -910,11 +910,8 @@ int gsi_channel_start(struct gsi *gsi, u32 channel_id) static int gsi_channel_stop_retry(struct gsi_channel *channel) { u32 retries = GSI_CHANNEL_STOP_RETRIES; - struct gsi *gsi = channel->gsi; int ret; - mutex_lock(&gsi->mutex); - do { ret = gsi_channel_stop_command(channel); if (ret != -EAGAIN) @@ -922,19 +919,26 @@ static int gsi_channel_stop_retry(struct gsi_channel *channel) usleep_range(3 * USEC_PER_MSEC, 5 * USEC_PER_MSEC); } while (retries--); - mutex_unlock(&gsi->mutex); - return ret; } static int __gsi_channel_stop(struct gsi_channel *channel, bool stop) { - int ret; + int ret = 0; /* Wait for any underway transactions to complete before stopping. */ gsi_channel_trans_quiesce(channel); - ret = stop ? gsi_channel_stop_retry(channel) : 0; + if (stop) { + struct gsi *gsi = channel->gsi; + + mutex_lock(&gsi->mutex); + + ret = gsi_channel_stop_retry(channel); + + mutex_unlock(&gsi->mutex); + } + /* Finally, ensure NAPI polling has finished. */ if (!ret) napi_synchronize(&channel->napi); @@ -948,15 +952,14 @@ int gsi_channel_stop(struct gsi *gsi, u32 channel_id) struct gsi_channel *channel = &gsi->channel[channel_id]; int ret; - /* Only disable the completion interrupt if stop is successful */ ret = __gsi_channel_stop(channel, true); - if (ret) - return ret; + if (ret) { + /* Disable the completion interrupt and NAPI if successful */ + gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id); + napi_disable(&channel->napi); + } - gsi_irq_ieob_disable_one(gsi, channel->evt_ring_id); - napi_disable(&channel->napi); - - return 0; + return ret; } /* Reset and reconfigure a channel, (possibly) enabling the doorbell engine */
Make __gsi_channel_start() and __gsi_channel_stop() more structurally and semantically similar to each other: - Restructure __gsi_channel_start() to always return at the end of the function, similar to the way __gsi_channel_stop() does. - Move the mutex calls out of gsi_channel_stop_retry() and into __gsi_channel_stop(). Restructure gsi_channel_stop() to always return at the end of the function, like gsi_channel_start() does. Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/net/ipa/gsi.c | 45 +++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 21 deletions(-)