| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5eloulbppelko7hwyppq4apkuqowe3yv5bd7rlipispetg6aqj@u3mzwubs6xxf>
Date: Thu, 25 Jul 2024 07:07:27 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jameson Thies <jthies@...gle.com>, Sebastian Reichel <sre@...nel.org>
Cc: heikki.krogerus@...ux.intel.com, linux-usb@...r.kernel.org,
bleung@...gle.com, abhishekpandit@...omium.org, andersson@...nel.org,
fabrice.gasnier@...s.st.com, gregkh@...uxfoundation.org, hdegoede@...hat.com,
neil.armstrong@...aro.org, rajaram.regupathy@...el.com, saranya.gopal@...el.com,
linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 3/4] usb: typec: ucsi: Set sink path based on UCSI
charge control
On Wed, Jul 24, 2024 at 08:11:15PM GMT, Jameson Thies wrote:
> Add POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX as a property to the UCSI
> power supply driver. When set to a positive value, enable the
> connector's sink path. When set to a negative value, disable the
> connector's sink path.
[Also added power supply maintainers to cc]
This really looks like a hack. As a user I'd expect that if I set the
charging limit, it really gets applied. In other words, I'd expect to be
able to limit 60W PSY to provide just 15W by limiting the current. This
is not the case with this patch.
Please provide rational for this change, i.e. why using the existing
typec property isn't enough for your use case.
>
> Signed-off-by: Jameson Thies <jthies@...gle.com>
> ---
> Changes in V2:
> - Added SET_SINK_PATH call when handling charge_control_limit_max
> update. Updated commit message.
>
> drivers/usb/typec/ucsi/psy.c | 52 +++++++++++++++++++++++++++++++++++
> drivers/usb/typec/ucsi/ucsi.c | 15 ++++++++++
> drivers/usb/typec/ucsi/ucsi.h | 5 ++++
> 3 files changed, 72 insertions(+)
>
> diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c
> index d708f9eb1654..28265feb9d13 100644
> --- a/drivers/usb/typec/ucsi/psy.c
> +++ b/drivers/usb/typec/ucsi/psy.c
> @@ -30,6 +30,7 @@ static enum power_supply_property ucsi_psy_props[] = {
> POWER_SUPPLY_PROP_CURRENT_NOW,
> POWER_SUPPLY_PROP_SCOPE,
> POWER_SUPPLY_PROP_STATUS,
> + POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX,
> };
>
> static int ucsi_psy_get_scope(struct ucsi_connector *con,
> @@ -275,11 +276,60 @@ static int ucsi_psy_get_prop(struct power_supply *psy,
> return ucsi_psy_get_scope(con, val);
> case POWER_SUPPLY_PROP_STATUS:
> return ucsi_psy_get_status(con, val);
> + case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
> + val->intval = 0;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ucsi_psy_set_charge_control_limit_max(struct ucsi_connector *con,
> + const union power_supply_propval *val)
> +{
> + /*
> + * Writing a negative value to the charge control limit max implies the
> + * port should not accept charge. Disable the sink path for a negative
> + * charge control limit, and enable the sink path for a positive charge
> + * control limit. If the requested charge port is a source, update the
> + * power role.
> + */
> + int ret;
> + bool sink_path = false;
> +
> + if (val->intval >= 0) {
> + sink_path = true;
> + if (!con->typec_cap.ops || !con->typec_cap.ops->pr_set)
> + return -EINVAL;
> +
> + ret = con->typec_cap.ops->pr_set(con->port, TYPEC_SINK);
Should there be a call to pr_set(TYPEC_SOURCE) if the value is negative?
> + if (ret < 0)
> + return ret;
> + }
> +
> + return ucsi_set_sink_path(con, sink_path);
You are calling SET_SINK_PATH uncoditionally, while this command was not
defined for UCSI 1.x. Also it is an error to call it when device is in
power source mode or if there is no partner connected.
Last, but not least, the property value survives between PSY reconnects
(which is expected), however after reconnecting the partner device, the
value won't be reprogrammed by the UCSI driver, resulting in the
inconsistency between the sysfs and actual system state.
> +}
> +
> +static int ucsi_psy_set_prop(struct power_supply *psy,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + struct ucsi_connector *con = power_supply_get_drvdata(psy);
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
> + return ucsi_psy_set_charge_control_limit_max(con, val);
> default:
> return -EINVAL;
> }
> }
>
> +static int ucsi_psy_prop_is_writeable(struct power_supply *psy,
> + enum power_supply_property psp)
> +{
> + return psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX;
> +}
> +
> static enum power_supply_usb_type ucsi_psy_usb_types[] = {
> POWER_SUPPLY_USB_TYPE_C,
> POWER_SUPPLY_USB_TYPE_PD,
> @@ -308,6 +358,8 @@ int ucsi_register_port_psy(struct ucsi_connector *con)
> con->psy_desc.properties = ucsi_psy_props;
> con->psy_desc.num_properties = ARRAY_SIZE(ucsi_psy_props);
> con->psy_desc.get_property = ucsi_psy_get_prop;
> + con->psy_desc.set_property = ucsi_psy_set_prop;
> + con->psy_desc.property_is_writeable = ucsi_psy_prop_is_writeable;
>
> con->psy = power_supply_register(dev, &con->psy_desc, &psy_cfg);
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index dcd3765cc1f5..02663fdebdd9 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1545,6 +1545,21 @@ static const struct typec_operations ucsi_ops = {
> .pr_set = ucsi_pr_swap
> };
>
> +int ucsi_set_sink_path(struct ucsi_connector *con, bool sink_path)
> +{
> + struct ucsi *ucsi = con->ucsi;
> + u64 command;
> + int ret;
> +
> + command = UCSI_SET_SINK_PATH | UCSI_CONNECTOR_NUMBER(con->num);
> + command |= UCSI_SET_SINK_PATH_SINK_PATH(sink_path);
> + ret = ucsi_send_command(ucsi, command, NULL, 0);
> + if (ret < 0)
> + dev_err(con->ucsi->dev, "SET_SINK_PATH failed (%d)\n", ret);
> +
> + return ret;
> +}
> +
> /* Caller must call fwnode_handle_put() after use */
> static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con)
> {
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 57129f3c0814..6a958eac5703 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -114,6 +114,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
> #define UCSI_GET_CONNECTOR_STATUS 0x12
> #define UCSI_GET_ERROR_STATUS 0x13
> #define UCSI_GET_PD_MESSAGE 0x15
> +#define UCSI_SET_SINK_PATH 0x1c
>
> #define UCSI_CONNECTOR_NUMBER(_num_) ((u64)(_num_) << 16)
> #define UCSI_COMMAND(_cmd_) ((_cmd_) & 0xff)
> @@ -187,6 +188,9 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num);
> #define UCSI_GET_PD_MESSAGE_TYPE_IDENTITY 4
> #define UCSI_GET_PD_MESSAGE_TYPE_REVISION 5
>
> +/* SET_SINK_PATH command bits */
> +#define UCSI_SET_SINK_PATH_SINK_PATH(_r_) (((_r_) ? 1 : 0) << 23)
> +
> /* -------------------------------------------------------------------------- */
>
> /* Error information returned by PPM in response to GET_ERROR_STATUS command. */
> @@ -492,6 +496,7 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
>
> void ucsi_altmode_update_active(struct ucsi_connector *con);
> int ucsi_resume(struct ucsi *ucsi);
> +int ucsi_set_sink_path(struct ucsi_connector *con, bool sink_path);
>
> void ucsi_notify_common(struct ucsi *ucsi, u32 cci);
> int ucsi_sync_control_common(struct ucsi *ucsi, u64 command);
> --
> 2.45.2.1089.g2a221341d9-goog
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists