On Fri, Jan 08, 2016 at 04:04:48PM +0100, Peter Rosin wrote:
> From: Peter Rosin <[email protected]>
>
> Hi!
>
> [doing a v3 even if there is no "big picture" feedback yet, but
> previous versions has bugs that make them harder to test than
> needed, and testing is very much desired]
>
> I have a pair of boards with this i2c topology:
>
> GPIO ---| ------ BAT1
> | v /
> I2C -----+------B---+---- MUX
> | \
> EEPROM ------ BAT2
>
> (B denotes the boundary between the boards)
>
> The problem with this is that the GPIO controller sits on the same i2c bus
> that it MUXes. For pca954x devices this is worked around by using unlocked
> transfers when updating the MUX. I have no such luck as the GPIO is a general
> purpose IO expander and the MUX is just a random bidirectional MUX, unaware
> of the fact that it is muxing an i2c bus, and extending unlocked transfers
> into the GPIO subsystem is too ugly to even think about. But the general hw
> approach is sane in my opinion, with the number of connections between the
> two boards minimized. To put is plainly, I need support for it.
>
> So, I observe that while it is needed to have the i2c bus locked during the
> actual MUX update in order to avoid random garbage on the slave side, it
> is not strictly a must to have it locked over the whole sequence of a full
> select-transfer-deselect operation. The MUX itself needs to be locked, so
> transfers to clients behind the mux are serialized, and the MUX needs to be
> stable during all i2c traffic (otherwise individual mux slave segments
> might see garbage).
>
> This series accomplishes this by adding code to i2c-mux-gpio and
> i2c-mux-pinctrl that determines if all involved devices used to update the
> mux are controlled by the same root i2c adapter that is muxed. When this
> is the case, the select-transfer-deselect operations should be locked
> individually to avoid the deadlock. The i2c bus *is* still locked
> during muxing, since the muxing happens as part of i2c transfers. This
> is true even if the MUX is updated with several transfers to the GPIO (at
> least as long as *all* MUX changes are using the i2s master bus). A lock
> is added to the mux so that transfers through the mux are serialized.
>
> Concerns:
> - The locking is perhaps too complex?
> - I worry about the priority inheritance aspect of the adapter lock. When
> the transfers behind the mux are divided into select-transfer-deselect all
> locked individually, low priority transfers get more chances to interfere
> with high priority transfers.
> - When doing an i2c_transfer() in_atomic() context or with irqs_disabled(),
> there is a higher possibility that the mux is not returned to its idle
> state after a failed (-EAGAIN) transfer due to trylock.
> - Is the detection of i2c-controlled gpios and pinctrls sane (i.e. the
> usage of the new i2c_root_adapter() function in 8/8)?
>
> To summarize the series, there's some i2c-mux infrastructure cleanup work
> first (I think that part stands by itself as desireable regardless), the
> locking changes are in the last three patches of the series, with the real
> meat in 8/8.
>
> PS. needs a bunch of testing, I do not have access to all the involved hw
I want to let you know that I am currently thinking about this series.
There seems to be a second occasion where it could have helped AFAICT.
http://patchwork.ozlabs.org/patch/584776/ (check my comments there from
yesterday and today)
First of all, really thank you that you tried to find the proper
solution and went all the way for it. It is easy to do a fire&forget
hack here, but you didn't.
I hope you understand, though, that I need to make a balance between
features and complexity in my subsystem to have maintainable and stable
code.
As I wrote in the mentioned thread already: "However, I am still
undecided if that series should go upstream because it makes the mux
code another magnitude more complex. And while this seems to be the
second issue which could be fixed by that series, both issues seem to
be corner cases, so I am not sure it is worth the complexity."
And for the cleanup series using struct mux_core. It is quite an
intrusive change and, frankly, the savings look surprisingly low. I
would have expected more, but you never find out until you do it. So, I
am unsure here as well.
I am not decided and open for discussion. This is just where we are
currently. All interested parties, I am looking forward to more
thoughts.
Thanks,
Wolfram
Wolfram Sang wrote:
> On Fri, Jan 08, 2016 at 04:04:48PM +0100, Peter Rosin wrote:
> >
> > Hi!
> >
> > [doing a v3 even if there is no "big picture" feedback yet, but
> > previous versions has bugs that make them harder to test than
> > needed, and testing is very much desired]
> >
> > I have a pair of boards with this i2c topology:
> >
> > GPIO ---| ------ BAT1
> > | v /
> > I2C -----+------B---+---- MUX
> > | \
> > EEPROM ------ BAT2
> >
> > (B denotes the boundary between the boards)
> >
> > The problem with this is that the GPIO controller sits on the same i2c bus
> > that it MUXes. For pca954x devices this is worked around by using unlocked
> > transfers when updating the MUX. I have no such luck as the GPIO is a general
> > purpose IO expander and the MUX is just a random bidirectional MUX, unaware
> > of the fact that it is muxing an i2c bus, and extending unlocked transfers
> > into the GPIO subsystem is too ugly to even think about. But the general hw
> > approach is sane in my opinion, with the number of connections between the
> > two boards minimized. To put is plainly, I need support for it.
> >
> > So, I observe that while it is needed to have the i2c bus locked during the
> > actual MUX update in order to avoid random garbage on the slave side, it
> > is not strictly a must to have it locked over the whole sequence of a full
> > select-transfer-deselect operation. The MUX itself needs to be locked, so
> > transfers to clients behind the mux are serialized, and the MUX needs to be
> > stable during all i2c traffic (otherwise individual mux slave segments
> > might see garbage).
> >
> > This series accomplishes this by adding code to i2c-mux-gpio and
> > i2c-mux-pinctrl that determines if all involved devices used to update the
> > mux are controlled by the same root i2c adapter that is muxed. When this
> > is the case, the select-transfer-deselect operations should be locked
> > individually to avoid the deadlock. The i2c bus *is* still locked
> > during muxing, since the muxing happens as part of i2c transfers. This
> > is true even if the MUX is updated with several transfers to the GPIO (at
> > least as long as *all* MUX changes are using the i2s master bus). A lock
> > is added to the mux so that transfers through the mux are serialized.
> >
> > Concerns:
> > - The locking is perhaps too complex?
> > - I worry about the priority inheritance aspect of the adapter lock. When
> > the transfers behind the mux are divided into select-transfer-deselect all
> > locked individually, low priority transfers get more chances to interfere
> > with high priority transfers.
> > - When doing an i2c_transfer() in_atomic() context or with irqs_disabled(),
> > there is a higher possibility that the mux is not returned to its idle
> > state after a failed (-EAGAIN) transfer due to trylock.
> > - Is the detection of i2c-controlled gpios and pinctrls sane (i.e. the
> > usage of the new i2c_root_adapter() function in 8/8)?
> >
> > To summarize the series, there's some i2c-mux infrastructure cleanup work
> > first (I think that part stands by itself as desireable regardless), the
> > locking changes are in the last three patches of the series, with the real
> > meat in 8/8.
> >
> > PS. needs a bunch of testing, I do not have access to all the involved hw
>
> I want to let you know that I am currently thinking about this series.
Glad to hear it!
> There seems to be a second occasion where it could have helped AFAICT.
> http://patchwork.ozlabs.org/patch/584776/ (check my comments there from
> yesterday and today)
The mpu6050 driver has to set muxc->i2c_controlled before adding
any child adapters for anything to behave differently, and it also
has to make sure that all accesses in select/deselect are normal
i2c accesses (i.e. not unlocked accesses). When doing so,
unreleated i2c traffic might interleave with the muxing. So, if
the chip is auto-deselecting on the first i2c transfer after select
it will never work properly.
> First of all, really thank you that you tried to find the proper
> solution and went all the way for it. It is easy to do a fire&forget
> hack here, but you didn't.
Fire&forget often turns out to be just the fire. If you do it properly
there is a better chance that you really can forget it...
> I hope you understand, though, that I need to make a balance between
> features and complexity in my subsystem to have maintainable and stable
> code.
>
> As I wrote in the mentioned thread already: "However, I am still
> undecided if that series should go upstream because it makes the mux
> code another magnitude more complex. And while this seems to be the
> second issue which could be fixed by that series, both issues seem to
> be corner cases, so I am not sure it is worth the complexity."
>
> And for the cleanup series using struct mux_core. It is quite an
> intrusive change and, frankly, the savings look surprisingly low. I
> would have expected more, but you never find out until you do it. So, I
> am unsure here as well.
Yes, that part of the series went ballistic when the half-dozen mux
users outside of drivers/i2c was added to the mix (I was originally
not aware of them). The savings looked better when only the i2c-internal
muxes was considered, mainly because the external muxes are often not
real muxes and only have one child adapter. Creating the mux-core for
that one adapter then swallows the savings.
Funnily enough, I was just the other day looking at the series again and
decided to redo those 5 patches so that I first add the new mux core
and implement the old interface in terms of the new interface, then
convert all the mux users one patch at a time, then remove the glue.
That means 15 patches instead of 5, but each patch only touches one
subsystem, which should ease the transition. The end result is
equivalent, I only had to change a few names to make it possible to
have both the old and the new interfaces active at the same time.
In doing so, I realized that what might be good for the non-generic
one-child-only "muxes" is perhaps an interface that creates a mux core
and registers one child adapter with one call?
One other thing that could help the +- statistics is to maybe add more
parameters to i2c_mux_alloc, such as parent adapter and select/deselect
ops. But that is fairly cosmetic...
> I am not decided and open for discussion. This is just where we are
> currently. All interested parties, I am looking forward to more
> thoughts.
Cheers,
Peter
On 02/03/16 17:29, Wolfram Sang wrote:
> On Fri, Jan 08, 2016 at 04:04:48PM +0100, Peter Rosin wrote:
>> From: Peter Rosin <[email protected]>
>>
>> Hi!
>>
>> [doing a v3 even if there is no "big picture" feedback yet, but
>> previous versions has bugs that make them harder to test than
>> needed, and testing is very much desired]
>>
>> I have a pair of boards with this i2c topology:
>>
>> GPIO ---| ------ BAT1
>> | v /
>> I2C -----+------B---+---- MUX
>> | \
>> EEPROM ------ BAT2
>>
>> (B denotes the boundary between the boards)
>>
>> The problem with this is that the GPIO controller sits on the same i2c bus
>> that it MUXes. For pca954x devices this is worked around by using unlocked
>> transfers when updating the MUX. I have no such luck as the GPIO is a general
>> purpose IO expander and the MUX is just a random bidirectional MUX, unaware
>> of the fact that it is muxing an i2c bus, and extending unlocked transfers
>> into the GPIO subsystem is too ugly to even think about. But the general hw
>> approach is sane in my opinion, with the number of connections between the
>> two boards minimized. To put is plainly, I need support for it.
>>
>> So, I observe that while it is needed to have the i2c bus locked during the
>> actual MUX update in order to avoid random garbage on the slave side, it
>> is not strictly a must to have it locked over the whole sequence of a full
>> select-transfer-deselect operation. The MUX itself needs to be locked, so
>> transfers to clients behind the mux are serialized, and the MUX needs to be
>> stable during all i2c traffic (otherwise individual mux slave segments
>> might see garbage).
>>
>> This series accomplishes this by adding code to i2c-mux-gpio and
>> i2c-mux-pinctrl that determines if all involved devices used to update the
>> mux are controlled by the same root i2c adapter that is muxed. When this
>> is the case, the select-transfer-deselect operations should be locked
>> individually to avoid the deadlock. The i2c bus *is* still locked
>> during muxing, since the muxing happens as part of i2c transfers. This
>> is true even if the MUX is updated with several transfers to the GPIO (at
>> least as long as *all* MUX changes are using the i2s master bus). A lock
>> is added to the mux so that transfers through the mux are serialized.
>>
>> Concerns:
>> - The locking is perhaps too complex?
>> - I worry about the priority inheritance aspect of the adapter lock. When
>> the transfers behind the mux are divided into select-transfer-deselect all
>> locked individually, low priority transfers get more chances to interfere
>> with high priority transfers.
>> - When doing an i2c_transfer() in_atomic() context or with irqs_disabled(),
>> there is a higher possibility that the mux is not returned to its idle
>> state after a failed (-EAGAIN) transfer due to trylock.
>> - Is the detection of i2c-controlled gpios and pinctrls sane (i.e. the
>> usage of the new i2c_root_adapter() function in 8/8)?
>>
>> To summarize the series, there's some i2c-mux infrastructure cleanup work
>> first (I think that part stands by itself as desireable regardless), the
>> locking changes are in the last three patches of the series, with the real
>> meat in 8/8.
>>
>> PS. needs a bunch of testing, I do not have access to all the involved hw
>
> I want to let you know that I am currently thinking about this series.
>
> There seems to be a second occasion where it could have helped AFAICT.
> http://patchwork.ozlabs.org/patch/584776/ (check my comments there from
> yesterday and today)
>
> First of all, really thank you that you tried to find the proper
> solution and went all the way for it. It is easy to do a fire&forget
> hack here, but you didn't.
>
> I hope you understand, though, that I need to make a balance between
> features and complexity in my subsystem to have maintainable and stable
> code.
>
> As I wrote in the mentioned thread already: "However, I am still
> undecided if that series should go upstream because it makes the mux
> code another magnitude more complex. And while this seems to be the
> second issue which could be fixed by that series, both issues seem to
> be corner cases, so I am not sure it is worth the complexity."
>
> And for the cleanup series using struct mux_core. It is quite an
> intrusive change and, frankly, the savings look surprisingly low. I
> would have expected more, but you never find out until you do it. So, I
> am unsure here as well.
>
> I am not decided and open for discussion. This is just where we are
> currently. All interested parties, I am looking forward to more
> thoughts.
>
> Thanks,
Perhaps it's one to let sit into at least the next cycle (and get some testing
on those media devices if we can) but, whilst it is fiddly the gains seen in
individual drivers (like the example Peter put in response to the V4 series)
make it look worthwhile to me. Also, whilst the invensense part is plain odd
in many ways, the case Peter had looks rather more normal.
At the end of the day, sometimes fiddly problems need fiddly code.
(says a guy who doesn't have to maintain it!)
It certainly helps that Peter has done a thorough job, broken the patches
up cleanly and provided clean descriptions of what he is doing.
Jonathan
>
> Wolfram
>
> Perhaps it's one to let sit into at least the next cycle (and get some testing
> on those media devices if we can) but, whilst it is fiddly the gains seen in
> individual drivers (like the example Peter put in response to the V4 series)
> make it look worthwhile to me. Also, whilst the invensense part is plain odd
> in many ways, the case Peter had looks rather more normal.
>
> At the end of the day, sometimes fiddly problems need fiddly code.
> (says a guy who doesn't have to maintain it!)
>
> It certainly helps that Peter has done a thorough job, broken the patches
> up cleanly and provided clean descriptions of what he is doing.
Yes, Peter has done a great job so far and the latest results were very
convincing (fixing the invensense issue and the savings for rtl2832).
And yes, I am reluctant to maintain this code alone, so my question
would be:
Peter, are you interested in becoming the i2c-mux maintainer and look
after the code even after it was merged? (From "you reviewing patches and
me picking them up" to "you have your own branch which I pull", we can
discuss the best workflow.)
If that would be the case, I have the same idea like Jonathan: Give it
another cycle for more review & test and aim for the 4.7 merge window.
I have to admit that I still haven't done a more thorough review, so I
can't say if I see a show-stopper in this series. Yet, even if so I am
positive it can be sorted out. Oh, and we should call for people with
special experience in locking.
What do people think?
Regards,
Wolfram
PS: Peter, have you seen my demuxer driver in my for-next branch? I hope
it won't spoil your design?
On 2016-03-05 19:29, Wolfram Sang wrote:
>
>> Perhaps it's one to let sit into at least the next cycle (and get some testing
>> on those media devices if we can) but, whilst it is fiddly the gains seen in
>> individual drivers (like the example Peter put in response to the V4 series)
>> make it look worthwhile to me. Also, whilst the invensense part is plain odd
>> in many ways, the case Peter had looks rather more normal.
>>
>> At the end of the day, sometimes fiddly problems need fiddly code.
>> (says a guy who doesn't have to maintain it!)
>>
>> It certainly helps that Peter has done a thorough job, broken the patches
>> up cleanly and provided clean descriptions of what he is doing.
>
> Yes, Peter has done a great job so far and the latest results were very
> convincing (fixing the invensense issue and the savings for rtl2832).
>
> And yes, I am reluctant to maintain this code alone, so my question
> would be:
>
> Peter, are you interested in becoming the i2c-mux maintainer and look
> after the code even after it was merged? (From "you reviewing patches and
> me picking them up" to "you have your own branch which I pull", we can
> discuss the best workflow.)
My code wouldn't be worth much if I didn't offer to look after it myself. On
the other hand, I am also reluctant to be the go-to person for all things
i2c-mux, as I don't have a clear picture of how much work it's going to be.
My offer is going to be this, I'll look after any unforeseen future problems
caused by this rework, and I can be the i2c-mux maintainer. But if being
the i2c-mux maintainer turns out to be a huge time-sink, there is no way I
can stay on in the long run. But I guess that is the same for any maintainer
(whose job description does not explicitly include being maintainer).
> If that would be the case, I have the same idea like Jonathan: Give it
> another cycle for more review & test and aim for the 4.7 merge window.
Yes, sounds good. One of the reasons for doing things right (or at least
trying to) is to have more people look at the work. *I* think it's good,
but more eyes can't hurt.
> I have to admit that I still haven't done a more thorough review, so I
> can't say if I see a show-stopper in this series. Yet, even if so I am
> positive it can be sorted out. Oh, and we should call for people with
> special experience in locking.
>
> What do people think?
>
> Regards,
>
> Wolfram
>
> PS: Peter, have you seen my demuxer driver in my for-next branch? I hope
> it won't spoil your design?
I had a brief look, and I can't see anything in the demux that's affected by
the mux update. The main commonality of the demux and the preexisting muxes
seems to be that the name includes "mux" and that it is all about i2c. Agreed?
In short, I see no problems.
Cheers,
Peter
> My offer is going to be this, I'll look after any unforeseen future problems
> caused by this rework, and I can be the i2c-mux maintainer. But if being
Yay, thanks a lot!
> the i2c-mux maintainer turns out to be a huge time-sink, there is no way I
> can stay on in the long run. But I guess that is the same for any maintainer
> (whose job description does not explicitly include being maintainer).
Well, since I became the I2C maintainer in late 2012, i2c-mux was always
low-bandwidth:
$ git log --pretty=oneline v3.2.. -- drivers/i2c/muxes/ drivers/i2c/i2c-mux.c | wc -l
72
And your patch series is already bigger than what was accepted in the
last year altogether :) I understand the uncertainty feeling about this
step; however, I truly think it is not much work. It is a niche -
though, one I'd like to have supported by your expertise.
> the mux update. The main commonality of the demux and the preexisting muxes
> seems to be that the name includes "mux" and that it is all about i2c. Agreed?
Yes, and because they are quite different, I wasn't sure if it a) is not
affected at all or b) totally breaks the design. Glad to hear it is a).
Thanks again, looks like we have a roadmap now for getting this series
in \o/
Wolfram