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
> >
> >
