On 2018-11-02 17:16, Fabrizio Castro wrote: >>> Here (and also in sii902x_i2c_bypass_deselect) you do a rmw access to the >>> SII902X_SYS_CTRL_DATA register without coordinating with regmap. Regmap is >>> also doing rmw accesses to that register in other parts of the driver. I >>> think you need to either add comment as to why that is safe (maybe other >>> things make it impossible for the two rmw accesses to cross?), or add the >>> missing coordination. >>> >> >> The other two places where SII902X_SYS_CTRL_DATA is being handled are >> sii902x_bridge_disable and sii902x_bridge_enable, I didn’t think there is >> any chance of the modes being probed while the bridge gets enabled/disabled, >> but now that you make me think about it user space may trigger the readback >> of the EDID at the most inconvenient times. Anyway, this is a good point, and >> since I don't think I am supposed to access regmap's lock from outside the APIs, >> I could switch to the unlocked version of update_bits from within sii902x_bridge_disable >> and sii902x_bridge_enable, and manually grab the i2c adapter lock, what do you think? >> > > The bridge enable/disable callbacks deal with different bits of register > SII902X_SYS_CTRL_DATA, and the value of register SII902X_SYS_CTRL_DATA after > sii902x_i2c_bypass_deselect is the same as when sii902x_i2c_bypass_select gets triggered > so basically even if we managed to get in the way of the regmap rmw (in the sense that > for example sii902x_bridge_enable reads SII902X_SYS_CTRL_DATA and then we call > into sii902x_i2c_bypass_select) by the time regmap_update_bits can perform the > write operation the value of register SII902X_SYS_CTRL_DATA is the same as the one > read by regmap, as "select xfer deselect" won't alter the final value of SII902X_SYS_CTRL_DATA > and nobody can get in between. > What do you think I should do? Do you think I can leave this alone and maybe add some > comments or do you think I should explicitly protect access to SII902X_SYS_CTRL_DATA? If the things you write about the bits are true (haven't looked closely), I wouldn't add any regmap coordination. But if I was the maintainer, I'd like a comment about this so that the next contributor has a better chance to understand the subtlety. But I'm not the maintainer, so this is not my call, but by adding the comment you also clarify the situation for the maintainer, which is something that is likely to be appreciated. Cheers, Peter
