Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "yekai(A)" <yekai13@huawei.com>
Subject: Re: [PATCH] uacce: add print information if not enable sva
To:     Greg KH <gregkh@linuxfoundation.org>
References: <1622792769-28017-1-git-send-email-yekai13@huawei.com>
 <YLnbgcJmQZChx1WV@kroah.com>
CC:     <linux-accelerators@lists.ozlabs.org>,
        <linux-kernel@vger.kernel.org>, <linuxarm@huawei.com>,
        <zhangfei.gao@linaro.org>, <wangzhou1@hisilicon.com>
Message-ID: <3f1590c4-b9f1-18f5-370a-57d9707f38a0@huawei.com>
Date:   Tue, 8 Jun 2021 09:39:52 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.7.1
MIME-Version: 1.0
In-Reply-To: <YLnbgcJmQZChx1WV@kroah.com>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2021/6/4 15:51, Greg KH wrote:
> On Fri, Jun 04, 2021 at 03:46:09PM +0800, Kai Ye wrote:
>> Add print information necessary if user not enable sva.
>>
>> Signed-off-by: Kai Ye <yekai13@huawei.com>
>> ---
>>  drivers/misc/uacce/uacce.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
>> index bae18ef0..fe38af8 100644
>> --- a/drivers/misc/uacce/uacce.c
>> +++ b/drivers/misc/uacce/uacce.c
>> @@ -387,15 +387,22 @@ static void uacce_release(struct device *dev)
>>
>>  static unsigned int uacce_enable_sva(struct device *parent, unsigned int flags)
>>  {
>> +	int ret;
>> +
>>  	if (!(flags & UACCE_DEV_SVA))
>>  		return flags;
>>
>>  	flags &= ~UACCE_DEV_SVA;
>>
>> -	if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_IOPF))
>> +	ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_IOPF);
>> +	if (ret) {
>> +		dev_err(parent, "failed to enable IOPF feature! ret = %d\n", ret);
>
> Why is this needed?  Has this ever happened in real life such that the
> log message is now required?
>
>
>
>>  		return flags;
>> +	}
>>
>> -	if (iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA)) {
>> +	ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);
>> +	if (ret) {
>> +		dev_err(parent, "failed to enable SVA feature! ret = %d\n", ret);
>
> Same here, does this happen in real systems?
>
> thanks,
>
> greg k-h
> .
>
In a other debug version, the SVA feature failed to be enabled, and no 
related information was printed. we don't know the cause of the problem. 
Finally, Although, it's not the problem here. but we find that the sva 
enable function doesn't have debug information. Therefore, it is 
inconvenient to locate the fault.
so i think the log message is required.

thanks

Kai