Message-ID: <c343d5d5-de87-089a-74b3-ca47089002b0@loongson.cn>
Date:   Tue, 27 Jun 2023 18:20:18 +0800
From:   Sui Jingfeng <suijingfeng@...ngson.cn>
To:     Jani Nikula <jani.nikula@...ux.intel.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Alex Deucher <alexander.deucher@....com>,
        Christian Koenig <christian.koenig@....com>,
        Pan Xinhui <Xinhui.Pan@....com>
Cc:     amd-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm: Remove the deprecated drm_put_dev() function

Hi,

On 2023/6/27 17:33, Jani Nikula wrote:
> On Tue, 27 Jun 2023, Sui Jingfeng <suijingfeng@...ngson.cn> wrote:
>> Hi,
>>
>> On 2023/6/26 15:48, Jani Nikula wrote:
>>> On Sun, 25 Jun 2023, Sui Jingfeng <suijingfeng@...ngson.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@...ngson.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.

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

Powered by blists - more mailing lists