Re: [PATCH v11 2/5] s390/pci: Improve function measurement state management

[Gerd Bayer <gbayer@linux.ibm.com>]

On Tue, 2026-11-03 at 12:40 -0500, Omar Elghoul wrote:
> On 11/3/26 12:02 PM, Gerd Bayer wrote:
> > On Wed, 2026-10-28 at 11:43 -0400, Omar Elghoul wrote:
> > 

  [ ... snip ...]

> > > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> > > index c055a9ad0972..5889dd227009 100644
> > > --- a/arch/s390/pci/pci.c
> > > +++ b/arch/s390/pci/pci.c
> > > @@ -164,8 +164,44 @@ int zpci_unregister_ioat(struct zpci_dev *zdev, u8 dmaas)
> > >   	return cc;
> > >   }
> > >   
> > > +int zpci_fmb_request_on(struct zpci_dev *zdev)
> > > +{
> > > +	int rc;
> > > +
> > > +	lockdep_assert_held(&zdev->fmb_lock);
> > > +
> > > +	if (zdev->statistics_on)
> > > +		return -EINVAL;
> > 
> > It's perhaps a question of taste, but is this strict rejection of
> > double-enable really necessary? Wouldn't it be more defensive if the
> > enable was just let to pass?
> 
> It isn't, and I actually just dropped it in v12. After introducing the
> fmb_enabled bool in v12, we now reject double-enables right from within
> zpci_fmb_enable() itself, matching the pre-existing behavior.

Looking at v12 ATM: Thanks for the change.

> 
> > Ah nevermind: That would allocate a new FMB buffer from zdev_fmb_cache
> > and is a memory leak.
> 
> With this patch alone, the buffer doesn't leak because zpci_fmb_enable()
> already rejects a double-enable with zdev->fmb is non-null. Of course,
> patch 3 here (v11) does introduce an issue where we reuse the same FMB
> buffer without adequately replacing the liveness check. This is again
> addressed in v12.

Still have to look into this in v12...

> > 
> > 
> > > +
> > > +	rc = zpci_fmb_enable(zdev);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	zdev->statistics_on = true;
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(zpci_fmb_request_on);
> > > +
> > > +int zpci_fmb_request_off(struct zpci_dev *zdev)
> > > +{
> > > +	int rc;
> > > +
> > > +	lockdep_assert_held(&zdev->fmb_lock);
> > > +
> > > +	if (!zdev->statistics_on)
> > > +		return -EINVAL;
> > 
> > Same question here: Would a double-disable hurt anybody?
> > Doh: Yeah, you don't want to go down that route...
> > 
> > > +
> > > +	rc = zpci_fmb_disable(zdev);
> > > +	if (rc)
> > > +		return rc;
> > > +
> > > +	zdev->statistics_on = false;
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(zpci_fmb_request_off);
> > > +
> > >   /* Modify PCI: Set PCI function measurement parameters */
> > > -int zpci_fmb_enable_device(struct zpci_dev *zdev)
> > > +int zpci_fmb_enable(struct zpci_dev *zdev)
> > >   {
> > >   	u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_SET_MEASURE);
> > >   	struct zpci_iommu_ctrs *ctrs;
> > > @@ -207,7 +243,7 @@ int zpci_fmb_enable_device(struct zpci_dev *zdev)
> > >   }
> > >   
> > >   /* Modify PCI: Disable PCI function measurement */
> > > -int zpci_fmb_disable_device(struct zpci_dev *zdev)
> > > +int zpci_fmb_disable(struct zpci_dev *zdev)
> > >   {
> > >   	u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, ZPCI_MOD_FC_SET_MEASURE);
> > >   	struct zpci_fib fib = {0};
> > > @@ -644,7 +680,8 @@ int pcibios_enable_device(struct pci_dev *pdev, int mask)
> > >   
> > >   	zpci_debug_init_device(zdev, dev_name(&pdev->dev));
> > >   	mutex_lock(&zdev->fmb_lock);
> > > -	zpci_fmb_enable_device(zdev);
> > > +	if (zdev->statistics_on)
> > > +		zpci_fmb_enable(zdev);
> > >   	mutex_unlock(&zdev->fmb_lock);
> > >   
> > >   	return pci_enable_resources(pdev, mask);
> > > @@ -655,7 +692,8 @@ void pcibios_disable_device(struct pci_dev *pdev)
> > >   	struct zpci_dev *zdev = to_zpci(pdev);
> > >   
> > >   	mutex_lock(&zdev->fmb_lock);
> > > -	zpci_fmb_disable_device(zdev);
> > > +	if (zdev->statistics_on)
> > > +		zpci_fmb_disable(zdev);
> > >   	mutex_unlock(&zdev->fmb_lock);
> > >   	zpci_debug_exit_device(zdev);
> > >   }
> > > @@ -737,8 +775,14 @@ int zpci_reenable_device(struct zpci_dev *zdev)
> > >   	}
> > >   
> > >   	rc = zpci_iommu_register_ioat(zdev, &status);
> > > -	if (rc)
> > > +	if (rc) {
> > >   		zpci_disable_device(zdev);
> > > +		return rc;
> > > +	}
> > > +
> > > +	guard(mutex)(&zdev->fmb_lock);
> > > +	if (zdev->statistics_on)
> > > +		zpci_fmb_enable(zdev);
> > 
> > But that discussion about double-enable above makes me wonder:
> > Would we leak an FMB buffer, here - at least until patch 3 comes to the
> > rescue? Maybe the two patches are not so independent after all?
> 
> Nope, we don't because the behavior of zpci_fmb_enable()/_disable() is
> still unchanged from before this patch. They will still reject a double-
> disable/enable attempt even without patch 3.

Yup, that's what my experimentation did show. Sorry for the noise.

> 
> > 
> > So, I experimented a little with patch 1 and 2 applied in an LPAR: I
> > injected PCI auto-recovery with 'zpcictl --reset-fw' to a NETD PF and a
> > NVMe PF and was pleasantly surprised that no resources were leaked
> > *and* statistics were enabled before and after the PCI auto-recovery.
> > 
> > Turns out that both the mlx5 and the nvme drivers pci_error_handlers
> > callbacks for error_detected() switch off statistics by way of
> > pci_disable_device() which calls pcibios_disable_device() - and the
> > callback for slot_reset (mlx5) resp. reset_done (nvme) restore the
> > statistics setting again by way of pci_enable_device() which calls
> > pcibios_enable_device().
> > 
> > So one could argue, that at least for the PCI auto-recovery case, this
> > conditional call to zpci_fmb_enable() is not needed?
> 
> I think you're right, but we still need this call here for other paths
> that call zpci_reenable_device() without pcibios_enable_device(). IIRC
> a sysfs recovery (via recover_store() -> _do_recover()) disables the
> device and then re-enables it using zpci_reenable_device().

Ah yes, I didn't think about that path.

I'll continue with reviewing v12.

Thanks,
Gerd