Messages in this thread | | | Date | Tue, 27 Jun 2023 17:55:39 +0800 | Subject | Re: [PATCH] drm: Remove the deprecated drm_put_dev() function | From | Sui Jingfeng <> |
| |
Hi
On 2023/6/27 17:33, Jani Nikula wrote: > On Tue, 27 Jun 2023, Sui Jingfeng <suijingfeng@loongson.cn> wrote: >> Hi, >> >> On 2023/6/26 15:48, Jani Nikula wrote: >>> On Sun, 25 Jun 2023, Sui Jingfeng <suijingfeng@loongson.cn> 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@loongson.cn> >>>> --- >>>> 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
| |