* [PATCH 4/4] usb: typec: hd3ss3220: Add polling support
From: Biju Das @ 2022-12-09 15:56 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Biju Das, Greg Kroah-Hartman, linux-usb, Geert Uytterhoeven,
Fabrizio Castro, linux-renesas-soc
In-Reply-To: <20221209155623.29147-1-biju.das.jz@bp.renesas.com>
Some platforms(for eg: RZ/V2M EVK) does not have interrupt pin
connected to HD3SS3220. Add polling support for role detection.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
drivers/usb/typec/hd3ss3220.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index 666a93f7ec65..9894fa96cc5f 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -14,6 +14,7 @@
#include <linux/slab.h>
#include <linux/usb/role.h>
#include <linux/usb/typec.h>
+#include <linux/workqueue.h>
#define HD3SS3220_REG_CN_STAT_CTRL 0x09
#define HD3SS3220_REG_GEN_CTRL 0x0A
@@ -37,6 +38,9 @@ struct hd3ss3220 {
struct regmap *regmap;
struct usb_role_switch *role_sw;
struct typec_port *port;
+ struct delayed_work output_poll_work;
+ enum usb_role role_state;
+ bool poll;
};
static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
@@ -118,6 +122,22 @@ static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
default:
break;
}
+
+ hd3ss3220->role_state = role_state;
+}
+
+static void output_poll_execute(struct work_struct *work)
+{
+ struct delayed_work *delayed_work = to_delayed_work(work);
+ struct hd3ss3220 *hd3ss3220 = container_of(delayed_work,
+ struct hd3ss3220,
+ output_poll_work);
+ enum usb_role role_state = hd3ss3220_get_attached_state(hd3ss3220);
+
+ if (hd3ss3220->role_state != role_state)
+ hd3ss3220_set_role(hd3ss3220);
+
+ schedule_delayed_work(&hd3ss3220->output_poll_work, HZ);
}
static irqreturn_t hd3ss3220_irq(struct hd3ss3220 *hd3ss3220)
@@ -227,6 +247,9 @@ static int hd3ss3220_probe(struct i2c_client *client,
"hd3ss3220", &client->dev);
if (ret)
goto err_unreg_port;
+ } else {
+ INIT_DELAYED_WORK(&hd3ss3220->output_poll_work, output_poll_execute);
+ hd3ss3220->poll = true;
}
ret = i2c_smbus_read_byte_data(client, HD3SS3220_REG_DEV_REV);
@@ -235,6 +258,9 @@ static int hd3ss3220_probe(struct i2c_client *client,
fwnode_handle_put(connector);
+ if (hd3ss3220->poll)
+ schedule_delayed_work(&hd3ss3220->output_poll_work, HZ);
+
dev_info(&client->dev, "probed revision=0x%x\n", ret);
return 0;
@@ -252,6 +278,9 @@ static void hd3ss3220_remove(struct i2c_client *client)
{
struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
+ if (hd3ss3220->poll)
+ cancel_delayed_work_sync(&hd3ss3220->output_poll_work);
+
typec_unregister_port(hd3ss3220->port);
usb_role_switch_put(hd3ss3220->role_sw);
}
--
2.25.1
^ permalink raw reply related
* [PATCH 3/4] usb: typec: hd3ss3220: Sort header files
From: Biju Das @ 2022-12-09 15:56 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Biju Das, Greg Kroah-Hartman, linux-usb, Geert Uytterhoeven,
Fabrizio Castro, linux-renesas-soc
In-Reply-To: <20221209155623.29147-1-biju.das.jz@bp.renesas.com>
This patch sorts the header files alphabetically.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
drivers/usb/typec/hd3ss3220.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index c24bbccd14f9..666a93f7ec65 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -5,15 +5,15 @@
* Copyright (C) 2019 Renesas Electronics Corp.
*/
-#include <linux/module.h>
+#include <linux/delay.h>
#include <linux/i2c.h>
-#include <linux/usb/role.h>
-#include <linux/irqreturn.h>
#include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/module.h>
#include <linux/regmap.h>
#include <linux/slab.h>
+#include <linux/usb/role.h>
#include <linux/usb/typec.h>
-#include <linux/delay.h>
#define HD3SS3220_REG_CN_STAT_CTRL 0x09
#define HD3SS3220_REG_GEN_CTRL 0x0A
--
2.25.1
^ permalink raw reply related
* [PATCH 2/4] usb: typec: hd3ss3220: Fix NULL pointer crash
From: Biju Das @ 2022-12-09 15:56 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Biju Das, Greg Kroah-Hartman, Biju Das, linux-usb,
Geert Uytterhoeven, Fabrizio Castro, linux-renesas-soc
In-Reply-To: <20221209155623.29147-1-biju.das.jz@bp.renesas.com>
The value returned by usb_role_switch_get() can be NULL and it leads
to NULL pointer crash. This patch fixes this issue by adding NULL
check for the role switch handle.
[ 25.336613] Hardware name: Silicon Linux RZ/G2E evaluation kit EK874 (CAT874 + CAT875) (DT)
[ 25.344991] Workqueue: events_unbound deferred_probe_work_func
[ 25.350869] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 25.357854] pc : renesas_usb3_role_switch_get+0x40/0x80 [renesas_usb3]
[ 25.364428] lr : renesas_usb3_role_switch_get+0x24/0x80 [renesas_usb3]
[ 25.370986] sp : ffff80000a4b3a40
[ 25.374311] x29: ffff80000a4b3a40 x28: 0000000000000000 x27: 0000000000000000
[ 25.381476] x26: ffff80000a3ade78 x25: ffff00000a809005 x24: ffff80000117f178
[ 25.388641] x23: ffff00000a8d7810 x22: ffff00000a8d8410 x21: 0000000000000000
[ 25.395805] x20: ffff000011cd7080 x19: ffff000011cd7080 x18: 0000000000000020
[ 25.402969] x17: ffff800076196000 x16: ffff800008004000 x15: 0000000000004000
[ 25.410133] x14: 000000000000022b x13: 0000000000000001 x12: 0000000000000001
[ 25.417291] x11: 0000000000000000 x10: 0000000000000a40 x9 : ffff80000a4b3770
[ 25.424452] x8 : ffff00007fbc9000 x7 : 0040000000000008 x6 : ffff00000a8d8590
[ 25.431615] x5 : ffff80000a4b3960 x4 : 0000000000000000 x3 : ffff00000a8d84f4
[ 25.438776] x2 : 0000000000000218 x1 : ffff80000a715218 x0 : 0000000000000218
[ 25.445942] Call trace:
[ 25.448398] renesas_usb3_role_switch_get+0x40/0x80 [renesas_usb3]
[ 25.454613] renesas_usb3_role_switch_set+0x4c/0x440 [renesas_usb3]
[ 25.460908] usb_role_switch_set_role+0x44/0xa4
[ 25.465468] hd3ss3220_set_role+0xa0/0x100 [hd3ss3220]
[ 25.470635] hd3ss3220_probe+0x118/0x2fc [hd3ss3220]
[ 25.475621] i2c_device_probe+0x338/0x384
Fixes: 5a9a8a4c5058 ("usb: typec: hd3ss3220: hd3ss3220_probe() warn: passing zero to 'PTR_ERR'")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
This issue triggered on RZ/G2E board, where there is no USB3 firmware and it
returned a null role switch handle.
---
drivers/usb/typec/hd3ss3220.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
index 2a58185fb14c..c24bbccd14f9 100644
--- a/drivers/usb/typec/hd3ss3220.c
+++ b/drivers/usb/typec/hd3ss3220.c
@@ -186,7 +186,10 @@ static int hd3ss3220_probe(struct i2c_client *client,
hd3ss3220->role_sw = usb_role_switch_get(hd3ss3220->dev);
}
- if (IS_ERR(hd3ss3220->role_sw)) {
+ if (!hd3ss3220->role_sw) {
+ ret = -ENODEV;
+ goto err_put_fwnode;
+ } else if (IS_ERR(hd3ss3220->role_sw)) {
ret = PTR_ERR(hd3ss3220->role_sw);
goto err_put_fwnode;
}
--
2.25.1
^ permalink raw reply related
* [PATCH 1/4] dt-bindings: usb: ti,hd3ss3220: Update interrupt property as optional
From: Biju Das @ 2022-12-09 15:56 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski
Cc: Biju Das, Greg Kroah-Hartman, linux-usb, devicetree,
Geert Uytterhoeven, Fabrizio Castro, linux-renesas-soc
In-Reply-To: <20221209155623.29147-1-biju.das.jz@bp.renesas.com>
On some platforms(rg: RZ/V2M EVK), interrupt is not populated. Update
the binding to make interrupt property as optional.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
Documentation/devicetree/bindings/usb/ti,hd3ss3220.yaml | 1 -
1 file changed, 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/usb/ti,hd3ss3220.yaml b/Documentation/devicetree/bindings/usb/ti,hd3ss3220.yaml
index b86bf6bc9cd6..a1cffb70c621 100644
--- a/Documentation/devicetree/bindings/usb/ti,hd3ss3220.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,hd3ss3220.yaml
@@ -46,7 +46,6 @@ properties:
required:
- compatible
- reg
- - interrupts
additionalProperties: false
--
2.25.1
^ permalink raw reply related
* [PATCH 0/4] Add Polling support for role detection with HD3SS3220
From: Biju Das @ 2022-12-09 15:56 UTC (permalink / raw)
To: Rob Herring, Heikki Krogerus, Krzysztof Kozlowski
Cc: Biju Das, Greg Kroah-Hartman, linux-usb, devicetree,
Geert Uytterhoeven, Fabrizio Castro, linux-renesas-soc
This patch series aims to add polling support for role detection
with HD3SS3220.
Whilst it also fix a null kernel crash and also sort the headers.
Biju Das (4):
dt-bindings: usb: ti,hd3ss3220: Update interrupt property as optional
usb: typec: hd3ss3220: Fix NULL pointer crash
usb: typec: hd3ss3220: Sort header files
usb: typec: hd3ss3220: Add polling support
.../devicetree/bindings/usb/ti,hd3ss3220.yaml | 1 -
drivers/usb/typec/hd3ss3220.c | 42 ++++++++++++++++---
2 files changed, 37 insertions(+), 6 deletions(-)
--
2.25.1
^ permalink raw reply
* Re: [PATCH v3 1/2] usb: serial: add support for CH348
From: Corentin LABBE @ 2022-12-09 14:18 UTC (permalink / raw)
To: Johan Hovold; +Cc: gregkh, linux-kernel, linux-usb
In-Reply-To: <Y4DETJvl12YfnMxF@hovoldconsulting.com>
Le Fri, Nov 25, 2022 at 02:34:04PM +0100, Johan Hovold a écrit :
> On Mon, Nov 14, 2022 at 07:37:35AM +0000, Corentin Labbe wrote:
> > The CH348 is an USB octo port serial adapter.
>
[...]
Hello
Thanks for your review, I have fixed all problems.
Thanks again for pointing mxuport, the code is shorter now.
Regards
^ permalink raw reply
* Re: [PATCH v6 1/3] usb: typec: tcpm: Add callbacks to mitigate wakeups due to contaminant
From: Guenter Roeck @ 2022-12-09 13:44 UTC (permalink / raw)
To: Badhri Jagan Sridharan
Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel,
Kyle Tso
In-Reply-To: <CAPTae5J-YOuifSRNJfsESs9YVJXt56BFMrx0f0GeEnK-zsotyQ@mail.gmail.com>
On 12/8/22 18:39, Badhri Jagan Sridharan wrote:
> On Thu, Dec 8, 2022 at 3:56 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 12/8/22 07:04, Badhri Jagan Sridharan wrote:
>>> On some of the TCPC implementations, when the Type-C port is exposed
>>> to contaminants, such as water, TCPC stops toggling while reporting OPEN
>>> either by the time TCPM reads CC pin status or during CC debounce
>>> window. This causes TCPM to be stuck in TOGGLING state. If TCPM is made
>>> to restart toggling, the behavior recurs causing redundant CPU wakeups
>>> till the USB-C port is free of contaminant.
>>>
>>> [206199.287817] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
>>> [206199.640337] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
>>> [206199.985789] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
>>> ...
>>>
>>> TCPM invokes is_potential_contaminant callback to allow low level chip
>>> drivers to monitor TCPM state machine transitions and notify TCPM when
>>> the Type-C port needs to be checked for potential contaminant presence.
>>> TCPCs which do have the needed hardware can implement the check_contaminant
>>> callback which is invoked by TCPM to evaluate for presence of contaminant.
>>> Lower level TCPC driver can restart toggling through TCPM_PORT_CLEAN event
>>> when the driver detects that USB-C port is free of contaminant.
>>>
>>> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
>>> ---
>>> Changes since v5:
>>> * Updated commit message. Removed change id.
>>> Changes since v4:
>>> * None
>>> Changes since v3:
>>> * None
>>> Changes since V2:
>>> * Offloaded tcpm from maintaining disconnect_while_debouncing logic
>>> * to lower level maxim tcpc driver based on feedback.
>>> ---
>>> drivers/usb/typec/tcpm/tcpm.c | 162 +++++++++-------------------------
>>> include/linux/usb/tcpm.h | 133 ++++++++++++++++++++++++++++
>>> 2 files changed, 177 insertions(+), 118 deletions(-)
>>>
>>> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
>>> index 904c7b4ce2f0..a138cea49612 100644
>>> --- a/drivers/usb/typec/tcpm/tcpm.c
>>> +++ b/drivers/usb/typec/tcpm/tcpm.c
>>> @@ -33,119 +33,6 @@
>>>
>>> #include <uapi/linux/sched/types.h>
>>>
>>> -#define FOREACH_STATE(S) \
>>> - S(INVALID_STATE), \
>>> - S(TOGGLING), \
>>> - S(SRC_UNATTACHED), \
>>> - S(SRC_ATTACH_WAIT), \
>>> - S(SRC_ATTACHED), \
>>> - S(SRC_STARTUP), \
>>> - S(SRC_SEND_CAPABILITIES), \
>>> - S(SRC_SEND_CAPABILITIES_TIMEOUT), \
>>> - S(SRC_NEGOTIATE_CAPABILITIES), \
>>> - S(SRC_TRANSITION_SUPPLY), \
>>> - S(SRC_READY), \
>>> - S(SRC_WAIT_NEW_CAPABILITIES), \
>>> - \
>>> - S(SNK_UNATTACHED), \
>>> - S(SNK_ATTACH_WAIT), \
>>> - S(SNK_DEBOUNCED), \
>>> - S(SNK_ATTACHED), \
>>> - S(SNK_STARTUP), \
>>> - S(SNK_DISCOVERY), \
>>> - S(SNK_DISCOVERY_DEBOUNCE), \
>>> - S(SNK_DISCOVERY_DEBOUNCE_DONE), \
>>> - S(SNK_WAIT_CAPABILITIES), \
>>> - S(SNK_NEGOTIATE_CAPABILITIES), \
>>> - S(SNK_NEGOTIATE_PPS_CAPABILITIES), \
>>> - S(SNK_TRANSITION_SINK), \
>>> - S(SNK_TRANSITION_SINK_VBUS), \
>>> - S(SNK_READY), \
>>> - \
>>> - S(ACC_UNATTACHED), \
>>> - S(DEBUG_ACC_ATTACHED), \
>>> - S(AUDIO_ACC_ATTACHED), \
>>> - S(AUDIO_ACC_DEBOUNCE), \
>>> - \
>>> - S(HARD_RESET_SEND), \
>>> - S(HARD_RESET_START), \
>>> - S(SRC_HARD_RESET_VBUS_OFF), \
>>> - S(SRC_HARD_RESET_VBUS_ON), \
>>> - S(SNK_HARD_RESET_SINK_OFF), \
>>> - S(SNK_HARD_RESET_WAIT_VBUS), \
>>> - S(SNK_HARD_RESET_SINK_ON), \
>>> - \
>>> - S(SOFT_RESET), \
>>> - S(SRC_SOFT_RESET_WAIT_SNK_TX), \
>>> - S(SNK_SOFT_RESET), \
>>> - S(SOFT_RESET_SEND), \
>>> - \
>>> - S(DR_SWAP_ACCEPT), \
>>> - S(DR_SWAP_SEND), \
>>> - S(DR_SWAP_SEND_TIMEOUT), \
>>> - S(DR_SWAP_CANCEL), \
>>> - S(DR_SWAP_CHANGE_DR), \
>>> - \
>>> - S(PR_SWAP_ACCEPT), \
>>> - S(PR_SWAP_SEND), \
>>> - S(PR_SWAP_SEND_TIMEOUT), \
>>> - S(PR_SWAP_CANCEL), \
>>> - S(PR_SWAP_START), \
>>> - S(PR_SWAP_SRC_SNK_TRANSITION_OFF), \
>>> - S(PR_SWAP_SRC_SNK_SOURCE_OFF), \
>>> - S(PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED), \
>>> - S(PR_SWAP_SRC_SNK_SINK_ON), \
>>> - S(PR_SWAP_SNK_SRC_SINK_OFF), \
>>> - S(PR_SWAP_SNK_SRC_SOURCE_ON), \
>>> - S(PR_SWAP_SNK_SRC_SOURCE_ON_VBUS_RAMPED_UP), \
>>> - \
>>> - S(VCONN_SWAP_ACCEPT), \
>>> - S(VCONN_SWAP_SEND), \
>>> - S(VCONN_SWAP_SEND_TIMEOUT), \
>>> - S(VCONN_SWAP_CANCEL), \
>>> - S(VCONN_SWAP_START), \
>>> - S(VCONN_SWAP_WAIT_FOR_VCONN), \
>>> - S(VCONN_SWAP_TURN_ON_VCONN), \
>>> - S(VCONN_SWAP_TURN_OFF_VCONN), \
>>> - \
>>> - S(FR_SWAP_SEND), \
>>> - S(FR_SWAP_SEND_TIMEOUT), \
>>> - S(FR_SWAP_SNK_SRC_TRANSITION_TO_OFF), \
>>> - S(FR_SWAP_SNK_SRC_NEW_SINK_READY), \
>>> - S(FR_SWAP_SNK_SRC_SOURCE_VBUS_APPLIED), \
>>> - S(FR_SWAP_CANCEL), \
>>> - \
>>> - S(SNK_TRY), \
>>> - S(SNK_TRY_WAIT), \
>>> - S(SNK_TRY_WAIT_DEBOUNCE), \
>>> - S(SNK_TRY_WAIT_DEBOUNCE_CHECK_VBUS), \
>>> - S(SRC_TRYWAIT), \
>>> - S(SRC_TRYWAIT_DEBOUNCE), \
>>> - S(SRC_TRYWAIT_UNATTACHED), \
>>> - \
>>> - S(SRC_TRY), \
>>> - S(SRC_TRY_WAIT), \
>>> - S(SRC_TRY_DEBOUNCE), \
>>> - S(SNK_TRYWAIT), \
>>> - S(SNK_TRYWAIT_DEBOUNCE), \
>>> - S(SNK_TRYWAIT_VBUS), \
>>> - S(BIST_RX), \
>>> - \
>>> - S(GET_STATUS_SEND), \
>>> - S(GET_STATUS_SEND_TIMEOUT), \
>>> - S(GET_PPS_STATUS_SEND), \
>>> - S(GET_PPS_STATUS_SEND_TIMEOUT), \
>>> - \
>>> - S(GET_SINK_CAP), \
>>> - S(GET_SINK_CAP_TIMEOUT), \
>>> - \
>>> - S(ERROR_RECOVERY), \
>>> - S(PORT_RESET), \
>>> - S(PORT_RESET_WAIT_OFF), \
>>> - \
>>> - S(AMS_START), \
>>> - S(CHUNK_NOT_SUPP)
>>> -
>>> #define FOREACH_AMS(S) \
>>> S(NONE_AMS), \
>>> S(POWER_NEGOTIATION), \
>>> @@ -182,13 +69,8 @@
>>> S(COUNTRY_INFO), \
>>> S(COUNTRY_CODES)
>>>
>>> -#define GENERATE_ENUM(e) e
>>> #define GENERATE_STRING(s) #s
>>>
>>> -enum tcpm_state {
>>> - FOREACH_STATE(GENERATE_ENUM)
>>> -};
>>> -
>>> static const char * const tcpm_states[] = {
>>> FOREACH_STATE(GENERATE_STRING)
>>> };
>>> @@ -249,6 +131,7 @@ enum frs_typec_current {
>>> #define TCPM_RESET_EVENT BIT(2)
>>> #define TCPM_FRS_EVENT BIT(3)
>>> #define TCPM_SOURCING_VBUS BIT(4)
>>> +#define TCPM_PORT_CLEAN BIT(5)
>>>
>>> #define LOG_BUFFER_ENTRIES 1024
>>> #define LOG_BUFFER_ENTRY_SIZE 128
>>> @@ -483,6 +366,14 @@ struct tcpm_port {
>>> * SNK_READY for non-pd link.
>>> */
>>> bool slow_charger_loop;
>>> +
>>> + /*
>>> + * When true indicates that the lower level drivers indicate potential presence
>>> + * of contaminant in the connector pins based on the tcpm state machine
>>> + * transitions.
>>> + */
>>> + bool potential_contaminant;
>>> +
>>> #ifdef CONFIG_DEBUG_FS
>>> struct dentry *dentry;
>>> struct mutex logbuffer_lock; /* log buffer access lock */
>>> @@ -3904,15 +3795,26 @@ static void run_state_machine(struct tcpm_port *port)
>>> unsigned int msecs;
>>> enum tcpm_state upcoming_state;
>>>
>>> + if (port->tcpc->is_potential_contaminant)
>>> + port->potential_contaminant =
>>> + port->tcpc->is_potential_contaminant(port->tcpc, port->state);
>>> +
>>> port->enter_state = port->state;
>>> switch (port->state) {
>>> case TOGGLING:
>>> break;
>>> + case CHECK_CONTAMINANT:
>>> + port->tcpc->check_contaminant(port->tcpc);
>>> + break;
>>> /* SRC states */
>>> case SRC_UNATTACHED:
>>> if (!port->non_pd_role_swap)
>>> tcpm_swap_complete(port, -ENOTCONN);
>>> tcpm_src_detach(port);
>>> + if (port->potential_contaminant && port->tcpc->check_contaminant) {
>>> + tcpm_set_state(port, CHECK_CONTAMINANT, 0);
>>> + break;
>>> + }
>>> if (tcpm_start_toggling(port, tcpm_rp_cc(port))) {
>>> tcpm_set_state(port, TOGGLING, 0);
>>> break;
>>> @@ -4150,6 +4052,10 @@ static void run_state_machine(struct tcpm_port *port)
>>> tcpm_swap_complete(port, -ENOTCONN);
>>> tcpm_pps_complete(port, -ENOTCONN);
>>> tcpm_snk_detach(port);
>>> + if (port->potential_contaminant && port->tcpc->check_contaminant) {
>>> + tcpm_set_state(port, CHECK_CONTAMINANT, 0);
>>> + break;
>>> + }
>>> if (tcpm_start_toggling(port, TYPEC_CC_RD)) {
>>> tcpm_set_state(port, TOGGLING, 0);
>>> break;
>>> @@ -4926,6 +4832,9 @@ static void _tcpm_cc_change(struct tcpm_port *port, enum typec_cc_status cc1,
>>> else if (tcpm_port_is_sink(port))
>>> tcpm_set_state(port, SNK_ATTACH_WAIT, 0);
>>> break;
>>> + case CHECK_CONTAMINANT:
>>> + /* Wait for Toggling to be resumed */
>>> + break;
>>> case SRC_UNATTACHED:
>>> case ACC_UNATTACHED:
>>> if (tcpm_port_is_debug(port) || tcpm_port_is_audio(port) ||
>>> @@ -5425,6 +5334,10 @@ static void tcpm_pd_event_handler(struct kthread_work *work)
>>> port->vbus_source = true;
>>> _tcpm_pd_vbus_on(port);
>>> }
>>> + if (events & TCPM_PORT_CLEAN) {
>>> + tcpm_log(port, "port clean");
>>> + tcpm_set_state(port, TOGGLING, 0);
>>> + }
>>
>> That sets the state to TOGGLING unconditionally, even if it not currently
>> CHECK_CONTAMINANT. Is that a potential problem ?
>
> Good catch ! Would make sense to check whether tcpm is in
> CHECK_CONTAMINANT state before setting the TOGGLING state.
>>
>>>
>>> spin_lock(&port->pd_event_lock);
>>> }
>>> @@ -5477,6 +5390,19 @@ void tcpm_sourcing_vbus(struct tcpm_port *port)
>>> }
>>> EXPORT_SYMBOL_GPL(tcpm_sourcing_vbus);
>>>
>>> +/*
>>> + * Low level tcpc drivers invoke this once the port is deemed clean to return
>>> + * the port to TOGGLING state.
>>> + */
>>> +void tcpm_port_clean(struct tcpm_port *port)
>>> +{
>>> + spin_lock(&port->pd_event_lock);
>>> + port->pd_events |= TCPM_PORT_CLEAN;
>>> + spin_unlock(&port->pd_event_lock);
>>> + kthread_queue_work(port->wq, &port->event_work);
>>> +}
>>> +EXPORT_SYMBOL_GPL(tcpm_port_clean);
>>> +
>>> static void tcpm_enable_frs_work(struct kthread_work *work)
>>> {
>>> struct tcpm_port *port = container_of(work, struct tcpm_port, enable_frs);
>>> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
>>> index bffc8d3e14ad..9cf16372a6e4 100644
>>> --- a/include/linux/usb/tcpm.h
>>> +++ b/include/linux/usb/tcpm.h
>>> @@ -10,6 +10,126 @@
>>> #include <linux/usb/typec.h>
>>> #include "pd.h"
>>>
>>> +#define FOREACH_STATE(S) \
>>> + S(INVALID_STATE), \
>>> + S(TOGGLING), \
>>> + S(CHECK_CONTAMINANT), \
>>> + S(SRC_UNATTACHED), \
>>> + S(SRC_ATTACH_WAIT), \
>>> + S(SRC_ATTACHED), \
>>> + S(SRC_STARTUP), \
>>> + S(SRC_SEND_CAPABILITIES), \
>>> + S(SRC_SEND_CAPABILITIES_TIMEOUT), \
>>> + S(SRC_NEGOTIATE_CAPABILITIES), \
>>> + S(SRC_TRANSITION_SUPPLY), \
>>> + S(SRC_READY), \
>>> + S(SRC_WAIT_NEW_CAPABILITIES), \
>>> + \
>>> + S(SNK_UNATTACHED), \
>>> + S(SNK_ATTACH_WAIT), \
>>> + S(SNK_DEBOUNCED), \
>>> + S(SNK_ATTACHED), \
>>> + S(SNK_STARTUP), \
>>> + S(SNK_DISCOVERY), \
>>> + S(SNK_DISCOVERY_DEBOUNCE), \
>>> + S(SNK_DISCOVERY_DEBOUNCE_DONE), \
>>> + S(SNK_WAIT_CAPABILITIES), \
>>> + S(SNK_NEGOTIATE_CAPABILITIES), \
>>> + S(SNK_NEGOTIATE_PPS_CAPABILITIES), \
>>> + S(SNK_TRANSITION_SINK), \
>>> + S(SNK_TRANSITION_SINK_VBUS), \
>>> + S(SNK_READY), \
>>> + \
>>> + S(ACC_UNATTACHED), \
>>> + S(DEBUG_ACC_ATTACHED), \
>>> + S(AUDIO_ACC_ATTACHED), \
>>> + S(AUDIO_ACC_DEBOUNCE), \
>>> + \
>>> + S(HARD_RESET_SEND), \
>>> + S(HARD_RESET_START), \
>>> + S(SRC_HARD_RESET_VBUS_OFF), \
>>> + S(SRC_HARD_RESET_VBUS_ON), \
>>> + S(SNK_HARD_RESET_SINK_OFF), \
>>> + S(SNK_HARD_RESET_WAIT_VBUS), \
>>> + S(SNK_HARD_RESET_SINK_ON), \
>>> + \
>>> + S(SOFT_RESET), \
>>> + S(SRC_SOFT_RESET_WAIT_SNK_TX), \
>>> + S(SNK_SOFT_RESET), \
>>> + S(SOFT_RESET_SEND), \
>>> + \
>>> + S(DR_SWAP_ACCEPT), \
>>> + S(DR_SWAP_SEND), \
>>> + S(DR_SWAP_SEND_TIMEOUT), \
>>> + S(DR_SWAP_CANCEL), \
>>> + S(DR_SWAP_CHANGE_DR), \
>>> + \
>>> + S(PR_SWAP_ACCEPT), \
>>> + S(PR_SWAP_SEND), \
>>> + S(PR_SWAP_SEND_TIMEOUT), \
>>> + S(PR_SWAP_CANCEL), \
>>> + S(PR_SWAP_START), \
>>> + S(PR_SWAP_SRC_SNK_TRANSITION_OFF), \
>>> + S(PR_SWAP_SRC_SNK_SOURCE_OFF), \
>>> + S(PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED), \
>>> + S(PR_SWAP_SRC_SNK_SINK_ON), \
>>> + S(PR_SWAP_SNK_SRC_SINK_OFF), \
>>> + S(PR_SWAP_SNK_SRC_SOURCE_ON), \
>>> + S(PR_SWAP_SNK_SRC_SOURCE_ON_VBUS_RAMPED_UP), \
>>> + \
>>> + S(VCONN_SWAP_ACCEPT), \
>>> + S(VCONN_SWAP_SEND), \
>>> + S(VCONN_SWAP_SEND_TIMEOUT), \
>>> + S(VCONN_SWAP_CANCEL), \
>>> + S(VCONN_SWAP_START), \
>>> + S(VCONN_SWAP_WAIT_FOR_VCONN), \
>>> + S(VCONN_SWAP_TURN_ON_VCONN), \
>>> + S(VCONN_SWAP_TURN_OFF_VCONN), \
>>> + \
>>> + S(FR_SWAP_SEND), \
>>> + S(FR_SWAP_SEND_TIMEOUT), \
>>> + S(FR_SWAP_SNK_SRC_TRANSITION_TO_OFF), \
>>> + S(FR_SWAP_SNK_SRC_NEW_SINK_READY), \
>>> + S(FR_SWAP_SNK_SRC_SOURCE_VBUS_APPLIED), \
>>> + S(FR_SWAP_CANCEL), \
>>> + \
>>> + S(SNK_TRY), \
>>> + S(SNK_TRY_WAIT), \
>>> + S(SNK_TRY_WAIT_DEBOUNCE), \
>>> + S(SNK_TRY_WAIT_DEBOUNCE_CHECK_VBUS), \
>>> + S(SRC_TRYWAIT), \
>>> + S(SRC_TRYWAIT_DEBOUNCE), \
>>> + S(SRC_TRYWAIT_UNATTACHED), \
>>> + \
>>> + S(SRC_TRY), \
>>> + S(SRC_TRY_WAIT), \
>>> + S(SRC_TRY_DEBOUNCE), \
>>> + S(SNK_TRYWAIT), \
>>> + S(SNK_TRYWAIT_DEBOUNCE), \
>>> + S(SNK_TRYWAIT_VBUS), \
>>> + S(BIST_RX), \
>>> + \
>>> + S(GET_STATUS_SEND), \
>>> + S(GET_STATUS_SEND_TIMEOUT), \
>>> + S(GET_PPS_STATUS_SEND), \
>>> + S(GET_PPS_STATUS_SEND_TIMEOUT), \
>>> + \
>>> + S(GET_SINK_CAP), \
>>> + S(GET_SINK_CAP_TIMEOUT), \
>>> + \
>>> + S(ERROR_RECOVERY), \
>>> + S(PORT_RESET), \
>>> + S(PORT_RESET_WAIT_OFF), \
>>> + \
>>> + S(AMS_START), \
>>> + S(CHUNK_NOT_SUPP)
>>> +
>>> +#define GENERATE_ENUM(e) e
>>> +
>>> +enum tcpm_state {
>>> + FOREACH_STATE(GENERATE_ENUM)
>>> +};
>>> +
>>
>> Sorry for not bringing it up earlier; I have been struggling with it all along,
>> and I could not decide if I should bring it up or not.
>>
>> I really don't feel comfortable with exporting states outside tcpm.
>> Is this really necessary ? The only use seems to be
>>
>> + if ((tcpm_prev_state == SRC_ATTACH_WAIT && current_state == SRC_UNATTACHED) ||
>> + (tcpm_prev_state == SNK_ATTACH_WAIT && current_state == SNK_UNATTACHED))
>>
>> Plus, of course, the CHECK_CONTAMINANT state.
>>
>> Is there reason to believe that the relevant state transitions would be different
>> for other chips ? If not, would it possibly make sense to move this check
>> into the state machine ?
>
> In my understanding, this is definitely one of the transitions that I
> think will happen when the port has contaminant. It's quite possible
> that there are transitions that I am not aware of yet.
> I am OK moving it back to the tcpm state machine, however, then tcpm
> would have to keep track of the tcpm state machine transitions and
> that would look like V2 version of the patch
> (https://lore.kernel.org/lkml/20220831001555.285081-1-badhri@google.com/)
> where disconnect_while_debouncing is used to track this. Would that be
> OK ?
>
Kind of, except that I think it was way too invasive. The current code checks for
contaminant once when entering the state machine. If you pull that code into the
state machine, I don't see why it would have to be more than a single call.
One possibility might be to move the call into tcpm_set_state() to catch all
state transitions, or to use the existing enter_state variable for that
purpose.
>>
>>> enum typec_cc_status {
>>> TYPEC_CC_OPEN,
>>> TYPEC_CC_RA,
>>> @@ -114,6 +234,16 @@ enum tcpm_transmit_type {
>>> * Optional; The USB Communications Capable bit indicates if port
>>> * partner is capable of communication over the USB data lines
>>> * (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
>>> + * @check_contaminant:
>>> + * Optional; The callback is invoked when chiplevel drivers indicated
>>> + * that the USB port needs to be checked for contaminant presence.
>>> + * Chip level drivers are expected to check for contaminant and call
>>> + * tcpm_clean_port when the port is clean to put the port back into
>>> + * toggling state.
>>> + * @is_potential_contaminant:
>>> + * Optional; TCPM invokes the callback for every TCPM state machine
>>> + * transition. Chiplevel drivers can monitor the state machine
>>> + * transitions to flag for potential contaminant presence.
>>
>> I kind of dislike the repeated checks for those callbacks in the state machine.
>> Does it make any sense to have one of those callbacks but not the other ?
>>
>> Because if not it might make sense to check in the registration function
>> if either both are NULL or both are set, and then drop most of the checks
>> in the state machine handler.
>
> I can definitely check for the registration of the callbacks but cant
> drop one of them unless we are OK to make tcpm transitions and invoke
> the check_contaminant callback directly.
>
I didn't necessarily mean to drop one of the callbacks (unless one of them is
pulled into the tcpm driver). I envisioned a check such as
if ((!tcpc->check_contaminant && tcpc->is_potential_contaminant) ||
((tcpc->check_contaminant && !tcpc->is_potential_contaminant))
return ERR_PTR(-EINVAL);
in tcpm_register_port().
With that, it would no longer be necessary to keep checking for
port->tcpc->check_contaminant later on because potential_contaminant would only
ever be true if the check_contaminant callback exists.
Another note: We currently don't log in SRC_UNATTACHED, SNK_UNATTACHED,
and TOGGLING states. Would it make sense to add CHECK_CONTAMINANT to
the checks in tcpm_log() ?
Thanks,
Guenter
> Thanks,
> Badhri
>
>
>>
>> Thanks,
>> Guenter
>>
>>> */
>>> struct tcpc_dev {
>>> struct fwnode_handle *fwnode;
>>> @@ -148,6 +278,8 @@ struct tcpc_dev {
>>> bool pps_active, u32 requested_vbus_voltage);
>>> bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
>>> void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
>>> + void (*check_contaminant)(struct tcpc_dev *dev);
>>> + bool (*is_potential_contaminant)(struct tcpc_dev *dev, enum tcpm_state current_state);
>>> };
>>>
>>> struct tcpm_port;
>>> @@ -165,5 +297,6 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
>>> enum tcpm_transmit_status status);
>>> void tcpm_pd_hard_reset(struct tcpm_port *port);
>>> void tcpm_tcpc_reset(struct tcpm_port *port);
>>> +void tcpm_port_clean(struct tcpm_port *port);
>>>
>>> #endif /* __LINUX_USB_TCPM_H */
>>>
>>> base-commit: 1524ceb14dd5ebd6f724d993c5ec1a9a8d445d8e
>>
^ permalink raw reply
* Re: [PATCH v1 2/3] Treewide: Stop corrupting socket's task_frag
From: Paolo Abeni @ 2022-12-09 12:37 UTC (permalink / raw)
To: Benjamin Coddington, netdev
Cc: linux-kernel, Philipp Reisner, Lars Ellenberg,
Christoph Böhmwalder, Jens Axboe, Josef Bacik, Keith Busch,
Christoph Hellwig, Sagi Grimberg, Lee Duncan, Chris Leech,
Mike Christie, James E.J. Bottomley, Martin K. Petersen,
Valentina Manea, Shuah Khan, Greg Kroah-Hartman, David Howells,
Marc Dionne, Steve French, Christine Caulfield, David Teigland,
Mark Fasheh, Joel Becker, Joseph Qi, Eric Van Hensbergen,
Latchesar Ionkov, Dominique Martinet, David S. Miller,
Eric Dumazet, Jakub Kicinski, Ilya Dryomov, Xiubo Li,
Trond Myklebust, Anna Schumaker, Chuck Lever, Jeff Layton,
drbd-dev, linux-block, nbd, linux-nvme, open-iscsi, linux-scsi,
linux-usb, linux-afs, linux-cifs, samba-technical, cluster-devel,
ocfs2-devel, v9fs-developer, ceph-devel, linux-nfs
In-Reply-To: <c2ec184226acd21a191ccc1aa46a1d7e43ca7104.1669036433.git.bcodding@redhat.com>
On Mon, 2022-11-21 at 08:35 -0500, Benjamin Coddington wrote:
> Since moving to memalloc_nofs_save/restore, SUNRPC has stopped setting the
> GFP_NOIO flag on sk_allocation which the networking system uses to decide
> when it is safe to use current->task_frag. The results of this are
> unexpected corruption in task_frag when SUNRPC is involved in memory
> reclaim.
>
> The corruption can be seen in crashes, but the root cause is often
> difficult to ascertain as a crashing machine's stack trace will have no
> evidence of being near NFS or SUNRPC code. I believe this problem to
> be much more pervasive than reports to the community may indicate.
>
> Fix this by having kernel users of sockets that may corrupt task_frag due
> to reclaim set sk_use_task_frag = false. Preemptively correcting this
> situation for users that still set sk_allocation allows them to convert to
> memalloc_nofs_save/restore without the same unexpected corruptions that are
> sure to follow, unlikely to show up in testing, and difficult to bisect.
>
> CC: Philipp Reisner <philipp.reisner@linbit.com>
> CC: Lars Ellenberg <lars.ellenberg@linbit.com>
> CC: "Christoph Böhmwalder" <christoph.boehmwalder@linbit.com>
> CC: Jens Axboe <axboe@kernel.dk>
> CC: Josef Bacik <josef@toxicpanda.com>
> CC: Keith Busch <kbusch@kernel.org>
> CC: Christoph Hellwig <hch@lst.de>
> CC: Sagi Grimberg <sagi@grimberg.me>
> CC: Lee Duncan <lduncan@suse.com>
> CC: Chris Leech <cleech@redhat.com>
> CC: Mike Christie <michael.christie@oracle.com>
> CC: "James E.J. Bottomley" <jejb@linux.ibm.com>
> CC: "Martin K. Petersen" <martin.petersen@oracle.com>
> CC: Valentina Manea <valentina.manea.m@gmail.com>
> CC: Shuah Khan <shuah@kernel.org>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: David Howells <dhowells@redhat.com>
> CC: Marc Dionne <marc.dionne@auristor.com>
> CC: Steve French <sfrench@samba.org>
> CC: Christine Caulfield <ccaulfie@redhat.com>
> CC: David Teigland <teigland@redhat.com>
> CC: Mark Fasheh <mark@fasheh.com>
> CC: Joel Becker <jlbec@evilplan.org>
> CC: Joseph Qi <joseph.qi@linux.alibaba.com>
> CC: Eric Van Hensbergen <ericvh@gmail.com>
> CC: Latchesar Ionkov <lucho@ionkov.net>
> CC: Dominique Martinet <asmadeus@codewreck.org>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Jakub Kicinski <kuba@kernel.org>
> CC: Paolo Abeni <pabeni@redhat.com>
> CC: Ilya Dryomov <idryomov@gmail.com>
> CC: Xiubo Li <xiubli@redhat.com>
> CC: Chuck Lever <chuck.lever@oracle.com>
> CC: Jeff Layton <jlayton@kernel.org>
> CC: Trond Myklebust <trond.myklebust@hammerspace.com>
> CC: Anna Schumaker <anna@kernel.org>
> CC: drbd-dev@lists.linbit.com
> CC: linux-block@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: nbd@other.debian.org
> CC: linux-nvme@lists.infradead.org
> CC: open-iscsi@googlegroups.com
> CC: linux-scsi@vger.kernel.org
> CC: linux-usb@vger.kernel.org
> CC: linux-afs@lists.infradead.org
> CC: linux-cifs@vger.kernel.org
> CC: samba-technical@lists.samba.org
> CC: cluster-devel@redhat.com
> CC: ocfs2-devel@oss.oracle.com
> CC: v9fs-developer@lists.sourceforge.net
> CC: netdev@vger.kernel.org
> CC: ceph-devel@vger.kernel.org
> CC: linux-nfs@vger.kernel.org
>
> Suggested-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
I think this is the most feasible way out of the existing issue, and I
think this patchset should go via the networking tree, targeting the
Linux 6.2.
If someone has disagreement with the above, please speak!
Thanks,
Paolo
^ permalink raw reply
* Re: [PATCH 3/3] usb: dwc2: prevent core phy initialisation
From: Quentin Schulz @ 2022-12-09 12:38 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Quentin Schulz, Minas Harutyunyan, linux-kernel, linux-usb,
William Wu, Bin Yang, Frank Wang
In-Reply-To: <Y5MmhEMMx2sy91LS@kroah.com>
Hi Greg,
On 12/9/22 13:13, Greg Kroah-Hartman wrote:
> On Fri, Dec 09, 2022 at 12:15:34PM +0100, Quentin Schulz wrote:
>> Hi Greg,
>>
>> On 12/8/22 16:53, Greg Kroah-Hartman wrote:
>>> On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote:
>>>> From: Bin Yang <yangbin@rock-chips.com>
>>>>
>>>> The usb phys need to be controlled dynamically on some Rockchip SoCs.
>>>> So set the new HCD flag which prevents USB core from trying to manage
>>>> our phys.
>>>>
>>>> Signed-off-by: Bin Yang <yangbin@rock-chips.com>
>>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>> ---
>>>> drivers/usb/dwc2/hcd.c | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>> index 657f1f659ffaf..757a66fa32fa8 100644
>>>> --- a/drivers/usb/dwc2/hcd.c
>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>> @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
>>>> if (!IS_ERR_OR_NULL(hsotg->uphy))
>>>> otg_set_host(hsotg->uphy->otg, &hcd->self);
>>>> + /*
>>>> + * do not manage the PHY state in the HCD core, instead let the driver
>>>> + * handle this (for example if the PHY can only be turned on after a
>>>> + * specific event)
>>>> + */
>>>> + hcd->skip_phy_initialization = 1;
>>>
>>> Wait, doesn't this mess with the phy logic for all other chips that use
>>> this IP block? Have you tested this on other systems?
>>>
>>
>> I have not. I asked this in the cover-letter but I guess I should have made
>> the patch series an RFC for this reason?
>
> Ah, should I drop the first 2 in this series that I already applied?
>
Up to you. I need the three patches to have it (somewhat) working, so
only merging the first two isn't going to improve the current situation
much for me. I don't mind carrying them over for a v2 (or how many
versions are needed).
>>> I'd like some verification first before taking this change as it seems
>>> very specific-platform.
>>>
>>
>> There's already some platform-specific callbacks for the driver (see
>> dwc2_set_rk_params in drivers/usb/dwc2/params.c) but this gets called too
>> early, before hcd structure is actually allocated. So we either need to use
>> some "proxy"/shadow variable in dwc2_core_params and then update it right
>> after hcd gets allocated or have another platform-specific callback only for
>> hcd (post-)initialization.
>>
>> Nothing too fancy so shouldn't take too long to implement. Any preference?
>> Something else?
>
> Which ever you think would be simplest.
>
Got it.
Cheers,
Quentin
^ permalink raw reply
* Re: [PATCH 3/3] usb: dwc2: prevent core phy initialisation
From: Greg Kroah-Hartman @ 2022-12-09 12:13 UTC (permalink / raw)
To: Quentin Schulz
Cc: Quentin Schulz, Minas Harutyunyan, linux-kernel, linux-usb,
William Wu, Bin Yang, Frank Wang
In-Reply-To: <69c0f9c0-5c89-e99e-c807-9963ca377093@theobroma-systems.com>
On Fri, Dec 09, 2022 at 12:15:34PM +0100, Quentin Schulz wrote:
> Hi Greg,
>
> On 12/8/22 16:53, Greg Kroah-Hartman wrote:
> > On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote:
> > > From: Bin Yang <yangbin@rock-chips.com>
> > >
> > > The usb phys need to be controlled dynamically on some Rockchip SoCs.
> > > So set the new HCD flag which prevents USB core from trying to manage
> > > our phys.
> > >
> > > Signed-off-by: Bin Yang <yangbin@rock-chips.com>
> > > Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > ---
> > > drivers/usb/dwc2/hcd.c | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> > > index 657f1f659ffaf..757a66fa32fa8 100644
> > > --- a/drivers/usb/dwc2/hcd.c
> > > +++ b/drivers/usb/dwc2/hcd.c
> > > @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
> > > if (!IS_ERR_OR_NULL(hsotg->uphy))
> > > otg_set_host(hsotg->uphy->otg, &hcd->self);
> > > + /*
> > > + * do not manage the PHY state in the HCD core, instead let the driver
> > > + * handle this (for example if the PHY can only be turned on after a
> > > + * specific event)
> > > + */
> > > + hcd->skip_phy_initialization = 1;
> >
> > Wait, doesn't this mess with the phy logic for all other chips that use
> > this IP block? Have you tested this on other systems?
> >
>
> I have not. I asked this in the cover-letter but I guess I should have made
> the patch series an RFC for this reason?
Ah, should I drop the first 2 in this series that I already applied?
> > I'd like some verification first before taking this change as it seems
> > very specific-platform.
> >
>
> There's already some platform-specific callbacks for the driver (see
> dwc2_set_rk_params in drivers/usb/dwc2/params.c) but this gets called too
> early, before hcd structure is actually allocated. So we either need to use
> some "proxy"/shadow variable in dwc2_core_params and then update it right
> after hcd gets allocated or have another platform-specific callback only for
> hcd (post-)initialization.
>
> Nothing too fancy so shouldn't take too long to implement. Any preference?
> Something else?
Which ever you think would be simplest.
thanks,
greg k-h
^ permalink raw reply
* Re: (subset) [PATCH 000/606] i2c: Complete conversion to i2c_probe_new
From: Robert Foss @ 2022-12-09 12:00 UTC (permalink / raw)
To: Uwe Kleine-König, Wolfram Sang, Angel Iglesias, Grant Likely,
Lee Jones
Cc: Robert Foss, Broadcom internal kernel review list, linux-gpio,
linux-rtc, linux-arm-kernel, linux-input, linux-integrity,
linux-media, openipmi-developer, linux-serial, linux-usb,
linux-spi, kernel, Purism Kernel Team, linux-rpi-kernel,
linux-leds, linux-actions, netdev, linux-iio, linux-pwm,
linux-staging, chrome-platform, linux-crypto, linux-watchdog,
linux-amlogic, linux-fbdev, linux-renesas-soc, linux-i2c,
linux-kernel, alsa-devel, linux-stm32, linuxppc-dev, patches,
linux-omap, linux-mtd, linux-samsung-soc, linux-pm,
platform-driver-x86, devicetree
In-Reply-To: <20221118224540.619276-1-uwe@kleine-koenig.org>
On Fri, 18 Nov 2022 23:35:34 +0100, Uwe Kleine-König wrote:
> since commit b8a1a4cd5a98 ("i2c: Provide a temporary .probe_new()
> call-back type") from 2016 there is a "temporary" alternative probe
> callback for i2c drivers.
>
> This series completes all drivers to this new callback (unless I missed
> something). It's based on current next/master.
> A part of the patches depend on commit 662233731d66 ("i2c: core:
> Introduce i2c_client_get_device_id helper function"), there is a branch that
> you can pull into your tree to get it:
>
> [...]
Applied all patches that build.
Patches excluded:
- ps8622
- ti-sn65dsi83
- adv7511
Repo: https://cgit.freedesktop.org/drm/drm-misc/
[014/606] drm/bridge: adv7511: Convert to i2c's .probe_new()
(no commit info)
[015/606] drm/bridge/analogix/anx6345: Convert to i2c's .probe_new()
(no commit info)
[016/606] drm/bridge/analogix/anx78xx: Convert to i2c's .probe_new()
(no commit info)
[017/606] drm/bridge: anx7625: Convert to i2c's .probe_new()
(no commit info)
[018/606] drm/bridge: icn6211: Convert to i2c's .probe_new()
(no commit info)
[019/606] drm/bridge: chrontel-ch7033: Convert to i2c's .probe_new()
commit: 8dc6de280f01c0f7b8d40435736f3c975368ad70
[020/606] drm/bridge: it6505: Convert to i2c's .probe_new()
(no commit info)
[021/606] drm/bridge: it66121: Convert to i2c's .probe_new()
(no commit info)
[022/606] drm/bridge: lt8912b: Convert to i2c's .probe_new()
(no commit info)
[023/606] drm/bridge: lt9211: Convert to i2c's .probe_new()
(no commit info)
[024/606] drm/bridge: lt9611: Convert to i2c's .probe_new()
(no commit info)
[025/606] drm/bridge: lt9611uxc: Convert to i2c's .probe_new()
(no commit info)
[026/606] drm/bridge: megachips: Convert to i2c's .probe_new()
(no commit info)
[027/606] drm/bridge: nxp-ptn3460: Convert to i2c's .probe_new()
(no commit info)
[028/606] drm/bridge: parade-ps8622: Convert to i2c's .probe_new()
(no commit info)
[029/606] drm/bridge: sii902x: Convert to i2c's .probe_new()
(no commit info)
[030/606] drm/bridge: sii9234: Convert to i2c's .probe_new()
(no commit info)
[031/606] drm/bridge: sii8620: Convert to i2c's .probe_new()
(no commit info)
[032/606] drm/bridge: tc358767: Convert to i2c's .probe_new()
(no commit info)
[033/606] drm/bridge: tc358768: Convert to i2c's .probe_new()
(no commit info)
[034/606] drm/bridge/tc358775: Convert to i2c's .probe_new()
(no commit info)
[035/606] drm/bridge: ti-sn65dsi83: Convert to i2c's .probe_new()
(no commit info)
[037/606] drm/bridge: tfp410: Convert to i2c's .probe_new()
(no commit info)
rob
^ permalink raw reply
* Re: usb: gadget: dwc2: not getting audio data
From: Minas Harutyunyan @ 2022-12-09 11:32 UTC (permalink / raw)
To: Palak SHAH, Maynard CABIENTE; +Cc: linux-usb@vger.kernel.org
In-Reply-To: <HE1PR0601MB258685C64D46C08C978C37EB8D1D9@HE1PR0601MB2586.eurprd06.prod.outlook.com>
Hi Palak,
On 12/8/2022 8:30 PM, Palak SHAH wrote:
> Hi Minas,
>
> We are using an Altera Cyclone V SoC FPGA on our board with linux kernel
> 5.4.80 and enabling the USB gadget for HID (keyboard and mouse) and
> audio (UAC1). The USB will always be configured for USB gadget (and
> never host).
>
> When the USB gadget is configured for audio (speaker), we are seeing
> that the USB gadget DWC2 driver is getting stuck and not getting the
> transfers completed. The USB speaker is configured for 44.1KHz, 2 channel.
>
Could you please apply below patch, test and send debug log.
index 8b15742d9e8a..3c03431023a8 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2218,6 +2218,8 @@ static void
dwc2_gadget_complete_isoc_request_ddma(struct dwc2_hsotg_ep *hs_ep)
dwc2_hsotg_complete_request(hsotg, hs_ep, hs_req, 0);
+ dev_dbg(hsotg->dev, "%s: compl_desc # %d\n", __func__,
hs_ep->compl_desc);
+
hs_ep->compl_desc++;
if (hs_ep->compl_desc > (MAX_DMA_DESC_NUM_HS_ISOC - 1))
hs_ep->compl_desc = 0;
@@ -2892,6 +2894,9 @@ static void
dwc2_gadget_handle_out_token_ep_disabled(struct dwc2_hsotg_ep *ep)
return;
if (using_desc_dma(hsotg)) {
+
+ dev_dbg(hsotg->dev, "%s: target_frame = 0x%08x\n",
__func__, ep->target_frame);
+
if (ep->target_frame == TARGET_FRAME_INITIAL) {
/* Start first ISO Out */
ep->target_frame = hsotg->frame_number;
BTW, correct subject should be follow:
usb: dwc2: gadget: not getting....
Thanks,
Minas
^ permalink raw reply related
* Re: [PATCH 3/3] usb: dwc2: prevent core phy initialisation
From: Quentin Schulz @ 2022-12-09 11:15 UTC (permalink / raw)
To: Greg Kroah-Hartman, Quentin Schulz
Cc: Minas Harutyunyan, linux-kernel, linux-usb, William Wu, Bin Yang,
Frank Wang
In-Reply-To: <Y5IIaeip81DIvEZ6@kroah.com>
Hi Greg,
On 12/8/22 16:53, Greg Kroah-Hartman wrote:
> On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote:
>> From: Bin Yang <yangbin@rock-chips.com>
>>
>> The usb phys need to be controlled dynamically on some Rockchip SoCs.
>> So set the new HCD flag which prevents USB core from trying to manage
>> our phys.
>>
>> Signed-off-by: Bin Yang <yangbin@rock-chips.com>
>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>> drivers/usb/dwc2/hcd.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 657f1f659ffaf..757a66fa32fa8 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
>> if (!IS_ERR_OR_NULL(hsotg->uphy))
>> otg_set_host(hsotg->uphy->otg, &hcd->self);
>>
>> + /*
>> + * do not manage the PHY state in the HCD core, instead let the driver
>> + * handle this (for example if the PHY can only be turned on after a
>> + * specific event)
>> + */
>> + hcd->skip_phy_initialization = 1;
>
> Wait, doesn't this mess with the phy logic for all other chips that use
> this IP block? Have you tested this on other systems?
>
I have not. I asked this in the cover-letter but I guess I should have
made the patch series an RFC for this reason?
> I'd like some verification first before taking this change as it seems
> very specific-platform.
>
There's already some platform-specific callbacks for the driver (see
dwc2_set_rk_params in drivers/usb/dwc2/params.c) but this gets called
too early, before hcd structure is actually allocated. So we either need
to use some "proxy"/shadow variable in dwc2_core_params and then update
it right after hcd gets allocated or have another platform-specific
callback only for hcd (post-)initialization.
Nothing too fancy so shouldn't take too long to implement. Any
preference? Something else?
Also on a side note, after further testing, USB peripheral mode in
dual-role mode does not seem to be entirely fixed with this patch
series, I still have occasional locks. But considering that it
absolutely didn't work before, it is some kind of progress.
There are also some issues related to USB host mode in dual-role mode
but I saw those happening before the patch series too. I'll see what I
can do, really frustrating to work with IPs for which there's no
documentation :/
Cheers,
Quentin
^ permalink raw reply
* Re: [PATCH v3 3/9] dt-bindings: PCI: renesas,pci-rcar-gen2: 'depends-on' is no more optional
From: Krzysztof Kozlowski @ 2022-12-09 8:06 UTC (permalink / raw)
To: Herve Codina
Cc: Geert Uytterhoeven, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Greg Kroah-Hartman, Magnus Damm,
Gareth Williams, linux-renesas-soc, linux-clk, devicetree,
linux-kernel, linux-usb, Thomas Petazzoni, Miquel Raynal
In-Reply-To: <20221208165101.584e4b92@bootlin.com>
On 08/12/2022 16:51, Herve Codina wrote:
> Hi Krzysztof,
>
> On Thu, 8 Dec 2022 10:46:32 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>
>> On 08/12/2022 10:05, Herve Codina wrote:
>>> Hi Krzysztof,
>>>
>>> On Thu, 8 Dec 2022 09:26:41 +0100
>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>>> On 07/12/2022 17:24, Herve Codina wrote:
>>>>> The 'depends-on' property is set in involved DTS.
>>>>>
>>>>> Move it to a required property.
>>>>>
>>>>> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
>>>>> ---
>>>>> Documentation/devicetree/bindings/pci/renesas,pci-rcar-gen2.yaml | 1 +
>>>>
>>>> This should be squashed with previous patch. There is no point to add
>>>> property and immediately in the next patch make it required. Remember
>>>> that bindings are separate from DTS.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> I though about make dtbs_check in case of git bisect.
>>
>> And what would this commit change? In Git you will have
>> 1. dt-bindings: PCI: renesas,pci-rcar-gen2: Add depends-on for RZ/N1 SoC
>> family
>> 2. dt-bindings: PCI: renesas,pci-rcar-gen2: 'depends-on' is no more optional
>>
>> so what is the difference for git bisect?
>
> Well, today, I have:
> 1. dt-bindings: Add depends-on
> 2. dts: Add depends-on
> 3. dt-bindings: Move depends-on to mandatory
What does it mean "I have"? Patches on mailing list? But we talk about
Git and I wrote you bindings are DTS are not going the same tree.
>
> If I squash dt-bindings commits, I am going to have:
> 1. dt-bindings: Add mandatory depends-on
> 2. dts: Add depends-on
> or
> 1. dts: Add depends-on
> 2. dt-bindings: Add mandatory depends-on
And how does it matter? Anyway it goes separate trees.
>
> I have not tested but if I used only the first commit in each
> case (git bisect):
It's not bisectable anyway, you cannot make it bisectable within one
release.
> In the first case, dtbs_check is probably going to signal the
> missing 'depends-on' property on dts.
> In the second case, dtbs_check is probably going to signal the
> not described 'depends-on' property present in dts.
And why is that even a problem?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH] xhci: print warning when HCE was set
From: liulongfang @ 2022-12-09 6:13 UTC (permalink / raw)
To: Mathias Nyman, Mathias Nyman, gregkh
Cc: linux-usb, linux-kernel, yisen.zhuang
In-Reply-To: <7163ea05-7ea5-998b-932a-25ffd36ed296@intel.com>
On 2022/10/14 15:56, Mathias Nyman Wrote:
> On 14.10.2022 6.12, liulongfang wrote:
>> On 2022/9/26 15:58, Mathias Nyman wrote:
>>> On 24.9.2022 5.35, liulongfang wrote:
>>>> On 2022/9/22 21:01, Mathias Nyman Wrote:
>>>>> Hi
>>>>>
>>>>> On 15.9.2022 4.11, Longfang Liu wrote:
>>>>>> When HCE(Host Controller Error) is set, it means that the xhci hardware
>>>>>> controller has an error at this time, but the current xhci driver
>>>>>> software does not log this event.
>>>>>>
>>>>>> By adding an HCE event detection in the xhci interrupt processing
>>>>>> interface, a warning log is output to the system, which is convenient
>>>>>> for system device status tracking.
>>>>>>
>>>>>
>>>>> xHC should cease all activity when it sets HCE, and is probably not
>>>>> generating interrupts anymore.
>>>>>
>>>>> Would probably be more useful to check for HCE at timeouts than in the
>>>>> interrupt handler.
>>>>>
>>>>
>>>> Which function of the driver code is this timeout in?
>>>
>>> xhci_handle_command_timeout() will usually trigger at some point,
>>>
>>
>> Because this HCE error is reported in the form of an interrupt signal, it is more
>> concise to put it in xhci_irq() than in xhci_handle_command_timeout().
>>
>
> Patch was added to queue after you reported your xHC hardware triggers interrupts when HCE is set.
> I'll send it forward after 6.1-rc1
>
In our test version, a test log is added to xhci_irq(). In the test case that triggers HCE,
the HCE interrupt is reported and recorded through the log:
{53}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 0
{53}[Hardware Error]: event severity: recoverable
{53}[Hardware Error]: Error 0, type: recoverable
{53}[Hardware Error]: section type: unknown, c8b328a8-9917-4af6-9a13-2e08ab2e7586
{53}[Hardware Error]: section length: 0x48
{53}[Hardware Error]: 00000000: 0000186b 00000201 001a0001 00000000 k...............
{53}[Hardware Error]: 00000010: 00000000 00000000 00000000 00000028 ............(...
{53}[Hardware Error]: 00000020: 00000000 00000000 00000000 00000000 ................
{53}[Hardware Error]: 00000030: 00000000 00000000 00000000 00000000 ................
{53}[Hardware Error]: 00000040: 00000001 00000000 ........
xhci_hcd 0000:30:01.0: xHCI host not responding to stop endpoint command.
xhci_hcd 0000:30:01.0: USBSTS: PCD HCE
xhci_hcd 0000:30:01.0: xHCI host controller not responding, assume dead
xhci_hcd 0000:30:01.0: HC died; cleaning up
usb usb1-port1: couldn't allocate usb_device
rmmod xhci-pci
xhci_hcd 0000:30:01.0: remove, state 4
usb usb2: USB disconnect, device number 1
xhci_hcd 0000:30:01.0: USB bus 2 deregistered
xhci_hcd 0000:30:01.0: remove, state 1
usb usb1: USB disconnect, device number 1
xhci_hcd 0000:30:01.0: USB bus 1 deregistered
Thanks,
Longfang.
> xHCI specification still indicate HCE might not trigger interrupts:
>
> Section 4.24.1 -Internal Errors
> ...
> "Software should implement an algorithm for checking the HCE flag if the xHC is
> not responding."
>
> Thanks
> -Mathias
> .
>
^ permalink raw reply
* [usb:usb-testing] BUILD SUCCESS 81c25247a2a03a0f97e4805d7aff7541ccff6baa
From: kernel test robot @ 2022-12-09 4:04 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-usb
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
branch HEAD: 81c25247a2a03a0f97e4805d7aff7541ccff6baa usb: gadget: uvc: Rename bmInterfaceFlags -> bmInterlaceFlags
elapsed time: 729m
configs tested: 60
configs skipped: 2
The following configs have been built successfully.
More configs may be tested in the coming days.
gcc tested configs:
um i386_defconfig
um x86_64_defconfig
powerpc allnoconfig
arc randconfig-r043-20221207
m68k allyesconfig
m68k allmodconfig
arc allyesconfig
alpha allyesconfig
riscv randconfig-r042-20221207
s390 randconfig-r044-20221207
arc defconfig
s390 allmodconfig
alpha defconfig
s390 defconfig
s390 allyesconfig
sh allmodconfig
x86_64 defconfig
x86_64 rhel-8.3
i386 defconfig
x86_64 allyesconfig
ia64 allmodconfig
mips allyesconfig
powerpc allmodconfig
x86_64 randconfig-a004
x86_64 randconfig-a002
x86_64 rhel-8.3-syz
x86_64 rhel-8.3-kunit
x86_64 rhel-8.3-kvm
x86_64 randconfig-a006
x86_64 rhel-8.3-rust
i386 randconfig-a001
i386 randconfig-a014
i386 randconfig-a003
i386 randconfig-a012
x86_64 rhel-8.3-kselftests
x86_64 randconfig-a013
arm defconfig
x86_64 randconfig-a011
i386 randconfig-a016
i386 allyesconfig
i386 randconfig-a005
x86_64 randconfig-a015
x86_64 rhel-8.3-func
arm allyesconfig
arm64 allyesconfig
clang tested configs:
arm randconfig-r046-20221207
hexagon randconfig-r041-20221207
hexagon randconfig-r045-20221207
x86_64 randconfig-a001
x86_64 randconfig-a003
i386 randconfig-a013
x86_64 randconfig-a005
x86_64 randconfig-a014
i386 randconfig-a002
x86_64 randconfig-a012
i386 randconfig-a011
i386 randconfig-a006
i386 randconfig-a015
i386 randconfig-a004
x86_64 randconfig-a016
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply
* [usb:usb-next] BUILD SUCCESS 82710ecd0e5d401c36ad21f00d644672005233b9
From: kernel test robot @ 2022-12-09 4:04 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-usb
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-next
branch HEAD: 82710ecd0e5d401c36ad21f00d644672005233b9 Merge tag 'usb-serial-6.2-rc1' of https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial into usb-next
elapsed time: 730m
configs tested: 60
configs skipped: 2
The following configs have been built successfully.
More configs may be tested in the coming days.
gcc tested configs:
um i386_defconfig
um x86_64_defconfig
powerpc allnoconfig
sh allmodconfig
m68k allyesconfig
mips allyesconfig
m68k allmodconfig
powerpc allmodconfig
arc allyesconfig
alpha allyesconfig
arc randconfig-r043-20221207
riscv randconfig-r042-20221207
s390 randconfig-r044-20221207
i386 defconfig
x86_64 defconfig
x86_64 rhel-8.3
x86_64 randconfig-a004
x86_64 randconfig-a002
x86_64 allyesconfig
x86_64 rhel-8.3-syz
x86_64 rhel-8.3-kunit
i386 randconfig-a014
x86_64 rhel-8.3-kvm
i386 randconfig-a012
i386 randconfig-a016
x86_64 randconfig-a006
arc defconfig
s390 allmodconfig
alpha defconfig
arm defconfig
i386 allyesconfig
s390 defconfig
x86_64 rhel-8.3-rust
x86_64 rhel-8.3-kselftests
x86_64 rhel-8.3-func
x86_64 randconfig-a015
x86_64 randconfig-a013
x86_64 randconfig-a011
i386 randconfig-a001
i386 randconfig-a003
ia64 allmodconfig
arm allyesconfig
s390 allyesconfig
i386 randconfig-a005
arm64 allyesconfig
clang tested configs:
arm randconfig-r046-20221207
hexagon randconfig-r041-20221207
hexagon randconfig-r045-20221207
i386 randconfig-a013
x86_64 randconfig-a001
x86_64 randconfig-a003
i386 randconfig-a011
i386 randconfig-a015
x86_64 randconfig-a005
x86_64 randconfig-a014
x86_64 randconfig-a016
x86_64 randconfig-a012
i386 randconfig-a002
i386 randconfig-a004
i386 randconfig-a006
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply
* Re: [PATCH v6 1/3] usb: typec: tcpm: Add callbacks to mitigate wakeups due to contaminant
From: Badhri Jagan Sridharan @ 2022-12-09 2:39 UTC (permalink / raw)
To: Guenter Roeck
Cc: Heikki Krogerus, Greg Kroah-Hartman, linux-usb, linux-kernel,
Kyle Tso
In-Reply-To: <819cb665-ec2f-5542-8ecf-e8ff7ca29908@roeck-us.net>
On Thu, Dec 8, 2022 at 3:56 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 12/8/22 07:04, Badhri Jagan Sridharan wrote:
> > On some of the TCPC implementations, when the Type-C port is exposed
> > to contaminants, such as water, TCPC stops toggling while reporting OPEN
> > either by the time TCPM reads CC pin status or during CC debounce
> > window. This causes TCPM to be stuck in TOGGLING state. If TCPM is made
> > to restart toggling, the behavior recurs causing redundant CPU wakeups
> > till the USB-C port is free of contaminant.
> >
> > [206199.287817] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
> > [206199.640337] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
> > [206199.985789] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
> > ...
> >
> > TCPM invokes is_potential_contaminant callback to allow low level chip
> > drivers to monitor TCPM state machine transitions and notify TCPM when
> > the Type-C port needs to be checked for potential contaminant presence.
> > TCPCs which do have the needed hardware can implement the check_contaminant
> > callback which is invoked by TCPM to evaluate for presence of contaminant.
> > Lower level TCPC driver can restart toggling through TCPM_PORT_CLEAN event
> > when the driver detects that USB-C port is free of contaminant.
> >
> > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> > ---
> > Changes since v5:
> > * Updated commit message. Removed change id.
> > Changes since v4:
> > * None
> > Changes since v3:
> > * None
> > Changes since V2:
> > * Offloaded tcpm from maintaining disconnect_while_debouncing logic
> > * to lower level maxim tcpc driver based on feedback.
> > ---
> > drivers/usb/typec/tcpm/tcpm.c | 162 +++++++++-------------------------
> > include/linux/usb/tcpm.h | 133 ++++++++++++++++++++++++++++
> > 2 files changed, 177 insertions(+), 118 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 904c7b4ce2f0..a138cea49612 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -33,119 +33,6 @@
> >
> > #include <uapi/linux/sched/types.h>
> >
> > -#define FOREACH_STATE(S) \
> > - S(INVALID_STATE), \
> > - S(TOGGLING), \
> > - S(SRC_UNATTACHED), \
> > - S(SRC_ATTACH_WAIT), \
> > - S(SRC_ATTACHED), \
> > - S(SRC_STARTUP), \
> > - S(SRC_SEND_CAPABILITIES), \
> > - S(SRC_SEND_CAPABILITIES_TIMEOUT), \
> > - S(SRC_NEGOTIATE_CAPABILITIES), \
> > - S(SRC_TRANSITION_SUPPLY), \
> > - S(SRC_READY), \
> > - S(SRC_WAIT_NEW_CAPABILITIES), \
> > - \
> > - S(SNK_UNATTACHED), \
> > - S(SNK_ATTACH_WAIT), \
> > - S(SNK_DEBOUNCED), \
> > - S(SNK_ATTACHED), \
> > - S(SNK_STARTUP), \
> > - S(SNK_DISCOVERY), \
> > - S(SNK_DISCOVERY_DEBOUNCE), \
> > - S(SNK_DISCOVERY_DEBOUNCE_DONE), \
> > - S(SNK_WAIT_CAPABILITIES), \
> > - S(SNK_NEGOTIATE_CAPABILITIES), \
> > - S(SNK_NEGOTIATE_PPS_CAPABILITIES), \
> > - S(SNK_TRANSITION_SINK), \
> > - S(SNK_TRANSITION_SINK_VBUS), \
> > - S(SNK_READY), \
> > - \
> > - S(ACC_UNATTACHED), \
> > - S(DEBUG_ACC_ATTACHED), \
> > - S(AUDIO_ACC_ATTACHED), \
> > - S(AUDIO_ACC_DEBOUNCE), \
> > - \
> > - S(HARD_RESET_SEND), \
> > - S(HARD_RESET_START), \
> > - S(SRC_HARD_RESET_VBUS_OFF), \
> > - S(SRC_HARD_RESET_VBUS_ON), \
> > - S(SNK_HARD_RESET_SINK_OFF), \
> > - S(SNK_HARD_RESET_WAIT_VBUS), \
> > - S(SNK_HARD_RESET_SINK_ON), \
> > - \
> > - S(SOFT_RESET), \
> > - S(SRC_SOFT_RESET_WAIT_SNK_TX), \
> > - S(SNK_SOFT_RESET), \
> > - S(SOFT_RESET_SEND), \
> > - \
> > - S(DR_SWAP_ACCEPT), \
> > - S(DR_SWAP_SEND), \
> > - S(DR_SWAP_SEND_TIMEOUT), \
> > - S(DR_SWAP_CANCEL), \
> > - S(DR_SWAP_CHANGE_DR), \
> > - \
> > - S(PR_SWAP_ACCEPT), \
> > - S(PR_SWAP_SEND), \
> > - S(PR_SWAP_SEND_TIMEOUT), \
> > - S(PR_SWAP_CANCEL), \
> > - S(PR_SWAP_START), \
> > - S(PR_SWAP_SRC_SNK_TRANSITION_OFF), \
> > - S(PR_SWAP_SRC_SNK_SOURCE_OFF), \
> > - S(PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED), \
> > - S(PR_SWAP_SRC_SNK_SINK_ON), \
> > - S(PR_SWAP_SNK_SRC_SINK_OFF), \
> > - S(PR_SWAP_SNK_SRC_SOURCE_ON), \
> > - S(PR_SWAP_SNK_SRC_SOURCE_ON_VBUS_RAMPED_UP), \
> > - \
> > - S(VCONN_SWAP_ACCEPT), \
> > - S(VCONN_SWAP_SEND), \
> > - S(VCONN_SWAP_SEND_TIMEOUT), \
> > - S(VCONN_SWAP_CANCEL), \
> > - S(VCONN_SWAP_START), \
> > - S(VCONN_SWAP_WAIT_FOR_VCONN), \
> > - S(VCONN_SWAP_TURN_ON_VCONN), \
> > - S(VCONN_SWAP_TURN_OFF_VCONN), \
> > - \
> > - S(FR_SWAP_SEND), \
> > - S(FR_SWAP_SEND_TIMEOUT), \
> > - S(FR_SWAP_SNK_SRC_TRANSITION_TO_OFF), \
> > - S(FR_SWAP_SNK_SRC_NEW_SINK_READY), \
> > - S(FR_SWAP_SNK_SRC_SOURCE_VBUS_APPLIED), \
> > - S(FR_SWAP_CANCEL), \
> > - \
> > - S(SNK_TRY), \
> > - S(SNK_TRY_WAIT), \
> > - S(SNK_TRY_WAIT_DEBOUNCE), \
> > - S(SNK_TRY_WAIT_DEBOUNCE_CHECK_VBUS), \
> > - S(SRC_TRYWAIT), \
> > - S(SRC_TRYWAIT_DEBOUNCE), \
> > - S(SRC_TRYWAIT_UNATTACHED), \
> > - \
> > - S(SRC_TRY), \
> > - S(SRC_TRY_WAIT), \
> > - S(SRC_TRY_DEBOUNCE), \
> > - S(SNK_TRYWAIT), \
> > - S(SNK_TRYWAIT_DEBOUNCE), \
> > - S(SNK_TRYWAIT_VBUS), \
> > - S(BIST_RX), \
> > - \
> > - S(GET_STATUS_SEND), \
> > - S(GET_STATUS_SEND_TIMEOUT), \
> > - S(GET_PPS_STATUS_SEND), \
> > - S(GET_PPS_STATUS_SEND_TIMEOUT), \
> > - \
> > - S(GET_SINK_CAP), \
> > - S(GET_SINK_CAP_TIMEOUT), \
> > - \
> > - S(ERROR_RECOVERY), \
> > - S(PORT_RESET), \
> > - S(PORT_RESET_WAIT_OFF), \
> > - \
> > - S(AMS_START), \
> > - S(CHUNK_NOT_SUPP)
> > -
> > #define FOREACH_AMS(S) \
> > S(NONE_AMS), \
> > S(POWER_NEGOTIATION), \
> > @@ -182,13 +69,8 @@
> > S(COUNTRY_INFO), \
> > S(COUNTRY_CODES)
> >
> > -#define GENERATE_ENUM(e) e
> > #define GENERATE_STRING(s) #s
> >
> > -enum tcpm_state {
> > - FOREACH_STATE(GENERATE_ENUM)
> > -};
> > -
> > static const char * const tcpm_states[] = {
> > FOREACH_STATE(GENERATE_STRING)
> > };
> > @@ -249,6 +131,7 @@ enum frs_typec_current {
> > #define TCPM_RESET_EVENT BIT(2)
> > #define TCPM_FRS_EVENT BIT(3)
> > #define TCPM_SOURCING_VBUS BIT(4)
> > +#define TCPM_PORT_CLEAN BIT(5)
> >
> > #define LOG_BUFFER_ENTRIES 1024
> > #define LOG_BUFFER_ENTRY_SIZE 128
> > @@ -483,6 +366,14 @@ struct tcpm_port {
> > * SNK_READY for non-pd link.
> > */
> > bool slow_charger_loop;
> > +
> > + /*
> > + * When true indicates that the lower level drivers indicate potential presence
> > + * of contaminant in the connector pins based on the tcpm state machine
> > + * transitions.
> > + */
> > + bool potential_contaminant;
> > +
> > #ifdef CONFIG_DEBUG_FS
> > struct dentry *dentry;
> > struct mutex logbuffer_lock; /* log buffer access lock */
> > @@ -3904,15 +3795,26 @@ static void run_state_machine(struct tcpm_port *port)
> > unsigned int msecs;
> > enum tcpm_state upcoming_state;
> >
> > + if (port->tcpc->is_potential_contaminant)
> > + port->potential_contaminant =
> > + port->tcpc->is_potential_contaminant(port->tcpc, port->state);
> > +
> > port->enter_state = port->state;
> > switch (port->state) {
> > case TOGGLING:
> > break;
> > + case CHECK_CONTAMINANT:
> > + port->tcpc->check_contaminant(port->tcpc);
> > + break;
> > /* SRC states */
> > case SRC_UNATTACHED:
> > if (!port->non_pd_role_swap)
> > tcpm_swap_complete(port, -ENOTCONN);
> > tcpm_src_detach(port);
> > + if (port->potential_contaminant && port->tcpc->check_contaminant) {
> > + tcpm_set_state(port, CHECK_CONTAMINANT, 0);
> > + break;
> > + }
> > if (tcpm_start_toggling(port, tcpm_rp_cc(port))) {
> > tcpm_set_state(port, TOGGLING, 0);
> > break;
> > @@ -4150,6 +4052,10 @@ static void run_state_machine(struct tcpm_port *port)
> > tcpm_swap_complete(port, -ENOTCONN);
> > tcpm_pps_complete(port, -ENOTCONN);
> > tcpm_snk_detach(port);
> > + if (port->potential_contaminant && port->tcpc->check_contaminant) {
> > + tcpm_set_state(port, CHECK_CONTAMINANT, 0);
> > + break;
> > + }
> > if (tcpm_start_toggling(port, TYPEC_CC_RD)) {
> > tcpm_set_state(port, TOGGLING, 0);
> > break;
> > @@ -4926,6 +4832,9 @@ static void _tcpm_cc_change(struct tcpm_port *port, enum typec_cc_status cc1,
> > else if (tcpm_port_is_sink(port))
> > tcpm_set_state(port, SNK_ATTACH_WAIT, 0);
> > break;
> > + case CHECK_CONTAMINANT:
> > + /* Wait for Toggling to be resumed */
> > + break;
> > case SRC_UNATTACHED:
> > case ACC_UNATTACHED:
> > if (tcpm_port_is_debug(port) || tcpm_port_is_audio(port) ||
> > @@ -5425,6 +5334,10 @@ static void tcpm_pd_event_handler(struct kthread_work *work)
> > port->vbus_source = true;
> > _tcpm_pd_vbus_on(port);
> > }
> > + if (events & TCPM_PORT_CLEAN) {
> > + tcpm_log(port, "port clean");
> > + tcpm_set_state(port, TOGGLING, 0);
> > + }
>
> That sets the state to TOGGLING unconditionally, even if it not currently
> CHECK_CONTAMINANT. Is that a potential problem ?
Good catch ! Would make sense to check whether tcpm is in
CHECK_CONTAMINANT state before setting the TOGGLING state.
>
> >
> > spin_lock(&port->pd_event_lock);
> > }
> > @@ -5477,6 +5390,19 @@ void tcpm_sourcing_vbus(struct tcpm_port *port)
> > }
> > EXPORT_SYMBOL_GPL(tcpm_sourcing_vbus);
> >
> > +/*
> > + * Low level tcpc drivers invoke this once the port is deemed clean to return
> > + * the port to TOGGLING state.
> > + */
> > +void tcpm_port_clean(struct tcpm_port *port)
> > +{
> > + spin_lock(&port->pd_event_lock);
> > + port->pd_events |= TCPM_PORT_CLEAN;
> > + spin_unlock(&port->pd_event_lock);
> > + kthread_queue_work(port->wq, &port->event_work);
> > +}
> > +EXPORT_SYMBOL_GPL(tcpm_port_clean);
> > +
> > static void tcpm_enable_frs_work(struct kthread_work *work)
> > {
> > struct tcpm_port *port = container_of(work, struct tcpm_port, enable_frs);
> > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> > index bffc8d3e14ad..9cf16372a6e4 100644
> > --- a/include/linux/usb/tcpm.h
> > +++ b/include/linux/usb/tcpm.h
> > @@ -10,6 +10,126 @@
> > #include <linux/usb/typec.h>
> > #include "pd.h"
> >
> > +#define FOREACH_STATE(S) \
> > + S(INVALID_STATE), \
> > + S(TOGGLING), \
> > + S(CHECK_CONTAMINANT), \
> > + S(SRC_UNATTACHED), \
> > + S(SRC_ATTACH_WAIT), \
> > + S(SRC_ATTACHED), \
> > + S(SRC_STARTUP), \
> > + S(SRC_SEND_CAPABILITIES), \
> > + S(SRC_SEND_CAPABILITIES_TIMEOUT), \
> > + S(SRC_NEGOTIATE_CAPABILITIES), \
> > + S(SRC_TRANSITION_SUPPLY), \
> > + S(SRC_READY), \
> > + S(SRC_WAIT_NEW_CAPABILITIES), \
> > + \
> > + S(SNK_UNATTACHED), \
> > + S(SNK_ATTACH_WAIT), \
> > + S(SNK_DEBOUNCED), \
> > + S(SNK_ATTACHED), \
> > + S(SNK_STARTUP), \
> > + S(SNK_DISCOVERY), \
> > + S(SNK_DISCOVERY_DEBOUNCE), \
> > + S(SNK_DISCOVERY_DEBOUNCE_DONE), \
> > + S(SNK_WAIT_CAPABILITIES), \
> > + S(SNK_NEGOTIATE_CAPABILITIES), \
> > + S(SNK_NEGOTIATE_PPS_CAPABILITIES), \
> > + S(SNK_TRANSITION_SINK), \
> > + S(SNK_TRANSITION_SINK_VBUS), \
> > + S(SNK_READY), \
> > + \
> > + S(ACC_UNATTACHED), \
> > + S(DEBUG_ACC_ATTACHED), \
> > + S(AUDIO_ACC_ATTACHED), \
> > + S(AUDIO_ACC_DEBOUNCE), \
> > + \
> > + S(HARD_RESET_SEND), \
> > + S(HARD_RESET_START), \
> > + S(SRC_HARD_RESET_VBUS_OFF), \
> > + S(SRC_HARD_RESET_VBUS_ON), \
> > + S(SNK_HARD_RESET_SINK_OFF), \
> > + S(SNK_HARD_RESET_WAIT_VBUS), \
> > + S(SNK_HARD_RESET_SINK_ON), \
> > + \
> > + S(SOFT_RESET), \
> > + S(SRC_SOFT_RESET_WAIT_SNK_TX), \
> > + S(SNK_SOFT_RESET), \
> > + S(SOFT_RESET_SEND), \
> > + \
> > + S(DR_SWAP_ACCEPT), \
> > + S(DR_SWAP_SEND), \
> > + S(DR_SWAP_SEND_TIMEOUT), \
> > + S(DR_SWAP_CANCEL), \
> > + S(DR_SWAP_CHANGE_DR), \
> > + \
> > + S(PR_SWAP_ACCEPT), \
> > + S(PR_SWAP_SEND), \
> > + S(PR_SWAP_SEND_TIMEOUT), \
> > + S(PR_SWAP_CANCEL), \
> > + S(PR_SWAP_START), \
> > + S(PR_SWAP_SRC_SNK_TRANSITION_OFF), \
> > + S(PR_SWAP_SRC_SNK_SOURCE_OFF), \
> > + S(PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED), \
> > + S(PR_SWAP_SRC_SNK_SINK_ON), \
> > + S(PR_SWAP_SNK_SRC_SINK_OFF), \
> > + S(PR_SWAP_SNK_SRC_SOURCE_ON), \
> > + S(PR_SWAP_SNK_SRC_SOURCE_ON_VBUS_RAMPED_UP), \
> > + \
> > + S(VCONN_SWAP_ACCEPT), \
> > + S(VCONN_SWAP_SEND), \
> > + S(VCONN_SWAP_SEND_TIMEOUT), \
> > + S(VCONN_SWAP_CANCEL), \
> > + S(VCONN_SWAP_START), \
> > + S(VCONN_SWAP_WAIT_FOR_VCONN), \
> > + S(VCONN_SWAP_TURN_ON_VCONN), \
> > + S(VCONN_SWAP_TURN_OFF_VCONN), \
> > + \
> > + S(FR_SWAP_SEND), \
> > + S(FR_SWAP_SEND_TIMEOUT), \
> > + S(FR_SWAP_SNK_SRC_TRANSITION_TO_OFF), \
> > + S(FR_SWAP_SNK_SRC_NEW_SINK_READY), \
> > + S(FR_SWAP_SNK_SRC_SOURCE_VBUS_APPLIED), \
> > + S(FR_SWAP_CANCEL), \
> > + \
> > + S(SNK_TRY), \
> > + S(SNK_TRY_WAIT), \
> > + S(SNK_TRY_WAIT_DEBOUNCE), \
> > + S(SNK_TRY_WAIT_DEBOUNCE_CHECK_VBUS), \
> > + S(SRC_TRYWAIT), \
> > + S(SRC_TRYWAIT_DEBOUNCE), \
> > + S(SRC_TRYWAIT_UNATTACHED), \
> > + \
> > + S(SRC_TRY), \
> > + S(SRC_TRY_WAIT), \
> > + S(SRC_TRY_DEBOUNCE), \
> > + S(SNK_TRYWAIT), \
> > + S(SNK_TRYWAIT_DEBOUNCE), \
> > + S(SNK_TRYWAIT_VBUS), \
> > + S(BIST_RX), \
> > + \
> > + S(GET_STATUS_SEND), \
> > + S(GET_STATUS_SEND_TIMEOUT), \
> > + S(GET_PPS_STATUS_SEND), \
> > + S(GET_PPS_STATUS_SEND_TIMEOUT), \
> > + \
> > + S(GET_SINK_CAP), \
> > + S(GET_SINK_CAP_TIMEOUT), \
> > + \
> > + S(ERROR_RECOVERY), \
> > + S(PORT_RESET), \
> > + S(PORT_RESET_WAIT_OFF), \
> > + \
> > + S(AMS_START), \
> > + S(CHUNK_NOT_SUPP)
> > +
> > +#define GENERATE_ENUM(e) e
> > +
> > +enum tcpm_state {
> > + FOREACH_STATE(GENERATE_ENUM)
> > +};
> > +
>
> Sorry for not bringing it up earlier; I have been struggling with it all along,
> and I could not decide if I should bring it up or not.
>
> I really don't feel comfortable with exporting states outside tcpm.
> Is this really necessary ? The only use seems to be
>
> + if ((tcpm_prev_state == SRC_ATTACH_WAIT && current_state == SRC_UNATTACHED) ||
> + (tcpm_prev_state == SNK_ATTACH_WAIT && current_state == SNK_UNATTACHED))
>
> Plus, of course, the CHECK_CONTAMINANT state.
>
> Is there reason to believe that the relevant state transitions would be different
> for other chips ? If not, would it possibly make sense to move this check
> into the state machine ?
In my understanding, this is definitely one of the transitions that I
think will happen when the port has contaminant. It's quite possible
that there are transitions that I am not aware of yet.
I am OK moving it back to the tcpm state machine, however, then tcpm
would have to keep track of the tcpm state machine transitions and
that would look like V2 version of the patch
(https://lore.kernel.org/lkml/20220831001555.285081-1-badhri@google.com/)
where disconnect_while_debouncing is used to track this. Would that be
OK ?
>
> > enum typec_cc_status {
> > TYPEC_CC_OPEN,
> > TYPEC_CC_RA,
> > @@ -114,6 +234,16 @@ enum tcpm_transmit_type {
> > * Optional; The USB Communications Capable bit indicates if port
> > * partner is capable of communication over the USB data lines
> > * (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
> > + * @check_contaminant:
> > + * Optional; The callback is invoked when chiplevel drivers indicated
> > + * that the USB port needs to be checked for contaminant presence.
> > + * Chip level drivers are expected to check for contaminant and call
> > + * tcpm_clean_port when the port is clean to put the port back into
> > + * toggling state.
> > + * @is_potential_contaminant:
> > + * Optional; TCPM invokes the callback for every TCPM state machine
> > + * transition. Chiplevel drivers can monitor the state machine
> > + * transitions to flag for potential contaminant presence.
>
> I kind of dislike the repeated checks for those callbacks in the state machine.
> Does it make any sense to have one of those callbacks but not the other ?
>
> Because if not it might make sense to check in the registration function
> if either both are NULL or both are set, and then drop most of the checks
> in the state machine handler.
I can definitely check for the registration of the callbacks but cant
drop one of them unless we are OK to make tcpm transitions and invoke
the check_contaminant callback directly.
Thanks,
Badhri
>
> Thanks,
> Guenter
>
> > */
> > struct tcpc_dev {
> > struct fwnode_handle *fwnode;
> > @@ -148,6 +278,8 @@ struct tcpc_dev {
> > bool pps_active, u32 requested_vbus_voltage);
> > bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
> > void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
> > + void (*check_contaminant)(struct tcpc_dev *dev);
> > + bool (*is_potential_contaminant)(struct tcpc_dev *dev, enum tcpm_state current_state);
> > };
> >
> > struct tcpm_port;
> > @@ -165,5 +297,6 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
> > enum tcpm_transmit_status status);
> > void tcpm_pd_hard_reset(struct tcpm_port *port);
> > void tcpm_tcpc_reset(struct tcpm_port *port);
> > +void tcpm_port_clean(struct tcpm_port *port);
> >
> > #endif /* __LINUX_USB_TCPM_H */
> >
> > base-commit: 1524ceb14dd5ebd6f724d993c5ec1a9a8d445d8e
>
^ permalink raw reply
* [PATCH] usb: dwc3: gadget: Ignore End Transfer delay on teardown
From: Thinh Nguyen @ 2022-12-09 0:50 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, Thinh Nguyen, linux-usb
Cc: John Youn, stable, Wesley Cheng, Dan Scally
If we delay sending End Transfer for Setup TRB to be prepared, we need
to check if the End Transfer was in preparation for a driver
teardown/soft-disconnect. In those cases, just send the End Transfer
command without delay.
In the case of soft-disconnect, there's a very small chance the command
may not go through immediately. But should it happen, the Setup TRB will
be prepared during the polling of the controller halted state, allowing
the command to go through then.
In the case of disabling endpoint due to reconfiguration (e.g.
set_interface(alt-setting) or usb reset), then it's driven by the host.
Typically the host wouldn't immediately cancel the control request and
send another control transfer to trigger the End Transfer command
timeout.
Fixes: 4db0fbb60136 ("usb: dwc3: gadget: Don't delay End Transfer on delayed_status")
Cc: stable@vger.kernel.org
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
drivers/usb/dwc3/gadget.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 789976567f9f..89dcfac01235 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1727,6 +1727,7 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
else if (!ret)
dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+ dep->flags &= ~DWC3_EP_DELAY_STOP;
return ret;
}
@@ -3732,8 +3733,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
return;
+ if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
+ return;
+
if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
- (dep->flags & DWC3_EP_DELAY_STOP) ||
(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
return;
base-commit: 81c25247a2a03a0f97e4805d7aff7541ccff6baa
--
2.28.0
^ permalink raw reply related
* Re: [PATCH v6 1/3] usb: typec: tcpm: Add callbacks to mitigate wakeups due to contaminant
From: Guenter Roeck @ 2022-12-08 23:56 UTC (permalink / raw)
To: Badhri Jagan Sridharan, Heikki Krogerus, Greg Kroah-Hartman
Cc: linux-usb, linux-kernel, Kyle Tso
In-Reply-To: <20221208150456.473056-1-badhri@google.com>
On 12/8/22 07:04, Badhri Jagan Sridharan wrote:
> On some of the TCPC implementations, when the Type-C port is exposed
> to contaminants, such as water, TCPC stops toggling while reporting OPEN
> either by the time TCPM reads CC pin status or during CC debounce
> window. This causes TCPM to be stuck in TOGGLING state. If TCPM is made
> to restart toggling, the behavior recurs causing redundant CPU wakeups
> till the USB-C port is free of contaminant.
>
> [206199.287817] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
> [206199.640337] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
> [206199.985789] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
> ...
>
> TCPM invokes is_potential_contaminant callback to allow low level chip
> drivers to monitor TCPM state machine transitions and notify TCPM when
> the Type-C port needs to be checked for potential contaminant presence.
> TCPCs which do have the needed hardware can implement the check_contaminant
> callback which is invoked by TCPM to evaluate for presence of contaminant.
> Lower level TCPC driver can restart toggling through TCPM_PORT_CLEAN event
> when the driver detects that USB-C port is free of contaminant.
>
> Signed-off-by: Badhri Jagan Sridharan <badhri@google.com>
> ---
> Changes since v5:
> * Updated commit message. Removed change id.
> Changes since v4:
> * None
> Changes since v3:
> * None
> Changes since V2:
> * Offloaded tcpm from maintaining disconnect_while_debouncing logic
> * to lower level maxim tcpc driver based on feedback.
> ---
> drivers/usb/typec/tcpm/tcpm.c | 162 +++++++++-------------------------
> include/linux/usb/tcpm.h | 133 ++++++++++++++++++++++++++++
> 2 files changed, 177 insertions(+), 118 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 904c7b4ce2f0..a138cea49612 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -33,119 +33,6 @@
>
> #include <uapi/linux/sched/types.h>
>
> -#define FOREACH_STATE(S) \
> - S(INVALID_STATE), \
> - S(TOGGLING), \
> - S(SRC_UNATTACHED), \
> - S(SRC_ATTACH_WAIT), \
> - S(SRC_ATTACHED), \
> - S(SRC_STARTUP), \
> - S(SRC_SEND_CAPABILITIES), \
> - S(SRC_SEND_CAPABILITIES_TIMEOUT), \
> - S(SRC_NEGOTIATE_CAPABILITIES), \
> - S(SRC_TRANSITION_SUPPLY), \
> - S(SRC_READY), \
> - S(SRC_WAIT_NEW_CAPABILITIES), \
> - \
> - S(SNK_UNATTACHED), \
> - S(SNK_ATTACH_WAIT), \
> - S(SNK_DEBOUNCED), \
> - S(SNK_ATTACHED), \
> - S(SNK_STARTUP), \
> - S(SNK_DISCOVERY), \
> - S(SNK_DISCOVERY_DEBOUNCE), \
> - S(SNK_DISCOVERY_DEBOUNCE_DONE), \
> - S(SNK_WAIT_CAPABILITIES), \
> - S(SNK_NEGOTIATE_CAPABILITIES), \
> - S(SNK_NEGOTIATE_PPS_CAPABILITIES), \
> - S(SNK_TRANSITION_SINK), \
> - S(SNK_TRANSITION_SINK_VBUS), \
> - S(SNK_READY), \
> - \
> - S(ACC_UNATTACHED), \
> - S(DEBUG_ACC_ATTACHED), \
> - S(AUDIO_ACC_ATTACHED), \
> - S(AUDIO_ACC_DEBOUNCE), \
> - \
> - S(HARD_RESET_SEND), \
> - S(HARD_RESET_START), \
> - S(SRC_HARD_RESET_VBUS_OFF), \
> - S(SRC_HARD_RESET_VBUS_ON), \
> - S(SNK_HARD_RESET_SINK_OFF), \
> - S(SNK_HARD_RESET_WAIT_VBUS), \
> - S(SNK_HARD_RESET_SINK_ON), \
> - \
> - S(SOFT_RESET), \
> - S(SRC_SOFT_RESET_WAIT_SNK_TX), \
> - S(SNK_SOFT_RESET), \
> - S(SOFT_RESET_SEND), \
> - \
> - S(DR_SWAP_ACCEPT), \
> - S(DR_SWAP_SEND), \
> - S(DR_SWAP_SEND_TIMEOUT), \
> - S(DR_SWAP_CANCEL), \
> - S(DR_SWAP_CHANGE_DR), \
> - \
> - S(PR_SWAP_ACCEPT), \
> - S(PR_SWAP_SEND), \
> - S(PR_SWAP_SEND_TIMEOUT), \
> - S(PR_SWAP_CANCEL), \
> - S(PR_SWAP_START), \
> - S(PR_SWAP_SRC_SNK_TRANSITION_OFF), \
> - S(PR_SWAP_SRC_SNK_SOURCE_OFF), \
> - S(PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED), \
> - S(PR_SWAP_SRC_SNK_SINK_ON), \
> - S(PR_SWAP_SNK_SRC_SINK_OFF), \
> - S(PR_SWAP_SNK_SRC_SOURCE_ON), \
> - S(PR_SWAP_SNK_SRC_SOURCE_ON_VBUS_RAMPED_UP), \
> - \
> - S(VCONN_SWAP_ACCEPT), \
> - S(VCONN_SWAP_SEND), \
> - S(VCONN_SWAP_SEND_TIMEOUT), \
> - S(VCONN_SWAP_CANCEL), \
> - S(VCONN_SWAP_START), \
> - S(VCONN_SWAP_WAIT_FOR_VCONN), \
> - S(VCONN_SWAP_TURN_ON_VCONN), \
> - S(VCONN_SWAP_TURN_OFF_VCONN), \
> - \
> - S(FR_SWAP_SEND), \
> - S(FR_SWAP_SEND_TIMEOUT), \
> - S(FR_SWAP_SNK_SRC_TRANSITION_TO_OFF), \
> - S(FR_SWAP_SNK_SRC_NEW_SINK_READY), \
> - S(FR_SWAP_SNK_SRC_SOURCE_VBUS_APPLIED), \
> - S(FR_SWAP_CANCEL), \
> - \
> - S(SNK_TRY), \
> - S(SNK_TRY_WAIT), \
> - S(SNK_TRY_WAIT_DEBOUNCE), \
> - S(SNK_TRY_WAIT_DEBOUNCE_CHECK_VBUS), \
> - S(SRC_TRYWAIT), \
> - S(SRC_TRYWAIT_DEBOUNCE), \
> - S(SRC_TRYWAIT_UNATTACHED), \
> - \
> - S(SRC_TRY), \
> - S(SRC_TRY_WAIT), \
> - S(SRC_TRY_DEBOUNCE), \
> - S(SNK_TRYWAIT), \
> - S(SNK_TRYWAIT_DEBOUNCE), \
> - S(SNK_TRYWAIT_VBUS), \
> - S(BIST_RX), \
> - \
> - S(GET_STATUS_SEND), \
> - S(GET_STATUS_SEND_TIMEOUT), \
> - S(GET_PPS_STATUS_SEND), \
> - S(GET_PPS_STATUS_SEND_TIMEOUT), \
> - \
> - S(GET_SINK_CAP), \
> - S(GET_SINK_CAP_TIMEOUT), \
> - \
> - S(ERROR_RECOVERY), \
> - S(PORT_RESET), \
> - S(PORT_RESET_WAIT_OFF), \
> - \
> - S(AMS_START), \
> - S(CHUNK_NOT_SUPP)
> -
> #define FOREACH_AMS(S) \
> S(NONE_AMS), \
> S(POWER_NEGOTIATION), \
> @@ -182,13 +69,8 @@
> S(COUNTRY_INFO), \
> S(COUNTRY_CODES)
>
> -#define GENERATE_ENUM(e) e
> #define GENERATE_STRING(s) #s
>
> -enum tcpm_state {
> - FOREACH_STATE(GENERATE_ENUM)
> -};
> -
> static const char * const tcpm_states[] = {
> FOREACH_STATE(GENERATE_STRING)
> };
> @@ -249,6 +131,7 @@ enum frs_typec_current {
> #define TCPM_RESET_EVENT BIT(2)
> #define TCPM_FRS_EVENT BIT(3)
> #define TCPM_SOURCING_VBUS BIT(4)
> +#define TCPM_PORT_CLEAN BIT(5)
>
> #define LOG_BUFFER_ENTRIES 1024
> #define LOG_BUFFER_ENTRY_SIZE 128
> @@ -483,6 +366,14 @@ struct tcpm_port {
> * SNK_READY for non-pd link.
> */
> bool slow_charger_loop;
> +
> + /*
> + * When true indicates that the lower level drivers indicate potential presence
> + * of contaminant in the connector pins based on the tcpm state machine
> + * transitions.
> + */
> + bool potential_contaminant;
> +
> #ifdef CONFIG_DEBUG_FS
> struct dentry *dentry;
> struct mutex logbuffer_lock; /* log buffer access lock */
> @@ -3904,15 +3795,26 @@ static void run_state_machine(struct tcpm_port *port)
> unsigned int msecs;
> enum tcpm_state upcoming_state;
>
> + if (port->tcpc->is_potential_contaminant)
> + port->potential_contaminant =
> + port->tcpc->is_potential_contaminant(port->tcpc, port->state);
> +
> port->enter_state = port->state;
> switch (port->state) {
> case TOGGLING:
> break;
> + case CHECK_CONTAMINANT:
> + port->tcpc->check_contaminant(port->tcpc);
> + break;
> /* SRC states */
> case SRC_UNATTACHED:
> if (!port->non_pd_role_swap)
> tcpm_swap_complete(port, -ENOTCONN);
> tcpm_src_detach(port);
> + if (port->potential_contaminant && port->tcpc->check_contaminant) {
> + tcpm_set_state(port, CHECK_CONTAMINANT, 0);
> + break;
> + }
> if (tcpm_start_toggling(port, tcpm_rp_cc(port))) {
> tcpm_set_state(port, TOGGLING, 0);
> break;
> @@ -4150,6 +4052,10 @@ static void run_state_machine(struct tcpm_port *port)
> tcpm_swap_complete(port, -ENOTCONN);
> tcpm_pps_complete(port, -ENOTCONN);
> tcpm_snk_detach(port);
> + if (port->potential_contaminant && port->tcpc->check_contaminant) {
> + tcpm_set_state(port, CHECK_CONTAMINANT, 0);
> + break;
> + }
> if (tcpm_start_toggling(port, TYPEC_CC_RD)) {
> tcpm_set_state(port, TOGGLING, 0);
> break;
> @@ -4926,6 +4832,9 @@ static void _tcpm_cc_change(struct tcpm_port *port, enum typec_cc_status cc1,
> else if (tcpm_port_is_sink(port))
> tcpm_set_state(port, SNK_ATTACH_WAIT, 0);
> break;
> + case CHECK_CONTAMINANT:
> + /* Wait for Toggling to be resumed */
> + break;
> case SRC_UNATTACHED:
> case ACC_UNATTACHED:
> if (tcpm_port_is_debug(port) || tcpm_port_is_audio(port) ||
> @@ -5425,6 +5334,10 @@ static void tcpm_pd_event_handler(struct kthread_work *work)
> port->vbus_source = true;
> _tcpm_pd_vbus_on(port);
> }
> + if (events & TCPM_PORT_CLEAN) {
> + tcpm_log(port, "port clean");
> + tcpm_set_state(port, TOGGLING, 0);
> + }
That sets the state to TOGGLING unconditionally, even if it not currently
CHECK_CONTAMINANT. Is that a potential problem ?
>
> spin_lock(&port->pd_event_lock);
> }
> @@ -5477,6 +5390,19 @@ void tcpm_sourcing_vbus(struct tcpm_port *port)
> }
> EXPORT_SYMBOL_GPL(tcpm_sourcing_vbus);
>
> +/*
> + * Low level tcpc drivers invoke this once the port is deemed clean to return
> + * the port to TOGGLING state.
> + */
> +void tcpm_port_clean(struct tcpm_port *port)
> +{
> + spin_lock(&port->pd_event_lock);
> + port->pd_events |= TCPM_PORT_CLEAN;
> + spin_unlock(&port->pd_event_lock);
> + kthread_queue_work(port->wq, &port->event_work);
> +}
> +EXPORT_SYMBOL_GPL(tcpm_port_clean);
> +
> static void tcpm_enable_frs_work(struct kthread_work *work)
> {
> struct tcpm_port *port = container_of(work, struct tcpm_port, enable_frs);
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index bffc8d3e14ad..9cf16372a6e4 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -10,6 +10,126 @@
> #include <linux/usb/typec.h>
> #include "pd.h"
>
> +#define FOREACH_STATE(S) \
> + S(INVALID_STATE), \
> + S(TOGGLING), \
> + S(CHECK_CONTAMINANT), \
> + S(SRC_UNATTACHED), \
> + S(SRC_ATTACH_WAIT), \
> + S(SRC_ATTACHED), \
> + S(SRC_STARTUP), \
> + S(SRC_SEND_CAPABILITIES), \
> + S(SRC_SEND_CAPABILITIES_TIMEOUT), \
> + S(SRC_NEGOTIATE_CAPABILITIES), \
> + S(SRC_TRANSITION_SUPPLY), \
> + S(SRC_READY), \
> + S(SRC_WAIT_NEW_CAPABILITIES), \
> + \
> + S(SNK_UNATTACHED), \
> + S(SNK_ATTACH_WAIT), \
> + S(SNK_DEBOUNCED), \
> + S(SNK_ATTACHED), \
> + S(SNK_STARTUP), \
> + S(SNK_DISCOVERY), \
> + S(SNK_DISCOVERY_DEBOUNCE), \
> + S(SNK_DISCOVERY_DEBOUNCE_DONE), \
> + S(SNK_WAIT_CAPABILITIES), \
> + S(SNK_NEGOTIATE_CAPABILITIES), \
> + S(SNK_NEGOTIATE_PPS_CAPABILITIES), \
> + S(SNK_TRANSITION_SINK), \
> + S(SNK_TRANSITION_SINK_VBUS), \
> + S(SNK_READY), \
> + \
> + S(ACC_UNATTACHED), \
> + S(DEBUG_ACC_ATTACHED), \
> + S(AUDIO_ACC_ATTACHED), \
> + S(AUDIO_ACC_DEBOUNCE), \
> + \
> + S(HARD_RESET_SEND), \
> + S(HARD_RESET_START), \
> + S(SRC_HARD_RESET_VBUS_OFF), \
> + S(SRC_HARD_RESET_VBUS_ON), \
> + S(SNK_HARD_RESET_SINK_OFF), \
> + S(SNK_HARD_RESET_WAIT_VBUS), \
> + S(SNK_HARD_RESET_SINK_ON), \
> + \
> + S(SOFT_RESET), \
> + S(SRC_SOFT_RESET_WAIT_SNK_TX), \
> + S(SNK_SOFT_RESET), \
> + S(SOFT_RESET_SEND), \
> + \
> + S(DR_SWAP_ACCEPT), \
> + S(DR_SWAP_SEND), \
> + S(DR_SWAP_SEND_TIMEOUT), \
> + S(DR_SWAP_CANCEL), \
> + S(DR_SWAP_CHANGE_DR), \
> + \
> + S(PR_SWAP_ACCEPT), \
> + S(PR_SWAP_SEND), \
> + S(PR_SWAP_SEND_TIMEOUT), \
> + S(PR_SWAP_CANCEL), \
> + S(PR_SWAP_START), \
> + S(PR_SWAP_SRC_SNK_TRANSITION_OFF), \
> + S(PR_SWAP_SRC_SNK_SOURCE_OFF), \
> + S(PR_SWAP_SRC_SNK_SOURCE_OFF_CC_DEBOUNCED), \
> + S(PR_SWAP_SRC_SNK_SINK_ON), \
> + S(PR_SWAP_SNK_SRC_SINK_OFF), \
> + S(PR_SWAP_SNK_SRC_SOURCE_ON), \
> + S(PR_SWAP_SNK_SRC_SOURCE_ON_VBUS_RAMPED_UP), \
> + \
> + S(VCONN_SWAP_ACCEPT), \
> + S(VCONN_SWAP_SEND), \
> + S(VCONN_SWAP_SEND_TIMEOUT), \
> + S(VCONN_SWAP_CANCEL), \
> + S(VCONN_SWAP_START), \
> + S(VCONN_SWAP_WAIT_FOR_VCONN), \
> + S(VCONN_SWAP_TURN_ON_VCONN), \
> + S(VCONN_SWAP_TURN_OFF_VCONN), \
> + \
> + S(FR_SWAP_SEND), \
> + S(FR_SWAP_SEND_TIMEOUT), \
> + S(FR_SWAP_SNK_SRC_TRANSITION_TO_OFF), \
> + S(FR_SWAP_SNK_SRC_NEW_SINK_READY), \
> + S(FR_SWAP_SNK_SRC_SOURCE_VBUS_APPLIED), \
> + S(FR_SWAP_CANCEL), \
> + \
> + S(SNK_TRY), \
> + S(SNK_TRY_WAIT), \
> + S(SNK_TRY_WAIT_DEBOUNCE), \
> + S(SNK_TRY_WAIT_DEBOUNCE_CHECK_VBUS), \
> + S(SRC_TRYWAIT), \
> + S(SRC_TRYWAIT_DEBOUNCE), \
> + S(SRC_TRYWAIT_UNATTACHED), \
> + \
> + S(SRC_TRY), \
> + S(SRC_TRY_WAIT), \
> + S(SRC_TRY_DEBOUNCE), \
> + S(SNK_TRYWAIT), \
> + S(SNK_TRYWAIT_DEBOUNCE), \
> + S(SNK_TRYWAIT_VBUS), \
> + S(BIST_RX), \
> + \
> + S(GET_STATUS_SEND), \
> + S(GET_STATUS_SEND_TIMEOUT), \
> + S(GET_PPS_STATUS_SEND), \
> + S(GET_PPS_STATUS_SEND_TIMEOUT), \
> + \
> + S(GET_SINK_CAP), \
> + S(GET_SINK_CAP_TIMEOUT), \
> + \
> + S(ERROR_RECOVERY), \
> + S(PORT_RESET), \
> + S(PORT_RESET_WAIT_OFF), \
> + \
> + S(AMS_START), \
> + S(CHUNK_NOT_SUPP)
> +
> +#define GENERATE_ENUM(e) e
> +
> +enum tcpm_state {
> + FOREACH_STATE(GENERATE_ENUM)
> +};
> +
Sorry for not bringing it up earlier; I have been struggling with it all along,
and I could not decide if I should bring it up or not.
I really don't feel comfortable with exporting states outside tcpm.
Is this really necessary ? The only use seems to be
+ if ((tcpm_prev_state == SRC_ATTACH_WAIT && current_state == SRC_UNATTACHED) ||
+ (tcpm_prev_state == SNK_ATTACH_WAIT && current_state == SNK_UNATTACHED))
Plus, of course, the CHECK_CONTAMINANT state.
Is there reason to believe that the relevant state transitions would be different
for other chips ? If not, would it possibly make sense to move this check
into the state machine ?
> enum typec_cc_status {
> TYPEC_CC_OPEN,
> TYPEC_CC_RA,
> @@ -114,6 +234,16 @@ enum tcpm_transmit_type {
> * Optional; The USB Communications Capable bit indicates if port
> * partner is capable of communication over the USB data lines
> * (e.g. D+/- or SS Tx/Rx). Called to notify the status of the bit.
> + * @check_contaminant:
> + * Optional; The callback is invoked when chiplevel drivers indicated
> + * that the USB port needs to be checked for contaminant presence.
> + * Chip level drivers are expected to check for contaminant and call
> + * tcpm_clean_port when the port is clean to put the port back into
> + * toggling state.
> + * @is_potential_contaminant:
> + * Optional; TCPM invokes the callback for every TCPM state machine
> + * transition. Chiplevel drivers can monitor the state machine
> + * transitions to flag for potential contaminant presence.
I kind of dislike the repeated checks for those callbacks in the state machine.
Does it make any sense to have one of those callbacks but not the other ?
Because if not it might make sense to check in the registration function
if either both are NULL or both are set, and then drop most of the checks
in the state machine handler.
Thanks,
Guenter
> */
> struct tcpc_dev {
> struct fwnode_handle *fwnode;
> @@ -148,6 +278,8 @@ struct tcpc_dev {
> bool pps_active, u32 requested_vbus_voltage);
> bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
> void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
> + void (*check_contaminant)(struct tcpc_dev *dev);
> + bool (*is_potential_contaminant)(struct tcpc_dev *dev, enum tcpm_state current_state);
> };
>
> struct tcpm_port;
> @@ -165,5 +297,6 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
> enum tcpm_transmit_status status);
> void tcpm_pd_hard_reset(struct tcpm_port *port);
> void tcpm_tcpc_reset(struct tcpm_port *port);
> +void tcpm_port_clean(struct tcpm_port *port);
>
> #endif /* __LINUX_USB_TCPM_H */
>
> base-commit: 1524ceb14dd5ebd6f724d993c5ec1a9a8d445d8e
^ permalink raw reply
* Re: dwc3 38100000.usb: No resource for ep1in
From: Thinh Nguyen @ 2022-12-08 23:18 UTC (permalink / raw)
To: Dan Scally
Cc: Felipe Balbi, Laurent Pinchart, Kieran Bingham,
linux-usb@vger.kernel.org, Thinh Nguyen, Greg Kroah-Hartman
In-Reply-To: <f7196abb-d92e-ab47-6c7e-bea686f402c4@ideasonboard.com>
Hi Dan,
On Thu, Dec 08, 2022, Dan Scally wrote:
> Good morning
>
> On 08/12/2022 06:11, Felipe Balbi wrote:
> > Hi,
> >
> > Dan Scally <dan.scally@ideasonboard.com> writes:
> > > I'm having an issue with DWC3 which I'm hoping you might be able to shed
> > > light on. I'm using the UVC gadget function to stream video from an
> > > imx8mp platform, which works just fine. Once I have stopped streaming
> > > however and after some time has passed (the exact duration seems to vary
> > > quite a lot from 1-2 minutes to 15+ mins) I get a kernel warning like so:
> > As Greg mentioned, please add Thinh, the new maintainer, to the loop.
>
>
> Apologies to both of you; for some reason I thought you had taken over from
> Thinh rather than the other way around. Thanks for the heads up Greg.
>
> >
> > > [ 737.996842] configfs-gadget gadget.0: uvc: uvc_function_disable()
> > > [ 738.519582] configfs-gadget gadget.0: uvc: uvc_function_set_alt(0, 0)
> > > [ 738.526060] configfs-gadget gadget.0: uvc: reset UVC interrupt endpoint
> > > [ 738.532713] ------------[ cut here ]------------
> > > [ 738.537683] dwc3 38100000.usb: No resource for ep1in
> > > [ 738.542712] WARNING: CPU: 0 PID: 645 at drivers/usb/dwc3/gadget.c:384
> > > dwc3_send_gadget_ep_cmd+0x478/0x580
> > > [ 738.552314] Modules linked in:
> > > [ 738.555402] CPU: 0 PID: 645 Comm: irq/208-dwc3 Not tainted
> > > 5.19.0-00034-gf017ce943b32 #82
> > > [ 738.563601] Hardware name: Polyhex Debix Model A i.MX8MPlus board (DT)
> > > [ 738.570145] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS
> > > BTYPE=--)
> > > [ 738.577128] pc : dwc3_send_gadget_ep_cmd+0x478/0x580
> > > [ 738.582116] lr : dwc3_send_gadget_ep_cmd+0x478/0x580
> > > [ 738.587102] sp : ffffffc00c3c39e0
> > > [ 738.590433] x29: ffffffc00c3c39e0 x28: 0000000000031006 x27:
> > > 00000000ffffffea
> > > [ 738.597618] x26: 0000000000000006 x25: ffffff8004f32880 x24:
> > > ffffffc00c3c3abc
> > > [ 738.604801] x23: 0000000000000406 x22: ffffffffffff3f00 x21:
> > > ffffffffffff3f0c
> > > [ 738.611984] x20: ffffff8003872600 x19: 0000000000000001 x18:
> > > ffffffffffffffff
> > > [ 738.619166] x17: 000000000000001c x16: 00000000c99c42a5 x15:
> > > ffffffc08c3c36b7
> > > [ 738.626347] x14: 0000000000000000 x13: 6e6931706520726f x12:
> > > 6620656372756f73
> > > [ 738.633528] x11: 00000000000c02a7 x10: ffffffc009af1ac0 x9 :
> > > ffffffc0080c42bc
> > > [ 738.640710] x8 : 00000000ffffefff x7 : ffffffc009af1ac0 x6 :
> > > 0000000000000000
> > > [ 738.647891] x5 : ffffff807fb2eb08 x4 : 0000000000000000 x3 :
> > > 0000000000000027
> > > [ 738.655072] x2 : 0000000000000000 x1 : 0000000000000000 x0 :
> > > ffffff80158a8000
> > > [ 738.662255] Call trace:
> > > [ 738.664721] dwc3_send_gadget_ep_cmd+0x478/0x580
> > > [ 738.669362] __dwc3_gadget_ep_enable+0x4f4/0x714
> > > [ 738.674004] dwc3_gadget_ep_enable+0x6c/0x15c
> > > [ 738.678382] usb_ep_enable+0x4c/0x1bc
> > > [ 738.682067] uvc_function_set_alt+0xcc/0x264
> > > [ 738.686362] composite_setup+0x7ec/0x195c
> > > [ 738.690395] configfs_composite_setup+0x90/0xc0
> > > [ 738.694950] dwc3_ep0_interrupt+0x834/0x9e0
> > > [ 738.699156] dwc3_thread_interrupt+0x994/0xd70
> > > [ 738.703624] irq_thread_fn+0x38/0xa4
> > > [ 738.707221] irq_thread+0x184/0x230
> > > [ 738.710733] kthread+0x118/0x120
> > > [ 738.713981] ret_from_fork+0x10/0x20
> > > [ 738.717582] irq event stamp: 41021
> > > [ 738.720998] hardirqs last enabled at (41019): [<ffffffc008082858>]
> > > finish_task_switch.isra.0+0xe8/0x264
> > > [ 738.730501] hardirqs last disabled at (41021): [<ffffffc008f15058>]
> > > _raw_spin_lock_irqsave+0xc4/0x170
> > > [ 738.739747] softirqs last enabled at (41014): [<ffffffc008a25b90>]
> > > dwc3_thread_interrupt+0x1c0/0xd70
> > > [ 738.748988] softirqs last disabled at (41020): [<ffffffc008a259dc>]
> > > dwc3_thread_interrupt+0xc/0xd70
> > >
> > >
> > > ep1in in this instance refers to a Status Interrupt Endpoint under the
> > > UVC specification, which is being "reset" by uvc_function_set_alt()
> > > against the VideoControl interface (meaning a call to usb_ep_disable()
> > > followed by usb_ep_enable(), see [1]). The isochronous endpoint that
> > > data is streamed across is treated similarly in the same function (when
> > > called for the VideoStreaming interface) and never seems to show the
> > > same warning and as far as I can tell the operation ought to be safe, so
> > > I'm struggling to see anything in the f_uvc code that's misbehaving to
> > > cause the problem. I wondered if you might be able to take a look at the
> > > trace and regdump of the dwc3 (which was taken immediately after the
> > > warning is thrown) and see if that sheds any light on what might be
> > > going on?
> > Best way forward is to collect dwc3 trace points, so we can see what the
> > HW is doing. For details, see [1].
> >
> > [1] https://urldefense.com/v3/__https://kernel.org/doc/html/latest/driver-api/usb/dwc3.html*reporting-bugs__;Iw!!A4F2R9G_pg!Zb5Cx7r3McJ2FCbH_oK14MqINrzor9SihvyA9qFxrlmzU-d_VcBhZYs2jTpWMNE8oxhIH8PMIdk0Nhqj8xBEmkEwe9H6$
>
>
> Both the trace and regdump should be in the attached .tar.gz
>
> >
Looks like we're missing the check to ignore delay End Transfer.
Can you try this to see if it fixes your issue?
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index ebc0e147cc71..65500246323b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2008,6 +2008,7 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
else if (!ret)
dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+ dep->flags &= ~DWC3_EP_DELAY_STOP;
return ret;
}
@@ -4279,8 +4280,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
dwc->hiber_state != DWC3_HIBER_ENTERING_DISCONNECTED)
return;
+ if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
+ return;
+
if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
- (dep->flags & DWC3_EP_DELAY_STOP) ||
(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
return;
Thanks,
Thinh
^ permalink raw reply related
* Re: [RFC PATCH 0/2] Handling of non-numbered feature reports by hidraw
From: Andrey Smirnov @ 2022-12-08 20:58 UTC (permalink / raw)
To: David Rheinsberg
Cc: linux-input, Jiri Kosina, Benjamin Tissoires, linux-kernel,
linux-usb
In-Reply-To: <CADyDSO4uh6b+sSZTkZ2_DR923=bA=kXgK1LqUMkknCMzf_DSwQ@mail.gmail.com>
On Thu, Dec 8, 2022 at 7:46 AM David Rheinsberg
<david.rheinsberg@gmail.com> wrote:
>
> Hi
>
> On Mon, 5 Dec 2022 at 22:04, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > I'm working on a firmware of a device that exposes a HID interface via
> > USB and/or BLE and uses, among other things, non-numbered feature
> > reports. Included in this series are two paches I had to create in
> > order for hidraw devices created for aforementioned subsystems to
> > behave in the same way when exerciesd by the same test tool.
> >
> > I don't know if the patches are acceptable as-is WRT to not breaking
> > existing userspace, hence the RFC tag.
>
> Can you elaborate why you remove the special handling from USBHID but
> add it to UHID? They both operate logically on the same level, so
> shouldn't we simply adjust uhid to include the report-id in buf[0]?
>
> Also, you override buf[0] in UHID, so I wonder what UHID currently
> returns there?
>
> IOW, can you elaborate a bit what the current behavior of each of the
> involved modules is, and what behavior you would expect? This would
> allow to better understand what you are trying to achieve. The more
> context you can give, the easier it is to understand what happens
> there.
>
Sorry it's not very clear, so the difference between the cases is that
in the case of UHID the report ID ends up being included as a part of
"SET_FEATURE", so BlueZ checks UHID_DEV_NUMBERED_FEATURE_REPORTS,
which is not set (correctly) and tries to send the whole payload. This
ends up as a maxlen + 1 (extra byte) write to a property that is
maxlen long, which gets rejected by device's BLE stack.
In the case of USBHID the problem happens in "GET_FEATURE" path. When
userspace reads the expected data back it gets an extra 0 prepended to
the payload, so all of the actual payload has an offset of 1. This
doesn't happen with UHID, which I think is the correct behavior here.
Hopefully that explains the difference, let me know if something is unclear
^ permalink raw reply
* Re: [PATCH v3 0/9] Add the Renesas USBF controller support
From: Rob Herring @ 2022-12-08 20:44 UTC (permalink / raw)
To: Herve Codina
Cc: Geert Uytterhoeven, Michael Turquette, Stephen Boyd,
Krzysztof Kozlowski, Greg Kroah-Hartman, Magnus Damm,
Gareth Williams, linux-renesas-soc, linux-clk, devicetree,
linux-kernel, linux-usb, Thomas Petazzoni, Miquel Raynal
In-Reply-To: <20221208092439.6170cf5e@bootlin.com>
On Thu, Dec 8, 2022 at 2:24 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Rob,
>
> On Wed, 7 Dec 2022 16:19:42 -0600
> Rob Herring <robh+dt@kernel.org> wrote:
>
> > On Wed, Dec 7, 2022 at 10:24 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > >
> > > Hi,
> > >
> > > This series add support for the Renesas USBF controller (USB Device
> > > Controller) available in the Renesas RZ/N1 SoC.
> > >
> > > Based on previous review:
> > > https://lore.kernel.org/all/20221114111513.1436165-3-herve.codina@bootlin.com/
> > >
> > > A new strategy is proposed to handle the H2MODE bit from CFG_USB
> > > register compared to the previous versions on the series. As a
> > > reminder, H2MODE bit allows to configure the internal USB Port
> > > interface for two hosts or one host and one device.
> >
> > Is this case any different from all the phandle properties we have in
> > bindings that point to some misc registers somewhere else you need to
> > poke? If so, I'm not really a fan of duplicating the information.
>
> Our case is that there is a bit in a register that affect several
> devices. This bit must be set before the devices are started.
> If this bit is changed while affected devices are running, system
> hangs can occurs (datasheet).
>
> So, in order to do that we need the device in charge to set
> this bit (sysctrl) to set this bit before other devices (USBF
> and PCI bridge) were started.
That sounds like you just need some platform level initialization and
you are working around the desire to not have platform level
initialization.
Why doesn't/can't the bootloader initialize this? Seems like it might
want to use PCI or USB too.
> At sysctrl level, the bit is set during the probe() call.
> The property 'depends-on' aim is to ensure the probe() calls
> order between provider (sysctrl) and consumers (USBF and PCI
> bridge).
>
> regmap and syscon are used to export registers from one device
> to an other and the probe() calls order is not ensured by the
> core or regmap infrastructure. Indeed, the regmap provider
> probe() will not be called if the regmap provider was not probed
> before the consumer ask for the regmap.
> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/syscon.c#L152
> https://elixir.bootlin.com/linux/latest/source/drivers/mfd/syscon.c#L43
> No specific action synchronisation are done with regmap/syscon
> other than the regmap creation itself.
Oh right. That's in place of course to avoid probe ordering issues...
> I don't think the regmap/syscon will help in our case.
>
> >
> > We also have cases of of_find_compatible_node(NULL, NULL,
> > "foo-bar-syscon") which is a dependency expressed in the driver, but
> > not DT. In either case, adding 'depends-on' would be an ABI break as
> > you are requiring a DT change.
>
> In order to avoid the DT change, I can keep the 'depends-on'
> optional in the PCI bridge binding.
> This will be functionnal as sysctrl is already used in this node
> (power-domain = <&sysctrl>). The relationship is already present
> with this power-domain link.
>
> If ok, I will do this change in v4 series.
I agree with Geert that this shouldn't be needed.
Rob
^ permalink raw reply
* Re: [syzbot] KASAN: use-after-free Read in __usb_hcd_giveback_urb (2)
From: Alan Stern @ 2022-12-08 17:40 UTC (permalink / raw)
To: Oliver Neukum
Cc: syzbot, WeitaoWang-oc, arnd, gregkh, khalid.masum.92, kishon,
linux-kernel, linux-usb, syzkaller-bugs
In-Reply-To: <cac60598-5080-5876-d28d-e8caab8b9b0f@suse.com>
On Thu, Dec 08, 2022 at 03:36:45PM +0100, Oliver Neukum wrote:
> On 06.12.22 16:38, Alan Stern wrote:
>
> Hi,
>
> > Oliver:
> >
> > This looks like a bug in the anchor API.
>
> Yes, it does.
> > On Tue, Dec 06, 2022 at 02:43:41AM -0800, syzbot wrote:
> > > Hello,
> > >
> > > syzbot found the following issue on:
> > >
> > > HEAD commit: ef4d3ea40565 afs: Fix server->active leak in afs_put_server
> > > git tree: upstream
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=100b244d880000
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=8e7e79f8a1e34200
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=712fd0e60dda3ba34642
> > > compiler: Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63, GNU ld (GNU Binutils for Debian) 2.35.2
> > >
> > > Unfortunately, I don't have any reproducer for this issue yet.
> > >
> > > Downloadable assets:
> > > disk image: https://storage.googleapis.com/syzbot-assets/ef790e7777cd/disk-ef4d3ea4.raw.xz
> > > vmlinux: https://storage.googleapis.com/syzbot-assets/2ed3c6bc9230/vmlinux-ef4d3ea4.xz
> > > kernel image: https://storage.googleapis.com/syzbot-assets/f1dbd004fa88/bzImage-ef4d3ea4.xz
> > >
> > > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > > Reported-by: syzbot+712fd0e60dda3ba34642@syzkaller.appspotmail.com
> > >
> > > xpad 3-1:179.65: xpad_irq_in - usb_submit_urb failed with result -19
> > > xpad 3-1:179.65: xpad_irq_out - usb_submit_urb failed with result -19
> > > ==================================================================
> > > BUG: KASAN: use-after-free in register_lock_class+0x8d2/0x9b0 kernel/locking/lockdep.c:1338
> > > __wake_up+0xf8/0x1c0 kernel/sched/wait.c:156
> > > __usb_hcd_giveback_urb+0x3a0/0x530 drivers/usb/core/hcd.c:1674
>
>
> > This is the call to usb_anchor_resume_wakeups(). The call is made after
> > the completion handler callback. Evidently the xpad driver deallocated
> > the anchor during that time window. This can happen if the driver is
> > just waiting for its last URB to complete before freeing all its memory.
>
> Yes, complete() had run.
> >
> > I don't know what the best solution is. It may be necessary to refcount
> > anchors somehow.
>
> Then we cannot embed them anymore. Many drivers would need a lot of changes.
> xpad included.
It's hard to tell what's really going on. Looking at
xpad_stop_output(), you see that it doesn't do anything if xpad->type is
XTYPE_UNKNOWN. Is that what happened here?
I can't figure out where the underlying race is. Maybe it's not
directly connected with anchors after all.
> As far as I can tell the order we decrease use_count is correct. But:
>
> 6ec4147e7bdbd (Hans de Goede 2013-10-09 17:01:41 +0200 1674) usb_anchor_resume_wakeups(anchor);
> 94dfd7edfd5c9 (Ming Lei 2013-07-03 22:53:07 +0800 1675) atomic_dec(&urb->use_count);
>
> Do we need to guarantee memory ordering here?
I don't think we need to do anything more. usb_kill_urb() is careful to
wait for completion handlers to finish, and we already have
smp_mb__after_atomic() barriers in the appropriate places to ensure
proper memory ordering.
Alan Stern
^ permalink raw reply
* Re: [PATCH] usb: gadget: Assign a unique name for each configfs
From: Frank Li @ 2022-12-08 16:53 UTC (permalink / raw)
To: gregkh, Rondreis; +Cc: balbi, linux-kernel, linux-usb, stern, imx
In-Reply-To: <YxiHE9s4NDZOILpe@kroah.com>
From: Frank Li <Frank.li@nxp.com>
On Wed, Sep 07, 2022 at 01:57:07PM +0200, Greg KH wrote:
> On Wed, Sep 07, 2022 at 07:22:10PM +0800, Rondreis wrote:
> > When fuzzing the kernel, I couldn't use configfs to attach more than one
> > gadget. When attaching the second gadget with a different UDC it always
> > failed and the kernel message said:
> >
> > Error: Driver 'configfs-gadget' is already registered, aborting...
> > UDC core: g1: driver registration failed: -16
> >
> > The problem is that when creating multiple gadgets with configfs and
> > binding them to different UDCs, the UDC drivers have the same name
> > "configfs-gadget". Because of the addition of the "gadget" bus,
> > naming conflicts will occur when more than one UDC drivers
> > registered to the bus.
> >
> > It's not an isolated case, this patch refers to the commit f2d8c2606825
> > ("usb: gadget: Fix non-unique driver names in raw-gadget driver").
> > Each configfs-gadget driver will be assigned a unique name
> > "configfs-gadget.N", with a different value of N for each driver instance.
>
> Please wrap your changelog text at 72 columns like the documentation
> asks for.
>
> > Reported-and-tested-by: Rondreis <linhaoguo86@gmail.com>
> > Signed-off-by: Rondreis <linhaoguo86@gmail.com>
>
I met the same issue, do you plan continue this patch after fix greg's comments.
If you have not time, I can continue it.
Frank Li
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox