Messages in this thread | | | Date | Sun, 3 Mar 2019 16:11:28 -0500 | From | Kimberly Brown <> | Subject | Re: [PATCH v4] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used |
| |
On Sun, Mar 03, 2019 at 09:05:43AM +0100, Greg KH wrote: > On Fri, Mar 01, 2019 at 02:18:24PM -0500, Kimberly Brown wrote: > > +/* > > + * Channel-level attribute_group callback function. Returns the permission for > > + * each attribute, and returns 0 if an attribute is not visible. > > + */ > > +static umode_t vmbus_chan_attr_is_visible(struct kobject *kobj, > > + struct attribute *attr, int idx) > > +{ > > + const struct vmbus_channel *channel = > > + container_of(kobj, struct vmbus_channel, kobj); > > + > > + /* Hide the monitor attributes if the monitor mechanism is not used. */ > > + if (!channel->offermsg.monitor_allocated && > > + (attr == &chan_attr_pending.attr || > > + attr == &chan_attr_latency.attr || > > + attr == &chan_attr_monitor_id.attr)) > > + return 0; > > + > > + return attr->mode; > > +} > > + > > +static struct attribute_group vmbus_chan_group = { > > + .attrs = vmbus_chan_attrs, > > + .is_visible = vmbus_chan_attr_is_visible > > +}; > > + > > static struct kobj_type vmbus_chan_ktype = { > > .sysfs_ops = &vmbus_chan_sysfs_ops, > > .release = vmbus_chan_release, > > - .default_attrs = vmbus_chan_attrs, > > Why did you remove this line?
I removed the default attributes because vmbus_chan_attrs contains non-default attributes. You suggested that I use one attribute_group and an is_visible() callback for the device-level attributes (see https://lore.kernel.org/lkml/20190226081848.GA15659@kroah.com/). I assumed (possibly incorrectly) that I should do the same for these channel-level attributes.
> > }; > > > > /* > > @@ -1571,6 +1624,12 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel) > > if (ret) > > return ret; > > > > + ret = sysfs_create_group(kobj, &vmbus_chan_group); > > Why are you adding these "by hand"? What was wrong with using the > default attribute group pointer? You also are not removing the > attributes :(
Are you referring to default_attrs in kobj_type? It's not an attribute_group pointer, it's a pointer to an attribute pointer array. The problem with using default_attrs is that all of the attributes are visible.
I'm fairly certain that the monitor attributes are being removed. sysfs_create_group() uses the attribute_group's is_visible() callback to control the attribute visibility. And, when I look at the sysfs files, I can see that the monitor sysyfs files are removed.
In v3, I proposed moving the monitor attributes to a special attribute_group and adding that group manually when needed. Do you prefer that approach for the channel-level monitor attributes?
Thanks, Kim
> > greg k-h
| |