Re: [PATCH v4 01/11] PCI: liveupdate: Set up FLB handler for the PCI core

[David Matlack <dmatlack@google.com>]

On 2026-04-30 11:48 AM, Vipin Sharma wrote:
> On Tue, Apr 28, 2026 at 11:51:37PM +0000, David Matlack wrote:
> > On 2026-04-28 12:45 PM, Vipin Sharma wrote:
> > > On Thu, Apr 23, 2026 at 09:23:05PM +0000, David Matlack wrote:
> > > > +	pr_debug("Preserving struct pci_ser with room for %u devices\n",
> > > > +		 max_nr_devices);
> > > > +
> > > > +	ser = kho_alloc_preserve(size);
> > > > +	if (IS_ERR(ser))
> > > > +		return PTR_ERR(ser);
> > > 
> > > Should there be a similar pr_debug() in case of failure to denote that above
> > > "Preserving ..." message didn't finish, or, maybe just print one
> > > pr_debug() after the error check above?
> > 
> > Hm... I guess there could always be more pr_debug()s but I don't want to
> > instrument every error path. I could move it to the success path but I
> > don't see how that makes it any better.
> > 
> 
> In current way, it is printing that it is preserving pci_ser, but there
> is no indication did it succeed or not in logs. If we are printing the
> logs then may be complete picture will be to know what is the action and its
> result.
> 
> I think moving to success path or printing again based on the failure
> provides assurance of what happened. If this gets printed in happy
> path, then we will know it succeeded in preserving that struct on
> kho.  Absence means it didn't. 
> 
> We can also remove pr_debug(), if this is of no value.

I think I'll just drop these. BPF can be used to trace this function and
what it returns when debugging issues.

> 
> > > 
> > > > +/**
> > > > + * struct pci_dev_ser - Serialized state about a single PCI device.
> > > > + *
> > > > + * @domain: The device's PCI domain number (segment).
> > > > + * @bdf: The device's PCI bus, device, and function number.
> > > > + * @reserved: Reserved (to naturally align struct pci_dev_ser).
> > > > + */
> > > > +struct pci_dev_ser {
> > > > +	u32 domain;
> > > > +	u16 bdf;
> > > > +	u16 reserved;
> > > 
> > > Should this be renamed to 'u8 __padding[2];' instead? This will allow to
> > > just change the array length based on the need (0, 1, 2, 3).
> > 
> > Sorry I'm not following what you mean here. What is the reason to rename
> > this field and change it to an array?
> > 
> 
> Having a padding explicitly tells there is a requirement of being
> aligned. Reserved sounds more like don't use this u16.

It's documented above, but agree padding is a better name.

> If someone add more field down the line say u8, then to make struct size
> aligned they will need to add another u8, u16, u32, and name those
> fields padding or reserved. IMO, having a u8 array named padding makes
> it easier to just change array length as per the need.

This field does get replaced in the next patch. But I see your point
that if we need to pad by an amount other than u8, u16, or u32, then we
would need an array.