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.
Well, drm/i915 don't call this function either.
Only the drm/radeon call this function.
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.
But I do agree with you that the comments of this function are sort of
good quality document.
After your change, we've lost that information, and we haven't fixed
anything.
But back to the technique question, is it true that
the drm_dev_unregister() function alone can't guarantee
the device isn't userspace accessible any more ?
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
