Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5aee218e-2e46-b929-f905-a28794caac8c@loongson.cn>
Date:   Tue, 27 Jun 2023 17:04:42 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] drm: Remove the deprecated drm_put_dev() function
Content-Language: en-US
To:     Jani Nikula <jani.nikula@linux.intel.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Alex Deucher <alexander.deucher@amd.com>,
        Christian Koenig <christian.koenig@amd.com>,
        Pan Xinhui <Xinhui.Pan@amd.com>
Cc:     amd-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org
References: <20230625050901.393055-1-suijingfeng@loongson.cn>
 <87jzvqy897.fsf@intel.com>
From:   Sui Jingfeng <suijingfeng@loongson.cn>
Organization: Loongson
In-Reply-To: <87jzvqy897.fsf@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

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