| 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: <20250331122609.508bc3e2@jic23-huawei>
Date: Mon, 31 Mar 2025 12:26:09 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
Cc: Jean-Baptiste Maneyrol via B4 Relay
<devnull+jean-baptiste.maneyrol.tdk.com@...nel.org>, Lars-Peter Clausen
<lars@...afoo.de>, "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality
for Wake-on-Motion
On Wed, 19 Mar 2025 21:06:41 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com> wrote:
> Hello Jonathan,
>
> I understand the issue, but using spi->irq for getting interrupt is something we are already doing inside the driver. I agree we need to fix that, but I would prefer fixing it after adding WoM support.
>
> Is that OK for you? Or do I need to fix getting the right interrupt first?
It's a pretty trivial fix to add the sanity check,
but I don't mind if you do it as a follow up.
Jonathan
>
> Thanks,
> JB
>
> ________________________________________
> From: Jonathan Cameron <jic23@...nel.org>
> Sent: Saturday, March 15, 2025 19:17
> To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>
> Cc: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@...nel.org>; Lars-Peter Clausen <lars@...afoo.de>; linux-iio@...r.kernel.org <linux-iio@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
> Subject: Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion
>
> This Message Is From an External Sender
> This message came from outside your organization.
>
> On Tue, 11 Mar 2025 15:31:44 +0000
> Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com> wrote:
>
> > Hello Jonathan,
> >
> > still sorry for not being able to reply in-line.
> >
> > No problem for all changes, except handling the 2 interrupt lines. Currently our driver only supports INT1 and cannot work with INT2, and this is not related to Wake-on-Motion feature. This is something we could add in another series, and I prefer to have a dedicated series for that.
>
> You should check it isn't INT2 that you are getting via spi->irq etc.
> Absolutely fine to reject that in the driver but you need to be
> sure you have what you think you do and the spi->irq etc don't
> provide that surety - they just give you the first one in the
> dt-binding.
>
> Jonathan
>
>
> >
> > For the mutex rework, I will delete them. This is something we can also do in another dedicated patch not inside this series.
> >
> > Is that OK for you?
> >
> > Thanks,
> > JB
> >
> > ________________________________________
> > From: Jonathan Cameron <jic23@...nel.org>
> > Sent: Saturday, February 22, 2025 17:22
> > To: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@...nel.org>
> > Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@....com>; Lars-Peter Clausen <lars@...afoo.de>; linux-iio@...r.kernel.org <linux-iio@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>
> > Subject: Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion
> >
> > This Message Is From an External Sender
> > This message came from outside your organization.
> >
> > On Thu, 20 Feb 2025 21:52:07 +0100
> > Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@...nel.org> wrote:
> >
> > > From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
> > >
> > > When Wake-on-Motion is on, enable system wakeup and keep chip on for
> > > waking up system with interrupt.
> > >
> > > Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
> > Hi Jean-Baptiste,
> >
> > A few comments inline.
> >
> > > ---
> > > drivers/iio/imu/inv_icm42600/inv_icm42600.h | 2 +
> > > drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c | 3 +
> > > drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 89 +++++++++++++++--------
> > > 3 files changed, 63 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > > index 8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465..baf1dcd714800e84ccd21dc1d1e486849c77a9ae 100644
> > > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > > @@ -151,6 +151,7 @@ struct inv_icm42600_apex {
> > > * @map: regmap pointer.
> > > * @vdd_supply: VDD voltage regulator for the chip.
> > > * @vddio_supply: I/O voltage regulator for the chip.
> > > + * @irq: chip irq.
> >
> > Maybe say a little on what the variable is used for. It's not otherwise
> > obvious why we need it. Also, does this chip really only have one irq line?
> > Looks like there are two. So the drivers should be fixed to cope with the
> > only one wired being irq2 unless I've found the wrong datasheet which is
> > certainly possible.
> >
> >
> > > * @orientation: sensor chip orientation relative to main hardware.
> > > * @conf: chip sensors configurations.
> > > * @suspended: suspended sensors configuration.
> > > @@ -168,6 +169,7 @@ struct inv_icm42600_state {
> > > struct regmap *map;
> > > struct regulator *vdd_supply;
> > > struct regulator *vddio_supply;
> > > + int irq;
> > > struct iio_mount_matrix orientation;
> > > struct inv_icm42600_conf conf;
> > > struct inv_icm42600_suspended suspended;
> > > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > > index 8ce2276b3edc61cc1ea26810198dd0057054ec48..4240e8c576f4d07af5434e9a91dfda532f87ffb9 100644
> > > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > > @@ -1149,6 +1149,9 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
> > > if (ret)
> > > return ERR_PTR(ret);
> > >
> > > + /* accel events are wakeup capable */
> > > + device_set_wakeup_capable(&indio_dev->dev, true);
> > > +
> > > return indio_dev;
> > > }
> > >
> > > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > > index c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2..f94bda5dc094d6cc85e3facbd480b830bfbaa3f9 100644
> > > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > > @@ -751,6 +751,7 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
> > > mutex_init(&st->lock);
> > > st->chip = chip;
> > > st->map = regmap;
> > > + st->irq = irq;
> > >
> > > ret = iio_read_mount_matrix(dev, &st->orientation);
> > > if (ret) {
> > > @@ -829,44 +830,56 @@ EXPORT_SYMBOL_NS_GPL(inv_icm42600_core_probe, "IIO_ICM42600");
> > > static int inv_icm42600_suspend(struct device *dev)
> > > {
> > > struct inv_icm42600_state *st = dev_get_drvdata(dev);
> > > + struct device *accel_dev;
> > > + bool wakeup;
> > > + int accel_conf;
> > > int ret;
> > >
> > > - mutex_lock(&st->lock);
> > > + guard(mutex)(&st->lock);
> >
> > As below. Pull these guard changes out as a precursor patch. That will make
> > it easier to spot the real changes here.
> >
> > >
> > > st->suspended.gyro = st->conf.gyro.mode;
> > > st->suspended.accel = st->conf.accel.mode;
> > > st->suspended.temp = st->conf.temp_en;
> > > - if (pm_runtime_suspended(dev)) {
> > > - ret = 0;
> > > - goto out_unlock;
> > > - }
> > > + if (pm_runtime_suspended(dev))
> > > + return 0;
> > >
> > > /* disable FIFO data streaming */
> > > if (st->fifo.on) {
> > > ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> > > INV_ICM42600_FIFO_CONFIG_BYPASS);
> > > if (ret)
> > > - goto out_unlock;
> > > + return ret;
> > > }
> > >
> > > - /* disable APEX features */
> > > - if (st->apex.wom.enable) {
> > > - ret = inv_icm42600_set_wom(st, false);
> > > - if (ret)
> > > - goto out_unlock;
> > > + /* keep chip on and wake-up capable if APEX and wakeup on */
> > > + accel_dev = &st->indio_accel->dev;
> > > + wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> > > +
> > > + if (!wakeup) {
> > > + /* disable APEX features and accel if wakeup disabled */
> > > + if (st->apex.wom.enable) {
> > > + ret = inv_icm42600_set_wom(st, false);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > + accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
> > > + } else {
> > > + /* keep accel on and setup irq for wakeup */
> > > + accel_conf = st->conf.accel.mode;
> > > + enable_irq_wake(st->irq);
> > > + disable_irq(st->irq);
> > > }
> > >
> > > ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> > > - INV_ICM42600_SENSOR_MODE_OFF, false,
> > > - NULL);
> > > + accel_conf, false, NULL);
> > > if (ret)
> > > - goto out_unlock;
> > > + return ret;
> > >
> > > - regulator_disable(st->vddio_supply);
> > > + /* disable vddio regulator if chip is sleeping */
> > > + if (!wakeup)
> > > + regulator_disable(st->vddio_supply);
> > >
> > > -out_unlock:
> > > - mutex_unlock(&st->lock);
> > > - return ret;
> > > + return 0;
> > > }
> > >
> > > /*
> > > @@ -878,13 +891,25 @@ static int inv_icm42600_resume(struct device *dev)
> > > struct inv_icm42600_state *st = dev_get_drvdata(dev);
> > > struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
> > > struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
> > > + struct device *accel_dev;
> > > + bool wakeup;
> > > int ret;
> > >
> > > - mutex_lock(&st->lock);
> > > + guard(mutex)(&st->lock);
> >
> > Good change. But separate patch as not related to most of what is going on here.
> >
> >
> > >
> > > - ret = inv_icm42600_enable_regulator_vddio(st);
> > > - if (ret)
> > > - goto out_unlock;
> > > + /* check wakeup capability */
> > > + accel_dev = &st->indio_accel->dev;
> > > + wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> > > +
> > > + /* restore vddio if cut off or irq state */
> > > + if (!wakeup) {
> > > + ret = inv_icm42600_enable_regulator_vddio(st);
> > > + if (ret)
> > > + return ret;
> > > + } else {
> > > + enable_irq(st->irq);
> > > + disable_irq_wake(st->irq);
> > > + }
> > >
> > > pm_runtime_disable(dev);
> > > pm_runtime_set_active(dev);
> > > @@ -895,13 +920,15 @@ static int inv_icm42600_resume(struct device *dev)
> > > st->suspended.accel,
> > > st->suspended.temp, NULL);
> > > if (ret)
> > > - goto out_unlock;
> > > + return ret;
> > >
> > > - /* restore APEX features */
> > > - if (st->apex.wom.enable) {
> > > - ret = inv_icm42600_set_wom(st, true);
> > > - if (ret)
> > > - goto out_unlock;
> > > + /* restore APEX features if disabled */
> > > + if (!wakeup) {
> > > + if (st->apex.wom.enable) {
> > > + ret = inv_icm42600_set_wom(st, true);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > }
> > >
> > > /* restore FIFO data streaming */
> > > @@ -910,11 +937,11 @@ static int inv_icm42600_resume(struct device *dev)
> > > inv_sensors_timestamp_reset(&accel_st->ts);
> > > ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> > > INV_ICM42600_FIFO_CONFIG_STREAM);
> > > + if (ret)
> > > + return ret;
> > > }
> > >
> > > -out_unlock:
> > > - mutex_unlock(&st->lock);
> > > - return ret;
> > > + return 0;
> > > }
> > >
> > > /* Runtime suspend will turn off sensors that are enabled by iio devices. */
> > >
> >
>
Powered by blists - more mailing lists