>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@xxxxxxx>"
>> #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
