| Message ID | 20240423223309.1468198-4-aren@peacevolution.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: light: stk3310: support powering off during suspend | expand |
On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolution.org> wrote: > > From: Ondrej Jirman <megi@xff.cz> > > VDD power input can be used to completely power off the chip during > system suspend. Do so if available. ... > ret = stk3310_init(indio_dev); > if (ret < 0) > - return ret; > + goto err_vdd_disable; This is wrong. You will have the regulator being disabled _before_ IRQ. Note, that the original code likely has a bug which sets states before disabling IRQ and removing a handler. Side note, you may make the driver neater with help of struct device *dev = &client->dev; defined in this patch. ... > static int stk3310_suspend(struct device *dev) > { > struct stk3310_data *data; > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); Side note: This may be updated (in a separate change) to use dev_get_drvdata() directly. Jonathan, do we have something like iio_priv_from_drvdata(struct device *dev)? Seems many drivers may utilise it. > } ... > static int stk3310_resume(struct device *dev) Ditto. -- With Best Regards, Andy Shevchenko
Hi Aren,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on sunxi/sunxi/for-next robh/for-next linus/master v6.9-rc5 next-20240423]
[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/Aren-Moynihan/dt-bindings-iio-light-stk33xx-add-vdd-and-leda-regulators/20240424-064250
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20240423223309.1468198-4-aren%40peacevolution.org
patch subject: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
config: arm64-randconfig-001-20240424 (https://download.01.org/0day-ci/archive/20240424/202404242057.PUDY5RB1-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 5ef5eb66fb428aaf61fb51b709f065c069c11242)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240424/202404242057.PUDY5RB1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404242057.PUDY5RB1-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/iio/light/stk3310.c:10:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2208:
include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
522 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/iio/light/stk3310.c:615:38: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
615 | return dev_err_probe(&client->dev, ret, "get regulator vdd failed\n");
| ^~~
drivers/iio/light/stk3310.c:594:9: note: initialize the variable 'ret' to silence this warning
594 | int ret;
| ^
| = 0
2 warnings generated.
vim +/ret +615 drivers/iio/light/stk3310.c
591
592 static int stk3310_probe(struct i2c_client *client)
593 {
594 int ret;
595 struct iio_dev *indio_dev;
596 struct stk3310_data *data;
597
598 indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
599 if (!indio_dev) {
600 dev_err(&client->dev, "iio allocation failed!\n");
601 return -ENOMEM;
602 }
603
604 data = iio_priv(indio_dev);
605 data->client = client;
606 i2c_set_clientdata(client, indio_dev);
607
608 device_property_read_u32(&client->dev, "proximity-near-level",
609 &data->ps_near_level);
610
611 mutex_init(&data->lock);
612
613 data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
614 if (IS_ERR(data->vdd_reg))
> 615 return dev_err_probe(&client->dev, ret, "get regulator vdd failed\n");
616
617 ret = stk3310_regmap_init(data);
618 if (ret < 0)
619 return ret;
620
621 indio_dev->info = &stk3310_info;
622 indio_dev->name = STK3310_DRIVER_NAME;
623 indio_dev->modes = INDIO_DIRECT_MODE;
624 indio_dev->channels = stk3310_channels;
625 indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
626
627 ret = regulator_enable(data->vdd_reg);
628 if (ret)
629 return dev_err_probe(&client->dev, ret,
630 "regulator vdd enable failed\n");
631
632 /* we need a short delay to allow the chip time to power on */
633 fsleep(1000);
634
635 ret = stk3310_init(indio_dev);
636 if (ret < 0)
637 goto err_vdd_disable;
638
639 if (client->irq > 0) {
640 ret = devm_request_threaded_irq(&client->dev, client->irq,
641 stk3310_irq_handler,
642 stk3310_irq_event_handler,
643 IRQF_TRIGGER_FALLING |
644 IRQF_ONESHOT,
645 STK3310_EVENT, indio_dev);
646 if (ret < 0) {
647 dev_err(&client->dev, "request irq %d failed\n",
648 client->irq);
649 goto err_standby;
650 }
651 }
652
653 ret = iio_device_register(indio_dev);
654 if (ret < 0) {
655 dev_err(&client->dev, "device_register failed\n");
656 goto err_standby;
657 }
658
659 return 0;
660
661 err_standby:
662 stk3310_set_state(data, STK3310_STATE_STANDBY);
663 err_vdd_disable:
664 regulator_disable(data->vdd_reg);
665 return ret;
666 }
667
On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolution.org> wrote: > > > > From: Ondrej Jirman <megi@xff.cz> > > > > VDD power input can be used to completely power off the chip during > > system suspend. Do so if available. > > ... > > > ret = stk3310_init(indio_dev); > > if (ret < 0) > > - return ret; > > + goto err_vdd_disable; > > This is wrong. You will have the regulator being disabled _before_ > IRQ. Note, that the original code likely has a bug which sets states > before disabling IRQ and removing a handler. How so? stk3310_init is called before enabling the interrupt. Original code has a bug that IRQ is enabled before registering the IIO device, so if IRQ is triggered before registration, iio_push_event from IRQ handler may be called on a not yet registered IIO device. Never saw it happen, though. :) kind regards, o. > Side note, you may make the driver neater with help of > > struct device *dev = &client->dev; > > defined in this patch. > > ... > > > static int stk3310_suspend(struct device *dev) > > { > > struct stk3310_data *data; > > > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > > Side note: This may be updated (in a separate change) to use > dev_get_drvdata() directly. > > Jonathan, do we have something like iio_priv_from_drvdata(struct > device *dev)? Seems many drivers may utilise it. > > > } > > ... > > > static int stk3310_resume(struct device *dev) > > Ditto. > > -- > With Best Regards, > Andy Shevchenko
On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman <megi@xff.cz> wrote: > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolution.org> wrote: ... > > > ret = stk3310_init(indio_dev); > > > if (ret < 0) > > > - return ret; > > > + goto err_vdd_disable; > > > > This is wrong. You will have the regulator being disabled _before_ > > IRQ. Note, that the original code likely has a bug which sets states > > before disabling IRQ and removing a handler. > > How so? stk3310_init is called before enabling the interrupt. Exactly, IRQ is registered with devm and hence the error path and remove stages will got it in a wrong order. > Original code has a bug that IRQ is enabled before registering the > IIO device, Indeed, but this is another bug. > so if IRQ is triggered before registration, iio_push_event > from IRQ handler may be called on a not yet registered IIO device. > > Never saw it happen, though. :) Because nobody cares enough to enable DEBUG_SHIRQ.
On Wed, Apr 24, 2024 at 06:20:41PM GMT, Andy Shevchenko wrote: > On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman <megi@xff.cz> wrote: > > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolution.org> wrote: > > ... > > > > > ret = stk3310_init(indio_dev); > > > > if (ret < 0) > > > > - return ret; > > > > + goto err_vdd_disable; > > > > > > This is wrong. You will have the regulator being disabled _before_ > > > IRQ. Note, that the original code likely has a bug which sets states > > > before disabling IRQ and removing a handler. > > > > How so? stk3310_init is called before enabling the interrupt. > > Exactly, IRQ is registered with devm and hence the error path and > remove stages will got it in a wrong order. Makes no sense. IRQ is not enabled here, yet. So in error path, the code will just disable the regulator and devm will unref it later on. IRQ doesn't enter the picture here at all in the error path. > > Original code has a bug that IRQ is enabled before registering the > > IIO device, > > Indeed, but this is another bug. > > > so if IRQ is triggered before registration, iio_push_event > > from IRQ handler may be called on a not yet registered IIO device. > > > > Never saw it happen, though. :) > > Because nobody cares enough to enable DEBUG_SHIRQ. Nice debug tool. I bet it makes quite a mess when enabled. :) Kind regards, o. > -- > With Best Regards, > Andy Shevchenko
On Wed, Apr 24, 2024 at 7:14 PM Ondřej Jirman <megi@xff.cz> wrote: > On Wed, Apr 24, 2024 at 06:20:41PM GMT, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman <megi@xff.cz> wrote: > > > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolution.org> wrote: ... > > > > > ret = stk3310_init(indio_dev); > > > > > if (ret < 0) > > > > > - return ret; > > > > > + goto err_vdd_disable; > > > > > > > > This is wrong. You will have the regulator being disabled _before_ > > > > IRQ. Note, that the original code likely has a bug which sets states > > > > before disabling IRQ and removing a handler. > > > > > > How so? stk3310_init is called before enabling the interrupt. > > > > Exactly, IRQ is registered with devm and hence the error path and > > remove stages will got it in a wrong order. > > Makes no sense. Huh?! > IRQ is not enabled here, yet. So in error path, the code will > just disable the regulator and devm will unref it later on. IRQ doesn't enter > the picture here at all in the error path. Error path _after_ IRQ handler has been _successfully_ installed. And complete ->remove() stage. > > > Original code has a bug that IRQ is enabled before registering the > > > IIO device, > > > > Indeed, but this is another bug. > > > > > so if IRQ is triggered before registration, iio_push_event > > > from IRQ handler may be called on a not yet registered IIO device. > > > > > > Never saw it happen, though. :) > > > > Because nobody cares enough to enable DEBUG_SHIRQ. > > Nice debug tool. I bet it makes quite a mess when enabled. :) FWIW, I have had it enabled for ages, but I have only a few devices, so I fixed a few cases in the past WRT shared IRQ issues.
On Wed, Apr 24, 2024 at 08:31:27PM GMT, Andy Shevchenko wrote: > On Wed, Apr 24, 2024 at 7:14 PM Ondřej Jirman <megi@xff.cz> wrote: > > On Wed, Apr 24, 2024 at 06:20:41PM GMT, Andy Shevchenko wrote: > > > On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman <megi@xff.cz> wrote: > > > > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > > > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolution.org> wrote: > > ... > > > > > > > ret = stk3310_init(indio_dev); > > > > > > if (ret < 0) > > > > > > - return ret; > > > > > > + goto err_vdd_disable; > > > > > > > > > > This is wrong. You will have the regulator being disabled _before_ > > > > > IRQ. Note, that the original code likely has a bug which sets states > > > > > before disabling IRQ and removing a handler. > > > > > > > > How so? stk3310_init is called before enabling the interrupt. > > > > > > Exactly, IRQ is registered with devm and hence the error path and > > > remove stages will got it in a wrong order. > > > > Makes no sense. > > Huh?! > > > IRQ is not enabled here, yet. So in error path, the code will > > just disable the regulator and devm will unref it later on. IRQ doesn't enter > > the picture here at all in the error path. > > Error path _after_ IRQ handler has been _successfully_ installed. > And complete ->remove() stage. Allright. So fixing the other issue I mentioned will fix this one too, because there will be no error path after IRQ enable, then. kind regards, o. > > > > Original code has a bug that IRQ is enabled before registering the > > > > IIO device, > > > > > > Indeed, but this is another bug. > > > > > > > so if IRQ is triggered before registration, iio_push_event > > > > from IRQ handler may be called on a not yet registered IIO device. > > > > > > > > Never saw it happen, though. :) > > > > > > Because nobody cares enough to enable DEBUG_SHIRQ. > > > > Nice debug tool. I bet it makes quite a mess when enabled. :) > > FWIW, I have had it enabled for ages, but I have only a few devices, > so I fixed a few cases in the past WRT shared IRQ issues. > > -- > With Best Regards, > Andy Shevchenko
On Wed, Apr 24, 2024 at 02:16:06AM +0300, Andy Shevchenko wrote: > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolution.org> wrote: > > > > From: Ondrej Jirman <megi@xff.cz> > > > > VDD power input can be used to completely power off the chip during > > system suspend. Do so if available. > > ... > > > ret = stk3310_init(indio_dev); > > if (ret < 0) > > - return ret; > > + goto err_vdd_disable; > > This is wrong. You will have the regulator being disabled _before_ > IRQ. Note, that the original code likely has a bug which sets states > before disabling IRQ and removing a handler. Oh! now I see the issue you were talking about last time around. I expect that means the irq shouldn't be managed with devres, so it can be the first thing freed in the remove function (I haven't checked the docs to see if there's an easier way yet). I'll add a patch to fix the order of the handling of the irq (both this and the issue Ondřej brought up). > Side note, you may make the driver neater with help of > > struct device *dev = &client->dev; > > defined in this patch. Good point, it's minor, but it should be a net improvement. > ... > > > static int stk3310_suspend(struct device *dev) > > { > > struct stk3310_data *data; > > > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > > Side note: This may be updated (in a separate change) to use > dev_get_drvdata() directly. > > Jonathan, do we have something like iio_priv_from_drvdata(struct > device *dev)? Seems many drivers may utilise it. > At this rate I'm going to need to split off a separate style / code cleanup series so I don't keep introducing dumb bugs while rebasing this one. Thank you for your time - Aren
Hi Aren,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Aren-Moynihan/dt-bindings-iio-light-stk33xx-add-vdd-and-leda-regulators/20240424-064250
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20240423223309.1468198-4-aren%40peacevolution.org
patch subject: [PATCH v2 2/6] iio: light: stk3310: Implement vdd supply and power it off during suspend
config: i386-randconfig-141-20240424 (https://download.01.org/0day-ci/archive/20240425/202404251021.4OPER3OS-lkp@intel.com/config)
compiler: gcc-10 (Ubuntu 10.5.0-1ubuntu1) 10.5.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202404251021.4OPER3OS-lkp@intel.com/
smatch warnings:
drivers/iio/light/stk3310.c:615 stk3310_probe() error: uninitialized symbol 'ret'.
vim +/ret +615 drivers/iio/light/stk3310.c
9046d80dce04c6 Uwe Kleine-König 2022-11-18 592 static int stk3310_probe(struct i2c_client *client)
be9e6229d67696 Tiberiu Breana 2015-04-27 593 {
be9e6229d67696 Tiberiu Breana 2015-04-27 594 int ret;
be9e6229d67696 Tiberiu Breana 2015-04-27 595 struct iio_dev *indio_dev;
be9e6229d67696 Tiberiu Breana 2015-04-27 596 struct stk3310_data *data;
be9e6229d67696 Tiberiu Breana 2015-04-27 597
be9e6229d67696 Tiberiu Breana 2015-04-27 598 indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
be9e6229d67696 Tiberiu Breana 2015-04-27 599 if (!indio_dev) {
be9e6229d67696 Tiberiu Breana 2015-04-27 600 dev_err(&client->dev, "iio allocation failed!\n");
be9e6229d67696 Tiberiu Breana 2015-04-27 601 return -ENOMEM;
be9e6229d67696 Tiberiu Breana 2015-04-27 602 }
be9e6229d67696 Tiberiu Breana 2015-04-27 603
be9e6229d67696 Tiberiu Breana 2015-04-27 604 data = iio_priv(indio_dev);
be9e6229d67696 Tiberiu Breana 2015-04-27 605 data->client = client;
be9e6229d67696 Tiberiu Breana 2015-04-27 606 i2c_set_clientdata(client, indio_dev);
d6ecb01583d4e0 Arnaud Ferraris 2022-04-20 607
d6ecb01583d4e0 Arnaud Ferraris 2022-04-20 608 device_property_read_u32(&client->dev, "proximity-near-level",
d6ecb01583d4e0 Arnaud Ferraris 2022-04-20 609 &data->ps_near_level);
d6ecb01583d4e0 Arnaud Ferraris 2022-04-20 610
be9e6229d67696 Tiberiu Breana 2015-04-27 611 mutex_init(&data->lock);
be9e6229d67696 Tiberiu Breana 2015-04-27 612
dd231c1d219f6b Ondrej Jirman 2024-04-23 613 data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
dd231c1d219f6b Ondrej Jirman 2024-04-23 614 if (IS_ERR(data->vdd_reg))
dd231c1d219f6b Ondrej Jirman 2024-04-23 @615 return dev_err_probe(&client->dev, ret, "get regulator vdd failed\n");
s/ret/PTR_ERR(data->vdd_reg)/
dd231c1d219f6b Ondrej Jirman 2024-04-23 616
be9e6229d67696 Tiberiu Breana 2015-04-27 617 ret = stk3310_regmap_init(data);
be9e6229d67696 Tiberiu Breana 2015-04-27 618 if (ret < 0)
be9e6229d67696 Tiberiu Breana 2015-04-27 619 return ret;
be9e6229d67696 Tiberiu Breana 2015-04-27 620
be9e6229d67696 Tiberiu Breana 2015-04-27 621 indio_dev->info = &stk3310_info;
be9e6229d67696 Tiberiu Breana 2015-04-27 622 indio_dev->name = STK3310_DRIVER_NAME;
be9e6229d67696 Tiberiu Breana 2015-04-27 623 indio_dev->modes = INDIO_DIRECT_MODE;
be9e6229d67696 Tiberiu Breana 2015-04-27 624 indio_dev->channels = stk3310_channels;
be9e6229d67696 Tiberiu Breana 2015-04-27 625 indio_dev->num_channels = ARRAY_SIZE(stk3310_channels);
be9e6229d67696 Tiberiu Breana 2015-04-27 626
dd231c1d219f6b Ondrej Jirman 2024-04-23 627 ret = regulator_enable(data->vdd_reg);
dd231c1d219f6b Ondrej Jirman 2024-04-23 628 if (ret)
dd231c1d219f6b Ondrej Jirman 2024-04-23 629 return dev_err_probe(&client->dev, ret,
dd231c1d219f6b Ondrej Jirman 2024-04-23 630 "regulator vdd enable failed\n");
dd231c1d219f6b Ondrej Jirman 2024-04-23 631
dd231c1d219f6b Ondrej Jirman 2024-04-23 632 /* we need a short delay to allow the chip time to power on */
dd231c1d219f6b Ondrej Jirman 2024-04-23 633 fsleep(1000);
dd231c1d219f6b Ondrej Jirman 2024-04-23 634
be9e6229d67696 Tiberiu Breana 2015-04-27 635 ret = stk3310_init(indio_dev);
be9e6229d67696 Tiberiu Breana 2015-04-27 636 if (ret < 0)
dd231c1d219f6b Ondrej Jirman 2024-04-23 637 goto err_vdd_disable;
be9e6229d67696 Tiberiu Breana 2015-04-27 638
6839c1b0700a79 Octavian Purdila 2015-09-23 639 if (client->irq > 0) {
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 640 ret = devm_request_threaded_irq(&client->dev, client->irq,
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 641 stk3310_irq_handler,
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 642 stk3310_irq_event_handler,
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 643 IRQF_TRIGGER_FALLING |
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 644 IRQF_ONESHOT,
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 645 STK3310_EVENT, indio_dev);
7c7a9eeaa335df Hartmut Knaack 2015-07-09 646 if (ret < 0) {
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 647 dev_err(&client->dev, "request irq %d failed\n",
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 648 client->irq);
7c7a9eeaa335df Hartmut Knaack 2015-07-09 649 goto err_standby;
7c7a9eeaa335df Hartmut Knaack 2015-07-09 650 }
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 651 }
3dd477acbdd1f1 Tiberiu Breana 2015-04-27 652
037e966f2d6389 Hartmut Knaack 2015-07-09 653 ret = iio_device_register(indio_dev);
037e966f2d6389 Hartmut Knaack 2015-07-09 654 if (ret < 0) {
037e966f2d6389 Hartmut Knaack 2015-07-09 655 dev_err(&client->dev, "device_register failed\n");
7c7a9eeaa335df Hartmut Knaack 2015-07-09 656 goto err_standby;
037e966f2d6389 Hartmut Knaack 2015-07-09 657 }
037e966f2d6389 Hartmut Knaack 2015-07-09 658
7c7a9eeaa335df Hartmut Knaack 2015-07-09 659 return 0;
7c7a9eeaa335df Hartmut Knaack 2015-07-09 660
7c7a9eeaa335df Hartmut Knaack 2015-07-09 661 err_standby:
7c7a9eeaa335df Hartmut Knaack 2015-07-09 662 stk3310_set_state(data, STK3310_STATE_STANDBY);
dd231c1d219f6b Ondrej Jirman 2024-04-23 663 err_vdd_disable:
dd231c1d219f6b Ondrej Jirman 2024-04-23 664 regulator_disable(data->vdd_reg);
be9e6229d67696 Tiberiu Breana 2015-04-27 665 return ret;
be9e6229d67696 Tiberiu Breana 2015-04-27 666 }
On Wed, 24 Apr 2024 02:16:06 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolution.org> wrote: > > > > From: Ondrej Jirman <megi@xff.cz> > > > > VDD power input can be used to completely power off the chip during > > system suspend. Do so if available. > > ... > > > ret = stk3310_init(indio_dev); > > if (ret < 0) > > - return ret; > > + goto err_vdd_disable; > > This is wrong. You will have the regulator being disabled _before_ > IRQ. Note, that the original code likely has a bug which sets states > before disabling IRQ and removing a handler. > > Side note, you may make the driver neater with help of > > struct device *dev = &client->dev; > > defined in this patch. > > ... > > > static int stk3310_suspend(struct device *dev) > > { > > struct stk3310_data *data; > > > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > > Side note: This may be updated (in a separate change) to use > dev_get_drvdata() directly. > > Jonathan, do we have something like iio_priv_from_drvdata(struct > device *dev)? Seems many drivers may utilise it. Not yet, but I'm not sure it's a good idea as there is no inherent reason to assume the drvdata is a struct iio_dev. It often is but adding a function that assumes that is a path to subtle bugs. Jonathan > > > } > > ... > > > static int stk3310_resume(struct device *dev) > > Ditto. > > -- > With Best Regards, > Andy Shevchenko
On Wed, 24 Apr 2024 18:20:41 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman <megi@xff.cz> wrote: > > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolution.org> wrote: > > ... > > > > > ret = stk3310_init(indio_dev); > > > > if (ret < 0) > > > > - return ret; > > > > + goto err_vdd_disable; > > > > > > This is wrong. You will have the regulator being disabled _before_ > > > IRQ. Note, that the original code likely has a bug which sets states > > > before disabling IRQ and removing a handler. > > > > How so? stk3310_init is called before enabling the interrupt. > > Exactly, IRQ is registered with devm and hence the error path and > remove stages will got it in a wrong order. > > > Original code has a bug that IRQ is enabled before registering the > > IIO device, > > Indeed, but this is another bug. It shouldn't be. A device that produces interrupts before we have told it to is a) buggy, b) almost certainly already had it's interrupt masked due to spurious interrupt detection. Definitely don't want to do it in the opposite order where userspace could turn the device on and have it start generating interrupts before the irq is registered. I'd rather assume non buggy hardware (and that if there are bugs, the normal protections kick in) than introduce a race into the software. > > > so if IRQ is triggered before registration, iio_push_event > > from IRQ handler may be called on a not yet registered IIO device. > > > > Never saw it happen, though. :) > > Because nobody cares enough to enable DEBUG_SHIRQ In most devices there is a status register and we should be doing nothing unless that is set. Interestingly this device either doesn't have one or the driver doesn't read it - it reads a flag only and so will always push an event. Such a register read doesn't require the IIO device registration to be complete. There are corner cases where that isn't true that need to manually mask at the host but they are rare. There is also a basic level of defense in iio_push_event() against that being called when the event interface is not registered. Jonathan >
On Wed, 24 Apr 2024 22:00:46 +0200 Ondřej Jirman <megi@xff.cz> wrote: > On Wed, Apr 24, 2024 at 08:31:27PM GMT, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 7:14 PM Ondřej Jirman <megi@xff.cz> wrote: > > > On Wed, Apr 24, 2024 at 06:20:41PM GMT, Andy Shevchenko wrote: > > > > On Wed, Apr 24, 2024 at 3:59 PM Ondřej Jirman <megi@xff.cz> wrote: > > > > > On Wed, Apr 24, 2024 at 02:16:06AM GMT, Andy Shevchenko wrote: > > > > > > On Wed, Apr 24, 2024 at 1:41 AM Aren Moynihan <aren@peacevolution.org> wrote: > > > > ... > > > > > > > > > ret = stk3310_init(indio_dev); > > > > > > > if (ret < 0) > > > > > > > - return ret; > > > > > > > + goto err_vdd_disable; > > > > > > > > > > > > This is wrong. You will have the regulator being disabled _before_ > > > > > > IRQ. Note, that the original code likely has a bug which sets states > > > > > > before disabling IRQ and removing a handler. > > > > > > > > > > How so? stk3310_init is called before enabling the interrupt. > > > > > > > > Exactly, IRQ is registered with devm and hence the error path and > > > > remove stages will got it in a wrong order. > > > > > > Makes no sense. > > > > Huh?! > > > > > IRQ is not enabled here, yet. So in error path, the code will > > > just disable the regulator and devm will unref it later on. IRQ doesn't enter > > > the picture here at all in the error path. > > > > Error path _after_ IRQ handler has been _successfully_ installed. > > And complete ->remove() stage. > > Allright. So fixing the other issue I mentioned will fix this one too, because > there will be no error path after IRQ enable, then. Don't do reorder stuff past iio_device_register. It generates a bunch of it's own issues wrt to functionality. The iio_device_register() call is the last one because the device must be in a state to correctly deal with all userspace actions by the time they are made available. Harden the driver to not call IIO core functions for false events if that is easy to do, but there shouldn't be an issue if you do (if there is we should address that in the IIO core). Jonathan > > kind regards, > o. > > > > > > Original code has a bug that IRQ is enabled before registering the > > > > > IIO device, > > > > > > > > Indeed, but this is another bug. > > > > > > > > > so if IRQ is triggered before registration, iio_push_event > > > > > from IRQ handler may be called on a not yet registered IIO device. > > > > > > > > > > Never saw it happen, though. :) > > > > > > > > Because nobody cares enough to enable DEBUG_SHIRQ. > > > > > > Nice debug tool. I bet it makes quite a mess when enabled. :) > > > > FWIW, I have had it enabled for ages, but I have only a few devices, > > so I fixed a few cases in the past WRT shared IRQ issues. > > > > -- > > With Best Regards, > > Andy Shevchenko
On Tue, 23 Apr 2024 18:33:05 -0400 Aren Moynihan <aren@peacevolution.org> wrote: > From: Ondrej Jirman <megi@xff.cz> > > VDD power input can be used to completely power off the chip during > system suspend. Do so if available. > > Signed-off-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Aren Moynihan <aren@peacevolution.org> Suggestions inline. Key thing is take the whole thing devm_ managed and your life gets much easier. It is mixing the two approaches that causes problems and often the best plan is to do everything in probe/remove with devm_ calls to do the cleanup for you. > --- > > Notes: > Changes in v2: > - always enable / disable regulators and rely on a dummy regulator if > one isn't specified > - replace usleep_range with fsleep > - reorder includes so iio headers are last > - add missing error handling to resume > > drivers/iio/light/stk3310.c | 49 ++++++++++++++++++++++++++++++++++--- > 1 file changed, 46 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c > index 7b71ad71d78d..a0547eeca3e3 100644 > --- a/drivers/iio/light/stk3310.c > +++ b/drivers/iio/light/stk3310.c > @@ -13,6 +13,8 @@ > #include <linux/module.h> > #include <linux/mod_devicetable.h> > #include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > + > #include <linux/iio/events.h> > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > @@ -117,6 +119,7 @@ struct stk3310_data { > struct regmap_field *reg_int_ps; > struct regmap_field *reg_flag_psint; > struct regmap_field *reg_flag_nf; > + struct regulator *vdd_reg; > }; > > static const struct iio_event_spec stk3310_events[] = { > @@ -607,6 +610,10 @@ static int stk3310_probe(struct i2c_client *client) > > mutex_init(&data->lock); > > + data->vdd_reg = devm_regulator_get(&client->dev, "vdd"); > + if (IS_ERR(data->vdd_reg)) > + return dev_err_probe(&client->dev, ret, "get regulator vdd failed\n"); > + > ret = stk3310_regmap_init(data); > if (ret < 0) > return ret; > @@ -617,9 +624,17 @@ static int stk3310_probe(struct i2c_client *client) > indio_dev->channels = stk3310_channels; > indio_dev->num_channels = ARRAY_SIZE(stk3310_channels); > > + ret = regulator_enable(data->vdd_reg); > + if (ret) > + return dev_err_probe(&client->dev, ret, > + "regulator vdd enable failed\n"); > + > + /* we need a short delay to allow the chip time to power on */ > + fsleep(1000); > + > ret = stk3310_init(indio_dev); > if (ret < 0) > - return ret; > + goto err_vdd_disable; > > if (client->irq > 0) { > ret = devm_request_threaded_irq(&client->dev, client->irq, > @@ -645,32 +660,60 @@ static int stk3310_probe(struct i2c_client *client) > > err_standby: > stk3310_set_state(data, STK3310_STATE_STANDBY); Move this handling to a devm_add_action_or_reset() callback in a precursor patch. That will fix the current ordering issue wrt to the irq registration. Then use devm_iio_device_register() in that precursor patch. > +err_vdd_disable: > + regulator_disable(data->vdd_reg); Add a devm_add_action_or_reset() callback to disable this regulator in this patch. Register that just after the enable. That way the ordering will be maintained for all calls. > return ret; > } > > static void stk3310_remove(struct i2c_client *client) > { > struct iio_dev *indio_dev = i2c_get_clientdata(client); > + struct stk3310_data *data = iio_priv(indio_dev); > > iio_device_unregister(indio_dev); > stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); > + regulator_disable(data->vdd_reg); With above suggested changes, you can drop the remove function entirely. > } > > static int stk3310_suspend(struct device *dev) > { > struct stk3310_data *data; > + int ret; > > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > > - return stk3310_set_state(data, STK3310_STATE_STANDBY); > + ret = stk3310_set_state(data, STK3310_STATE_STANDBY); > + if (ret) > + return ret; > + > + regcache_mark_dirty(data->regmap); > + regulator_disable(data->vdd_reg); > + > + return 0; > } > > static int stk3310_resume(struct device *dev) > { > - u8 state = 0; > struct stk3310_data *data; > + u8 state = 0; > + int ret; > > data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); > + > + ret = regulator_enable(data->vdd_reg); > + if (ret) { > + dev_err(dev, "Failed to re-enable regulator vdd\n"); > + return ret; > + } > + > + fsleep(1000); > + > + ret = regcache_sync(data->regmap); > + if (ret) { > + dev_err(dev, "Failed to restore registers: %d\n", ret); > + return ret; > + } > + > if (data->ps_enabled) > state |= STK3310_STATE_EN_PS; > if (data->als_enabled)
On Sun, Apr 28, 2024 at 05:53:37PM GMT, Jonathan Cameron wrote: > On Tue, 23 Apr 2024 18:33:05 -0400 > Aren Moynihan <aren@peacevolution.org> wrote: > > > From: Ondrej Jirman <megi@xff.cz> > > > > VDD power input can be used to completely power off the chip during > > system suspend. Do so if available. > > > > Signed-off-by: Ondrej Jirman <megi@xff.cz> > > Signed-off-by: Aren Moynihan <aren@peacevolution.org> > > Suggestions inline. Key thing is take the whole thing devm_ managed > and your life gets much easier. It is mixing the two approaches that > causes problems and often the best plan is to do everything in probe/remove > with devm_ calls to do the cleanup for you. Thank you for writing this up. I've been a bit distracted lately, but I'm hoping to find some time to implement this in a new revision soon. - Aren
diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c index 7b71ad71d78d..a0547eeca3e3 100644 --- a/drivers/iio/light/stk3310.c +++ b/drivers/iio/light/stk3310.c @@ -13,6 +13,8 @@ #include <linux/module.h> #include <linux/mod_devicetable.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> + #include <linux/iio/events.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> @@ -117,6 +119,7 @@ struct stk3310_data { struct regmap_field *reg_int_ps; struct regmap_field *reg_flag_psint; struct regmap_field *reg_flag_nf; + struct regulator *vdd_reg; }; static const struct iio_event_spec stk3310_events[] = { @@ -607,6 +610,10 @@ static int stk3310_probe(struct i2c_client *client) mutex_init(&data->lock); + data->vdd_reg = devm_regulator_get(&client->dev, "vdd"); + if (IS_ERR(data->vdd_reg)) + return dev_err_probe(&client->dev, ret, "get regulator vdd failed\n"); + ret = stk3310_regmap_init(data); if (ret < 0) return ret; @@ -617,9 +624,17 @@ static int stk3310_probe(struct i2c_client *client) indio_dev->channels = stk3310_channels; indio_dev->num_channels = ARRAY_SIZE(stk3310_channels); + ret = regulator_enable(data->vdd_reg); + if (ret) + return dev_err_probe(&client->dev, ret, + "regulator vdd enable failed\n"); + + /* we need a short delay to allow the chip time to power on */ + fsleep(1000); + ret = stk3310_init(indio_dev); if (ret < 0) - return ret; + goto err_vdd_disable; if (client->irq > 0) { ret = devm_request_threaded_irq(&client->dev, client->irq, @@ -645,32 +660,60 @@ static int stk3310_probe(struct i2c_client *client) err_standby: stk3310_set_state(data, STK3310_STATE_STANDBY); +err_vdd_disable: + regulator_disable(data->vdd_reg); return ret; } static void stk3310_remove(struct i2c_client *client) { struct iio_dev *indio_dev = i2c_get_clientdata(client); + struct stk3310_data *data = iio_priv(indio_dev); iio_device_unregister(indio_dev); stk3310_set_state(iio_priv(indio_dev), STK3310_STATE_STANDBY); + regulator_disable(data->vdd_reg); } static int stk3310_suspend(struct device *dev) { struct stk3310_data *data; + int ret; data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); - return stk3310_set_state(data, STK3310_STATE_STANDBY); + ret = stk3310_set_state(data, STK3310_STATE_STANDBY); + if (ret) + return ret; + + regcache_mark_dirty(data->regmap); + regulator_disable(data->vdd_reg); + + return 0; } static int stk3310_resume(struct device *dev) { - u8 state = 0; struct stk3310_data *data; + u8 state = 0; + int ret; data = iio_priv(i2c_get_clientdata(to_i2c_client(dev))); + + ret = regulator_enable(data->vdd_reg); + if (ret) { + dev_err(dev, "Failed to re-enable regulator vdd\n"); + return ret; + } + + fsleep(1000); + + ret = regcache_sync(data->regmap); + if (ret) { + dev_err(dev, "Failed to restore registers: %d\n", ret); + return ret; + } + if (data->ps_enabled) state |= STK3310_STATE_EN_PS; if (data->als_enabled)