Re: [PATCH v11 3/5] s390/pci: Reuse FMB buffer across enable and disable cycles

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

On 10/28/26 11:59 AM, sashiko-bot@kernel.org wrote:
[...]
> 
> [Severity: Medium]
> Can keeping zdev->fmb non-NULL when zpci_mod_fc() fails in zpci_fmb_enable()
> leave the FMB enablement state inconsistent?

Valid concern, but unfortunately I think this is a tradeoff we are going
to have to accept. Based on this discussion [1] under a previous version
of the series, we can accept these stale reads because as Niklas said,
the firmware-written sample includes a timestamp of the last write. So
we're already transparent about giving the user stale data, and in the
unlikely event that this happens, the user will be able to recover by
disabling and re-enabling statistics via debugfs or simply re-enabling
it directly via vfio.

Thanks

[1] 
https://lore.kernel.org/all/a24337bd66864dd70ae61e28d86590052ac0fc5d.camel@linux.ibm.com/

> 
> zdev->statistics_on is initialized to true in zpci_create_device(). When
> pcibios_enable_device() or zpci_reenable_device() calls zpci_fmb_enable(),
> the return value is ignored and zdev->statistics_on remains true:
> 
> arch/s390/pci/pci.c:pcibios_enable_device() {
> 	...
> 	mutex_lock(&zdev->fmb_lock);
> 	if (zdev->statistics_on)
> 		zpci_fmb_enable(zdev);
> 	mutex_unlock(&zdev->fmb_lock);
> 	...
> }
> 
> arch/s390/pci/pci.c:zpci_reenable_device() {
> 	...
> 	guard(mutex)(&zdev->fmb_lock);
> 	if (zdev->statistics_on)
> 		zpci_fmb_enable(zdev);
> 	...
> }
> 
> Previously, zpci_fmb_enable() freed zdev->fmb and reset it to NULL on
> zpci_mod_fc() failure, and zpci_fmb_disable() reset zdev->fmb to NULL on
> disable. With zdev->fmb kept allocated both across disable/re-enable cycles
> and when zpci_mod_fc() fails (or if zpci_reenable_device() returns early on an
> error after a prior allocation), both zdev->statistics_on and zdev->fmb remain
> non-zero even though hardware measurement is not active.
> 
> In arch/s390/pci/pci_debug.c, pci_perf_show() checks whether FMB is disabled
> using both fields:
> 
> arch/s390/pci/pci_debug.c:pci_perf_show() {
> 	...
> 	mutex_lock(&zdev->fmb_lock);
> 	if (!zdev->statistics_on || !zdev->fmb) {
> 		mutex_unlock(&zdev->fmb_lock);
> 		seq_puts(m, "FMB statistics disabled\n");
> 		return 0;
> 	}
> 	...
> }
> 
> Similarly, the later commit "vfio-pci/zdev: Add VFIO FMB device features"
> relies on the same (!zdev->statistics_on || !zdev->fmb) check in
> vfio_pci_zdev_feature_fmb_read(), and zpci_fmb_request_on() rejects subsequent
> enable attempts with -EINVAL when zdev->statistics_on is still true.
> 
> Would this cause pci_perf_show() to report zeroed or stale FMB counters instead
> of "FMB statistics disabled" when enabling FMB fails?
>