On Thu, 11 Feb 2021 19:52:43 -0300
Daniel Henrique Barboza <danielhb...@gmail.com> 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.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com>
> ---
Reviewed-by: Greg Kurz <gr...@kaod.org>
> hw/ppc/spapr.c | 6 +++---
> hw/ppc/spapr_drc.c | 4 ++--
> hw/ppc/spapr_pci.c | 2 +-
> hw/ppc/trace-events | 2 +-
> include/hw/ppc/spapr_drc.h | 2 +-
> 5 files changed, 8 insertions(+), 8 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 1791d98a49..9334ba5dbb 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1726,7 +1726,7 @@ static void spapr_pci_unplug_request(HotplugHandler
> *plug_handler,
> if (state == SPAPR_DR_ENTITY_SENSE_PRESENT) {
> /* Mark the DRC as requested unplug if needed. */
> if (!spapr_drc_unplug_requested(func_drc)) {
> - spapr_drc_detach(func_drc);
> + spapr_drc_unplug_request(func_drc);
> }
> spapr_hotplug_req_remove_by_index(func_drc);
> }
> 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.
