| Message ID | 20230420142750.6950-1-Jonathan.Cameron@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hw/pci-bridge: pci_expander_bridge: Fix wrong type and rework inheritance. | expand |
On Thu, Apr 20, 2023 at 03:27:48PM +0100, Jonathan Cameron wrote: > Peter Maydell highlighted an incorrect conversion to TYPE_PXB_DEVICE from > a device that didn't have that a an ancestor type. PXB_DEV() used instead of > PXB_CXL_DEV()/ > > https://lore.kernel.org/qemu-devel/CAFEAcA-+de+eeLCE4YsAw1O-Qyd_4W1Ra05mGDsU_-3a6d92qw@mail.gmail.com/ > > During the discussion it became clear that the inheritance of the various > TYPE_PXB*_DEVICE was unusual. This patchset first provides the minimal > fix then cleans up the inheritance of types based on functionality. > > There is also a rename to TYPE_PXB*_DEV to allow removal of some boilerplate. > > Before this series > TYPE_PXB_DEVICE, TYPE_PXB_PCIE_DEVICE and TYPE_PXB_CXL_DEVICE all > had TYPE_PCI_DEVICE as their direct parent though they shared a common > struct PXBDev for their state. As a result this state contained > some data that was irrelevant for some the types. > > This series changes to > TYPE_PXB_CXL_DEV has a parent of TYPE_PXB_PCIE_DEV > TYPE_PXB_PCIE_DEV has a parent of TYPE_PXB_DEV > TYPE_PXB_DEV continues to have a parent of TYPE_PCI_DEVICE. > > Each of the TYPE_PXB*_DEV has a state structure adding those elements > to their parent that they need. This also allowed dropping a wrapping > structure for the CXL state as the PXBCXLDev structure already provides > the equivalent grouping. > > Patches are similar to those posted in the thread but rebased on v8.0.0. this conflicts with Revert "hw/pxb-cxl: Support passthrough HDM Decoders unless overridden" I think you acked that one? > Jonathan Cameron (2): > hw/pci-bridge: pci_expander_bridge fix type in pxb_cxl_dev_reset() > hw/pci-bridge: Make PCIe and CXL PXB Devices inherit from TYPE_PXB_DEV > > hw/acpi/cxl.c | 11 +++--- > hw/cxl/cxl-host.c | 4 +- > hw/pci-bridge/pci_expander_bridge.c | 61 ++++++++++------------------- > include/hw/cxl/cxl.h | 4 +- > include/hw/pci/pci_bridge.h | 28 +++++++++---- > 5 files changed, 50 insertions(+), 58 deletions(-) > > -- > 2.37.2
On Fri, 21 Apr 2023 at 09:19, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Apr 20, 2023 at 03:27:48PM +0100, Jonathan Cameron wrote: > > Peter Maydell highlighted an incorrect conversion to TYPE_PXB_DEVICE from > > a device that didn't have that a an ancestor type. PXB_DEV() used instead of > > PXB_CXL_DEV()/ > > > > https://lore.kernel.org/qemu-devel/CAFEAcA-+de+eeLCE4YsAw1O-Qyd_4W1Ra05mGDsU_-3a6d92qw@mail.gmail.com/ > > > > During the discussion it became clear that the inheritance of the various > > TYPE_PXB*_DEVICE was unusual. This patchset first provides the minimal > > fix then cleans up the inheritance of types based on functionality. > > > > There is also a rename to TYPE_PXB*_DEV to allow removal of some boilerplate. > > > > Before this series > > TYPE_PXB_DEVICE, TYPE_PXB_PCIE_DEVICE and TYPE_PXB_CXL_DEVICE all > > had TYPE_PCI_DEVICE as their direct parent though they shared a common > > struct PXBDev for their state. As a result this state contained > > some data that was irrelevant for some the types. > > > > This series changes to > > TYPE_PXB_CXL_DEV has a parent of TYPE_PXB_PCIE_DEV > > TYPE_PXB_PCIE_DEV has a parent of TYPE_PXB_DEV > > TYPE_PXB_DEV continues to have a parent of TYPE_PCI_DEVICE. > > > > Each of the TYPE_PXB*_DEV has a state structure adding those elements > > to their parent that they need. This also allowed dropping a wrapping > > structure for the CXL state as the PXBCXLDev structure already provides > > the equivalent grouping. > > > > Patches are similar to those posted in the thread but rebased on v8.0.0. > > this conflicts with > Revert "hw/pxb-cxl: Support passthrough HDM Decoders unless overridden" > > I think you acked that one? We should take one or the other, but not both. If this patchset is good then it's probably better to fix the bug rather than revert the feature, I think. -- PMM
On Fri, 21 Apr 2023 09:59:57 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On Fri, 21 Apr 2023 at 09:19, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Thu, Apr 20, 2023 at 03:27:48PM +0100, Jonathan Cameron wrote: > > > Peter Maydell highlighted an incorrect conversion to TYPE_PXB_DEVICE from > > > a device that didn't have that a an ancestor type. PXB_DEV() used instead of > > > PXB_CXL_DEV()/ > > > > > > https://lore.kernel.org/qemu-devel/CAFEAcA-+de+eeLCE4YsAw1O-Qyd_4W1Ra05mGDsU_-3a6d92qw@mail.gmail.com/ > > > > > > During the discussion it became clear that the inheritance of the various > > > TYPE_PXB*_DEVICE was unusual. This patchset first provides the minimal > > > fix then cleans up the inheritance of types based on functionality. > > > > > > There is also a rename to TYPE_PXB*_DEV to allow removal of some boilerplate. > > > > > > Before this series > > > TYPE_PXB_DEVICE, TYPE_PXB_PCIE_DEVICE and TYPE_PXB_CXL_DEVICE all > > > had TYPE_PCI_DEVICE as their direct parent though they shared a common > > > struct PXBDev for their state. As a result this state contained > > > some data that was irrelevant for some the types. > > > > > > This series changes to > > > TYPE_PXB_CXL_DEV has a parent of TYPE_PXB_PCIE_DEV > > > TYPE_PXB_PCIE_DEV has a parent of TYPE_PXB_DEV > > > TYPE_PXB_DEV continues to have a parent of TYPE_PCI_DEVICE. > > > > > > Each of the TYPE_PXB*_DEV has a state structure adding those elements > > > to their parent that they need. This also allowed dropping a wrapping > > > structure for the CXL state as the PXBCXLDev structure already provides > > > the equivalent grouping. > > > > > > Patches are similar to those posted in the thread but rebased on v8.0.0. > > > > this conflicts with > > Revert "hw/pxb-cxl: Support passthrough HDM Decoders unless overridden" > > > > I think you acked that one? > > We should take one or the other, but not both. If this patchset > is good then it's probably better to fix the bug rather than > revert the feature, I think. If it's easy to drop the revert that would be my preference. If not, then I'm fine spinning a new version of that patch without the bug (so with patch 1 of this squashed in). Patch 2 is somewhat related refactoring. Not necessary to fix the issue even though it was motivated by that bug. Jonathan > > -- PMM
On 21/04/2023 14.19, Jonathan Cameron via wrote: > On Fri, 21 Apr 2023 09:59:57 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: > >> On Fri, 21 Apr 2023 at 09:19, Michael S. Tsirkin <mst@redhat.com> wrote: >>> >>> On Thu, Apr 20, 2023 at 03:27:48PM +0100, Jonathan Cameron wrote: >>>> Peter Maydell highlighted an incorrect conversion to TYPE_PXB_DEVICE from >>>> a device that didn't have that a an ancestor type. PXB_DEV() used instead of >>>> PXB_CXL_DEV()/ >>>> >>>> https://lore.kernel.org/qemu-devel/CAFEAcA-+de+eeLCE4YsAw1O-Qyd_4W1Ra05mGDsU_-3a6d92qw@mail.gmail.com/ >>>> >>>> During the discussion it became clear that the inheritance of the various >>>> TYPE_PXB*_DEVICE was unusual. This patchset first provides the minimal >>>> fix then cleans up the inheritance of types based on functionality. >>>> >>>> There is also a rename to TYPE_PXB*_DEV to allow removal of some boilerplate. >>>> >>>> Before this series >>>> TYPE_PXB_DEVICE, TYPE_PXB_PCIE_DEVICE and TYPE_PXB_CXL_DEVICE all >>>> had TYPE_PCI_DEVICE as their direct parent though they shared a common >>>> struct PXBDev for their state. As a result this state contained >>>> some data that was irrelevant for some the types. >>>> >>>> This series changes to >>>> TYPE_PXB_CXL_DEV has a parent of TYPE_PXB_PCIE_DEV >>>> TYPE_PXB_PCIE_DEV has a parent of TYPE_PXB_DEV >>>> TYPE_PXB_DEV continues to have a parent of TYPE_PCI_DEVICE. >>>> >>>> Each of the TYPE_PXB*_DEV has a state structure adding those elements >>>> to their parent that they need. This also allowed dropping a wrapping >>>> structure for the CXL state as the PXBCXLDev structure already provides >>>> the equivalent grouping. >>>> >>>> Patches are similar to those posted in the thread but rebased on v8.0.0. >>> >>> this conflicts with >>> Revert "hw/pxb-cxl: Support passthrough HDM Decoders unless overridden" >>> >>> I think you acked that one? >> >> We should take one or the other, but not both. If this patchset >> is good then it's probably better to fix the bug rather than >> revert the feature, I think. > > If it's easy to drop the revert that would be my preference. Yes, the patch has not been picked up yet, so we can simply forget about the revert. Thomas
On Fri, Apr 21, 2023 at 01:19:08PM +0100, Jonathan Cameron wrote: > On Fri, 21 Apr 2023 09:59:57 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: > > > On Fri, 21 Apr 2023 at 09:19, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Thu, Apr 20, 2023 at 03:27:48PM +0100, Jonathan Cameron wrote: > > > > Peter Maydell highlighted an incorrect conversion to TYPE_PXB_DEVICE from > > > > a device that didn't have that a an ancestor type. PXB_DEV() used instead of > > > > PXB_CXL_DEV()/ > > > > > > > > https://lore.kernel.org/qemu-devel/CAFEAcA-+de+eeLCE4YsAw1O-Qyd_4W1Ra05mGDsU_-3a6d92qw@mail.gmail.com/ > > > > > > > > During the discussion it became clear that the inheritance of the various > > > > TYPE_PXB*_DEVICE was unusual. This patchset first provides the minimal > > > > fix then cleans up the inheritance of types based on functionality. > > > > > > > > There is also a rename to TYPE_PXB*_DEV to allow removal of some boilerplate. > > > > > > > > Before this series > > > > TYPE_PXB_DEVICE, TYPE_PXB_PCIE_DEVICE and TYPE_PXB_CXL_DEVICE all > > > > had TYPE_PCI_DEVICE as their direct parent though they shared a common > > > > struct PXBDev for their state. As a result this state contained > > > > some data that was irrelevant for some the types. > > > > > > > > This series changes to > > > > TYPE_PXB_CXL_DEV has a parent of TYPE_PXB_PCIE_DEV > > > > TYPE_PXB_PCIE_DEV has a parent of TYPE_PXB_DEV > > > > TYPE_PXB_DEV continues to have a parent of TYPE_PCI_DEVICE. > > > > > > > > Each of the TYPE_PXB*_DEV has a state structure adding those elements > > > > to their parent that they need. This also allowed dropping a wrapping > > > > structure for the CXL state as the PXBCXLDev structure already provides > > > > the equivalent grouping. > > > > > > > > Patches are similar to those posted in the thread but rebased on v8.0.0. > > > > > > this conflicts with > > > Revert "hw/pxb-cxl: Support passthrough HDM Decoders unless overridden" > > > > > > I think you acked that one? > > > > We should take one or the other, but not both. If this patchset > > is good then it's probably better to fix the bug rather than > > revert the feature, I think. > > If it's easy to drop the revert that would be my preference. OK, I'll do that. Thanks! > If not, then I'm fine spinning a new version of that patch without > the bug (so with patch 1 of this squashed in). Patch 2 is somewhat related > refactoring. Not necessary to fix the issue even though it was motivated > by that bug. > > Jonathan > > > > > -- PMM