Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 13 Apr 2022 09:33:17 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Yao Hongbo <yaohongbo@linux.alibaba.com>
Cc:     mst@redhat.com, alikernel-developer@linux.alibaba.com,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] uio/uio_pci_generic: Introduce refcnt on open/release
Message-ID: <YlZ8vZ9RX5i7mWNk@kroah.com>
References: <1649833302-27299-1-git-send-email-yaohongbo@linux.alibaba.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1649833302-27299-1-git-send-email-yaohongbo@linux.alibaba.com>
Precedence: bulk

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?

> 
> Fixes: 865a11f987ab("uio/uio_pci_generic: Disable bus-mastering on release")
> Reported-by: Xiu Yang <yangxiu.yx@alibaba-inc.com>
> Signed-off-by: Yao Hongbo <yaohongbo@linux.alibaba.com>
> ---
> 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