[PATCH 6/6] drm/tinydrm: Support device unplug

[Noralf Trønnes]

Den 04.09.2017 11.04, skrev Daniel Vetter:
> On Mon, Sep 04, 2017 at 11:41:05AM +0300, Laurent Pinchart wrote:
>> Hi Daniel,
>>
>> On Monday, 4 September 2017 10:26:15 EEST Daniel Vetter wrote:
>>> On Thu, Aug 31, 2017 at 09:22:03PM +0200, Noralf Trønnes wrote:
>>>> Den 31.08.2017 14.59, skrev Laurent Pinchart:
>>>>> On Wednesday, 30 August 2017 20:18:49 EEST Daniel Vetter wrote:
>>>>>> On Wed, Aug 30, 2017 at 6:31 PM, Noralf Trønnes wrote:
>>>>>>> Den 28.08.2017 23.56, skrev Daniel Vetter:
>>>>>>>> On Mon, Aug 28, 2017 at 07:17:48PM +0200, Noralf Trønnes wrote:
>>>>>>>>> +
>>>>>>>>> +       drm_fbdev_cma_dev_unplug(tdev->fbdev_cma);
>>>>>>>>> +       drm_dev_unplug(&tdev->drm);
>>>>>>>>> +
>>>>>>>>> +       /* Make sure framebuffer flushing is done */
>>>>>>>>> +       mutex_lock(&tdev->dirty_lock);
>>>>>>>>> +       mutex_unlock(&tdev->dirty_lock);
>>>>>>>> Is this really needed? Or, doesn't it just paper over a driver bug
>>>>>>>> you have already anyway, since native kms userspace can directly
>>>>>>>> call fb->funcs->dirty too, and you already protect against that.
>>>>>>>>
>>>>>>>> This definitely looks like the fbdev helper is leaking
>>>>>>>> implementation details to callers where it shouldn't do that.
>>>>>>> Flushing can happen while drm_dev_unplug() is called, and when we
>>>>>>> leave this function the device facing resources controlled by devres
>>>>>>> will be removed. Thus I have to make sure any such flushing is done
>>>>>>> before leaving so the next flush is stopped by the
>>>>>>> drm_dev_is_unplugged() check. I don't see any other way of ensuring
>>>>>>> that.
>>>>>>>
>>>>>>> I see now that I should move the call to
>>>>>>> drm_atomic_helper_shutdown() after drm_dev_unplug() to properly
>>>>>>> protect the pipe .enable/.disable callbacks.
>>>>>> Hm, calling _shutdown when the hw is gone already won't end well.
>>>>>> Fundamentally this race exists for all use-cases, and I'm somewhat
>>>>>> leaning towards plugging it in the core.
>>>>>>
>>>>>> The general solution probably involves something that smells a lot
>>>>>> like srcu, i.e. at every possible entry point into a drm driver
>>>>>> (ioctl, fbdev, dma-buf sharing, everything really) we take that
>>>>>> super-cheap read-side look, and drop it when we leave.
>>>>> That's similar to what we plan to do in V4L2. The idea is to set a
>>>>> device removed flag at the beginning of the .remove() handler and wait
>>>>> for all pending operations to complete. The core will reject any new
>>>>> operation when the flag is set. To wait for completion, every entry
>>>>> point would increase a use count, and decrease it on exit. When the use
>>>>> count is decreased to 0 waiters will be woken up. This should solve the
>>>>> unplug/user race.
>>>> Ah, such a simple solution, easy to understand and difficult to get wrong!
>>>> And it's even nestable, no danger of deadlocking.
>>>>
>>>> Maybe I can use it with tinydrm:
>>>>
>>>> * @dev_use: Tracks use of functions acessing the parent device.
>>>> *           If it is zero, the device is gone. See ...
>>>> struct tinydrm_device {
>>>>      atomic_t dev_use;
>>>> };
>>>>
>>>> /**
>>>>   * tinydrm_dev_enter - Enter device accessing function
>>>>   * @tdev: tinydrm device
>>>>   *
>>>>   * This function protects against using a device and it's resources after
>>>>   * it's removed. Should be called at the beginning of the function.
>>>>   *
>>>>   * Returns:
>>>>   * False if the device is still present, true if it is gone.
>>>>   */
>>>> static inline bool tinydrm_dev_enter(struct tinydrm_device *tdev)
>>>> {
>>>>      return !atomic_inc_not_zero(&tdev->dev_use);
>>>> }
>>>>
>>>> static inline void tinydrm_dev_exit(struct tinydrm_device *tdev)
>>>> {
>>>>      atomic_dec(&tdev->dev_use);
>>>> }
>>>>
>>>> static inline bool tinydrm_is_unplugged(struct tinydrm_device *tdev)
>>>> {
>>>>      bool ret = !atomic_read(&tdev->dev_use);
>>>>      smp_rmb();
>>>>      return ret;
>>>> }
>>>>
>>>>
>>>> static int tinydrm_init(...)
>>>> {
>>>>      /* initialize */
>>>>
>>>>      /* Set device is present */
>>>>      atomic_set(&tdev->dev_use, 1);
>>>> }
>>>>
>>>> static void tinydrm_unregister(...)
>>>> {
>>>>      /* Set device gone */
>>>>      atomic_dec(&tdev->dev_use);
>>>>
>>>>      /* Wait for all device facing functions to finish */
>>>>      while (!tinydrm_is_unplugged(tdev)) {
>>>>          cond_resched();
>>>>      }
>>>>
>>>>      /* proceed with unregistering */
>>>> }
>>>>
>>>> static int mipi_dbi_fb_dirty(...)
>>>> {
>>>>      if (tinydrm_dev_enter(tdev))
>>>>          return -ENODEV;
>>>>
>>>>      /* flush framebuffer */
>>>>
>>>>      tinydrm_dev_exit(tdev);
>>>> }
>>> Yup, expect imo this should be done in drm core (as much as possible, i.e.
>>> for ioctl at least, standard sysfs), plus then enter/exit exported to
>>> drivers for their stuff.
>>>
>>> And it really needs to be srcu. For kms drivers the atomic_inc/dec wont
>>> matter, for render drivers it will show up (some of our ioctls are 100%
>>> lock less in the fastpath, not a single atomic op in there).
>> Don't forget that we need to check the disconnect flag when entering ioctls to
>> return -ENODEV. I'm open to clever solutions, but I'd first aim for
>> correctness with a lock and then replace the internal implementation.
> As Noralf pointed out, we already check for drm_dev_is_unplugged(). Maybe
> not in all places, but that can be fixed.
>
> And you can't first make all ioctl slower and then fix it up, at least not
> spread over multiple patch series. I guess for developing, doing the
> simpler atomic counter first is ok. But the same patch series needs to
> move over to srcu at the end.

I've never used srcu/rcu before, care to explain a little bit more?
Is it drm_device we are protecting here?

How can srcu be better for the fast path you're talking about if it's
half of srcu (assuming that makes srcu slower)?

Noralf.

> But I'm not sure that's a good idea, since implementing this 100%
> correctly using your atomic_t idea means you implement half of srcu
> anyway. Otoh discovering all those races should be an interesting journey
> :-)
> -Daniel