invalid drv data in show attribute

invalid drv data in show attribute

[Czerwacki, Eial]

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:
#include <linux/string.h>
#include <linux/module.h>
#include <linux/pci.h>

/* modules info */
#define DEVICE_NAME "vSMP"
#define DRIVER_LICENSE "GPL v2"
#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
#define DRIVER_DESC "vSMP hypervisor driver"
#define DRIVER_VERSION "0.1"

#define PCI_DEVICE_ID_SAP_FLX_VSMP_CTL 0x1011

#define ROOT_SYFS_FOLDER "vsmp"
#define DEVICE_ATTR_NAME(_name_) &dev_attr_##_name_ .attr

#ifndef sysfs_emit
#define sysfs_emit sprintf
#endif // sysfs_emit

MODULE_LICENSE(DRIVER_LICENSE);
MODULE_AUTHOR(DRIVER_AUTHOR);
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_VERSION(DRIVER_VERSION);

#define MAX_LABEL_LEN 81

struct data {
	char version[MAX_LABEL_LEN];
	struct pci_dev *pdev;
	struct kobject *kobj;
};

static ssize_t version_show(struct device *dev, struct device_attribute *attr,
			    char *buf)
{
	struct data *d = dev_get_drvdata(dev);

 	return sysfs_emit(buf, "%s\n", d->version);
}

DEVICE_ATTR_RO(version);

static int populate_version(struct data *d)
{
	int ret_val;

	snprintf(d->version, sizeof(d->version) - 1,
		 "%u.%u.%u.%u", 1,2,3,4);

	ret_val = sysfs_add_file_to_group(d->kobj, DEVICE_ATTR_NAME(version), NULL);
	if (ret_val) {
		dev_err(&d->pdev->dev, "Failed to create version entry, error (%d)\n",
			ret_val);
		return -EINVAL;
	}
	dev_info(&d->pdev->dev, "version is %s\n", d->version);

	return ret_val;
}

static const struct pci_device_id vsmp_pci_table[] = {
	{ PCI_VDEVICE(SCALEMP, PCI_DEVICE_ID_SAP_FLX_VSMP_CTL), 0, },
	{ 0, }, /* terminate list */
};

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);
	if (!d->kobj) {
		kfree(d);
		dev_err(&d->pdev->dev, "Failed to create vSMP sysfs entry.\n");

		return -EINVAL;
	}

	ret_val = sysfs_create_link(hypervisor_kobj, d->kobj, ROOT_SYFS_FOLDER);
	if (ret_val) {
		kobject_del(d->kobj);
		kfree(d);
		dev_err(&d->pdev->dev, "Failed to link obj to hypervisor, error (%d)\n", ret_val);

		return ret_val;
	}

	populate_version(d);

	dev_info(&d->pdev->dev, "Driver up and running\n");
	return ret_val;
}

static void vsmp_pci_remove(struct pci_dev *pci)
{
	struct data *d = pci_get_drvdata(pci);

	kobject_del(d->kobj);
	sysfs_remove_link(hypervisor_kobj, ROOT_SYFS_FOLDER);
	kfree(d);
}

static struct pci_driver vsmp_pci_driver = {
	.name		= DEVICE_NAME,
	.id_table	= vsmp_pci_table,
	.probe		= vsmp_pci_probe,
	.remove		= vsmp_pci_remove,
};

module_pci_driver(vsmp_pci_driver);

it compiles well (the kernel I use doesn't have sysfs_emit so I had to improvise) and loads well but when I cat the version file, I don't get valid data. even with printks.
printing the addresses of d from the init and the d inside the show attr func returns different addresses.
can anyone help me understand what I'm doing wrong?

Thanks,

Eial

Re: invalid drv data in show attribute

[Greg KH]

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

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

thanks,

greg k-h

Re: invalid drv data in show attribute

[Czerwacki, Eial]

> 
>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.
I'll move to newer kernel.
I'll worry about backports later

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

that I'd like to see under /sys/hypervisor
I have no problems using link from the device's path to /sys/hypervisor

Thanks,

Eial

Re: invalid drv data in show attribute

[Greg KH]

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()?

> I'll move to newer kernel.
> I'll worry about backports later

backports are always easier than forward-ports.

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

And what exactly is in the other files?

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

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.

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?

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

thanks,

greg k-h

Re: invalid drv data in show attribute

[Czerwacki, Eial]

>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

Re: invalid drv data in show attribute

[Greg KH]

On Mon, Sep 05, 2022 at 07:28:02AM +0000, Czerwacki, Eial wrote:
> >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

Ick, never work on an enterprise kernel for new stuff, they are crazy
and you will need to usually redo your whole thing for upstream in the
end.

Just work off of the latest tree please and then backport when needed.

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

Which can be a device, we have loads of virtual devices, it's how the
driver model works.

> each board has its own data I'd like to export.

Then they should be treated as a device.

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

Ok, those can be simple attribute groups, if they are static and do not
change.

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

Then you need to have a bus, or a class, as that is how you orginize
things.  Please read the driver model chapter in the Linux Device
Drivers book as an example of the basic ideas here.

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

Great, make them devices.

> 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

Ok, then they all need to be "one value per file" like sysfs requires.

good luck!

greg k-h

Re: invalid drv data in show attribute

[Czerwacki, Eial]

>On Mon, Sep 05, 2022 at 07:28:02AM +0000, Czerwacki, Eial wrote:
>> >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
>
>Ick, never work on an enterprise kernel for new stuff, they are crazy
>and you will need to usually redo your whole thing for upstream in the
>end.
>
>Just work off of the latest tree please and then backport when needed.
understood, that is the main focus now

>
>> >> >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.
>
>Which can be a device, we have loads of virtual devices, it's how the
>driver model works.
>
>> each board has its own data I'd like to export.
>
>Then they should be treated as a device.
not sure I understand how they are connected, is there an example I can look at?

>
>> >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
>
>Ok, those can be simple attribute groups, if they are static and do not
>change.
that's correct, all the information in the initial stage (system or board-wise) is static and per boot.

>
>> >> 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
>
>Then you need to have a bus, or a class, as that is how you orginize
>things.  Please read the driver model chapter in the Linux Device
>Drivers book as an example of the basic ideas here.
I'll look it up

>
>> >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.
>
>Great, make them devices.
I still need to understand how to make them as devices.
if I understood you correctly, your suggestion is to run modprobe vsmp and there will be virtual device per board?
then for each I'll create the relevant sysfs entries?
if so, where can I find the global sysfs entries?
also, how can organize the entries so they will appear under one location which is static?
I'd prefer not adding complex code to the utils which need to traverse the sysfs tree to collect the right data

>
>> 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
>
>Ok, then they all need to be "one value per file" like sysfs requires.
the current implementation does that exactly. each file represents one value either numerical or string

Eial

Re: invalid drv data in show attribute

[Greg KH]

On Mon, Sep 05, 2022 at 12:02:47PM +0000, Czerwacki, Eial wrote:
> >On Mon, Sep 05, 2022 at 07:28:02AM +0000, Czerwacki, Eial wrote:
> >> >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
> >
> >Ick, never work on an enterprise kernel for new stuff, they are crazy
> >and you will need to usually redo your whole thing for upstream in the
> >end.
> >
> >Just work off of the latest tree please and then backport when needed.
> understood, that is the main focus now
> 
> >
> >> >> >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.
> >
> >Which can be a device, we have loads of virtual devices, it's how the
> >driver model works.
> >
> >> each board has its own data I'd like to export.
> >
> >Then they should be treated as a device.
> not sure I understand how they are connected, is there an example I can look at?

The kernel has loads of busses and classes as real-world examples :)

Think of it this way, each of your board will have a `struct device` in
it, and then you will add them to the system.  As you don't really have
a complex thing, each board will just be bound to the "same" driver, and
your bus will only have one driver, but that will allow them all to be
enumerated and handled easily.

And they will all hang off of the PCI device that you had here, it's up
to you if you want to make a "bus device" in the middle or not like many
busses do, but that's getting ahead a bit I think.

Go read the documentation and try it out, there's a lot of concepts here
that I think you need to learn up on in order to make this work well.
Sorry it's not the simplest thing, but then again, it is an operating
system :)

And aren't there others at your company that can help you with this?

> >Great, make them devices.
> I still need to understand how to make them as devices.
> if I understood you correctly, your suggestion is to run modprobe vsmp and there will be virtual device per board?
> then for each I'll create the relevant sysfs entries?
> if so, where can I find the global sysfs entries?

What do you mean by "global" here?  Just put those below /sys/hypervisor
if you want, that should be simple, but not for the individual devices.

> also, how can organize the entries so they will appear under one location which is static?

The kernel device tree is never static.

> I'd prefer not adding complex code to the utils which need to traverse the sysfs tree to collect the right data

for device in $(ls /sys/bus/vsmp/device/); do

that's not that hard :)

thanks,

greg k-h

Re: invalid drv data in show attribute

[Czerwacki, Eial]

>On Mon, Sep 05, 2022 at 12:02:47PM +0000, Czerwacki, Eial wrote:
>> >On Mon, Sep 05, 2022 at 07:28:02AM +0000, Czerwacki, Eial wrote:
>> >> >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
>> >
>> >Ick, never work on an enterprise kernel for new stuff, they are crazy
>> >and you will need to usually redo your whole thing for upstream in the
>> >end.
>> >
>> >Just work off of the latest tree please and then backport when needed.
>> understood, that is the main focus now
>> 
>> >
>> >> >> >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.
>> >
>> >Which can be a device, we have loads of virtual devices, it's how the
>> >driver model works.
>> >
>> >> each board has its own data I'd like to export.
>> >
>> >Then they should be treated as a device.
>> not sure I understand how they are connected, is there an example I can look at?
>
>The kernel has loads of busses and classes as real-world examples :)
>
>Think of it this way, each of your board will have a `struct device` in
>it, and then you will add them to the system.  As you don't really have
>a complex thing, each board will just be bound to the "same" driver, and
>your bus will only have one driver, but that will allow them all to be
>enumerated and handled easily.
>
>And they will all hang off of the PCI device that you had here, it's up
>to you if you want to make a "bus device" in the middle or not like many
>busses do, but that's getting ahead a bit I think.
>
>Go read the documentation and try it out, there's a lot of concepts here
>that I think you need to learn up on in order to make this work well.
>Sorry it's not the simplest thing, but then again, it is an operating
>system :)
I understand what you are saying, I'll try to search the code for it.

>
>And aren't there others at your company that can help you with this?
afaik, there is not expert on kernel in the company.
there are some workers that have experience in kernel development but they are from our team

>
>> >Great, make them devices.
>> I still need to understand how to make them as devices.
>> if I understood you correctly, your suggestion is to run modprobe vsmp and there will be virtual device per board?
>> then for each I'll create the relevant sysfs entries?
>> if so, where can I find the global sysfs entries?
>
>What do you mean by "global" here?  Just put those below /sys/hypervisor
>if you want, that should be simple, but not for the individual devices.
>
>> also, how can organize the entries so they will appear under one location which is static?
>
>The kernel device tree is never static.
>
>> I'd prefer not adding complex code to the utils which need to traverse the sysfs tree to collect the right data
>
>for device in $(ls /sys/bus/vsmp/device/); do
now I understand!
that is what I was missing.
so inside /sys/bus/vsmp/device/ I can find the following tree?
1
2
3
...
n
?

so I can create entries in /sys/hypervisor/vsmp for the global info and link /sys/bus/vsmp/device/ to /sys/hypervisor/vsmp/boards?

>
>that's not that hard :)
>
>thanks,
>
>greg k-h

Thanks,

Eial

Re: invalid drv data in show attribute

[Greg KH]

On Mon, Sep 05, 2022 at 12:56:29PM +0000, Czerwacki, Eial wrote:
> >And aren't there others at your company that can help you with this?
> afaik, there is not expert on kernel in the company.
> there are some workers that have experience in kernel development but they are from our team

SAP is a big company, I think you should seek them out if you are
wanting to add new subsystems to the kernel to get their help as doing
this is not a trivial task.

> >> I'd prefer not adding complex code to the utils which need to traverse the sysfs tree to collect the right data
> >
> >for device in $(ls /sys/bus/vsmp/device/); do
> now I understand!
> that is what I was missing.
> so inside /sys/bus/vsmp/device/ I can find the following tree?
> 1
> 2
> 3
> ...
> n
> ?

Yes, or how ever you want to name these "boards".

> so I can create entries in /sys/hypervisor/vsmp for the global info

Yes.

> and link /sys/bus/vsmp/device/ to /sys/hypervisor/vsmp/boards?

No symlink needed at all, they are independent things, right?  What
would require such a symlink?

thanks,

greg k-h

Re: invalid drv data in show attribute

[Czerwacki, Eial]

>On Mon, Sep 05, 2022 at 12:56:29PM +0000, Czerwacki, Eial wrote:
>> >And aren't there others at your company that can help you with this?
>> afaik, there is not expert on kernel in the company.
>> there are some workers that have experience in kernel development but they are from our team
>
>SAP is a big company, I think you should seek them out if you are
>wanting to add new subsystems to the kernel to get their help as doing
>this is not a trivial task.
I'll try to reach out however I'm not sure there is any.

>
>> >> I'd prefer not adding complex code to the utils which need to traverse the sysfs tree to collect the right data
>> >
>> >for device in $(ls /sys/bus/vsmp/device/); do
>> now I understand!
>> that is what I was missing.
>> so inside /sys/bus/vsmp/device/ I can find the following tree?
>> 1
>> 2
>> 3
>> ...
>> n
>> ?
>
>Yes, or how ever you want to name these "boards".
/sys/bus/vsmp/boards/ or /sys/bus/vsmp/device/board#?

>
>> so I can create entries in /sys/hypervisor/vsmp for the global info
>
>Yes.
>
>> and link /sys/bus/vsmp/device/ to /sys/hypervisor/vsmp/boards?
>
>No symlink needed at all, they are independent things, right?  What
>would require such a symlink?
>
>thanks,
>
>greg k-h
I'd rather have all the info under one place than in multiple places if possible.
is seams more logical to me

Eial

Re: invalid drv data in show attribute

[Greg KH]

On Mon, Sep 05, 2022 at 03:52:01PM +0000, Czerwacki, Eial wrote:
> >On Mon, Sep 05, 2022 at 12:56:29PM +0000, Czerwacki, Eial wrote:
> >> >> I'd prefer not adding complex code to the utils which need to traverse the sysfs tree to collect the right data
> >> >
> >> >for device in $(ls /sys/bus/vsmp/device/); do
> >> now I understand!
> >> that is what I was missing.
> >> so inside /sys/bus/vsmp/device/ I can find the following tree?
> >> 1
> >> 2
> >> 3
> >> ...
> >> n
> >> ?
> >
> >Yes, or how ever you want to name these "boards".
> /sys/bus/vsmp/boards/ or /sys/bus/vsmp/device/board#?

You can not create /sys/bus/vsmp/boards/ the driver model will not let
you do that.

You will have the device/ directory created automatically.  It's how you
want to name the board itself is up to you, it just has to be unique on
the bus.

> >> so I can create entries in /sys/hypervisor/vsmp for the global info
> >
> >Yes.
> >
> >> and link /sys/bus/vsmp/device/ to /sys/hypervisor/vsmp/boards?
> >
> >No symlink needed at all, they are independent things, right?  What
> >would require such a symlink?
> >
> >thanks,
> >
> >greg k-h
> I'd rather have all the info under one place than in multiple places if possible.
> is seams more logical to me

That's not how sysfs works, sorry.  :)

Hypervisors are not normal, the fact that you are using that location in
the first place is very very odd, only 1 other in-kernel user is using
that.  So maybe you don't even need that at all?

thanks,

greg k-h

Re: invalid drv data in show attribute

[Czerwacki, Eial]

>On Mon, Sep 05, 2022 at 03:52:01PM +0000, Czerwacki, Eial wrote:
>> >On Mon, Sep 05, 2022 at 12:56:29PM +0000, Czerwacki, Eial wrote:
>> >> >> I'd prefer not adding complex code to the utils which need to traverse the sysfs tree to collect the right data
>> >> >
>> >> >for device in $(ls /sys/bus/vsmp/device/); do
>> >> now I understand!
>> >> that is what I was missing.
>> >> so inside /sys/bus/vsmp/device/ I can find the following tree?
>> >> 1
>> >> 2
>> >> 3
>> >> ...
>> >> n
>> >> ?
>> >
>> >Yes, or how ever you want to name these "boards".
>> /sys/bus/vsmp/boards/ or /sys/bus/vsmp/device/board#?
>
>You can not create /sys/bus/vsmp/boards/ the driver model will not let
>you do that.
>
>You will have the device/ directory created automatically.  It's how you
>want to name the board itself is up to you, it just has to be unique on
>the bus.
I understand, I think /sys/bus/vsmp/device/board# is adequate for our needs.

>
>> >> so I can create entries in /sys/hypervisor/vsmp for the global info
>> >
>> >Yes.
>> >
>> >> and link /sys/bus/vsmp/device/ to /sys/hypervisor/vsmp/boards?
>> >
>> >No symlink needed at all, they are independent things, right?  What
>> >would require such a symlink?
>> >
>> >thanks,
>> >
>> >greg k-h
>> I'd rather have all the info under one place than in multiple places if possible.
>> is seams more logical to me
>
>That's not how sysfs works, sorry.  :)
>
>Hypervisors are not normal, the fact that you are using that location in
>the first place is very very odd, only 1 other in-kernel user is using
>that.  So maybe you don't even need that at all?
>
>thanks,
>
>greg k-h
as vSMP is an hypervisor, I've thought /sys/hypervisor is an appropriate location.
if you think of another location is more suitable, I'm open to suggestions

Thanks,

Eial

Re: invalid drv data in show attribute

[Greg KH]

On Tue, Sep 06, 2022 at 07:30:20AM +0000, Czerwacki, Eial wrote:
> >> >> so I can create entries in /sys/hypervisor/vsmp for the global info
> >> >
> >> >Yes.
> >> >
> >> >> and link /sys/bus/vsmp/device/ to /sys/hypervisor/vsmp/boards?
> >> >
> >> >No symlink needed at all, they are independent things, right?  What
> >> >would require such a symlink?
> >> >
> >> >thanks,
> >> >
> >> >greg k-h
> >> I'd rather have all the info under one place than in multiple places if possible.
> >> is seams more logical to me
> >
> >That's not how sysfs works, sorry.  :)
> >
> >Hypervisors are not normal, the fact that you are using that location in
> >the first place is very very odd, only 1 other in-kernel user is using
> >that.  So maybe you don't even need that at all?
> >
> >thanks,
> >
> >greg k-h
> as vSMP is an hypervisor, I've thought /sys/hypervisor is an appropriate location.

For generic stuff related to your hypervisor, yes.  But for devices that
are part of your hypervisor bus, no, they do not belong there.

thanks,

greg k-h

Re: invalid drv data in show attribute

[Czerwacki, Eial]

>On Tue, Sep 06, 2022 at 07:30:20AM +0000, Czerwacki, Eial wrote:
>> >> >> so I can create entries in /sys/hypervisor/vsmp for the global info
>> >> >
>> >> >Yes.
>> >> >
>> >> >> and link /sys/bus/vsmp/device/ to /sys/hypervisor/vsmp/boards?
>> >> >
>> >> >No symlink needed at all, they are independent things, right?  What
>> >> >would require such a symlink?
>> >> >
>> >> >thanks,
>> >> >
>> >> >greg k-h
>> >> I'd rather have all the info under one place than in multiple places if possible.
>> >> is seams more logical to me
>> >
>> >That's not how sysfs works, sorry.  :)
>> >
>> >Hypervisors are not normal, the fact that you are using that location in
>> >the first place is very very odd, only 1 other in-kernel user is using
>> >that.  So maybe you don't even need that at all?
>> >
>> >thanks,
>> >
>> >greg k-h
>> as vSMP is an hypervisor, I've thought /sys/hypervisor is an appropriate location.
>
>For generic stuff related to your hypervisor, yes.  But for devices that
>are part of your hypervisor bus, no, they do not belong there.
>
>thanks,
>
>greg k-h

I understand, thanks for the explanation

Eial

Re: invalid drv data in show attribute

[Greg KH]

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:

Other minor things now that my coffee has kicked in:

> /* modules info */
> #define DEVICE_NAME "vSMP"
> #define DRIVER_LICENSE "GPL v2"
> #define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
> #define DRIVER_DESC "vSMP hypervisor driver"

For things that you only ever use once, no need for a #define.

> #define DRIVER_VERSION "0.1"

drivers never have versions once they are merged into the kernel tree,
just drop it.

> 
> #define PCI_DEVICE_ID_SAP_FLX_VSMP_CTL 0x1011
> 
> #define ROOT_SYFS_FOLDER "vsmp"
> #define DEVICE_ATTR_NAME(_name_) &dev_attr_##_name_ .attr

That's very very odd, don't do that.  Spell it out so that it makes
sense what is happening, otherwise this looks like a driver-core #define
that I had never known was there.  Turns out it wasn't there :)

> #ifndef sysfs_emit
> #define sysfs_emit sprintf
> #endif // sysfs_emit
> 
> MODULE_LICENSE(DRIVER_LICENSE);
> MODULE_AUTHOR(DRIVER_AUTHOR);
> MODULE_DESCRIPTION(DRIVER_DESC);
> MODULE_VERSION(DRIVER_VERSION);
> 
> #define MAX_LABEL_LEN 81
> 
> struct data {
> 	char version[MAX_LABEL_LEN];
> 	struct pci_dev *pdev;
> 	struct kobject *kobj;

Think about which structure is going to own this data.  You have
pointers to 2 reference counted objects, yet no reference count on this
object.  So who controls the lifespan of it and how?

> };
> 
> static ssize_t version_show(struct device *dev, struct device_attribute *attr,
> 			    char *buf)
> {
> 	struct data *d = dev_get_drvdata(dev);
> 
>  	return sysfs_emit(buf, "%s\n", d->version);
> }
> 
> DEVICE_ATTR_RO(version);
> 
> static int populate_version(struct data *d)
> {
> 	int ret_val;
> 
> 	snprintf(d->version, sizeof(d->version) - 1,
> 		 "%u.%u.%u.%u", 1,2,3,4);
> 
> 	ret_val = sysfs_add_file_to_group(d->kobj, DEVICE_ATTR_NAME(version), NULL);

Just call sysfs_create_file(), don't add a file to a group like this.

If you want a named group, then statically create it ahead of time with
a name, which makes this all much simpler.

In fact, never call any "create_file()" function, use lists of attribute
groups as the driver core handles them automatically for you, no need to
manually add or remove or anything else.  Much simpler and easier for a
driver writer to use instead of having to have the same error handling
logic everywhere.

> 	if (ret_val) {
> 		dev_err(&d->pdev->dev, "Failed to create version entry, error (%d)\n",
> 			ret_val);
> 		return -EINVAL;
> 	}
> 	dev_info(&d->pdev->dev, "version is %s\n", d->version);
> 
> 	return ret_val;
> }
> 
> static const struct pci_device_id vsmp_pci_table[] = {
> 	{ PCI_VDEVICE(SCALEMP, PCI_DEVICE_ID_SAP_FLX_VSMP_CTL), 0, },
> 	{ 0, }, /* terminate list */
> };
> 
> 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);

That's a crazy pointer chain there.

Never add a "raw" kobject to a struct device as you then just broke the
tree and userspace will never know it is there so userspace tools and
libraries can't see it unless they manually walk the sysfs tree (and you
really don't want them doing that.)  With what you did here, this object
is now invisible to libudev so no library will see it :(


> 	if (!d->kobj) {
> 		kfree(d);
> 		dev_err(&d->pdev->dev, "Failed to create vSMP sysfs entry.\n");
> 
> 		return -EINVAL;
> 	}
> 
> 	ret_val = sysfs_create_link(hypervisor_kobj, d->kobj, ROOT_SYFS_FOLDER);

Again, no need to ever create a link here.  symlinks are usually for
pointing to attributes of something (driver, device, etc.) or for when
we have messed up in the past and had to move an object somewhere else
and keep userspace from breaking by adding a link so that tools continue
to work properly.

Don't create links to start with for no good reason.  And if you do,
document the heck out of it so we understand why.

> 	if (ret_val) {
> 		kobject_del(d->kobj);
> 		kfree(d);
> 		dev_err(&d->pdev->dev, "Failed to link obj to hypervisor, error (%d)\n", ret_val);
> 
> 		return ret_val;
> 	}
> 
> 	populate_version(d);

No error handling :)

thanks,

greg k-h

Re: invalid drv data in show attribute

[Czerwacki, Eial]

>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:
>
>Other minor things now that my coffee has kicked in:
>
>> /* modules info */
>> #define DEVICE_NAME "vSMP"
>> #define DRIVER_LICENSE "GPL v2"
>> #define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
>> #define DRIVER_DESC "vSMP hypervisor driver"
>
>For things that you only ever use once, no need for a #define.
sure

>
>> #define DRIVER_VERSION "0.1"
>
>drivers never have versions once they are merged into the kernel tree,
>just drop it.
acked

>
>>
>> #define PCI_DEVICE_ID_SAP_FLX_VSMP_CTL 0x1011
>>
>> #define ROOT_SYFS_FOLDER "vsmp"
>> #define DEVICE_ATTR_NAME(_name_) &dev_attr_##_name_ .attr
>
>That's very very odd, don't do that.  Spell it out so that it makes
>sense what is happening, otherwise this looks like a driver-core #define
>that I had never known was there.  Turns out it wasn't there :)
will do

>
>> #ifndef sysfs_emit
>> #define sysfs_emit sprintf
>> #endif // sysfs_emit
>>
>> MODULE_LICENSE(DRIVER_LICENSE);
>> MODULE_AUTHOR(DRIVER_AUTHOR);
>> MODULE_DESCRIPTION(DRIVER_DESC);
>> MODULE_VERSION(DRIVER_VERSION);
>>
>> #define MAX_LABEL_LEN 81
>>
>> struct data {
>>       char version[MAX_LABEL_LEN];
>>       struct pci_dev *pdev;
>>       struct kobject *kobj;
>
>Think about which structure is going to own this data.  You have
>pointers to 2 reference counted objects, yet no reference count on this
>object.  So who controls the lifespan of it and how?
>
the kobj ptr is just for the link I've mentioned in the other mail.
as for pdev, I assume that I can get the bars somehow from the device struct right?

>> };
>>
>> static ssize_t version_show(struct device *dev, struct device_attribute *attr,
>>                           char *buf)
>> {
>>       struct data *d = dev_get_drvdata(dev);
>>
>>       return sysfs_emit(buf, "%s\n", d->version);
>> }
>>
>> DEVICE_ATTR_RO(version);
>>
>> static int populate_version(struct data *d)
>> {
>>       int ret_val;
>>
>>       snprintf(d->version, sizeof(d->version) - 1,
>>                "%u.%u.%u.%u", 1,2,3,4);
>>
>>       ret_val = sysfs_add_file_to_group(d->kobj, DEVICE_ATTR_NAME(version), NULL);
>
>Just call sysfs_create_file(), don't add a file to a group like this.
>
>If you want a named group, then statically create it ahead of time with
>a name, which makes this all much simpler.
>
>In fact, never call any "create_file()" function, use lists of attribute
>groups as the driver core handles them automatically for you, no need to
>manually add or remove or anything else.  Much simpler and easier for a
>driver writer to use instead of having to have the same error handling
>logic everywhere.
you are correct, I don't know why I decided to treat the parent folder as group.
will use that, thanks

>
>>       if (ret_val) {
>>               dev_err(&d->pdev->dev, "Failed to create version entry, error (%d)\n",
>>                       ret_val);
>>               return -EINVAL;
>>       }
>>       dev_info(&d->pdev->dev, "version is %s\n", d->version);
>>
>>       return ret_val;
>> }
>>
>> static const struct pci_device_id vsmp_pci_table[] = {
>>       { PCI_VDEVICE(SCALEMP, PCI_DEVICE_ID_SAP_FLX_VSMP_CTL), 0, },
>>       { 0, }, /* terminate list */
>> };
>>
>> 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);
>
>That's a crazy pointer chain there.
>
>Never add a "raw" kobject to a struct device as you then just broke the
>tree and userspace will never know it is there so userspace tools and
>libraries can't see it unless they manually walk the sysfs tree (and you
>really don't want them doing that.)  With what you did here, this object
>is now invisible to libudev so no library will see it :(
that is the root of my issue, right?

>
>
>>       if (!d->kobj) {
>>               kfree(d);
>>               dev_err(&d->pdev->dev, "Failed to create vSMP sysfs entry.\n");
>>
>>               return -EINVAL;
>>       }
>>
>>       ret_val = sysfs_create_link(hypervisor_kobj, d->kobj, ROOT_SYFS_FOLDER);
>
>Again, no need to ever create a link here.  symlinks are usually for
>pointing to attributes of something (driver, device, etc.) or for when
>we have messed up in the past and had to move an object somewhere else
>and keep userspace from breaking by adding a link so that tools continue
>to work properly.
>
>Don't create links to start with for no good reason.  And if you do,
>document the heck out of it so we understand why.
I understand, as said in the other mail, the link is for easy access to the data like
block devices can be found under /sys/block

>
>>       if (ret_val) {
>>               kobject_del(d->kobj);
>>               kfree(d);
>>               dev_err(&d->pdev->dev, "Failed to link obj to hypervisor, error (%d)\n", ret_val);
>>
>>               return ret_val;
>>       }
>>
>>       populate_version(d);
>
>No error handling :)
right, thanks!

Eial