On Tue, Jan 21, 2020 at 12:39:55PM +0530, Ramalingam C wrote:
> As we are not using the sysfs infrastructure anymore, link to it is
> removed. And global srm data and mutex to protect it are removed,
> with required handling at revocation check function.
>
Hi Ram,
Thanks for taking this on. A few comments below..
> Yet to test hence RFC tag is added.
>
> Signed-off-by: Ramalingam C
> Suggested-by: Sean Paul
> ---
> drivers/gpu/drm/drm_hdcp.c | 68 ++
> drivers/gpu/drm/drm_internal.h | 4 --
> drivers/gpu/drm/drm_sysfs.c| 2 -
> 3 files changed, 27 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> index 9191633a3c43..cc08d953eb53 100644
> --- a/drivers/gpu/drm/drm_hdcp.c
> +++ b/drivers/gpu/drm/drm_hdcp.c
> @@ -23,13 +23,10 @@
>
> #include "drm_internal.h"
>
> -static struct hdcp_srm {
> +struct hdcp_srm {
struct drm_hdcp_srm
would be more consistent with the rest of the subsystem
> u32 revoked_ksv_cnt;
> u8 *revoked_ksv_list;
> -
> - /* Mutex to protect above struct member */
> - struct mutex mutex;
> -} *srm_data;
> +};
>
> static inline void drm_hdcp_print_ksv(const u8 *ksv)
> {
> @@ -91,7 +88,8 @@ static inline u32 get_vrl_length(const u8 *buf)
> return drm_hdcp_be24_to_cpu(buf);
> }
>
> -static int drm_hdcp_parse_hdcp1_srm(const u8 *buf, size_t count)
> +static int drm_hdcp_parse_hdcp1_srm(const u8 *buf, size_t count,
> + struct hdcp_srm *srm_data)
Usually the drm structs are the first argument.
> {
> struct hdcp_srm_header *header;
> u32 vrl_length, ksv_count;
> @@ -153,7 +151,8 @@ static int drm_hdcp_parse_hdcp1_srm(const u8 *buf, size_t
> count)
> return count;
> }
>
> -static int drm_hdcp_parse_hdcp2_srm(const u8 *buf, size_t count)
> +static int drm_hdcp_parse_hdcp2_srm(const u8 *buf, size_t count,
> + struct hdcp_srm *srm_data)
> {
> struct hdcp_srm_header *header;
> u32 vrl_length, ksv_count, ksv_sz;
> @@ -226,18 +225,20 @@ static inline bool is_srm_version_hdcp2(const u8 *buf)
> return *buf == (u8)(DRM_HDCP_2_SRM_ID << 4 | DRM_HDCP_2_INDICATOR);
> }
>
> -static void drm_hdcp_srm_update(const u8 *buf, size_t count)
> +static void drm_hdcp_srm_update(const u8 *buf, size_t count,
> + struct hdcp_srm *srm_data)
> {
> if (count < sizeof(struct hdcp_srm_header))
> return;
>
> if (is_srm_version_hdcp1(buf))
> - drm_hdcp_parse_hdcp1_srm(buf, count);
> + drm_hdcp_parse_hdcp1_srm(buf, count, srm_data);
> else if (is_srm_version_hdcp2(buf))
> - drm_hdcp_parse_hdcp2_srm(buf, count);
> + drm_hdcp_parse_hdcp2_srm(buf, count, srm_data);
> }
>
> -static void drm_hdcp_request_srm(struct drm_device *drm_dev)
> +static void drm_hdcp_request_srm(struct drm_device *drm_dev,
> + struct hdcp_srm *srm_data)
> {
> char fw_name[36] = "display_hdcp_srm.bin";
> const struct firmware *fw;
> @@ -250,7 +251,7 @@ static void drm_hdcp_request_srm(struct drm_device
> *drm_dev)
> goto exit;
>
> if (fw->size && fw->data)
> - drm_hdcp_srm_update(fw->data, fw->size);
> + drm_hdcp_srm_update(fw->data, fw->size, srm_data);
>
> exit:
> release_firmware(fw);
> @@ -284,35 +285,33 @@ static void drm_hdcp_request_srm(struct drm_device
> *drm_dev)
> bool drm_hdcp_check_ksvs_revoked(struct drm_device *drm_dev, u8 *ksvs,
Unrelated nit: This function should really return int and let the caller decide
what to do
with the result.
>u32 ksv_count)
> {
> - u32 rev_ksv_cnt, cnt, i, j;
> + struct hdcp_srm *srm_data;
> u8 *rev_ksv_list;
> + bool ret = false;
> + u32 cnt, i, j;
>
> + srm_data = kzalloc(sizeof(*srm_data), GFP_KERNEL);
No need to put srm_data on the heap, it's only a pointer and a u32.
> if (!srm_data)
> - return false;
> + return ret;
>
> - mutex_lock(_data->mutex);
> - drm_hdcp_request_srm(drm_dev);
> -
> - rev_ksv_cnt = srm_data->revoked_ksv_cnt;
> + drm_hdcp_request_srm(drm_dev, srm_data);
> rev_ksv_list = srm_data->revoked_ksv_list;
>
> /* If the Revoked ksv list is empty */
> - if (!rev_ksv_cnt || !rev_ksv_list) {
> - mutex_unlock(_data->mutex);
> - return false;
> - }
> + if (!srm_data->revoked_ksv_cnt || !rev_ksv_list)
You can't have one of these true without the other, right?
I think you should just reverse the loops below and remove this condition
entirely.
> + goto out;
>
> for (cnt = 0; cnt < ksv_count; cnt++) {
> rev_ksv_list = srm_data->revoked_ksv_list;
> - for (i = 0; i < rev_ksv_cnt; i++) {
> + for (i = 0; i <
