On Fri, Jan 15, 2021 at 12:20:59PM +0100, Niklas Schnelle wrote:
>
>
> On 1/14/21 5:14 PM, Greg Kroah-Hartman wrote:
> > On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote:
> >>
> >>
> >> On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote:
> >>> On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote:
> >>>>
> >>>>
> >>>> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote:
> >>>>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote:
> >>>>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote:
> >>>>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote:
> >>>>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote:
> ... snip ...
> >>>>>>
> >>>>>> Hey Niklas et al. :)
> >>>>>>
> >>>>>> I think this will need input from Greg. He should be best versed in
> >>>>>> sysfs attributes. The problem with KERNEL_ATTR_* to me seems that it's
> >>>>>> supposed to be kernel internal. Now, that might just be a matter of
> >>>>>> renaming the macro but let's see whether Greg has any better idea or
> >>>>>> more questions. :)
> >>>>>
> >>>>> The big question is, why are you needing this?
> >>>>>
> >>>>> No driver or driver subsystem should EVER be messing with a "raw"
> >>>>> kobject like this. Just use the existing DEVICE_* macros instead
> >>>>> please.
> >>>>>
> >>>>> If you are using a raw kobject, please ask me how to do this properly,
> >>>>> as that is something that should NEVER show up in the /sys/devices/*
> >>>>> tree. Otherwise userspace tools will break.
> >>>>>
> >>>>> thanks,
> >>>>>
> >>>>> greg k-h
> >>>>
> >>>> Hi Greg,
> >>>>
> >>>> this is for an architecture specific but global i.e. not device bound PCI
> >>>> attribute. That's why DEVICE_ATTR_* does not work. BUS_ATTR_* would work
> >>>> but only if we place the attribute directly under /sys/bus/pci/new_attr.
> >>>
> >>> Then you are doing something wrong :)
> >>
> >> That is very possible.
> >>
> >>>
> >>> Where _exactly_ are you wanting to put this attribute?
> >>
> >> I'm trying for /sys/bus/pci/zpci/uid_checking, I'm using
> >> the below code and the attribute even shows up but reading
> >> it gives me two 0 bytes only.
> >> The relevant code is only a slight alteration of the original patch
> >> as follows:
> >>
> >> static ssize_t uid_checking_show(struct bus_type *bus, char *buf)
> >> {
> >> return sprintf(buf, "%i\n", zpci_unique_uid);
> >> }
> >> static BUS_ATTR_RO(uid_checking);
> >>
> >> static struct kset *zpci_global_kset;
> >>
> >> static struct attribute *zpci_attrs_global[] = {
> >> &bus_attr_uid_checking.attr,
> >> NULL,
> >> };
> >>
> >> static struct attribute_group zpci_attr_group_global = {
> >> .attrs = zpci_attrs_global,
> >> };
> >
> > Name your attribute group, and then you do not have to mess with a
> > "raw" kobject like you are below:
>
> Thanks for this tip and sorry for bothering you again.
>
> >
> >>
> >> int __init zpci_sysfs_init(void)
> >> {
> >> struct kset *pci_bus_kset;
> >>
> >> pci_bus_kset = bus_get_kset(&pci_bus_type);
> >>
> >> zpci_global_kset = kset_create_and_add("zpci", NULL, &pci_bus_kset->kobj);
> >
> > No, do not mess with at kset, just set the default attribute group for
> > the bus to the above, and you should be fine.
>
> Oh ok, I got this idea from the code adding /sys/bus/pci/slots/ in
> drivers/pci/slot.c:pci_slot_init(). See below maybe we can clean that up too.
Slots are "interesting" and that code is really old, we know how to do
things better now :)
But I doubt we should change anything there, as it does work, and
userspace is used to how they come/go.
> >> if (!zpci_global_kset)
> >> return -ENOMEM;
> >>
> >> return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global);
> >
> > Huge hint, if in a driver, or bus subsystem, and you call sysfs_*,
> > that's usually a huge clue that you are doing something wrong.
> >
> > Try the above again, with a simple attribute group, and name for it, and
> > it should "just work".
>
> I'm probably missing something but I don't get how this could work in
> this case. If I'm seeing this right the default attribute group here
> is pci_bus_type.bus_groups and that is already set in drivers/pci/pci-driver.c
> so I don't think I should set that.
Yes, add your group to that list of groups and all should be good.
> I did however find bus_create_file() which does work when using the path
> /sys/bus/pci/uid_checking instead. This would work for us if Bjorn is okay with
> that path and the code is really clean and simple too.
No, use the above group you already have please.
> That said, I think we could also add something like bus_create_group().
No, use the group list as show above please.
> Then we could use that to also clean up drivers/pci/slot.c:pci_slot_init()
> and get the original path /sys/bus/pci/zpci/uid_checking.
What needs to be cleaned up there?
> I think this would also allow us to get rid of pci_bus_get_kset() which is
> only used in that function and seems to me like it encourages use of raw ksets.
> Or I'm completely off the mark and just missing something important.
Cleaning up slots is great, but they are "odd", so be careful.
thanks,
greg k-h
