linux-kernel - Re: [PATCH v5 2/3] uacce: add uacce driver

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: <20191015175555.GB1072965@kroah.com>
Date:   Tue, 15 Oct 2019 19:55:55 +0200
From:   reg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     zhangfei <zhangfei.gao@...aro.org>
Cc:     Jonathan Cameron <jonathan.cameron@...wei.com>,
        Arnd Bergmann <arnd@...db.de>, grant.likely@....com,
        jean-philippe <jean-philippe@...aro.org>,
        ilias.apalodimas@...aro.org, francois.ozog@...aro.org,
        kenneth-lee-2012@...mail.com, Wangzhou <wangzhou1@...ilicon.com>,
        linux-accelerators@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        Kenneth Lee <liguozhu@...ilicon.com>,
        Zaibo Xu <xuzaibo@...wei.com>
Subject: Re: [PATCH v5 2/3] uacce: add uacce driver

On Tue, Oct 15, 2019 at 03:39:00PM +0800, zhangfei wrote:
> Hi, Jonathan
> 
> On 2019/10/14 下午6:32, Jonathan Cameron wrote:
> > On Mon, 14 Oct 2019 14:48:54 +0800
> > Zhangfei Gao <zhangfei.gao@...aro.org> wrote:
> > 
> > > From: Kenneth Lee <liguozhu@...ilicon.com>
> > > 
> > > Uacce (Unified/User-space-access-intended Accelerator Framework) targets to
> > > provide Shared Virtual Addressing (SVA) between accelerators and processes.
> > > So accelerator can access any data structure of the main cpu.
> > > This differs from the data sharing between cpu and io device, which share
> > > data content rather than address.
> > > Since unified address, hardware and user space of process can share the
> > > same virtual address in the communication.
> > > 
> > > Uacce create a chrdev for every registration, the queue is allocated to
> > > the process when the chrdev is opened. Then the process can access the
> > > hardware resource by interact with the queue file. By mmap the queue
> > > file space to user space, the process can directly put requests to the
> > > hardware without syscall to the kernel space.
> > > 
> > > Signed-off-by: Kenneth Lee <liguozhu@...ilicon.com>
> > > Signed-off-by: Zaibo Xu <xuzaibo@...wei.com>
> > > Signed-off-by: Zhou Wang <wangzhou1@...ilicon.com>
> > > Signed-off-by: Zhangfei Gao <zhangfei.gao@...aro.org>
> > Hi,
> > 
> > Some superficial comments from me.
> Thanks for the suggestion.
> > > +/*
> > > + * While user space releases a queue, all the relatives on the queue
> > > + * should be released immediately by this putting.
> > This one needs rewording but I'm not quite sure what
> > relatives are in this case.
> > > + */
> > > +static long uacce_put_queue(struct uacce_queue *q)
> > > +{
> > > +	struct uacce_device *uacce = q->uacce;
> > > +
> > > +	mutex_lock(&uacce_mutex);
> > > +
> > > +	if ((q->state == UACCE_Q_STARTED) && uacce->ops->stop_queue)
> > > +		uacce->ops->stop_queue(q);
> > > +
> > > +	if ((q->state == UACCE_Q_INIT || q->state == UACCE_Q_STARTED) &&
> > > +	     uacce->ops->put_queue)
> > > +		uacce->ops->put_queue(q);
> > > +
> > > +	q->state = UACCE_Q_ZOMBIE;
> > > +	mutex_unlock(&uacce_mutex);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > ..
> > 
> > > +
> > > +static ssize_t qfrs_size_show(struct device *dev,
> > > +				struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct uacce_device *uacce = to_uacce_device(dev);
> > > +	unsigned long size;
> > > +	int i, ret;
> > > +
> > > +	for (i = 0, ret = 0; i < UACCE_QFRT_MAX; i++) {
> > > +		size = uacce->qf_pg_size[i] << PAGE_SHIFT;
> > > +		if (i == UACCE_QFRT_SS)
> > > +			break;
> > > +		ret += sprintf(buf + ret, "%lu\t", size);
> > > +	}
> > > +	ret += sprintf(buf + ret, "%lu\n", size);
> > > +
> > > +	return ret;
> > > +}
> > This may break the sysfs rule of one thing per file.  If you have
> > multiple regions, they should probably each have their own file
> > to give their size.
> Is the rule must be applied?

Yes, it always must be followed.  Please fix.