Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAMhUBjmX0BDh-35GQxf3-fXFa8_EKfh2fwYKZDgH4NzGGQHiSg@mail.gmail.com>
 <20220717070204.705878-1-ztong0001@gmail.com> <20220718120149.GD2338@kadam>
In-Reply-To: <20220718120149.GD2338@kadam>
From:   Tong Zhang <ztong0001@gmail.com>
Date:   Mon, 18 Jul 2022 22:52:57 -0700
Message-ID: <CAA5qM4DWdtx6rndL5SGo=MacHYH8L2FVhmO_F07EDppkTnKbHA@mail.gmail.com>
Subject: Re: [PATCH] staging: rtl8192u: fix rmmod warn when wlan0 is renamed
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Colin Ian King <colin.king@intel.com>,
        Saurav Girepunje <saurav.girepunje@gmail.com>,
        Johan Hovold <johan@kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        linux-staging@lists.linux.dev, Zheyu Ma <zheyuma97@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jul 18, 2022 at 5:02 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Sun, Jul 17, 2022 at 12:01:57AM -0700, Tong Zhang wrote:
> > +static int rtl8192_usb_netdev_event(struct notifier_block *nb, unsigned long event,
> > +             void *data)
> > +{
> > +     struct net_device *netdev = netdev_notifier_info_to_dev(data);
> > +
> > +     if (netdev->netdev_ops != &rtl8192_netdev_ops)
> > +             goto out;
> > +
> > +     switch (event) {
> > +             case NETDEV_CHANGENAME:
> > +                     rtl8192_debugfs_rename(netdev);
> > +                     break;
> > +
> > +             default:
> > +                     break;
> > +     }
>
> This isn't indented properly.  Don't forget to run ./scripts/checkpatch.pl
> on your patch.
>
> > +
> > +out:
> > +     return NOTIFY_DONE;
> > +}
> > +
> > +static struct notifier_block rtl8192_usb_netdev_notifier = {
> > +     .notifier_call = rtl8192_usb_netdev_event,
> > +};
> > +
> > +
>
> Here too.
>
> >  static int __init rtl8192_usb_module_init(void)
> >  {
> >       int ret;
> > @@ -4788,10 +4638,14 @@ static int __init rtl8192_usb_module_init(void)
>
> [ snip ]
>
> > +void rtl8192_debugfs_init(struct net_device *dev)
> > +{
> > +     struct dentry *dir;
> > +     struct r8192_priv *priv = (struct r8192_priv *)ieee80211_priv(dev);
> > +
> > +     dir = debugfs_create_dir(dev->name, NULL);
> > +     if (IS_ERR_OR_NULL(dir))
> > +             return;
>
> In olden times the debugfs_create_dir() function used to return a mix
> of error pointers and NULL.  But the idea with that function is that
> most people are not supposed to check for errors.  But instead of that
> they added all kind of *buggy* checks.  So then we made it just return
> error pointers.
>
> This code *does* care about it because it uses the
> priv->debugfs_dir->d_name.name in rtl8192_debugfs_rename().
>
> But caring about the debugfs dir and creating a rtl8192_debugfs_rename()
> function is really unusual.  And normally when we have to do something
> unusual that means we are doing something wrong...  :/
>
> Anyway, just check for if (IS_ERR(dir)) {
>
> > +
> > +     debugfs_create_file("stats-rx", 0444, dir, dev, &rtl8192_usb_stats_rx_fops);
> > +     debugfs_create_file("stats-tx", 0444, dir, dev, &rtl8192_usb_stats_tx_fops);
> > +     debugfs_create_file("stats-ap", 0444, dir, dev, &rtl8192_usb_stats_ap_fops);
> > +     debugfs_create_file("registers", 0444, dir, dev, &rtl8192_usb_registers_fops);
> > +
> > +     priv->debugfs_dir = dir;
> > +}
> > +
> > +void rtl8192_debugfs_exit(struct net_device *dev)
> > +{
> > +     struct r8192_priv *priv = (struct r8192_priv *)ieee80211_priv(dev);
> > +     if (!priv->debugfs_dir)
> > +             return;
> > +     debugfs_remove_recursive(priv->debugfs_dir);
> > +     priv->debugfs_dir = NULL;
> > +}
> > +
> > +void rtl8192_debugfs_rename(struct net_device *dev)
> > +{
> > +     struct r8192_priv *priv = (struct r8192_priv *)ieee80211_priv(dev);
> > +
> > +     if (!priv->debugfs_dir)
> > +             return;
> > +
> > +     if (!strcmp(priv->debugfs_dir->d_name.name, dev->name))
> > +             return;
> > +
> > +     debugfs_rename(priv->debugfs_dir->d_parent, priv->debugfs_dir,
> > +                     priv->debugfs_dir->d_parent, dev->name);
> > +}
> > +
>
> regards,
> dan carpenter

I modified the patch and sent it as v2.
Thanks and have a good one!
- Tong