Hi,
On Mon, Jan 23, 2017 at 04:44:23PM +0200, Felipe Balbi wrote:
> 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:
We will always get all three VDOs as response to discover identity
command, but..
> static void typec_report_identity(struct device *dev, const char *prop)
> {
> sysfs_notify(&dev->kobj, "identity", prop);
> }
Sure, that works for me.
> > +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.
I just changed this to usb_power_delivery because somebody did not
like "USB Power Delivery" :-).
I don't have strong opinion on the output format, but Guenter said
that he would prefer more machine readable style. So Guenter, and also
Oliver! Could you give your opinion?
> > +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".
We are not the vconn sink when we are not sourcing it. So we just
either source vconn or not.
> > +
> > + 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?
I need fix this function.
Thanks,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
