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

[David Herrmann]

Hi

On Wed, Jul 23, 2014 at 9:25 PM, Daniel Vetter <daniel at 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 at 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.

Cheers
David