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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html