On Mon, Jul 18, 2022 at 5:02 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
