Re: [PATCH 4/9] drm/privacy-screen: Add notifier support

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 9/15/21 10:26 PM, Lyude Paul wrote:
> On Mon, 2021-09-06 at 09:35 +0200, Hans de Goede wrote:
>> Add support for privacy-screen consumers to register a notifier to
>> be notified of external (e.g. done by the hw itself on a hotkey press)
>> state changes.
>>
>> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpu/drm/drm_privacy_screen.c      | 67 +++++++++++++++++++++++
>>  include/drm/drm_privacy_screen_consumer.h | 15 +++++
>>  include/drm/drm_privacy_screen_driver.h   |  4 ++
>>  3 files changed, 86 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_privacy_screen.c
>> b/drivers/gpu/drm/drm_privacy_screen.c
>> index 294a09194bfb..7a5f878c3171 100644
>> --- a/drivers/gpu/drm/drm_privacy_screen.c
>> +++ b/drivers/gpu/drm/drm_privacy_screen.c
>> @@ -255,6 +255,49 @@ void drm_privacy_screen_get_state(struct
>> drm_privacy_screen *priv,
>>  }
>>  EXPORT_SYMBOL(drm_privacy_screen_get_state);
>>  
>> +/**
>> + * drm_privacy_screen_register_notifier - register a notifier
>> + * @priv: Privacy screen to register the notifier with
>> + * @nb: Notifier-block for the notifier to register
>> + *
>> + * Register a notifier with the privacy-screen to be notified of changes
>> made
>> + * to the privacy-screen state from outside of the privacy-screen class.
>> + * E.g. the state may be changed by the hardware itself in response to a
>> + * hotkey press.
>> + *
>> + * The notifier is called with no locks held. The new hw_state and sw_state
>> + * can be retrieved using the drm_privacy_screen_get_state() function.
>> + * A pointer to the drm_privacy_screen's struct is passed as the void *data
>> + * argument of the notifier_block's notifier_call.
>> + *
>> + * The notifier will NOT be called when changes are made through
>> + * drm_privacy_screen_set_sw_state(). It is only called for external
>> changes.
>> + *
>> + * Return: 0 on success, negative error code on failure.
>> + */
>> +int drm_privacy_screen_register_notifier(struct drm_privacy_screen *priv,
>> +                                        struct notifier_block *nb)
>> +{
>> +       return blocking_notifier_chain_register(&priv->notifier_head, nb);
>> +}
>> +EXPORT_SYMBOL(drm_privacy_screen_register_notifier);
>> +
>> +/**
>> + * drm_privacy_screen_unregister_notifier - unregister a notifier
>> + * @priv: Privacy screen to register the notifier with
>> + * @nb: Notifier-block for the notifier to register
>> + *
>> + * Unregister a notifier registered with
>> drm_privacy_screen_register_notifier().
>> + *
>> + * Return: 0 on success, negative error code on failure.
>> + */
>> +int drm_privacy_screen_unregister_notifier(struct drm_privacy_screen *priv,
>> +                                          struct notifier_block *nb)
>> +{
>> +       return blocking_notifier_chain_unregister(&priv->notifier_head, nb);
>> +}
>> +EXPORT_SYMBOL(drm_privacy_screen_unregister_notifier);
>> +
>>  /*** drm_privacy_screen_driver.h functions ***/
>>  
>>  static ssize_t sw_state_show(struct device *dev,
>> @@ -352,6 +395,7 @@ struct drm_privacy_screen *drm_privacy_screen_register(
>>                 return ERR_PTR(-ENOMEM);
>>  
>>         mutex_init(&priv->lock);
>> +       BLOCKING_INIT_NOTIFIER_HEAD(&priv->notifier_head);
>>  
>>         priv->dev.class = drm_class;
>>         priv->dev.type = &drm_privacy_screen_type;
>> @@ -399,3 +443,26 @@ void drm_privacy_screen_unregister(struct
>> drm_privacy_screen *priv)
>>         device_unregister(&priv->dev);
>>  }
>>  EXPORT_SYMBOL(drm_privacy_screen_unregister);
>> +
>> +/**
>> + * drm_privacy_screen_call_notifier_chain - notify consumers of state
>> change
>> + * @priv: Privacy screen to register the notifier with
>> + *
>> + * A privacy-screen provider driver can call this functions upon external
>> + * changes to the privacy-screen state. E.g. the state may be changed by
>> the
>> + * hardware itself in response to a hotkey press.
>> + * This function must be called without holding the privacy-screen lock.
>> + * the driver must update sw_state and hw_state to reflect the new state
>> before
>> + * calling this function.
>> + * The expected behavior from the driver upon receiving an external state
>> + * change event is: 1. Take the lock; 2. Update sw_state and hw_state;
>> + * 3. Release the lock. 4. Call drm_privacy_screen_call_notifier_chain().
>> + */
>> +void drm_privacy_screen_call_notifier_chain(struct drm_privacy_screen
>> *priv)
>> +{
>> +       if (WARN_ON(mutex_is_locked(&priv->lock)))
>> +               return;
> 
> Are we sure about this check? mutex_is_locked() checks whether a mutex is
> locked by anyone, not just us. So this seems like it would cause us to
> WARN_ON() and abort if anyone else (not just ourselves) is holding the lock to
> read the privacy screen state.

Thank you for catching this, yes this check indeed is wrong. AFAIK
there is no way to check that the mutex has been locked by us, so this
extra sanity check simply needs to be removed.

I'll drop the check before pushing this to drm-misc-next (more on
that in a reply to the cover letter), if that is ok with you.

Or do you want me to do a new version addressing this?

Regards,

Hans



> 
>> +
>> +       blocking_notifier_call_chain(&priv->notifier_head, 0, priv);
>> +}
>> +EXPORT_SYMBOL(drm_privacy_screen_call_notifier_chain);
>> diff --git a/include/drm/drm_privacy_screen_consumer.h
>> b/include/drm/drm_privacy_screen_consumer.h
>> index 0cbd23b0453d..7f66a90d15b7 100644
>> --- a/include/drm/drm_privacy_screen_consumer.h
>> +++ b/include/drm/drm_privacy_screen_consumer.h
>> @@ -24,6 +24,11 @@ int drm_privacy_screen_set_sw_state(struct
>> drm_privacy_screen *priv,
>>  void drm_privacy_screen_get_state(struct drm_privacy_screen *priv,
>>                                   enum drm_privacy_screen_status
>> *sw_state_ret,
>>                                   enum drm_privacy_screen_status
>> *hw_state_ret);
>> +
>> +int drm_privacy_screen_register_notifier(struct drm_privacy_screen *priv,
>> +                                        struct notifier_block *nb);
>> +int drm_privacy_screen_unregister_notifier(struct drm_privacy_screen *priv,
>> +                                          struct notifier_block *nb);
>>  #else
>>  static inline struct drm_privacy_screen *drm_privacy_screen_get(struct
>> device *dev,
>>                                                                 const char
>> *con_id)
>> @@ -45,6 +50,16 @@ static inline void drm_privacy_screen_get_state(struct
>> drm_privacy_screen *priv,
>>         *sw_state_ret = PRIVACY_SCREEN_DISABLED;
>>         *hw_state_ret = PRIVACY_SCREEN_DISABLED;
>>  }
>> +static inline int drm_privacy_screen_register_notifier(struct
>> drm_privacy_screen *priv,
>> +                                                      struct notifier_block
>> *nb)
>> +{
>> +       return -ENODEV;
>> +}
>> +static inline int drm_privacy_screen_unregister_notifier(struct
>> drm_privacy_screen *priv,
>> +                                                        struct
>> notifier_block *nb)
>> +{
>> +       return -ENODEV;
>> +}
>>  #endif
>>  
>>  #endif
>> diff --git a/include/drm/drm_privacy_screen_driver.h
>> b/include/drm/drm_privacy_screen_driver.h
>> index 5187ae52eb03..24591b607675 100644
>> --- a/include/drm/drm_privacy_screen_driver.h
>> +++ b/include/drm/drm_privacy_screen_driver.h
>> @@ -54,6 +54,8 @@ struct drm_privacy_screen {
>>         struct mutex lock;
>>         /** @list: privacy-screen devices list list-entry. */
>>         struct list_head list;
>> +       /** @notifier_head: privacy-screen notifier head. */
>> +       struct blocking_notifier_head notifier_head;
>>         /**
>>          * @ops: &struct drm_privacy_screen_ops for this privacy-screen.
>>          * This is NULL if the driver has unregistered the privacy-screen.
>> @@ -77,4 +79,6 @@ struct drm_privacy_screen *drm_privacy_screen_register(
>>         struct device *parent, const struct drm_privacy_screen_ops *ops);
>>  void drm_privacy_screen_unregister(struct drm_privacy_screen *priv);
>>  
>> +void drm_privacy_screen_call_notifier_chain(struct drm_privacy_screen
>> *priv);
>> +
>>  #endif
>