| Message ID | 20240329000429.7493-1-peter.colberg@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [RESEND] fpga: dfl: omit unneeded casts of u64 values for dev_dbg() | expand |
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 > >
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 > > > >
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
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
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`?
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
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
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 :-)
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(®ion->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; }
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(-)