| 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
| ||
|
Date: Fri, 28 Jun 2019 21:38:26 -0300
From: Ezequiel Garcia <ezequiel@...labora.com>
To: Kishon Vijay Abraham I <kishon@...com>,
Heiko Stuebner <heiko@...ech.de>
Cc: Enric Balletbo i Serra <enric.balletbo@...labora.com>,
kernel@...labora.com,
Gaël PORTAY <gael.portay@...labora.com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v2] phy: rockchip-inno-usb2: allow to force the B-Device
Session Valid bit.
Hi Heiko, Kishon,
I'll try to pick up this patch.
Some comments below, just for self-reference.
On Wed, 2019-06-26 at 12:32 -0300, Ezequiel Garcia wrote:
> On Wed, 2019-05-15 at 18:20 -0400, Gaël PORTAY wrote:
> > From: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> >
> > The OTG disconnection event is generated after the presence/absence of
> > an ID connection, but some platforms don't have the ID pin connected, so
> > the event is not generated. In such case, for detecting the
> > disconnection event, we can get the cable state from an extcon driver.
> > We need, though, to force to set the B-Device Session Valid bit on the
> > PHY to have the device respond to the setup address. Otherwise, the
> > following error is shown:
> >
> > usb 2-2: Device not responding to setup address.
> > usb 2-2: device not accepting address 14, error -71
> > usb usb2-port2: unable to enumerate USB device
> >
> > The patch tells the PHY to force the B-Device Session Valid bit when the
> > OTG role is device and clear that bit if the OTG role is host, when an
> > extcon is available.
> >
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> > Signed-off-by: Gaël PORTAY <gael.portay@...labora.com>
> > ---
> >
> > Hi all,
> >
> > The main purpose of this patch is have the Type-C port on the Samsung
> > Chromebook Plus work as a device or in OTG mode.
> >
> > That patch was originally a part of that patchset[1]; all other patches
> > was merged recently in master.
> >
> > The patch was tested on a Samsung Chromebook Plus by configuring one
> > port to work as device, configure a cdc ethernet gadget and communicate
> > via ethernet gadget my workstation with the chromebook through a usb-a
> > to type-c cable.
> >
> > Best regards,
> > Gaël
> >
> > [1]: https://lkml.org/lkml/2018/8/15/141
> >
We still need the above devicetree changes.
> > Changes since v1:
> > - [PATCH 3/4] Remove introduction of dt property "rockchip,force-bvalid"
> > and replace cable state using extcon instead (if set).
> >
> > drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 51 +++++++++++++++++++
> > 1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > index ba07121c3eff..5e9d50b5ae16 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > @@ -125,6 +125,7 @@ struct rockchip_chg_det_reg {
> > * @bvalid_det_en: vbus valid rise detection enable register.
> > * @bvalid_det_st: vbus valid rise detection status register.
> > * @bvalid_det_clr: vbus valid rise detection clear register.
> > + * @bvalid_session: force B-device session valid register.
> > * @ls_det_en: linestate detection enable register.
> > * @ls_det_st: linestate detection state register.
> > * @ls_det_clr: linestate detection clear register.
> > @@ -138,6 +139,7 @@ struct rockchip_usb2phy_port_cfg {
> > struct usb2phy_reg bvalid_det_en;
> > struct usb2phy_reg bvalid_det_st;
> > struct usb2phy_reg bvalid_det_clr;
> > + struct usb2phy_reg bvalid_session;
> > struct usb2phy_reg ls_det_en;
> > struct usb2phy_reg ls_det_st;
> > struct usb2phy_reg ls_det_clr;
> > @@ -169,6 +171,7 @@ struct rockchip_usb2phy_cfg {
> > * @port_id: flag for otg port or host port.
> > * @suspended: phy suspended flag.
> > * @vbus_attached: otg device vbus status.
> > + * @force_bvalid: force the control of the B-device session valid bit.
> > * @bvalid_irq: IRQ number assigned for vbus valid rise detection.
> > * @ls_irq: IRQ number assigned for linestate detection.
> > * @otg_mux_irq: IRQ number which multiplex otg-id/otg-bvalid/linestate
> > @@ -187,6 +190,7 @@ struct rockchip_usb2phy_port {
> > unsigned int port_id;
> > bool suspended;
> > bool vbus_attached;
> > + bool force_bvalid;
> > int bvalid_irq;
> > int ls_irq;
> > int otg_mux_irq;
> > @@ -553,6 +557,13 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
> > switch (rport->state) {
> > case OTG_STATE_UNDEFINED:
> > rport->state = OTG_STATE_B_IDLE;
> > + if (rport->force_bvalid) {
> > + property_enable(rphy->grf,
> > + &rport->port_cfg->bvalid_session,
> > + true);
> > + dev_dbg(&rport->phy->dev,
> > + "set the B-Device Session Valid\n");
> > + }
> > if (!vbus_attach)
> > rockchip_usb2phy_power_off(rport->phy);
> > /* fall through */
> > @@ -560,6 +571,14 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
> > if (extcon_get_state(rphy->edev, EXTCON_USB_HOST) > 0) {
> > dev_dbg(&rport->phy->dev, "usb otg host connect\n");
> > rport->state = OTG_STATE_A_HOST;
> > + /* When leaving device mode force end the session */
> > + if (rport->force_bvalid) {
> > + property_enable(rphy->grf,
> > + &rport->port_cfg->bvalid_session,
> > + false);
> > + dev_dbg(&rport->phy->dev,
> > + "clear the B-Device Session Valid\n");
> > + }
> > rockchip_usb2phy_power_on(rport->phy);
> > return;
> > } else if (vbus_attach) {
> > @@ -634,6 +653,14 @@ static void rockchip_usb2phy_otg_sm_work(struct work_struct *work)
> > if (extcon_get_state(rphy->edev, EXTCON_USB_HOST) == 0) {
> > dev_dbg(&rport->phy->dev, "usb otg host disconnect\n");
> > rport->state = OTG_STATE_B_IDLE;
> > + /* When leaving host mode force start the session */
> > + if (rport->force_bvalid) {
> > + property_enable(rphy->grf,
> > + &rport->port_cfg->bvalid_session,
> > + true);
> > + dev_dbg(&rport->phy->dev,
> > + "set the B-Device Session Valid\n");
> > + }
> > rockchip_usb2phy_power_off(rport->phy);
> > }
> > break;
> > @@ -1016,6 +1043,28 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
> > INIT_DELAYED_WORK(&rport->chg_work, rockchip_chg_detect_work);
> > INIT_DELAYED_WORK(&rport->otg_sm_work, rockchip_usb2phy_otg_sm_work);
> >
> > + /*
> > + * Some platforms doesn't have the ID pin connected to the phy, hence
> > + * the OTD ID event is not generated, but in some cases we can get the
> > + * cable state from an extcon driver. In such case we can force to set
> > + * the B-Device Session Valid bit on the PHY to have the device working
> > + * as a OTG.
> > + */
> > + if (rphy->edev) {
I might be missing something, but this check seems bogus.
edev can't be NULL as the driver creates an extcon if
there is none assigned in the devicetree.
> > + /*
> > + * Check if bvalid_session register is set in the structure
> > + * rockchip_usb2phy_cfg for this SoC.
> > + */
> > + if (rport->port_cfg->bvalid_session.offset == 0x0) {
> > + rport->force_bvalid = false;
> > + dev_err(rphy->dev,
> > + "cannot force B-device session, the register is not set for that SoC\n");
> > + } else {
> > + rport->force_bvalid = true;
> > + dev_info(rphy->dev, "force B-device session enabled\n");
> > + }
I think we should be doing something more like:
if (HAS_REG(rport->port_cfg->bvalid_session)) {
rport->force_bvalid = true;
dev_info(rphy->dev, "force B-device session enabled\n");
}
And not error verbosely on platforms that don't
care about this.
Thanks,
Ezequiel
Powered by blists - more mailing lists