On 2018-11-01 12:04, Fabrizio Castro wrote:
> Hello Peter,
>
> Thank you for your feedback!
>
> snip
>
>>> Yeah, there is some issue with locking here, and that's coming down
>>> from the fact that when we call into drm_get_edid, we implicitly call
>>> i2c_transfer which ultimately locks the i2c adapter, and then calls
>>> into our sii902x_i2c_bypass_select, which in turn calls into the regmap
>>> functions and therefore we try and lock the same i2c adapter again,
>>> driving the system into a deadlock.
>>> Having the option of using "unlocked" flavours of reads and writes
>>> is what we need here, but looking at drivers/base/regmap/regmap-i2c.c
>>> I couldn't find anything suitable for my case, maybe Mark could advise
>>> on this one? I am sure I overlooked something here, is there a better
>>> way to address this?
>>
>> This recursive locking problem surfaces when an i2c mux is operated
>> by writing commands on the same i2c bus that is muxed. When this
>> happens for a typical i2c mux chip like nxp,pca9548 this can be kept
>> local to that driver. However, when there are interactions with other
>> parts of the kernel (e.g. if the i2c-mux is operated by gpio pins,
>> and those gpio pins in turn are operated by accesses to the i2c bus
>> that is muxed) this locked vs. unlocked thing would spread like
>> wildfire.
>>
>> Since I did not like that wildfire, I came up with the "mux-locked"
>> scheme. It is not appropriate everywhere, but in this case I think it
>> is a perfect fit. By using it, you should be able to dodge all locking
>> issues and it should be possible to keep the regmap usage as-is and the
>> patch would in all likelihood be much less intrusive.
>>
>> For some background on "mux-locked" vs. "parent-locked" (which is what
>> you have used in this RFC patch), see Documentation/i2c/i2c-topology.
>
> The "mux-locked" implementation was the one I first tried and I discovered
> it doesn't work for me, as other traffic on the parent adapter can get in the
> way. What we need for this device is no other traffic on the bus during the
> "select transaction deselect" procedure, that's why I went with the parent
> locking. Also this device needs a delay between stop and start conditions
> while addressing the monitor.
Ok, I thought the problem was that a delay was needed between the STOP
of the command opening the gate and the START of the edid eeprom xfer, and
that everything else worked normally. Too bad this wasn't the actual problem.
Hmm. If it is indeed true that other xfers must never creep into the "select
xfer deselect" procedure then you are indeed stuck with a parent-locking.
But is that really the case? Could it be that the extra delay between
STOP-START is only needed after the absolutely last xfer before the
command closing the gate?
If that problem description is correct, it should be possible to go back to
a mux-locked gate, but then in your deselect implementation grab the i2c adapter
lock manually - with i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER) - before the
30 us delay, then open code the command to close the gate with an unlocked i2c
access, and finally release the i2c bus lock. That way you have ensured silence
on the bus for the required time before closing the gate. You would still need
to bypass regmap, but only in this one place (but maybe you should bypass
regmap for opening the gate too, for symmetry).
Cheers,
Peter
>>
>> There are a couple of more comments below, inline.
>>
>
> snip
>
>>>>>
>>>>> +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
>>>>> +{
>>>>> + struct sii902x *sii902x = i2c_mux_priv(mux);
>>>>> + struct device *dev = &sii902x->i2c->dev;
>>>>> + unsigned long timeout;
>>>>> + u8 status;
>>>>> + int ret;
>>>>> +
>>>>> + ret = __sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA,
>>>>> + SII902X_SYS_CTRL_DDC_BUS_REQ,
>>>>> + SII902X_SYS_CTRL_DDC_BUS_REQ);
>>>>> +
>>>>> + timeout = jiffies +
>>>>> + msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
>>>>> + do {
>>>>> + ret = __sii902x_read(sii902x->i2c, SII902X_SYS_CTRL_DATA,
>>>>> + &status);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> + } while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
>>>>> + time_before(jiffies, timeout));
>>>>> +
>>>>> + if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
>>>>> + dev_err(dev, "Failed to acquire the i2c bus\n");
>>>>> + return -ETIMEDOUT;
>>>>> + }
>>>>> +
>>>>> + ret = __sii902x_write(sii902x->i2c, SII902X_SYS_CTRL_DATA, status);
>>>>> + if (ret)
>>>>> + return ret;
>>
>> Why do I not see a delay here? I thought the whole point of adding the i2c gate
>> was that it enabled the introduction of a delay between opening for edid reading
>> and the actual edid reading?
>
> The delay is needed between STOP and START condition while in passthrough mode.
> __i2c_transfer gets called after the select callback (which enables the passthrough
> mode), when __i2c_transfer is done it generates a STOP condition and then we call into
> the deselect callback. We need a delay between __i2c_transfer and deselect.
>
>>
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
>>>>> +{
>>>>> + struct sii902x *sii902x = i2c_mux_priv(mux);
>>>>> + struct device *dev = &sii902x->i2c->dev;
>>>>> + unsigned long timeout;
>>>>> + unsigned int retries;
>>>>> + u8 status;
>>>>> + int ret;
>>>>> +
>>>>> + udelay(30);
>
> Here is where we need the delay
>
> snip
>
>>>>> @@ -433,6 +558,22 @@ static int sii902x_probe(struct i2c_client *client,
>>>>>
>>>>> i2c_set_clientdata(client, sii902x);
>>>>>
>>>>> + sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
>>>>> + 1, 0, I2C_MUX_GATE,
>>
>> Just use I2C_MUX_GATE | I2C_MUX_LOCKED for the flags argument, and you should be
>> able to make normal i2c accesses.
>
> This is precisely what I tried in the first place, but by generating traffic on the parent adapter
> (since I don't have any other slave on the same i2c bus I have been using i2cdetect) while talking
> to the monitor I have been able to break communications, and sometimes stall the bus, therefore
> I have taken out I2C_MUX_LOCKED.
>
> Ah, do you think the following is going to cause any issue in the future?
> -edid = drm_get_edid(connector, sii902x->i2c->adapter);
> +edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
No, that was a critical part of the idea to use a gate to introduce the
needed delay.
Cheers,
Peter
