Received-SPF: pass (google.com: domain of linux-kernel+bounces-37793-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Kasireddy, Vivek" <vivek.kasireddy@intel.com>
To: Andrew Davis <afd@ti.com>, Gerd Hoffmann <kraxel@redhat.com>, Sumit Semwal
	<sumit.semwal@linaro.org>, =?iso-8859-1?Q?Christian_K=F6nig?=
	<christian.koenig@amd.com>, Paul Cercueil <paul@crapouillou.net>
CC: "linaro-mm-sig@lists.linaro.org" <linaro-mm-sig@lists.linaro.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>
Subject: RE: [PATCH 2/3] udmabuf: Sync buffer mappings for attached devices
Thread-Topic: [PATCH 2/3] udmabuf: Sync buffer mappings for attached devices
Thread-Index: AQHaTklVj+JfGoUYak+H25iUh1Y+RrDpk0Jg
Date: Wed, 24 Jan 2024 23:05:43 +0000
Message-ID: <IA0PR11MB7185DDD7A972ED546B4CEA10F87B2@IA0PR11MB7185.namprd11.prod.outlook.com>
References: <20240123221227.868341-1-afd@ti.com>
 <20240123221227.868341-2-afd@ti.com>
In-Reply-To: <20240123221227.868341-2-afd@ti.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?yZIOI4YWUTZGIYYF94gWtPyk0dGF5c5IGnKWYUuS7kgbhYf02+B8nxdJQt?=
 =?iso-8859-1?Q?NsyMR78dOhEAcsbSWkLZg/8stoGMD5xjDhCjhum49CDek/H49E1haLn6oM?=
 =?iso-8859-1?Q?y3831YgW1MDUyXthtWRKEG+rOQJYjJV57vQghXYoZbo7KVou1Ek778LCHD?=
 =?iso-8859-1?Q?2NDYEXzLDcqy0lV170mHDrlxum2h+6NIRAiuScw9XkQPxJ6uNjjdT7rSbt?=
 =?iso-8859-1?Q?53XEt5f3sZ7AIHKYvFjjftJ/VlLRRlQd09jyY62D2AFWelXo/5VFN79RUZ?=
 =?iso-8859-1?Q?m8Bx0knUVkHoGvUcyVWsZHpncUTglNgw2if9+m3hZbXcicSeY2WYIQ2niv?=
 =?iso-8859-1?Q?b0JPJF3ZJkEnT/FVOPoUnBIBbE3jgGmRuNf5Fd8T41vFWNvXCsyz3s7/Pb?=
 =?iso-8859-1?Q?zAftk7/Scadt+G31s73xuuaxCN5oh61nehvAZKphc2WJYOJRRAxr1JmU58?=
 =?iso-8859-1?Q?Sbnxm07MBR8/SDojSHHsCOfXO0W8wfgux9Tsd0kssGSGsX/L1+8/7uExPu?=
 =?iso-8859-1?Q?0jS0rXk1f1NCg09A4zEv5JIxeNI4rPdaC3AH4tiswZhi3sLzIz+mFZjnn/?=
 =?iso-8859-1?Q?vBxfPBQgzaaFb6CuiM+sq9aneIvuZIs3XjFV3c3QXdijbk/OCIeS+VW/7I?=
 =?iso-8859-1?Q?xaI2XGeK7OzYhnKVXNRWcsCF3kYOYUrRawTqdTTcT5Sc8MUj7FzX79V41Z?=
 =?iso-8859-1?Q?5eH/DInReCmTczHjAu0XWqeQuvPDznub1F0MqG+W3r0utBoUtOD2ccB/vP?=
 =?iso-8859-1?Q?8aWmQDNNLQ7YvJ9qaG56jiiIflO0OFG6c8UCc8IIGLzbqyfVU3t6FCSDon?=
 =?iso-8859-1?Q?3IrAEHbe9j1kiZbtp+WZEUAAsxQtkFTSF16gW2L6gHe+jzNVosHTznRLHW?=
 =?iso-8859-1?Q?ErRgFHwDh68sskZ/uHxXXMnbIse8AhnDn4igVhOcScXSe/asKXTo7PlAqJ?=
 =?iso-8859-1?Q?ELeV2gUYyFKSgDiQHH30kpasetGwGQ3K7nwdDEqQE/6ETdghjdYU5ekj3F?=
 =?iso-8859-1?Q?HCbsTCeYCwakWIgcqLTW0Lq1cjH05aMoAkdl6gSgIKqwlstc3VxRU8AEut?=
 =?iso-8859-1?Q?yHyqlyv2kW8WAnOhwW1EYo2robUXqYzhao6H3p8T/IW0E+auAOYXp2wUin?=
 =?iso-8859-1?Q?OF5Ce/U9kGowW2gqAfepp09eWrMQr++cAjg/t1E1c8PnretI+QqnkJKazE?=
 =?iso-8859-1?Q?YnSpmgss7T616z1JViGw4ca2pa8UYLchvF8ILGkeGbAe0OmhIIK++fZbL3?=
 =?iso-8859-1?Q?9iw8GD4VEcB41oZMwUt8xqFVYiUdv25y0HWWqhfNQEdZJm4GHsvsIVk1LB?=
 =?iso-8859-1?Q?fYkjNPWFzgLFYg8PIzScZduKGYNQQdikNJH1dmPZLbQHWRz/uy0aACp0Pm?=
 =?iso-8859-1?Q?W3lvn+WYK8xfYHMtwbqUfpxPfnlCa/7TrKSC9Rbj2wrWgqd+9P+t0GS6I9?=
 =?iso-8859-1?Q?4XP0hcn6QKgILnG7MY2G/lS/smwVH5PVaqK9mXS/HcvsNJJMibSvvbmysG?=
 =?iso-8859-1?Q?c5VyknR2gSeDZtaFTjm4d2Xyt+lXgDcmCgPU41r68eWK1IO5Muw0R7Fox2?=
 =?iso-8859-1?Q?kPZvHsobW5XDD1xSmNTSzXYsI4nKJe0E2EQ7TLau0NP77sE9MppeRDz6nN?=
 =?iso-8859-1?Q?TulITecNaJYhksFjLkNmyYEvoqzFpuGeFq?=
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: IA0PR11MB7185.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1cf2527c-609e-47cc-7cad-08dc1d30fde9
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jan 2024 23:05:43.4816
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: WPK9l0dy5cQHjtd79AmMFbE8KqlgU8s9KKywNy0gh2XrLMoTCuKUsFLtrOVsnLsw2BPB3MjMdqwns+fkAsMSlvkq/KAzZ053cE6FK+3DADc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7643
X-OriginatorOrg: intel.com

Hi Andrew,

> Currently this driver creates a SGT table using the CPU as the
> target device, then performs the dma_sync operations against
> that SGT. This is backwards to how DMA-BUFs are supposed to behave.
> This may have worked for the case where these buffers were given
> only back to the same CPU that produced them as in the QEMU case.
> And only then because the original author had the dma_sync
> operations also backwards, syncing for the "device" on begin_cpu.
> This was noticed and "fixed" in this patch[0].
>=20
> That then meant we were sync'ing from the CPU to the CPU using
> a pseudo-device "miscdevice". Which then caused another issue
> due to the miscdevice not having a proper DMA mask (and why should
> it, the CPU is not a DMA device). The fix for that was an even
> more egregious hack[1] that declares the CPU is coherent with
> itself and can access its own memory space..
>=20
> Unwind all this and perform the correct action by doing the dma_sync
> operations for each device currently attached to the backing buffer.
Makes sense.

>=20
> [0] commit 1ffe09590121 ("udmabuf: fix dma-buf cpu access")
> [1] commit 9e9fa6a9198b ("udmabuf: Set the DMA mask for the udmabuf
> device (v2)")
>=20
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  drivers/dma-buf/udmabuf.c | 41 +++++++++++++++------------------------
>  1 file changed, 16 insertions(+), 25 deletions(-)
>=20
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 3a23f0a7d112a..ab6764322523c 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -26,8 +26,6 @@ MODULE_PARM_DESC(size_limit_mb, "Max size of a
> dmabuf, in megabytes. Default is
>  struct udmabuf {
>  	pgoff_t pagecount;
>  	struct page **pages;
> -	struct sg_table *sg;
> -	struct miscdevice *device;
>  	struct list_head attachments;
>  	struct mutex lock;
>  };
> @@ -169,12 +167,8 @@ static void unmap_udmabuf(struct
> dma_buf_attachment *at,
>  static void release_udmabuf(struct dma_buf *buf)
>  {
>  	struct udmabuf *ubuf =3D buf->priv;
> -	struct device *dev =3D ubuf->device->this_device;
>  	pgoff_t pg;
>=20
> -	if (ubuf->sg)
> -		put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
What happens if the last importer maps the dmabuf but erroneously
closes it immediately? Would unmap somehow get called in this case?

Thanks,
Vivek

> -
>  	for (pg =3D 0; pg < ubuf->pagecount; pg++)
>  		put_page(ubuf->pages[pg]);
>  	kfree(ubuf->pages);
> @@ -185,33 +179,31 @@ static int begin_cpu_udmabuf(struct dma_buf
> *buf,
>  			     enum dma_data_direction direction)
>  {
>  	struct udmabuf *ubuf =3D buf->priv;
> -	struct device *dev =3D ubuf->device->this_device;
> -	int ret =3D 0;
> -
> -	if (!ubuf->sg) {
> -		ubuf->sg =3D get_sg_table(dev, buf, direction);
> -		if (IS_ERR(ubuf->sg)) {
> -			ret =3D PTR_ERR(ubuf->sg);
> -			ubuf->sg =3D NULL;
> -		}
> -	} else {
> -		dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents,
> -				    direction);
> -	}
> +	struct udmabuf_attachment *a;
>=20
> -	return ret;
> +	mutex_lock(&ubuf->lock);
> +
> +	list_for_each_entry(a, &ubuf->attachments, list)
> +		dma_sync_sgtable_for_cpu(a->dev, a->table, direction);
> +
> +	mutex_unlock(&ubuf->lock);
> +
> +	return 0;
>  }
>=20
>  static int end_cpu_udmabuf(struct dma_buf *buf,
>  			   enum dma_data_direction direction)
>  {
>  	struct udmabuf *ubuf =3D buf->priv;
> -	struct device *dev =3D ubuf->device->this_device;
> +	struct udmabuf_attachment *a;
>=20
> -	if (!ubuf->sg)
> -		return -EINVAL;
> +	mutex_lock(&ubuf->lock);
> +
> +	list_for_each_entry(a, &ubuf->attachments, list)
> +		dma_sync_sgtable_for_device(a->dev, a->table, direction);
> +
> +	mutex_unlock(&ubuf->lock);
>=20
> -	dma_sync_sg_for_device(dev, ubuf->sg->sgl, ubuf->sg->nents,
> direction);
>  	return 0;
>  }
>=20
> @@ -307,7 +299,6 @@ static long udmabuf_create(struct miscdevice
> *device,
>  	exp_info.priv =3D ubuf;
>  	exp_info.flags =3D O_RDWR;
>=20
> -	ubuf->device =3D device;
>  	buf =3D dma_buf_export(&exp_info);
>  	if (IS_ERR(buf)) {
>  		ret =3D PTR_ERR(buf);
> --
> 2.39.2