lkml.org 
[lkml]   [2011]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Broken pci_block_user_cfg_access interface
    On Mon, Aug 29, 2011 at 05:42:16PM +0200, Jan Kiszka wrote:
    > 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?).

    Well I made the error EIO but it really is something like
    EAGAIN which means 'I would block if I could, but I was
    asked not to'.

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

    Meaning pci_user_ read/write config?

    > 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

    We still need to block userspace access while INTx does
    the status/masking access, right?




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


    \
     
     \ /
      Last update: 2011-08-29 18:01    [W:0.044 / U:213.136 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site