Re: [PATCH] staging: most: dim2: fix device registration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Commit 723de0f9171e ("staging: most: remove device from interface
structure") moved registration of driver-provided struct device to
the most subsystem, but did not properly update dim2 driver to
work with that change.

After most subsystem passes driver's dev to register_device(), it
becomes refcounted, and can be only deallocated in the release method.
Provide that by:
- not using devres to allocate the device,
- moving shutdown actions from _remove() to the device release method,
- not calling shutdown actions in _probe() after the device becomes
   refcounted.

Should this be 3 patches?

But these three items are deeply interconnected, and fix the issue together. Must not manually free device structure passed to register_device(), thus must not allocate via devres (because otherwise, devres will free it). Once not using devres for it, must deallocate it somehow else, thus must rework the release paths.

Perhaps I just shall not go into these details in the commit message.

Also, driver used to register it's dev itself, to provide a custom
attribute. With the modified most subsystem, this causes duplicate
registration of the same device object. Fix that by adding that custom
attribute to the platform device - that is a better location for
a platform-specific attribute anyway.

Nope, it should be 4 patches.

Unlike the above three, this item could be separated.
Will split into two patches now - the first for this (and with fix to the attributes issue noted below) and the second for proper device releasing.

Also, why have you not cc:ed the original author of the commit you are
"fixing" here?   They are the maintainer of this code, right?

I was under impression that "git send-email" does that automatically...

CCing them now.

One note on your change that would keep me from accepting it even if all
of the above was not an issue:

diff --git a/drivers/staging/most/dim2/sysfs.c b/drivers/staging/most/dim2/sysfs.c
index c85b2cdcdca3..22836c8ed554 100644
--- a/drivers/staging/most/dim2/sysfs.c
+++ b/drivers/staging/most/dim2/sysfs.c
@@ -39,11 +39,10 @@ static const struct attribute_group *dev_attr_groups[] = {
int dim2_sysfs_probe(struct device *dev)
  {
-	dev->groups = dev_attr_groups;
-	return device_register(dev);
+	return sysfs_create_groups(&dev->kobj, dev_attr_groups);

No driver code should ever be calling a sysfs_* function, which is a
huge hint that this is incorrect.

You also just raced with userspace and lost, please use the default
attributes for the driver or bus for this, but NEVER manually add and
remove sysfs files, that way lies madness and hard to maintain code.
I'm aware of this race, but still creating attributes on device probe is under wide use in the kernel:

nikita@cobook:~/kernel$ grep -r device_create_file drivers | wc -l
448

Still, in case of dim2 driver, moving to driver's dev_groups is trivial. Preparing that patch now.

Nikita





[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux