linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] uacce: add print information if not enable sva
@ 2021-06-04  7:46 Kai Ye
  2021-06-04  7:51 ` Greg KH
  2021-06-04  7:57 ` Joe Perches
  0 siblings, 2 replies; 5+ messages in thread
From: Kai Ye @ 2021-06-04  7:46 UTC (permalink / raw)
  To: gregkh, linux-accelerators, linux-kernel, linuxarm, zhangfei.gao,
	wangzhou1, yekai13

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);
 		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);
 		iommu_dev_disable_feature(parent, IOMMU_DEV_FEAT_IOPF);
 		return flags;
 	}
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] uacce: add print information if not enable sva
  2021-06-04  7:46 [PATCH] uacce: add print information if not enable sva Kai Ye
@ 2021-06-04  7:51 ` Greg KH
  2021-06-08  1:39   ` yekai(A)
  2021-06-04  7:57 ` Joe Perches
  1 sibling, 1 reply; 5+ messages in thread
From: Greg KH @ 2021-06-04  7:51 UTC (permalink / raw)
  To: Kai Ye
  Cc: linux-accelerators, linux-kernel, linuxarm, zhangfei.gao, wangzhou1

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] uacce: add print information if not enable sva
  2021-06-04  7:46 [PATCH] uacce: add print information if not enable sva Kai Ye
  2021-06-04  7:51 ` Greg KH
@ 2021-06-04  7:57 ` Joe Perches
  1 sibling, 0 replies; 5+ messages in thread
From: Joe Perches @ 2021-06-04  7:57 UTC (permalink / raw)
  To: Kai Ye, gregkh, linux-accelerators, linux-kernel, linuxarm,
	zhangfei.gao, wangzhou1

On Fri, 2021-06-04 at 15:46 +0800, Kai Ye wrote:
> Add print information necessary if user not enable sva.
[]
> diff --git 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;
[]
> +	ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_IOPF);
> +	if (ret) {
> +		dev_err(parent, "failed to enable IOPF feature! ret = %d\n", ret);

Perhaps use %pe, ERR_PTR(ret) to get descriptive error output

> +		dev_err(parent, "failed to enable SVA feature! ret = %d\n", ret);

here too



^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] uacce: add print information if not enable sva
  2021-06-04  7:51 ` Greg KH
@ 2021-06-08  1:39   ` yekai(A)
  2021-06-09  9:52     ` Greg KH
  0 siblings, 1 reply; 5+ messages in thread
From: yekai(A) @ 2021-06-08  1:39 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-accelerators, linux-kernel, linuxarm, zhangfei.gao, wangzhou1



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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] uacce: add print information if not enable sva
  2021-06-08  1:39   ` yekai(A)
@ 2021-06-09  9:52     ` Greg KH
  0 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2021-06-09  9:52 UTC (permalink / raw)
  To: yekai(A)
  Cc: linux-accelerators, linux-kernel, linuxarm, zhangfei.gao, wangzhou1

On Tue, Jun 08, 2021 at 09:39:52AM +0800, yekai(A) wrote:
> 
> 
> 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.

Ok, can you resend with the changes that Joe suggested to make this even
easier to use?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-09  9:52 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-04  7:46 [PATCH] uacce: add print information if not enable sva Kai Ye
2021-06-04  7:51 ` Greg KH
2021-06-08  1:39   ` yekai(A)
2021-06-09  9:52     ` Greg KH
2021-06-04  7:57 ` Joe Perches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).