[PATCH 0/4] hw/qdev: Housekeeping around qdev_get_parent_bus()

[PATCH 0/4] hw/qdev: Housekeeping around qdev_get_parent_bus()

[Philippe Mathieu-Daudé]

DeviceState::parent_bus is an internal field and should be
accessed by the qdev_get_parent_bus() helper. Replace most uses.

Philippe Mathieu-Daudé (4):
  hw/qdev: Constify DeviceState* argument of qdev_get_parent_bus()
  hw: Replace dev->parent_bus by qdev_get_parent_bus(dev)
  hw: Use qdev_get_parent_bus() in
    qdev_get_own_fw_dev_path_from_handler()
  qdev-monitor: Use qdev_get_parent_bus() in bus_print_dev()

 hw/audio/intel-hda.c                |  2 +-
 hw/block/fdc.c                      |  2 +-
 hw/block/swim.c                     |  2 +-
 hw/core/qdev-fw.c                   |  9 +++++----
 hw/core/qdev.c                      |  2 +-
 hw/ide/qdev.c                       |  4 ++--
 hw/net/virtio-net.c                 |  2 +-
 hw/pci-bridge/pci_expander_bridge.c |  2 +-
 hw/scsi/scsi-bus.c                  |  2 +-
 hw/usb/bus.c                        |  2 +-
 hw/usb/desc.c                       |  2 +-
 hw/usb/dev-smartcard-reader.c       | 16 ++++++++--------
 include/hw/qdev-core.h              |  4 ++--
 softmmu/bootdevice.c                |  2 +-
 softmmu/qdev-monitor.c              |  6 +++---
 15 files changed, 30 insertions(+), 29 deletions(-)

-- 
2.38.1

[PATCH 1/4] hw/qdev: Constify DeviceState* argument of qdev_get_parent_bus()

[Philippe Mathieu-Daudé]

The structure is accessed read-only by qdev_get_parent_bus().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/qdev.c         | 2 +-
 include/hw/qdev-core.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d759c4602c..43d863b0c5 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -330,7 +330,7 @@ bool qdev_machine_modified(void)
     return qdev_hot_added || qdev_hot_removed;
 }
 
-BusState *qdev_get_parent_bus(DeviceState *dev)
+BusState *qdev_get_parent_bus(const DeviceState *dev)
 {
     return dev->parent_bus;
 }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 35fddb19a6..f5b3b2f89a 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -715,7 +715,7 @@ static inline void qdev_init_gpio_in_named(DeviceState *dev,
 void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
                      const char *name);
 
-BusState *qdev_get_parent_bus(DeviceState *dev);
+BusState *qdev_get_parent_bus(const DeviceState *dev);
 
 /*** BUS API. ***/
 
-- 
2.38.1

Re: [PATCH 1/4] hw/qdev: Constify DeviceState* argument of qdev_get_parent_bus()

[Richard Henderson]

On 2/12/23 12:47, Philippe Mathieu-Daudé wrote:
> The structure is accessed read-only by qdev_get_parent_bus().
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/core/qdev.c         | 2 +-
>   include/hw/qdev-core.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/4] hw: Replace dev->parent_bus by qdev_get_parent_bus(dev)

[Philippe Mathieu-Daudé]

DeviceState::parent_bus is an internal field and should be
accessed by the qdev_get_parent_bus() helper. Replace all
uses in hw/ except the QDev uses in hw/core/.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/audio/intel-hda.c                |  2 +-
 hw/block/fdc.c                      |  2 +-
 hw/block/swim.c                     |  2 +-
 hw/ide/qdev.c                       |  4 ++--
 hw/net/virtio-net.c                 |  2 +-
 hw/pci-bridge/pci_expander_bridge.c |  2 +-
 hw/scsi/scsi-bus.c                  |  2 +-
 hw/usb/bus.c                        |  2 +-
 hw/usb/desc.c                       |  2 +-
 hw/usb/dev-smartcard-reader.c       | 16 ++++++++--------
 10 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index b9ed231fe8..6bc239a981 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -59,7 +59,7 @@ void hda_codec_bus_init(DeviceState *dev, HDACodecBus *bus, size_t bus_size,
 
 static void hda_codec_dev_realize(DeviceState *qdev, Error **errp)
 {
-    HDACodecBus *bus = HDA_BUS(qdev->parent_bus);
+    HDACodecBus *bus = HDA_BUS(qdev_get_parent_bus(qdev));
     HDACodecDevice *dev = HDA_CODEC_DEVICE(qdev);
     HDACodecDeviceClass *cdc = HDA_CODEC_DEVICE_GET_CLASS(dev);
 
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 64ae4a6899..31ad6f6ae0 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -466,7 +466,7 @@ static Property floppy_drive_properties[] = {
 static void floppy_drive_realize(DeviceState *qdev, Error **errp)
 {
     FloppyDrive *dev = FLOPPY_DRIVE(qdev);
-    FloppyBus *bus = FLOPPY_BUS(qdev->parent_bus);
+    FloppyBus *bus = FLOPPY_BUS(qdev_get_parent_bus(qdev));
     FDrive *drive;
     bool read_only;
     int ret;
diff --git a/hw/block/swim.c b/hw/block/swim.c
index 333da08ce0..64e30e9e80 100644
--- a/hw/block/swim.c
+++ b/hw/block/swim.c
@@ -157,7 +157,7 @@ static Property swim_drive_properties[] = {
 static void swim_drive_realize(DeviceState *qdev, Error **errp)
 {
     SWIMDrive *dev = SWIM_DRIVE(qdev);
-    SWIMBus *bus = SWIM_BUS(qdev->parent_bus);
+    SWIMBus *bus = SWIM_BUS(qdev_get_parent_bus(qdev));
     FDrive *drive;
     int ret;
 
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6f6c7462f3..96582ce49b 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -81,7 +81,7 @@ static char *idebus_get_fw_dev_path(DeviceState *dev)
     char path[30];
 
     snprintf(path, sizeof(path), "%s@%x", qdev_fw_name(dev),
-             ((IDEBus*)dev->parent_bus)->bus_id);
+             ((IDEBus*)qdev_get_parent_bus(dev))->bus_id);
 
     return g_strdup(path);
 }
@@ -90,7 +90,7 @@ static void ide_qdev_realize(DeviceState *qdev, Error **errp)
 {
     IDEDevice *dev = IDE_DEVICE(qdev);
     IDEDeviceClass *dc = IDE_DEVICE_GET_CLASS(dev);
-    IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus);
+    IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev_get_parent_bus(qdev));
 
     if (dev->unit == -1) {
         dev->unit = bus->master ? 1 : 0;
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3ae909041a..8bc160ab59 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3423,7 +3423,7 @@ static bool failover_replug_primary(VirtIONet *n, DeviceState *dev,
     if (!pdev->partially_hotplugged) {
         return true;
     }
-    primary_bus = dev->parent_bus;
+    primary_bus = qdev_get_parent_bus(dev);
     if (!primary_bus) {
         error_setg(errp, "virtio_net: couldn't find primary bus");
         return false;
diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index e752a21292..8c0649c071 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -151,7 +151,7 @@ static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
     assert(position >= 0);
 
     pxb_dev_base = DEVICE(pxb_dev);
-    main_host = PCI_HOST_BRIDGE(pxb_dev_base->parent_bus->parent);
+    main_host = PCI_HOST_BRIDGE(qdev_get_parent_bus(pxb_dev_base)->parent);
     main_host_sbd = SYS_BUS_DEVICE(main_host);
 
     if (main_host_sbd->num_mmio > 0) {
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index ceceafb2cd..3127cd7273 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1671,7 +1671,7 @@ void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense)
 static char *scsibus_get_dev_path(DeviceState *dev)
 {
     SCSIDevice *d = SCSI_DEVICE(dev);
-    DeviceState *hba = dev->parent_bus->parent;
+    DeviceState *hba = qdev_get_parent_bus(dev)->parent;
     char *id;
     char *path;
 
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 92d6ed5626..d7c3c71435 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -595,7 +595,7 @@ static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
 static char *usb_get_dev_path(DeviceState *qdev)
 {
     USBDevice *dev = USB_DEVICE(qdev);
-    DeviceState *hcd = qdev->parent_bus->parent;
+    DeviceState *hcd = qdev_get_parent_bus(qdev)->parent;
     char *id = qdev_get_dev_path(hcd);
 
     if (id) {
diff --git a/hw/usb/desc.c b/hw/usb/desc.c
index 7f6cc2f99b..2646515e26 100644
--- a/hw/usb/desc.c
+++ b/hw/usb/desc.c
@@ -553,7 +553,7 @@ void usb_desc_set_string(USBDevice *dev, uint8_t index, const char *str)
  */
 void usb_desc_create_serial(USBDevice *dev)
 {
-    DeviceState *hcd = dev->qdev.parent_bus->parent;
+    DeviceState *hcd = qdev_get_parent_bus(DEVICE(dev))->parent;
     const USBDesc *desc = usb_device_get_usb_desc(dev);
     int index = desc->id.iSerialNumber;
     char *path, *serial;
diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
index 28164d89be..5e94b4f64a 100644
--- a/hw/usb/dev-smartcard-reader.c
+++ b/hw/usb/dev-smartcard-reader.c
@@ -1187,7 +1187,7 @@ void ccid_card_send_apdu_to_guest(CCIDCardState *card,
                                   uint8_t *apdu, uint32_t len)
 {
     DeviceState *qdev = DEVICE(card);
-    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
+    USBDevice *dev = USB_DEVICE(qdev_get_parent_bus(qdev)->parent);
     USBCCIDState *s = USB_CCID_DEV(dev);
     Answer *answer;
 
@@ -1210,7 +1210,7 @@ void ccid_card_send_apdu_to_guest(CCIDCardState *card,
 void ccid_card_card_removed(CCIDCardState *card)
 {
     DeviceState *qdev = DEVICE(card);
-    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
+    USBDevice *dev = USB_DEVICE(qdev_get_parent_bus(qdev)->parent);
     USBCCIDState *s = USB_CCID_DEV(dev);
 
     ccid_on_slot_change(s, false);
@@ -1221,7 +1221,7 @@ void ccid_card_card_removed(CCIDCardState *card)
 int ccid_card_ccid_attach(CCIDCardState *card)
 {
     DeviceState *qdev = DEVICE(card);
-    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
+    USBDevice *dev = USB_DEVICE(qdev_get_parent_bus(qdev)->parent);
     USBCCIDState *s = USB_CCID_DEV(dev);
 
     DPRINTF(s, 1, "CCID Attach\n");
@@ -1231,7 +1231,7 @@ int ccid_card_ccid_attach(CCIDCardState *card)
 void ccid_card_ccid_detach(CCIDCardState *card)
 {
     DeviceState *qdev = DEVICE(card);
-    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
+    USBDevice *dev = USB_DEVICE(qdev_get_parent_bus(qdev)->parent);
     USBCCIDState *s = USB_CCID_DEV(dev);
 
     DPRINTF(s, 1, "CCID Detach\n");
@@ -1244,7 +1244,7 @@ void ccid_card_ccid_detach(CCIDCardState *card)
 void ccid_card_card_error(CCIDCardState *card, uint64_t error)
 {
     DeviceState *qdev = DEVICE(card);
-    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
+    USBDevice *dev = USB_DEVICE(qdev_get_parent_bus(qdev)->parent);
     USBCCIDState *s = USB_CCID_DEV(dev);
 
     s->bmCommandStatus = COMMAND_STATUS_FAILED;
@@ -1263,7 +1263,7 @@ void ccid_card_card_error(CCIDCardState *card, uint64_t error)
 void ccid_card_card_inserted(CCIDCardState *card)
 {
     DeviceState *qdev = DEVICE(card);
-    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
+    USBDevice *dev = USB_DEVICE(qdev_get_parent_bus(qdev)->parent);
     USBCCIDState *s = USB_CCID_DEV(dev);
 
     s->bmCommandStatus = COMMAND_STATUS_NO_ERROR;
@@ -1275,7 +1275,7 @@ static void ccid_card_unrealize(DeviceState *qdev)
 {
     CCIDCardState *card = CCID_CARD(qdev);
     CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
-    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
+    USBDevice *dev = USB_DEVICE(qdev_get_parent_bus(qdev)->parent);
     USBCCIDState *s = USB_CCID_DEV(dev);
 
     if (ccid_card_inserted(s)) {
@@ -1291,7 +1291,7 @@ static void ccid_card_realize(DeviceState *qdev, Error **errp)
 {
     CCIDCardState *card = CCID_CARD(qdev);
     CCIDCardClass *cc = CCID_CARD_GET_CLASS(card);
-    USBDevice *dev = USB_DEVICE(qdev->parent_bus->parent);
+    USBDevice *dev = USB_DEVICE(qdev_get_parent_bus(qdev)->parent);
     USBCCIDState *s = USB_CCID_DEV(dev);
     Error *local_err = NULL;
 
-- 
2.38.1

Re: [PATCH 2/4] hw: Replace dev->parent_bus by qdev_get_parent_bus(dev)

[Philippe Mathieu-Daudé]

On 12/2/23 23:47, Philippe Mathieu-Daudé wrote:
> DeviceState::parent_bus is an internal field and should be
> accessed by the qdev_get_parent_bus() helper. Replace all
> uses in hw/ except the QDev uses in hw/core/.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/audio/intel-hda.c                |  2 +-
>   hw/block/fdc.c                      |  2 +-
>   hw/block/swim.c                     |  2 +-
>   hw/ide/qdev.c                       |  4 ++--
>   hw/net/virtio-net.c                 |  2 +-
>   hw/pci-bridge/pci_expander_bridge.c |  2 +-
>   hw/scsi/scsi-bus.c                  |  2 +-
>   hw/usb/bus.c                        |  2 +-
>   hw/usb/desc.c                       |  2 +-
>   hw/usb/dev-smartcard-reader.c       | 16 ++++++++--------
>   10 files changed, 18 insertions(+), 18 deletions(-)

I missed:

-- >8 --
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 6bc239a981..ec38828da0 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -103,14 +103,14 @@ HDACodecDevice *hda_codec_find(HDACodecBus *bus, 
uint32_t cad)

  void hda_codec_response(HDACodecDevice *dev, bool solicited, uint32_t 
response)
  {
-    HDACodecBus *bus = HDA_BUS(dev->qdev.parent_bus);
+    HDACodecBus *bus = HDA_BUS(qdev_get_parent_bus(DEVICE(dev)));
      bus->response(dev, solicited, response);
  }

  bool hda_codec_xfer(HDACodecDevice *dev, uint32_t stnr, bool output,
                      uint8_t *buf, uint32_t len)
  {
-    HDACodecBus *bus = HDA_BUS(dev->qdev.parent_bus);
+    HDACodecBus *bus = HDA_BUS(qdev_get_parent_bus(DEVICE(dev)));
      return bus->xfer(dev, stnr, output, buf, len);
  }

@@ -344,7 +344,7 @@ static void intel_hda_corb_run(IntelHDAState *d)
  static void intel_hda_response(HDACodecDevice *dev, bool solicited, 
uint32_t response)
  {
      const MemTxAttrs attrs = { .memory = true };
-    HDACodecBus *bus = HDA_BUS(dev->qdev.parent_bus);
+    HDACodecBus *bus = HDA_BUS(qdev_get_parent_bus(DEVICE(dev)));
      IntelHDAState *d = container_of(bus, IntelHDAState, codecs);
      hwaddr addr;
      uint32_t wp, ex;
@@ -399,7 +399,7 @@ static bool intel_hda_xfer(HDACodecDevice *dev, 
uint32_t stnr, bool output,
                             uint8_t *buf, uint32_t len)
  {
      const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
-    HDACodecBus *bus = HDA_BUS(dev->qdev.parent_bus);
+    HDACodecBus *bus = HDA_BUS(qdev_get_parent_bus(DEVICE(dev)));
      IntelHDAState *d = container_of(bus, IntelHDAState, codecs);
      hwaddr addr;
      uint32_t s, copy, left;
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 96582ce49b..6ae2627a56 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -164,7 +164,7 @@ typedef struct IDEDrive {

  static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error 
**errp)
  {
-    IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
+    IDEBus *bus = DO_UPCAST(IDEBus, qbus, 
qdev_get_parent_bus(DEVICE(dev)));
      IDEState *s = bus->ifs + dev->unit;
      int ret;

diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 9d4fec2c04..dfc5c436bd 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -382,7 +382,7 @@ static void rtas_quiesce(PowerPCCPU *cpu, 
SpaprMachineState *spapr,

  static SpaprVioDevice *reg_conflict(SpaprVioDevice *dev)
  {
-    SpaprVioBus *bus = SPAPR_VIO_BUS(dev->qdev.parent_bus);
+    SpaprVioBus *bus = SPAPR_VIO_BUS(qdev_get_parent_bus(DEVICE(dev)));
      BusChild *kid;
      SpaprVioDevice *other;

@@ -492,7 +492,7 @@ static void spapr_vio_busdev_realize(DeviceState 
*qdev, Error **errp)
          }
      } else {
          /* Need to assign an address */
-        SpaprVioBus *bus = SPAPR_VIO_BUS(dev->qdev.parent_bus);
+        SpaprVioBus *bus = SPAPR_VIO_BUS(qdev_get_parent_bus(DEVICE(dev)));

          do {
              dev->reg = bus->next_reg++;
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 3127cd7273..7b2a82b335 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -104,7 +104,7 @@ static void scsi_device_unrealize(SCSIDevice *s)
  int scsi_bus_parse_cdb(SCSIDevice *dev, SCSICommand *cmd, uint8_t *buf,
                         size_t buf_len, void *hba_private)
  {
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, 
qdev_get_parent_bus(DEVICE(dev)));
      int rc;

      assert(cmd->len == 0);
@@ -250,7 +250,7 @@ static bool scsi_bus_check_address(BusState *qbus, 
DeviceState *qdev, Error **er
  static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
  {
      SCSIDevice *dev = SCSI_DEVICE(qdev);
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, 
qdev_get_parent_bus(DEVICE(dev)));
      bool is_free;
      Error *local_err = NULL;

@@ -705,7 +705,7 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps 
*reqops, SCSIDevice *d,
  SCSIRequest *scsi_req_new(SCSIDevice *d, uint32_t tag, uint32_t lun,
                            uint8_t *buf, size_t buf_len, void *hba_private)
  {
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus);
+    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, 
qdev_get_parent_bus(DEVICE(d)));
      const SCSIReqOps *ops;
      SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(d);
      SCSIRequest *req;
@@ -1353,7 +1353,7 @@ int scsi_req_parse_cdb(SCSIDevice *dev, 
SCSICommand *cmd, uint8_t *buf,

  void scsi_device_report_change(SCSIDevice *dev, SCSISense sense)
  {
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, 
qdev_get_parent_bus(DEVICE(dev)));

      scsi_device_set_ua(dev, sense);
      if (bus->info->change) {
@@ -1372,7 +1372,7 @@ void scsi_req_unref(SCSIRequest *req)
  {
      assert(req->refcount > 0);
      if (--req->refcount == 0) {
-        BusState *qbus = req->dev->qdev.parent_bus;
+        BusState *qbus = qdev_get_parent_bus(DEVICE(req->dev));
          SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, qbus);

          if (bus->info->free_request && req->hba_private) {
@@ -1444,7 +1444,7 @@ void scsi_req_print(SCSIRequest *req)
      int i;

      fprintf(fp, "[%s id=%d] %s",
-            req->dev->qdev.parent_bus->name,
+            qdev_get_parent_bus(DEVICE(req->dev))->name,
              req->dev->id,
              scsi_command_name(req->cmd.buf[0]));
      for (i = 1; i < req->cmd.len; i++) {
@@ -1698,7 +1698,7 @@ static int put_scsi_requests(QEMUFile *f, void 
*pv, size_t size,
                               const VMStateField *field, JSONWriter 
*vmdesc)
  {
      SCSIDevice *s = pv;
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, s->qdev.parent_bus);
+    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, 
qdev_get_parent_bus(DEVICE(s)));
      SCSIRequest *req;

      QTAILQ_FOREACH(req, &s->requests, next) {
@@ -1726,7 +1726,7 @@ static int get_scsi_requests(QEMUFile *f, void 
*pv, size_t size,
                               const VMStateField *field)
  {
      SCSIDevice *s = pv;
-    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, s->qdev.parent_bus);
+    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, 
qdev_get_parent_bus(DEVICE(s)));
      int8_t sbyte;

      while ((sbyte = qemu_get_sbyte(f)) > 0) {
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 6ea4b64fe7..843dde8851 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -177,7 +177,7 @@ static inline void scsi_bus_init(SCSIBus *bus, 
size_t bus_size,

  static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
  {
-    return DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus);
+    return DO_UPCAST(SCSIBus, qbus, qdev_get_parent_bus(DEVICE(d)));
  }

  SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
diff --git a/include/hw/usb.h b/include/hw/usb.h
index 32c23a5ca2..b2111bb1c7 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -520,7 +520,7 @@ void usb_check_attach(USBDevice *dev, Error **errp);

  static inline USBBus *usb_bus_from_device(USBDevice *d)
  {
-    return DO_UPCAST(USBBus, qbus, d->qdev.parent_bus);
+    return DO_UPCAST(USBBus, qbus, qdev_get_parent_bus(DEVICE(d)));
  }

  extern const VMStateDescription vmstate_usb_device;
---

Re: [PATCH 2/4] hw: Replace dev->parent_bus by qdev_get_parent_bus(dev)

[Richard Henderson]

On 2/12/23 13:03, Philippe Mathieu-Daudé wrote:
> On 12/2/23 23:47, Philippe Mathieu-Daudé wrote:
>> DeviceState::parent_bus is an internal field and should be
>> accessed by the qdev_get_parent_bus() helper. Replace all
>> uses in hw/ except the QDev uses in hw/core/.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/audio/intel-hda.c                |  2 +-
>>   hw/block/fdc.c                      |  2 +-
>>   hw/block/swim.c                     |  2 +-
>>   hw/ide/qdev.c                       |  4 ++--
>>   hw/net/virtio-net.c                 |  2 +-
>>   hw/pci-bridge/pci_expander_bridge.c |  2 +-
>>   hw/scsi/scsi-bus.c                  |  2 +-
>>   hw/usb/bus.c                        |  2 +-
>>   hw/usb/desc.c                       |  2 +-
>>   hw/usb/dev-smartcard-reader.c       | 16 ++++++++--------
>>   10 files changed, 18 insertions(+), 18 deletions(-)
> 
> I missed:

Did you use a temporary rename of the field to catch all the uses?

>   void hda_codec_response(HDACodecDevice *dev, bool solicited, uint32_t response)
>   {
> -    HDACodecBus *bus = HDA_BUS(dev->qdev.parent_bus);
> +    HDACodecBus *bus = HDA_BUS(qdev_get_parent_bus(DEVICE(dev)));

I'm never sure the cast is clearer than &dev->qdev.
But it seems the normal way in qemu...

Acked-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 2/4] hw: Replace dev->parent_bus by qdev_get_parent_bus(dev)

[Philippe Mathieu-Daudé]

On 14/2/23 00:19, Richard Henderson wrote:
> On 2/12/23 13:03, Philippe Mathieu-Daudé wrote:
>> On 12/2/23 23:47, Philippe Mathieu-Daudé wrote:
>>> DeviceState::parent_bus is an internal field and should be
>>> accessed by the qdev_get_parent_bus() helper. Replace all
>>> uses in hw/ except the QDev uses in hw/core/.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/audio/intel-hda.c                |  2 +-
>>>   hw/block/fdc.c                      |  2 +-
>>>   hw/block/swim.c                     |  2 +-
>>>   hw/ide/qdev.c                       |  4 ++--
>>>   hw/net/virtio-net.c                 |  2 +-
>>>   hw/pci-bridge/pci_expander_bridge.c |  2 +-
>>>   hw/scsi/scsi-bus.c                  |  2 +-
>>>   hw/usb/bus.c                        |  2 +-
>>>   hw/usb/desc.c                       |  2 +-
>>>   hw/usb/dev-smartcard-reader.c       | 16 ++++++++--------
>>>   10 files changed, 18 insertions(+), 18 deletions(-)
>>
>> I missed:
> 
> Did you use a temporary rename of the field to catch all the uses?

No, git-grep. Good idea.

>>   void hda_codec_response(HDACodecDevice *dev, bool solicited, 
>> uint32_t response)
>>   {
>> -    HDACodecBus *bus = HDA_BUS(dev->qdev.parent_bus);
>> +    HDACodecBus *bus = HDA_BUS(qdev_get_parent_bus(DEVICE(dev)));
> 
> I'm never sure the cast is clearer than &dev->qdev.

Maybe this one isn't obvious, but see for QOM macros use:
https://lore.kernel.org/qemu-devel/20230213170145.45666-3-philmd@linaro.org/:

-    vcdev->vdev.dev = &vcdev->cdev.parent_obj.parent_obj;
+    vcdev->vdev.dev = DEVICE(vcdev);

We should agree on how we want to use this API. If the
DeviceState::parent_bus field isn't considered internal,
the we should remove the qdev_get_parent_bus() helper which
is simply:

hw/core/qdev.c:333:BusState *qdev_get_parent_bus(DeviceState *dev)
hw/core/qdev.c-334-{
hw/core/qdev.c-335-    return dev->parent_bus;
hw/core/qdev.c-336-}

Note the alternate series expanding QDev macros:
https://lore.kernel.org/qemu-devel/20230213105609.6173-1-philmd@linaro.org/

> But it seems the normal way in qemu...
> 
> Acked-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!

[PATCH 3/4] hw: Use qdev_get_parent_bus() in qdev_get_own_fw_dev_path_from_handler()

[Philippe Mathieu-Daudé]

No need to pass 'dev' and 'dev->parent_bus' when we can
retrieve 'parent_bus' with qdev_get_parent_bus().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/qdev-fw.c      | 9 +++++----
 include/hw/qdev-core.h | 2 +-
 softmmu/bootdevice.c   | 2 +-
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/core/qdev-fw.c b/hw/core/qdev-fw.c
index a31958355f..c2df1f4796 100644
--- a/hw/core/qdev-fw.c
+++ b/hw/core/qdev-fw.c
@@ -41,9 +41,10 @@ static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
     return NULL;
 }
 
-static char *qdev_get_fw_dev_path_from_handler(BusState *bus, DeviceState *dev)
+static char *qdev_get_fw_dev_path_from_handler(DeviceState *dev)
 {
     Object *obj = OBJECT(dev);
+    BusState *bus = qdev_get_parent_bus(dev);
     char *d = NULL;
 
     while (!d && obj->parent) {
@@ -53,11 +54,11 @@ static char *qdev_get_fw_dev_path_from_handler(BusState *bus, DeviceState *dev)
     return d;
 }
 
-char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev)
+char *qdev_get_own_fw_dev_path_from_handler(DeviceState *dev)
 {
     Object *obj = OBJECT(dev);
 
-    return fw_path_provider_try_get_dev_path(obj, bus, dev);
+    return fw_path_provider_try_get_dev_path(obj, qdev_get_parent_bus(dev), dev);
 }
 
 static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
@@ -67,7 +68,7 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
     if (dev && dev->parent_bus) {
         char *d;
         l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
-        d = qdev_get_fw_dev_path_from_handler(dev->parent_bus, dev);
+        d = qdev_get_fw_dev_path_from_handler(dev);
         if (!d) {
             d = bus_get_fw_dev_path(dev->parent_bus, dev);
         }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index f5b3b2f89a..93718be156 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -774,7 +774,7 @@ bool bus_is_in_reset(BusState *bus);
 BusState *sysbus_get_default(void);
 
 char *qdev_get_fw_dev_path(DeviceState *dev);
-char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev);
+char *qdev_get_own_fw_dev_path_from_handler(DeviceState *dev);
 
 void device_class_set_props(DeviceClass *dc, Property *props);
 
diff --git a/softmmu/bootdevice.c b/softmmu/bootdevice.c
index 2106f1026f..7834bf3333 100644
--- a/softmmu/bootdevice.c
+++ b/softmmu/bootdevice.c
@@ -214,7 +214,7 @@ static char *get_boot_device_path(DeviceState *dev, bool ignore_suffixes,
 
     if (!ignore_suffixes) {
         if (dev) {
-            d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);
+            d = qdev_get_own_fw_dev_path_from_handler(dev);
             if (d) {
                 assert(!suffix);
                 s = d;
-- 
2.38.1

Re: [PATCH 3/4] hw: Use qdev_get_parent_bus() in qdev_get_own_fw_dev_path_from_handler()

[Richard Henderson]

On 2/12/23 12:47, Philippe Mathieu-Daudé wrote:
> -static char *qdev_get_fw_dev_path_from_handler(BusState *bus, DeviceState *dev)
> +static char *qdev_get_fw_dev_path_from_handler(DeviceState *dev)
>   {
>       Object *obj = OBJECT(dev);
> +    BusState *bus = qdev_get_parent_bus(dev);
>       char *d = NULL;
>   
>       while (!d && obj->parent) {

This is a separate change from...

> -char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev)
> +char *qdev_get_own_fw_dev_path_from_handler(DeviceState *dev)
>   {
>       Object *obj = OBJECT(dev);
>   
> -    return fw_path_provider_try_get_dev_path(obj, bus, dev);
> +    return fw_path_provider_try_get_dev_path(obj, qdev_get_parent_bus(dev), dev);

... this, which is what $SUBJECT says.

> @@ -67,7 +68,7 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>       if (dev && dev->parent_bus) {
>           char *d;
>           l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
> -        d = qdev_get_fw_dev_path_from_handler(dev->parent_bus, dev);
> +        d = qdev_get_fw_dev_path_from_handler(dev);

We've already accessed parent_bus just above

>           if (!d) {
>               d = bus_get_fw_dev_path(dev->parent_bus, dev);

... and just below.  So, what's the cleanup?


r~

Re: [PATCH 3/4] hw: Use qdev_get_parent_bus() in qdev_get_own_fw_dev_path_from_handler()

[Philippe Mathieu-Daudé]

On 14/2/23 00:29, Richard Henderson wrote:
> On 2/12/23 12:47, Philippe Mathieu-Daudé wrote:
>> -static char *qdev_get_fw_dev_path_from_handler(BusState *bus, 
>> DeviceState *dev)
>> +static char *qdev_get_fw_dev_path_from_handler(DeviceState *dev)
>>   {
>>       Object *obj = OBJECT(dev);
>> +    BusState *bus = qdev_get_parent_bus(dev);
>>       char *d = NULL;
>>       while (!d && obj->parent) {
> 
> This is a separate change from...
> 
>> -char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, 
>> DeviceState *dev)
>> +char *qdev_get_own_fw_dev_path_from_handler(DeviceState *dev)
>>   {
>>       Object *obj = OBJECT(dev);
>> -    return fw_path_provider_try_get_dev_path(obj, bus, dev);
>> +    return fw_path_provider_try_get_dev_path(obj, 
>> qdev_get_parent_bus(dev), dev);
> 
> ... this, which is what $SUBJECT says.
> 
>> @@ -67,7 +68,7 @@ static int qdev_get_fw_dev_path_helper(DeviceState 
>> *dev, char *p, int size)
>>       if (dev && dev->parent_bus) {
>>           char *d;
>>           l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, 
>> size);
>> -        d = qdev_get_fw_dev_path_from_handler(dev->parent_bus, dev);
>> +        d = qdev_get_fw_dev_path_from_handler(dev);
> 
> We've already accessed parent_bus just above
> 
>>           if (!d) {
>>               d = bus_get_fw_dev_path(dev->parent_bus, dev);
> 
> ... and just below.  So, what's the cleanup?

qdev_get_own_fw_dev_path_from_handler() being a public API, I wanted to
clean it to avoid a funny case when it is called with
bus != qdev_get_parent_bus(dev). Maybe I merged 2 patches in one, I'll
revisit. Or I can just add assert(bus == qdev_get_parent_bus(dev)) to
prove the API is convoluted. I'll reword on before respin.

[PATCH 4/4] qdev-monitor: Use qdev_get_parent_bus() in bus_print_dev()

[Philippe Mathieu-Daudé]

No need to pass 'dev' and 'dev->parent_bus' when we can
retrieve 'parent_bus' with qdev_get_parent_bus().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 softmmu/qdev-monitor.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 820e7f52ad..12e4899f0d 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -770,9 +770,9 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
     }
 }
 
-static void bus_print_dev(BusState *bus, Monitor *mon, DeviceState *dev, int indent)
+static void bus_print_dev(Monitor *mon, DeviceState *dev, int indent)
 {
-    BusClass *bc = BUS_GET_CLASS(bus);
+    BusClass *bc = BUS_GET_CLASS(qdev_get_parent_bus(dev));
 
     if (bc->print_dev) {
         bc->print_dev(mon, dev, indent);
@@ -811,7 +811,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
         qdev_print_props(mon, dev, DEVICE_CLASS(class)->props_, indent);
         class = object_class_get_parent(class);
     } while (class != object_class_by_name(TYPE_DEVICE));
-    bus_print_dev(dev->parent_bus, mon, dev, indent);
+    bus_print_dev(mon, dev, indent);
     QLIST_FOREACH(child, &dev->child_bus, sibling) {
         qbus_print(mon, child, indent);
     }
-- 
2.38.1

Re: [PATCH 4/4] qdev-monitor: Use qdev_get_parent_bus() in bus_print_dev()

[Richard Henderson]

On 2/12/23 12:47, Philippe Mathieu-Daudé wrote:
> No need to pass 'dev' and 'dev->parent_bus' when we can
> retrieve 'parent_bus' with qdev_get_parent_bus().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   softmmu/qdev-monitor.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

> 
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 820e7f52ad..12e4899f0d 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -770,9 +770,9 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
>       }
>   }
>   
> -static void bus_print_dev(BusState *bus, Monitor *mon, DeviceState *dev, int indent)
> +static void bus_print_dev(Monitor *mon, DeviceState *dev, int indent)
>   {
> -    BusClass *bc = BUS_GET_CLASS(bus);
> +    BusClass *bc = BUS_GET_CLASS(qdev_get_parent_bus(dev));
>   
>       if (bc->print_dev) {
>           bc->print_dev(mon, dev, indent);
> @@ -811,7 +811,7 @@ static void qdev_print(Monitor *mon, DeviceState *dev, int indent)
>           qdev_print_props(mon, dev, DEVICE_CLASS(class)->props_, indent);
>           class = object_class_get_parent(class);
>       } while (class != object_class_by_name(TYPE_DEVICE));
> -    bus_print_dev(dev->parent_bus, mon, dev, indent);
> +    bus_print_dev(mon, dev, indent);
>       QLIST_FOREACH(child, &dev->child_bus, sibling) {
>           qbus_print(mon, child, indent);
>       }