Re: [net-next,06/10] devlink: add devlink_dev_driver_name() helper and use it in trace events

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "FYI"

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
devlink: add devlink_dev_driver_name() helper and use it in trace events

In preparation to dev-less devlinks, this patch adds
devlink_dev_driver_name() that safely returns the driver name or NULL,
and uses it in all trace events.

> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 0afb0958b910..45dec7067a8e 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1613,6 +1613,7 @@ struct devlink *priv_to_devlink(void *priv);
>  struct device *devlink_to_dev(const struct devlink *devlink);
>  const char *devlink_bus_name(const struct devlink *devlink);
>  const char *devlink_dev_name(const struct devlink *devlink);
> +const char *devlink_dev_driver_name(const struct devlink *devlink);
>
>  /* Devlink instance explicit locking */
>  void devl_lock(struct devlink *devlink);

[ ... ]

> diff --git a/net/devlink/core.c b/net/devlink/core.c
> index 2dd6d45bec18..85e83a3b082a 100644
> --- a/net/devlink/core.c
> +++ b/net/devlink/core.c
> @@ -260,6 +260,13 @@ const char *devlink_dev_name(const struct devlink *devlink)
>  }
>  EXPORT_SYMBOL_GPL(devlink_dev_name);
>
> +const char *devlink_dev_driver_name(const struct devlink *devlink)
> +{
> +	struct device *dev = devlink->dev;
> +
> +	return dev ? dev->driver->name : NULL;
                      ^^^^^^^^^^^^
> +}

Does this safely handle the case when dev->driver is NULL? The function
checks whether dev is NULL but does not check whether dev->driver is
NULL before dereferencing it.

In the kernel device model, struct device.driver is a pointer that is
NULL when no driver is bound. While devlink instances are typically
created during driver probe (so dev->driver should be non-NULL during
the devlink lifetime), the commit message describes this as a function
that "safely returns the driver name or NULL."

For full safety, especially as this prepares for dev-less devlinks where
lifecycle assumptions may change, should the function also guard against
dev->driver being NULL?

The same codebase already handles this condition in
devlink_nl_driver_info_get():

net/devlink/dev.c:857:
    struct device_driver *drv = dev->driver;

    if (!drv)
        return 0;

Should this be:

    return (dev && dev->driver) ? dev->driver->name : NULL;


Is devlink_dev_driver_name() missing EXPORT_SYMBOL_GPL? All sibling
accessor functions (devlink_to_dev, devlink_bus_name, devlink_dev_name)
are exported with EXPORT_SYMBOL_GPL.

The new function is declared in the public header include/net/devlink.h
and is called from trace event definitions in
include/trace/events/devlink.h. The mlxsw driver
(drivers/net/ethernet/mellanox/mlxsw/core.c) includes this trace header
and invokes trace_devlink_hwmsg() and trace_devlink_hwerr(), which
evaluate devlink_dev_driver_name() at the call site.

Since MLXSW_CORE is tristate (can be built as a module), this will cause
a link failure for modular builds.