| Message ID | 20211108102833.2793803-1-megous@megous.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 362468830dd5bea8bf6ad5203b2ea61f8a4e8288 |
| Headers | show |
| Series | [v2] usb: typec: fusb302: Fix masking of comparator and bc_lvl interrupts | expand |
Hi, On 11/8/21 11:28, Ondrej Jirman wrote: > The code that enables either BC_LVL or COMP_CHNG interrupt in tcpm_set_cc > wrongly assumes that the interrupt is unmasked by writing 1 to the apropriate > bit in the mask register. In fact, interrupts are enabled when the mask > is 0, so the tcpm_set_cc enables interrupt for COMP_CHNG when it expects > BC_LVL interrupt to be enabled. > > This causes inability of the driver to recognize cable unplug events > in host mode (unplug is recognized only via a COMP_CHNG interrupt). > > In device mode this bug was masked by simultaneous triggering of the VBUS > change interrupt, because of loss of VBUS when the port peer is providing > power. > > Fixes: 48242e30532b ("usb: typec: fusb302: Revert "Resolve fixed power role contract setup"") > Signed-off-by: Ondrej Jirman <megous@megous.com> > Cc: Hans de Goede <hdegoede@redhat.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/usb/typec/tcpm/fusb302.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 7a2a17866a823..72f9001b07921 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -669,25 +669,27 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc) > ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK, > FUSB_REG_MASK_BC_LVL | > FUSB_REG_MASK_COMP_CHNG, > - FUSB_REG_MASK_COMP_CHNG); > + FUSB_REG_MASK_BC_LVL); > if (ret < 0) { > fusb302_log(chip, "cannot set SRC interrupt, ret=%d", > ret); > goto done; > } > chip->intr_comp_chng = true; > + chip->intr_bc_lvl = false; FWIW this is not necessary because the fusb302_set_toggling(chip, TOGGLING_MODE_OFF) already does this, but it makes the code more clear, so lets keep it. > break; > case TYPEC_CC_RD: > ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK, > FUSB_REG_MASK_BC_LVL | > FUSB_REG_MASK_COMP_CHNG, > - FUSB_REG_MASK_BC_LVL); > + FUSB_REG_MASK_COMP_CHNG); > if (ret < 0) { > fusb302_log(chip, "cannot set SRC interrupt, ret=%d", > ret); > goto done; > } > chip->intr_bc_lvl = true; > + chip->intr_comp_chng = false; Idem. > break; > default: > break; > Regards, Hans
On Mon, Nov 08, 2021 at 11:28:32AM +0100, Ondrej Jirman wrote: > The code that enables either BC_LVL or COMP_CHNG interrupt in tcpm_set_cc > wrongly assumes that the interrupt is unmasked by writing 1 to the apropriate > bit in the mask register. In fact, interrupts are enabled when the mask > is 0, so the tcpm_set_cc enables interrupt for COMP_CHNG when it expects > BC_LVL interrupt to be enabled. > > This causes inability of the driver to recognize cable unplug events > in host mode (unplug is recognized only via a COMP_CHNG interrupt). > > In device mode this bug was masked by simultaneous triggering of the VBUS > change interrupt, because of loss of VBUS when the port peer is providing > power. > > Fixes: 48242e30532b ("usb: typec: fusb302: Revert "Resolve fixed power role contract setup"") > Signed-off-by: Ondrej Jirman <megous@megous.com> > Cc: Hans de Goede <hdegoede@redhat.com> Should this go to stable? Acked-by: Heikki Krogerus@linux.intel.com > --- > drivers/usb/typec/tcpm/fusb302.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > index 7a2a17866a823..72f9001b07921 100644 > --- a/drivers/usb/typec/tcpm/fusb302.c > +++ b/drivers/usb/typec/tcpm/fusb302.c > @@ -669,25 +669,27 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc) > ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK, > FUSB_REG_MASK_BC_LVL | > FUSB_REG_MASK_COMP_CHNG, > - FUSB_REG_MASK_COMP_CHNG); > + FUSB_REG_MASK_BC_LVL); > if (ret < 0) { > fusb302_log(chip, "cannot set SRC interrupt, ret=%d", > ret); > goto done; > } > chip->intr_comp_chng = true; > + chip->intr_bc_lvl = false; > break; > case TYPEC_CC_RD: > ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK, > FUSB_REG_MASK_BC_LVL | > FUSB_REG_MASK_COMP_CHNG, > - FUSB_REG_MASK_BC_LVL); > + FUSB_REG_MASK_COMP_CHNG); > if (ret < 0) { > fusb302_log(chip, "cannot set SRC interrupt, ret=%d", > ret); > goto done; > } > chip->intr_bc_lvl = true; > + chip->intr_comp_chng = false; > break; > default: > break; thanks,
On Mon, Nov 15, 2021 at 09:11:48AM +0200, Heikki Krogerus wrote: > On Mon, Nov 08, 2021 at 11:28:32AM +0100, Ondrej Jirman wrote: > > The code that enables either BC_LVL or COMP_CHNG interrupt in tcpm_set_cc > > wrongly assumes that the interrupt is unmasked by writing 1 to the apropriate > > bit in the mask register. In fact, interrupts are enabled when the mask > > is 0, so the tcpm_set_cc enables interrupt for COMP_CHNG when it expects > > BC_LVL interrupt to be enabled. > > > > This causes inability of the driver to recognize cable unplug events > > in host mode (unplug is recognized only via a COMP_CHNG interrupt). > > > > In device mode this bug was masked by simultaneous triggering of the VBUS > > change interrupt, because of loss of VBUS when the port peer is providing > > power. > > > > Fixes: 48242e30532b ("usb: typec: fusb302: Revert "Resolve fixed power role contract setup"") > > Signed-off-by: Ondrej Jirman <megous@megous.com> > > Cc: Hans de Goede <hdegoede@redhat.com> > > Should this go to stable? > > Acked-by: Heikki Krogerus@linux.intel.com Sorry, that's wrong... Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > --- > > drivers/usb/typec/tcpm/fusb302.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > index 7a2a17866a823..72f9001b07921 100644 > > --- a/drivers/usb/typec/tcpm/fusb302.c > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > @@ -669,25 +669,27 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc) > > ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK, > > FUSB_REG_MASK_BC_LVL | > > FUSB_REG_MASK_COMP_CHNG, > > - FUSB_REG_MASK_COMP_CHNG); > > + FUSB_REG_MASK_BC_LVL); > > if (ret < 0) { > > fusb302_log(chip, "cannot set SRC interrupt, ret=%d", > > ret); > > goto done; > > } > > chip->intr_comp_chng = true; > > + chip->intr_bc_lvl = false; > > break; > > case TYPEC_CC_RD: > > ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK, > > FUSB_REG_MASK_BC_LVL | > > FUSB_REG_MASK_COMP_CHNG, > > - FUSB_REG_MASK_BC_LVL); > > + FUSB_REG_MASK_COMP_CHNG); > > if (ret < 0) { > > fusb302_log(chip, "cannot set SRC interrupt, ret=%d", > > ret); > > goto done; > > } > > chip->intr_bc_lvl = true; > > + chip->intr_comp_chng = false; > > break; > > default: > > break; > > thanks,
On Mon, Nov 15, 2021 at 09:11:42AM +0200, Heikki Krogerus wrote: > On Mon, Nov 08, 2021 at 11:28:32AM +0100, Ondrej Jirman wrote: > > The code that enables either BC_LVL or COMP_CHNG interrupt in tcpm_set_cc > > wrongly assumes that the interrupt is unmasked by writing 1 to the apropriate > > bit in the mask register. In fact, interrupts are enabled when the mask > > is 0, so the tcpm_set_cc enables interrupt for COMP_CHNG when it expects > > BC_LVL interrupt to be enabled. > > > > This causes inability of the driver to recognize cable unplug events > > in host mode (unplug is recognized only via a COMP_CHNG interrupt). > > > > In device mode this bug was masked by simultaneous triggering of the VBUS > > change interrupt, because of loss of VBUS when the port peer is providing > > power. > > > > Fixes: 48242e30532b ("usb: typec: fusb302: Revert "Resolve fixed power role contract setup"") > > Signed-off-by: Ondrej Jirman <megous@megous.com> > > Cc: Hans de Goede <hdegoede@redhat.com> > > Should this go to stable? Without this patch, VBUS is not turned off when I disconnect a hub from the Type-C port (because fusb302 will not notice the disconnect), and it stays on until next plugin of some device, say a normal non PD charger. So I guess for a brief period you can have both sides provide VBUS (until fusb302/tcpm processes the next plugin). It may be a problem if VBUS was more than 5V (not very likely for devices running this driver, I guess). regards, o. > Acked-by: Heikki Krogerus@linux.intel.com > > > --- > > drivers/usb/typec/tcpm/fusb302.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > index 7a2a17866a823..72f9001b07921 100644 > > --- a/drivers/usb/typec/tcpm/fusb302.c > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > @@ -669,25 +669,27 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc) > > ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK, > > FUSB_REG_MASK_BC_LVL | > > FUSB_REG_MASK_COMP_CHNG, > > - FUSB_REG_MASK_COMP_CHNG); > > + FUSB_REG_MASK_BC_LVL); > > if (ret < 0) { > > fusb302_log(chip, "cannot set SRC interrupt, ret=%d", > > ret); > > goto done; > > } > > chip->intr_comp_chng = true; > > + chip->intr_bc_lvl = false; > > break; > > case TYPEC_CC_RD: > > ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK, > > FUSB_REG_MASK_BC_LVL | > > FUSB_REG_MASK_COMP_CHNG, > > - FUSB_REG_MASK_BC_LVL); > > + FUSB_REG_MASK_COMP_CHNG); > > if (ret < 0) { > > fusb302_log(chip, "cannot set SRC interrupt, ret=%d", > > ret); > > goto done; > > } > > chip->intr_bc_lvl = true; > > + chip->intr_comp_chng = false; > > break; > > default: > > break; > > thanks, > > -- > heikki
On Mon, Nov 15, 2021 at 09:55:32AM +0100, Ondřej Jirman wrote: > On Mon, Nov 15, 2021 at 09:11:42AM +0200, Heikki Krogerus wrote: > > On Mon, Nov 08, 2021 at 11:28:32AM +0100, Ondrej Jirman wrote: > > > The code that enables either BC_LVL or COMP_CHNG interrupt in tcpm_set_cc > > > wrongly assumes that the interrupt is unmasked by writing 1 to the apropriate > > > bit in the mask register. In fact, interrupts are enabled when the mask > > > is 0, so the tcpm_set_cc enables interrupt for COMP_CHNG when it expects > > > BC_LVL interrupt to be enabled. > > > > > > This causes inability of the driver to recognize cable unplug events > > > in host mode (unplug is recognized only via a COMP_CHNG interrupt). > > > > > > In device mode this bug was masked by simultaneous triggering of the VBUS > > > change interrupt, because of loss of VBUS when the port peer is providing > > > power. > > > > > > Fixes: 48242e30532b ("usb: typec: fusb302: Revert "Resolve fixed power role contract setup"") > > > Signed-off-by: Ondrej Jirman <megous@megous.com> > > > Cc: Hans de Goede <hdegoede@redhat.com> > > > > Should this go to stable? > > Without this patch, VBUS is not turned off when I disconnect a hub from the > Type-C port (because fusb302 will not notice the disconnect), and it stays on > until next plugin of some device, say a normal non PD charger. > > So I guess for a brief period you can have both sides provide VBUS (until > fusb302/tcpm processes the next plugin). It may be a problem if VBUS was more > than 5V (not very likely for devices running this driver, I guess). OK. So this should find it's way to the stable kernel releases as well, and it should have the appropriate "Cc: stable@vger.kernel.org" tag: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#select-the-recipients-for-your-patch thanks,
diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c index 7a2a17866a823..72f9001b07921 100644 --- a/drivers/usb/typec/tcpm/fusb302.c +++ b/drivers/usb/typec/tcpm/fusb302.c @@ -669,25 +669,27 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum typec_cc_status cc) ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK, FUSB_REG_MASK_BC_LVL | FUSB_REG_MASK_COMP_CHNG, - FUSB_REG_MASK_COMP_CHNG); + FUSB_REG_MASK_BC_LVL); if (ret < 0) { fusb302_log(chip, "cannot set SRC interrupt, ret=%d", ret); goto done; } chip->intr_comp_chng = true; + chip->intr_bc_lvl = false; break; case TYPEC_CC_RD: ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK, FUSB_REG_MASK_BC_LVL | FUSB_REG_MASK_COMP_CHNG, - FUSB_REG_MASK_BC_LVL); + FUSB_REG_MASK_COMP_CHNG); if (ret < 0) { fusb302_log(chip, "cannot set SRC interrupt, ret=%d", ret); goto done; } chip->intr_bc_lvl = true; + chip->intr_comp_chng = false; break; default: break;
The code that enables either BC_LVL or COMP_CHNG interrupt in tcpm_set_cc wrongly assumes that the interrupt is unmasked by writing 1 to the apropriate bit in the mask register. In fact, interrupts are enabled when the mask is 0, so the tcpm_set_cc enables interrupt for COMP_CHNG when it expects BC_LVL interrupt to be enabled. This causes inability of the driver to recognize cable unplug events in host mode (unplug is recognized only via a COMP_CHNG interrupt). In device mode this bug was masked by simultaneous triggering of the VBUS change interrupt, because of loss of VBUS when the port peer is providing power. Fixes: 48242e30532b ("usb: typec: fusb302: Revert "Resolve fixed power role contract setup"") Signed-off-by: Ondrej Jirman <megous@megous.com> Cc: Hans de Goede <hdegoede@redhat.com> --- drivers/usb/typec/tcpm/fusb302.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)