Re: [E1000-devel] [net-next v4 7/8] i40e: sysfs and debugfs interfaces

[Stephen Hemminger <stephen@networkplumber.org>]

I don't think you need this. If you put a NULL pointer in for the __ATTR()
then it will do the right thing for you.

> +/**
> + * i40e_sys_store_ro - callback for readonly attributes in sysfs
> + * @kobj: object in the sysfs model
> + * @attr: attribute being read
> + * @buf: buffer to put data
> + * @count: buffer size
> + **/
> +static ssize_t i40e_sys_store_ro(struct kobject *kobj,
> +				 struct kobj_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	return -1;
> +}
> +

These are bogus, just return an error code or make don't define
the file in the first place. Returning error in contents
is not useful for programmatic usage.

> +
> +/**
> + * i40e_sys_veb_svlan_read - read the VEB svlan
> + **/
> +static ssize_t i40e_sys_veb_svlan_read(struct kobject *kobj,
> +				       struct kobj_attribute *attr,
> +				       char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "(not implemented)\n");
> +}
> +

The store routines should all be checking for permissions.

	if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
		return -EPERM;


The sysfs API is doing writes to device state without locking.
You probably want to do:
	if (!rtnl_trylock())
		return restart_syscall();

...
	rtnl_unlock();


Also, anything in sysfs is device specific and you really need to make
a strong case for why your device is special and needs an exception.
Other devices will have hardware switches and doing something through
sysfs is going to create a pain for any controller application.

I vote against including the sysfs VEB stuff because it will become
a lifetime ABI.