Hi,

On 1/22/21 2:04 PM, Charles Keepax wrote:
> On Fri, Jan 22, 2021 at 01:23:44PM +0100, Hans de Goede wrote:
>> On 1/22/21 12:26 PM, Charles Keepax wrote:
>>> On Thu, Jan 21, 2021 at 05:55:00PM +0100, Hans de Goede wrote:
>>>> On 1/19/21 10:51 AM, Richard Fitzgerald wrote:
>>>>> On 18/01/2021 17:24, Andy Shevchenko wrote:
>>>>>> On Sun, Jan 17, 2021 at 6:06 PM Hans de Goede <[email protected]> wrote:
>>>> Note there is a pretty big issue with the original code here, if
>>>> the MICVDD DAPM pin is on for an internal-mic and then we run through the
>>>> jack-detect mic-detect sequence, we end up setting
>>>> bypass=true causing the micbias for the internal-mic to no longer
>>>> be what was configured. IOW poking the bypass setting underneath the
>>>> DAPM code is racy.
>>>>
>>>
>>> The regulator bypass code keeps an internal reference count. All
>>> the users of the regulator need to allow bypass for it to be
>>> placed into bypass mode, so I believe this can't happen.
>>
>> Ah I did not know that, since the regulator_allow_bypass function
>> takes a bool rather then having enable/disable variants I thought
>> it would directly set the bypass, but you are right. So this is not
>> a problem, good.
>>
>> So this has made me look at the problem again and I believe that
>> a much better solution is to simply re-use the MICVDD regulator-reference
>> which has been regulator_get-ed by the dapm code when instantiating the:
>>
>> SND_SOC_DAPM_REGULATOR_SUPPLY("MICVDD", 0, SND_SOC_DAPM_REGULATOR_BYPASS),
>>
>> widget. So I plan to have a new patch in v3 of the series which replaces
>> the devm_regulator_get with something like this:
>>
>> /*
>> * There is a DAPM widget for the MICVDD regulator, since
>> * the button-press detection has special requirements wrt
>> * the regulator bypass settings we cannot directly
>> * use snd_soc_component_force_enable_pin("MICVDD") /
>> * snd_soc_component_disable_pin("MICVDD").
>> *
>> * Instead we lookup the widget's regulator reference here
>> * and use that to directly control the regulator.
>> * Both the regulator's enable and bypass settings are
>> * ref-counted so this will not interfere with the DAPM use
>> * of the regulator.
>> */
>> for_each_card_widgets(dapm->card, w) {
>> if (!strcmp(w->name, "MICVDD"))
>> info->micvdd_regulator = w->regulator;
>> break;
>> }
>> }
>>
>> (note I've not tested this yet, but I expect this to work fine).
>>
>

<note replying in a singe email to 2 strongly related
replies from Charles on this>

> Alas this won't work either. When I say reference count that
> isn't quite a totally accurate reflection of the usage of the
> function. When you call allow_bypass you are saying as this
> consumer of the regulator I don't mind if it goes into bypass.
> Then if all consumers agree the regulator will be put into
> bypass. So it is comparing the reference count to the number of
> consumers the regulator has to make a decision.
>
> If you call allow_bypass independently from the jack detection
> code and the ASoC framework on the same consumer, as you
> describe here you will get bad effects. For example the
> regulator has two consumers, our CODEC driver and some other
> device. If our codec driver calls allow_bypass twice, then
> the regulator would go into bypass without the other consumer
> having approved it would could be fatal to that device.

So I just double checked the regulator core code and you
are right that the bypass thing is per consumer. So we
will indeed need 2 calls to regulator_get, one for the
dapm use and one for the jack-det use since those 2
are independent.

Note your example does not work as you think it will though:

int regulator_allow_bypass(struct regulator *regulator, bool enable)
{
...

if (enable && !regulator->bypass) {
rdev->bypass_count++;
...

} else if (!enable && regulator->bypass) {
rdev->bypass_count--;
...
}

if (ret == 0)
regulator->bypass = enable;
}

So a second call to allow_bypass(..., true) from the same
consumer will be a no-op.

Sharing the same struct regulator result between the dapm widget
and the jack-det code would still be an issue though since it
will introduce the race which I was worried about earlier.

>> 1. Keep the code as is, live with the debugfs error. This might be
>> best for now, as I don't want to grow the scope of this series too much.
>> I will go with this for the next version of this series (unless
>> I receive feedback otherwise before I get around to posting the next
>> version).
>
> I wonder if this commit was related to that:
>
> commit ff268b56ce8c ("regulator: core: Don't spew backtraces on duplicate sysfs")
>
> Apologies I don't have as much time as I normally would to look
> into such issues at the moment, due to various internal company
> things going on.

Actually you are being super helpful, thank you. I believe that
with your latest email this is fully resolved.

> I do suspect that this option is the way to go though and if
> there are issues of duplicates being created by the regulator
> core those probably need to be resolved in there. But that can
> probably be done separate from this series.

Good catch, thanks. This means that having multiple consumers /
regulator_get calls from the same consumer-dev is supposed to work
and the debugfs error needs to be silenced somehow. I will look
into silencing the error (as a patch separate from this series).

Regards,

Hans