>On Mon, Sep 05, 2022 at 06:07:07AM +0000, Czerwacki, Eial wrote:
>> >
>> >On Sun, Sep 04, 2022 at 02:37:32PM +0000, Czerwacki, Eial wrote:
>> >> Greetings,
>> >>
>> >> while working on a driver, I've found a bug that I'm unable to understand.
>> >> I assume that I'm doing something wrong. here is my reduced c file:
>> >
>> ><snip>
>> >
>> >I'll provide a better review after my coffee, but just one comment
>> >first. Ok, two:
>> >
>> >> #ifndef sysfs_emit
>> >> #define sysfs_emit sprintf
>> >> #endif // sysfs_emit
>> >
>> >Wait what? You mention at the end that you do nto have sysfs_emit in
>> >your kernel tree, but all activly maintained kernels does have this
>> >function. You should NEVER be working on a kernel tree that is not
>> >actually supported, and for new code like you are wanting to submit, you
>> >should always work on Linus's tree, or the last release, or something
>> >newer.
>> >
>> >Please move to 5.19 now, it will save you so much time later on...
>> well, I'm kinda binded to this kernel version buy you are right.
>
>What kernel version does not have sysfs_emit()?
SELS 15 SP2, it uses kernel 5.3.18-24.67
>
>> I'll move to newer kernel.
>> I'll worry about backports later
>
>backports are always easier than forward-ports.
sounds logical
>
>> >> static int vsmp_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
>> >> {
>> >> struct data *d;
>> >> int ret_val = 0;
>> >>
>> >> d = devm_kzalloc(&pci->dev, sizeof(*d), GFP_KERNEL);
>> >> if (IS_ERR(d))
>> >> return PTR_ERR(d);
>> >>
>> >> d->pdev = pci;
>> >> pci_set_drvdata(pci, d);
>> >> d->kobj = kobject_create_and_add(ROOT_SYFS_FOLDER, &d->pdev->dev.kobj);
>> >
>> >This is the issue, no driver should ever be calling any kobject_*() or
>> >sysfs_*() calls, unless something went very very wrong.
>> >
>> >Drivers and devices have their own tree in sysfs, with 'struct device'
>> >to use. You have a device here, use that, don't try to create a whole
>> >new sysfs tree somewhere else in the heiarchy which does not reflect the
>> >actual device you are using here.
>> >
>> >You then try to tie a device as a child to a kobject, which breaks the
>> >whole logical chain here, and then confuses your callback as you really
>> >don't have the device pointer anymore, it's some other random thing.
>> >
>> >So step back and try to describe first what you want to see in sysfs,
>> >and then maybe it will be more obvious as to what you need to do here.
>> >
>> >Write the Documentation/ABI/ entries first, what do they look like for
>> >your new sysfs files?
>>
>> I thought that is my issue but wasn't sure.
>> what I'm looking for is this tree:
>> - vsmp
>> -- version
>> -- summery
>> --- data#1
>> ...
>> --- data#n
>> -- boards
>> --- 0
>> ---- data#1
>> ...
>> ---- data#k
>> --- 1
>> ...
>> --- l
>>
>> each board has a predefine set of attributes when I need to add another depending on the type.
>> also there are shared attributes between summery folder and the boards. that I was able to implement based on the name of the entry
>
>So "boards" are devices, and then you need a bus to manage them. Are
>you sure you need/want all of this?
no, boards are not devices, they logical partitions inside the hypervisor.
each board has its own data I'd like to export.
>
>And what exactly is in the other files?
version is the hypervisor's version, the summery is data the user can extract
in order to build a complete image of the system configuration
>
>> that I'd like to see under /sys/hypervisor
>
>/sys/hypervisor probably isn't the place for all of that mess. You are
>going to have devices and a bus and all sorts of other complex things.
from what I understand, when I add a sysfs tree to a device, it exists in side the device's full path.
as the device can sit on different slots, it will have different path.
to enable simpler data extraction, I'd like place it under a constant path, like every block device
can be accessed via /sys/block
in later stages of the driver, more information will be exported and maybe data will be imported into the device using
the driver
>
>Step back and explain exactly what you are trying to export, who will be
>using it, and what format you want it in. This feels like a lot of
>stuff that is probably not needed except for debugging.
each instance of vSMP is comprised of logical partitions.
the instance has it's own data required to be exported.
each logical partition has it's own data required to be exported.
that is why the above tree was suggested.
the data will be used by a layer that gather the information and exports it to ui and to scripts that log the info
it isn't a debug info because there are some tools that need to know the a partition's information to run properly
>
>If this isn't for debugging, it's going to be a lot more complex than
>"simple" attributes for a driver, sorry. You are going to need as I
>mention above, to use a bus and other things which feels kind of wrong
>as odds are this isn't a bus, right?
that is correct, see above explanation
>
>> I have no problems using link from the device's path to /sys/hypervisor
>
>Why would you create a link? Look at what existing hypervisors do in
>/sys/hypervisor, there's no links there, only a small set of information
>that userspace can use for the Xen code, or a more complex virtual
>filesystem that the s390 hypervisor mounts at that location. No one
>creates a symlink under there, why would that be necessary?
>
>But again, stop and think about the information you want to export and
>who would be using it. If you can explain that first, that might make
>it easier to point out how to do it best.
>
I hope I've explained myself properly,
Thanks,
Eial
