2009-09-01 13:51:36

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH 1/2] AB3100 regulator support v2

On Mon, Aug 31, 2009 at 04:16:15PM +0200, Linus Walleij wrote:
> That said, I think the regulator paths are entirely in-kernel and
> under such circumstances that signals from userspace are blocked
> anyway.

Only if you explicitly block signals will pending signals be ignored
by the foo_interruptible() functions.


2009-09-01 22:05:11

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 1/2] AB3100 regulator support v2

2009/9/1 Russell King - ARM Linux <[email protected]>:
> On Mon, Aug 31, 2009 at 04:16:15PM +0200, Linus Walleij wrote:
>> That said, I think the regulator paths are entirely in-kernel and
>> under such circumstances that signals from userspace are blocked
>> anyway.
>
> Only if you explicitly block signals will pending signals be ignored
> by the foo_interruptible() functions.

Yep and we have some code like that currently. I'm currently
contemplating refactoring it the chain down through ab3100-core
and the i2c driver here to make sure no _interruptible suffixed
operations are used anywhere.

On U300 that is, since we have our own I2C driver we can
rid it there.

But generally speaking the AB3100 can be used on top of any
i2c adapter and a bunch of them actually use
wait_for_completion_interruptible_timeout();
and wait_event_interruptible_timeout(); for example.

One of them being i2c/buses/i2c-imx.c actually, Mark does
this mean you actually have the risk of being kill -9:ed in the
middle of a regulator operation for the WM drivers, for
example

In the general sense perhaps this doesn't happen so much,
what we've seen is system shut down, here some code get
interrupted by signals, so atleast the shut down path should be
protected I guess, I will do that in the U300 board setup
pm_poweroff() hook then, which calls down the
regulator/ab3100/i2c chain so all that is secured.

Yours,
Linus Walleij

2009-09-01 22:10:03

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 1/2] AB3100 regulator support v2

2009/9/2 Linus Walleij <[email protected]>:

[SIGKILL]
> In the general sense perhaps this doesn't happen so much,
> what we've seen is system shut down, here some code get
> interrupted by signals, so atleast the shut down path should be
> protected I guess, I will do that in the U300 board setup
> pm_poweroff() hook then, which calls down the
> regulator/ab3100/i2c chain so all that is secured.

Or rather, I already do (I forgot about adding in that code!)

+ */
+void u300_pm_poweroff(void)
+{
+ sigset_t old, all;
+
+ sigfillset(&all);
+ if (!sigprocmask(SIG_BLOCK, &all, &old)) {
+ /* Disable LDO D to shut down the system */
+ if (main_power_15)
+ regulator_disable(main_power_15);
+ else
+ pr_err("regulator not available to shut down system\n");
+ (void) sigprocmask(SIG_SETMASK, &old, NULL);
+ }
+ return;
+}

Linus Walleij

2009-09-01 22:11:02

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 1/2] AB3100 regulator support v2

On Wed, Sep 02, 2009 at 12:05:10AM +0200, Linus Walleij wrote:

> But generally speaking the AB3100 can be used on top of any
> i2c adapter and a bunch of them actually use
> wait_for_completion_interruptible_timeout();
> and wait_event_interruptible_timeout(); for example.

There was some discussion of this on the I2C list and a decision that it
was sensible to move away from doing that since it was being found to
cause more problems than it solved - the FSL driver is the one I
remember actually doing so.

> One of them being i2c/buses/i2c-imx.c actually, Mark does
> this mean you actually have the risk of being kill -9:ed in the
> middle of a regulator operation for the WM drivers, for
> example

Yes. Pretty much any embedded I2C driver could cause issues for the
Wolfson drivers - there's a whole bunch of audio CODECs that are very
widely deployed.

> In the general sense perhaps this doesn't happen so much,

Yes, it's fairly rare which is the main reason it's not being chased
after particularly actively.