Re: [PATCH] drm: Remove the deprecated drm_put_dev() function

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

 



Hi,

On 2023/6/27 17:33, Jani Nikula wrote:
On Tue, 27 Jun 2023, Sui Jingfeng <suijingfeng@xxxxxxxxxxx> wrote:
Hi,

On 2023/6/26 15:48, Jani Nikula wrote:
On Sun, 25 Jun 2023, Sui Jingfeng <suijingfeng@xxxxxxxxxxx> wrote:
As this function can be replaced with drm_dev_unregister() + drm_dev_put(),
it is already marked as deprecated, so remove it. No functional change.

Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
---
   drivers/gpu/drm/drm_drv.c           | 28 ----------------------------
   drivers/gpu/drm/drm_pci.c           |  3 ++-
   drivers/gpu/drm/radeon/radeon_drv.c |  3 ++-
   include/drm/drm_drv.h               |  1 -
   4 files changed, 4 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 12687dd9e1ac..5057307fe22a 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -406,34 +406,6 @@ void drm_minor_release(struct drm_minor *minor)
    * possibly leaving the hardware enabled.
    */
-/**
- * drm_put_dev - Unregister and release a DRM device
- * @dev: DRM device
- *
- * Called at module unload time or when a PCI device is unplugged.
- *
- * Cleans up all DRM device, calling drm_lastclose().
- *
- * Note: Use of this function is deprecated. It will eventually go away
- * completely.  Please use drm_dev_unregister() and drm_dev_put() explicitly
- * instead to make sure that the device isn't userspace accessible any more
- * while teardown is in progress, ensuring that userspace can't access an
- * inconsistent state.
The last sentence is the crucial one. While the patch has no functional
changes,
But my patch help to save a useless check(if (!dev))

on where we found the check is not necessary.

```

-	if (!dev) {
-		DRM_ERROR("cleanup called no dev\n");
-		return;
-	}

```


I believe the goal never was to just mechanically replace one
call with the two.
The DRM core lose nothing, just a function wrapper.

Instead, this is probably a good chance to migrate the burden to the
driver side.
The point is to *fix* this stuff while doing the conversion. Anyone can
replace one function call with two, but that's just brushing the problem
under the carpet.

Interesting!

To be honest, I love you reviews.


I think, I probably be able to solve more practical problem,

This patch is just a trivial, I don't care much.


Can you help to have a look at my vgaarb patch set [1]

[1] https://patchwork.freedesktop.org/series/119250/

Really need you guidance there.


The current state is that we have a function the use of which is
potentially problematic, it's documented, and we can trivially locate
all the call sites.

I can merge the document to comments of the drm_dev_unregister() function,

remove the drm_put_dev function only. What do you think about this?

After your change, we've lost that information, and we haven't fixed
anything.

My understanding is that drm_dev_unregister() alone is enough to stop the user-space

program accessing the kernel interface exposed.

```

radeon_pci_remove(struct pci_dev *pdev)

{

     // should be called first in the pci remove funcition

    drm_dev_unregister(dev);

    // do the various(necessary) cleanup and free resource allocated


    // call this function at the bottom of the radeon_pci_remove() function

    drm_dev_put(dev);

}

```


And the drm_dev_unregister() function already told us, see below:


```
/**
 * drm_dev_unregister - Unregister DRM device
 * @dev: Device to unregister
 *
 * Unregister the DRM device from the system. This does the reverse of
 * drm_dev_register() but does not deallocate the device. The caller must call  * drm_dev_put() to drop their final reference, unless it is managed with devres  * (as devices allocated with devm_drm_dev_alloc() are), in which case there is
 * already an unwind action registered.
 *
 * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
 * which can be called while there are still open users of @dev.
 *
 * This should be called first in the device teardown code to make sure
 * userspace can't access the device instance any more.
 */
void drm_dev_unregister(struct drm_device *dev)

...

```


Thanks for the interesting remark again.

BR,
Jani.


I think the device driver(drm/radeon, for example) have better understanding

about how to ensure that userspace can't access an inconsistent state
than the DRM core.

BR,
Jani.


- */
-void drm_put_dev(struct drm_device *dev)
-{
-	DRM_DEBUG("\n");
-
-	if (!dev) {
-		DRM_ERROR("cleanup called no dev\n");
-		return;
-	}
-
-	drm_dev_unregister(dev);
-	drm_dev_put(dev);
-}
-EXPORT_SYMBOL(drm_put_dev);
-
   /**
    * drm_dev_enter - Enter device critical section
    * @dev: DRM device
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 39d35fc3a43b..b3a68a92eaa6 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -257,7 +257,8 @@ void drm_legacy_pci_exit(const struct drm_driver *driver,
   					 legacy_dev_list) {
   			if (dev->driver == driver) {
   				list_del(&dev->legacy_dev_list);
-				drm_put_dev(dev);
+				drm_dev_unregister(dev);
+				drm_dev_put(dev);
   			}
   		}
   		mutex_unlock(&legacy_dev_list_lock);
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index e4374814f0ef..a4955ae10659 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -357,7 +357,8 @@ radeon_pci_remove(struct pci_dev *pdev)
   {
   	struct drm_device *dev = pci_get_drvdata(pdev);
- drm_put_dev(dev);
+	drm_dev_unregister(dev);
+	drm_dev_put(dev);
   }
static void
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 89e2706cac56..289c97b12e82 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -511,7 +511,6 @@ void drm_dev_unregister(struct drm_device *dev);
void drm_dev_get(struct drm_device *dev);
   void drm_dev_put(struct drm_device *dev);
-void drm_put_dev(struct drm_device *dev);
   bool drm_dev_enter(struct drm_device *dev, int *idx);
   void drm_dev_exit(int idx);
   void drm_dev_unplug(struct drm_device *dev);

--
Jingfeng




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux