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

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

On Tue, 2026-11-03 at 10:01 -0500, 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()/_off(), which defer
> to zpci_fmb_enable()/_disable() to perform the actual firmware change.
> This decouples the enablement or disablement step from managing the
> user's requested state.
> 
> Restore the state in pcibios_enable_device(), pcibios_disable_device(),
> and zpci_reenable_device(). 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         | 54 +++++++++++++++++++++++++++++++++----
>  arch/s390/pci/pci_debug.c   |  4 +--
>  3 files changed, 56 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 88a125b92bdd..dd58c48ac2ed 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;
> +	u8		statistics_on	: 1; /* 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..f49736386eb7 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -164,8 +164,38 @@ 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);
> +
> +	rc = zpci_fmb_enable(zdev);
> +	if (rc)
> +		return rc;
> +
> +	zdev->statistics_on = 1;
> +	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);
> +
> +	rc = zpci_fmb_disable(zdev);
> +	if (rc)
> +		return rc;
> +
> +	zdev->statistics_on = 0;
> +	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 +237,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 +674,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 +686,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 +769,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);
>  
>  	return rc;
>  }
> @@ -847,6 +885,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 = 1;
> +
>  	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..06e2e8bb2f82 100644
> --- a/arch/s390/pci/pci_debug.c
> +++ b/arch/s390/pci/pci_debug.c
> @@ -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);

Yeah - this looks good to me!
Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com>

Thank you for your work!