Re: [PATCH] pci-bridge/xio3130_downstream: fix invalid link speed and link width

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Nam,

On 29/5/24 15:21, Nam Cao wrote:
> Set link width to x1 and link speed to 2.5 Gb/s as specified by the
> datasheet. Without this, these fields in the link status register read
> zero, which is incorrect.
> 
> This problem appeared since 3d67447fe7c2 ("pcie: Fill PCIESlot link fields
> to support higher speeds and widths"), which allows PCIe slot to set link
> width and link speed. However, if PCIe slot does not explicitly set these
> properties, they will be zero. Before this commit, the width and speed
> default to x1 and 2.5 Gb/s.
> 
> Fixes: 3d67447fe7c2 ("pcie: Fill PCIESlot link fields to support higher speeds and widths")
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---
>   hw/pci-bridge/xio3130_downstream.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index 38a2361fa2..d949431191 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -172,10 +172,18 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
>       device_class_set_props(dc, xio3130_downstream_props);
>   }
>   
> +static void xio3130_downstream_instance_post_init(Object *obj)
> +{
> +    PCIESlot *s = PCIE_SLOT(obj);
> +    s->speed = QEMU_PCI_EXP_LNK_2_5GT;
> +    s->width = QEMU_PCI_EXP_LNK_X1;

- You ignore previous value and overwrite
- You don't warn the user about missing config value

Is post_init() the correct way to deal with that? Usually
this is done in realize(), where you can set *errp to
notify the user of invalid values.

> +}
> +
>   static const TypeInfo xio3130_downstream_info = {
>       .name          = TYPE_XIO3130_DOWNSTREAM,
>       .parent        = TYPE_PCIE_SLOT,
>       .class_init    = xio3130_downstream_class_init,
> +    .instance_post_init = xio3130_downstream_instance_post_init,
>       .interfaces = (InterfaceInfo[]) {
>           { INTERFACE_PCIE_DEVICE },
>           { }