Re: [PATCH v2 2/3] staging: rtl8192u: move debug files to debugfs

[Tong Zhang]

On Tue, Jul 26, 2022 at 11:37 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Jul 19, 2022 at 11:30:48PM -0700, Tong Zhang wrote:
> > On Tue, Jul 19, 2022 at 5:53 AM Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Mon, Jul 18, 2022 at 10:50:37PM -0700, Tong Zhang wrote:
> > > > There are 4 debug files created under /proc/net/[Devname]. Devname could
> > > > Due to this is purely for debuging as files are created read only,
> > > > move this to debugfs like other NIC drivers do instead of using procfs.
> > > > This is also to prepare for address rmmod warn issue.
> > >
> > > Minor comments based on good debugfs usage:
> > >
> > > > --- a/drivers/staging/rtl8192u/r8192U.h
> > > > +++ b/drivers/staging/rtl8192u/r8192U.h
> > > > @@ -1061,6 +1061,9 @@ typedef struct r8192_priv {
> > > >       struct delayed_work gpio_change_rf_wq;
> > > >       struct delayed_work initialgain_operate_wq;
> > > >       struct workqueue_struct *priv_wq;
> > > > +
> > > > +     /* debugfs */
> > > > +     struct dentry *debugfs_dir;
> > >
> > > Why do you need to save this dentry?  Can't you just look it up when you
> > > want to remove the files?
> > >
> > > > +void rtl8192_debugfs_init(struct net_device *dev)
> > > >  {
> > > > -     struct proc_dir_entry *dir;
> > > > +     struct dentry *dir;
> > > > +     struct r8192_priv *priv = (struct r8192_priv *)ieee80211_priv(dev);
> > >
> > > No need to cast this.  Same for later on in this file.
> > >
> > > > -     if (!rtl8192_proc)
> > > > +     dir = debugfs_create_dir(dev->name, NULL);
> > > > +     if (IS_ERR(dir))
> > > >               return;
> > >
> >
> > I'm reading this code and your comment again.
> > Adding this check will avoid calling into debugfs_create_file() and 4
> > function calls and doing checks from there, probably will save a
> > couple of CPU cycles and avoid branch prediction penalty if there is
> > any.
> > I don't think the compiler can optimize for this case though it's not
> > performance critical. Anyho I personally feel it is better to keep
> > this.
>
> It's not an optimization issue, it's a "we never care about the results
> of a call to debugfs_*() issue".
>
> That's all, debugfs is intended to be easy to use, and you should never
> care about the return values of if it worked or not, so your code should
> not check it and do anything different based on it.
>
> Yes, it's not like "normal" kernel code, but debugfs is not normal at
> all, and should never expect to work as it's only for debugging.

Hi Greg,
Thanks for your review. I am mostly on the same page with you.
IMHO, since Ubuntu(or Debian derivative in general?) enables debugfs by default,
I think we need to make those ``debug''' code work although they might
not be designed to be used like this.
To my limited knowledge, iotop actually uses debugfs to show IO utilization.
(There might be more though)
Anyway, I incorporated your review and sent it as v3.
Thanks again!
- Tong

>
> thanks,
>
> greg k-h