Re: [PATCH v2] uio/uio_pci_generic: Introduce refcnt on open/release

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux