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

["Yajun Deng" <yajun.deng@linux.dev>]

May 17, 2025 at 6:26 AM, "Jakub Kicinski" <kuba@kernel.org> wrote:



> 
> 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.
> 

These attributes in net_class_attrs had __ro_after_init before this patch.


> > 
> > + &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.
> 
Okay.

> -- 
> 
> pw-bot: cr
>