Re: [PATCH] drm: Remove the deprecated drm_put_dev() function

[Sui Jingfeng]

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