Two separate i2c transfers

Two separate i2c transfers

[Adamski, Krzysztof (Nokia - PL/Wrocław)]

Hi,

I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an
advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus
master selector channels. Both masters must do such a sequence before performing long operation:

1. Read status register.
2. If busy flag is not set - write to a register to start operation (this sets busy flag), otherwise exit.

but we cannot call "start operation" if busy flag is already set.

We can solve this problem by keeping our channel selected by BMS between operation 1 and 2 but that is not possible via
i2c framework right now as i2c_mux_master_xfer() will always do select(); i2c_transfer(); deselect() and we need to do
two separate calls to i2c_transfer() (as we have to skip the 2nd one if busy flag is returned by the first one).

There is no way to pass flags to the I2C transfer currently, we can only pass flags to individual messages. A dirty
solution would be to introduce a flag like I2C_M_NO_DESELECT that would be checked in the first (or last) message passed
to i2c_mux_master_xfer() (it would be ignored by normal i2c adapters) before calling deselect(). I'm afraid, however,
that we might need something similar for select as I imagine some i2c muxes might return NAK if you try to select some
channel while other is already selected. But maybe there is some better way of handling that you could suggest?

I imagine this is not a really common scenario but also think we might not be the only ones with such a need technically
this is achievable - the only problem is we cannot express this in current code.

Best regards,
Krzysztof Adamski

Re: Two separate i2c transfers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

Hi Krzysztof,

On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:

Adding Peter as the mux maintainer to CC.

> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an
> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus
> master selector channels. Both masters must do such a sequence before performing long operation:

I need a diagram of that setup. What is the BMS? A chip? Some software?
Can you draw a graph and give names of chips etc...?

And, of course, why on earth do you need to access the same chip from
two masters within one Linux? :) (That's how I understood it)

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Two separate i2c transfers

[Adamski, Krzysztof (Nokia - PL/Wrocław)]

Hi Wolfram, Peter,

Thank you for the quick response.

W dniu 14.05.2020 o 16:50, Wolfram Sang pisze:
> Hi Krzysztof,
> 
> On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
> 
> Adding Peter as the mux maintainer to CC.
> 
>> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an
>> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus
>> master selector channels. Both masters must do such a sequence before performing long operation:
> 
> I need a diagram of that setup. What is the BMS? A chip? Some software?
> Can you draw a graph and give names of chips etc...?
> 
> And, of course, why on earth do you need to access the same chip from
> two masters within one Linux? :) (That's how I understood it)
> 

A diagram might be good indeed as it seems to be hard to explain. I don't use two masters from one Linux, both masters
are own two separate, independent systems and each of them needs access to the target device. BMS is a commonly
available chip - PCA9641 2-channel I2C-bus master arbiter but the problem is not related to any specific arbitrator
device. The target device is also a physical commonly available chip and it is designed in a way that the operation we
are performing might not be interrupted by other transfers except reading status register. The arbitrator device is
there to temporary block access from the other system if the first one is doing its transactions so it gives us a
possibility to do a mutex lock on the bus.

Here's my try to draw this, hopefully this clears things up:

                          +---------------+
                          | Target device |
                          +---------------+
                                  |
                      +-----+-----|----+------+
                      |     | Upstream |      |
                      |     +----------+      |
                      |   Bus Master Arbiter  |
                      |       PCA9641         |
                      |    +---+   +---+      |
                      |    |Ch0|   |Ch1|      |
                      +----+---+---+---+------+
                               |   |
+--------------------------+   |   |   +--------------------------+
| System 1   +------------+|   |   |   |+------------+  System 2  |
|            | I2C Master |+---+   +---+| I2C Master |            |
|            +------------+|           |+------------+            |
|            +-----+       |           |       +-----+            |
|            | CPU |       |           |       | CPU |            |
|            +-----+       |           |       +-----+            |
+--------------------------+           +--------------------------+

I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make
sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already
have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to
do this kind of deeper lock on the bus.


Best regards,
Krzysztof Adamski

Re: Two separate i2c transfers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2963 bytes --]


> A diagram might be good indeed as it seems to be hard to explain. I don't use two masters from one Linux, both masters
> are own two separate, independent systems and each of them needs access to the target device. BMS is a commonly
> available chip - PCA9641 2-channel I2C-bus master arbiter but the problem is not related to any specific arbitrator
> device. The target device is also a physical commonly available chip and it is designed in a way that the operation we
> are performing might not be interrupted by other transfers except reading status register. The arbitrator device is
> there to temporary block access from the other system if the first one is doing its transactions so it gives us a
> possibility to do a mutex lock on the bus.
> 
> Here's my try to draw this, hopefully this clears things up:
> 
>                           +---------------+
>                           | Target device |
>                           +---------------+
>                                   |
>                       +-----+-----|----+------+
>                       |     | Upstream |      |
>                       |     +----------+      |
>                       |   Bus Master Arbiter  |
>                       |       PCA9641         |
>                       |    +---+   +---+      |
>                       |    |Ch0|   |Ch1|      |
>                       +----+---+---+---+------+
>                                |   |
> +--------------------------+   |   |   +--------------------------+
> | System 1   +------------+|   |   |   |+------------+  System 2  |
> |            | I2C Master |+---+   +---+| I2C Master |            |
> |            +------------+|           |+------------+            |
> |            +-----+       |           |       +-----+            |
> |            | CPU |       |           |       | CPU |            |
> |            +-----+       |           |       +-----+            |
> +--------------------------+           +--------------------------+
> 
> I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make
> sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already
> have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to
> do this kind of deeper lock on the bus.

Thanks for this explanation. Much more clear to me now.

However, Peter might have way more insight than me because he was
already working on PCA9641 driver a year ago and I don't know how the
arbitration was designed there.

http://patchwork.ozlabs.org/project/linux-i2c/list/?series=95793

(Do you use this driver or a custom one?)

I'd think that the PCA9641 driver should return -EBUSY if another master
is active, so we'd have the lock on that level. But I may be totally
missing some detail here.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Two separate i2c transfers

[Peter Rosin]

On 2020-05-15 09:04, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
> Hi Wolfram, Peter,
> 
> Thank you for the quick response.
> 
> W dniu 14.05.2020 o 16:50, Wolfram Sang pisze:
>> Hi Krzysztof,
>>
>> On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>>
>> Adding Peter as the mux maintainer to CC.
>>
>>> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an
>>> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus
>>> master selector channels. Both masters must do such a sequence before performing long operation:
>>
>> I need a diagram of that setup. What is the BMS? A chip? Some software?
>> Can you draw a graph and give names of chips etc...?
>>
>> And, of course, why on earth do you need to access the same chip from
>> two masters within one Linux? :) (That's how I understood it)
>>

*snip* useful layout.

> I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make
> sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already
> have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to
> do this kind of deeper lock on the bus.

Yes, that definitely makes it clearer. So, what you need is something
more complex than an I2C xfer between select/deselect. Your proposal
to add a flag to not do the deselect should probably have a matching
flag to not do the select on the following xfer. Otherwise that second
xfer is likely to do useless work. This should probably also be two
independent flags, so that you can add intermediate xfers that neither
select nor deselect. But you also need an explicit deselect mechanism
for use if there is a problem half way through...

But, I think all that exposes the too much to the users, and while it
is certainly possible (most things are), I not a huge fan of it.
Maintainers of other subsystems will not know the rules, and drivers
are, over time, bound to start using these facilities in half-baked
ways.

I would rather have some generic I2C mechanism to request more than
a single xfer as a unit, such that the muxes/arbs/gates can then lock
the mux/arb/gate, do the select, feed the unit to the parent adapter
and finally deselect and unlock the mux/arb/gate. With the locking
and selecting centralized instead of spread out to all users. This
helps avoid bugs where things are not cleaned up on error paths etc.

How that "I2C unit" should be constructed is another matter. The
cool thing of the day seems to be eBPF programs, but I don't know
if that fits here?

Note that there are other users needing more complex "units" than
xfers. Muxes themselves could use it, and there are a number of
drivers that lock the adapter manually, do a couple of unlocked xfers,
and then unlock the adapter. All that could probably be cleaned up a
bit with a generic "I2C unit" facility.

Cheers,
Peter

Re: Two separate i2c transfers

[Adamski, Krzysztof (Nokia - PL/Wrocław)]

Hi Peter,

W dniu 15.05.2020 o 10:02, Peter Rosin pisze:
> 
> 
> On 2020-05-15 09:04, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>> Hi Wolfram, Peter,
>>
>> Thank you for the quick response.
>>
>> W dniu 14.05.2020 o 16:50, Wolfram Sang pisze:
>>> Hi Krzysztof,
>>>
>>> On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>>>
>>> Adding Peter as the mux maintainer to CC.
>>>
>>>> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an
>>>> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus
>>>> master selector channels. Both masters must do such a sequence before performing long operation:
>>>
>>> I need a diagram of that setup. What is the BMS? A chip? Some software?
>>> Can you draw a graph and give names of chips etc...?
>>>
>>> And, of course, why on earth do you need to access the same chip from
>>> two masters within one Linux? :) (That's how I understood it)
>>>
> 
> *snip* useful layout.
> 
>> I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make
>> sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already
>> have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to
>> do this kind of deeper lock on the bus.
> 
> Yes, that definitely makes it clearer. So, what you need is something
> more complex than an I2C xfer between select/deselect. Your proposal
> to add a flag to not do the deselect should probably have a matching
> flag to not do the select on the following xfer. Otherwise that second
> xfer is likely to do useless work. This should probably also be two
> independent flags, so that you can add intermediate xfers that neither
> select nor deselect. But you also need an explicit deselect mechanism
> for use if there is a problem half way through...
> 
> But, I think all that exposes the too much to the users, and while it
> is certainly possible (most things are), I not a huge fan of it.
> Maintainers of other subsystems will not know the rules, and drivers
> are, over time, bound to start using these facilities in half-baked
> ways.
> 
> I would rather have some generic I2C mechanism to request more than
> a single xfer as a unit, such that the muxes/arbs/gates can then lock
> the mux/arb/gate, do the select, feed the unit to the parent adapter
> and finally deselect and unlock the mux/arb/gate. With the locking
> and selecting centralized instead of spread out to all users. This
> helps avoid bugs where things are not cleaned up on error paths etc.
> 

Hmm.. but isn't such a "unit" expressed right now by using i2c_lock_bus()? If you want to do more transfers and do not
want to be disturbed by anyone on local system, you do:

i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
 __i2c_transfer(client->adapter, ...);
 some_logic
 __ice_transfer(client->adapter, ...);
i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);

right? So my idea would be to extend it to "outside of local system" if we have an arbiter as a parent. To not change
existing semantics, that might require additional flag (so such a "deeper lock" would be opt-in):

i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);
 __i2c_transfer(client->adapter, ...)
 some_logic
 __ice_transfer(client->adapter, ...)
i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);


The I2C_LOCK_ARBITER flag would only be relevant for i2c_mux_lock_bus() that could do:
if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) {
    priv->muxc->select(muxc, priv->chan_id);
    priv->muxc->arbitrator_selected = true;
}

and unlock could do the opposite:
if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) {
    priv->muxc->select(muxc, priv->chan_id);
    priv->muxc->arbitrator_selected = true;
}

Now we would also need to change the xfer functions in i2c-mux.c to check for muxc->arbitrator_selected - if it is set,
it would skip doing both select and deselect as this "flag" would mean that selecting/deselecting is done on lock/unlock
and not on transfer level. Of course we could also skip the check for muxc->arbitrator if you think such a lock could
make sense for other types of muxes as well..

Would that really be confusing / un-intutive ?

Best regards,

Re: Two separate i2c transfers

[Adamski, Krzysztof (Nokia - PL/Wrocław)]

Hi Wolfram,

W dniu 15.05.2020 o 09:53, Wolfram Sang pisze:
>
> http://patchwork.ozlabs.org/project/linux-i2c/list/?series=95793
> 
> (Do you use this driver or a custom one?)

Unfortunatly not right now. For historical reasons we have a custom one but I would like to move to this upstream one in
near future.

> 
> I'd think that the PCA9641 driver should return -EBUSY if another master
> is active, so we'd have the lock on that level. But I may be totally
> missing some detail here.

That is a good point however right now the logic in i2c-mux.c is like this:

ret = muxc->select(muxc, priv->chan_id);
if (ret >= 0)
    ret = i2c_transfer(parent, msgs, num);
if (muxc->deselect)
	muxc->deselect(muxc, priv->chan_id);

So this does not really solve my problem - even if we do get the lock by calling select(), we will simply release it as
soon as our i2c_transfer() is finished (and will let the other master take it, breaking atomicity of the two operations
I need to perform on target device) and this is exactly what I need to avoid.

BTW the deselect is called regardless of the return code of select() - even if it returns an error. I'm not sure if this
might cause some problems (unbalanced numer of select/deselect operation) but I find it a little odd. Is this deliberate?

Best regards,
Krzysztof Adamski

Re: Two separate i2c transfers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]


> So this does not really solve my problem - even if we do get the lock
> by calling select(), we will simply release it as soon as our
> i2c_transfer() is finished (and will let the other master take it,
> breaking atomicity of the two operations I need to perform on target
> device) and this is exactly what I need to avoid.

I was assuming that once you have the lock from the arbitrator you can
be sure that the other master is not active and, thus, you maybe don't
need to read the status register at all? But I am probably wrong here.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Two separate i2c transfers

[Peter Rosin]

On 2020-05-15 11:20, Wolfram Sang wrote:
> 
>> So this does not really solve my problem - even if we do get the lock
>> by calling select(), we will simply release it as soon as our
>> i2c_transfer() is finished (and will let the other master take it,
>> breaking atomicity of the two operations I need to perform on target
>> device) and this is exactly what I need to avoid.
> 
> I was assuming that once you have the lock from the arbitrator you can
> be sure that the other master is not active and, thus, you maybe don't
> need to read the status register at all? But I am probably wrong here.

You are right in that when you have the lock from the arbtrator, you
lock the other master(s) out, but the issue here is that the mux code
releases the arbitrator lock after each xfer. Krzysztof needs to do
two xfers with the arbitrator lock held over both so that another
master cannot sneak in and change the world view.

Cheers,
Peter

Re: Two separate i2c transfers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]


> > I was assuming that once you have the lock from the arbitrator you can
> > be sure that the other master is not active and, thus, you maybe don't
> > need to read the status register at all? But I am probably wrong here.
> 
> You are right in that when you have the lock from the arbtrator, you
> lock the other master(s) out, but the issue here is that the mux code
> releases the arbitrator lock after each xfer. Krzysztof needs to do
> two xfers with the arbitrator lock held over both so that another
> master cannot sneak in and change the world view.

Yes. My probably wrong assumption was that the first part of the
transfer (reading a status reg) was only to check if another master has
started the device and is running some operation. So, if we could avoid
that this happens by proper locking of the arbitrator, you could either
skip the first part of the transaction or just send both in one go.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Two separate i2c transfers

[Adamski, Krzysztof (Nokia - PL/Wrocław)]

Hi Wolfram

W dniu 15.05.2020 o 11:20, Wolfram Sang pisze:
> 
>> So this does not really solve my problem - even if we do get the lock
>> by calling select(), we will simply release it as soon as our
>> i2c_transfer() is finished (and will let the other master take it,
>> breaking atomicity of the two operations I need to perform on target
>> device) and this is exactly what I need to avoid.
> 
> I was assuming that once you have the lock from the arbitrator you can
> be sure that the other master is not active and, thus, you maybe don't
> need to read the status register at all? But I am probably wrong here.
> 

The problem here is that the i2c-mux framework will only take lock for one transfer i2c and will always drop this lock
after this one transfer. We can send multiple messages in one transaction and keep the lock for the whole time but what
I actually needs is to send multiple transactions, not just messages. We are programming flash memory of an target
device and we have to poll for some status bits before we can send more data, for example.

So there are two possibilities:

1. I could lock the arbitrator for the whole time the programming is done. This way I could indeed skip reading status
register.
2. We could lock the arbitrator, read the status register of the target device to check if programming is already
running, if it is not, then start it (which will set the bit in status register). Those to operations have to be atomic
so I have to hold the lock between the two transactions.

None of the two options can be done currently as mux layer will call deselect (releasing the lock) after each
transaction and we need more than one transaction (as described above). We need to use the 2nd option, however, as this
lets 2nd system still talk to some other devices on the downstream bus while the 1st one is performing the flashing of
the target device for example while we're waiting for target device to be ready for next page of data) but this is not
that relevant in this discussion.

Best regards,
Krzysztof Adamski

Re: Two separate i2c transfers

[Peter Rosin]

On 2020-05-15 10:36, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
> Hi Peter,
> 
> W dniu 15.05.2020 o 10:02, Peter Rosin pisze:
>>
>>
>> On 2020-05-15 09:04, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>>> Hi Wolfram, Peter,
>>>
>>> Thank you for the quick response.
>>>
>>> W dniu 14.05.2020 o 16:50, Wolfram Sang pisze:
>>>> Hi Krzysztof,
>>>>
>>>> On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>>>>
>>>> Adding Peter as the mux maintainer to CC.
>>>>
>>>>> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an
>>>>> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus
>>>>> master selector channels. Both masters must do such a sequence before performing long operation:
>>>>
>>>> I need a diagram of that setup. What is the BMS? A chip? Some software?
>>>> Can you draw a graph and give names of chips etc...?
>>>>
>>>> And, of course, why on earth do you need to access the same chip from
>>>> two masters within one Linux? :) (That's how I understood it)
>>>>
>>
>> *snip* useful layout.
>>
>>> I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make
>>> sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already
>>> have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to
>>> do this kind of deeper lock on the bus.
>>
>> Yes, that definitely makes it clearer. So, what you need is something
>> more complex than an I2C xfer between select/deselect. Your proposal
>> to add a flag to not do the deselect should probably have a matching
>> flag to not do the select on the following xfer. Otherwise that second
>> xfer is likely to do useless work. This should probably also be two
>> independent flags, so that you can add intermediate xfers that neither
>> select nor deselect. But you also need an explicit deselect mechanism
>> for use if there is a problem half way through...
>>
>> But, I think all that exposes the too much to the users, and while it
>> is certainly possible (most things are), I not a huge fan of it.
>> Maintainers of other subsystems will not know the rules, and drivers
>> are, over time, bound to start using these facilities in half-baked
>> ways.
>>
>> I would rather have some generic I2C mechanism to request more than
>> a single xfer as a unit, such that the muxes/arbs/gates can then lock
>> the mux/arb/gate, do the select, feed the unit to the parent adapter
>> and finally deselect and unlock the mux/arb/gate. With the locking
>> and selecting centralized instead of spread out to all users. This
>> helps avoid bugs where things are not cleaned up on error paths etc.
>>
> 
> Hmm.. but isn't such a "unit" expressed right now by using i2c_lock_bus()? If you want to do more transfers and do not
> want to be disturbed by anyone on local system, you do:
> 
> i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
>  __i2c_transfer(client->adapter, ...);
>  some_logic
>  __ice_transfer(client->adapter, ...);
> i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
> 
> right? So my idea would be to extend it to "outside of local system" if we have an arbiter as a parent. To not change
> existing semantics, that might require additional flag (so such a "deeper lock" would be opt-in):

Ok, now I see what you mean, and I realize that you were talking
about a new flag for i2c_bus_lock() all along. Brain-fart on my
part. Sorry.

Yes, by adding the flag to i2c_bus_(un)lock() you get something that
is much closer to existing semantics.

> i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);
>  __i2c_transfer(client->adapter, ...)
>  some_logic
>  __ice_transfer(client->adapter, ...)
> i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);
> 
> 
> The I2C_LOCK_ARBITER flag would only be relevant for i2c_mux_lock_bus() that could do:

This is not true, you would have to do it in i2c_parent_(un)lock_bus
too. The I2C client drivers should not need to know (or care) if the
muxes are parent-locked or mux-locked.

> if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) {
>     priv->muxc->select(muxc, priv->chan_id);
>     priv->muxc->arbitrator_selected = true;
> }
> 
> and unlock could do the opposite:
> if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) {
>     priv->muxc->select(muxc, priv->chan_id);
>     priv->muxc->arbitrator_selected = true;

(unselect and false, but I get it)

> }
> 
> Now we would also need to change the xfer functions in i2c-mux.c to check for muxc->arbitrator_selected - if it is set,
> it would skip doing both select and deselect as this "flag" would mean that selecting/deselecting is done on lock/unlock
> and not on transfer level. Of course we could also skip the check for muxc->arbitrator if you think such a lock could
> make sense for other types of muxes as well..

I think it may well make sense, so skip the check and name the
name of the flag can be just 'selected'.

> Would that really be confusing / un-intutive ?

No, I guess not. It's pretty much on-par with the existing I2C locking
facilities.

Hmmm, on second thought, maybe the select/deselect should be part of
lock/unlock unconditionally? The big problem with that is that
"locking" may fail. And, I now realize that this is the big issue
with the whole approach. You need to add a return value to
i2c_lock_operations.lock_bus, and that feels like a *big* change.
Maybe not technically, the there will be a signal that the function
may fail. I.e. every client will, with that return value, get pressure
to check and act on locking failures when it is in fact only needed in
this new situation.

I guess you could do the select/deselect thing conditionally, like
you suggested, but make it an error to try to use it with
i2c_lock_bus(), and only allow it from i2c_trylock_bus() which
already has a return value. But with that, I feel that you need to
somehow separate the cases when you want to actually wait for the lock
and when you want to give up immediately if the lock is taken.

I don't know if I'm making sense. I'm typing as I'm thinking if
anyone hadn't noticed...

Cheers,
Peter

Re: Two separate i2c transfers

[Peter Rosin]

On 2020-05-15 23:19, Peter Rosin wrote:
> 
> 
> On 2020-05-15 10:36, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>> Hi Peter,
>>
>> W dniu 15.05.2020 o 10:02, Peter Rosin pisze:
>>>
>>>
>>> On 2020-05-15 09:04, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>>>> Hi Wolfram, Peter,
>>>>
>>>> Thank you for the quick response.
>>>>
>>>> W dniu 14.05.2020 o 16:50, Wolfram Sang pisze:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>>>>>
>>>>> Adding Peter as the mux maintainer to CC.
>>>>>
>>>>>> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an
>>>>>> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus
>>>>>> master selector channels. Both masters must do such a sequence before performing long operation:
>>>>>
>>>>> I need a diagram of that setup. What is the BMS? A chip? Some software?
>>>>> Can you draw a graph and give names of chips etc...?
>>>>>
>>>>> And, of course, why on earth do you need to access the same chip from
>>>>> two masters within one Linux? :) (That's how I understood it)
>>>>>
>>>
>>> *snip* useful layout.
>>>
>>>> I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make
>>>> sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already
>>>> have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to
>>>> do this kind of deeper lock on the bus.
>>>
>>> Yes, that definitely makes it clearer. So, what you need is something
>>> more complex than an I2C xfer between select/deselect. Your proposal
>>> to add a flag to not do the deselect should probably have a matching
>>> flag to not do the select on the following xfer. Otherwise that second
>>> xfer is likely to do useless work. This should probably also be two
>>> independent flags, so that you can add intermediate xfers that neither
>>> select nor deselect. But you also need an explicit deselect mechanism
>>> for use if there is a problem half way through...
>>>
>>> But, I think all that exposes the too much to the users, and while it
>>> is certainly possible (most things are), I not a huge fan of it.
>>> Maintainers of other subsystems will not know the rules, and drivers
>>> are, over time, bound to start using these facilities in half-baked
>>> ways.
>>>
>>> I would rather have some generic I2C mechanism to request more than
>>> a single xfer as a unit, such that the muxes/arbs/gates can then lock
>>> the mux/arb/gate, do the select, feed the unit to the parent adapter
>>> and finally deselect and unlock the mux/arb/gate. With the locking
>>> and selecting centralized instead of spread out to all users. This
>>> helps avoid bugs where things are not cleaned up on error paths etc.
>>>
>>
>> Hmm.. but isn't such a "unit" expressed right now by using i2c_lock_bus()? If you want to do more transfers and do not
>> want to be disturbed by anyone on local system, you do:
>>
>> i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
>>  __i2c_transfer(client->adapter, ...);
>>  some_logic
>>  __ice_transfer(client->adapter, ...);
>> i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
>>
>> right? So my idea would be to extend it to "outside of local system" if we have an arbiter as a parent. To not change
>> existing semantics, that might require additional flag (so such a "deeper lock" would be opt-in):
> 
> Ok, now I see what you mean, and I realize that you were talking
> about a new flag for i2c_bus_lock() all along. Brain-fart on my
> part. Sorry.
> 
> Yes, by adding the flag to i2c_bus_(un)lock() you get something that
> is much closer to existing semantics.
> 
>> i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);
>>  __i2c_transfer(client->adapter, ...)
>>  some_logic
>>  __ice_transfer(client->adapter, ...)
>> i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);
>>
>>
>> The I2C_LOCK_ARBITER flag would only be relevant for i2c_mux_lock_bus() that could do:
> 
> This is not true, you would have to do it in i2c_parent_(un)lock_bus
> too. The I2C client drivers should not need to know (or care) if the
> muxes are parent-locked or mux-locked.
> 
>> if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) {
>>     priv->muxc->select(muxc, priv->chan_id);
>>     priv->muxc->arbitrator_selected = true;
>> }
>>
>> and unlock could do the opposite:
>> if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) {
>>     priv->muxc->select(muxc, priv->chan_id);
>>     priv->muxc->arbitrator_selected = true;
> 
> (unselect and false, but I get it)
> 
>> }
>>
>> Now we would also need to change the xfer functions in i2c-mux.c to check for muxc->arbitrator_selected - if it is set,
>> it would skip doing both select and deselect as this "flag" would mean that selecting/deselecting is done on lock/unlock
>> and not on transfer level. Of course we could also skip the check for muxc->arbitrator if you think such a lock could
>> make sense for other types of muxes as well..
> 
> I think it may well make sense, so skip the check and name the
> name of the flag can be just 'selected'.
> 
>> Would that really be confusing / un-intutive ?
> 
> No, I guess not. It's pretty much on-par with the existing I2C locking
> facilities.
> 
> Hmmm, on second thought, maybe the select/deselect should be part of
> lock/unlock unconditionally? The big problem with that is that
> "locking" may fail. And, I now realize that this is the big issue
> with the whole approach. You need to add a return value to
> i2c_lock_operations.lock_bus, and that feels like a *big* change.
> Maybe not technically, the there will be a signal that the function
> may fail. I.e. every client will, with that return value, get pressure
> to check and act on locking failures when it is in fact only needed in
> this new situation.
> 
> I guess you could do the select/deselect thing conditionally, like
> you suggested, but make it an error to try to use it with
> i2c_lock_bus(), and only allow it from i2c_trylock_bus() which
> already has a return value. But with that, I feel that you need to
> somehow separate the cases when you want to actually wait for the lock
> and when you want to give up immediately if the lock is taken.
> 
> I don't know if I'm making sense. I'm typing as I'm thinking if
> anyone hadn't noticed...

The return value could be added to i2c_lock_operations.lock_bus, but not
be exposed when called through i2c_lock_bus(). That wouldn't bee too bad
I guess. Then a separate set of functions could be added for lock+select
that do expose the return value (and adds the new flag of course). I.e.
i2c_lock_select_bus, i2c_trylock_select_bus and t2c_deselect_unlock_bus.
Or something like that.

However, I have the feeling that *most* client drivers that do their
own i2c_lock_adapter/__i2c_transfer/.../i2c_unlock_adapter really would
like to also keep arbitrators locked during the operation to prevent not
only the local master from clobbering the chip status.

Hmm. New idea. Maybe the arbitrator ->select should remain with the xfer
as it is today, but then you set a flag that the arbitrator is selected,
and delay the deselect until the unlock (instead of doing it last in the
xfer). You would of course need to check the flag before doing the select
in the xfer, so that follow-up xfers don't redo the select.

Something like this (untested, written in the mail client):

static int __i2c_mux_master_xfer(struct i2c_adapter *adap,
				 struct i2c_msg msgs[], int num)
{
	struct i2c_mux_priv *priv = adap->algo_data;
	struct i2c_mux_core *muxc = priv->muxc;
	struct i2c_adapter *parent = muxc->parent;

	/* Switch to the right mux port and perform the transfer. */

	if (!muxc->selected) {
		int ret = muxc->select(muxc, priv->chan_id);
		muxc->selected = true;
		if (ret < 0)
			return ret;
	}

	return __i2c_transfer(parent, msgs, num);
}

static void i2c_parent_unlock_bus(struct i2c_adapter *adapter,
				  unsigned int flags)
{
	struct i2c_mux_priv *priv = adapter->algo_data;
	struct i2c_mux_core *muxc = priv->muxc;
	struct i2c_adapter *parent = muxc->parent;

	if (muxc->selected && muxc->deselect)
		muxc->deselect(muxc, priv->chan_id);
	muxc->selected = false;
	i2c_unlock_bus(parent, flags);
	rt_mutex_unlock(&parent->mux_lock);
}

(i2c_mux_master_xfer(), __i2c_mux_smbus_xfer(), i2c_mux_smbus_xfer() and
i2c_mux_unlock_bus() need corresponding changes)

One more thing to keep in mind. Some arbs/muxes/gates auto-deselects after
the Nth xfer (always the 1st?), so arb/mux/gate drivers must probably have
some way to opt out of whatever we do to support "I2C units" across
arbs/muxes/gates. Maybe that property mostly applies to gates? Anyway, the
above is not a complete solution.

Cheers,
Peter

Re: Two separate i2c transfers

[Adamski, Krzysztof (Nokia - PL/Wrocław)]

Hi Peter, Wofram,

W dniu 17.05.2020 o 23:32, Peter Rosin pisze:
> On 2020-05-15 23:19, Peter Rosin wrote:
>
> Ok, now I see what you mean, and I realize that you were talking
> about a new flag for i2c_bus_lock() all along. Brain-fart on my
> part. Sorry.
>
> Yes, by adding the flag to i2c_bus_(un)lock() you get something that
> is much closer to existing semantics.
>
>>> i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);
>>>  __i2c_transfer(client->adapter, ...)
>>>  some_logic
>>>  __ice_transfer(client->adapter, ...)
>>> i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);
>>>
>>>
>>> The I2C_LOCK_ARBITER flag would only be relevant for i2c_mux_lock_bus() that could do:
>> This is not true, you would have to do it in i2c_parent_(un)lock_bus
>> too. The I2C client drivers should not need to know (or care) if the
>> muxes are parent-locked or mux-locked.

Yes, I agree. That was what I was thinking about - I meant that it is only relevant to callbacks in i2c-mux.c

>> Hmmm, on second thought, maybe the select/deselect should be part of
>> lock/unlock unconditionally? The big problem with that is that
>> "locking" may fail. And, I now realize that this is the big issue
>> with the whole approach. You need to add a return value to
>> i2c_lock_operations.lock_bus, and that feels like a *big* change.
>> Maybe not technically, the there will be a signal that the function
>> may fail. I.e. every client will, with that return value, get pressure
>> to check and act on locking failures when it is in fact only needed in
>> this new situation.

This is indeed a bigger problem I didn't think about earlier. But at I guess we agree that having such a possibility is
useful, we just have to find out the best way to implement this.

>>
>> I guess you could do the select/deselect thing conditionally, like
>> you suggested, but make it an error to try to use it with
>> i2c_lock_bus(), and only allow it from i2c_trylock_bus() which
>> already has a return value. But with that, I feel that you need to
>> somehow separate the cases when you want to actually wait for the lock
>> and when you want to give up immediately if the lock is taken.

But the problem is that currently the select callback does not have a "trylock" semantics - it tries for quite some time
before giving up. We would need to introduce some kind of flag to distinguish between the two. But the caller then would
have to most likely implement his own timeout loop which is not elegant solution either.

> The return value could be added to i2c_lock_operations.lock_bus, but not
> be exposed when called through i2c_lock_bus(). That wouldn't bee too bad
> I guess. Then a separate set of functions could be added for lock+select
> that do expose the return value (and adds the new flag of course). I.e.
> i2c_lock_select_bus, i2c_trylock_select_bus and t2c_deselect_unlock_bus.
> Or something like that.

That would be ok for me but Wolfram would have to comment, I guess.

> However, I have the feeling that *most* client drivers that do their
> own i2c_lock_adapter/__i2c_transfer/.../i2c_unlock_adapter really would
> like to also keep arbitrators locked during the operation to prevent not
> only the local master from clobbering the chip status.

I'm not sure how useful this is for generic case (if it's "most" or only "some") but as you pointed out, it is not a
simple extension of a lock as now the client would have to deal with the fail possibility so I think such a lock should
still be considered an opt-in. For this, a new API is good idea.

> Hmm. New idea. Maybe the arbitrator ->select should remain with the xfer
> as it is today, but then you set a flag that the arbitrator is selected,
> and delay the deselect until the unlock (instead of doing it last in the
> xfer). You would of course need to check the flag before doing the select
> in the xfer, so that follow-up xfers don't redo the select.
>
> Something like this (untested, written in the mail client):
>
> static int __i2c_mux_master_xfer(struct i2c_adapter *adap,
> 				 struct i2c_msg msgs[], int num)
> {
> 	struct i2c_mux_priv *priv = adap->algo_data;
> 	struct i2c_mux_core *muxc = priv->muxc;
> 	struct i2c_adapter *parent = muxc->parent;
>
> 	/* Switch to the right mux port and perform the transfer. */
>
> 	if (!muxc->selected) {
> 		int ret = muxc->select(muxc, priv->chan_id);
> 		muxc->selected = true;
> 		if (ret < 0)
> 			return ret;
> 	}
>
> 	return __i2c_transfer(parent, msgs, num);
> }
>
> static void i2c_parent_unlock_bus(struct i2c_adapter *adapter,
> 				  unsigned int flags)
> {
> 	struct i2c_mux_priv *priv = adapter->algo_data;
> 	struct i2c_mux_core *muxc = priv->muxc;
> 	struct i2c_adapter *parent = muxc->parent;
>
> 	if (muxc->selected && muxc->deselect)
> 		muxc->deselect(muxc, priv->chan_id);
> 	muxc->selected = false;
> 	i2c_unlock_bus(parent, flags);
> 	rt_mutex_unlock(&parent->mux_lock);
> }
>
> (i2c_mux_master_xfer(), __i2c_mux_smbus_xfer(), i2c_mux_smbus_xfer() and
> i2c_mux_unlock_bus() need corresponding changes)

Something like this was my initial idea. But I'm not that sure if such a "deeper lock" should indeed be default option
for all callers. Could that introduce some regressions? Especially if we somehow get deselected automatically by the
device (like noted below).

> One more thing to keep in mind. Some arbs/muxes/gates auto-deselects after
> the Nth xfer (always the 1st?), so arb/mux/gate drivers must probably have
> some way to opt out of whatever we do to support "I2C units" across
> arbs/muxes/gates. Maybe that property mostly applies to gates? Anyway, the
> above is not a complete solution.

Also there is a possibility that a mux could be deselected after some idle time as well. I think the PCA9461 does
support something like that but I would have to check.

Maybe a hybrid approach would be best, then? The user could provide a flag to i2c_bus_lock() to opt-in for a deeper lock
(to avoid possible regressions and changed behavior) but the actual select would be called in __i2c_mux_mater_xfer and
deselect in unlock_bus as you suggested above (but only if the flag was provided to i2c_bus_{un,}lock(). I'm not sure if
this is better than introducing i2c_{un,}lock_select_bus() as suggested previously, however. Wolfram, what are your
thoughts? I think an additional point of view would be useful here.

Krzysztof