Re: [PATCHv9 04/13] cec: expose the new connector info API

[Hans Verkuil]

On 6/25/19 4:58 PM, Dariusz Marcinkiewicz wrote:
> Hi again.
> On Tue, Jun 25, 2019 at 4:45 PM Hans Verkuil  wrote:
>>
>> +   mutex_lock(>lock);
>> +   if (copy_to_user(parg, >conn_info, sizeof(adap->conn_info)))
>> +   ret = -EFAULT;
>> +   mutex_unlock(>lock);
>> +   return ret;
>> +}
> Shouldn't the lock be released before calling copy_to_user? I guess
> you need to make an extra copy of the conn_info, like it is done for
> other ioctls.

There is no reason for doing that, it would just use additional stack
space.

Regards,

Hans

Re: [PATCHv9 04/13] cec: expose the new connector info API

[Dariusz Marcinkiewicz]

Hi again.
On Tue, Jun 25, 2019 at 4:45 PM Hans Verkuil  wrote:
>
> +   mutex_lock(>lock);
> +   if (copy_to_user(parg, >conn_info, sizeof(adap->conn_info)))
> +   ret = -EFAULT;
> +   mutex_unlock(>lock);
> +   return ret;
> +}
Shouldn't the lock be released before calling copy_to_user? I guess
you need to make an extra copy of the conn_info, like it is done for
other ioctls.

Regards.

[PATCHv9 04/13] cec: expose the new connector info API

[Hans Verkuil]

From: Dariusz Marcinkiewicz 

Until now the connector info API was a kernel-internal API only.
This moves it to the public API and adds the new ioctl to retrieve
this information.

Signed-off-by: Dariusz Marcinkiewicz 
Signed-off-by: Hans Verkuil 
---
 drivers/media/cec/cec-adap.c |  2 ++
 drivers/media/cec/cec-api.c  | 20 ++
 drivers/media/cec/cec-core.c |  5 -
 include/media/cec.h  | 31 
 include/uapi/linux/cec.h | 40 
 5 files changed, 62 insertions(+), 36 deletions(-)

diff --git a/drivers/media/cec/cec-adap.c b/drivers/media/cec/cec-adap.c
index 451c61bde4d4..059c83525024 100644
--- a/drivers/media/cec/cec-adap.c
+++ b/drivers/media/cec/cec-adap.c
@@ -319,6 +319,8 @@ static void cec_post_state_event(struct cec_adapter *adap)
 
ev.state_change.phys_addr = adap->phys_addr;
ev.state_change.log_addr_mask = adap->log_addrs.log_addr_mask;
+   ev.state_change.have_conn_info =
+   adap->conn_info.type != CEC_CONNECTOR_TYPE_NO_CONNECTOR;
cec_queue_event(adap, );
 }
 
diff --git a/drivers/media/cec/cec-api.c b/drivers/media/cec/cec-api.c
index 12d676484472..17d1cb2e5f97 100644
--- a/drivers/media/cec/cec-api.c
+++ b/drivers/media/cec/cec-api.c
@@ -187,6 +187,21 @@ static long cec_adap_s_log_addrs(struct cec_adapter *adap, 
struct cec_fh *fh,
return 0;
 }
 
+static long cec_adap_g_connector_info(struct cec_adapter *adap,
+ struct cec_log_addrs __user *parg)
+{
+   int ret = 0;
+
+   if (!(adap->capabilities & CEC_CAP_CONNECTOR_INFO))
+   return -ENOTTY;
+
+   mutex_lock(>lock);
+   if (copy_to_user(parg, >conn_info, sizeof(adap->conn_info)))
+   ret = -EFAULT;
+   mutex_unlock(>lock);
+   return ret;
+}
+
 static long cec_transmit(struct cec_adapter *adap, struct cec_fh *fh,
 bool block, struct cec_msg __user *parg)
 {
@@ -506,6 +521,9 @@ static long cec_ioctl(struct file *filp, unsigned int cmd, 
unsigned long arg)
case CEC_ADAP_S_LOG_ADDRS:
return cec_adap_s_log_addrs(adap, fh, block, parg);
 
+   case CEC_ADAP_G_CONNECTOR_INFO:
+   return cec_adap_g_connector_info(adap, parg);
+
case CEC_TRANSMIT:
return cec_transmit(adap, fh, block, parg);
 
@@ -578,6 +596,8 @@ static int cec_open(struct inode *inode, struct file *filp)
/* Queue up initial state events */
ev.state_change.phys_addr = adap->phys_addr;
ev.state_change.log_addr_mask = adap->log_addrs.log_addr_mask;
+   ev.state_change.have_conn_info =
+   adap->conn_info.type != CEC_CONNECTOR_TYPE_NO_CONNECTOR;
cec_queue_event_fh(fh, , 0);
 #ifdef CONFIG_CEC_PIN
if (adap->pin && adap->pin->ops->read_hpd) {
diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c
index 9c610e1e99b8..db7adffcdc76 100644
--- a/drivers/media/cec/cec-core.c
+++ b/drivers/media/cec/cec-core.c
@@ -257,11 +257,6 @@ struct cec_adapter *cec_allocate_adapter(const struct 
cec_adap_ops *ops,
struct cec_adapter *adap;
int res;
 
-   /*
-* Disable this capability until the connector info public API
-* is ready.
-*/
-   caps &= ~CEC_CAP_CONNECTOR_INFO;
 #ifndef CONFIG_MEDIA_CEC_RC
caps &= ~CEC_CAP_RC;
 #endif
diff --git a/include/media/cec.h b/include/media/cec.h
index 4d59387bc61b..0a4f69cc9dd4 100644
--- a/include/media/cec.h
+++ b/include/media/cec.h
@@ -18,9 +18,6 @@
 #include 
 #include 
 
-/* CEC_ADAP_G_CONNECTOR_INFO is available */
-#define CEC_CAP_CONNECTOR_INFO (1 << 8)
-
 #define CEC_CAP_DEFAULTS (CEC_CAP_LOG_ADDRS | CEC_CAP_TRANSMIT | \
  CEC_CAP_PASSTHROUGH | CEC_CAP_RC)
 
@@ -147,34 +144,6 @@ struct cec_adap_ops {
  */
 #define CEC_MAX_MSG_TX_QUEUE_SZ(18 * 1)
 
-/**
- * struct cec_drm_connector_info - tells which drm connector is
- * associated with the CEC adapter.
- * @card_no: drm card number
- * @connector_id: drm connector ID
- */
-struct cec_drm_connector_info {
-   __u32 card_no;
-   __u32 connector_id;
-};
-
-#define CEC_CONNECTOR_TYPE_NO_CONNECTOR0
-#define CEC_CONNECTOR_TYPE_DRM 1
-
-/**
- * struct cec_connector_info - tells if and which connector is
- * associated with the CEC adapter.
- * @type: connector type (if any)
- * @drm: drm connector info
- */
-struct cec_connector_info {
-   __u32 type;
-   union {
-   struct cec_drm_connector_info drm;
-   __u32 raw[16];
-   };
-};
-
 struct cec_adapter {
struct module *owner;
char name[32];
diff --git a/include/uapi/linux/cec.h b/include/uapi/linux/cec.h
index 5704fa0292b5..0115c5aa0d36 100644
--- a/include/uapi/linux/cec.h
+++ b/include/uapi/linux/cec.h
@@ -317,6 +317,8 @@ static inline int cec_is_unconfigured(__u16 log_addr_mask)
 #define CEC_CAP_NEEDS_HPD  (1 << 6)