[PATCH RESEND] fpga: dfl: omit unneeded casts of u64 values for dev_dbg()

[PATCH RESEND] fpga: dfl: omit unneeded casts of u64 values for dev_dbg()

[Peter Colberg]

Omit unneeded casts of u64 values to unsigned long long for use with
printk() format specifier %llx. Unlike user space, the kernel defines
u64 as unsigned long long for all architectures; see commit 2a7930bd77fe
("Documentation/printk-formats.txt: No casts needed for u64/s64").

These changes are cosmetic only; no functional changes.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Peter Colberg <peter.colberg@intel.com>
---
This is an unmodified resend of the second patch only from the series
"fpga: dfl: clean up string formatting for sysfs_emit() and dev_dbg()".

Link: https://patchwork.kernel.org/project/linux-fpga/patch/0cffb04a207a98148c61f512aa4dc90880e51251.1687301688.git.peter.colberg@intel.com/
---
 drivers/fpga/dfl-afu-dma-region.c | 14 ++++++--------
 drivers/fpga/dfl-afu-main.c       |  4 +---
 drivers/fpga/dfl-fme-mgr.c        |  5 ++---
 3 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/fpga/dfl-afu-dma-region.c b/drivers/fpga/dfl-afu-dma-region.c
index 02b60fde0430..71ed9d394d07 100644
--- a/drivers/fpga/dfl-afu-dma-region.c
+++ b/drivers/fpga/dfl-afu-dma-region.c
@@ -146,8 +146,7 @@ static int afu_dma_region_add(struct dfl_feature_platform_data *pdata,
 	struct dfl_afu *afu = dfl_fpga_pdata_get_private(pdata);
 	struct rb_node **new, *parent = NULL;
 
-	dev_dbg(&pdata->dev->dev, "add region (iova = %llx)\n",
-		(unsigned long long)region->iova);
+	dev_dbg(&pdata->dev->dev, "add region (iova = %llx)\n", region->iova);
 
 	new = &afu->dma_regions.rb_node;
 
@@ -187,8 +186,7 @@ static void afu_dma_region_remove(struct dfl_feature_platform_data *pdata,
 {
 	struct dfl_afu *afu;
 
-	dev_dbg(&pdata->dev->dev, "del region (iova = %llx)\n",
-		(unsigned long long)region->iova);
+	dev_dbg(&pdata->dev->dev, "del region (iova = %llx)\n", region->iova);
 
 	afu = dfl_fpga_pdata_get_private(pdata);
 	rb_erase(&region->node, &afu->dma_regions);
@@ -210,7 +208,7 @@ void afu_dma_region_destroy(struct dfl_feature_platform_data *pdata)
 		region = container_of(node, struct dfl_afu_dma_region, node);
 
 		dev_dbg(&pdata->dev->dev, "del region (iova = %llx)\n",
-			(unsigned long long)region->iova);
+			region->iova);
 
 		rb_erase(node, &afu->dma_regions);
 
@@ -255,7 +253,7 @@ afu_dma_region_find(struct dfl_feature_platform_data *pdata, u64 iova, u64 size)
 
 		if (dma_region_check_iova(region, iova, size)) {
 			dev_dbg(dev, "find region (iova = %llx)\n",
-				(unsigned long long)region->iova);
+				region->iova);
 			return region;
 		}
 
@@ -268,8 +266,8 @@ afu_dma_region_find(struct dfl_feature_platform_data *pdata, u64 iova, u64 size)
 			break;
 	}
 
-	dev_dbg(dev, "region with iova %llx and size %llx is not found\n",
-		(unsigned long long)iova, (unsigned long long)size);
+	dev_dbg(dev, "region with iova %llx and size %llx is not found\n", iova,
+		size);
 
 	return NULL;
 }
diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c
index c0a75ca360d6..4342d40a2106 100644
--- a/drivers/fpga/dfl-afu-main.c
+++ b/drivers/fpga/dfl-afu-main.c
@@ -730,9 +730,7 @@ afu_ioctl_dma_map(struct dfl_feature_platform_data *pdata, void __user *arg)
 	}
 
 	dev_dbg(&pdata->dev->dev, "dma map: ua=%llx, len=%llx, iova=%llx\n",
-		(unsigned long long)map.user_addr,
-		(unsigned long long)map.length,
-		(unsigned long long)map.iova);
+		map.user_addr, map.length, map.iova);
 
 	return 0;
 }
diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
index ab228d8837a0..da3cb9c35de5 100644
--- a/drivers/fpga/dfl-fme-mgr.c
+++ b/drivers/fpga/dfl-fme-mgr.c
@@ -150,7 +150,7 @@ static int fme_mgr_write_init(struct fpga_manager *mgr,
 	priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
 	if (priv->pr_error)
 		dev_dbg(dev, "previous PR error detected %llx\n",
-			(unsigned long long)priv->pr_error);
+			priv->pr_error);
 
 	dev_dbg(dev, "set PR port ID\n");
 
@@ -242,8 +242,7 @@ static int fme_mgr_write_complete(struct fpga_manager *mgr,
 	dev_dbg(dev, "PR operation complete, checking status\n");
 	priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
 	if (priv->pr_error) {
-		dev_dbg(dev, "PR error detected %llx\n",
-			(unsigned long long)priv->pr_error);
+		dev_dbg(dev, "PR error detected %llx\n", priv->pr_error);
 		return -EIO;
 	}
 
-- 
2.44.0

Re: [PATCH RESEND] fpga: dfl: omit unneeded casts of u64 values for dev_dbg()

[Xu Yilun]

On Thu, Mar 28, 2024 at 08:04:29PM -0400, Peter Colberg wrote:
> Omit unneeded casts of u64 values to unsigned long long for use with
> printk() format specifier %llx. Unlike user space, the kernel defines
> u64 as unsigned long long for all architectures; see commit 2a7930bd77fe
> ("Documentation/printk-formats.txt: No casts needed for u64/s64").

The change is OK. But I suggest just delete the unnecessary dev_dbg()
since now people normally don't want these "Hello, I'm here!" info.

> 
> These changes are cosmetic only; no functional changes.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Peter Colberg <peter.colberg@intel.com>
> ---
> This is an unmodified resend of the second patch only from the series
> "fpga: dfl: clean up string formatting for sysfs_emit() and dev_dbg()".

Why only pick this patch out of the series?

[...]

> diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> index ab228d8837a0..da3cb9c35de5 100644
> --- a/drivers/fpga/dfl-fme-mgr.c
> +++ b/drivers/fpga/dfl-fme-mgr.c
> @@ -150,7 +150,7 @@ static int fme_mgr_write_init(struct fpga_manager *mgr,
>  	priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
>  	if (priv->pr_error)
>  		dev_dbg(dev, "previous PR error detected %llx\n",
> -			(unsigned long long)priv->pr_error);
> +			priv->pr_error);

I'm not sure if this is an real problem. Maybe we could keep it.

>  
>  	dev_dbg(dev, "set PR port ID\n");
>  
> @@ -242,8 +242,7 @@ static int fme_mgr_write_complete(struct fpga_manager *mgr,
>  	dev_dbg(dev, "PR operation complete, checking status\n");
>  	priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
>  	if (priv->pr_error) {
> -		dev_dbg(dev, "PR error detected %llx\n",
> -			(unsigned long long)priv->pr_error);
> +		dev_dbg(dev, "PR error detected %llx\n", priv->pr_error);

This is a real problem, is it? Change to dev_err()?

Thanks,
Yilun

>  		return -EIO;
>  	}
>  
> -- 
> 2.44.0
> 
> 

Re: [PATCH RESEND] fpga: dfl: omit unneeded casts of u64 values for dev_dbg()

[Colberg, Peter]

On Tue, 2024-04-02 at 11:29 +0800, Xu Yilun wrote:
> On Thu, Mar 28, 2024 at 08:04:29PM -0400, Peter Colberg wrote:
> > Omit unneeded casts of u64 values to unsigned long long for use with
> > printk() format specifier %llx. Unlike user space, the kernel defines
> > u64 as unsigned long long for all architectures; see commit 2a7930bd77fe
> > ("Documentation/printk-formats.txt: No casts needed for u64/s64").
> 
> The change is OK. But I suggest just delete the unnecessary dev_dbg()
> since now people normally don't want these "Hello, I'm here!" info.

Do you have specific commits in mind as a precedent that remove similar
occurrences? This patch was intended to be cosmetic, whereas dropping
dev_dbg() would introduce functional changes in a strict sense.

> > 
> > These changes are cosmetic only; no functional changes.
> > 
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Peter Colberg <peter.colberg@intel.com>
> > ---
> > This is an unmodified resend of the second patch only from the series
> > "fpga: dfl: clean up string formatting for sysfs_emit() and dev_dbg()".
> 
> Why only pick this patch out of the series?

The two patches were cleanup patches but otherwise unrelated. I still
have to carry out the perf testing of the other patch as you suggested.

> 
> [...]
> 
> > diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c
> > index ab228d8837a0..da3cb9c35de5 100644
> > --- a/drivers/fpga/dfl-fme-mgr.c
> > +++ b/drivers/fpga/dfl-fme-mgr.c
> > @@ -150,7 +150,7 @@ static int fme_mgr_write_init(struct fpga_manager *mgr,
> >  	priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> >  	if (priv->pr_error)
> >  		dev_dbg(dev, "previous PR error detected %llx\n",
> > -			(unsigned long long)priv->pr_error);
> > +			priv->pr_error);
> 
> I'm not sure if this is an real problem. Maybe we could keep it.
> 
> >  
> >  	dev_dbg(dev, "set PR port ID\n");
> >  
> > @@ -242,8 +242,7 @@ static int fme_mgr_write_complete(struct fpga_manager *mgr,
> >  	dev_dbg(dev, "PR operation complete, checking status\n");
> >  	priv->pr_error = fme_mgr_pr_error_handle(fme_pr);
> >  	if (priv->pr_error) {
> > -		dev_dbg(dev, "PR error detected %llx\n",
> > -			(unsigned long long)priv->pr_error);
> > +		dev_dbg(dev, "PR error detected %llx\n", priv->pr_error);
> 
> This is a real problem, is it? Change to dev_err()?

Thanks; yes, dev_err() seems reasonable and is consistent with the
earlier dev_err() after readq_poll_timeout().

Peter

> 
> Thanks,
> Yilun
> 
> >  		return -EIO;
> >  	}
> >  
> > -- 
> > 2.44.0
> > 
> > 

Re: [PATCH RESEND] fpga: dfl: omit unneeded casts of u64 values for dev_dbg()

[Colberg, Peter]

On Tue, 2024-04-02 at 11:29 +0800, Xu Yilun wrote:
> On Thu, Mar 28, 2024 at 08:04:29PM -0400, Peter Colberg wrote:
> > Omit unneeded casts of u64 values to unsigned long long for use with
> > printk() format specifier %llx. Unlike user space, the kernel defines
> > u64 as unsigned long long for all architectures; see commit 2a7930bd77fe
> > ("Documentation/printk-formats.txt: No casts needed for u64/s64").
> 
> The change is OK. But I suggest just delete the unnecessary dev_dbg()
> since now people normally don't want these "Hello, I'm here!" info.

How would you like me to proceed? Should I remove dev_dbg() in all DFL
modules? There are "Hello, I'm here!" in non-DFL FPGA modules, too.

$ rg --sort=path -c dev_dbg drivers/fpga/
drivers/fpga/altera-freeze-bridge.c:7
drivers/fpga/altera-pr-ip-core.c:1
drivers/fpga/dfl-afu-dma-region.c:7
drivers/fpga/dfl-afu-error.c:1
drivers/fpga/dfl-afu-main.c:8
drivers/fpga/dfl-fme-error.c:1
drivers/fpga/dfl-fme-main.c:3
drivers/fpga/dfl-fme-mgr.c:12
drivers/fpga/dfl-fme-perf.c:2
drivers/fpga/dfl-fme-pr.c:1
drivers/fpga/dfl-fme-region.c:1
drivers/fpga/dfl-n3000-nios.c:1
drivers/fpga/dfl-pci.c:3
drivers/fpga/dfl.c:4
drivers/fpga/fpga-bridge.c:4
drivers/fpga/fpga-region.c:3
drivers/fpga/socfpga-a10.c:2
drivers/fpga/stratix10-soc.c:4

Thanks,
Peter

Re: [PATCH RESEND] fpga: dfl: omit unneeded casts of u64 values for dev_dbg()

[Xu Yilun]

On Thu, Apr 18, 2024 at 09:30:48PM +0000, Colberg, Peter wrote:
> On Tue, 2024-04-02 at 11:29 +0800, Xu Yilun wrote:
> > On Thu, Mar 28, 2024 at 08:04:29PM -0400, Peter Colberg wrote:
> > > Omit unneeded casts of u64 values to unsigned long long for use with
> > > printk() format specifier %llx. Unlike user space, the kernel defines
> > > u64 as unsigned long long for all architectures; see commit 2a7930bd77fe
> > > ("Documentation/printk-formats.txt: No casts needed for u64/s64").
> > 
> > The change is OK. But I suggest just delete the unnecessary dev_dbg()
> > since now people normally don't want these "Hello, I'm here!" info.
> 
> How would you like me to proceed? Should I remove dev_dbg() in all DFL

I think do for all DFL would be enough. Usually you want something to
follow up and do the related clean up in advance.

Thanks,
Yilun

> modules? There are "Hello, I'm here!" in non-DFL FPGA modules, too.
> 
> $ rg --sort=path -c dev_dbg drivers/fpga/
> drivers/fpga/altera-freeze-bridge.c:7
> drivers/fpga/altera-pr-ip-core.c:1
> drivers/fpga/dfl-afu-dma-region.c:7
> drivers/fpga/dfl-afu-error.c:1
> drivers/fpga/dfl-afu-main.c:8
> drivers/fpga/dfl-fme-error.c:1
> drivers/fpga/dfl-fme-main.c:3
> drivers/fpga/dfl-fme-mgr.c:12
> drivers/fpga/dfl-fme-perf.c:2
> drivers/fpga/dfl-fme-pr.c:1
> drivers/fpga/dfl-fme-region.c:1
> drivers/fpga/dfl-n3000-nios.c:1
> drivers/fpga/dfl-pci.c:3
> drivers/fpga/dfl.c:4
> drivers/fpga/fpga-bridge.c:4
> drivers/fpga/fpga-region.c:3
> drivers/fpga/socfpga-a10.c:2
> drivers/fpga/stratix10-soc.c:4
> 
> Thanks,
> Peter

Re: [PATCH RESEND] fpga: dfl: omit unneeded casts of u64 values for dev_dbg()

[andriy.shevchenko]

On Thu, Apr 18, 2024 at 09:30:48PM +0000, Colberg, Peter wrote:
> On Tue, 2024-04-02 at 11:29 +0800, Xu Yilun wrote:
> > On Thu, Mar 28, 2024 at 08:04:29PM -0400, Peter Colberg wrote:

...

> $ rg --sort=path -c dev_dbg drivers/fpga/

Side Q: is 'rg' an alias to `git grep`?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RESEND] fpga: dfl: omit unneeded casts of u64 values for dev_dbg()

[Colberg, Peter]

On Fri, 2024-04-19 at 17:05 +0800, Xu Yilun wrote:
> On Thu, Apr 18, 2024 at 09:30:48PM +0000, Colberg, Peter wrote:
> > On Tue, 2024-04-02 at 11:29 +0800, Xu Yilun wrote:
> > > On Thu, Mar 28, 2024 at 08:04:29PM -0400, Peter Colberg wrote:
> > > > Omit unneeded casts of u64 values to unsigned long long for use with
> > > > printk() format specifier %llx. Unlike user space, the kernel defines
> > > > u64 as unsigned long long for all architectures; see commit 2a7930bd77fe
> > > > ("Documentation/printk-formats.txt: No casts needed for u64/s64").
> > > 
> > > The change is OK. But I suggest just delete the unnecessary dev_dbg()
> > > since now people normally don't want these "Hello, I'm here!" info.
> > 
> > How would you like me to proceed? Should I remove dev_dbg() in all DFL
> 
> I think do for all DFL would be enough. Usually you want something to
> follow up and do the related clean up in advance.

Thanks Yilun. I will include the removals of dev_dbg() in a revision of
the SRIOV patch series, since it touches many DFL modules as well.

Thanks,
Peter

Re: [PATCH RESEND] fpga: dfl: omit unneeded casts of u64 values for dev_dbg()

[Colberg, Peter]

On Fri, 2024-04-19 at 16:24 +0300, andriy.shevchenko@linux.intel.com
wrote:
> > On Thu, Apr 18, 2024 at 09:30:48PM +0000, Colberg, Peter wrote:
> > > > On Tue, 2024-04-02 at 11:29 +0800, Xu Yilun wrote:
> > > > > > On Thu, Mar 28, 2024 at 08:04:29PM -0400, Peter Colberg wrote:
> > 
> > ...
> > 
> > > > $ rg --sort=path -c dev_dbg drivers/fpga/
> > 
> > Side Q: is 'rg' an alias to `git grep`?
> > 

No, ripgrep; thanks for the hint to prefer `git grep` going forward.

Thanks,
Peter

Re: [PATCH RESEND] fpga: dfl: omit unneeded casts of u64 values for dev_dbg()

[andriy.shevchenko]

On Fri, Apr 19, 2024 at 07:19:26PM +0000, Colberg, Peter wrote:
> On Fri, 2024-04-19 at 16:24 +0300, andriy.shevchenko@linux.intel.com
> wrote:
> > > On Thu, Apr 18, 2024 at 09:30:48PM +0000, Colberg, Peter wrote:
> > > > > On Tue, 2024-04-02 at 11:29 +0800, Xu Yilun wrote:
> > > > > > > On Thu, Mar 28, 2024 at 08:04:29PM -0400, Peter Colberg wrote:

...

> > > > > $ rg --sort=path -c dev_dbg drivers/fpga/
> > > 
> > > Side Q: is 'rg' an alias to `git grep`?
> > > 
> 
> No, ripgrep; thanks for the hint to prefer `git grep` going forward.

You're welcome!

P.S. The motivation to ask is to point out that `git grep` does it much much
faster as it goes via index, it's unlikely external tools use libgit2 or
similar approaches when they are run in the Git repo. Nevertheless, I am always
curious about such tools and open to learn :-)

-- 
With Best Regards,
Andy Shevchenko