[PATCH] misc: hisi_hikey_usb: cancel role switch work on remove

[PATCH] misc: hisi_hikey_usb: cancel role switch work on remove

[Hongyan Xu]

The hub role switch set callback schedules private work that dereferences
both driver data and the downstream role switch. Unregistering the hub
switch does not drain work already queued, and a callback that fetched
driver data before unregister can queue more work during removal.

Mark the instance as removing under its mutex and serialize removal
with role switch set callbacks by setting the role to USB_ROLE_NONE.
The callback clears the switch driver data when it observes removal and
no longer queues work. Unregister the switch, cancel the work, and only
then drop the downstream role switch reference.

This issue was found by a static analysis tool.

Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
 drivers/misc/hisi_hikey_usb.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
index 2165ec3..9db6429 100644
--- a/drivers/misc/hisi_hikey_usb.c
+++ b/drivers/misc/hisi_hikey_usb.c
@@ -45,6 +45,7 @@ struct hisi_hikey_usb {
 
 	struct mutex lock;
 	struct work_struct work;
+	bool removing;
 
 	struct notifier_block nb;
 };
@@ -130,17 +131,23 @@ static void relay_set_role_switch(struct work_struct *work)
 static int hub_usb_role_switch_set(struct usb_role_switch *sw, enum usb_role role)
 {
 	struct hisi_hikey_usb *hisi_hikey_usb = usb_role_switch_get_drvdata(sw);
+	int ret = 0;
 
-	if (!hisi_hikey_usb || !hisi_hikey_usb->dev_role_sw)
-		return -EINVAL;
+	if (!hisi_hikey_usb)
+		return -ENODEV;
 
 	mutex_lock(&hisi_hikey_usb->lock);
-	hisi_hikey_usb->role = role;
+	if (hisi_hikey_usb->removing || !hisi_hikey_usb->dev_role_sw) {
+		if (hisi_hikey_usb->removing)
+			usb_role_switch_set_drvdata(sw, NULL);
+		ret = -ENODEV;
+	} else {
+		hisi_hikey_usb->role = role;
+		schedule_work(&hisi_hikey_usb->work);
+	}
 	mutex_unlock(&hisi_hikey_usb->lock);
 
-	schedule_work(&hisi_hikey_usb->work);
-
-	return 0;
+	return ret;
 }
 
 static int hisi_hikey_usb_of_role_switch(struct platform_device *pdev,
@@ -244,7 +251,15 @@ static int  hisi_hikey_usb_remove(struct platform_device *pdev)
 	struct hisi_hikey_usb *hisi_hikey_usb = platform_get_drvdata(pdev);
 
 	if (hisi_hikey_usb->hub_role_sw) {
+		mutex_lock(&hisi_hikey_usb->lock);
+		hisi_hikey_usb->removing = true;
+		mutex_unlock(&hisi_hikey_usb->lock);
+
+		/* Serialize with set callbacks and clear their private data. */
+		usb_role_switch_set_role(hisi_hikey_usb->hub_role_sw,
+					 USB_ROLE_NONE);
 		usb_role_switch_unregister(hisi_hikey_usb->hub_role_sw);
+		cancel_work_sync(&hisi_hikey_usb->work);
 
 		if (hisi_hikey_usb->dev_role_sw)
 			usb_role_switch_put(hisi_hikey_usb->dev_role_sw);
-- 
2.50.1.windows.1

Re: [PATCH] misc: hisi_hikey_usb: cancel role switch work on remove

[John Stultz]

On Mon, Jul 27, 2026 at 11:56 PM Hongyan Xu <getshell@seu.edu.cn> wrote:
>
> The hub role switch set callback schedules private work that dereferences
> both driver data and the downstream role switch. Unregistering the hub
> switch does not drain work already queued, and a callback that fetched
> driver data before unregister can queue more work during removal.
>
> Mark the instance as removing under its mutex and serialize removal
> with role switch set callbacks by setting the role to USB_ROLE_NONE.
> The callback clears the switch driver data when it observes removal and
> no longer queues work. Unregister the switch, cancel the work, and only
> then drop the downstream role switch reference.
>
> This issue was found by a static analysis tool.
>
> Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
> ---
>  drivers/misc/hisi_hikey_usb.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
>

I no longer have hardware to test with, but on quick glance this looks
reasonable to me. Adding Linaro folks who might still be able to test
and validate.

For what it's worth:
Acked-by: John Stultz <jstultz@google.com>

Re: [PATCH] misc: hisi_hikey_usb: cancel role switch work on remove

[Arnd Bergmann]

On Wed, Jul 29, 2026, at 04:54, John Stultz wrote:
> On Mon, Jul 27, 2026 at 11:56 PM Hongyan Xu <getshell@seu.edu.cn> wrote:
>
> I no longer have hardware to test with, but on quick glance this looks
> reasonable to me. Adding Linaro folks who might still be able to test
> and validate.
>
> For what it's worth:
> Acked-by: John Stultz <jstultz@google.com>

From what I can tell, the driver never actually worked upstream. Mauro tried
fixing it until late 2021 in this series
https://lore.kernel.org/all/cover.1639735742.git.mchehab@kernel.org/
and then gave up. The .compatible="hisilicon,usbhub" ID does not match
on any dts file and there is no binding for it.

       Arnd

Re: [PATCH] misc: hisi_hikey_usb: cancel role switch work on remove

[Hongyan Xu]

Thanks for the context. I wasn't aware that the driver had no upstream
DT user or binding.

Given that, I will hold off on revising this patch. If removing the
unused driver is the preferred direction, I can prepare a separate
removal patch.

Thanks,
Hongyan

Re: [PATCH] misc: hisi_hikey_usb: cancel role switch work on remove

[Greg Kroah-Hartman]

On Wed, Jul 29, 2026 at 06:56:18PM +0800, Hongyan Xu wrote:
> Thanks for the context. I wasn't aware that the driver had no upstream
> DT user or binding.

If so, should we just drop the driver if there are no in-kernel users of
it and no one has hardware for it anymore?

thanks,

greg k-h

Re: [PATCH] misc: hisi_hikey_usb: cancel role switch work on remove

[Greg Kroah-Hartman]

On Wed, Jul 29, 2026 at 06:56:18PM +0800, Hongyan Xu wrote:
> Thanks for the context. I wasn't aware that the driver had no upstream
> DT user or binding.
> 
> Given that, I will hold off on revising this patch. If removing the
> unused driver is the preferred direction, I can prepare a separate
> removal patch.

Sorry, yes, that's what I was wanting to ask as well.

Re: [PATCH] misc: hisi_hikey_usb: cancel role switch work on remove

[Arnd Bergmann]

On Wed, Jul 29, 2026, at 15:42, Greg Kroah-Hartman wrote:
> On Wed, Jul 29, 2026 at 06:56:18PM +0800, Hongyan Xu wrote:
>> Thanks for the context. I wasn't aware that the driver had no upstream
>> DT user or binding.
>> 
>> Given that, I will hold off on revising this patch. If removing the
>> unused driver is the preferred direction, I can prepare a separate
>> removal patch.
>
> Sorry, yes, that's what I was wanting to ask as well.

I asked around internally, and Ben Copeland mentioned that the hikey960
itself should still work fine with mainline kernels, but the role switch
driver was never getting tested.

I think we can just remove this driver. If anyone wants to get
the functionality working later, they can reintroduce a working
version in drivers/phy/, where all other usb-role-switch support
lives these days.

     Arnd

Re: [PATCH] misc: hisi_hikey_usb: cancel role switch work on remove

[Hongyan Xu]

Hi Greg, Arnd,

Thanks, that makes sense.

I'll drop the role-switch work cancellation patch and send a
separate patch to remove this driver instead.

Thanks,
Hongyan

[PATCH] misc: hisi_hikey_usb: remove untested role-switch driver

[Hongyan Xu]

During review of a lifetime fix for this driver, it turned out that the
role-switch path has no upstream DT binding or in-tree user and has not
been tested in years.

Rather than keep carrying and fixing an effectively unused board-specific
driver, remove it. If this functionality is needed again later, it can be
reintroduced in drivers/phy/, where USB role-switch support lives today.

Link: https://lore.kernel.org/r/20260728065558.1532-1-getshell@seu.edu.cn
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Hongyan Xu <getshell@seu.edu.cn>
---
 MAINTAINERS                   |   6 -
 drivers/misc/Kconfig          |  10 --
 drivers/misc/Makefile         |   1 -
 drivers/misc/hisi_hikey_usb.c | 274 ----------------------------------
 4 files changed, 291 deletions(-)
 delete mode 100644 drivers/misc/hisi_hikey_usb.c

diff --git a/MAINTAINERS b/MAINTAINERS
index dd844ac8d..76d082981 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10636,12 +10636,6 @@ W:	http://www.highpoint-tech.com
 F:	Documentation/scsi/hptiop.rst
 F:	drivers/scsi/hptiop.c
 
-HIKEY960 ONBOARD USB GPIO HUB DRIVER
-M:	John Stultz <jstultz@google.com>
-L:	linux-kernel@vger.kernel.org
-S:	Maintained
-F:	drivers/misc/hisi_hikey_usb.c
-
 HIMAX HX83112B TOUCHSCREEN SUPPORT
 M:	Job Noorman <job@noorman.info>
 L:	linux-input@vger.kernel.org
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 6b37d6115..564e200be 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -492,16 +492,6 @@ config MISC_RTSX
 	tristate
 	default MISC_RTSX_PCI || MISC_RTSX_USB
 
-config HISI_HIKEY_USB
-	tristate "USB GPIO Hub on HiSilicon Hikey 960/970 Platform"
-	depends on (OF && GPIOLIB) || COMPILE_TEST
-	depends on USB_ROLE_SWITCH
-	help
-	  If you say yes here this adds support for the on-board USB GPIO hub
-	  found on HiKey 960/970 boards, which is necessary to support
-	  switching between the dual-role USB-C port and the USB-A host ports
-	  using only one USB controller.
-
 config OPEN_DICE
 	tristate "Open Profile for DICE driver"
 	depends on OF_RESERVED_MEM
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d6c917229..fc5ab6ad4 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -58,7 +58,6 @@ obj-y				+= cardreader/
 obj-$(CONFIG_PVPANIC)   	+= pvpanic/
 obj-$(CONFIG_UACCE)		+= uacce/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
-obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
 obj-$(CONFIG_NTSYNC)		+= ntsync.o
 obj-$(CONFIG_HI6421V600_IRQ)	+= hi6421v600-irq.o
 obj-$(CONFIG_OPEN_DICE)		+= open-dice.o
diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
deleted file mode 100644
index ffe7b945a..000000000
--- a/drivers/misc/hisi_hikey_usb.c
+++ /dev/null
@@ -1,274 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Support for usb functionality of Hikey series boards
- * based on Hisilicon Kirin Soc.
- *
- * Copyright (C) 2017-2018 Hilisicon Electronics Co., Ltd.
- *		http://www.huawei.com
- *
- * Authors: Yu Chen <chenyu56@huawei.com>
- */
-
-#include <linux/gpio/consumer.h>
-#include <linux/kernel.h>
-#include <linux/mod_devicetable.h>
-#include <linux/module.h>
-#include <linux/notifier.h>
-#include <linux/platform_device.h>
-#include <linux/property.h>
-#include <linux/regulator/consumer.h>
-#include <linux/slab.h>
-#include <linux/usb/role.h>
-
-#define DEVICE_DRIVER_NAME "hisi_hikey_usb"
-
-#define HUB_VBUS_POWER_ON 1
-#define HUB_VBUS_POWER_OFF 0
-#define USB_SWITCH_TO_HUB 1
-#define USB_SWITCH_TO_TYPEC 0
-#define TYPEC_VBUS_POWER_ON 1
-#define TYPEC_VBUS_POWER_OFF 0
-
-struct hisi_hikey_usb {
-	struct device *dev;
-	struct gpio_desc *otg_switch;
-	struct gpio_desc *typec_vbus;
-	struct gpio_desc *reset;
-
-	struct regulator *regulator;
-
-	struct usb_role_switch *hub_role_sw;
-
-	struct usb_role_switch *dev_role_sw;
-	enum usb_role role;
-
-	struct mutex lock;
-	struct work_struct work;
-
-	struct notifier_block nb;
-};
-
-static void hub_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb, int value)
-{
-	int ret, status;
-
-	if (!hisi_hikey_usb->regulator)
-		return;
-
-	status = regulator_is_enabled(hisi_hikey_usb->regulator);
-	if (status == !!value)
-		return;
-
-	if (value)
-		ret = regulator_enable(hisi_hikey_usb->regulator);
-	else
-		ret = regulator_disable(hisi_hikey_usb->regulator);
-
-	if (ret)
-		dev_err(hisi_hikey_usb->dev,
-			"Can't switch regulator state to %s\n",
-			value ? "enabled" : "disabled");
-}
-
-static void usb_switch_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
-			    int switch_to)
-{
-	if (!hisi_hikey_usb->otg_switch)
-		return;
-
-	gpiod_set_value_cansleep(hisi_hikey_usb->otg_switch, switch_to);
-}
-
-static void usb_typec_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
-				 int value)
-{
-	if (!hisi_hikey_usb->typec_vbus)
-		return;
-
-	gpiod_set_value_cansleep(hisi_hikey_usb->typec_vbus, value);
-}
-
-static void relay_set_role_switch(struct work_struct *work)
-{
-	struct hisi_hikey_usb *hisi_hikey_usb = container_of(work,
-							struct hisi_hikey_usb,
-							work);
-	struct usb_role_switch *sw;
-	enum usb_role role;
-
-	if (!hisi_hikey_usb || !hisi_hikey_usb->dev_role_sw)
-		return;
-
-	mutex_lock(&hisi_hikey_usb->lock);
-	switch (hisi_hikey_usb->role) {
-	case USB_ROLE_NONE:
-		usb_typec_power_ctrl(hisi_hikey_usb, TYPEC_VBUS_POWER_OFF);
-		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_HUB);
-		hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_ON);
-		break;
-	case USB_ROLE_HOST:
-		hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_OFF);
-		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_TYPEC);
-		usb_typec_power_ctrl(hisi_hikey_usb, TYPEC_VBUS_POWER_ON);
-		break;
-	case USB_ROLE_DEVICE:
-		hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_OFF);
-		usb_typec_power_ctrl(hisi_hikey_usb, TYPEC_VBUS_POWER_OFF);
-		usb_switch_ctrl(hisi_hikey_usb, USB_SWITCH_TO_TYPEC);
-		break;
-	default:
-		break;
-	}
-	sw = hisi_hikey_usb->dev_role_sw;
-	role = hisi_hikey_usb->role;
-	mutex_unlock(&hisi_hikey_usb->lock);
-
-	usb_role_switch_set_role(sw, role);
-}
-
-static int hub_usb_role_switch_set(struct usb_role_switch *sw, enum usb_role role)
-{
-	struct hisi_hikey_usb *hisi_hikey_usb = usb_role_switch_get_drvdata(sw);
-
-	if (!hisi_hikey_usb || !hisi_hikey_usb->dev_role_sw)
-		return -EINVAL;
-
-	mutex_lock(&hisi_hikey_usb->lock);
-	hisi_hikey_usb->role = role;
-	mutex_unlock(&hisi_hikey_usb->lock);
-
-	schedule_work(&hisi_hikey_usb->work);
-
-	return 0;
-}
-
-static int hisi_hikey_usb_of_role_switch(struct platform_device *pdev,
-					 struct hisi_hikey_usb *hisi_hikey_usb)
-{
-	struct device *dev = &pdev->dev;
-	struct usb_role_switch_desc hub_role_switch = {NULL};
-
-	if (!device_property_read_bool(dev, "usb-role-switch"))
-		return 0;
-
-	hisi_hikey_usb->otg_switch = devm_gpiod_get(dev, "otg-switch",
-						    GPIOD_OUT_HIGH);
-	if (IS_ERR(hisi_hikey_usb->otg_switch)) {
-		dev_err(dev, "get otg-switch failed with error %ld\n",
-			PTR_ERR(hisi_hikey_usb->otg_switch));
-		return PTR_ERR(hisi_hikey_usb->otg_switch);
-	}
-
-	hisi_hikey_usb->typec_vbus = devm_gpiod_get(dev, "typec-vbus",
-						    GPIOD_OUT_LOW);
-	if (IS_ERR(hisi_hikey_usb->typec_vbus)) {
-		dev_err(dev, "get typec-vbus failed with error %ld\n",
-			PTR_ERR(hisi_hikey_usb->typec_vbus));
-		return PTR_ERR(hisi_hikey_usb->typec_vbus);
-	}
-
-	hisi_hikey_usb->reset = devm_gpiod_get_optional(dev,
-							"hub-reset-en",
-							GPIOD_OUT_HIGH);
-	if (IS_ERR(hisi_hikey_usb->reset)) {
-		dev_err(dev, "get hub-reset-en failed with error %ld\n",
-			PTR_ERR(hisi_hikey_usb->reset));
-		return PTR_ERR(hisi_hikey_usb->reset);
-	}
-
-	hisi_hikey_usb->dev_role_sw = usb_role_switch_get(dev);
-	if (!hisi_hikey_usb->dev_role_sw)
-		return -EPROBE_DEFER;
-	if (IS_ERR(hisi_hikey_usb->dev_role_sw)) {
-		dev_err(dev, "get device role switch failed with error %ld\n",
-			PTR_ERR(hisi_hikey_usb->dev_role_sw));
-		return PTR_ERR(hisi_hikey_usb->dev_role_sw);
-	}
-
-	INIT_WORK(&hisi_hikey_usb->work, relay_set_role_switch);
-
-	hub_role_switch.fwnode = dev_fwnode(dev);
-	hub_role_switch.set = hub_usb_role_switch_set;
-	hub_role_switch.driver_data = hisi_hikey_usb;
-
-	hisi_hikey_usb->hub_role_sw = usb_role_switch_register(dev,
-							       &hub_role_switch);
-
-	if (IS_ERR(hisi_hikey_usb->hub_role_sw)) {
-		dev_err(dev,
-			"failed to register hub role with error %ld\n",
-			PTR_ERR(hisi_hikey_usb->hub_role_sw));
-		usb_role_switch_put(hisi_hikey_usb->dev_role_sw);
-		return PTR_ERR(hisi_hikey_usb->hub_role_sw);
-	}
-
-	return 0;
-}
-
-static int hisi_hikey_usb_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct hisi_hikey_usb *hisi_hikey_usb;
-	int ret;
-
-	hisi_hikey_usb = devm_kzalloc(dev, sizeof(*hisi_hikey_usb), GFP_KERNEL);
-	if (!hisi_hikey_usb)
-		return -ENOMEM;
-
-	hisi_hikey_usb->dev = &pdev->dev;
-	mutex_init(&hisi_hikey_usb->lock);
-
-	hisi_hikey_usb->regulator = devm_regulator_get(dev, "hub-vdd");
-	if (IS_ERR(hisi_hikey_usb->regulator)) {
-		if (PTR_ERR(hisi_hikey_usb->regulator) == -EPROBE_DEFER) {
-			dev_info(dev, "waiting for hub-vdd-supply\n");
-			return PTR_ERR(hisi_hikey_usb->regulator);
-		}
-		dev_err(dev, "get hub-vdd-supply failed with error %ld\n",
-			PTR_ERR(hisi_hikey_usb->regulator));
-		return PTR_ERR(hisi_hikey_usb->regulator);
-	}
-
-	ret = hisi_hikey_usb_of_role_switch(pdev, hisi_hikey_usb);
-	if (ret)
-		return ret;
-
-	platform_set_drvdata(pdev, hisi_hikey_usb);
-
-	return 0;
-}
-
-static void hisi_hikey_usb_remove(struct platform_device *pdev)
-{
-	struct hisi_hikey_usb *hisi_hikey_usb = platform_get_drvdata(pdev);
-
-	if (hisi_hikey_usb->hub_role_sw) {
-		usb_role_switch_unregister(hisi_hikey_usb->hub_role_sw);
-
-		if (hisi_hikey_usb->dev_role_sw)
-			usb_role_switch_put(hisi_hikey_usb->dev_role_sw);
-	} else {
-		hub_power_ctrl(hisi_hikey_usb, HUB_VBUS_POWER_OFF);
-	}
-}
-
-static const struct of_device_id id_table_hisi_hikey_usb[] = {
-	{ .compatible = "hisilicon,usbhub" },
-	{}
-};
-MODULE_DEVICE_TABLE(of, id_table_hisi_hikey_usb);
-
-static struct platform_driver hisi_hikey_usb_driver = {
-	.probe = hisi_hikey_usb_probe,
-	.remove = hisi_hikey_usb_remove,
-	.driver = {
-		.name = DEVICE_DRIVER_NAME,
-		.of_match_table = id_table_hisi_hikey_usb,
-	},
-};
-
-module_platform_driver(hisi_hikey_usb_driver);
-
-MODULE_AUTHOR("Yu Chen <chenyu56@huawei.com>");
-MODULE_DESCRIPTION("Driver Support for USB functionality of Hikey");
-MODULE_LICENSE("GPL v2");
-- 
2.50.1.windows.1