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

[Omar Elghoul <oelghoul@linux.ibm.com>]

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@linux.ibm.com/
>>
>>
> 
> [...]
> 
> Thank you,
> Gerd