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