Re: [PATCH v6 06/11] cec: add HDMI CEC framework

Typo and question:

On 05/04/15 19:32, Kamil Debski wrote:

+static long cec_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) +{

• struct cec_devnode *cecdev = cec_devnode_data(filp);
• struct cec_adapter *adap = to_cec_adapter(cecdev);
• void __user *parg = (void __user *)arg;
• int err;
• if (!cec_devnode_is_registered(cecdev))

• return -EIO;
  

• switch (cmd) {

snip

• case CEC_G_ADAP_STATE: {

• u32 state = adap->state != CEC_ADAP_STATE_DISABLED;
  

• if (copy_to_user(parg, &state, sizeof(state)))
  

• 	return -EFAULT;
  

• break;
  

• }
• case CEC_S_ADAP_STATE: {

• u32 state;
  

• if (!(adap->capabilities & CEC_CAP_STATE))
  

• 	return -ENOTTY;
  

• if (copy_from_user(&state, parg, sizeof(state)))
  

• 	return -EFAULT;
  

• if (!state && adap->state == CEC_ADAP_STATE_DISABLED)
  

• 	return 0;
  

• if (state && adap->state != CEC_ADAP_STATE_DISABLED)
  

• 	return 0;
  

• cec_enable(adap, !!state);
  

• break;
  

• }
• case CEC_G_ADAP_PHYS_ADDR:

• if (copy_to_user(parg, &adap->phys_addr,
  

• 				sizeof(adap->phys_addr)))
  

• 	return -EFAULT;
  

If the adapter requires that userspace sets up the phys addr, then what should this return if no such address has been set up?

I see two options: either 0xffff (which should be used if the HDMI cable is disconnected), or return an error (perhaps ENODATA).

I think 0xffff might be best. This will still allow the unregistered logical address.

Note that the comment in uapi/linux/cec.h for G_ADAP_LOG_ADDRS says that it will return an error if the physical address is not set. That's not true as far as I can tell, and if we go for 0xffff as the default in a case like that, then it isn't necessary either to return an error.

cec_create_adapter already initialized the physical address to 0xffff, so that looks good. But it should be documented in cec-ioc-g-adap-phys-addr.xml.

• break;
  

• case CEC_S_ADAP_PHYS_ADDR: {

• u16 phys_addr;
  

• if (!(adap->capabilities & CEC_CAP_PHYS_ADDR))
  

• 	return -ENOTTY;
  

• if (copy_from_user(&phys_addr, parg, sizeof(phys_addr)))
  

• 	return -EFAULT;
  

• adap->phys_addr = phys_addr;
  

• break;
  

• }
• case CEC_G_ADAP_LOG_ADDRS: {

• struct cec_log_addrs log_addrs;
  

• log_addrs.cec_version = adap->version;
  

• log_addrs.num_log_addrs = adap->num_log_addrs;
  

• memcpy(log_addrs.primary_device_type, adap->prim_device,
  

• 					CEC_MAX_LOG_ADDRS);
  

• memcpy(log_addrs.log_addr_type, adap->log_addr_type,
  

• 					CEC_MAX_LOG_ADDRS);
  

• memcpy(log_addrs.log_addr, adap->log_addr,
  

• 					CEC_MAX_LOG_ADDRS);
  

• if (copy_to_user(parg, &log_addrs, sizeof(log_addrs)))
  

• 	return -EFAULT;
  

• break;
  

• }
• case CEC_S_ADAP_LOG_ADDRS: {

• struct cec_log_addrs log_addrs;
  

• if (!(adap->capabilities & CEC_CAP_LOG_ADDRS))
  

• 	return -ENOTTY;
  

• if (copy_from_user(&log_addrs, parg, sizeof(log_addrs)))
  

• 	return -EFAULT;
  

• err = cec_claim_log_addrs(adap, &log_addrs,
  

• 			!(filp->f_flags & O_NONBLOCK));
  

• if (err)
  

• 	return err;
  

• if (copy_to_user(parg, &log_addrs, sizeof(log_addrs)))
  

• 	return -EFAULT;
  

• break;
  

• }
• case CEC_G_VENDOR_ID:

• if (copy_to_user(parg, &adap->vendor_id,
  

• 				sizeof(adap->vendor_id)))
  

• 	return -EFAULT;
  

I've been reading up on this. If I understand it correctly, then this is optional (only if a device supports vendor commands does it have to implement this).

So if the VENDOR capability is set, then userspace *may* change it. If it is left undefined, then no vendor commands are allowed.

I think this should be redesigned:

One CEC_CAP_VENDOR_CMDS: if set, then vendor commands are allowed. One CEC_CAP_VENDOR_ID: userspace may set the Vendor ID. No vendor commands are allowed as long as no vendor ID was set.

So if VENDOR_CMDS is set and VENDOR_ID isn't, then that means that the driver will have set the vendor ID and the application can retrieve it with G_VENDOR_ID. If both are set, then userspace has to provide a vendor ID before vendor commands will be allowed.

That leaves the problem of determining that no vendor ID was set. Unfortunately the whole range of 0x000000-0xffffff is valid (and 0x000000 maps in fact to a company (Xerox) according to the IEEE Registration Authority Committee website.

We can define a CEC_VENDOR_ID_INVALID 0xffffffff, that might be the easiest way of doing this.

A related question: should userspace be allowed to change a valid physical address or a valid vendor ID to something else once the logical addresses have been claimed? Or should that result in -EBUSY? I'm leaning towards that.

Actually, the same question is true for S_LOG_ADDRS: that should be done only once as well as long as the adapter is enabled.

BTW, the documentation does not mention the order in which S_PHYS_ADDR and S_VENDOR_ID should be issued: should this be done before the adapter is enabled or before the logical addresses are claimed?

• break;
  

• case CEC_S_VENDOR_ID: {

• u32 vendor_id;
  

• if (!(adap->capabilities & CEC_CAP_VENDOR_ID))
  

• 	return -ENOTTY;
  

• if (copy_from_user(&vendor_id, parg, sizeof(vendor_id)))
  

• 	return -EFAULT;
  

• /* Vendori ID is a 24 bit number, so check if the value is
  

Typo: Vendori -> Vendor

•  * within the correct range. */
  

• if ((vendor_id & 0xff000000) != 0)
  

• 	return -EINVAL;
  

• adap->vendor_id = vendor_id;
  

• break;
  

• }

Working on a compliance test is always a great way of finding all these corner cases...

Regards,

Hans