Re: Broken pci_block_user_cfg_access interface

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2011-08-29 17:05, Michael S. Tsirkin wrote:
> So how about something like the following?
> Compile tested only, and I'm not sure I got the
> ipr and iov error handling right.
> Comments?

This still doesn't synchronize two callers of pci_block_user_cfg_access
unless one was reset. We may not have such a scenario yet, but that's
how the old code started as well...

And it makes the interface more convoluted (from 10000 meter, why should
pci_block_user_cfg_access return an error if reset is running?).

> 
> ---->
> Subject: [PATCH] pci: fail block usercfg access on reset
> 
> Anyone who wants to block usercfg access will
> also want to block reset from userspace.
> On the other hand, reset from userspace
> should block any other access from userspace.
> 
> Finally, make it possible to detect reset in
> pregress by returning an error status from
> pci_block_user_cfg_access.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/pci/access.c          |   45 ++++++++++++++++++++++++++++++++++++----
>  drivers/pci/iov.c             |   19 ++++++++++++----
>  drivers/pci/pci.c             |    4 +-
>  drivers/scsi/ipr.c            |   22 ++++++++++++++++++-
>  drivers/uio/uio_pci_generic.c |    7 +++++-
>  include/linux/pci.h           |    6 ++++-
>  6 files changed, 87 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index fdaa42a..2492365 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -139,7 +139,7 @@ static noinline void pci_wait_ucfg(struct pci_dev *dev)
>  		raw_spin_unlock_irq(&pci_lock);
>  		schedule();
>  		raw_spin_lock_irq(&pci_lock);
> -	} while (dev->block_ucfg_access);
> +	} while (dev->block_ucfg_access || dev->reset_in_progress);
>  	__remove_wait_queue(&pci_ucfg_wait, &wait);
>  }
>  
> @@ -153,7 +153,8 @@ int pci_user_read_config_##size						\
>  	if (PCI_##size##_BAD)						\
>  		return -EINVAL;						\
>  	raw_spin_lock_irq(&pci_lock);				\
> -	if (unlikely(dev->block_ucfg_access)) pci_wait_ucfg(dev);	\
> +	if (unlikely(dev->block_ucfg_access || dev->reset_in_progress)) \
> +		pci_wait_ucfg(dev);					\
>  	ret = dev->bus->ops->read(dev->bus, dev->devfn,			\
>  					pos, sizeof(type), &data);	\
>  	raw_spin_unlock_irq(&pci_lock);				\
> @@ -171,8 +172,9 @@ int pci_user_write_config_##size					\
>  	int ret = -EIO;							\
>  	if (PCI_##size##_BAD)						\
>  		return -EINVAL;						\
> -	raw_spin_lock_irq(&pci_lock);				\
> -	if (unlikely(dev->block_ucfg_access)) pci_wait_ucfg(dev);	\
> +	raw_spin_lock_irq(&pci_lock);					\
> +	if (unlikely(dev->block_ucfg_access || dev->reset_in_progress)) \
> +		pci_wait_ucfg(dev);					\
>  	ret = dev->bus->ops->write(dev->bus, dev->devfn,		\
>  					pos, sizeof(type), val);	\
>  	raw_spin_unlock_irq(&pci_lock);				\
> @@ -408,19 +410,24 @@ EXPORT_SYMBOL(pci_vpd_truncate);
>   * sleep until access is unblocked again.  We don't allow nesting of
>   * block/unblock calls.
>   */
> -void pci_block_user_cfg_access(struct pci_dev *dev)
> +int pci_block_user_cfg_access(struct pci_dev *dev)
>  {
>  	unsigned long flags;
>  	int was_blocked;
> +	int in_progress;
>  
>  	raw_spin_lock_irqsave(&pci_lock, flags);
>  	was_blocked = dev->block_ucfg_access;
>  	dev->block_ucfg_access = 1;
> +	in_progress = dev->reset_in_progress;
>  	raw_spin_unlock_irqrestore(&pci_lock, flags);
>  
> +	if (in_progress)
> +		return -EIO;
>  	/* If we BUG() inside the pci_lock, we're guaranteed to hose
>  	 * the machine */
>  	BUG_ON(was_blocked);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(pci_block_user_cfg_access);
>  
> @@ -445,3 +452,31 @@ void pci_unblock_user_cfg_access(struct pci_dev *dev)
>  	raw_spin_unlock_irqrestore(&pci_lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(pci_unblock_user_cfg_access);
> +
> +void pci_reset_start(struct pci_dev *dev)
> +{
> +	int was_started;
> +
> +	raw_spin_lock_irq(&pci_lock);
> +	if (unlikely(dev->block_ucfg_access || dev->reset_in_progress))
> +		pci_wait_ucfg(dev);
> +	was_started = dev->reset_in_progress;
> +	dev->reset_in_progress = 1;
> +	raw_spin_unlock_irq(&pci_lock);
> +
> +	/* If we BUG() inside the pci_lock, we're guaranteed to hose
> +	 * the machine */
> +	BUG_ON(was_started);
> +}
> +void pci_reset_end(struct pci_dev *dev)
> +{
> +	raw_spin_lock_irq(&pci_lock);
> +
> +	/* This indicates a problem in the caller, but we don't need
> +	 * to kill them, unlike a double-reset above. */
> +	WARN_ON(!dev->reset_in_progress);
> +
> +	dev->reset_in_progress = 0;
> +	wake_up_all(&pci_ucfg_wait);
> +	raw_spin_unlock_irq(&pci_lock);
> +}
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 42fae47..464d9b5 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -275,7 +275,7 @@ static void sriov_disable_migration(struct pci_dev *dev)
>  
>  static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>  {
> -	int rc;
> +	int rc, rc1;
>  	int i, j;
>  	int nres;
>  	u16 offset, stride, initial;
> @@ -340,7 +340,10 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
>  	}
>  
>  	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
> -	pci_block_user_cfg_access(dev);
> +	rc = pci_block_user_cfg_access(dev);
> +	if (rc)
> +		goto err;
> +
>  	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
>  	msleep(100);
>  	pci_unblock_user_cfg_access(dev);
> @@ -371,11 +374,14 @@ failed:
>  		virtfn_remove(dev, j, 0);
>  
>  	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
> -	pci_block_user_cfg_access(dev);
> +	rc1 = pci_block_user_cfg_access(dev);
> +	if (rc1)
> +		goto err;
>  	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
>  	ssleep(1);
>  	pci_unblock_user_cfg_access(dev);
>  
> +err:
>  	if (iov->link != dev->devfn)
>  		sysfs_remove_link(&dev->dev.kobj, "dep_link");
>  
> @@ -384,7 +390,7 @@ failed:
>  
>  static void sriov_disable(struct pci_dev *dev)
>  {
> -	int i;
> +	int i, rc;
>  	struct pci_sriov *iov = dev->sriov;
>  
>  	if (!iov->nr_virtfn)
> @@ -397,11 +403,14 @@ static void sriov_disable(struct pci_dev *dev)
>  		virtfn_remove(dev, i, 0);
>  
>  	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
> -	pci_block_user_cfg_access(dev);
> +	rc = pci_block_user_cfg_access(dev);
> +	if (rc)
> +		goto err;
>  	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
>  	ssleep(1);
>  	pci_unblock_user_cfg_access(dev);
>  
> +err:
>  	if (iov->link != dev->devfn)
>  		sysfs_remove_link(&dev->dev.kobj, "dep_link");
>  
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 0ce6742..815efc2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2959,7 +2959,7 @@ static int pci_dev_reset(struct pci_dev *dev, int probe)
>  	might_sleep();
>  
>  	if (!probe) {
> -		pci_block_user_cfg_access(dev);
> +		pci_reset_start(dev);
>  		/* block PM suspend, driver probe, etc. */
>  		device_lock(&dev->dev);
>  	}
> @@ -2984,7 +2984,7 @@ static int pci_dev_reset(struct pci_dev *dev, int probe)
>  done:
>  	if (!probe) {
>  		device_unlock(&dev->dev);
> -		pci_unblock_user_cfg_access(dev);
> +		pci_reset_end(dev);
>  	}
>  
>  	return rc;
> diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
> index 8d63630..d322da3 100644
> --- a/drivers/scsi/ipr.c
> +++ b/drivers/scsi/ipr.c
> @@ -7661,7 +7661,9 @@ static int ipr_reset_start_bist(struct ipr_cmnd *ipr_cmd)
>  	int rc = PCIBIOS_SUCCESSFUL;
>  
>  	ENTER;
> -	pci_block_user_cfg_access(ioa_cfg->pdev);
> +	rc = pci_block_user_cfg_access(ioa_cfg->pdev);
> +	if (rc)
> +		goto err;
>  
>  	if (ioa_cfg->ipr_chip->bist_method == IPR_MMIO)
>  		writel(IPR_UPROCI_SIS64_START_BIST,
> @@ -7681,6 +7683,13 @@ static int ipr_reset_start_bist(struct ipr_cmnd *ipr_cmd)
>  
>  	LEAVE;
>  	return rc;
> +
> +err:
> +
> +	ipr_cmd->s.ioasa.hdr.ioasc = cpu_to_be32(IPR_IOASC_PCI_ACCESS_ERROR);
> +	rc = IPR_RC_JOB_CONTINUE;
> +	LEAVE;
> +	return rc;
>  }
>  
>  /**
> @@ -7715,14 +7724,23 @@ static int ipr_reset_slot_reset(struct ipr_cmnd *ipr_cmd)
>  {
>  	struct ipr_ioa_cfg *ioa_cfg = ipr_cmd->ioa_cfg;
>  	struct pci_dev *pdev = ioa_cfg->pdev;
> +	int rc;
>  
>  	ENTER;
> -	pci_block_user_cfg_access(pdev);
> +	rc = pci_block_user_cfg_access(pdev);
> +	if (rc)
> +		goto err;

Looks like the target for this jump is missing.

> +
>  	pci_set_pcie_reset_state(pdev, pcie_warm_reset);
>  	ipr_cmd->job_step = ipr_reset_slot_reset_done;
>  	ipr_reset_start_timer(ipr_cmd, IPR_PCI_RESET_TIMEOUT);
>  	LEAVE;
>  	return IPR_RC_JOB_RETURN;
> +
> +	ipr_cmd->s.ioasa.hdr.ioasc = cpu_to_be32(IPR_IOASC_PCI_ACCESS_ERROR);
> +	rc = IPR_RC_JOB_CONTINUE;
> +	LEAVE;
> +	return rc;
>  }
>  
>  /**
> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> index fc22e1e..a26d35f 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -51,6 +51,7 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
>  	irqreturn_t ret = IRQ_NONE;
>  	u32 cmd_status_dword;
>  	u16 origcmd, newcmd, status;
> +	int r;
>  
>  	/* We do a single dword read to retrieve both command and status.
>  	 * Document assumptions that make this possible. */
> @@ -58,7 +59,9 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
>  	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
>  
>  	spin_lock_irq(&gdev->lock);
> -	pci_block_user_cfg_access(pdev);
> +	r = pci_block_user_cfg_access(pdev);
> +	if (r < 0)
> +		goto err;
>  
>  	/* Read both command and status registers in a single 32-bit operation.
>  	 * Note: we could cache the value for command and move the status read
> @@ -83,6 +86,8 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
>  done:
>  
>  	pci_unblock_user_cfg_access(pdev);
> +err:
> +
>  	spin_unlock_irq(&gdev->lock);
>  	return ret;
>  }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8c230cb..ec3b8fe 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -322,6 +322,7 @@ struct pci_dev {
>  	unsigned int    is_hotplug_bridge:1;
>  	unsigned int    __aer_firmware_first_valid:1;
>  	unsigned int	__aer_firmware_first:1;
> +	unsigned int	reset_in_progress:1;
>  	pci_dev_flags_t dev_flags;
>  	atomic_t	enable_cnt;	/* pci_enable_device has been called */
>  
> @@ -1079,9 +1080,12 @@ int  ht_create_irq(struct pci_dev *dev, int idx);
>  void ht_destroy_irq(unsigned int irq);
>  #endif /* CONFIG_HT_IRQ */
>  
> -extern void pci_block_user_cfg_access(struct pci_dev *dev);
> +extern int pci_block_user_cfg_access(struct pci_dev *dev);
>  extern void pci_unblock_user_cfg_access(struct pci_dev *dev);
>  
> +extern void pci_reset_start(struct pci_dev *dev);
> +extern void pci_reset_end(struct pci_dev *dev);
> +
>  /*
>   * PCI domain support.  Sometimes called PCI segment (eg by ACPI),
>   * a PCI domain is defined to be a set of PCI busses which share

I still don't get what prevents converting ipr to allow plain mutex
synchronization. My vision is:
 - push reset-on-error of ipr into workqueue (or threaded IRQ?)
 - require mutex synchronization for common config space access and the
   full reset cycle
 - only exception: INTx status/masking access
    => use pci_lock + test for reset_in_progress, skip operation if
       that is the case

That would allow to drop the whole block_user_cfg infrastructure.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux