extcon: usbc-tusb320: unregister typec port on driver removal
Commit Message
From: Alvin Šipraga <alsi@bang-olufsen.dk>
The driver can register a typec port if suitable firmware properties are
present. But if the driver is removed through sysfs unbind, rmmod or
similar, then it does not clean up after itself and the typec port
device remains registered. This can be seen in sysfs, where stale typec
ports get left over in /sys/class/typec.
In order to fix this we have to add an i2c_driver remove function and
call typec_unregister_port(), which is a no-op in the case where no
typec port is created and the pointer remains NULL.
In the process we should also put the fwnode_handle when the typec port
isn't registered anymore, including if an error occurs during probe. The
typec subsystem does not increase or decrease the reference counter for
us, so we track it in the driver's private data.
Note that the conditional check on TYPEC_PWR_MODE_PD was removed in the
probe path because a call to tusb320_set_adv_pwr_mode() will perform an
even more robust validation immediately after, hence there is no
functional change here.
Fixes: bf7571c00dca ("extcon: usbc-tusb320: Add USB TYPE-C support")
Cc: stable@vger.kernel.org
Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
drivers/extcon/extcon-usbc-tusb320.c | 42 ++++++++++++++++++++++------
1 file changed, 34 insertions(+), 8 deletions(-)
Comments
Hi Alvin,
I love your patch! Perhaps something to improve:
[auto build test WARNING on chanwoo-extcon/extcon-next]
[also build test WARNING on linus/master v6.3-rc2 next-20230310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Alvin-ipraga/extcon-usbc-tusb320-unregister-typec-port-on-driver-removal/20230313-210245
base: https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
patch link: https://lore.kernel.org/r/20230313130105.4183296-1-alvin%40pqrs.dk
patch subject: [PATCH] extcon: usbc-tusb320: unregister typec port on driver removal
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230313/202303132335.Qnq7apal-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/fe414069d19f6d59c7c34f820459f4114e2de136
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Alvin-ipraga/extcon-usbc-tusb320-unregister-typec-port-on-driver-removal/20230313-210245
git checkout fe414069d19f6d59c7c34f820459f4114e2de136
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303132335.Qnq7apal-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/extcon/extcon-usbc-tusb320.c: In function 'tusb320_typec_probe':
>> drivers/extcon/extcon-usbc-tusb320.c:429:13: warning: statement with no effect [-Wunused-value]
429 | priv->connector_fwnode;
| ~~~~^~~~~~~~~~~~~~~~~~
vim +429 drivers/extcon/extcon-usbc-tusb320.c
379
380 static int tusb320_typec_probe(struct i2c_client *client,
381 struct tusb320_priv *priv)
382 {
383 struct fwnode_handle *connector;
384 const char *cap_str;
385 int ret;
386
387 /* The Type-C connector is optional, for backward compatibility. */
388 connector = device_get_named_child_node(&client->dev, "connector");
389 if (!connector)
390 return 0;
391
392 /* Type-C connector found. */
393 ret = typec_get_fw_cap(&priv->cap, connector);
394 if (ret)
395 goto err_put;
396
397 priv->port_type = priv->cap.type;
398
399 /* This goes into register 0x8 field CURRENT_MODE_ADVERTISE */
400 ret = fwnode_property_read_string(connector, "typec-power-opmode", &cap_str);
401 if (ret)
402 goto err_put;
403
404 ret = typec_find_pwr_opmode(cap_str);
405 if (ret < 0)
406 goto err_put;
407
408 priv->pwr_opmode = ret;
409
410 /* Initialize the hardware with the devicetree settings. */
411 ret = tusb320_set_adv_pwr_mode(priv);
412 if (ret)
413 goto err_put;
414
415 priv->cap.revision = USB_TYPEC_REV_1_1;
416 priv->cap.accessory[0] = TYPEC_ACCESSORY_AUDIO;
417 priv->cap.accessory[1] = TYPEC_ACCESSORY_DEBUG;
418 priv->cap.orientation_aware = true;
419 priv->cap.driver_data = priv;
420 priv->cap.ops = &tusb320_typec_ops;
421 priv->cap.fwnode = connector;
422
423 priv->port = typec_register_port(&client->dev, &priv->cap);
424 if (IS_ERR(priv->port)) {
425 ret = PTR_ERR(priv->port);
426 goto err_put;
427 }
428
> 429 priv->connector_fwnode;
430
431 return 0;
432
433 err_put:
434 fwnode_handle_put(connector);
435
436 return ret;
437 }
438
On Tue, Mar 14, 2023 at 12:04:34AM +0800, kernel test robot wrote:
> Hi Alvin,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on chanwoo-extcon/extcon-next]
> [also build test WARNING on linus/master v6.3-rc2 next-20230310]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Alvin-ipraga/extcon-usbc-tusb320-unregister-typec-port-on-driver-removal/20230313-210245
> base: https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
> patch link: https://lore.kernel.org/r/20230313130105.4183296-1-alvin%40pqrs.dk
> patch subject: [PATCH] extcon: usbc-tusb320: unregister typec port on driver removal
> config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230313/202303132335.Qnq7apal-lkp@intel.com/config)
> compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
> reproduce (this is a W=1 build):
> # https://github.com/intel-lab-lkp/linux/commit/fe414069d19f6d59c7c34f820459f4114e2de136
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Alvin-ipraga/extcon-usbc-tusb320-unregister-typec-port-on-driver-removal/20230313-210245
> git checkout fe414069d19f6d59c7c34f820459f4114e2de136
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> make W=1 O=build_dir ARCH=x86_64 olddefconfig
> make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Link: https://lore.kernel.org/oe-kbuild-all/202303132335.Qnq7apal-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> drivers/extcon/extcon-usbc-tusb320.c: In function 'tusb320_typec_probe':
> >> drivers/extcon/extcon-usbc-tusb320.c:429:13: warning: statement with no effect [-Wunused-value]
> 429 | priv->connector_fwnode;
> | ~~~~^~~~~~~~~~~~~~~~~~
Oops, I was on an old test branch when sending this patch... Thank you
kernel test robot. I will send v2 tomorrow to allow for some other
comments first. The only difference in v2 is that this line is corrected
to:
priv->connector_fwnode = connector;
Kind regards,
Alvin
>
>
> vim +429 drivers/extcon/extcon-usbc-tusb320.c
>
> 379
> 380 static int tusb320_typec_probe(struct i2c_client *client,
> 381 struct tusb320_priv *priv)
> 382 {
> 383 struct fwnode_handle *connector;
> 384 const char *cap_str;
> 385 int ret;
> 386
> 387 /* The Type-C connector is optional, for backward compatibility. */
> 388 connector = device_get_named_child_node(&client->dev, "connector");
> 389 if (!connector)
> 390 return 0;
> 391
> 392 /* Type-C connector found. */
> 393 ret = typec_get_fw_cap(&priv->cap, connector);
> 394 if (ret)
> 395 goto err_put;
> 396
> 397 priv->port_type = priv->cap.type;
> 398
> 399 /* This goes into register 0x8 field CURRENT_MODE_ADVERTISE */
> 400 ret = fwnode_property_read_string(connector, "typec-power-opmode", &cap_str);
> 401 if (ret)
> 402 goto err_put;
> 403
> 404 ret = typec_find_pwr_opmode(cap_str);
> 405 if (ret < 0)
> 406 goto err_put;
> 407
> 408 priv->pwr_opmode = ret;
> 409
> 410 /* Initialize the hardware with the devicetree settings. */
> 411 ret = tusb320_set_adv_pwr_mode(priv);
> 412 if (ret)
> 413 goto err_put;
> 414
> 415 priv->cap.revision = USB_TYPEC_REV_1_1;
> 416 priv->cap.accessory[0] = TYPEC_ACCESSORY_AUDIO;
> 417 priv->cap.accessory[1] = TYPEC_ACCESSORY_DEBUG;
> 418 priv->cap.orientation_aware = true;
> 419 priv->cap.driver_data = priv;
> 420 priv->cap.ops = &tusb320_typec_ops;
> 421 priv->cap.fwnode = connector;
> 422
> 423 priv->port = typec_register_port(&client->dev, &priv->cap);
> 424 if (IS_ERR(priv->port)) {
> 425 ret = PTR_ERR(priv->port);
> 426 goto err_put;
> 427 }
> 428
> > 429 priv->connector_fwnode;
> 430
> 431 return 0;
> 432
> 433 err_put:
> 434 fwnode_handle_put(connector);
> 435
> 436 return ret;
> 437 }
> 438
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests
Hi Alvin,
I love your patch! Perhaps something to improve:
[auto build test WARNING on chanwoo-extcon/extcon-next]
[also build test WARNING on linus/master v6.3-rc2 next-20230310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Alvin-ipraga/extcon-usbc-tusb320-unregister-typec-port-on-driver-removal/20230313-210245
base: https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
patch link: https://lore.kernel.org/r/20230313130105.4183296-1-alvin%40pqrs.dk
patch subject: [PATCH] extcon: usbc-tusb320: unregister typec port on driver removal
config: i386-randconfig-a013-20230313 (https://download.01.org/0day-ci/archive/20230314/202303141316.EltVGG8V-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/fe414069d19f6d59c7c34f820459f4114e2de136
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Alvin-ipraga/extcon-usbc-tusb320-unregister-typec-port-on-driver-removal/20230313-210245
git checkout fe414069d19f6d59c7c34f820459f4114e2de136
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/extcon/ drivers/media/common/videobuf2/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303141316.EltVGG8V-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/extcon/extcon-usbc-tusb320.c:429:8: warning: expression result unused [-Wunused-value]
priv->connector_fwnode;
~~~~ ^~~~~~~~~~~~~~~~
1 warning generated.
vim +429 drivers/extcon/extcon-usbc-tusb320.c
379
380 static int tusb320_typec_probe(struct i2c_client *client,
381 struct tusb320_priv *priv)
382 {
383 struct fwnode_handle *connector;
384 const char *cap_str;
385 int ret;
386
387 /* The Type-C connector is optional, for backward compatibility. */
388 connector = device_get_named_child_node(&client->dev, "connector");
389 if (!connector)
390 return 0;
391
392 /* Type-C connector found. */
393 ret = typec_get_fw_cap(&priv->cap, connector);
394 if (ret)
395 goto err_put;
396
397 priv->port_type = priv->cap.type;
398
399 /* This goes into register 0x8 field CURRENT_MODE_ADVERTISE */
400 ret = fwnode_property_read_string(connector, "typec-power-opmode", &cap_str);
401 if (ret)
402 goto err_put;
403
404 ret = typec_find_pwr_opmode(cap_str);
405 if (ret < 0)
406 goto err_put;
407
408 priv->pwr_opmode = ret;
409
410 /* Initialize the hardware with the devicetree settings. */
411 ret = tusb320_set_adv_pwr_mode(priv);
412 if (ret)
413 goto err_put;
414
415 priv->cap.revision = USB_TYPEC_REV_1_1;
416 priv->cap.accessory[0] = TYPEC_ACCESSORY_AUDIO;
417 priv->cap.accessory[1] = TYPEC_ACCESSORY_DEBUG;
418 priv->cap.orientation_aware = true;
419 priv->cap.driver_data = priv;
420 priv->cap.ops = &tusb320_typec_ops;
421 priv->cap.fwnode = connector;
422
423 priv->port = typec_register_port(&client->dev, &priv->cap);
424 if (IS_ERR(priv->port)) {
425 ret = PTR_ERR(priv->port);
426 goto err_put;
427 }
428
> 429 priv->connector_fwnode;
430
431 return 0;
432
433 err_put:
434 fwnode_handle_put(connector);
435
436 return ret;
437 }
438
@@ -78,6 +78,7 @@ struct tusb320_priv {
struct typec_capability cap;
enum typec_port_type port_type;
enum typec_pwr_opmode pwr_opmode;
+ struct fwnode_handle *connector_fwnode;
};
static const char * const tusb_attached_states[] = {
@@ -391,27 +392,25 @@ static int tusb320_typec_probe(struct i2c_client *client,
/* Type-C connector found. */
ret = typec_get_fw_cap(&priv->cap, connector);
if (ret)
- return ret;
+ goto err_put;
priv->port_type = priv->cap.type;
/* This goes into register 0x8 field CURRENT_MODE_ADVERTISE */
ret = fwnode_property_read_string(connector, "typec-power-opmode", &cap_str);
if (ret)
- return ret;
+ goto err_put;
ret = typec_find_pwr_opmode(cap_str);
if (ret < 0)
- return ret;
- if (ret == TYPEC_PWR_MODE_PD)
- return -EINVAL;
+ goto err_put;
priv->pwr_opmode = ret;
/* Initialize the hardware with the devicetree settings. */
ret = tusb320_set_adv_pwr_mode(priv);
if (ret)
- return ret;
+ goto err_put;
priv->cap.revision = USB_TYPEC_REV_1_1;
priv->cap.accessory[0] = TYPEC_ACCESSORY_AUDIO;
@@ -422,10 +421,25 @@ static int tusb320_typec_probe(struct i2c_client *client,
priv->cap.fwnode = connector;
priv->port = typec_register_port(&client->dev, &priv->cap);
- if (IS_ERR(priv->port))
- return PTR_ERR(priv->port);
+ if (IS_ERR(priv->port)) {
+ ret = PTR_ERR(priv->port);
+ goto err_put;
+ }
+
+ priv->connector_fwnode;
return 0;
+
+err_put:
+ fwnode_handle_put(connector);
+
+ return ret;
+}
+
+static void tusb320_typec_remove(struct tusb320_priv *priv)
+{
+ typec_unregister_port(priv->port);
+ fwnode_handle_put(priv->connector_fwnode);
}
static int tusb320_probe(struct i2c_client *client)
@@ -438,7 +452,9 @@ static int tusb320_probe(struct i2c_client *client)
priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
+
priv->dev = &client->dev;
+ i2c_set_clientdata(client, priv);
priv->regmap = devm_regmap_init_i2c(client, &tusb320_regmap_config);
if (IS_ERR(priv->regmap))
@@ -489,10 +505,19 @@ static int tusb320_probe(struct i2c_client *client)
tusb320_irq_handler,
IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
client->name, priv);
+ if (ret)
+ tusb320_typec_remove(priv);
return ret;
}
+static void tusb320_remove(struct i2c_client *client)
+{
+ struct tusb320_priv *priv = i2c_get_clientdata(client);
+
+ tusb320_typec_remove(priv);
+}
+
static const struct of_device_id tusb320_extcon_dt_match[] = {
{ .compatible = "ti,tusb320", .data = &tusb320_ops, },
{ .compatible = "ti,tusb320l", .data = &tusb320l_ops, },
@@ -502,6 +527,7 @@ MODULE_DEVICE_TABLE(of, tusb320_extcon_dt_match);
static struct i2c_driver tusb320_extcon_driver = {
.probe_new = tusb320_probe,
+ .remove = tusb320_remove,
.driver = {
.name = "extcon-tusb320",
.of_match_table = tusb320_extcon_dt_match,