Re: [PATCH net-next] net: sysfs: Implement is_visible for phys_(port_id, port_name, switch_id)

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 15 May 2025 21:02:05 +0800 Yajun Deng wrote:
> +static struct attribute *netdev_phys_attrs[] __ro_after_init = {

Why __ro_after_init and not const? I can't find the reason with 
a quick grep. This is just an array of pointers, not objects.

> +	&dev_attr_phys_port_id.attr,
> +	&dev_attr_phys_port_name.attr,
> +	&dev_attr_phys_switch_id.attr,
> +	NULL,
> +};
> +
> +static umode_t netdev_phys_is_visible(struct kobject *kobj,
> +				      struct attribute *attr, int index)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct net_device *netdev = to_net_dev(dev);
> +
> +	if (attr == &dev_attr_phys_port_id.attr) {
> +		/* The check is also done in dev_get_phys_port_id; this helps returning
> +		 * early without hitting the locking section below.
> +		 */
> +		if (!netdev->netdev_ops->ndo_get_phys_port_id)
> +			return 0;
> +	} else if (attr == &dev_attr_phys_port_name.attr) {
> +		/* The checks are also done in dev_get_phys_port_name; this helps
> +		 * returning early without hitting the locking section below.
> +		 */
> +		if (!netdev->netdev_ops->ndo_get_phys_port_name &&
> +		    !netdev->devlink_port)
> +			return 0;
> +	} else if (attr == &dev_attr_phys_switch_id.attr) {
> +		/* The checks are also done in dev_get_phys_port_name; this helps
> +		 * returning early without hitting the locking section below. This works
> +		 * because recurse is false when calling dev_get_port_parent_id.
> +		 */
> +		if (!netdev->netdev_ops->ndo_get_port_parent_id &&
> +		    !netdev->devlink_port)
> +			return 0;

I'm slightly worried some user space depends on the files existing,
but maybe ENOENT vs EOPNOTSUPP doesn't make a big difference.

Can you remove the comments, tho? I don't think they add much value.
-- 
pw-bot: cr