Re: [Qemu-devel] [RESEND PATCH v10 2/5] hw/ppc: removing drc->detach_cb and drc->detach_cb_opaque

2017-05-18 Thread David Gibson
On Thu, May 18, 2017 at 06:54:13PM -0300, Daniel Henrique Barboza wrote:
> The pointer drc->detach_cb is being used as a way of informing
> the detach() function inside spapr_drc.c which cb to execute. This
> information can also be retrieved simply by checking drc->type and
> choosing the right callback based on it. In this context, detach_cb
> is redundant information that must be managed.
> 
> After the previous spapr_lmb_release change, no detach_cb_opaques
> are being used by any of the three callbacks functions. This is
> yet another information that is now unused and, on top of that, can't
> be migrated either.
> 
> This patch makes the following changes:
> 
> - removal of detach_cb_opaque. the 'opaque' argument was removed from
> the callbacks and from the detach() function of sPAPRConnectorClass. The
> attribute detach_cb_opaque of sPAPRConnector was removed.
> 
> - removal of detach_cb from the detach() call. The function pointer
> detach_cb of sPAPRConnector was removed. detach() now uses a
> switch(drc->type) to execute the apropriate callback. To achieve this,
> spapr_core_release, spapr_lmb_release and spapr_phb_remove_pci_device_cb
> callbacks were made public to be visible inside detach().
> 
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: David Gibson 

> ---
>  hw/ppc/spapr.c  | 10 ++
>  hw/ppc/spapr_drc.c  | 36 
>  hw/ppc/spapr_pci.c  |  5 +++--
>  include/hw/pci-host/spapr.h |  3 +++
>  include/hw/ppc/spapr.h  |  4 
>  include/hw/ppc/spapr_drc.h  |  8 +---
>  6 files changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b05abe5..5602cfc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2649,7 +2649,8 @@ static uint64_t spapr_dimm_get_address(PCDIMMDevice 
> *dimm)
>  return addr;
>  }
>  
> -static void spapr_lmb_release(DeviceState *dev, void *opaque)
> +/* Callback to be called during DRC release. */
> +void spapr_lmb_release(DeviceState *dev)
>  {
>  HotplugHandler *hotplug_ctrl;
>  uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev));
> @@ -2690,7 +2691,7 @@ static void spapr_del_lmbs(DeviceState *dev, uint64_t 
> addr_start, uint64_t size,
>  g_assert(drc);
>  
>  drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -drck->detach(drc, dev, spapr_lmb_release, NULL, errp);
> +drck->detach(drc, dev, errp);
>  addr += SPAPR_MEMORY_BLOCK_SIZE;
>  }
>  
> @@ -2766,7 +2767,8 @@ static void spapr_core_unplug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  object_unparent(OBJECT(dev));
>  }
>  
> -static void spapr_core_release(DeviceState *dev, void *opaque)
> +/* Callback to be called during DRC release. */
> +void spapr_core_release(DeviceState *dev)
>  {
>  HotplugHandler *hotplug_ctrl;
>  
> @@ -2799,7 +2801,7 @@ void spapr_core_unplug_request(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  g_assert(drc);
>  
>  drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -drck->detach(drc, dev, spapr_core_release, NULL, &local_err);
> +drck->detach(drc, dev, &local_err);
>  if (local_err) {
>  error_propagate(errp, local_err);
>  return;
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 9fa5545..2851e16 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -20,6 +20,7 @@
>  #include "qapi/visitor.h"
>  #include "qemu/error-report.h"
>  #include "hw/ppc/spapr.h" /* for RTAS return codes */
> +#include "hw/pci-host/spapr.h" /* spapr_phb_remove_pci_device_cb callback */
>  #include "trace.h"
>  
>  #define DRC_CONTAINER_PATH "/dr-connector"
> @@ -99,8 +100,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
>  if (drc->awaiting_release) {
>  if (drc->configured) {
>  
> trace_spapr_drc_set_isolation_state_finalizing(get_index(drc));
> -drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
> - drc->detach_cb_opaque, NULL);
> +drck->detach(drc, DEVICE(drc->dev), NULL);
>  } else {
>  
> trace_spapr_drc_set_isolation_state_deferring(get_index(drc));
>  }
> @@ -153,8 +153,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector 
> *drc,
>  if (drc->awaiting_release &&
>  drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
>  trace_spapr_drc_set_allocation_state_finalizing(get_index(drc));
> -drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
> - drc->detach_cb_opaque, NULL);
> +drck->detach(drc, DEVICE(drc->dev), NULL);
>  } else if (drc->allocation_state == 
> SPAPR_DR_ALLOCATION_STATE_USABLE) {
>  drc->awaiting_allocation = false;
>  }
> @@ -404,15 +403,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState 
> *d, void *fdt,
>   NULL, 0, NULL

[Qemu-devel] [RESEND PATCH v10 2/5] hw/ppc: removing drc->detach_cb and drc->detach_cb_opaque

2017-05-18 Thread Daniel Henrique Barboza
The pointer drc->detach_cb is being used as a way of informing
the detach() function inside spapr_drc.c which cb to execute. This
information can also be retrieved simply by checking drc->type and
choosing the right callback based on it. In this context, detach_cb
is redundant information that must be managed.

After the previous spapr_lmb_release change, no detach_cb_opaques
are being used by any of the three callbacks functions. This is
yet another information that is now unused and, on top of that, can't
be migrated either.

This patch makes the following changes:

- removal of detach_cb_opaque. the 'opaque' argument was removed from
the callbacks and from the detach() function of sPAPRConnectorClass. The
attribute detach_cb_opaque of sPAPRConnector was removed.

- removal of detach_cb from the detach() call. The function pointer
detach_cb of sPAPRConnector was removed. detach() now uses a
switch(drc->type) to execute the apropriate callback. To achieve this,
spapr_core_release, spapr_lmb_release and spapr_phb_remove_pci_device_cb
callbacks were made public to be visible inside detach().

Signed-off-by: Daniel Henrique Barboza 
---
 hw/ppc/spapr.c  | 10 ++
 hw/ppc/spapr_drc.c  | 36 
 hw/ppc/spapr_pci.c  |  5 +++--
 include/hw/pci-host/spapr.h |  3 +++
 include/hw/ppc/spapr.h  |  4 
 include/hw/ppc/spapr_drc.h  |  8 +---
 6 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b05abe5..5602cfc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2649,7 +2649,8 @@ static uint64_t spapr_dimm_get_address(PCDIMMDevice *dimm)
 return addr;
 }
 
-static void spapr_lmb_release(DeviceState *dev, void *opaque)
+/* Callback to be called during DRC release. */
+void spapr_lmb_release(DeviceState *dev)
 {
 HotplugHandler *hotplug_ctrl;
 uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev));
@@ -2690,7 +2691,7 @@ static void spapr_del_lmbs(DeviceState *dev, uint64_t 
addr_start, uint64_t size,
 g_assert(drc);
 
 drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-drck->detach(drc, dev, spapr_lmb_release, NULL, errp);
+drck->detach(drc, dev, errp);
 addr += SPAPR_MEMORY_BLOCK_SIZE;
 }
 
@@ -2766,7 +2767,8 @@ static void spapr_core_unplug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 object_unparent(OBJECT(dev));
 }
 
-static void spapr_core_release(DeviceState *dev, void *opaque)
+/* Callback to be called during DRC release. */
+void spapr_core_release(DeviceState *dev)
 {
 HotplugHandler *hotplug_ctrl;
 
@@ -2799,7 +2801,7 @@ void spapr_core_unplug_request(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 g_assert(drc);
 
 drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-drck->detach(drc, dev, spapr_core_release, NULL, &local_err);
+drck->detach(drc, dev, &local_err);
 if (local_err) {
 error_propagate(errp, local_err);
 return;
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 9fa5545..2851e16 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -20,6 +20,7 @@
 #include "qapi/visitor.h"
 #include "qemu/error-report.h"
 #include "hw/ppc/spapr.h" /* for RTAS return codes */
+#include "hw/pci-host/spapr.h" /* spapr_phb_remove_pci_device_cb callback */
 #include "trace.h"
 
 #define DRC_CONTAINER_PATH "/dr-connector"
@@ -99,8 +100,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
 if (drc->awaiting_release) {
 if (drc->configured) {
 trace_spapr_drc_set_isolation_state_finalizing(get_index(drc));
-drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
- drc->detach_cb_opaque, NULL);
+drck->detach(drc, DEVICE(drc->dev), NULL);
 } else {
 trace_spapr_drc_set_isolation_state_deferring(get_index(drc));
 }
@@ -153,8 +153,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
 if (drc->awaiting_release &&
 drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
 trace_spapr_drc_set_allocation_state_finalizing(get_index(drc));
-drck->detach(drc, DEVICE(drc->dev), drc->detach_cb,
- drc->detach_cb_opaque, NULL);
+drck->detach(drc, DEVICE(drc->dev), NULL);
 } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) {
 drc->awaiting_allocation = false;
 }
@@ -404,15 +403,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, 
void *fdt,
  NULL, 0, NULL);
 }
 
-static void detach(sPAPRDRConnector *drc, DeviceState *d,
-   spapr_drc_detach_cb *detach_cb,
-   void *detach_cb_opaque, Error **errp)
+static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
 {
 trace_spapr_drc_detach(get_index(drc));
 
-drc->detach_cb = detach_cb;
-drc->