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-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
