This function has two unused parameters - remove them.
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 <david@gibson.dropbear.id.au>
---
hw/ppc/spapr.c | 11 +++--------
hw/ppc/spapr_drc.c | 12 ++++++------
hw/ppc/spapr_pci.c | 7 +------
include/hw/ppc/spapr_drc.h | 2 +-
4 files changed, 11 insertions(+), 21 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ff2aa6b..5c528e2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2654,7 +2654,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
addr -= SPAPR_MEMORY_BLOCK_SIZE;
drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
addr / SPAPR_MEMORY_BLOCK_SIZE);
- spapr_drc_detach(drc, dev, NULL);
+ spapr_drc_detach(drc);
}
g_free(fdt);
error_propagate(errp, local_err);
@@ -2877,7 +2877,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;
}
@@ -2944,7 +2944,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();
@@ -2961,11 +2960,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, &local_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 89ba3d6..08fc715 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 1e84c55..092a2f5 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1478,7 +1478,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) {
error_setg(errp, QERR_BUS_NO_HOTPLUG,
@@ -1516,11 +1515,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
}
}
- spapr_drc_detach(drc, DEVICE(pdev), &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- return;
- }
+ spapr_drc_detach(drc);
/* if this isn't func 0, defer unplug event. otherwise signal removal
* for all present functions
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 18a196e..fc8b721 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -242,6 +242,6 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
int fdt_start_offset, Error **errp);
-void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
+void spapr_drc_detach(sPAPRDRConnector *drc);
#endif /* HW_SPAPR_DRC_H */
--
2.9.4
On Wed, 12 Jul 2017 15:53:13 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:
> This function has two unused parameters - remove them.
>
> 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 <david@gibson.dropbear.id.au>
> ---
Patch 3 drops the hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort)
lines that could be called below spapr_drc_detach(), but we still have:
spapr_drc_detach()
spapr_drc_release()
object_property_del(OBJECT(drc), "device", NULL);
^^
Should this be &error_abort or should we pass an Error ** down to here ?
(which, would require to add an errp argument to both spapr_drc_release()
and spapr_drc_reset())
I would favor &error_abort since a failure in object_property_del() would
mean we're calling spapr_drc_detach() on a DRC that wasn't attached to a
device... ie, a bug. In this case:
Reviewed-by: Greg Kurz <groug@kaod.org>
> hw/ppc/spapr.c | 11 +++--------
> hw/ppc/spapr_drc.c | 12 ++++++------
> hw/ppc/spapr_pci.c | 7 +------
> include/hw/ppc/spapr_drc.h | 2 +-
> 4 files changed, 11 insertions(+), 21 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ff2aa6b..5c528e2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2654,7 +2654,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> addr -= SPAPR_MEMORY_BLOCK_SIZE;
> drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> addr / SPAPR_MEMORY_BLOCK_SIZE);
> - spapr_drc_detach(drc, dev, NULL);
> + spapr_drc_detach(drc);
> }
> g_free(fdt);
> error_propagate(errp, local_err);
> @@ -2877,7 +2877,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;
> }
>
> @@ -2944,7 +2944,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();
>
> @@ -2961,11 +2960,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, &local_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 89ba3d6..08fc715 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 1e84c55..092a2f5 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1478,7 +1478,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) {
> error_setg(errp, QERR_BUS_NO_HOTPLUG,
> @@ -1516,11 +1515,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
> }
> }
>
> - spapr_drc_detach(drc, DEVICE(pdev), &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> - return;
> - }
> + spapr_drc_detach(drc);
>
> /* if this isn't func 0, defer unplug event. otherwise signal removal
> * for all present functions
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 18a196e..fc8b721 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -242,6 +242,6 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>
> void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> int fdt_start_offset, Error **errp);
> -void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
> +void spapr_drc_detach(sPAPRDRConnector *drc);
>
> #endif /* HW_SPAPR_DRC_H */
On Wed, Jul 12, 2017 at 01:47:15PM +0200, Greg Kurz wrote:
> On Wed, 12 Jul 2017 15:53:13 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > This function has two unused parameters - remove them.
> >
> > 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 <david@gibson.dropbear.id.au>
> > ---
>
> Patch 3 drops the hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort)
> lines that could be called below spapr_drc_detach(), but we still have:
>
> spapr_drc_detach()
> spapr_drc_release()
> object_property_del(OBJECT(drc), "device", NULL);
> ^^
> Should this be &error_abort or should we pass an Error ** down to here ?
> (which, would require to add an errp argument to both spapr_drc_release()
> and spapr_drc_reset())
>
> I would favor &error_abort since a failure in object_property_del() would
> mean we're calling spapr_drc_detach() on a DRC that wasn't attached to a
> device... ie, a bug. In this case:
Done, although as a separate patch, since it's not really tied to the
changes here.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> > hw/ppc/spapr.c | 11 +++--------
> > hw/ppc/spapr_drc.c | 12 ++++++------
> > hw/ppc/spapr_pci.c | 7 +------
> > include/hw/ppc/spapr_drc.h | 2 +-
> > 4 files changed, 11 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index ff2aa6b..5c528e2 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2654,7 +2654,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
> > addr -= SPAPR_MEMORY_BLOCK_SIZE;
> > drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> > addr / SPAPR_MEMORY_BLOCK_SIZE);
> > - spapr_drc_detach(drc, dev, NULL);
> > + spapr_drc_detach(drc);
> > }
> > g_free(fdt);
> > error_propagate(errp, local_err);
> > @@ -2877,7 +2877,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;
> > }
> >
> > @@ -2944,7 +2944,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();
> >
> > @@ -2961,11 +2960,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, &local_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 89ba3d6..08fc715 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 1e84c55..092a2f5 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1478,7 +1478,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) {
> > error_setg(errp, QERR_BUS_NO_HOTPLUG,
> > @@ -1516,11 +1515,7 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
> > }
> > }
> >
> > - spapr_drc_detach(drc, DEVICE(pdev), &local_err);
> > - if (local_err) {
> > - error_propagate(errp, local_err);
> > - return;
> > - }
> > + spapr_drc_detach(drc);
> >
> > /* if this isn't func 0, defer unplug event. otherwise signal removal
> > * for all present functions
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 18a196e..fc8b721 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -242,6 +242,6 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
> >
> > void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > int fdt_start_offset, Error **errp);
> > -void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
> > +void spapr_drc_detach(sPAPRDRConnector *drc);
> >
> > #endif /* HW_SPAPR_DRC_H */
>
--
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
© 2016 - 2026 Red Hat, Inc.