在 2022/4/13 下午4:51, Michael S. Tsirkin 写道:
On Wed, Apr 13, 2022 at 09:33:17AM +0200, Greg KH wrote:
On Wed, Apr 13, 2022 at 03:01:42PM +0800, Yao Hongbo wrote:
If two userspace programs both open the PCI UIO fd, when one
of the program exits uncleanly, the other will cause IO hang
due to bus-mastering disabled.
It's a common usage for spdk/dpdk to use UIO. So, introduce refcnt
to avoid such problems.
Why do you have multiple userspace programs opening the same device?
Shouldn't they coordinate?
Or to restate, I think the question is, why not open the device
once and pass the FD around?
Hmm, it will have the same result, no matter whether opening the same
device or pass the FD around.
Our expectation is that even if the primary process exits abnormally,
the second process can still send
or receive data.
The impact of disabling pci bus-master is relatively large, and we
should make some restrictions on
this behavior.
Fixes: 865a11f987ab("uio/uio_pci_generic: Disable bus-mastering on release")
space missing before ( here .
Reported-by: Xiu Yang <yangxiu.yx@xxxxxxxxxxxxxxx>
Signed-off-by: Yao Hongbo <yaohongbo@xxxxxxxxxxxxxxxxx>
---
Changes for v2:
Use refcount_t instead of atomic_t to catch overflow/underflows.
---
drivers/uio/uio_pci_generic.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index e03f9b5..1a5e1fd 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -31,6 +31,7 @@
struct uio_pci_generic_dev {
struct uio_info info;
struct pci_dev *pdev;
+ refcount_t refcnt;
};
static inline struct uio_pci_generic_dev *
@@ -39,6 +40,14 @@ struct uio_pci_generic_dev {
return container_of(info, struct uio_pci_generic_dev, info);
}
+static int open(struct uio_info *info, struct inode *inode)
+{
+ struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
+
+ refcount_inc(&gdev->refcnt);
+ return 0;
+}
+
static int release(struct uio_info *info, struct inode *inode)
{
struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
@@ -51,7 +60,9 @@ static int release(struct uio_info *info, struct inode *inode)
* Note that there's a non-zero chance doing this will wedge the device
* at least until reset.
*/
- pci_clear_master(gdev->pdev);
+ if (refcount_dec_and_test(&gdev->refcnt))
+ pci_clear_master(gdev->pdev);
The goal here is to flush things when userspace closes the device, as
the comment says. So don't you want that to happen for when userspace
closes the file handle no matter who opened it?
As this is a functional change, how is userspace going to "know" this
functionality is now changed or not?
And if userspace really wants to open this multiple times, then properly
switch the code to only create the device-specific structures when open
is called. Otherwise you are sharing structures here that are not
intended to be shared, shouldn't you have your own private one?
this feels odd.
thanks,
greg k-h
Sigh. Maybe it was a mistake to do 865a11f987ab to begin with.
Anyone doing DMA with UIO is already on very thin ice.
But oh well.
