Re: [PATCH v3 1/7] spapr_drc.c: do not call spapr_drc_detach() in drc_isolate_logical()

[Greg Kurz]

On Thu, 11 Feb 2021 19:52:40 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> drc_isolate_logical() is used to move the DRC from the "Configured" to the
> "Available" state, erroring out if the DRC is in the unexpected "Unisolate"
> state and doing nothing (with RTAS_OUT_SUCCESS) if the DRC is already in
> "Available" or in "Unusable" state.
> 
> When moving from "Configured" to "Available", the DRC is moved to the
> LOGICAL_AVAILABLE state, a drc->unplug_requested check is done and, if true,
> spapr_drc_detach() is called.
> 
> What spapr_drc_detach() does then is:
> 
> - set drc->unplug_requested to true. In fact, this is the only place where
> unplug_request is set to true;
> - does nothing else if drc->state != drck->empty_state. If the DRC state
> is equal to drck->empty_state, spapr_drc_release() is called. For logical
> DRCs, drck->empty_state = LOGICAL_UNUSABLE.
> 
> In short, calling spapr_drc_detach() in drc_isolate_logical() does nothing. 
> It'll
> set unplug_request to true again ('again' since it was already true - 
> otherwise the
> function wouldn't be called), and will return without calling 
> spapr_drc_release()
> because the DRC is not in LOGICAL_UNUSABLE, since drc_isolate_logical() just
> moved it to LOGICAL_AVAILABLE. The only place where the logical DRC is 
> released
> is when called from drc_set_unusable(), when it is moved to the "Unusable" 
> state.
> As it should, according to PAPR.
> 
> Even though calling spapr_drc_detach() in drc_isolate_logical() is benign, 
> removing
> it will avoid further thought about the matter. So let's go ahead and do that.
> 

Good catch. This path looks useless for logical DRCs indeed.

> As a note, this logic was introduced in commit bbf5c878ab76. Since then, the 
> DRC
> handling code was refactored and enhanced, and PAPR itself went through some
> changes in the DRC area as well. It is expected that some assumptions we had 
> back
> then are now deprecated.
> 

As specified in [1]:

Please do not use lines that are longer than 76 characters in your
commit message (so that the text still shows up nicely with "git show"
in a 80-columns terminal window).

[1] 
https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message

> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr_drc.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 8571d5bafe..84bd3c881f 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -132,19 +132,6 @@ static uint32_t drc_isolate_logical(SpaprDrc *drc)
>  
>      drc->state = SPAPR_DRC_STATE_LOGICAL_AVAILABLE;
>  
> -    /* if we're awaiting release, but still in an unconfigured state,
> -     * it's likely the guest is still in the process of configuring
> -     * the device and is transitioning the devices to an ISOLATED
> -     * state as a part of that process. so we only complete the
> -     * removal when this transition happens for a device in a
> -     * configured state, as suggested by the state diagram from PAPR+
> -     * 2.7, 13.4
> -     */
> -    if (drc->unplug_requested) {
> -        uint32_t drc_index = spapr_drc_index(drc);
> -        trace_spapr_drc_set_isolation_state_finalizing(drc_index);

I was expecting a change in hw/ppc/trace-events to ditch this trace,
but it is still called by drc_isolate_physical(), so we're good.

Reviewed-by: Greg Kurz <groug@kaod.org>

> -        spapr_drc_detach(drc);
> -    }
>      return RTAS_OUT_SUCCESS;
>  }
>