Re: [PATCH v2] hw/arm: Adjust some coding styles about memory hotplug

[zhukeqian]

On 2020/1/17 19:07, Peter Maydell wrote:
> On Fri, 17 Jan 2020 at 06:41, Keqian Zhu  wrote:
>>
>> From: zhukeqian 
>>
>> There is extra indent in ACPI GED plug cb. And we can use
>> existing helper function to trigger hotplug handler plug.
>>
>> Reviewed-by: Igor Mammedov 
>> Signed-off-by: Keqian Zhu 
>> ---
>>
>> v1->v2:
>>  - Add Igor's R-b
>>
>> Cc: Shameer Kolothum 
>> Cc: "Michael S. Tsirkin" 
>> Cc: Igor Mammedov 
>> Cc: Peter Maydell 
>> ---
>>  hw/acpi/generic_event_device.c | 2 +-
>>  hw/arm/virt.c  | 6 +++---
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
>> index 9cee90cc70..55eb29d80a 100644
>> --- a/hw/acpi/generic_event_device.c
>> +++ b/hw/acpi/generic_event_device.c
>> @@ -175,7 +175,7 @@ static void acpi_ged_device_plug_cb(HotplugHandler 
>> *hotplug_dev,
>>  AcpiGedState *s = ACPI_GED(hotplug_dev);
>>
>>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>> -acpi_memory_plug_cb(hotplug_dev, >memhp_state, dev, errp);
>> +acpi_memory_plug_cb(hotplug_dev, >memhp_state, dev, errp);
>>  } else {
>>  error_setg(errp, "virt: device plug request for unsupported device"
>> " type: %s", object_get_typename(OBJECT(dev)));
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 39ab5f47e0..656b0081c2 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1934,7 +1934,6 @@ static void virt_memory_pre_plug(HotplugHandler 
>> *hotplug_dev, DeviceState *dev,
>>  static void virt_memory_plug(HotplugHandler *hotplug_dev,
>>   DeviceState *dev, Error **errp)
>>  {
>> -HotplugHandlerClass *hhc;
>>  VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>>  Error *local_err = NULL;
>>
>> @@ -1943,8 +1942,9 @@ static void virt_memory_plug(HotplugHandler 
>> *hotplug_dev,
>>  goto out;
>>  }
>>
>> -hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
>> -hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, _abort);
>> +hotplug_handler_plug(HOTPLUG_HANDLER(vms->acpi_dev),
>> + dev, _abort);
>> +
>>  out:
>>  error_propagate(errp, local_err);
>>  }
> 
> These two changes are unrelated (one is just a whitespace
> fixup, and it's in an entirely different file to the other
> one, which is making an actual code change). I think they
> should not be in the same patch.
> 
> thanks
> -- PMM
> 
Ok, I will reform this patch.

Thanks,
Keqian
> .
> 

Re: [PATCH v2] hw/arm: Adjust some coding styles about memory hotplug

[Peter Maydell]

On Fri, 17 Jan 2020 at 06:41, Keqian Zhu  wrote:
>
> From: zhukeqian 
>
> There is extra indent in ACPI GED plug cb. And we can use
> existing helper function to trigger hotplug handler plug.
>
> Reviewed-by: Igor Mammedov 
> Signed-off-by: Keqian Zhu 
> ---
>
> v1->v2:
>  - Add Igor's R-b
>
> Cc: Shameer Kolothum 
> Cc: "Michael S. Tsirkin" 
> Cc: Igor Mammedov 
> Cc: Peter Maydell 
> ---
>  hw/acpi/generic_event_device.c | 2 +-
>  hw/arm/virt.c  | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 9cee90cc70..55eb29d80a 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -175,7 +175,7 @@ static void acpi_ged_device_plug_cb(HotplugHandler 
> *hotplug_dev,
>  AcpiGedState *s = ACPI_GED(hotplug_dev);
>
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -acpi_memory_plug_cb(hotplug_dev, >memhp_state, dev, errp);
> +acpi_memory_plug_cb(hotplug_dev, >memhp_state, dev, errp);
>  } else {
>  error_setg(errp, "virt: device plug request for unsupported device"
> " type: %s", object_get_typename(OBJECT(dev)));
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 39ab5f47e0..656b0081c2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1934,7 +1934,6 @@ static void virt_memory_pre_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  static void virt_memory_plug(HotplugHandler *hotplug_dev,
>   DeviceState *dev, Error **errp)
>  {
> -HotplugHandlerClass *hhc;
>  VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>  Error *local_err = NULL;
>
> @@ -1943,8 +1942,9 @@ static void virt_memory_plug(HotplugHandler 
> *hotplug_dev,
>  goto out;
>  }
>
> -hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> -hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, _abort);
> +hotplug_handler_plug(HOTPLUG_HANDLER(vms->acpi_dev),
> + dev, _abort);
> +
>  out:
>  error_propagate(errp, local_err);
>  }

These two changes are unrelated (one is just a whitespace
fixup, and it's in an entirely different file to the other
one, which is making an actual code change). I think they
should not be in the same patch.

thanks
-- PMM

Re: [PATCH v2] hw/arm: Adjust some coding styles about memory hotplug

[Igor Mammedov]

On Fri, 17 Jan 2020 14:38:22 +0800
Keqian Zhu  wrote:

> From: zhukeqian 
> 
> There is extra indent in ACPI GED plug cb. And we can use
> existing helper function to trigger hotplug handler plug.
> 
> Reviewed-by: Igor Mammedov 
> Signed-off-by: Keqian Zhu 
> ---
> 
> v1->v2:
>  - Add Igor's R-b

FYI one does not need to repost v2 for the purpose of
picking up tags, usually maintainer takes care of that
when patch is queued 

> 
> Cc: Shameer Kolothum 
> Cc: "Michael S. Tsirkin" 
> Cc: Igor Mammedov 
> Cc: Peter Maydell 
> ---
>  hw/acpi/generic_event_device.c | 2 +-
>  hw/arm/virt.c  | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 9cee90cc70..55eb29d80a 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -175,7 +175,7 @@ static void acpi_ged_device_plug_cb(HotplugHandler 
> *hotplug_dev,
>  AcpiGedState *s = ACPI_GED(hotplug_dev);
>  
>  if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -acpi_memory_plug_cb(hotplug_dev, >memhp_state, dev, errp);
> +acpi_memory_plug_cb(hotplug_dev, >memhp_state, dev, errp);
>  } else {
>  error_setg(errp, "virt: device plug request for unsupported device"
> " type: %s", object_get_typename(OBJECT(dev)));
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 39ab5f47e0..656b0081c2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1934,7 +1934,6 @@ static void virt_memory_pre_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  static void virt_memory_plug(HotplugHandler *hotplug_dev,
>   DeviceState *dev, Error **errp)
>  {
> -HotplugHandlerClass *hhc;
>  VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>  Error *local_err = NULL;
>  
> @@ -1943,8 +1942,9 @@ static void virt_memory_plug(HotplugHandler 
> *hotplug_dev,
>  goto out;
>  }
>  
> -hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> -hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, _abort);
> +hotplug_handler_plug(HOTPLUG_HANDLER(vms->acpi_dev),
> + dev, _abort);
> +
>  out:
>  error_propagate(errp, local_err);
>  }

[PATCH v2] hw/arm: Adjust some coding styles about memory hotplug

[Keqian Zhu]

From: zhukeqian 

There is extra indent in ACPI GED plug cb. And we can use
existing helper function to trigger hotplug handler plug.

Reviewed-by: Igor Mammedov 
Signed-off-by: Keqian Zhu 
---

v1->v2:
 - Add Igor's R-b

Cc: Shameer Kolothum 
Cc: "Michael S. Tsirkin" 
Cc: Igor Mammedov 
Cc: Peter Maydell 
---
 hw/acpi/generic_event_device.c | 2 +-
 hw/arm/virt.c  | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 9cee90cc70..55eb29d80a 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -175,7 +175,7 @@ static void acpi_ged_device_plug_cb(HotplugHandler 
*hotplug_dev,
 AcpiGedState *s = ACPI_GED(hotplug_dev);
 
 if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-acpi_memory_plug_cb(hotplug_dev, >memhp_state, dev, errp);
+acpi_memory_plug_cb(hotplug_dev, >memhp_state, dev, errp);
 } else {
 error_setg(errp, "virt: device plug request for unsupported device"
" type: %s", object_get_typename(OBJECT(dev)));
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 39ab5f47e0..656b0081c2 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1934,7 +1934,6 @@ static void virt_memory_pre_plug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
 static void virt_memory_plug(HotplugHandler *hotplug_dev,
  DeviceState *dev, Error **errp)
 {
-HotplugHandlerClass *hhc;
 VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
 Error *local_err = NULL;
 
@@ -1943,8 +1942,9 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
 goto out;
 }
 
-hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
-hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, _abort);
+hotplug_handler_plug(HOTPLUG_HANDLER(vms->acpi_dev),
+ dev, _abort);
+
 out:
 error_propagate(errp, local_err);
 }
-- 
2.19.1