Re: [PATCH v1 1/2] staging: gdm724x: fix type confusion in gdm_lte_event_rcv()

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jul 03, 2025 at 02:29:40PM +0800, Lin Ma wrote:
> Hello Greg,
> 
> > > @@ -522,6 +522,9 @@ static void gdm_lte_event_rcv(struct net_device *dev, u16 type,
> > >  {
> > >  	struct nic *nic = netdev_priv(dev);
> > >  
> > > +	if (dev->netdev_ops->ndo_open != gdm_lte_open)
> > > +		return;
> > 
> > Why should a driver be poking around in netdev_ops?  That feels wrong,
> > what would ever change this?  Why not fix that instead?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Yes, it seems that comparing ops is quite ambiguous and weird. However, investigation on
> code shows various checks against net_device.
> 
> - by `dev->priv_flags`. See is_vlan_dev()
> ```
> static inline bool is_vlan_dev(const struct net_device *dev)
> {
>         return dev->priv_flags & IFF_802_1Q_VLAN;
> }
> ```
> 
> - by `dev->rtnl_link_ops->kind`. See lookup_interface()
> ```
> static struct wg_device *lookup_interface(struct nlattr **attrs,
>                       struct sk_buff *skb)
> {
>     dev = dev_get_by_index(sock_net(skb->sk),
>                        nla_get_u32(attrs[WGDEVICE_A_IFINDEX]));
>     ...
>     if (!dev->rtnl_link_ops || !dev->rtnl_link_ops->kind ||
>         strcmp(dev->rtnl_link_ops->kind, KBUILD_MODNAME)) {
>     ...
> }
> ```
> 
> - by `dev->type`. See ax25_device_event()
> ```
> static int ax25_device_event(struct notifier_block *this, unsigned long event,
>                  void *ptr)
> {
>     struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> 
>     ......
> 
>     /* Reject non AX.25 devices */
>     if (dev->type != ARPHRD_AX25)
>         return NOTIFY_DONE;
> ```


Those are core functions that all drivers are using, and the "type" of
device is also ok to look at.  You are trying to compare a specific
callback in this change, which feels wrong to me.

Wait, what tree are you making this change against?  I don't even see
the file you are trying to patch in the latest tree, are you sure it's
not just deleted already?

> As for poking around in netdev_ops, we didn't know if it is the best solution,
> just because some code did similar checks, e.g.
> 
> ```
> static struct team *team_nl_team_get(struct genl_info *info)
> {
>     dev = dev_get_by_index(net, ifindex);
>     if (!dev || dev->netdev_ops != &team_netdev_ops) {
>         ... 
>     }
> }
> ```
> 
> and
> 
> ```
> static int mlx5e_set_vf_tunnel(...)
> {
>     route_dev = dev_get_by_index(dev_net(out_dev), route_dev_ifindex);
> 
>     if (!route_dev || route_dev->netdev_ops != &mlx5e_netdev_ops || ... ) {
>         ...
>     }
> }
> ```
> 
> From my point of view, using `dev->type/flags/priv_flags` could be a better
> choice if it's okay to introduce more definitions.
> 
> Using netdev_ops is more straightforward. Or maybe I should just compare the
> `->netdev_ops` instead of the `->netdev_ops->ndo_open`?

Again, make sure this file is still present in the tree before going
further :)

thanks,

greg k-h