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

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

On 11/3/26 12:02 PM, Gerd Bayer wrote:
> On Wed, 2026-10-28 at 11:43 -0400, Omar Elghoul wrote:
>> When a zPCI function is disabled and re-enabled, the firmware implicitly
>> disables function measurement while the kernel still believes it is
>> enabled, leading to a disconnect between the user-visible state and the
>> actual device state.
>>
>> Add the statistics_on field to struct zpci_dev to track the desired FMB
>> state. Introduce the functions zpci_fmb_request_on() and off(), which
>> defer to zpci_fmb_enable() or disable() to perform the actual firmware
>> change. This decouples the enablement or disablement step from managing
>> the user's requested state.
>>
>> Preserve the state in pcibios_enable_device(), pcibios_disable_device(),
>> and zpci_reenable_device(), so measurement is restored automatically
>> after a re-enable. New devices start with measurement on by default,
>> unchanged from before.
>>
>> Signed-off-by: Omar Elghoul <oelghoul@linux.ibm.com>
>> ---
>>   arch/s390/include/asm/pci.h |  7 +++--
>>   arch/s390/pci/pci.c         | 60 +++++++++++++++++++++++++++++++++----
>>   arch/s390/pci/pci_debug.c   |  6 ++--
>>   3 files changed, 63 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>> index 88a125b92bdd..dd7a17955362 100644
>> --- a/arch/s390/include/asm/pci.h
>> +++ b/arch/s390/include/asm/pci.h
>> @@ -208,6 +208,7 @@ struct zpci_dev {
>>   	struct zpci_fmb *fmb;
>>   	u16		fmb_update;	/* update interval */
>>   	u16		fmb_length;
>> +	bool		statistics_on;	/* desired FMB enablement */
>>   
>>   	u8		version;
>>   	enum pci_bus_speed max_bus_speed;
>> @@ -349,8 +350,10 @@ int zpci_create_parent_msi_domain(struct zpci_bus *zbus);
>>   void zpci_remove_parent_msi_domain(struct zpci_bus *zbus);
>>   
>>   /* FMB */
>> -int zpci_fmb_enable_device(struct zpci_dev *);
>> -int zpci_fmb_disable_device(struct zpci_dev *);
>> +int zpci_fmb_request_on(struct zpci_dev *zdev);
>> +int zpci_fmb_request_off(struct zpci_dev *zdev);
>> +int zpci_fmb_enable(struct zpci_dev *zdev);
>> +int zpci_fmb_disable(struct zpci_dev *zdev);
>>   
>>   /* Debug */
>>   int zpci_debug_init(void);
>> 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.

> 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.

> 
> 
>> +
>> +	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.

> 
> 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().

Thanks

> 
>>   
>>   	return rc;
>>   }
>> @@ -847,6 +891,12 @@ struct zpci_dev *zpci_create_device(u32 fid, u32 fh, enum zpci_state state)
>>   		goto error;
>>   	zdev->state =  state;
>>   
>> +	/*
>> +	 * Measurement is on by default for new functions, this will be honored
>> +	 * by pcibios_enable_device().
>> +	 */
>> +	zdev->statistics_on = true;
>> +
>>   	mutex_init(&zdev->state_lock);
>>   	mutex_init(&zdev->fmb_lock);
>>   	mutex_init(&zdev->kzdev_lock);
>> diff --git a/arch/s390/pci/pci_debug.c b/arch/s390/pci/pci_debug.c
>> index c7ed7bf254b5..e479763858cd 100644
>> --- a/arch/s390/pci/pci_debug.c
>> +++ b/arch/s390/pci/pci_debug.c
>> @@ -97,7 +97,7 @@ static int pci_perf_show(struct seq_file *m, void *v)
>>   		return 0;
>>   
>>   	mutex_lock(&zdev->fmb_lock);
>> -	if (!zdev->fmb) {
>> +	if (!zdev->statistics_on || !zdev->fmb) {
>>   		mutex_unlock(&zdev->fmb_lock);
>>   		seq_puts(m, "FMB statistics disabled\n");
>>   		return 0;
>> @@ -156,10 +156,10 @@ static ssize_t pci_perf_seq_write(struct file *file, const char __user *ubuf,
>>   	mutex_lock(&zdev->fmb_lock);
>>   	switch (val) {
>>   	case 0:
>> -		rc = zpci_fmb_disable_device(zdev);
>> +		rc = zpci_fmb_request_off(zdev);
>>   		break;
>>   	case 1:
>> -		rc = zpci_fmb_enable_device(zdev);
>> +		rc = zpci_fmb_request_on(zdev);
>>   		break;
>>   	}
>>   	mutex_unlock(&zdev->fmb_lock);
> 
> But overall, this is looking good!
> Thanks, Gerd