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

[Nam Cao <namcao@linutronix.de>]

On Wed, May 29, 2024 at 04:17:04PM +0200, Nam Cao wrote:
> On Wed, May 29, 2024 at 03:50:14PM +0200, Philippe Mathieu-Daudé wrote:
> > 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
> 
> Users cannot set speed/width for this device. At the moment, only PCI root
> port supports that.
> 
> It wouldn't make sense for users to set these properties nonetheless, these
> properties are fixed according to datasheet.
> 
> > 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.
> 
> I stole this from hw/pci-bridge/cxl_downstream.c and
> hw/pci-bridge/pcie_root_port.c. So I think it's correct.

Ah wait, now I read those files again, I think those devices do it this way
because they want to have default values.

For this case, I think you are right that realize() is the more suitable
place because I am not setting the default, I am setting the fixed values.

I can send a v2 with that change, unless you have other comments?

Best regards,
Nam

> I am new to QEMU code, so please tell me if you think this is not the best
> approach.
> 
> Best regards,
> Nam
>