Re: Broken pci_block_user_cfg_access interface
On 08/29/2011 02:18 PM, Michael S. Tsirkin wrote: On Mon, Aug 29, 2011 at 08:47:07PM +0200, Jan Kiszka wrote: On 2011-08-29 17:42, Jan Kiszka wrote: 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?) I'm starting to like your proposal: I had a look at ipr, but it turned out to be anything but trivial to convert that driver. It runs its complete state machine under spin_lock_irq, and the functions calling pci_block/unblock_user_cfg_access are deep inside this thing. I have no hardware to test whatever change, and I feel a bit uncomfortable asking Brian to redesign his driver that massively. So back to your idea: I would generalize pci_block_user_cfg_access to pci_block_cfg_access. It should fail when some other site already holds the access lock, but it should remain non-blocking - for the sake of ipr. It would be easy to have blocking and non-blocking variants. But - I have no idea whether supporting sysfs config/reset access while ipr is active makes any sense - I know we need it for uio. I really don't think it makes sense. Ideally, I really think the driver should be able to override the PCI layer reset interface in sysfs. If a driver is loaded, the driver owns all the state of that device and resetting it without informing the driver is just nasty. Additionally, many devices may have much more complex logic to performing a reset than what PCI defines. With ipr, for example, it needs to get a shutdown command issued to it prior to the reset if at all possible so that the firmware quiesces any I/O it is performing. It also needs additional communication prior to resetting the chip to ensure the firmware is not modifying its persistent error log on the adapter's flash, since resetting the card while the flash segment is being updated will cause the adapter to lose the persistent error log. Post reset, ipr has a bunch of work to do to get the firmware back up and running to a state where it can handle I/O again. Different ipr chips also have different requirements as to what reset mechanisms defined by PCI actually work. Some chips require BIST to be run via PCI config space, while others require a PCI warm reset, otherwise the card ends up in an unusable state. So, here is my proposal to resolve this particular issue. Add a reset function to the pci_driver struct which would allow drivers to override the default reset action. Drivers that can tolerate the existing reset mechanism can simply point to a generic PCI function to perform the reset. Drivers which can't handle their device getting reset, will simply not have a reset function defined. In this case, anyone attempting to issue a reset via sysfs will get an error. If a driver is not loaded, then we can perform the default reset method and fix any device specific oddities with quirks. I like keeping pci_block_user_cfg_access a non blocking interface. If it can return a failure due to some other caller, it should be easy enough to modify the ipr driver to wait for access to get unblocked before resetting the adapter. Thanks, Brian -- Brian King Linux on Power Virtualization IBM Linux Technology Center -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Broken pci_block_user_cfg_access interface
On Tue, Aug 30, 2011 at 11:30:35AM -0500, Brian King wrote: On 08/29/2011 02:18 PM, Michael S. Tsirkin wrote: On Mon, Aug 29, 2011 at 08:47:07PM +0200, Jan Kiszka wrote: On 2011-08-29 17:42, Jan Kiszka wrote: 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?) I'm starting to like your proposal: I had a look at ipr, but it turned out to be anything but trivial to convert that driver. It runs its complete state machine under spin_lock_irq, and the functions calling pci_block/unblock_user_cfg_access are deep inside this thing. I have no hardware to test whatever change, and I feel a bit uncomfortable asking Brian to redesign his driver that massively. So back to your idea: I would generalize pci_block_user_cfg_access to pci_block_cfg_access. It should fail when some other site already holds the access lock, but it should remain non-blocking - for the sake of ipr. It would be easy to have blocking and non-blocking variants. But - I have no idea whether supporting sysfs config/reset access while ipr is active makes any sense - I know we need it for uio. I really don't think it makes sense. Ideally, I really think the driver should be able to override the PCI layer reset interface in sysfs. Well, it's always possible for root to do silly things like reset the device from userspace using sysfs config access. So I don't really see this blocking as necessary: broken application with access to a physical device will lead to problems. If a driver is loaded, the driver owns all the state of that device and resetting it without informing the driver is just nasty. Additionally, many devices may have much more complex logic to performing a reset than what PCI defines. With ipr, for example, it needs to get a shutdown command issued to it prior to the reset if at all possible so that the firmware quiesces any I/O it is performing. It also needs additional communication prior to resetting the chip to ensure the firmware is not modifying its persistent error log on the adapter's flash, since resetting the card while the flash segment is being updated will cause the adapter to lose the persistent error log. Post reset, ipr has a bunch of work to do to get the firmware back up and running to a state where it can handle I/O again. Different ipr chips also have different requirements as to what reset mechanisms defined by PCI actually work. Some chips require BIST to be run via PCI config space, while others require a PCI warm reset, otherwise the card ends up in an unusable state. So, here is my proposal to resolve this particular issue. As userspace has no place touching the device while ipr is active, there's no issue with ipr at all, is there? Add a reset function to the pci_driver struct which would allow drivers to override the default reset action. Drivers that can tolerate the existing reset mechanism can simply point to a generic PCI function to perform the reset. Drivers which can't handle their device getting reset, will simply not have a reset function defined. In this case, anyone attempting to issue a reset via sysfs will get an error. If a driver is not loaded, then we can perform the default reset method and fix any device specific oddities with quirks. I like keeping pci_block_user_cfg_access a non blocking interface. If it can return a failure due to some other caller, it should be easy enough to modify the ipr driver to wait for access to get unblocked before resetting the adapter. Thanks, Brian There shouldn't be other callers though, should there? So it might be enough to fail gracefully (to make debugging easier) rather than retry. -- Brian King Linux on Power Virtualization IBM Linux Technology Center -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Broken pci_block_user_cfg_access interface
On 08/30/2011 01:01 PM, Michael S. Tsirkin wrote: On Tue, Aug 30, 2011 at 11:30:35AM -0500, Brian King wrote: On 08/29/2011 02:18 PM, Michael S. Tsirkin wrote: On Mon, Aug 29, 2011 at 08:47:07PM +0200, Jan Kiszka wrote: On 2011-08-29 17:42, Jan Kiszka wrote: 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?) I'm starting to like your proposal: I had a look at ipr, but it turned out to be anything but trivial to convert that driver. It runs its complete state machine under spin_lock_irq, and the functions calling pci_block/unblock_user_cfg_access are deep inside this thing. I have no hardware to test whatever change, and I feel a bit uncomfortable asking Brian to redesign his driver that massively. So back to your idea: I would generalize pci_block_user_cfg_access to pci_block_cfg_access. It should fail when some other site already holds the access lock, but it should remain non-blocking - for the sake of ipr. It would be easy to have blocking and non-blocking variants. But - I have no idea whether supporting sysfs config/reset access while ipr is active makes any sense - I know we need it for uio. I really don't think it makes sense. Ideally, I really think the driver should be able to override the PCI layer reset interface in sysfs. Well, it's always possible for root to do silly things like reset the device from userspace using sysfs config access. So I don't really see this blocking as necessary: broken application with access to a physical device will lead to problems. I agree that it is probably not necessary for the current use of the sysfs API, but if there was interest in expanding it to be usable for other purposes, we might need to do something like what I am proposing. However, it doesn't appear that anyone has any immediate need for doing so. If a driver is loaded, the driver owns all the state of that device and resetting it without informing the driver is just nasty. Additionally, many devices may have much more complex logic to performing a reset than what PCI defines. With ipr, for example, it needs to get a shutdown command issued to it prior to the reset if at all possible so that the firmware quiesces any I/O it is performing. It also needs additional communication prior to resetting the chip to ensure the firmware is not modifying its persistent error log on the adapter's flash, since resetting the card while the flash segment is being updated will cause the adapter to lose the persistent error log. Post reset, ipr has a bunch of work to do to get the firmware back up and running to a state where it can handle I/O again. Different ipr chips also have different requirements as to what reset mechanisms defined by PCI actually work. Some chips require BIST to be run via PCI config space, while others require a PCI warm reset, otherwise the card ends up in an unusable state. So, here is my proposal to resolve this particular issue. As userspace has no place touching the device while ipr is active, there's no issue with ipr at all, is there? Correct. Add a reset function to the pci_driver struct which would allow drivers to override the default reset action. Drivers that can tolerate the existing reset mechanism can simply point to a generic PCI function to perform the reset. Drivers which can't handle their device getting reset, will simply not have a reset function defined. In this case, anyone attempting to issue a reset via sysfs will get an error. If a driver is not loaded, then we can perform the default reset method and fix any device specific oddities with quirks. I like keeping pci_block_user_cfg_access a non blocking interface. If it can return a failure due to some other caller, it should be easy enough to modify the ipr driver to wait for access to get unblocked before resetting the adapter. Thanks, Brian There shouldn't be other callers though, should there? So it might be enough to fail gracefully (to make debugging easier) rather than retry. Actually, its probably not worth the complexity to change much of anything in the ipr driver. With the current users, ipr should be the only caller. If, for some reason, we were double blocked due to ipr, the ipr driver already handles that today just fine and having the second block essentially be a noop is fine, since we will only get a single unblock due to ipr's reset state machine. If another caller is added in the future, then we will need to make potential changes, but without knowing the use case, its not clear to me what the proper action would be anyway. Thanks, Brian -- Brian King Linux on Power Virtualization IBM Linux Technology Center -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: Broken pci_block_user_cfg_access interface
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?
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 m...@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
@@
Re: Broken pci_block_user_cfg_access interface
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 1 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 m...@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
Re: 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 1 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 m...@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
Re: Broken pci_block_user_cfg_access interface
On 2011-08-29 17:58, Michael S. Tsirkin wrote: On Mon, Aug 29, 2011 at 05:42:16PM +0200, Jan Kiszka wrote: 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 pci_dev_reset, yes. 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? Yes, pci_lock would do that for us. We should consider making the related bits for INTx test mask/unmask generic PCI services so that no user (uio_pci_generic, kvm, vfio) needs to worry about the locking details. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Broken pci_block_user_cfg_access interface
On Mon, Aug 29, 2011 at 06:14:39PM +0200, Jan Kiszka wrote: On 2011-08-29 17:58, Michael S. Tsirkin wrote: On Mon, Aug 29, 2011 at 05:42:16PM +0200, Jan Kiszka wrote: 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 pci_dev_reset, yes. 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? Yes, pci_lock would do that for us. Well this means block_user_cfg is not going away, this is what it really is: pci_lock + a bit to lock out userspace. We should consider making the related bits for INTx test mask/unmask generic PCI services so that no user (uio_pci_generic, kvm, vfio) needs to worry about the locking details. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Broken pci_block_user_cfg_access interface
On 2011-08-29 18:23, Michael S. Tsirkin wrote: On Mon, Aug 29, 2011 at 06:14:39PM +0200, Jan Kiszka wrote: On 2011-08-29 17:58, Michael S. Tsirkin wrote: On Mon, Aug 29, 2011 at 05:42:16PM +0200, Jan Kiszka wrote: 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 pci_dev_reset, yes. 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? Yes, pci_lock would do that for us. Well this means block_user_cfg is not going away, this is what it really is: pci_lock + a bit to lock out userspace. I does as we only end up with a mutex and pci_lock. No more hand-crafted queuing/blocking/waking. INTx masking is a bit special as it's the only thing that truly requires atomic context. But that's something we should address generically anyway. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Broken pci_block_user_cfg_access interface
On 2011-08-29 17:42, Jan Kiszka wrote: 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?) I'm starting to like your proposal: I had a look at ipr, but it turned out to be anything but trivial to convert that driver. It runs its complete state machine under spin_lock_irq, and the functions calling pci_block/unblock_user_cfg_access are deep inside this thing. I have no hardware to test whatever change, and I feel a bit uncomfortable asking Brian to redesign his driver that massively. So back to your idea: I would generalize pci_block_user_cfg_access to pci_block_cfg_access. It should fail when some other site already holds the access lock, but it should remain non-blocking - for the sake of ipr. We should still provide generic pci-2.3 IRQ masking services, but that could be done in a second step. I could have a look at this. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Broken pci_block_user_cfg_access interface
On Mon, Aug 29, 2011 at 08:47:07PM +0200, Jan Kiszka wrote: On 2011-08-29 17:42, Jan Kiszka wrote: 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?) I'm starting to like your proposal: I had a look at ipr, but it turned out to be anything but trivial to convert that driver. It runs its complete state machine under spin_lock_irq, and the functions calling pci_block/unblock_user_cfg_access are deep inside this thing. I have no hardware to test whatever change, and I feel a bit uncomfortable asking Brian to redesign his driver that massively. So back to your idea: I would generalize pci_block_user_cfg_access to pci_block_cfg_access. It should fail when some other site already holds the access lock, but it should remain non-blocking - for the sake of ipr. It would be easy to have blocking and non-blocking variants. But - I have no idea whether supporting sysfs config/reset access while ipr is active makes any sense - I know we need it for uio. - reset while uio handles interrupt needs to block, not fail I think We should still provide generic pci-2.3 IRQ masking services, but that could be done in a second step. I could have a look at this. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
