Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v5 2/3] uacce: add uacce driver
To:     reg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Jonathan Cameron <jonathan.cameron@huawei.com>,
        Arnd Bergmann <arnd@arndb.de>, grant.likely@arm.com,
        jean-philippe <jean-philippe@linaro.org>,
        ilias.apalodimas@linaro.org, francois.ozog@linaro.org,
        kenneth-lee-2012@foxmail.com, Wangzhou <wangzhou1@hisilicon.com>,
        linux-accelerators@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        Kenneth Lee <liguozhu@hisilicon.com>,
        Zaibo Xu <xuzaibo@huawei.com>
References: <1571035735-31882-1-git-send-email-zhangfei.gao@linaro.org>
 <1571035735-31882-3-git-send-email-zhangfei.gao@linaro.org>
 <20191014113231.00002967@huawei.com>
 <e71edf57-8449-e8d1-01ba-aed3ecf379e1@linaro.org>
 <20191015175555.GB1072965@kroah.com>
From:   zhangfei <zhangfei.gao@linaro.org>
Message-ID: <f20230bd-0c48-4362-c434-207f6f2478f0@linaro.org>
Date:   Wed, 16 Oct 2019 16:37:28 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <20191015175555.GB1072965@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 2019/10/16 上午1:55, reg Kroah-Hartman wrote:
> 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@linaro.org> wrote:
>>>
>>>> From: Kenneth Lee <liguozhu@hisilicon.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@hisilicon.com>
>>>> Signed-off-by: Zaibo Xu <xuzaibo@huawei.com>
>>>> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.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.
>
OK, understand. Have updated in v6.

Thanks for the patience.