From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f54.google.com (mail-lf1-f54.google.com [209.85.167.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 449B95577C for ; Mon, 8 Jan 2024 19:19:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=ffwll.ch Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="YFcx1vSV" Received: by mail-lf1-f54.google.com with SMTP id 2adb3069b0e04-50e6c0c0c6bso371418e87.0 for ; Mon, 08 Jan 2024 11:19:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; t=1704741568; x=1705346368; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:mail-followup-to:message-id:subject:cc:to :from:date:from:to:cc:subject:date:message-id:reply-to; bh=EJ2fxcKwoH+j3zf8yKo3Vu+cgAPiKaowc7Mo1YkN7dA=; b=YFcx1vSVOGbIvCJfYVZEgJsYKAHOD4M4FFjb+40kd6e81VEW+wFnI/nUS4f1+lkW+6 YkTv0jRH92QYuw1e3J41rlB7BGa7Yy3mGodwU/V7Drq9tsOQ4X86W8/HaZJYodKR+cAS 3WFPyuxP7IKiT7ofrSP8lyo1/ExVV//GoPjnk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704741568; x=1705346368; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:mail-followup-to:message-id:subject:cc:to :from:date:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=EJ2fxcKwoH+j3zf8yKo3Vu+cgAPiKaowc7Mo1YkN7dA=; b=iQ6BsETQCnAyfnwGmbXCYD6YnIk0JvAOyO3O9xI4DbDteBfLriTyeAcsc8LEusE/Xr DPunwkqSV3r5U0ijAFm5OxAsM9cftRejI333OQ2muesMkGRN4nlYIYub2DUugFMVIOhz oBMDntKV3UC0ukopuapHFYW3RSTyO9pUaDEHLxElzpLTvTidBXS+QzUeuevD3/1lSmO+ +V90tOXu82qR9QvWrA1uZnH0BLPpgqrfsA57yUZDpBnkU/nimkiKIeaLL0RrOcZYzXGt SxFBtEpoXDJbSfyqReP2ys02NixGm4KM9oDrW2ls7wPMnFvflkw00Pet1naXc5KX3CHU mSbA== X-Gm-Message-State: AOJu0YwjwXx7iw4FsxbadNh3u6OjrMoQB2PcEp8PD/QN0aUU2SYv1J24 YLt8j2+CeickM+ZRrmGxehdEmEYORjn4NQ== X-Google-Smtp-Source: AGHT+IE/x3yLDZZ6n/5O2iCOUHnc0Ya5TLPSdiVuLb4m5g8sI5sQyD6iWnZm6A7Hd4Q/GNClgkhmrQ== X-Received: by 2002:a05:6512:3ba1:b0:50e:b2ba:15d with SMTP id g33-20020a0565123ba100b0050eb2ba015dmr4109768lfv.1.1704741567605; Mon, 08 Jan 2024 11:19:27 -0800 (PST) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id p7-20020a05600c468700b0040d30af488asm12097912wmo.40.2024.01.08.11.19.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 08 Jan 2024 11:19:27 -0800 (PST) Date: Mon, 8 Jan 2024 20:19:25 +0100 From: Daniel Vetter To: Paul Cercueil Cc: Daniel Vetter , Greg Kroah-Hartman , Sumit Semwal , Christian =?iso-8859-1?Q?K=F6nig?= , Jonathan Corbet , Michael Hennerich , linux-doc@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Andrzej Pietrasiewicz , linaro-mm-sig@lists.linaro.org, Nuno =?iso-8859-1?Q?S=E1?= , Jonathan Cameron , linux-media@vger.kernel.org Subject: Re: [PATCH v3 3/4] usb: gadget: functionfs: Add DMABUF import interface Message-ID: Mail-Followup-To: Paul Cercueil , Greg Kroah-Hartman , Sumit Semwal , Christian =?iso-8859-1?Q?K=F6nig?= , Jonathan Corbet , Michael Hennerich , linux-doc@vger.kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Andrzej Pietrasiewicz , linaro-mm-sig@lists.linaro.org, Nuno =?iso-8859-1?Q?S=E1?= , Jonathan Cameron , linux-media@vger.kernel.org References: <20240108120056.22165-1-paul@crapouillou.net> <20240108120056.22165-4-paul@crapouillou.net> <2c0d4ef1b657c56ea2290fe16d757ce563a3e71b.camel@crapouillou.net> Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2c0d4ef1b657c56ea2290fe16d757ce563a3e71b.camel@crapouillou.net> X-Operating-System: Linux phenom 6.5.0-4-amd64 On Mon, Jan 08, 2024 at 05:27:33PM +0100, Paul Cercueil wrote: > Le lundi 08 janvier 2024 à 16:29 +0100, Daniel Vetter a écrit : > > On Mon, Jan 08, 2024 at 03:21:21PM +0100, Paul Cercueil wrote: > > > Hi Daniel (Sima?), > > > > > > Le lundi 08 janvier 2024 à 13:39 +0100, Daniel Vetter a écrit : > > > > On Mon, Jan 08, 2024 at 01:00:55PM +0100, Paul Cercueil wrote: > > > > > +static void ffs_dmabuf_signal_done(struct ffs_dma_fence > > > > > *dma_fence, int ret) > > > > > +{ > > > > > + struct ffs_dmabuf_priv *priv = dma_fence->priv; > > > > > + struct dma_fence *fence = &dma_fence->base; > > > > > + > > > > > + dma_fence_get(fence); > > > > > + fence->error = ret; > > > > > + dma_fence_signal(fence); > > > > > + > > > > > + dma_buf_unmap_attachment(priv->attach, dma_fence->sgt, > > > > > dma_fence->dir); > > > > > + dma_fence_put(fence); > > > > > + ffs_dmabuf_put(priv->attach); > > > > > > > > So this can in theory take the dma_resv lock, and if the usb > > > > completion > > > > isn't an unlimited worker this could hold up completion of future > > > > dma_fence, resulting in a deadlock. > > > > > > > > Needs to be checked how usb works, and if stalling indefinitely > > > > in > > > > the > > > > io_complete callback can hold up the usb stack you need to: > > > > > > > > - drop a dma_fence_begin/end_signalling annotations in here > > > > - pull out the unref stuff into a separate preallocated worker > > > > (or at > > > >   least the final unrefs for ffs_dma_buf). > > > > > > Only ffs_dmabuf_put() can attempt to take the dma_resv and would > > > have > > > to be in a worker, right? Everything else would be inside the > > > dma_fence_begin/end_signalling() annotations? > > > > Yup. Also I noticed that unlike the iio patches you don't have the > > dma_buf_unmap here in the completion path (or I'm blind?), which > > helps a > > lot with avoiding trouble. > > They both call dma_buf_unmap_attachment() in the "signal done" > callback, the only difference I see is that it is called after the > dma_fence_put() in the iio patches, while it's called before > dma_fence_put() here. I was indeed blind ... So the trouble is this wont work because: - dma_buf_unmap_attachment() requires dma_resv_lock. This is a somewhat recent-ish change from 47e982d5195d ("dma-buf: Move dma_buf_map_attachment() to dynamic locking specification"), so maybe old kernel or you don't have full lockdep enabled to get the right splat. - dma_fence critical section forbids dma_resv_lock Which means you need to move this out, but then there's the potential cache management issue. Which current gpu drivers just kinda ignore because it doesn't matter for current use-case, they all cache the mapping for about as long as the attachment exists. You might want to do the same, unless that somehow breaks a use-case you have, I have no idea about that. If something breaks with unmap_attachment moved out of the fence handling then I guess it's high time to add separate cache-management only to dma_buf (and that's probably going to be quite some wiring up, not sure even how easy that would be to do nor what exactly the interface should look like). Cheers, Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch