Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220207043907.2758424-1-badhri@google.com> <YgDk8M3N8VEZK5Sk@kuha.fi.intel.com>
In-Reply-To: <YgDk8M3N8VEZK5Sk@kuha.fi.intel.com>
From:   Badhri Jagan Sridharan <badhri@google.com>
Date:   Mon, 7 Feb 2022 14:38:09 -0800
Message-ID: <CAPTae5KG5iJSkB352KnUwtp741kFocnugfqHGKvtGzRTCPeP2w@mail.gmail.com>
Subject: Re: [PATCH v1 1/2] usb: typec: Introduce typec attributes for
 limiting source current
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Guenter Roeck <linux@roeck-us.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        USB <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Kyle Tso <kyletso@google.com>, Benson Leung <bleung@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Feb 7, 2022 at 1:23 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> +Benson
>
> On Sun, Feb 06, 2022 at 08:39:06PM -0800, Badhri Jagan Sridharan wrote:
> > This change introduces the following two attributes to the
> > typec sysfs class which allows userspace to limit the power
> > advertised while acting as source. This is useful to mitigate
> > battery drain in portable devices when the battery SOC is low.
> >
> > New attibutes introduced:
> > 1. limit_src_current_active
> > 2. limit_src_current_ma
> >
> > The port while in PD contract and acting as source would
> > only advertise vSafe5V fixed PDO with current set through
> > limit_src_current_ma when limit_src_current_active is set
> > to 1. When limit_src_current_active is set to 0, the port
> > would publish the default source capabilities.
> > limit_src_current_ma would limit the current to the
> > Maximum current published by vSafe5V fixed pdo of the default
> > source capabilities of the port.
>
> This competes with Benson's idea of having "sets" of capabilites from
> which to choose the ones that we advertise to the partner. You could
> also use that idea to cover this case as well. You just have two
> source capabilities sets defined - one where you only have the vSafe5V
> and another for everything.

Agree. I was actually wondering whether there were any follow-ups for
the proposal.
Happy to make use of the interface additions that you are exposing through your
patchset.  Thanks for sending them out !

>
> Benson's idea also seems to be something what we can support with UCSI
> and some native USB PD controller host interfaces, but limiting the
> source capabitites to only vSafe5V is something that we can't do. I
> means, on some platforms we may have a source capabilities "set" that
> we can choose that only exposes the vSafe5V, but there is no guarantee
> that we always have it (and it's unlikely that we ever have it). It's
> up to some firmware that we have no control over.

Given that all pd sources should
mandatorily support vSafe5V and the patch only "reduces" the maximum
current exposed by default, there are no new additional capabilities
that the source
should support. So not sure why this would be not supportable though.

>
> So this is the wrong way and Benson's idea is the right way IMO.
>
> I already prepared a proposal for adding support for Benson's idea:
> https://lore.kernel.org/linux-usb/20220203144657.16527-1-heikki.krogerus@linux.intel.com/
>
> This patch adds the attributes that you can use to choose the
> capabilities that are advertised to the partner:
> https://lore.kernel.org/linux-usb/20220203144657.16527-3-heikki.krogerus@linux.intel.com/
>
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> >  Documentation/ABI/testing/sysfs-class-typec | 25 ++++++
> >  drivers/usb/typec/class.c                   | 99 +++++++++++++++++++++
> >  drivers/usb/typec/class.h                   |  5 ++
> >  include/linux/usb/typec.h                   |  4 +
> >  4 files changed, 133 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > index 75088ecad202..dd2240632172 100644
> > --- a/Documentation/ABI/testing/sysfs-class-typec
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -141,6 +141,31 @@ Description:
> >               - "reverse": CC2 orientation
> >               - "unknown": Orientation cannot be determined.
> >
> > +What:                /sys/class/typec/<port>/limit_src_current_active
> > +Date:                February 2022
> > +Contact:     Badhri Jagan Sridharan <badhri@google.com>
> > +Description:
> > +             This attribute can be used to make the port only publish
> > +             vSafe5V fixed pdo with Maximum current limited to the
> > +             current limit set by limit_src_current_ma when the port
> > +             is acting as source.
> > +             Valid values:
> > +             - write(2) "1" limits source capabilities to vSafe5V
> > +               with max current specified by limit_src_current_ma
> > +             - write(2) "0" publishes the default source capabilities
> > +               of the port.
> > +
> > +What:                /sys/class/typec/<port>/limit_src_current_ma
> > +Date:                February 2022
> > +Contact:     Badhri Jagan Sridharan <badhri@google.com>
> > +Description:
> > +             This attribute allows write(2) to set the Maximum
> > +             current published when limit_src_current_active is set
> > +             to 1. When limit_src_current_active is already set
> > +             to 1, if the port is already acting as source with
> > +             explicit contract in place, write(2) will make the port
> > +             renegotiate the pd contract.
> > +
> >  USB Type-C partner devices (eg. /sys/class/typec/port0-partner/)
> >
> >  What:                /sys/class/typec/<port>-partner/accessory_mode
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 45a6f0c807cb..3b3c7b080ad1 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -1403,6 +1403,102 @@ port_type_show(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RW(port_type);
> >
> > +static ssize_t
> > +limit_src_current_active_store(struct device *dev, struct device_attribute *attr, const char *buf,
> > +                            size_t size)
> > +{
> > +     struct typec_port *port = to_typec_port(dev);
> > +     int ret;
> > +     u8 active;
> > +
> > +     if (port->cap->type == TYPEC_PORT_SNK || !port->ops || !port->ops->limit_src_current_set ||
> > +         !port->cap->pd_revision) {
> > +             dev_dbg(dev, "Limiting source current not supported\n");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     if (kstrtou8(buf, 0, &active))
> > +             return -EINVAL;
> > +
> > +     if (active != 1 && active != 0)
> > +             return -EINVAL;
> > +
> > +     mutex_lock(&port->limit_src_current_lock);
> > +
> > +     if (port->limit_src_current_active == (bool)active) {
> > +             ret = size;
> > +             goto unlock_and_ret;
> > +     }
> > +
> > +     ret = port->ops->limit_src_current_set(port, port->limit_src_current_ma, active);
> > +     if (ret)
> > +             goto unlock_and_ret;
> > +
> > +     port->limit_src_current_active = active;
> > +     ret = size;
> > +
> > +unlock_and_ret:
> > +     mutex_unlock(&port->limit_src_current_lock);
> > +     return ret;
> > +}
> > +
> > +static ssize_t
> > +limit_src_current_active_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +     struct typec_port *port = to_typec_port(dev);
> > +
> > +     return sysfs_emit(buf, "%d\n", port->limit_src_current_active ? 1 : 0);
> > +}
> > +static DEVICE_ATTR_RW(limit_src_current_active);
> > +
> > +static ssize_t
> > +limit_src_current_ma_store(struct device *dev, struct device_attribute *attr, const char *buf,
> > +                        size_t size)
> > +{
> > +     struct typec_port *port = to_typec_port(dev);
> > +     int ret;
> > +     u32 src_current_ma;
> > +
> > +     if (port->cap->type == TYPEC_PORT_SNK || !port->ops || !port->ops->limit_src_current_set ||
> > +         !port->cap->pd_revision) {
> > +             dev_dbg(dev, "Limiting source current not supported\n");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     if (kstrtou32(buf, 0, &src_current_ma))
> > +             return -EINVAL;
> > +
> > +     mutex_lock(&port->limit_src_current_lock);
> > +
> > +     if (port->limit_src_current_ma == src_current_ma) {
> > +             ret = size;
> > +             goto unlock_and_ret;
> > +     }
> > +
> > +     if (port->limit_src_current_active) {
> > +             ret = port->ops->limit_src_current_set(port, src_current_ma,
> > +                                                    port->limit_src_current_active);
> > +             if (ret)
> > +                     goto unlock_and_ret;
> > +     }
> > +
> > +     port->limit_src_current_ma = src_current_ma;
> > +     ret = size;
> > +
> > +unlock_and_ret:
> > +     mutex_unlock(&port->limit_src_current_lock);
> > +     return ret;
> > +}
> > +
> > +static ssize_t
> > +limit_src_current_ma_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +     struct typec_port *port = to_typec_port(dev);
> > +
> > +     return sysfs_emit(buf, "%u\n", port->limit_src_current_ma);
> > +}
> > +static DEVICE_ATTR_RW(limit_src_current_ma);
> > +
> >  static const char * const typec_pwr_opmodes[] = {
> >       [TYPEC_PWR_MODE_USB]    = "default",
> >       [TYPEC_PWR_MODE_1_5A]   = "1.5A",
> > @@ -1536,6 +1632,8 @@ static struct attribute *typec_attrs[] = {
> >       &dev_attr_vconn_source.attr,
> >       &dev_attr_port_type.attr,
> >       &dev_attr_orientation.attr,
> > +     &dev_attr_limit_src_current_active.attr,
> > +     &dev_attr_limit_src_current_ma.attr,
> >       NULL,
> >  };
> >
> > @@ -2039,6 +2137,7 @@ struct typec_port *typec_register_port(struct device *parent,
> >
> >       ida_init(&port->mode_ids);
> >       mutex_init(&port->port_type_lock);
> > +     mutex_init(&port->limit_src_current_lock);
> >
> >       port->id = id;
> >       port->ops = cap->ops;
> > diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
> > index 0f1bd6d19d67..3856bc058444 100644
> > --- a/drivers/usb/typec/class.h
> > +++ b/drivers/usb/typec/class.h
> > @@ -54,6 +54,11 @@ struct typec_port {
> >
> >       const struct typec_capability   *cap;
> >       const struct typec_operations   *ops;
> > +
> > +     /* lock to protect limit_src_current_*_store operation */
> > +     struct mutex                    limit_src_current_lock;
> > +     u32                             limit_src_current_ma;
> > +     bool                            limit_src_current_active;
> >  };
> >
> >  #define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
> > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> > index 7ba45a97eeae..1b1958ae4c16 100644
> > --- a/include/linux/usb/typec.h
> > +++ b/include/linux/usb/typec.h
> > @@ -213,6 +213,8 @@ struct typec_partner_desc {
> >   * @pr_set: Set Power Role
> >   * @vconn_set: Source VCONN
> >   * @port_type_set: Set port type
> > + * @limit_src_current_set: Used to limit source current advertisement while
> > + *                         acting as source
> >   */
> >  struct typec_operations {
> >       int (*try_role)(struct typec_port *port, int role);
> > @@ -221,6 +223,8 @@ struct typec_operations {
> >       int (*vconn_set)(struct typec_port *port, enum typec_role role);
> >       int (*port_type_set)(struct typec_port *port,
> >                            enum typec_port_type type);
> > +     int (*limit_src_current_set)(struct typec_port *port, u32 limit_src_current_ma,
> > +                                  bool enable);
> >  };
> >
> >  enum usb_pd_svdm_ver {
> > --
> > 2.35.0.263.gb82422642f-goog
>
> --
> heikki