| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aKxOOGBkR7ZfwCo5@xhacker>
Date: Mon, 25 Aug 2025 19:51:20 +0800
From: Jisheng Zhang <jszhang@...nel.org>
To: Richard Leitner <richard.leitner@...ux.dev>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-usb@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] usb: usb251xb: support usage case without I2C
control
On Sun, Aug 24, 2025 at 08:55:09PM +0200, Richard Leitner wrote:
> Hi Jisheng,
>
> On Thu, Aug 21, 2025 at 12:17:43AM +0800, Jisheng Zhang wrote:
> > Currently, the usb251xb assumes i2c control. But from HW point of
> > view, the hub supports usage case without any i2c, we only want the
> > gpio controls.
> >
> > Refactor the code so that register writes for configuration are only
> > performed if the device has a i2c_client provided and also register as
> > a platform driver. This allows the driver to be used to manage GPIO
> > based control of the device.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@...nel.org>
> > ---
> > drivers/usb/misc/usb251xb.c | 108 +++++++++++++++++++++++++++++++-----
> > 1 file changed, 94 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
> > index cb2f946de42c..e9a9404d17b2 100644
> > --- a/drivers/usb/misc/usb251xb.c
> > +++ b/drivers/usb/misc/usb251xb.c
> > @@ -17,6 +17,7 @@
> > #include <linux/module.h>
> > #include <linux/nls.h>
> > #include <linux/of.h>
> > +#include <linux/platform_device.h>
> > #include <linux/regulator/consumer.h>
> > #include <linux/slab.h>
> >
> > @@ -242,15 +243,19 @@ static int usb251xb_check_dev_children(struct device *dev, void *child)
> > static int usb251x_check_gpio_chip(struct usb251xb *hub)
> > {
> > struct gpio_chip *gc = gpiod_to_chip(hub->gpio_reset);
> > - struct i2c_adapter *adap = hub->i2c->adapter;
> > + struct i2c_adapter *adap;
> > int ret;
> >
> > + if (!hub->i2c)
> > + return 0;
> > +
> > if (!hub->gpio_reset)
> > return 0;
> >
> > if (!gc)
> > return -EINVAL;
> >
> > + adap = hub->i2c->adapter;
> > ret = usb251xb_check_dev_children(&adap->dev, gc->parent);
> > if (ret) {
> > dev_err(hub->dev, "Reset GPIO chip is at the same i2c-bus\n");
> > @@ -271,7 +276,8 @@ static void usb251xb_reset(struct usb251xb *hub)
> > if (!hub->gpio_reset)
> > return;
> >
> > - i2c_lock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
> > + if (hub->i2c)
> > + i2c_lock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
> >
> > gpiod_set_value_cansleep(hub->gpio_reset, 1);
> > usleep_range(1, 10); /* >=1us RESET_N asserted */
> > @@ -280,7 +286,8 @@ static void usb251xb_reset(struct usb251xb *hub)
> > /* wait for hub recovery/stabilization */
> > usleep_range(500, 750); /* >=500us after RESET_N deasserted */
> >
> > - i2c_unlock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
> > + if (hub->i2c)
> > + i2c_unlock_bus(hub->i2c->adapter, I2C_LOCK_SEGMENT);
> > }
> >
> > static int usb251xb_connect(struct usb251xb *hub)
> > @@ -289,7 +296,11 @@ static int usb251xb_connect(struct usb251xb *hub)
> > int err, i;
> > char i2c_wb[USB251XB_I2C_REG_SZ];
> >
> > - memset(i2c_wb, 0, USB251XB_I2C_REG_SZ);
> > + if (!hub->i2c) {
> > + usb251xb_reset(hub);
> > + dev_info(dev, "hub is put in default configuration.\n");
> > + return 0;
> > + }
>
> You dropped the memset of write buffer. Are you sure this doesn't cause
> any issues? (Sorry, I'm currently travelling, so I wasn't able to test
> the series on real hw)
oops, good catch! Even if the removing memset() works it should be in a
seperate patch. I will send v2 soon. Thanks for your review!
>
> Apart from that the patch LGTM. Thanks! :-)
>
> >
> > if (hub->skip_config) {
> > dev_info(dev, "Skip hub configuration, only attach.\n");
> > @@ -698,18 +709,13 @@ static int usb251xb_i2c_probe(struct i2c_client *i2c)
> > return usb251xb_probe(hub);
> > }
> >
> > -static int usb251xb_suspend(struct device *dev)
> > +static int usb251xb_suspend(struct usb251xb *hub)
> > {
> > - struct i2c_client *client = to_i2c_client(dev);
> > - struct usb251xb *hub = i2c_get_clientdata(client);
> > -
> > return regulator_disable(hub->vdd);
> > }
> >
> > -static int usb251xb_resume(struct device *dev)
> > +static int usb251xb_resume(struct usb251xb *hub)
> > {
> > - struct i2c_client *client = to_i2c_client(dev);
> > - struct usb251xb *hub = i2c_get_clientdata(client);
> > int err;
> >
> > err = regulator_enable(hub->vdd);
> > @@ -719,7 +725,23 @@ static int usb251xb_resume(struct device *dev)
> > return usb251xb_connect(hub);
> > }
> >
> > -static DEFINE_SIMPLE_DEV_PM_OPS(usb251xb_pm_ops, usb251xb_suspend, usb251xb_resume);
> > +static int usb251xb_i2c_suspend(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct usb251xb *hub = i2c_get_clientdata(client);
> > +
> > + return usb251xb_suspend(hub);
> > +}
> > +
> > +static int usb251xb_i2c_resume(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct usb251xb *hub = i2c_get_clientdata(client);
> > +
> > + return usb251xb_resume(hub);
> > +}
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(usb251xb_i2c_pm_ops, usb251xb_i2c_suspend, usb251xb_i2c_resume);
> >
> > static const struct i2c_device_id usb251xb_id[] = {
> > { "usb2422" },
> > @@ -739,13 +761,71 @@ static struct i2c_driver usb251xb_i2c_driver = {
> > .driver = {
> > .name = DRIVER_NAME,
> > .of_match_table = usb251xb_of_match,
> > - .pm = pm_sleep_ptr(&usb251xb_pm_ops),
> > + .pm = pm_sleep_ptr(&usb251xb_i2c_pm_ops),
> > },
> > .probe = usb251xb_i2c_probe,
> > .id_table = usb251xb_id,
> > };
> >
> > -module_i2c_driver(usb251xb_i2c_driver);
> > +static int usb251xb_plat_probe(struct platform_device *pdev)
> > +{
> > + struct usb251xb *hub;
> > +
> > + hub = devm_kzalloc(&pdev->dev, sizeof(*hub), GFP_KERNEL);
> > + if (!hub)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, hub);
> > + hub->dev = &pdev->dev;
> > +
> > + return usb251xb_probe(hub);
> > +}
> > +
> > +static int usb251xb_plat_suspend(struct device *dev)
> > +{
> > + return usb251xb_suspend(dev_get_drvdata(dev));
> > +}
> > +
> > +static int usb251xb_plat_resume(struct device *dev)
> > +{
> > + return usb251xb_resume(dev_get_drvdata(dev));
> > +}
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(usb251xb_plat_pm_ops, usb251xb_plat_suspend, usb251xb_plat_resume);
> > +
> > +static struct platform_driver usb251xb_plat_driver = {
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .of_match_table = usb251xb_of_match,
> > + .pm = pm_sleep_ptr(&usb251xb_plat_pm_ops),
> > + },
> > + .probe = usb251xb_plat_probe,
> > +};
> > +
> > +static int __init usb251xb_init(void)
> > +{
> > + int err;
> > +
> > + err = i2c_add_driver(&usb251xb_i2c_driver);
> > + if (err)
> > + return err;
> > +
> > + err = platform_driver_register(&usb251xb_plat_driver);
> > + if (err) {
> > + i2c_del_driver(&usb251xb_i2c_driver);
> > + return err;
> > + }
> > +
> > + return 0;
> > +}
> > +module_init(usb251xb_init);
> > +
> > +static void __exit usb251xb_exit(void)
> > +{
> > + platform_driver_unregister(&usb251xb_plat_driver);
> > + i2c_del_driver(&usb251xb_i2c_driver);
> > +}
> > +module_exit(usb251xb_exit);
> >
> > MODULE_AUTHOR("Richard Leitner <richard.leitner@...data.com>");
> > MODULE_DESCRIPTION("USB251x/xBi USB 2.0 Hub Controller Driver");
> > --
> > 2.50.0
> >
>
> regards;rl
Powered by blists - more mailing lists