Hi,
Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> writes:
> +static void typec_report_identity(struct device *dev)
> +{
> + sysfs_notify(&dev->kobj, "identity", "id_header");
> + sysfs_notify(&dev->kobj, "identity", "cert_stat");
> + sysfs_notify(&dev->kobj, "identity", "product");
if you sysfs_notify() all three everytime this might cause issues for
userspace pollers. What will happen is that you're gonna change
e.g. id_header and threads polling id_header, cert_stat and product will
be notified of what's supposed to be new data. Maybe this should know
what it's notifying and only notify what actually changed. Seems like
just passing one extra char * argument is enough:
static void typec_report_identity(struct device *dev, const char *prop)
{
sysfs_notify(&dev->kobj, "identity", prop);
}
> +static ssize_t power_role_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct typec_port *port = to_typec_port(dev);
> +
> + if (port->cap->type == TYPEC_PORT_DRP)
> + return sprintf(buf, "%s\n", port->pwr_role == TYPEC_SOURCE ?
> + "[source] sink" : "source [sink]");
> +
> + return sprintf(buf, "[%s]\n", typec_roles[port->pwr_role]);
> +}
> +static DEVICE_ATTR_RW(power_role);
> +
> +static const char * const typec_pwr_opmodes[] = {
> + [TYPEC_PWR_MODE_USB] = "default",
> + [TYPEC_PWR_MODE_1_5A] = "1.5A",
> + [TYPEC_PWR_MODE_3_0A] = "3.0A",
> + [TYPEC_PWR_MODE_PD] = "usb_power_delivery",
do you need this to have underscores? How about "USB Power Delivery",
instead? Userland could use the string untreated as part of GUI.
> +static ssize_t vconn_source_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct typec_port *port = to_typec_port(dev);
> + enum typec_role role;
> + int ret;
> +
> + if (!port->cap->pd_revision) {
> + dev_dbg(dev, "VCONN swap depends on USB Power Delivery\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (!port->cap->vconn_set) {
> + dev_dbg(dev, "VCONN swapping not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (sysfs_streq(buf, "1"))
> + role = TYPEC_SOURCE;
> + else if (sysfs_streq(buf, "0"))
> + role = TYPEC_SINK;
> + else
> + return -EINVAL;
seems to me this could be stringified (both store and show) to "source"
and "sink".
> +
> + ret = port->cap->vconn_set(port->cap, role);
> + if (ret)
> + return ret;
> +
> + return size;
> +}
> +
> +static ssize_t vconn_source_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct typec_port *port = to_typec_port(dev);
> +
> + return sprintf(buf, "%d\n", port->vconn_role == TYPEC_SOURCE ? 1 : 0);
> +}
> +static DEVICE_ATTR_RW(vconn_source);
> +
> +static ssize_t supported_accessory_modes_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct typec_port *port = to_typec_port(dev);
> + ssize_t ret = 0;
> + int i;
> +
> + if (!port->cap->accessory)
> + return 0;
> +
> + for (i = 0; port->cap->accessory[i]; i++)
> + ret += sprintf(buf + ret, "%s ",
> + typec_accessory_modes[port->cap->accessory[i]]);
possible buffer overflow here?
--
balbi
Attachment:
signature.asc
Description: PGP signature
