Hello Peter,
Thank you for your feedback!
> > +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_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > + SII902X_SYS_CTRL_DDC_BUS_REQ,
> > + SII902X_SYS_CTRL_DDC_BUS_REQ);
>
> 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?
Fab
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
