Re: [Qemu-devel] [PATCH 2/5] spapr: Refactor spapr_drc_detach()

[David Gibson]

On Thu, Jun 22, 2017 at 11:32:15AM +0200, Greg Kurz wrote:
> On Wed, 21 Jun 2017 17:18:45 +0800
> David Gibson  wrote:
> 
> > This function has two unused parameters - remove them.
> > 
> 
> It's ok for the d argument but I'm not sure about errp... Indeed it isn't used
> in the current code, but looking at the paths below spapr_drc_detach(), we
> have:
> 
> spapr_drc_detach()
>  spapr_drc_release()
>   spapr_core_release()
>hotplug_handler_unplug(hotplug_ctrl, dev, _abort);
> 
> and
> 
> spapr_drc_detach()
>  spapr_drc_release()
>   spapr_lmb_release()
>hotplug_handler_unplug(hotplug_ctrl, dev, _abort);
> 
> Shouldn't we pass the errp down to hotplug_handler_unplug() instead of
> aborting ?

So, at first I thought you were right, and was going to rewrite on
that basis.

Then I realized that calling hotplug_handler_unplug() is a bizarrely
roundabout way of doing what we need anyway (usually the unplug
handler is never called if there is an unplug_request() handler).
Once we expand those out to what they actually call, the errors
disappear again.  I'll do that in the next spin.

-- 
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

Re: [Qemu-devel] [PATCH 2/5] spapr: Refactor spapr_drc_detach()

[Greg Kurz]

On Wed, 21 Jun 2017 17:18:45 +0800
David Gibson  wrote:

> This function has two unused parameters - remove them.
> 

It's ok for the d argument but I'm not sure about errp... Indeed it isn't used
in the current code, but looking at the paths below spapr_drc_detach(), we
have:

spapr_drc_detach()
 spapr_drc_release()
  spapr_core_release()
   hotplug_handler_unplug(hotplug_ctrl, dev, _abort);

and

spapr_drc_detach()
 spapr_drc_release()
  spapr_lmb_release()
   hotplug_handler_unplug(hotplug_ctrl, dev, _abort);

Shouldn't we pass the errp down to hotplug_handler_unplug() instead of
aborting ?

> It also sets awaiting_release on all paths, except one.  On that path
> setting it is harmless, since it will be immediately cleared by
> spapr_drc_release().  So factor it out of the if statements.
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr.c |  9 ++---
>  hw/ppc/spapr_drc.c | 12 ++--
>  hw/ppc/spapr_pci.c |  7 +--
>  include/hw/ppc/spapr_drc.h |  2 +-
>  4 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5aa3760..8b5ab3a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2827,7 +2827,7 @@ static void spapr_memory_unplug_request(HotplugHandler 
> *hotplug_dev,
>addr / SPAPR_MEMORY_BLOCK_SIZE);
>  g_assert(drc);
>  
> -spapr_drc_detach(drc, dev, errp);
> +spapr_drc_detach(drc);
>  addr += SPAPR_MEMORY_BLOCK_SIZE;
>  }
>  
> @@ -2902,7 +2902,6 @@ void spapr_core_unplug_request(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  {
>  int index;
>  sPAPRDRConnector *drc;
> -Error *local_err = NULL;
>  CPUCore *cc = CPU_CORE(dev);
>  int smt = kvmppc_smt_threads();
>  
> @@ -2919,11 +2918,7 @@ void spapr_core_unplug_request(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt);
>  g_assert(drc);
>  
> -spapr_drc_detach(drc, dev, _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> +spapr_drc_detach(drc);
>  
>  spapr_hotplug_req_remove_by_index(drc);
>  }
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 59e19f8..dae1f79 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -70,7 +70,7 @@ static uint32_t drc_isolate_physical(sPAPRDRConnector *drc)
>  uint32_t drc_index = spapr_drc_index(drc);
>  if (drc->configured) {
>  trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> -spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> +spapr_drc_detach(drc);
>  } else {
>  trace_spapr_drc_set_isolation_state_deferring(drc_index);
>  }
> @@ -134,7 +134,7 @@ static uint32_t drc_isolate_logical(sPAPRDRConnector *drc)
>  uint32_t drc_index = spapr_drc_index(drc);
>  if (drc->configured) {
>  trace_spapr_drc_set_isolation_state_finalizing(drc_index);
> -spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> +spapr_drc_detach(drc);
>  } else {
>  trace_spapr_drc_set_isolation_state_deferring(drc_index);
>  }
> @@ -187,7 +187,7 @@ static uint32_t drc_set_unusable(sPAPRDRConnector *drc)
>  if (drc->awaiting_release) {
>  uint32_t drc_index = spapr_drc_index(drc);
>  trace_spapr_drc_set_allocation_state_finalizing(drc_index);
> -spapr_drc_detach(drc, DEVICE(drc->dev), NULL);
> +spapr_drc_detach(drc);
>  }
>  
>  return RTAS_OUT_SUCCESS;
> @@ -371,20 +371,20 @@ static void spapr_drc_release(sPAPRDRConnector *drc)
>  drc->dev = NULL;
>  }
>  
> -void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
> +void spapr_drc_detach(sPAPRDRConnector *drc)
>  {
>  trace_spapr_drc_detach(spapr_drc_index(drc));
>  
> +drc->awaiting_release = true;
> +
>  if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
>  trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
> -drc->awaiting_release = true;
>  return;
>  }
>  
>  if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
>  drc->allocation_state != SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
>  trace_spapr_drc_awaiting_unusable(spapr_drc_index(drc));
> -drc->awaiting_release = true;
>  return;
>  }
>  
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index bda8938..af925c0 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1476,7 +1476,6 @@ static void spapr_pci_unplug_request(HotplugHandler 
> *plug_handler,
>  PCIDevice *pdev = PCI_DEVICE(plugged_dev);
>  sPAPRDRConnectorClass *drck;
>  sPAPRDRConnector *drc = spapr_phb_get_pci_drc(phb, pdev);
> -Error *local_err = NULL;
>  
>  if (!phb->dr_enabled) {
>