| 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: <CAPTae5+e9Pibj7RXE=4tzvyJLRudMgPhsEqrWtFjgsQRc+C5eQ@mail.gmail.com>
Date: Tue, 1 Dec 2020 19:34:01 -0800
From: Badhri Jagan Sridharan <badhri@...gle.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
USB <linux-usb@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 2/3] usb: typec: tcpci: Add support to report vSafe0V
On Tue, Dec 1, 2020 at 5:27 AM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On Mon, Nov 30, 2020 at 05:32:45PM -0800, Badhri Jagan Sridharan wrote:
> > This change adds vbus_vsafe0v which when set, makes TCPM
> > query for VSAFE0V by assigning the tcpc.is_vbus_vsafe0v callback.
> > Also enables ALERT.ExtendedStatus which is triggered when
> > status of EXTENDED_STATUS.vSafe0V changes.
> > EXTENDED_STATUS.vSafe0V is set when vbus is at vSafe0V and
> > cleared otherwise.
> >
> > Signed-off-by: Badhri Jagan Sridharan <badhri@...gle.com>
> > ---
> > drivers/usb/typec/tcpm/tcpci.c | 55 ++++++++++++++++++++++++++--------
> > drivers/usb/typec/tcpm/tcpci.h | 6 ++++
> > 2 files changed, 49 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> > index 12d983a75510..e281b8bee4db 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.c
> > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > @@ -402,6 +402,19 @@ static int tcpci_get_vbus(struct tcpc_dev *tcpc)
> > return !!(reg & TCPC_POWER_STATUS_VBUS_PRES);
> > }
> >
> > +static int tcpci_is_vbus_vsafe0v(struct tcpc_dev *tcpc)
> > +{
> > + struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > + unsigned int reg;
> > + int ret;
> > +
> > + ret = regmap_read(tcpci->regmap, TCPC_EXTENDED_STATUS, ®);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return !!(reg & TCPC_EXTENDED_STATUS_VSAFE0V);
> > +}
> > +
> > static int tcpci_set_vbus(struct tcpc_dev *tcpc, bool source, bool sink)
> > {
> > struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
> > @@ -554,12 +567,22 @@ static int tcpci_init(struct tcpc_dev *tcpc)
> > TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_CC_STATUS;
> > if (tcpci->controls_vbus)
> > reg |= TCPC_ALERT_POWER_STATUS;
> > + /* Enable VSAFE0V status interrupt when detecting VSAFE0V is supported */
> > + if (tcpci->data->vbus_vsafe0v) {
> > + reg |= TCPC_ALERT_EXTENDED_STATUS;
> > + ret = regmap_write(tcpci->regmap, TCPC_EXTENDED_STATUS_MASK,
> > + TCPC_EXTENDED_STATUS_VSAFE0V);
> > + if (ret < 0)
> > + return ret;
> > + }
> > return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
> > }
> >
> > irqreturn_t tcpci_irq(struct tcpci *tcpci)
> > {
> > u16 status;
> > + int ret;
> > + unsigned int raw;
> >
> > tcpci_read16(tcpci, TCPC_ALERT, &status);
> >
> > @@ -575,18 +598,17 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
> > tcpm_cc_change(tcpci->port);
> >
> > if (status & TCPC_ALERT_POWER_STATUS) {
> > - unsigned int reg;
> > -
> > - regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, ®);
> > -
> > - /*
> > - * If power status mask has been reset, then the TCPC
> > - * has reset.
> > - */
> > - if (reg == 0xff)
> > - tcpm_tcpc_reset(tcpci->port);
> > - else
> > - tcpm_vbus_change(tcpci->port);
> > + ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
> > + if (ret >= 0) {
> > + /*
> > + * If power status mask has been reset, then the TCPC
> > + * has reset.
> > + */
> > + if (raw == 0xff)
> > + tcpm_tcpc_reset(tcpci->port);
> > + else
> > + tcpm_vbus_change(tcpci->port);
> > + }
>
> This change seems unrelated to this patch. Besides that, are you sure that
> ignoring an error from regmap_read() is sensible here ?
Sorry should have split that into a separate patch. I was actually intending
to do the following where tcpm calls are not made if TCPC_POWER_STATUS_MASK
read returns error. The code was previously ignoring the error.
if (!ret) {
/*
* If power status mask has been reset, then the TCPC
* has reset.
*/
if (raw == 0xff)
tcpm_tcpc_reset(tcpci->port);
else
tcpm_vbus_change(tcpci->port);
}
This is reasonable right ?
}
>
> Overall, it may make sense to improve error handling in this driver, but I think
> it should be done in a separate patch.
>
> > }
> >
> > if (status & TCPC_ALERT_RX_STATUS) {
> > @@ -622,6 +644,12 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
> > tcpm_pd_receive(tcpci->port, &msg);
> > }
> >
> > + if (status & TCPC_ALERT_EXTENDED_STATUS) {
> > + ret = regmap_read(tcpci->regmap, TCPC_EXTENDED_STATUS, &raw);
> > + if (ret >= 0 && (raw & TCPC_EXTENDED_STATUS_VSAFE0V))
> > + tcpm_vbus_change(tcpci->port);
> > + }
> > +
> > if (status & TCPC_ALERT_RX_HARD_RST)
> > tcpm_pd_hard_reset(tcpci->port);
> >
> > @@ -699,6 +727,9 @@ struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data)
> > tcpci_set_auto_vbus_discharge_threshold;
> > }
> >
> > + if (tcpci->data->vbus_vsafe0v)
> > + tcpci->tcpc.is_vbus_vsafe0v = tcpci_is_vbus_vsafe0v;
> > +
> > err = tcpci_parse_config(tcpci);
> > if (err < 0)
> > return ERR_PTR(err);
> > diff --git a/drivers/usb/typec/tcpm/tcpci.h b/drivers/usb/typec/tcpm/tcpci.h
> > index 3fe313655f0c..116a69c85e38 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.h
> > +++ b/drivers/usb/typec/tcpm/tcpci.h
> > @@ -49,6 +49,9 @@
> > #define TCPC_TCPC_CTRL_ORIENTATION BIT(0)
> > #define TCPC_TCPC_CTRL_BIST_TM BIT(1)
> >
> > +#define TCPC_EXTENDED_STATUS 0x20
> > +#define TCPC_EXTENDED_STATUS_VSAFE0V BIT(0)
> > +
> > #define TCPC_ROLE_CTRL 0x1a
> > #define TCPC_ROLE_CTRL_DRP BIT(6)
> > #define TCPC_ROLE_CTRL_RP_VAL_SHIFT 4
> > @@ -155,11 +158,14 @@ struct tcpci;
> > * is sourcing vbus.
> > * @auto_discharge_disconnect:
> > * Optional; Enables TCPC to autonously discharge vbus on disconnect.
> > + * @vbus_vsafe0v:
> > + * optional; Set when TCPC can detect whether vbus is at VSAFE0V.
> > */
> > struct tcpci_data {
> > struct regmap *regmap;
> > unsigned char TX_BUF_BYTE_x_hidden:1;
> > unsigned char auto_discharge_disconnect:1;
> > + unsigned char vbus_vsafe0v:1;
> >
> > int (*init)(struct tcpci *tcpci, struct tcpci_data *data);
> > int (*set_vconn)(struct tcpci *tcpci, struct tcpci_data *data,
> > --
> > 2.29.2.454.gaff20da3a2-goog
> >
Powered by blists - more mailing lists