Re: [PATCH v7 2/4] s390/pci: Reuse FMB buffer and preserve state in device re-enablement

[Omar Elghoul]

On 9/24/26 11:55 AM, Gerd Bayer wrote:
>

[...]

> > > I see a little imbalance of the semantics of "reenable" in
> > > zpci_fmb_reenable_device() vs. zpci_reenable_device():
> > > zpci_reenable_device() "just" enables + registers existing data
> > > structures with the underlying system - while
> > > zpci_fmb_reenable_device() does both the disablement + the enablement.
> >
> > Strictly speaking, the disablement step may not be necessary, provided
> > firmware starts the counters at zero upon changing an FMB address, which
> > does seem to be true in practice. The architecture doesn't explicitly
> > require that though, so I thought it's a reasonable safeguard to use it
> > as an intermediate step that signals firmware to stop counting before we
> > immediately restart measurement after.
>
> Initially, my point was purely "semantics":
> If it is enough for zpci_reenable_device() to do only "enabling"-kind
> of steps, why is zpci_fmb_reenable_device() then also doing some
> "disabling" (under certain conditions). In my eyes the "reenable" was
> actually a "conditional-toggling-on".
>
> And this then led me to checking the paths leading into
> zpci_reenable_device(). I found that all paths would call
> zpci_disable_device() before and that led me to the next question:
>
> > > Since zpci_disable_device() includes the disablement of FMB per
> > > architecture, I wonder if it would suffice to set zdev->fmb_enabled = 0
> > > in that function, and drop the explicit disable FMB there?
> >
> > The semantics of the FMB re-enable function were intended to allow us
> > to re-enable the FMB when we re-enable the device after FMB was
> > implicitly disabled via zpci_disable_device(), like you said. Prior to
> > this patch, there was a sort of "limbo" state where firmware thinks FMB
> > is disabled, but the kernel is unaware of it because it was implicit.
> >
> > For that same reason, I would prefer to not touch zpci_disable_device()
> > at all, neither explicitly disabling FMB nor setting fmb_enabled to 0.
> > The purpose of fmb_enabled variable is to allow us to restore the
> > original FMB enablement when we re-enable the device, and so we want to
> > preserve it here [1].
>
> OK, I see. The whole point was to preserve the FMB enabled state over
> disable/enable sequences on a zdev: Re-enable if (and only if) it was
> enabled before the sequence. So I agree, you must not set zdev-
> > fmb_enabled = 0 in zpci_disable_device(). But you can trust firmware
> to stop updating the FMB buffer (after the ominous grace-period) after
> zpci_disable_device() ran - the zpci_mod_fc() to set FMB to 0 is done
> implicitly in clp_disable_fh().
>
> And you wanted to reuse the zdev->fmb buffer if there ever was one
> allocated for the zdev. It just occurred to me, what good is the whole
> zdev_fmb_cache if the life-time of struct zpci_fmb buffers becomes
> almost as static as struct zpci_dev (short of those that never leave
> "STANDBY"). Couldn't we just kzalloc() a struct zpci_fmb right in
> zpci_create_device()?

We could allocate the FMB buffer in zpci_create_device(). But we cannot
use kzalloc() because the architecture requires it to be 16-byte-aligned
(and apparently on some models it can't even cross a 4K-boundary...) The
existing zpci_fmb struct is 80 bytes and is aligned to the nearest power
of 2 (128 bytes), which I presume is to satisfy this requirement.

Thanks

> > Thanks
> >
> > [1]
> > https://lore.kernel.org/all/dae3c7cd-21aa-4263-bec5-792b018d21e2@xxxxxxxxxxxxx/
>
> [...]
>
> Thank you,
> Gerd