On Fri, Feb 16, 2024 at 03:39:25PM +0000, Jonathan Cameron wrote:
> On Wed, 14 Feb 2024 18:43:10 +0100
> Ondřej Jirman <megi@xxxxxx> wrote:
>
> > Hi Jonathan,
>
> Gah. Runtime pm always gives me a headache. I'd indeed misunderstood some
> of what you are doing.
> >
> > [...]
> >
> > I did, it will not work as you suggest. It's implemented as for loop with
> > condition, and the compiler will complain about fallthrough.
> >
> > I can do:
> >
> > scoped_guard(mutex, &data->mutex)
> > return af8133j_set_scale(data, val, val2);
> > return 0;
> >
> > but it looks weirder at first glance, at least to my eye.
>
> I agree that bit is less than ideal, but with your code it should also
> get confused about whether ret is ever set.
>
> scoped_guard(mutex, &data->mutex)
> return ...
> unreachable();
>
> perhaps? or just use a guard and add scope manually
>
> case IIO_CHAN_INFO_SCALE: {
> guard(mutex)(&data->mutex);
>
> return af8133j_set_scale(...);
> }
>
> I'd go with this as the cleanest solution in this case.
Yes, that looks much nicer. Thanks. :)
Though in the end I'll go with pushing the locking down to actual register
access in the af8133j_set_scale() function, so that I don't lock around
RPM disable/enable for no reason.
>
> >
> > > > + return ret;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > >
> > > > +static void af8133j_power_down_action(void *ptr)
> > > > +{
> > > > + struct af8133j_data *data = ptr;
> > > > + struct device *dev = &data->client->dev;
> > > > +
> > > > + pm_runtime_disable(dev);
> > > You group together unwinding of calls that occur in very
> > > different places in probe. Don't do that as it leas
> > > to disabling runtime pm having never enabled it
> > > in some error paths. That may be safe but if fails the
> > > obviously correct test.
> >
> > This whole disable/enable dance is here so that pm_runtime_status_suspended can
> > be trusted. Not for disabling PM during device remove or in error paths.
> >
> > There's no imbalance here or problem with disabling PM when it's already
> > disabled. Disable/enable is reference counted, and this function keeps the
> > balance.
>
> Whilst not buggy but I still want to be able to cleanly associate a given
> bit of cleanup with what is being cleaned up. That is the path to
> maintainable code longer term. Runtime PM does make a mess of doing this
> but tends to have somewhat logical sets of calls that go together.
>
> As long as we hold a reference, doesn't matter when we turn it on in probe()
> Only the put_autosuspend has to come after we done talking to it.
>
> >
> > > So this is a good solution to the normal dance of turning power on
> > > just to turn it off shortly afterwards.
> > >
> > > > + af8133j_power_down(data);
> > > > + pm_runtime_enable(dev);
> > > Why?
> >
> > See above. To keep the disable ref count balanced.
> >
> > Looks like actual RPM disable already happened at this point a bit earlier in
> > another callback registered via devm_pm_runtime_enable(). I guess this
> > pm_runtime_enable()/pm_runtime_disable() guard can just be skipped, because RPM
> > is already disabled thanks to reverse ordering of devm callbacks during device
> > remove. So while this is safe, it's redundant at this point and call to
> > pm_runtime_status_suspended() is safe without this.
>
> Yes, That's a side effect of only enabling it right at the end.
>
> >
> > > > +}
> > > > +
> > > > +static int af8133j_probe(struct i2c_client *client)
> > > > +{
> > > > + struct device *dev = &client->dev;
> > > > + struct af8133j_data *data;
> > > > + struct iio_dev *indio_dev;
> > > > + struct regmap *regmap;
> > > > + int ret, i;
> > > > +
> > > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > > > + if (!indio_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + regmap = devm_regmap_init_i2c(client, &af8133j_regmap_config);
> > > > + if (IS_ERR(regmap))
> > > > + return dev_err_probe(dev, PTR_ERR(regmap),
> > > > + "regmap initialization failed\n");
> > > > +
> > > > + data = iio_priv(indio_dev);
> > > > + i2c_set_clientdata(client, indio_dev);
> > > > + data->client = client;
> > > > + data->regmap = regmap;
> > > > + data->range = AF8133J_REG_RANGE_12G;
> > > > + mutex_init(&data->mutex);
> > > > +
> > > > + data->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > > + if (IS_ERR(data->reset_gpiod))
> > > > + return dev_err_probe(dev, PTR_ERR(data->reset_gpiod),
> > > > + "Failed to get reset gpio\n");
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(af8133j_supply_names); i++)
> > > > + data->supplies[i].supply = af8133j_supply_names[i];
> > > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
> > > > + data->supplies);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = iio_read_mount_matrix(dev, &data->orientation);
> > > > + if (ret)
> > > > + return dev_err_probe(dev, ret, "Failed to read mount matrix\n");
> > > > +
> > > > + ret = af8133j_power_up(data);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + pm_runtime_set_active(dev);
> > > > +
> > > > + ret = devm_add_action_or_reset(dev, af8133j_power_down_action, data);
> > >
> > > As mentioned above, this should only undo things done before this point.
> > > So just the af8133j_power_down() I think.
> >
> > The callback doesn't do anything else but power down. It leaves everything
> > else as is after it exits.
> >
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = af8133j_product_check(data);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + indio_dev->info = &af8133j_info;
> > > > + indio_dev->name = "af8133j";
> > > > + indio_dev->channels = af8133j_channels;
> > > > + indio_dev->num_channels = ARRAY_SIZE(af8133j_channels);
> > > > + indio_dev->modes = INDIO_DIRECT_MODE;
> > > > +
> > > > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > > + &af8133j_trigger_handler, NULL);
> > > > + if (ret < 0)
> > > > + return dev_err_probe(&client->dev, ret,
> > > > + "Failed to setup iio triggered buffer\n");
> > > > +
> > > > + ret = devm_iio_device_register(dev, indio_dev);
> > > > + if (ret)
> > > > + return dev_err_probe(dev, ret, "Failed to register iio device");
> > > > +
> > > > + pm_runtime_get_noresume(dev);
> > >
> > > > + pm_runtime_use_autosuspend(dev);
> > > > + pm_runtime_set_autosuspend_delay(dev, 500);
> > > > + ret = devm_pm_runtime_enable(dev);
> > >
> > > This already deals with pm_runtime_disable() so you shouldn't need do it manually.
> >
> > I'm not disabling RPM manually, it was just used as temporary guard to be able
> > to read pm_runtime_status_suspended() safely.
>
> I'd indeed misunderstood that. I forgot the oddity that runtime pm is effectively
> reference counting in only one direction for enable / disable and the opposite
> one for get and put.
>
> pm_runtime_disable()
> pm_runtime_disable()
> pm_runtime_enable()
> pm_runtime_enable()
> pm_runtime_enable()
> is fine, but
>
> pm_runtime_enable()
> pm_runtime_enable()
> pm_runtime_disable()
> pm_runtime_disable()
> pm_runtime_enable()
> is not.
>
> Which makes sense when you realise it's all about ensuring it is off, but
> never ensuring that it is turned on.
Yeah. Enabling already enabled RPM is thankfully easier to catch though, due to
a kernel warning:
https://elixir.bootlin.com/linux/latest/source/drivers/base/power/runtime.c#L1494
Unbalanced disable is annoying though. Not sure why, but disable_depth even
persists device unbind, so next rebinding will leave RPM disabled after probe.
That is when doing this after intentionally make the driver disable RPM twice
in remove callback:
echo 4-001c > /sys/bus/i2c/drivers/af8133j/unbind
echo 4-001c > /sys/bus/i2c/drivers/af8133j/bind
(driver remove/probe gets called)
Maybe it's due to RPM dependencies on parent device. Dunno. But it's a silent
problem in this case.
regards,
o.
>
>
>
> >
> > > Also you want to enable that before the devm_iio_device_register() to avoid
> > > problems with it not being available as the userspace interfaces are used.
> > >
> > > So just move this up a few lines.
> >
> > Good idea, thanks.
> >
> > kind regards,
> > o.
> >
> > >
> > >
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + pm_runtime_put_autosuspend(dev);
> > > > +
> > > > + return 0;
> > > > +}
> > >
>
