Re: [PATCH v4 2/5] spapr: rename spapr_drc_detach() to spapr_drc_unplug_request()

[David Gibson]

On Mon, Feb 22, 2021 at 04:45:28PM -0300, Daniel Henrique Barboza wrote:
> spapr_drc_detach() is not the best name for what the function does. The
> function does not detach the DRC, it makes an uncommited attempt to do
> it.  It'll mark the DRC as pending unplug, via the 'unplug_request'
> flag, and only if the DRC state is drck->empty_state it will detach the
> DRC, via spapr_drc_release().
> 
> This is a contrast with its pair spapr_drc_attach(), where the function
> is indeed creating the DRC QOM object. If you know what
> spapr_drc_attach() does, you can be misled into thinking that
> spapr_drc_detach() is removing the DRC from QEMU internal state, which
> isn't true.
> 
> The current role of this function is better described as a request for
> detach, since there's no guarantee that we're going to detach the DRC in
> the end.  Rename the function to spapr_drc_unplug_request to reflect
> what is is doing.
> 
> The initial idea was to change the name to spapr_drc_detach_request(),
> and later on change the unplug_request flag to detach_request. However,
> unplug_request is a migratable boolean for a long time now and renaming
> it is not worth the trouble. spapr_drc_unplug_request() setting
> drc->unplug_request is more natural than spapr_drc_detach_request
> setting drc->unplug_request.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/spapr.c             | 6 +++---
>  hw/ppc/spapr_drc.c         | 4 ++--
>  hw/ppc/spapr_pci.c         | 4 ++--
>  hw/ppc/trace-events        | 2 +-
>  include/hw/ppc/spapr_drc.h | 2 +-
>  5 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 85fe65f894..b066df68cb 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3654,7 +3654,7 @@ static void spapr_memory_unplug_request(HotplugHandler 
> *hotplug_dev,
>                                addr / SPAPR_MEMORY_BLOCK_SIZE);
>          g_assert(drc);
>  
> -        spapr_drc_detach(drc);
> +        spapr_drc_unplug_request(drc);
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
>      }
>  
> @@ -3722,7 +3722,7 @@ void spapr_core_unplug_request(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>      g_assert(drc);
>  
>      if (!spapr_drc_unplug_requested(drc)) {
> -        spapr_drc_detach(drc);
> +        spapr_drc_unplug_request(drc);
>          spapr_hotplug_req_remove_by_index(drc);
>      }
>  }
> @@ -3985,7 +3985,7 @@ static void spapr_phb_unplug_request(HotplugHandler 
> *hotplug_dev,
>      assert(drc);
>  
>      if (!spapr_drc_unplug_requested(drc)) {
> -        spapr_drc_detach(drc);
> +        spapr_drc_unplug_request(drc);
>          spapr_hotplug_req_remove_by_index(drc);
>      }
>  }
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 555a25517d..67041fb212 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -386,11 +386,11 @@ void spapr_drc_attach(SpaprDrc *drc, DeviceState *d)
>                               NULL, 0);
>  }
>  
> -void spapr_drc_detach(SpaprDrc *drc)
> +void spapr_drc_unplug_request(SpaprDrc *drc)
>  {
>      SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
> -    trace_spapr_drc_detach(spapr_drc_index(drc));
> +    trace_spapr_drc_unplug_request(spapr_drc_index(drc));
>  
>      g_assert(drc->dev);
>  
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index f1c7479816..b00e9609ae 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1723,12 +1723,12 @@ static void spapr_pci_unplug_request(HotplugHandler 
> *plug_handler,
>                       * functions, even if their unplug weren't requested
>                       * beforehand.
>                       */
> -                    spapr_drc_detach(func_drc);
> +                    spapr_drc_unplug_request(func_drc);
>                  }
>              }
>          }
>  
> -        spapr_drc_detach(drc);
> +        spapr_drc_unplug_request(drc);
>  
>          /* if this isn't func 0, defer unplug event. otherwise signal removal
>           * for all present functions
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 1e91984526..b4bbfbb013 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -50,7 +50,7 @@ spapr_drc_set_allocation_state(uint32_t index, int state) 
> "drc: 0x%"PRIx32", sta
>  spapr_drc_set_allocation_state_finalizing(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_set_configured(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_attach(uint32_t index) "drc: 0x%"PRIx32
> -spapr_drc_detach(uint32_t index) "drc: 0x%"PRIx32
> +spapr_drc_unplug_request(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_awaiting_quiesce(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_reset(uint32_t index) "drc: 0x%"PRIx32
>  spapr_drc_realize(uint32_t index) "drc: 0x%"PRIx32
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 8982927d5c..02a63b3666 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -243,7 +243,7 @@ int spapr_dt_drc(void *fdt, int offset, Object *owner, 
> uint32_t drc_type_mask);
>   * beforehand (eg. check drc->dev at pre-plug).
>   */
>  void spapr_drc_attach(SpaprDrc *drc, DeviceState *d);
> -void spapr_drc_detach(SpaprDrc *drc);
> +void spapr_drc_unplug_request(SpaprDrc *drc);
>  
>  /*
>   * Reset all DRCs, causing pending hot-plug/unplug requests to complete.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature