[v6,03/10] drm: revocation check at drm subsystem

Submitted by Ramalingam C on May 2, 2019, 1:22 p.m.

Details

Message ID 20190502132303.27696-4-ramalingam.c@intel.com
State New
Headers show
Series "HDCP2.2 Phase II" ( rev: 8 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ramalingam C May 2, 2019, 1:22 p.m.
On every hdcp revocation check request SRM is read from fw file
/lib/firmware/display_hdcp_srm.bin

SRM table is parsed and stored at drm_hdcp.c, with functions exported
for the services for revocation check from drivers (which
implements the HDCP authentication)

This patch handles the HDCP1.4 and 2.2 versions of SRM table.

v2:
  moved the uAPI to request_firmware_direct() [Daniel]
v3:
  kdoc added. [Daniel]
  srm_header unified and bit field definitions are removed. [Daniel]
  locking improved. [Daniel]
  vrl length violation is fixed. [Daniel]

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
Suggested-by: Daniel Vetter <daniel@ffwll.ch>
---
 Documentation/gpu/drm-kms-helpers.rst |   6 +
 drivers/gpu/drm/Makefile              |   2 +-
 drivers/gpu/drm/drm_hdcp.c            | 342 ++++++++++++++++++++++++++
 drivers/gpu/drm/drm_internal.h        |   4 +
 drivers/gpu/drm/drm_sysfs.c           |   2 +
 include/drm/drm_hdcp.h                |  24 ++
 6 files changed, 379 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_hdcp.c

Patch hide | download patch | download mbox

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 14102ae035dc..0fe726a6ee67 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -181,6 +181,12 @@  Panel Helper Reference
 .. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c
    :export:
 
+HDCP Helper Functions Reference
+===============================
+
+.. kernel-doc:: drivers/gpu/drm/drm_hdcp.c
+   :export:
+
 Display Port Helper Functions Reference
 =======================================
 
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 72f5036d9bfa..dd02e9dec810 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -17,7 +17,7 @@  drm-y       :=	drm_auth.o drm_cache.o \
 		drm_plane.o drm_color_mgmt.o drm_print.o \
 		drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
 		drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
-		drm_atomic_uapi.o
+		drm_atomic_uapi.o drm_hdcp.o
 
 drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o
 drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
new file mode 100644
index 000000000000..dc0e13409221
--- /dev/null
+++ b/drivers/gpu/drm/drm_hdcp.c
@@ -0,0 +1,342 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Intel Corporation.
+ *
+ * Authors:
+ * Ramalingam C <ramalingam.c@intel.com>
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gfp.h>
+#include <linux/export.h>
+#include <linux/slab.h>
+#include <linux/firmware.h>
+
+#include <drm/drm_hdcp.h>
+#include <drm/drm_sysfs.h>
+#include <drm/drm_print.h>
+#include <drm/drm_device.h>
+
+struct hdcp_srm {
+	u8 *srm_buf;
+	size_t received_srm_sz;
+	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)
+{
+	DRM_DEBUG("\t%#02x, %#02x, %#02x, %#02x, %#02x\n",
+		  ksv[0], ksv[1], ksv[2], ksv[3], ksv[4]);
+}
+
+static u32 drm_hdcp_get_revoked_ksv_count(const u8 *buf, u32 vrls_length)
+{
+	u32 parsed_bytes = 0, ksv_count = 0, vrl_ksv_cnt, vrl_sz;
+
+	while (parsed_bytes < vrls_length) {
+		vrl_ksv_cnt = *buf;
+		ksv_count += vrl_ksv_cnt;
+
+		vrl_sz = (vrl_ksv_cnt * DRM_HDCP_KSV_LEN) + 1;
+		buf += vrl_sz;
+		parsed_bytes += vrl_sz;
+	}
+
+	/*
+	 * When vrls are not valid, ksvs are not considered.
+	 * Hence SRM will be discarded.
+	 */
+	if (parsed_bytes != vrls_length)
+		ksv_count = 0;
+
+	return ksv_count;
+}
+
+static u32 drm_hdcp_get_revoked_ksvs(const u8 *buf, u8 *revoked_ksv_list,
+				     u32 vrls_length)
+{
+	u32 parsed_bytes = 0, ksv_count = 0;
+	u32 vrl_ksv_cnt, vrl_ksv_sz, vrl_idx = 0;
+
+	do {
+		vrl_ksv_cnt = *buf;
+		vrl_ksv_sz = vrl_ksv_cnt * DRM_HDCP_KSV_LEN;
+
+		buf++;
+
+		DRM_DEBUG("vrl: %d, Revoked KSVs: %d\n", vrl_idx++,
+			  vrl_ksv_cnt);
+		memcpy(revoked_ksv_list, buf, vrl_ksv_sz);
+
+		ksv_count += vrl_ksv_cnt;
+		revoked_ksv_list += vrl_ksv_sz;
+		buf += vrl_ksv_sz;
+
+		parsed_bytes += (vrl_ksv_sz + 1);
+	} while (parsed_bytes < vrls_length);
+
+	return ksv_count;
+}
+
+static inline u32 get_vrl_length(const u8 *buf)
+{
+	return (u32)(buf[0] << 16 | buf[1] << 8 | buf[2]);
+}
+
+static int drm_hdcp_parse_hdcp1_srm(const u8 *buf, size_t count)
+{
+	struct hdcp_srm_header *header;
+	u32 vrl_length, ksv_count;
+
+	if (count < (sizeof(struct hdcp_srm_header) +
+	    DRM_HDCP_1_4_VRL_LENGTH_SIZE + DRM_HDCP_1_4_DCP_SIG_SIZE)) {
+		DRM_ERROR("Invalid blob length\n");
+		return -EINVAL;
+	}
+
+	header = (struct hdcp_srm_header *)buf;
+	DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
+		  header->srm_id,
+		  __swab16(header->srm_version), header->srm_gen_no);
+
+	WARN_ON(header->reserved);
+
+	buf = buf + sizeof(*header);
+	vrl_length = get_vrl_length(buf);
+	if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
+	    vrl_length < (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
+			  DRM_HDCP_1_4_DCP_SIG_SIZE)) {
+		DRM_ERROR("Invalid blob length or vrl length\n");
+		return -EINVAL;
+	}
+
+	/* Length of the all vrls combined */
+	vrl_length -= (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
+		       DRM_HDCP_1_4_DCP_SIG_SIZE);
+
+	if (!vrl_length) {
+		DRM_ERROR("No vrl found\n");
+		return -EINVAL;
+	}
+
+	buf += DRM_HDCP_1_4_VRL_LENGTH_SIZE;
+	ksv_count = drm_hdcp_get_revoked_ksv_count(buf, vrl_length);
+	if (!ksv_count) {
+		DRM_DEBUG("Revoked KSV count is 0\n");
+		return count;
+	}
+
+	kfree(srm_data->revoked_ksv_list);
+	srm_data->revoked_ksv_list = kcalloc(ksv_count, DRM_HDCP_KSV_LEN,
+					     GFP_KERNEL);
+	if (!srm_data->revoked_ksv_list) {
+		DRM_ERROR("Out of Memory\n");
+		return -ENOMEM;
+	}
+
+	if (drm_hdcp_get_revoked_ksvs(buf, srm_data->revoked_ksv_list,
+				      vrl_length) != ksv_count) {
+		srm_data->revoked_ksv_cnt = 0;
+		kfree(srm_data->revoked_ksv_list);
+		return -EINVAL;
+	}
+
+	srm_data->revoked_ksv_cnt = ksv_count;
+	return count;
+}
+
+static int drm_hdcp_parse_hdcp2_srm(const u8 *buf, size_t count)
+{
+	struct hdcp_srm_header *header;
+	u32 vrl_length, ksv_count, ksv_sz;
+
+	if (count < (sizeof(struct hdcp_srm_header) +
+	    DRM_HDCP_2_VRL_LENGTH_SIZE + DRM_HDCP_2_DCP_SIG_SIZE)) {
+		DRM_ERROR("Invalid blob length\n");
+		return -EINVAL;
+	}
+
+	header = (struct hdcp_srm_header *)buf;
+	DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
+		  header->srm_id & DRM_HDCP_SRM_ID_MASK,
+		  __swab16(header->srm_version), header->srm_gen_no);
+
+	if (header->reserved)
+		return -EINVAL;
+
+	buf = buf + sizeof(*header);
+	vrl_length = get_vrl_length(buf);
+
+	if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
+	    vrl_length < (DRM_HDCP_2_VRL_LENGTH_SIZE +
+	    DRM_HDCP_2_DCP_SIG_SIZE)) {
+		DRM_ERROR("Invalid blob length or vrl length\n");
+		return -EINVAL;
+	}
+
+	/* Length of the all vrls combined */
+	vrl_length -= (DRM_HDCP_2_VRL_LENGTH_SIZE +
+		       DRM_HDCP_2_DCP_SIG_SIZE);
+
+	if (!vrl_length) {
+		DRM_ERROR("No vrl found\n");
+		return -EINVAL;
+	}
+
+	buf += DRM_HDCP_2_VRL_LENGTH_SIZE;
+	ksv_count = (*buf << 2) | DRM_HDCP_2_KSV_COUNT_2_LSBITS(*(buf + 1));
+	if (!ksv_count) {
+		DRM_DEBUG("Revoked KSV count is 0\n");
+		return count;
+	}
+
+	kfree(srm_data->revoked_ksv_list);
+	srm_data->revoked_ksv_list = kcalloc(ksv_count, DRM_HDCP_KSV_LEN,
+					     GFP_KERNEL);
+	if (!srm_data->revoked_ksv_list) {
+		DRM_ERROR("Out of Memory\n");
+		return -ENOMEM;
+	}
+
+	ksv_sz = ksv_count * DRM_HDCP_KSV_LEN;
+	buf += DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ;
+
+	DRM_DEBUG("Revoked KSVs: %d\n", ksv_count);
+	memcpy(srm_data->revoked_ksv_list, buf, ksv_sz);
+
+	srm_data->revoked_ksv_cnt = ksv_count;
+	return count;
+}
+
+static inline bool is_srm_version_hdcp1(const u8 *buf)
+{
+	return *buf == (u8)(DRM_HDCP_1_4_SRM_ID << 4);
+}
+
+static inline bool is_srm_version_hdcp2(const u8 *buf)
+{
+	return *buf == (u8)(DRM_HDCP_2_SRM_ID << 4 | DRM_HDCP_2_INDICATOR);
+}
+
+static ssize_t drm_hdcp_srm_update(const u8 *buf, size_t count)
+{
+	if (is_srm_version_hdcp1(buf))
+		return (ssize_t)drm_hdcp_parse_hdcp1_srm(buf, count);
+	else if (is_srm_version_hdcp2(buf))
+		return (ssize_t)drm_hdcp_parse_hdcp2_srm(buf, count);
+
+	return (ssize_t)-EINVAL;
+}
+
+void drm_hdcp_request_srm(struct drm_device *drm_dev)
+{
+	char fw_name[36] = "display_hdcp_srm.bin";
+	const struct firmware *fw;
+
+	int ret;
+
+	ret = request_firmware_direct(&fw, (const char *)fw_name,
+				      drm_dev->dev);
+	if (ret < 0)
+		goto exit;
+
+	if (fw->size && fw->data)
+		drm_hdcp_srm_update(fw->data, fw->size);
+
+exit:
+	release_firmware(fw);
+}
+
+/**
+ * drm_hdcp_check_ksvs_revoked - Check the revoked status of the IDs
+ *
+ * @drm_dev: drm_device for which HDCP revocation check is requested
+ * @ksvs: List of KSVs (HDCP receiver IDs)
+ * @ksv_count: KSV count passed in through @ksvs
+ *
+ * This function reads the HDCP System renewability Message(SRM Table)
+ * from userspace as a firmware and parses it for the revoked HDCP
+ * KSVs(Receiver IDs) detected by DCP LLC. Once the revoked KSVs are known,
+ * revoked state of the KSVs in the list passed in by display drivers are
+ * decided and response is sent.
+ *
+ * SRM should be presented in the name of "display_hdcp_srm.bin".
+ *
+ * Returns:
+ * TRUE on any of the KSV is revoked, else FALSE.
+ */
+bool drm_hdcp_check_ksvs_revoked(struct drm_device *drm_dev, u8 *ksvs,
+				 u32 ksv_count)
+{
+	u32 rev_ksv_cnt, cnt, i, j;
+	u8 *rev_ksv_list;
+
+	if (!srm_data)
+		return false;
+
+	mutex_lock(&srm_data->mutex);
+	drm_hdcp_request_srm(drm_dev);
+
+	rev_ksv_cnt = srm_data->revoked_ksv_cnt;
+	rev_ksv_list = srm_data->revoked_ksv_list;
+
+	/* If the Revoked ksv list is empty */
+	if (!rev_ksv_cnt || !rev_ksv_list) {
+		mutex_unlock(&srm_data->mutex);
+		return false;
+	}
+
+	for  (cnt = 0; cnt < ksv_count; cnt++) {
+		rev_ksv_list = srm_data->revoked_ksv_list;
+		for (i = 0; i < rev_ksv_cnt; i++) {
+			for (j = 0; j < DRM_HDCP_KSV_LEN; j++)
+				if (ksvs[j] != rev_ksv_list[j]) {
+					break;
+				} else if (j == (DRM_HDCP_KSV_LEN - 1)) {
+					DRM_DEBUG("Revoked KSV is ");
+					drm_hdcp_print_ksv(ksvs);
+					mutex_unlock(&srm_data->mutex);
+					return true;
+				}
+			/* Move the offset to next KSV in the revoked list */
+			rev_ksv_list += DRM_HDCP_KSV_LEN;
+		}
+
+		/* Iterate to next ksv_offset */
+		ksvs += DRM_HDCP_KSV_LEN;
+	}
+	mutex_unlock(&srm_data->mutex);
+	return false;
+}
+EXPORT_SYMBOL_GPL(drm_hdcp_check_ksvs_revoked);
+
+int drm_setup_hdcp_srm(struct class *drm_class)
+{
+	srm_data = kzalloc(sizeof(*srm_data), GFP_KERNEL);
+	if (!srm_data)
+		return -ENOMEM;
+
+	srm_data->srm_buf = kcalloc(DRM_HDCP_SRM_GEN1_MAX_BYTES,
+				    sizeof(u8), GFP_KERNEL);
+	if (!srm_data->srm_buf) {
+		kfree(srm_data);
+		return -ENOMEM;
+	}
+	mutex_init(&srm_data->mutex);
+
+	return 0;
+}
+
+void drm_teardown_hdcp_srm(struct class *drm_class)
+{
+	if (srm_data) {
+		kfree(srm_data->srm_buf);
+		kfree(srm_data->revoked_ksv_list);
+		kfree(srm_data);
+	}
+}
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index e19ac7ca602d..476a422414f6 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -201,3 +201,7 @@  int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
 void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
 				const struct drm_framebuffer *fb);
 int drm_framebuffer_debugfs_init(struct drm_minor *minor);
+
+/* drm_hdcp.c */
+int drm_setup_hdcp_srm(struct class *drm_class);
+void drm_teardown_hdcp_srm(struct class *drm_class);
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index ecb7b33002bb..18b1ac442997 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -78,6 +78,7 @@  int drm_sysfs_init(void)
 	}
 
 	drm_class->devnode = drm_devnode;
+	drm_setup_hdcp_srm(drm_class);
 	return 0;
 }
 
@@ -90,6 +91,7 @@  void drm_sysfs_destroy(void)
 {
 	if (IS_ERR_OR_NULL(drm_class))
 		return;
+	drm_teardown_hdcp_srm(drm_class);
 	class_remove_file(drm_class, &class_attr_version.attr);
 	class_destroy(drm_class);
 	drm_class = NULL;
diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
index f243408ecf26..292998a2497f 100644
--- a/include/drm/drm_hdcp.h
+++ b/include/drm/drm_hdcp.h
@@ -265,4 +265,28 @@  void drm_hdcp2_u32_to_seq_num(u8 seq_num[HDCP_2_2_SEQ_NUM_LEN], u32 val)
 	seq_num[2] = val;
 }
 
+#define DRM_HDCP_SRM_GEN1_MAX_BYTES		(5 * 1024)
+#define DRM_HDCP_1_4_SRM_ID			0x8
+#define DRM_HDCP_SRM_ID_MASK			(0xF << 4)
+#define DRM_HDCP_1_4_VRL_LENGTH_SIZE		3
+#define DRM_HDCP_1_4_DCP_SIG_SIZE		40
+#define DRM_HDCP_2_SRM_ID			0x9
+#define DRM_HDCP_2_INDICATOR			0x1
+#define DRM_HDCP_2_INDICATOR_MASK		0xF
+#define DRM_HDCP_2_VRL_LENGTH_SIZE		3
+#define DRM_HDCP_2_DCP_SIG_SIZE			384
+#define DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ	4
+#define DRM_HDCP_2_KSV_COUNT_2_LSBITS(byte)	(((byte) & 0xC) >> 6)
+
+struct hdcp_srm_header {
+	u8 srm_id;
+	u8 reserved;
+	u16 srm_version;
+	u8 srm_gen_no;
+} __packed;
+
+struct drm_device;
+
+bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,
+				 u8 *ksvs, u32 ksv_count);
 #endif

Comments

On Thu, May 02, 2019 at 06:52:56PM +0530, Ramalingam C wrote:
> On every hdcp revocation check request SRM is read from fw file
> /lib/firmware/display_hdcp_srm.bin
> 
> SRM table is parsed and stored at drm_hdcp.c, with functions exported
> for the services for revocation check from drivers (which
> implements the HDCP authentication)
> 
> This patch handles the HDCP1.4 and 2.2 versions of SRM table.
> 
> v2:
>   moved the uAPI to request_firmware_direct() [Daniel]
> v3:
>   kdoc added. [Daniel]
>   srm_header unified and bit field definitions are removed. [Daniel]
>   locking improved. [Daniel]
>   vrl length violation is fixed. [Daniel]
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>

Found a few small details to polish, but looks good to me. With the
details addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

We also still need an ack on the firmware blob approach from Matt Roper or
someone else at iotg I think.

Cheers, Daniel

> ---
>  Documentation/gpu/drm-kms-helpers.rst |   6 +
>  drivers/gpu/drm/Makefile              |   2 +-
>  drivers/gpu/drm/drm_hdcp.c            | 342 ++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_internal.h        |   4 +
>  drivers/gpu/drm/drm_sysfs.c           |   2 +
>  include/drm/drm_hdcp.h                |  24 ++
>  6 files changed, 379 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_hdcp.c
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 14102ae035dc..0fe726a6ee67 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -181,6 +181,12 @@ Panel Helper Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c
>     :export:
>  
> +HDCP Helper Functions Reference
> +===============================
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_hdcp.c
> +   :export:
> +
>  Display Port Helper Functions Reference
>  =======================================
>  
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 72f5036d9bfa..dd02e9dec810 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -17,7 +17,7 @@ drm-y       :=	drm_auth.o drm_cache.o \
>  		drm_plane.o drm_color_mgmt.o drm_print.o \
>  		drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
>  		drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> -		drm_atomic_uapi.o
> +		drm_atomic_uapi.o drm_hdcp.o
>  
>  drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o
>  drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> new file mode 100644
> index 000000000000..dc0e13409221
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_hdcp.c
> @@ -0,0 +1,342 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Intel Corporation.
> + *
> + * Authors:
> + * Ramalingam C <ramalingam.c@intel.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gfp.h>
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <linux/firmware.h>
> +
> +#include <drm/drm_hdcp.h>
> +#include <drm/drm_sysfs.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_device.h>
> +
> +struct hdcp_srm {
> +	u8 *srm_buf;

Allocated, but seems to not be used.

> +	size_t received_srm_sz;

Seems to be unused. Seems to both be leftovers from the sysfs interface.

> +	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)
> +{
> +	DRM_DEBUG("\t%#02x, %#02x, %#02x, %#02x, %#02x\n",
> +		  ksv[0], ksv[1], ksv[2], ksv[3], ksv[4]);
> +}
> +
> +static u32 drm_hdcp_get_revoked_ksv_count(const u8 *buf, u32 vrls_length)
> +{
> +	u32 parsed_bytes = 0, ksv_count = 0, vrl_ksv_cnt, vrl_sz;
> +
> +	while (parsed_bytes < vrls_length) {
> +		vrl_ksv_cnt = *buf;
> +		ksv_count += vrl_ksv_cnt;
> +
> +		vrl_sz = (vrl_ksv_cnt * DRM_HDCP_KSV_LEN) + 1;
> +		buf += vrl_sz;
> +		parsed_bytes += vrl_sz;
> +	}
> +
> +	/*
> +	 * When vrls are not valid, ksvs are not considered.
> +	 * Hence SRM will be discarded.
> +	 */
> +	if (parsed_bytes != vrls_length)
> +		ksv_count = 0;
> +
> +	return ksv_count;
> +}
> +
> +static u32 drm_hdcp_get_revoked_ksvs(const u8 *buf, u8 *revoked_ksv_list,
> +				     u32 vrls_length)
> +{
> +	u32 parsed_bytes = 0, ksv_count = 0;
> +	u32 vrl_ksv_cnt, vrl_ksv_sz, vrl_idx = 0;
> +
> +	do {
> +		vrl_ksv_cnt = *buf;
> +		vrl_ksv_sz = vrl_ksv_cnt * DRM_HDCP_KSV_LEN;
> +
> +		buf++;
> +
> +		DRM_DEBUG("vrl: %d, Revoked KSVs: %d\n", vrl_idx++,
> +			  vrl_ksv_cnt);
> +		memcpy(revoked_ksv_list, buf, vrl_ksv_sz);
> +
> +		ksv_count += vrl_ksv_cnt;
> +		revoked_ksv_list += vrl_ksv_sz;
> +		buf += vrl_ksv_sz;
> +
> +		parsed_bytes += (vrl_ksv_sz + 1);
> +	} while (parsed_bytes < vrls_length);
> +
> +	return ksv_count;
> +}
> +
> +static inline u32 get_vrl_length(const u8 *buf)
> +{
> +	return (u32)(buf[0] << 16 | buf[1] << 8 | buf[2]);

This is the same conversion as drm_hdcp2_seq_num_to_u32. Maybe rename that
to drm_hdcp_be24_bytes_to_cpu and use that instead of what you have hare.
Plus rename the other function to drm_hdcp_u32_to_b24_bytes for
consistency. Probably best to do that in a small prep patch.
> +}
> +
> +static int drm_hdcp_parse_hdcp1_srm(const u8 *buf, size_t count)
> +{
> +	struct hdcp_srm_header *header;
> +	u32 vrl_length, ksv_count;
> +
> +	if (count < (sizeof(struct hdcp_srm_header) +
> +	    DRM_HDCP_1_4_VRL_LENGTH_SIZE + DRM_HDCP_1_4_DCP_SIG_SIZE)) {
> +		DRM_ERROR("Invalid blob length\n");
> +		return -EINVAL;
> +	}
> +
> +	header = (struct hdcp_srm_header *)buf;
> +	DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
> +		  header->srm_id,
> +		  __swab16(header->srm_version), header->srm_gen_no);

s/__swab16/be16_to_cpu/ everywhere. Otherwise it'll stop working on big
endian cpus. Yes won't matter, but better correct, it's also better
self-documenting code.


> +
> +	WARN_ON(header->reserved);
> +
> +	buf = buf + sizeof(*header);
> +	vrl_length = get_vrl_length(buf);
> +	if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
> +	    vrl_length < (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
> +			  DRM_HDCP_1_4_DCP_SIG_SIZE)) {
> +		DRM_ERROR("Invalid blob length or vrl length\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Length of the all vrls combined */
> +	vrl_length -= (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
> +		       DRM_HDCP_1_4_DCP_SIG_SIZE);
> +
> +	if (!vrl_length) {
> +		DRM_ERROR("No vrl found\n");
> +		return -EINVAL;
> +	}
> +
> +	buf += DRM_HDCP_1_4_VRL_LENGTH_SIZE;
> +	ksv_count = drm_hdcp_get_revoked_ksv_count(buf, vrl_length);
> +	if (!ksv_count) {
> +		DRM_DEBUG("Revoked KSV count is 0\n");
> +		return count;
> +	}
> +
> +	kfree(srm_data->revoked_ksv_list);
> +	srm_data->revoked_ksv_list = kcalloc(ksv_count, DRM_HDCP_KSV_LEN,
> +					     GFP_KERNEL);
> +	if (!srm_data->revoked_ksv_list) {
> +		DRM_ERROR("Out of Memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	if (drm_hdcp_get_revoked_ksvs(buf, srm_data->revoked_ksv_list,
> +				      vrl_length) != ksv_count) {
> +		srm_data->revoked_ksv_cnt = 0;
> +		kfree(srm_data->revoked_ksv_list);
> +		return -EINVAL;
> +	}
> +
> +	srm_data->revoked_ksv_cnt = ksv_count;
> +	return count;
> +}
> +
> +static int drm_hdcp_parse_hdcp2_srm(const u8 *buf, size_t count)
> +{
> +	struct hdcp_srm_header *header;
> +	u32 vrl_length, ksv_count, ksv_sz;
> +
> +	if (count < (sizeof(struct hdcp_srm_header) +
> +	    DRM_HDCP_2_VRL_LENGTH_SIZE + DRM_HDCP_2_DCP_SIG_SIZE)) {
> +		DRM_ERROR("Invalid blob length\n");
> +		return -EINVAL;
> +	}
> +
> +	header = (struct hdcp_srm_header *)buf;
> +	DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
> +		  header->srm_id & DRM_HDCP_SRM_ID_MASK,
> +		  __swab16(header->srm_version), header->srm_gen_no);
> +
> +	if (header->reserved)
> +		return -EINVAL;
> +
> +	buf = buf + sizeof(*header);
> +	vrl_length = get_vrl_length(buf);
> +
> +	if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
> +	    vrl_length < (DRM_HDCP_2_VRL_LENGTH_SIZE +
> +	    DRM_HDCP_2_DCP_SIG_SIZE)) {
> +		DRM_ERROR("Invalid blob length or vrl length\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Length of the all vrls combined */
> +	vrl_length -= (DRM_HDCP_2_VRL_LENGTH_SIZE +
> +		       DRM_HDCP_2_DCP_SIG_SIZE);
> +
> +	if (!vrl_length) {
> +		DRM_ERROR("No vrl found\n");
> +		return -EINVAL;
> +	}
> +
> +	buf += DRM_HDCP_2_VRL_LENGTH_SIZE;
> +	ksv_count = (*buf << 2) | DRM_HDCP_2_KSV_COUNT_2_LSBITS(*(buf + 1));
> +	if (!ksv_count) {
> +		DRM_DEBUG("Revoked KSV count is 0\n");
> +		return count;
> +	}
> +
> +	kfree(srm_data->revoked_ksv_list);
> +	srm_data->revoked_ksv_list = kcalloc(ksv_count, DRM_HDCP_KSV_LEN,
> +					     GFP_KERNEL);
> +	if (!srm_data->revoked_ksv_list) {
> +		DRM_ERROR("Out of Memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	ksv_sz = ksv_count * DRM_HDCP_KSV_LEN;
> +	buf += DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ;
> +
> +	DRM_DEBUG("Revoked KSVs: %d\n", ksv_count);
> +	memcpy(srm_data->revoked_ksv_list, buf, ksv_sz);
> +
> +	srm_data->revoked_ksv_cnt = ksv_count;
> +	return count;
> +}
> +
> +static inline bool is_srm_version_hdcp1(const u8 *buf)
> +{
> +	return *buf == (u8)(DRM_HDCP_1_4_SRM_ID << 4);
> +}
> +
> +static inline bool is_srm_version_hdcp2(const u8 *buf)
> +{
> +	return *buf == (u8)(DRM_HDCP_2_SRM_ID << 4 | DRM_HDCP_2_INDICATOR);
> +}
> +
> +static ssize_t drm_hdcp_srm_update(const u8 *buf, size_t count)
> +{
> +	if (is_srm_version_hdcp1(buf))
> +		return (ssize_t)drm_hdcp_parse_hdcp1_srm(buf, count);
> +	else if (is_srm_version_hdcp2(buf))
> +		return (ssize_t)drm_hdcp_parse_hdcp2_srm(buf, count);
> +
> +	return (ssize_t)-EINVAL;

You wire return values through but then throw them away. Plus there's a
confusion between int and ssize_t. Since you have DRM_DEBUG everywhere
already and we don't care about any parsing errors anyyway I think best to
throw the return values away and use void for all these functions.

> +}
> +
> +void drm_hdcp_request_srm(struct drm_device *drm_dev)
> +{
> +	char fw_name[36] = "display_hdcp_srm.bin";
> +	const struct firmware *fw;
> +
> +	int ret;
> +
> +	ret = request_firmware_direct(&fw, (const char *)fw_name,
> +				      drm_dev->dev);
> +	if (ret < 0)
> +		goto exit;
> +
> +	if (fw->size && fw->data)
> +		drm_hdcp_srm_update(fw->data, fw->size);
> +
> +exit:
> +	release_firmware(fw);
> +}
> +
> +/**
> + * drm_hdcp_check_ksvs_revoked - Check the revoked status of the IDs
> + *
> + * @drm_dev: drm_device for which HDCP revocation check is requested
> + * @ksvs: List of KSVs (HDCP receiver IDs)
> + * @ksv_count: KSV count passed in through @ksvs
> + *
> + * This function reads the HDCP System renewability Message(SRM Table)
> + * from userspace as a firmware and parses it for the revoked HDCP
> + * KSVs(Receiver IDs) detected by DCP LLC. Once the revoked KSVs are known,
> + * revoked state of the KSVs in the list passed in by display drivers are
> + * decided and response is sent.
> + *
> + * SRM should be presented in the name of "display_hdcp_srm.bin".
> + *
> + * Returns:
> + * TRUE on any of the KSV is revoked, else FALSE.
> + */
> +bool drm_hdcp_check_ksvs_revoked(struct drm_device *drm_dev, u8 *ksvs,
> +				 u32 ksv_count)
> +{
> +	u32 rev_ksv_cnt, cnt, i, j;
> +	u8 *rev_ksv_list;
> +
> +	if (!srm_data)
> +		return false;
> +
> +	mutex_lock(&srm_data->mutex);
> +	drm_hdcp_request_srm(drm_dev);
> +
> +	rev_ksv_cnt = srm_data->revoked_ksv_cnt;
> +	rev_ksv_list = srm_data->revoked_ksv_list;
> +
> +	/* If the Revoked ksv list is empty */
> +	if (!rev_ksv_cnt || !rev_ksv_list) {
> +		mutex_unlock(&srm_data->mutex);
> +		return false;
> +	}
> +
> +	for  (cnt = 0; cnt < ksv_count; cnt++) {
> +		rev_ksv_list = srm_data->revoked_ksv_list;
> +		for (i = 0; i < rev_ksv_cnt; i++) {
> +			for (j = 0; j < DRM_HDCP_KSV_LEN; j++)
> +				if (ksvs[j] != rev_ksv_list[j]) {
> +					break;
> +				} else if (j == (DRM_HDCP_KSV_LEN - 1)) {
> +					DRM_DEBUG("Revoked KSV is ");
> +					drm_hdcp_print_ksv(ksvs);
> +					mutex_unlock(&srm_data->mutex);
> +					return true;
> +				}
> +			/* Move the offset to next KSV in the revoked list */
> +			rev_ksv_list += DRM_HDCP_KSV_LEN;
> +		}
> +
> +		/* Iterate to next ksv_offset */
> +		ksvs += DRM_HDCP_KSV_LEN;
> +	}
> +	mutex_unlock(&srm_data->mutex);
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(drm_hdcp_check_ksvs_revoked);
> +
> +int drm_setup_hdcp_srm(struct class *drm_class)
> +{
> +	srm_data = kzalloc(sizeof(*srm_data), GFP_KERNEL);
> +	if (!srm_data)
> +		return -ENOMEM;
> +
> +	srm_data->srm_buf = kcalloc(DRM_HDCP_SRM_GEN1_MAX_BYTES,
> +				    sizeof(u8), GFP_KERNEL);
> +	if (!srm_data->srm_buf) {
> +		kfree(srm_data);
> +		return -ENOMEM;
> +	}
> +	mutex_init(&srm_data->mutex);
> +
> +	return 0;
> +}
> +
> +void drm_teardown_hdcp_srm(struct class *drm_class)
> +{
> +	if (srm_data) {
> +		kfree(srm_data->srm_buf);
> +		kfree(srm_data->revoked_ksv_list);
> +		kfree(srm_data);
> +	}
> +}
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index e19ac7ca602d..476a422414f6 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -201,3 +201,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>  void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
>  				const struct drm_framebuffer *fb);
>  int drm_framebuffer_debugfs_init(struct drm_minor *minor);
> +
> +/* drm_hdcp.c */
> +int drm_setup_hdcp_srm(struct class *drm_class);
> +void drm_teardown_hdcp_srm(struct class *drm_class);
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index ecb7b33002bb..18b1ac442997 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -78,6 +78,7 @@ int drm_sysfs_init(void)
>  	}
>  
>  	drm_class->devnode = drm_devnode;
> +	drm_setup_hdcp_srm(drm_class);
>  	return 0;
>  }
>  
> @@ -90,6 +91,7 @@ void drm_sysfs_destroy(void)
>  {
>  	if (IS_ERR_OR_NULL(drm_class))
>  		return;
> +	drm_teardown_hdcp_srm(drm_class);
>  	class_remove_file(drm_class, &class_attr_version.attr);
>  	class_destroy(drm_class);
>  	drm_class = NULL;
> diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> index f243408ecf26..292998a2497f 100644
> --- a/include/drm/drm_hdcp.h
> +++ b/include/drm/drm_hdcp.h
> @@ -265,4 +265,28 @@ void drm_hdcp2_u32_to_seq_num(u8 seq_num[HDCP_2_2_SEQ_NUM_LEN], u32 val)
>  	seq_num[2] = val;
>  }
>  
> +#define DRM_HDCP_SRM_GEN1_MAX_BYTES		(5 * 1024)
> +#define DRM_HDCP_1_4_SRM_ID			0x8
> +#define DRM_HDCP_SRM_ID_MASK			(0xF << 4)

This is surprising (I'd have expected the hdcp2 indicator in the low bits)
from reading the spec. But I guess you tested this, and there's some
mumbo-jumbo about "everything is big endian", which might also mean
nibbles. Who knows.

> +#define DRM_HDCP_1_4_VRL_LENGTH_SIZE		3
> +#define DRM_HDCP_1_4_DCP_SIG_SIZE		40
> +#define DRM_HDCP_2_SRM_ID			0x9
> +#define DRM_HDCP_2_INDICATOR			0x1
> +#define DRM_HDCP_2_INDICATOR_MASK		0xF
> +#define DRM_HDCP_2_VRL_LENGTH_SIZE		3
> +#define DRM_HDCP_2_DCP_SIG_SIZE			384
> +#define DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ	4
> +#define DRM_HDCP_2_KSV_COUNT_2_LSBITS(byte)	(((byte) & 0xC) >> 6)
> +
> +struct hdcp_srm_header {
> +	u8 srm_id;
> +	u8 reserved;
> +	u16 srm_version;

Needs to be __be16, so that sparse can validate you got all the byte
swapping correct. Would be good to double-check with sparse yourself
(otherwise 0day will do it for you, but if there's a problem better to
catch it early).

> +	u8 srm_gen_no;
> +} __packed;
> +
> +struct drm_device;
> +
> +bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,
> +				 u8 *ksvs, u32 ksv_count);
>  #endif
> -- 
> 2.19.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, May 06, 2019 at 06:56:03PM +0200, Daniel Vetter wrote:
> On Thu, May 02, 2019 at 06:52:56PM +0530, Ramalingam C wrote:
> > On every hdcp revocation check request SRM is read from fw file
> > /lib/firmware/display_hdcp_srm.bin
> > 
> > SRM table is parsed and stored at drm_hdcp.c, with functions exported
> > for the services for revocation check from drivers (which
> > implements the HDCP authentication)
> > 
> > This patch handles the HDCP1.4 and 2.2 versions of SRM table.
> > 
> > v2:
> >   moved the uAPI to request_firmware_direct() [Daniel]
> > v3:
> >   kdoc added. [Daniel]
> >   srm_header unified and bit field definitions are removed. [Daniel]
> >   locking improved. [Daniel]
> >   vrl length violation is fixed. [Daniel]
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> 
> Found a few small details to polish, but looks good to me. With the
> details addressed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> We also still need an ack on the firmware blob approach from Matt Roper or
> someone else at iotg I think.

+Satyeshwar

Satyeshwar's probably the best person from IOTG to give the Ack since
he's part of the group that needs this functionality and is involved in
the userspace/compositor side as well.


Matt

> 
> Cheers, Daniel
> 
> > ---
> >  Documentation/gpu/drm-kms-helpers.rst |   6 +
> >  drivers/gpu/drm/Makefile              |   2 +-
> >  drivers/gpu/drm/drm_hdcp.c            | 342 ++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_internal.h        |   4 +
> >  drivers/gpu/drm/drm_sysfs.c           |   2 +
> >  include/drm/drm_hdcp.h                |  24 ++
> >  6 files changed, 379 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/drm_hdcp.c
> > 
> > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> > index 14102ae035dc..0fe726a6ee67 100644
> > --- a/Documentation/gpu/drm-kms-helpers.rst
> > +++ b/Documentation/gpu/drm-kms-helpers.rst
> > @@ -181,6 +181,12 @@ Panel Helper Reference
> >  .. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c
> >     :export:
> >  
> > +HDCP Helper Functions Reference
> > +===============================
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_hdcp.c
> > +   :export:
> > +
> >  Display Port Helper Functions Reference
> >  =======================================
> >  
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 72f5036d9bfa..dd02e9dec810 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -17,7 +17,7 @@ drm-y       :=	drm_auth.o drm_cache.o \
> >  		drm_plane.o drm_color_mgmt.o drm_print.o \
> >  		drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> >  		drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > -		drm_atomic_uapi.o
> > +		drm_atomic_uapi.o drm_hdcp.o
> >  
> >  drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o drm_dma.o drm_scatter.o drm_lock.o
> >  drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> > new file mode 100644
> > index 000000000000..dc0e13409221
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_hdcp.c
> > @@ -0,0 +1,342 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 Intel Corporation.
> > + *
> > + * Authors:
> > + * Ramalingam C <ramalingam.c@intel.com>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/gfp.h>
> > +#include <linux/export.h>
> > +#include <linux/slab.h>
> > +#include <linux/firmware.h>
> > +
> > +#include <drm/drm_hdcp.h>
> > +#include <drm/drm_sysfs.h>
> > +#include <drm/drm_print.h>
> > +#include <drm/drm_device.h>
> > +
> > +struct hdcp_srm {
> > +	u8 *srm_buf;
> 
> Allocated, but seems to not be used.
> 
> > +	size_t received_srm_sz;
> 
> Seems to be unused. Seems to both be leftovers from the sysfs interface.
> 
> > +	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)
> > +{
> > +	DRM_DEBUG("\t%#02x, %#02x, %#02x, %#02x, %#02x\n",
> > +		  ksv[0], ksv[1], ksv[2], ksv[3], ksv[4]);
> > +}
> > +
> > +static u32 drm_hdcp_get_revoked_ksv_count(const u8 *buf, u32 vrls_length)
> > +{
> > +	u32 parsed_bytes = 0, ksv_count = 0, vrl_ksv_cnt, vrl_sz;
> > +
> > +	while (parsed_bytes < vrls_length) {
> > +		vrl_ksv_cnt = *buf;
> > +		ksv_count += vrl_ksv_cnt;
> > +
> > +		vrl_sz = (vrl_ksv_cnt * DRM_HDCP_KSV_LEN) + 1;
> > +		buf += vrl_sz;
> > +		parsed_bytes += vrl_sz;
> > +	}
> > +
> > +	/*
> > +	 * When vrls are not valid, ksvs are not considered.
> > +	 * Hence SRM will be discarded.
> > +	 */
> > +	if (parsed_bytes != vrls_length)
> > +		ksv_count = 0;
> > +
> > +	return ksv_count;
> > +}
> > +
> > +static u32 drm_hdcp_get_revoked_ksvs(const u8 *buf, u8 *revoked_ksv_list,
> > +				     u32 vrls_length)
> > +{
> > +	u32 parsed_bytes = 0, ksv_count = 0;
> > +	u32 vrl_ksv_cnt, vrl_ksv_sz, vrl_idx = 0;
> > +
> > +	do {
> > +		vrl_ksv_cnt = *buf;
> > +		vrl_ksv_sz = vrl_ksv_cnt * DRM_HDCP_KSV_LEN;
> > +
> > +		buf++;
> > +
> > +		DRM_DEBUG("vrl: %d, Revoked KSVs: %d\n", vrl_idx++,
> > +			  vrl_ksv_cnt);
> > +		memcpy(revoked_ksv_list, buf, vrl_ksv_sz);
> > +
> > +		ksv_count += vrl_ksv_cnt;
> > +		revoked_ksv_list += vrl_ksv_sz;
> > +		buf += vrl_ksv_sz;
> > +
> > +		parsed_bytes += (vrl_ksv_sz + 1);
> > +	} while (parsed_bytes < vrls_length);
> > +
> > +	return ksv_count;
> > +}
> > +
> > +static inline u32 get_vrl_length(const u8 *buf)
> > +{
> > +	return (u32)(buf[0] << 16 | buf[1] << 8 | buf[2]);
> 
> This is the same conversion as drm_hdcp2_seq_num_to_u32. Maybe rename that
> to drm_hdcp_be24_bytes_to_cpu and use that instead of what you have hare.
> Plus rename the other function to drm_hdcp_u32_to_b24_bytes for
> consistency. Probably best to do that in a small prep patch.
> > +}
> > +
> > +static int drm_hdcp_parse_hdcp1_srm(const u8 *buf, size_t count)
> > +{
> > +	struct hdcp_srm_header *header;
> > +	u32 vrl_length, ksv_count;
> > +
> > +	if (count < (sizeof(struct hdcp_srm_header) +
> > +	    DRM_HDCP_1_4_VRL_LENGTH_SIZE + DRM_HDCP_1_4_DCP_SIG_SIZE)) {
> > +		DRM_ERROR("Invalid blob length\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	header = (struct hdcp_srm_header *)buf;
> > +	DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
> > +		  header->srm_id,
> > +		  __swab16(header->srm_version), header->srm_gen_no);
> 
> s/__swab16/be16_to_cpu/ everywhere. Otherwise it'll stop working on big
> endian cpus. Yes won't matter, but better correct, it's also better
> self-documenting code.
> 
> 
> > +
> > +	WARN_ON(header->reserved);
> > +
> > +	buf = buf + sizeof(*header);
> > +	vrl_length = get_vrl_length(buf);
> > +	if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
> > +	    vrl_length < (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
> > +			  DRM_HDCP_1_4_DCP_SIG_SIZE)) {
> > +		DRM_ERROR("Invalid blob length or vrl length\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Length of the all vrls combined */
> > +	vrl_length -= (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
> > +		       DRM_HDCP_1_4_DCP_SIG_SIZE);
> > +
> > +	if (!vrl_length) {
> > +		DRM_ERROR("No vrl found\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	buf += DRM_HDCP_1_4_VRL_LENGTH_SIZE;
> > +	ksv_count = drm_hdcp_get_revoked_ksv_count(buf, vrl_length);
> > +	if (!ksv_count) {
> > +		DRM_DEBUG("Revoked KSV count is 0\n");
> > +		return count;
> > +	}
> > +
> > +	kfree(srm_data->revoked_ksv_list);
> > +	srm_data->revoked_ksv_list = kcalloc(ksv_count, DRM_HDCP_KSV_LEN,
> > +					     GFP_KERNEL);
> > +	if (!srm_data->revoked_ksv_list) {
> > +		DRM_ERROR("Out of Memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	if (drm_hdcp_get_revoked_ksvs(buf, srm_data->revoked_ksv_list,
> > +				      vrl_length) != ksv_count) {
> > +		srm_data->revoked_ksv_cnt = 0;
> > +		kfree(srm_data->revoked_ksv_list);
> > +		return -EINVAL;
> > +	}
> > +
> > +	srm_data->revoked_ksv_cnt = ksv_count;
> > +	return count;
> > +}
> > +
> > +static int drm_hdcp_parse_hdcp2_srm(const u8 *buf, size_t count)
> > +{
> > +	struct hdcp_srm_header *header;
> > +	u32 vrl_length, ksv_count, ksv_sz;
> > +
> > +	if (count < (sizeof(struct hdcp_srm_header) +
> > +	    DRM_HDCP_2_VRL_LENGTH_SIZE + DRM_HDCP_2_DCP_SIG_SIZE)) {
> > +		DRM_ERROR("Invalid blob length\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	header = (struct hdcp_srm_header *)buf;
> > +	DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
> > +		  header->srm_id & DRM_HDCP_SRM_ID_MASK,
> > +		  __swab16(header->srm_version), header->srm_gen_no);
> > +
> > +	if (header->reserved)
> > +		return -EINVAL;
> > +
> > +	buf = buf + sizeof(*header);
> > +	vrl_length = get_vrl_length(buf);
> > +
> > +	if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
> > +	    vrl_length < (DRM_HDCP_2_VRL_LENGTH_SIZE +
> > +	    DRM_HDCP_2_DCP_SIG_SIZE)) {
> > +		DRM_ERROR("Invalid blob length or vrl length\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Length of the all vrls combined */
> > +	vrl_length -= (DRM_HDCP_2_VRL_LENGTH_SIZE +
> > +		       DRM_HDCP_2_DCP_SIG_SIZE);
> > +
> > +	if (!vrl_length) {
> > +		DRM_ERROR("No vrl found\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	buf += DRM_HDCP_2_VRL_LENGTH_SIZE;
> > +	ksv_count = (*buf << 2) | DRM_HDCP_2_KSV_COUNT_2_LSBITS(*(buf + 1));
> > +	if (!ksv_count) {
> > +		DRM_DEBUG("Revoked KSV count is 0\n");
> > +		return count;
> > +	}
> > +
> > +	kfree(srm_data->revoked_ksv_list);
> > +	srm_data->revoked_ksv_list = kcalloc(ksv_count, DRM_HDCP_KSV_LEN,
> > +					     GFP_KERNEL);
> > +	if (!srm_data->revoked_ksv_list) {
> > +		DRM_ERROR("Out of Memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ksv_sz = ksv_count * DRM_HDCP_KSV_LEN;
> > +	buf += DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ;
> > +
> > +	DRM_DEBUG("Revoked KSVs: %d\n", ksv_count);
> > +	memcpy(srm_data->revoked_ksv_list, buf, ksv_sz);
> > +
> > +	srm_data->revoked_ksv_cnt = ksv_count;
> > +	return count;
> > +}
> > +
> > +static inline bool is_srm_version_hdcp1(const u8 *buf)
> > +{
> > +	return *buf == (u8)(DRM_HDCP_1_4_SRM_ID << 4);
> > +}
> > +
> > +static inline bool is_srm_version_hdcp2(const u8 *buf)
> > +{
> > +	return *buf == (u8)(DRM_HDCP_2_SRM_ID << 4 | DRM_HDCP_2_INDICATOR);
> > +}
> > +
> > +static ssize_t drm_hdcp_srm_update(const u8 *buf, size_t count)
> > +{
> > +	if (is_srm_version_hdcp1(buf))
> > +		return (ssize_t)drm_hdcp_parse_hdcp1_srm(buf, count);
> > +	else if (is_srm_version_hdcp2(buf))
> > +		return (ssize_t)drm_hdcp_parse_hdcp2_srm(buf, count);
> > +
> > +	return (ssize_t)-EINVAL;
> 
> You wire return values through but then throw them away. Plus there's a
> confusion between int and ssize_t. Since you have DRM_DEBUG everywhere
> already and we don't care about any parsing errors anyyway I think best to
> throw the return values away and use void for all these functions.
> 
> > +}
> > +
> > +void drm_hdcp_request_srm(struct drm_device *drm_dev)
> > +{
> > +	char fw_name[36] = "display_hdcp_srm.bin";
> > +	const struct firmware *fw;
> > +
> > +	int ret;
> > +
> > +	ret = request_firmware_direct(&fw, (const char *)fw_name,
> > +				      drm_dev->dev);
> > +	if (ret < 0)
> > +		goto exit;
> > +
> > +	if (fw->size && fw->data)
> > +		drm_hdcp_srm_update(fw->data, fw->size);
> > +
> > +exit:
> > +	release_firmware(fw);
> > +}
> > +
> > +/**
> > + * drm_hdcp_check_ksvs_revoked - Check the revoked status of the IDs
> > + *
> > + * @drm_dev: drm_device for which HDCP revocation check is requested
> > + * @ksvs: List of KSVs (HDCP receiver IDs)
> > + * @ksv_count: KSV count passed in through @ksvs
> > + *
> > + * This function reads the HDCP System renewability Message(SRM Table)
> > + * from userspace as a firmware and parses it for the revoked HDCP
> > + * KSVs(Receiver IDs) detected by DCP LLC. Once the revoked KSVs are known,
> > + * revoked state of the KSVs in the list passed in by display drivers are
> > + * decided and response is sent.
> > + *
> > + * SRM should be presented in the name of "display_hdcp_srm.bin".
> > + *
> > + * Returns:
> > + * TRUE on any of the KSV is revoked, else FALSE.
> > + */
> > +bool drm_hdcp_check_ksvs_revoked(struct drm_device *drm_dev, u8 *ksvs,
> > +				 u32 ksv_count)
> > +{
> > +	u32 rev_ksv_cnt, cnt, i, j;
> > +	u8 *rev_ksv_list;
> > +
> > +	if (!srm_data)
> > +		return false;
> > +
> > +	mutex_lock(&srm_data->mutex);
> > +	drm_hdcp_request_srm(drm_dev);
> > +
> > +	rev_ksv_cnt = srm_data->revoked_ksv_cnt;
> > +	rev_ksv_list = srm_data->revoked_ksv_list;
> > +
> > +	/* If the Revoked ksv list is empty */
> > +	if (!rev_ksv_cnt || !rev_ksv_list) {
> > +		mutex_unlock(&srm_data->mutex);
> > +		return false;
> > +	}
> > +
> > +	for  (cnt = 0; cnt < ksv_count; cnt++) {
> > +		rev_ksv_list = srm_data->revoked_ksv_list;
> > +		for (i = 0; i < rev_ksv_cnt; i++) {
> > +			for (j = 0; j < DRM_HDCP_KSV_LEN; j++)
> > +				if (ksvs[j] != rev_ksv_list[j]) {
> > +					break;
> > +				} else if (j == (DRM_HDCP_KSV_LEN - 1)) {
> > +					DRM_DEBUG("Revoked KSV is ");
> > +					drm_hdcp_print_ksv(ksvs);
> > +					mutex_unlock(&srm_data->mutex);
> > +					return true;
> > +				}
> > +			/* Move the offset to next KSV in the revoked list */
> > +			rev_ksv_list += DRM_HDCP_KSV_LEN;
> > +		}
> > +
> > +		/* Iterate to next ksv_offset */
> > +		ksvs += DRM_HDCP_KSV_LEN;
> > +	}
> > +	mutex_unlock(&srm_data->mutex);
> > +	return false;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_hdcp_check_ksvs_revoked);
> > +
> > +int drm_setup_hdcp_srm(struct class *drm_class)
> > +{
> > +	srm_data = kzalloc(sizeof(*srm_data), GFP_KERNEL);
> > +	if (!srm_data)
> > +		return -ENOMEM;
> > +
> > +	srm_data->srm_buf = kcalloc(DRM_HDCP_SRM_GEN1_MAX_BYTES,
> > +				    sizeof(u8), GFP_KERNEL);
> > +	if (!srm_data->srm_buf) {
> > +		kfree(srm_data);
> > +		return -ENOMEM;
> > +	}
> > +	mutex_init(&srm_data->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +void drm_teardown_hdcp_srm(struct class *drm_class)
> > +{
> > +	if (srm_data) {
> > +		kfree(srm_data->srm_buf);
> > +		kfree(srm_data->revoked_ksv_list);
> > +		kfree(srm_data);
> > +	}
> > +}
> > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > index e19ac7ca602d..476a422414f6 100644
> > --- a/drivers/gpu/drm/drm_internal.h
> > +++ b/drivers/gpu/drm/drm_internal.h
> > @@ -201,3 +201,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> >  void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
> >  				const struct drm_framebuffer *fb);
> >  int drm_framebuffer_debugfs_init(struct drm_minor *minor);
> > +
> > +/* drm_hdcp.c */
> > +int drm_setup_hdcp_srm(struct class *drm_class);
> > +void drm_teardown_hdcp_srm(struct class *drm_class);
> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > index ecb7b33002bb..18b1ac442997 100644
> > --- a/drivers/gpu/drm/drm_sysfs.c
> > +++ b/drivers/gpu/drm/drm_sysfs.c
> > @@ -78,6 +78,7 @@ int drm_sysfs_init(void)
> >  	}
> >  
> >  	drm_class->devnode = drm_devnode;
> > +	drm_setup_hdcp_srm(drm_class);
> >  	return 0;
> >  }
> >  
> > @@ -90,6 +91,7 @@ void drm_sysfs_destroy(void)
> >  {
> >  	if (IS_ERR_OR_NULL(drm_class))
> >  		return;
> > +	drm_teardown_hdcp_srm(drm_class);
> >  	class_remove_file(drm_class, &class_attr_version.attr);
> >  	class_destroy(drm_class);
> >  	drm_class = NULL;
> > diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> > index f243408ecf26..292998a2497f 100644
> > --- a/include/drm/drm_hdcp.h
> > +++ b/include/drm/drm_hdcp.h
> > @@ -265,4 +265,28 @@ void drm_hdcp2_u32_to_seq_num(u8 seq_num[HDCP_2_2_SEQ_NUM_LEN], u32 val)
> >  	seq_num[2] = val;
> >  }
> >  
> > +#define DRM_HDCP_SRM_GEN1_MAX_BYTES		(5 * 1024)
> > +#define DRM_HDCP_1_4_SRM_ID			0x8
> > +#define DRM_HDCP_SRM_ID_MASK			(0xF << 4)
> 
> This is surprising (I'd have expected the hdcp2 indicator in the low bits)
> from reading the spec. But I guess you tested this, and there's some
> mumbo-jumbo about "everything is big endian", which might also mean
> nibbles. Who knows.
> 
> > +#define DRM_HDCP_1_4_VRL_LENGTH_SIZE		3
> > +#define DRM_HDCP_1_4_DCP_SIG_SIZE		40
> > +#define DRM_HDCP_2_SRM_ID			0x9
> > +#define DRM_HDCP_2_INDICATOR			0x1
> > +#define DRM_HDCP_2_INDICATOR_MASK		0xF
> > +#define DRM_HDCP_2_VRL_LENGTH_SIZE		3
> > +#define DRM_HDCP_2_DCP_SIG_SIZE			384
> > +#define DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ	4
> > +#define DRM_HDCP_2_KSV_COUNT_2_LSBITS(byte)	(((byte) & 0xC) >> 6)
> > +
> > +struct hdcp_srm_header {
> > +	u8 srm_id;
> > +	u8 reserved;
> > +	u16 srm_version;
> 
> Needs to be __be16, so that sparse can validate you got all the byte
> swapping correct. Would be good to double-check with sparse yourself
> (otherwise 0day will do it for you, but if there's a problem better to
> catch it early).
> 
> > +	u8 srm_gen_no;
> > +} __packed;
> > +
> > +struct drm_device;
> > +
> > +bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,
> > +				 u8 *ksvs, u32 ksv_count);
> >  #endif
> > -- 
> > 2.19.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Acked-by: Satyeshwar Singh <satyeshwar.singh@intel.com>

-----Original Message-----
From: Roper, Matthew D 
Sent: Monday, May 6, 2019 2:59 PM
To: Daniel Vetter <daniel@ffwll.ch>
Cc: C, Ramalingam <ramalingam.c@intel.com>; Vetter, Daniel <daniel.vetter@intel.com>; intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Singh, Satyeshwar <satyeshwar.singh@intel.com>
Subject: Re: [Intel-gfx] [PATCH v6 03/10] drm: revocation check at drm subsystem

On Mon, May 06, 2019 at 06:56:03PM +0200, Daniel Vetter wrote:
> On Thu, May 02, 2019 at 06:52:56PM +0530, Ramalingam C wrote:
> > On every hdcp revocation check request SRM is read from fw file 
> > /lib/firmware/display_hdcp_srm.bin
> > 
> > SRM table is parsed and stored at drm_hdcp.c, with functions 
> > exported for the services for revocation check from drivers (which 
> > implements the HDCP authentication)
> > 
> > This patch handles the HDCP1.4 and 2.2 versions of SRM table.
> > 
> > v2:
> >   moved the uAPI to request_firmware_direct() [Daniel]
> > v3:
> >   kdoc added. [Daniel]
> >   srm_header unified and bit field definitions are removed. [Daniel]
> >   locking improved. [Daniel]
> >   vrl length violation is fixed. [Daniel]
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> 
> Found a few small details to polish, but looks good to me. With the 
> details addressed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> We also still need an ack on the firmware blob approach from Matt 
> Roper or someone else at iotg I think.

+Satyeshwar

Satyeshwar's probably the best person from IOTG to give the Ack since he's part of the group that needs this functionality and is involved in the userspace/compositor side as well.


Matt

> 
> Cheers, Daniel
> 
> > ---
> >  Documentation/gpu/drm-kms-helpers.rst |   6 +
> >  drivers/gpu/drm/Makefile              |   2 +-
> >  drivers/gpu/drm/drm_hdcp.c            | 342 ++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_internal.h        |   4 +
> >  drivers/gpu/drm/drm_sysfs.c           |   2 +
> >  include/drm/drm_hdcp.h                |  24 ++
> >  6 files changed, 379 insertions(+), 1 deletion(-)  create mode 
> > 100644 drivers/gpu/drm/drm_hdcp.c
> > 
> > diff --git a/Documentation/gpu/drm-kms-helpers.rst 
> > b/Documentation/gpu/drm-kms-helpers.rst
> > index 14102ae035dc..0fe726a6ee67 100644
> > --- a/Documentation/gpu/drm-kms-helpers.rst
> > +++ b/Documentation/gpu/drm-kms-helpers.rst
> > @@ -181,6 +181,12 @@ Panel Helper Reference  .. kernel-doc:: 
> > drivers/gpu/drm/drm_panel_orientation_quirks.c
> >     :export:
> >  
> > +HDCP Helper Functions Reference
> > +===============================
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_hdcp.c
> > +   :export:
> > +
> >  Display Port Helper Functions Reference  
> > =======================================
> >  
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile 
> > index 72f5036d9bfa..dd02e9dec810 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -17,7 +17,7 @@ drm-y       :=	drm_auth.o drm_cache.o \
> >  		drm_plane.o drm_color_mgmt.o drm_print.o \
> >  		drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \
> >  		drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > -		drm_atomic_uapi.o
> > +		drm_atomic_uapi.o drm_hdcp.o
> >  
> >  drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o 
> > drm_context.o drm_dma.o drm_scatter.o drm_lock.o
> >  drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o diff --git 
> > a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c new file 
> > mode 100644 index 000000000000..dc0e13409221
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_hdcp.c
> > @@ -0,0 +1,342 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 Intel Corporation.
> > + *
> > + * Authors:
> > + * Ramalingam C <ramalingam.c@intel.com>  */
> > +
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/gfp.h>
> > +#include <linux/export.h>
> > +#include <linux/slab.h>
> > +#include <linux/firmware.h>
> > +
> > +#include <drm/drm_hdcp.h>
> > +#include <drm/drm_sysfs.h>
> > +#include <drm/drm_print.h>
> > +#include <drm/drm_device.h>
> > +
> > +struct hdcp_srm {
> > +	u8 *srm_buf;
> 
> Allocated, but seems to not be used.
> 
> > +	size_t received_srm_sz;
> 
> Seems to be unused. Seems to both be leftovers from the sysfs interface.
> 
> > +	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) {
> > +	DRM_DEBUG("\t%#02x, %#02x, %#02x, %#02x, %#02x\n",
> > +		  ksv[0], ksv[1], ksv[2], ksv[3], ksv[4]); }
> > +
> > +static u32 drm_hdcp_get_revoked_ksv_count(const u8 *buf, u32 
> > +vrls_length) {
> > +	u32 parsed_bytes = 0, ksv_count = 0, vrl_ksv_cnt, vrl_sz;
> > +
> > +	while (parsed_bytes < vrls_length) {
> > +		vrl_ksv_cnt = *buf;
> > +		ksv_count += vrl_ksv_cnt;
> > +
> > +		vrl_sz = (vrl_ksv_cnt * DRM_HDCP_KSV_LEN) + 1;
> > +		buf += vrl_sz;
> > +		parsed_bytes += vrl_sz;
> > +	}
> > +
> > +	/*
> > +	 * When vrls are not valid, ksvs are not considered.
> > +	 * Hence SRM will be discarded.
> > +	 */
> > +	if (parsed_bytes != vrls_length)
> > +		ksv_count = 0;
> > +
> > +	return ksv_count;
> > +}
> > +
> > +static u32 drm_hdcp_get_revoked_ksvs(const u8 *buf, u8 *revoked_ksv_list,
> > +				     u32 vrls_length)
> > +{
> > +	u32 parsed_bytes = 0, ksv_count = 0;
> > +	u32 vrl_ksv_cnt, vrl_ksv_sz, vrl_idx = 0;
> > +
> > +	do {
> > +		vrl_ksv_cnt = *buf;
> > +		vrl_ksv_sz = vrl_ksv_cnt * DRM_HDCP_KSV_LEN;
> > +
> > +		buf++;
> > +
> > +		DRM_DEBUG("vrl: %d, Revoked KSVs: %d\n", vrl_idx++,
> > +			  vrl_ksv_cnt);
> > +		memcpy(revoked_ksv_list, buf, vrl_ksv_sz);
> > +
> > +		ksv_count += vrl_ksv_cnt;
> > +		revoked_ksv_list += vrl_ksv_sz;
> > +		buf += vrl_ksv_sz;
> > +
> > +		parsed_bytes += (vrl_ksv_sz + 1);
> > +	} while (parsed_bytes < vrls_length);
> > +
> > +	return ksv_count;
> > +}
> > +
> > +static inline u32 get_vrl_length(const u8 *buf) {
> > +	return (u32)(buf[0] << 16 | buf[1] << 8 | buf[2]);
> 
> This is the same conversion as drm_hdcp2_seq_num_to_u32. Maybe rename 
> that to drm_hdcp_be24_bytes_to_cpu and use that instead of what you have hare.
> Plus rename the other function to drm_hdcp_u32_to_b24_bytes for 
> consistency. Probably best to do that in a small prep patch.
> > +}
> > +
> > +static int drm_hdcp_parse_hdcp1_srm(const u8 *buf, size_t count) {
> > +	struct hdcp_srm_header *header;
> > +	u32 vrl_length, ksv_count;
> > +
> > +	if (count < (sizeof(struct hdcp_srm_header) +
> > +	    DRM_HDCP_1_4_VRL_LENGTH_SIZE + DRM_HDCP_1_4_DCP_SIG_SIZE)) {
> > +		DRM_ERROR("Invalid blob length\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	header = (struct hdcp_srm_header *)buf;
> > +	DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
> > +		  header->srm_id,
> > +		  __swab16(header->srm_version), header->srm_gen_no);
> 
> s/__swab16/be16_to_cpu/ everywhere. Otherwise it'll stop working on 
> big endian cpus. Yes won't matter, but better correct, it's also 
> better self-documenting code.
> 
> 
> > +
> > +	WARN_ON(header->reserved);
> > +
> > +	buf = buf + sizeof(*header);
> > +	vrl_length = get_vrl_length(buf);
> > +	if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
> > +	    vrl_length < (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
> > +			  DRM_HDCP_1_4_DCP_SIG_SIZE)) {
> > +		DRM_ERROR("Invalid blob length or vrl length\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Length of the all vrls combined */
> > +	vrl_length -= (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
> > +		       DRM_HDCP_1_4_DCP_SIG_SIZE);
> > +
> > +	if (!vrl_length) {
> > +		DRM_ERROR("No vrl found\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	buf += DRM_HDCP_1_4_VRL_LENGTH_SIZE;
> > +	ksv_count = drm_hdcp_get_revoked_ksv_count(buf, vrl_length);
> > +	if (!ksv_count) {
> > +		DRM_DEBUG("Revoked KSV count is 0\n");
> > +		return count;
> > +	}
> > +
> > +	kfree(srm_data->revoked_ksv_list);
> > +	srm_data->revoked_ksv_list = kcalloc(ksv_count, DRM_HDCP_KSV_LEN,
> > +					     GFP_KERNEL);
> > +	if (!srm_data->revoked_ksv_list) {
> > +		DRM_ERROR("Out of Memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	if (drm_hdcp_get_revoked_ksvs(buf, srm_data->revoked_ksv_list,
> > +				      vrl_length) != ksv_count) {
> > +		srm_data->revoked_ksv_cnt = 0;
> > +		kfree(srm_data->revoked_ksv_list);
> > +		return -EINVAL;
> > +	}
> > +
> > +	srm_data->revoked_ksv_cnt = ksv_count;
> > +	return count;
> > +}
> > +
> > +static int drm_hdcp_parse_hdcp2_srm(const u8 *buf, size_t count) {
> > +	struct hdcp_srm_header *header;
> > +	u32 vrl_length, ksv_count, ksv_sz;
> > +
> > +	if (count < (sizeof(struct hdcp_srm_header) +
> > +	    DRM_HDCP_2_VRL_LENGTH_SIZE + DRM_HDCP_2_DCP_SIG_SIZE)) {
> > +		DRM_ERROR("Invalid blob length\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	header = (struct hdcp_srm_header *)buf;
> > +	DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",
> > +		  header->srm_id & DRM_HDCP_SRM_ID_MASK,
> > +		  __swab16(header->srm_version), header->srm_gen_no);
> > +
> > +	if (header->reserved)
> > +		return -EINVAL;
> > +
> > +	buf = buf + sizeof(*header);
> > +	vrl_length = get_vrl_length(buf);
> > +
> > +	if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||
> > +	    vrl_length < (DRM_HDCP_2_VRL_LENGTH_SIZE +
> > +	    DRM_HDCP_2_DCP_SIG_SIZE)) {
> > +		DRM_ERROR("Invalid blob length or vrl length\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Length of the all vrls combined */
> > +	vrl_length -= (DRM_HDCP_2_VRL_LENGTH_SIZE +
> > +		       DRM_HDCP_2_DCP_SIG_SIZE);
> > +
> > +	if (!vrl_length) {
> > +		DRM_ERROR("No vrl found\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	buf += DRM_HDCP_2_VRL_LENGTH_SIZE;
> > +	ksv_count = (*buf << 2) | DRM_HDCP_2_KSV_COUNT_2_LSBITS(*(buf + 1));
> > +	if (!ksv_count) {
> > +		DRM_DEBUG("Revoked KSV count is 0\n");
> > +		return count;
> > +	}
> > +
> > +	kfree(srm_data->revoked_ksv_list);
> > +	srm_data->revoked_ksv_list = kcalloc(ksv_count, DRM_HDCP_KSV_LEN,
> > +					     GFP_KERNEL);
> > +	if (!srm_data->revoked_ksv_list) {
> > +		DRM_ERROR("Out of Memory\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ksv_sz = ksv_count * DRM_HDCP_KSV_LEN;
> > +	buf += DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ;
> > +
> > +	DRM_DEBUG("Revoked KSVs: %d\n", ksv_count);
> > +	memcpy(srm_data->revoked_ksv_list, buf, ksv_sz);
> > +
> > +	srm_data->revoked_ksv_cnt = ksv_count;
> > +	return count;
> > +}
> > +
> > +static inline bool is_srm_version_hdcp1(const u8 *buf) {
> > +	return *buf == (u8)(DRM_HDCP_1_4_SRM_ID << 4); }
> > +
> > +static inline bool is_srm_version_hdcp2(const u8 *buf) {
> > +	return *buf == (u8)(DRM_HDCP_2_SRM_ID << 4 | 
> > +DRM_HDCP_2_INDICATOR); }
> > +
> > +static ssize_t drm_hdcp_srm_update(const u8 *buf, size_t count) {
> > +	if (is_srm_version_hdcp1(buf))
> > +		return (ssize_t)drm_hdcp_parse_hdcp1_srm(buf, count);
> > +	else if (is_srm_version_hdcp2(buf))
> > +		return (ssize_t)drm_hdcp_parse_hdcp2_srm(buf, count);
> > +
> > +	return (ssize_t)-EINVAL;
> 
> You wire return values through but then throw them away. Plus there's 
> a confusion between int and ssize_t. Since you have DRM_DEBUG 
> everywhere already and we don't care about any parsing errors anyyway 
> I think best to throw the return values away and use void for all these functions.
> 
> > +}
> > +
> > +void drm_hdcp_request_srm(struct drm_device *drm_dev) {
> > +	char fw_name[36] = "display_hdcp_srm.bin";
> > +	const struct firmware *fw;
> > +
> > +	int ret;
> > +
> > +	ret = request_firmware_direct(&fw, (const char *)fw_name,
> > +				      drm_dev->dev);
> > +	if (ret < 0)
> > +		goto exit;
> > +
> > +	if (fw->size && fw->data)
> > +		drm_hdcp_srm_update(fw->data, fw->size);
> > +
> > +exit:
> > +	release_firmware(fw);
> > +}
> > +
> > +/**
> > + * drm_hdcp_check_ksvs_revoked - Check the revoked status of the 
> > +IDs
> > + *
> > + * @drm_dev: drm_device for which HDCP revocation check is 
> > +requested
> > + * @ksvs: List of KSVs (HDCP receiver IDs)
> > + * @ksv_count: KSV count passed in through @ksvs
> > + *
> > + * This function reads the HDCP System renewability Message(SRM 
> > +Table)
> > + * from userspace as a firmware and parses it for the revoked HDCP
> > + * KSVs(Receiver IDs) detected by DCP LLC. Once the revoked KSVs 
> > +are known,
> > + * revoked state of the KSVs in the list passed in by display 
> > +drivers are
> > + * decided and response is sent.
> > + *
> > + * SRM should be presented in the name of "display_hdcp_srm.bin".
> > + *
> > + * Returns:
> > + * TRUE on any of the KSV is revoked, else FALSE.
> > + */
> > +bool drm_hdcp_check_ksvs_revoked(struct drm_device *drm_dev, u8 *ksvs,
> > +				 u32 ksv_count)
> > +{
> > +	u32 rev_ksv_cnt, cnt, i, j;
> > +	u8 *rev_ksv_list;
> > +
> > +	if (!srm_data)
> > +		return false;
> > +
> > +	mutex_lock(&srm_data->mutex);
> > +	drm_hdcp_request_srm(drm_dev);
> > +
> > +	rev_ksv_cnt = srm_data->revoked_ksv_cnt;
> > +	rev_ksv_list = srm_data->revoked_ksv_list;
> > +
> > +	/* If the Revoked ksv list is empty */
> > +	if (!rev_ksv_cnt || !rev_ksv_list) {
> > +		mutex_unlock(&srm_data->mutex);
> > +		return false;
> > +	}
> > +
> > +	for  (cnt = 0; cnt < ksv_count; cnt++) {
> > +		rev_ksv_list = srm_data->revoked_ksv_list;
> > +		for (i = 0; i < rev_ksv_cnt; i++) {
> > +			for (j = 0; j < DRM_HDCP_KSV_LEN; j++)
> > +				if (ksvs[j] != rev_ksv_list[j]) {
> > +					break;
> > +				} else if (j == (DRM_HDCP_KSV_LEN - 1)) {
> > +					DRM_DEBUG("Revoked KSV is ");
> > +					drm_hdcp_print_ksv(ksvs);
> > +					mutex_unlock(&srm_data->mutex);
> > +					return true;
> > +				}
> > +			/* Move the offset to next KSV in the revoked list */
> > +			rev_ksv_list += DRM_HDCP_KSV_LEN;
> > +		}
> > +
> > +		/* Iterate to next ksv_offset */
> > +		ksvs += DRM_HDCP_KSV_LEN;
> > +	}
> > +	mutex_unlock(&srm_data->mutex);
> > +	return false;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_hdcp_check_ksvs_revoked);
> > +
> > +int drm_setup_hdcp_srm(struct class *drm_class) {
> > +	srm_data = kzalloc(sizeof(*srm_data), GFP_KERNEL);
> > +	if (!srm_data)
> > +		return -ENOMEM;
> > +
> > +	srm_data->srm_buf = kcalloc(DRM_HDCP_SRM_GEN1_MAX_BYTES,
> > +				    sizeof(u8), GFP_KERNEL);
> > +	if (!srm_data->srm_buf) {
> > +		kfree(srm_data);
> > +		return -ENOMEM;
> > +	}
> > +	mutex_init(&srm_data->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +void drm_teardown_hdcp_srm(struct class *drm_class) {
> > +	if (srm_data) {
> > +		kfree(srm_data->srm_buf);
> > +		kfree(srm_data->revoked_ksv_list);
> > +		kfree(srm_data);
> > +	}
> > +}
> > diff --git a/drivers/gpu/drm/drm_internal.h 
> > b/drivers/gpu/drm/drm_internal.h index e19ac7ca602d..476a422414f6 
> > 100644
> > --- a/drivers/gpu/drm/drm_internal.h
> > +++ b/drivers/gpu/drm/drm_internal.h
> > @@ -201,3 +201,7 @@ int drm_syncobj_query_ioctl(struct drm_device 
> > *dev, void *data,  void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
> >  				const struct drm_framebuffer *fb);  int 
> > drm_framebuffer_debugfs_init(struct drm_minor *minor);
> > +
> > +/* drm_hdcp.c */
> > +int drm_setup_hdcp_srm(struct class *drm_class); void 
> > +drm_teardown_hdcp_srm(struct class *drm_class);
> > diff --git a/drivers/gpu/drm/drm_sysfs.c 
> > b/drivers/gpu/drm/drm_sysfs.c index ecb7b33002bb..18b1ac442997 
> > 100644
> > --- a/drivers/gpu/drm/drm_sysfs.c
> > +++ b/drivers/gpu/drm/drm_sysfs.c
> > @@ -78,6 +78,7 @@ int drm_sysfs_init(void)
> >  	}
> >  
> >  	drm_class->devnode = drm_devnode;
> > +	drm_setup_hdcp_srm(drm_class);
> >  	return 0;
> >  }
> >  
> > @@ -90,6 +91,7 @@ void drm_sysfs_destroy(void)  {
> >  	if (IS_ERR_OR_NULL(drm_class))
> >  		return;
> > +	drm_teardown_hdcp_srm(drm_class);
> >  	class_remove_file(drm_class, &class_attr_version.attr);
> >  	class_destroy(drm_class);
> >  	drm_class = NULL;
> > diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index 
> > f243408ecf26..292998a2497f 100644
> > --- a/include/drm/drm_hdcp.h
> > +++ b/include/drm/drm_hdcp.h
> > @@ -265,4 +265,28 @@ void drm_hdcp2_u32_to_seq_num(u8 seq_num[HDCP_2_2_SEQ_NUM_LEN], u32 val)
> >  	seq_num[2] = val;
> >  }
> >  
> > +#define DRM_HDCP_SRM_GEN1_MAX_BYTES		(5 * 1024)
> > +#define DRM_HDCP_1_4_SRM_ID			0x8
> > +#define DRM_HDCP_SRM_ID_MASK			(0xF << 4)
> 
> This is surprising (I'd have expected the hdcp2 indicator in the low 
> bits) from reading the spec. But I guess you tested this, and there's 
> some mumbo-jumbo about "everything is big endian", which might also 
> mean nibbles. Who knows.
> 
> > +#define DRM_HDCP_1_4_VRL_LENGTH_SIZE		3
> > +#define DRM_HDCP_1_4_DCP_SIG_SIZE		40
> > +#define DRM_HDCP_2_SRM_ID			0x9
> > +#define DRM_HDCP_2_INDICATOR			0x1
> > +#define DRM_HDCP_2_INDICATOR_MASK		0xF
> > +#define DRM_HDCP_2_VRL_LENGTH_SIZE		3
> > +#define DRM_HDCP_2_DCP_SIG_SIZE			384
> > +#define DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ	4
> > +#define DRM_HDCP_2_KSV_COUNT_2_LSBITS(byte)	(((byte) & 0xC) >> 6)
> > +
> > +struct hdcp_srm_header {
> > +	u8 srm_id;
> > +	u8 reserved;
> > +	u16 srm_version;
> 
> Needs to be __be16, so that sparse can validate you got all the byte 
> swapping correct. Would be good to double-check with sparse yourself 
> (otherwise 0day will do it for you, but if there's a problem better to 
> catch it early).
> 
> > +	u8 srm_gen_no;
> > +} __packed;
> > +
> > +struct drm_device;
> > +
> > +bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev,
> > +				 u8 *ksvs, u32 ksv_count);
> >  #endif
> > --
> > 2.19.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795