lkml.org 
[lkml]   [2006]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[PATCH] Block on access to temporarily unavailable pci device

    The existing implementation of pci_block_user_cfg_access() was recently
    criticised for providing out of date information and for returning errors
    on write, which applications won't be expecting.

    This reimplementation uses an rw semaphore to block accesses. I examined
    a couple of other alternatives (a mutex, which would unnecessarily
    serialise kernel BIST users; a per-device mutex, which would take another
    16 bytes per pci device; a custom queue), I felt the rwsem provided the
    best tradeoffs.

    I'll post the patch I used to test blocking device accesses separately.

    Signed-off-by: Matthew Wilcox <matthew@wil.cx>

    diff --git a/drivers/pci/access.c b/drivers/pci/access.c
    index ea16805..29581a2 100644
    --- a/drivers/pci/access.c
    +++ b/drivers/pci/access.c
    @@ -1,6 +1,7 @@
    #include <linux/pci.h>
    #include <linux/module.h>
    #include <linux/ioport.h>
    +#include <linux/rwsem.h>

    #include "pci.h"

    @@ -12,6 +13,18 @@ #include "pci.h"
    static DEFINE_SPINLOCK(pci_lock);

    /*
    + * Prevent the user from accessing PCI config space when it's unsafe to do
    + * so. Some devices require this during BIST and we're required to prevent
    + * it during D-state transitions. It could be made per-device, but it doesn't
    + * seem worthwhile for such a rare occurrence.
    + *
    + * User accesses are the 'writer' as only one is allowed at a time. Kernel
    + * blocking the user is the 'reader' as multiple devices can be blocked at
    + * the same time.
    + */
    +static DECLARE_RWSEM(pci_user_sem);
    +
    +/*
    * Wrappers for all PCI configuration access functions. They just check
    * alignment, do locking and call the low-level functions pointed to
    * by pci_dev->ops.
    @@ -63,15 +76,6 @@ EXPORT_SYMBOL(pci_bus_write_config_byte)
    EXPORT_SYMBOL(pci_bus_write_config_word);
    EXPORT_SYMBOL(pci_bus_write_config_dword);

    -static u32 pci_user_cached_config(struct pci_dev *dev, int pos)
    -{
    - u32 data;
    -
    - data = dev->saved_config_space[pos/sizeof(dev->saved_config_space[0])];
    - data >>= (pos % sizeof(dev->saved_config_space[0])) * 8;
    - return data;
    -}
    -
    #define PCI_USER_READ_CONFIG(size,type) \
    int pci_user_read_config_##size \
    (struct pci_dev *dev, int pos, type *val) \
    @@ -80,13 +84,12 @@ int pci_user_read_config_##size \
    int ret = 0; \
    u32 data = -1; \
    if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
    + down_write(&pci_user_sem); \
    spin_lock_irqsave(&pci_lock, flags); \
    - if (likely(!dev->block_ucfg_access)) \
    - ret = dev->bus->ops->read(dev->bus, dev->devfn, \
    + ret = dev->bus->ops->read(dev->bus, dev->devfn, \
    pos, sizeof(type), &data); \
    - else if (pos < sizeof(dev->saved_config_space)) \
    - data = pci_user_cached_config(dev, pos); \
    spin_unlock_irqrestore(&pci_lock, flags); \
    + up_write(&pci_user_sem); \
    *val = (type)data; \
    return ret; \
    }
    @@ -98,11 +101,12 @@ int pci_user_write_config_##size \
    unsigned long flags; \
    int ret = -EIO; \
    if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER; \
    + down_write(&pci_user_sem); \
    spin_lock_irqsave(&pci_lock, flags); \
    - if (likely(!dev->block_ucfg_access)) \
    - ret = dev->bus->ops->write(dev->bus, dev->devfn, \
    + ret = dev->bus->ops->write(dev->bus, dev->devfn, \
    pos, sizeof(type), val); \
    spin_unlock_irqrestore(&pci_lock, flags); \
    + up_write(&pci_user_sem); \
    return ret; \
    }

    @@ -117,21 +121,12 @@ PCI_USER_WRITE_CONFIG(dword, u32)
    * pci_block_user_cfg_access - Block userspace PCI config reads/writes
    * @dev: pci device struct
    *
    - * This function blocks any userspace PCI config accesses from occurring.
    - * When blocked, any writes will be bit bucketed and reads will return the
    - * data saved using pci_save_state for the first 64 bytes of config
    - * space and return 0xff for all other config reads.
    - **/
    + * When user access is blocked, any reads or writes to config space will
    + * sleep until access is unblocked again.
    + */
    void pci_block_user_cfg_access(struct pci_dev *dev)
    {
    - unsigned long flags;
    -
    - pci_save_state(dev);
    -
    - /* spinlock to synchronize with anyone reading config space now */
    - spin_lock_irqsave(&pci_lock, flags);
    - dev->block_ucfg_access = 1;
    - spin_unlock_irqrestore(&pci_lock, flags);
    + down_read(&pci_user_sem);
    }
    EXPORT_SYMBOL_GPL(pci_block_user_cfg_access);

    @@ -140,14 +135,9 @@ EXPORT_SYMBOL_GPL(pci_block_user_cfg_acc
    * @dev: pci device struct
    *
    * This function allows userspace PCI config accesses to resume.
    - **/
    + */
    void pci_unblock_user_cfg_access(struct pci_dev *dev)
    {
    - unsigned long flags;
    -
    - /* spinlock to synchronize with anyone reading saved config space */
    - spin_lock_irqsave(&pci_lock, flags);
    - dev->block_ucfg_access = 0;
    - spin_unlock_irqrestore(&pci_lock, flags);
    + up_read(&pci_user_sem);
    }
    EXPORT_SYMBOL_GPL(pci_unblock_user_cfg_access);
    diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
    index 2dde821..5d06837 100644
    --- a/drivers/scsi/ipr.c
    +++ b/drivers/scsi/ipr.c
    @@ -6174,6 +6174,7 @@ static int ipr_reset_start_bist(struct i
    int rc;

    ENTER;
    + pci_save_state(ioa_cfg->pdev);
    pci_block_user_cfg_access(ioa_cfg->pdev);
    rc = pci_write_config_byte(ioa_cfg->pdev, PCI_BIST, PCI_BIST_START);

    diff --git a/include/linux/pci.h b/include/linux/pci.h
    index 3632282..4dbcbbd 100644
    --- a/include/linux/pci.h
    +++ b/include/linux/pci.h
    @@ -174,7 +174,6 @@ struct pci_dev {
    unsigned int is_busmaster:1; /* device is busmaster */
    unsigned int no_msi:1; /* device may not use msi */
    unsigned int no_d1d2:1; /* only allow d0 or d3 */
    - unsigned int block_ucfg_access:1; /* userspace config space access is blocked */
    unsigned int broken_parity_status:1; /* Device generates false positive parity */
    unsigned int msi_enabled:1;
    unsigned int msix_enabled:1;
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2006-10-17 16:55    [W:0.049 / U:30.500 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site