Re: [PATCH 03/12] drm: call ->firstopen() before ->open()

[Alex Deucher <alexdeucher@gmail.com>]

On Thu, Jul 24, 2014 at 5:31 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Jul 23, 2014 at 9:25 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Jul 23, 2014 at 05:26:38PM +0200, David Herrmann wrote:
>>> Lets order things correctly:
>>>  ->load()
>>>    ->fistopen()
>>>      ->open()
>>>      ->close()
>>>    ->lastclose()
>>>  ->unload()
>>>
>>> This doesn't change much as only savage and radeon use ->firstopen() and
>>> they just do map-initialization. Therefore, the global drm mutex makes
>>> sure there cannot be any other f_op between ->open() and ->firstopen().
>>> However, once we get rid of that lock, we really want ->firstopen() to
>>> initialize the device before ->open() is called.
>>>
>>> Furthermore, this fixes the clean-up path in drm_open(). We currently
>>> don't cleanup the drm_file object if ->firstopen() fails.
>>>
>>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>>> ---
>>>  drivers/gpu/drm/drm_fops.c | 139 +++++++++++++++++++++------------------------
>>>  include/drm/drmP.h         |   2 +-
>>>  2 files changed, 66 insertions(+), 75 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>>> index 021fe5d..8e73519 100644
>>> --- a/drivers/gpu/drm/drm_fops.c
>>> +++ b/drivers/gpu/drm/drm_fops.c
>>> @@ -45,7 +45,7 @@ EXPORT_SYMBOL(drm_global_mutex);
>>>
>>>  static int drm_open_helper(struct file *filp, struct drm_minor *minor);
>>>
>>> -static int drm_setup(struct drm_device * dev)
>>> +static int drm_firstopen(struct drm_device * dev)
>>
>> All the stuff in here is only for legacy drivers. Imo we should rename
>> this to drm_legacy_setup and hide it harder.
>>
>> Also touching the init ordering for ums drivers is a bit risky, I'd advise
>> against it. firstopen is officially dead for kms driver and really there's
>> nothing legit you can do in there. imx abused until they've switched over
>> to the component framework.
>>
>> I'd just drop this one tbh.
>
> I did a driver audit when writing this patch, there're only 2 drivers
> using firstopen:
>
>  * radeon_cp: sets two fields of dev_private and calls drm_addmap() on
> those. In lastclose(), it calls drm_rmmap()
>  * savage: sets several fields in dev_private, calls
> arch_phys_wc_add() for some regions and requests a drm-map for those
> via drm_addmap(). On lastclose(), it reverts exactly those steps.
>
> Taking into account that firstopen() just runs with drm_device
> context, not with drm_file context, I really think we should be
> calling it _before_ doing ->open(). I even think the drivers would
> fail horribly if we don't do this patch and some other user-context
> sneaks in between both calls (currently impossible thanks to
> drm_global_mutex).
>
> Both ->firstopen() callbacks are fairly trivial. In favor of making
> the error-paths work in drm_open() I'd really like to get this in.
> This patch is preparing for my drm_file rework that can make
> drm_dev_unregister() stop all open files immediately.
>
> But I honestly don't care much for UMS drivers, so if you NACK this,
> I'd just ignore the error code and add a comment that we don't do
> error-handling for UMS.

Well, radeon UMS support is in the kernel deprecated and hasn't been
built by default for a while and I doubt anyone has tested it in
years.  I'm not sure what the current state of savage is.  I'm fine
either way.

Alex


>
> Cheers
> David
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel