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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Peter Colberg <peter.colberg@xxxxxxxxx>
> > ---
> > 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
> > 
> >