linux-kernel - Re: [PATCH 1/3] habanalabs: implement dma-fence mechanism

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200713155752.GC267581@kroah.com>
Date:   Mon, 13 Jul 2020 17:57:52 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Oded Gabbay <oded.gabbay@...il.com>
Cc:     linux-kernel@...r.kernel.org, SW_Drivers@...ana.ai,
        Ofir Bitton <obitton@...ana.ai>,
        Daniel Vetter <daniel.vetter@...ll.ch>
Subject: Re: [PATCH 1/3] habanalabs: implement dma-fence mechanism

On Mon, Jul 13, 2020 at 06:54:22PM +0300, Oded Gabbay wrote:
> From: Ofir Bitton <obitton@...ana.ai>
> 
> Instead of using standard dma-fence mechanism designed for GPU's, we
> introduce our own implementation based on the former one. This
> implementation is much more sparse than the original, contains only
> mandatory functionality required by the driver.

Sad you can't use the in-kernel code for this, I really don't understand
what's wrong with using it as-is.

Daniel, why do we need/want duplicate code floating around in the tree
like this?

Copying code leads to errors, here's some documentation ones:

> --- /dev/null
> +++ b/drivers/misc/habanalabs/hl_dma_fence.c
> @@ -0,0 +1,338 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Fence mechanism for dma-buf and to allow for asynchronous dma access

Is that what this still does?

> + *
> + * Copyright (C) 2012 Canonical Ltd
> + * Copyright (C) 2012 Texas Instruments
> + *
> + * Authors:
> + * Rob Clark <robdclark@...il.com>
> + * Maarten Lankhorst <maarten.lankhorst@...onical.com>
> + *
> + * The dma_fence module is a copy of dma-fence at drivers/dma-buf.

"The hl_dma_fence" module...

And is it a stand-alone module?  Or just a single file?

> + * This was done due to an explicit request by GPU developers who asked not
> + * to use the dma-buf module because we aren't part of DRM subsystem.

Why is dma-buf only for use for DRM?

If it is, should the symbol namespace be set to that to catch users that
want to use it for their own code?

> + * This copy was stripped from all extra features that habanalabs driver
> + * doesn't use, including the uapi interface dma-buf exposes.
> + * In addition, we removed the callbacks because the only usage is from inside
> + * habanalabs driver
> + */
> +
> +#include "hl_dma_fence.h"
> +#include "habanalabs.h"
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/atomic.h>
> +#include <linux/sched/signal.h>
> +
> +/**
> + * DOC: DMA fences overview
> + *
> + * DMA fences, represented by &struct hl_dma_fence, are the kernel internal
> + * synchronization primitive for DMA operations like GPU rendering, video
> + * encoding/decoding, or displaying buffers on a screen.

I don't think this is correct anymore, right?  :(

> --- /dev/null
> +++ b/drivers/misc/habanalabs/hl_dma_fence.h
> @@ -0,0 +1,148 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Fence mechanism for dma-buf to allow for asynchronous dma access
> + *
> + * Copyright (C) 2012 Canonical Ltd
> + * Copyright (C) 2012 Texas Instruments
> + *
> + * Authors:
> + * Rob Clark <robdclark@...il.com>
> + * Maarten Lankhorst <maarten.lankhorst@...onical.com>
> + *
> + * The dma_fence module is a copy of dma-fence at drivers/dma-buf.

Same comments here for the .h file.

thanks,

greg k-h