Message-ID: <766f323a-efcf-6552-f7e5-a736830c3f12@loongson.cn>
Date:   Tue, 27 Jun 2023 16:41:35 +0800
From:   Sui Jingfeng <suijingfeng@...ngson.cn>
To:     Thomas Zimmermann <tzimmermann@...e.de>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        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:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        amd-gfx@...ts.freedesktop.org
Subject: Re: [PATCH] drm: Remove the deprecated drm_put_dev() function

Hi,

On 2023/6/26 15:56, Thomas Zimmermann wrote:
> Hi
>
> Am 25.06.23 um 07:09 schrieb Sui Jingfeng:
>> 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.
>> - */
>> -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);
>
> Did you verify that dev cannot be NULL here? There was a check in 
> drm_put_dev() for !dev.
>

I have verified :


1)

If there is no radeon card(say R5-340) mounted in the system,  I 
modprobe the radeon.ko manually.

then both the radeon_pci_probe() and the radeon_pci_remove() function 
won't get called.

There is no chance that the driver_probe_device() function getting called.


|- driver_register()

|-- bus_add_driver()

|--- driver_attach()

|---- bus_for_each_dev(drv->bus, NULL, drv, __driver_attach)

|----- __driver_attach()

|------ __device_attach_driver()

// There is no chance that the driver_probe_device() function get called.

|------- driver_probe_device(drv, dev)

```


2)  normal case:


If there are radeon cards mounted in the system,

then as long as the pci_set_drvdata(pdev, dev) get called,

the 'driver_data' member of struct device will hold the pointer to the 
'struct drm_device';

So, it will be fine as long as the radeon.ko get loaded normally.


I'm sure it will works as expected on normal case, with 100% confident.


3) Abnormal case

If there is a error happen before the 'pci_set_drvdata(pdev, dev)' 
function get called.

It is also don't need to worry, if the ->probe() failed, then the 
->remove will be get called.


I have verified that

if the ->probe() failed, then the ->remove will be get called.

I'm doing the test by add a line before the drm_dev_alloc()

function in the body of radeon_pci_probe() function.

See below:

```

     return -ENODEV;

     dev = drm_dev_alloc(&kms_driver, &pdev->dev);
     if (IS_ERR(dev))
         return PTR_ERR(dev);

     ret = pci_enable_device(pdev);
     if (ret)
         goto err_free;
```


So, there is no problem, as far as I can see.


> Best regards
> Thomas
>
>> +    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