Re: [PATCH net-next v2 3/8] net: s/dev_get_mac_address/netif_get_mac_address/

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Stanislav Fomichev wrote:
> Commit cc34acd577f1 ("docs: net: document new locking reality")
> introduced netif_ vs dev_ function semantics: the former expects locked
> netdev, the latter takes care of the locking. We don't strictly
> follow this semantics on either side, but there are more dev_xxx handlers
> now that don't fit. Rename them to netif_xxx where appropriate.
> 
> netif_get_mac_address is used only by tun/tap, so move it into
> NETDEV_INTERNAL namespace.
> 
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  drivers/net/tap.c         | 6 ++++--
>  drivers/net/tun.c         | 4 +++-
>  include/linux/netdevice.h | 2 +-
>  net/core/dev.c            | 4 ++--
>  net/core/dev_ioctl.c      | 3 ++-
>  net/core/net-sysfs.c      | 2 +-
>  6 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index bdf0788d8e66..4c85770c809b 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -28,6 +28,8 @@
>  
>  #include "tun_vnet.h"
>  
> +MODULE_IMPORT_NS("NETDEV_INTERNAL");
> +
>  #define TAP_IFFEATURES (IFF_VNET_HDR | IFF_MULTI_QUEUE)
>  
>  static struct proto tap_proto = {
> @@ -1000,8 +1002,8 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
>  			return -ENOLINK;
>  		}
>  		ret = 0;
> -		dev_get_mac_address((struct sockaddr *)&ss, dev_net(tap->dev),
> -				    tap->dev->name);
> +		netif_get_mac_address((struct sockaddr *)&ss, dev_net(tap->dev),
> +				      tap->dev->name);
>  		if (copy_to_user(&ifr->ifr_name, tap->dev->name, IFNAMSIZ) ||
>  		    copy_to_user(&ifr->ifr_hwaddr, &ss, sizeof(ifr->ifr_hwaddr)))
>  			ret = -EFAULT;
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index f8c5e2fd04df..4509ae68decf 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -85,6 +85,8 @@
>  
>  #include "tun_vnet.h"
>  
> +MODULE_IMPORT_NS("NETDEV_INTERNAL");
> +

Thanks for giving this a go. Now that you've implemented it, does the
risk (of overlooking callers, mainly) indeed seem acceptable?

Documentation/core-api/symbol-namespaces.rst says

  It is advisable to add the MODULE_IMPORT_NS() statement close to other module
  metadata definitions like MODULE_AUTHOR() or MODULE_LICENSE().

No need to respin just for this from me. Something to consider,
especially if anything else comes up.

Just curious, did you use the modpost and make nsdeps, or was it
sufficient to find the callers with tools like cscope and grep?