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