| Message ID | 1452058920-9797-1-git-send-email-crope@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Antti, On 2016-01-06 06:42, Antti Palosaari wrote: > Recent i2c mux locking update offers support for i2c controlled i2c > muxes. Use it and get the rid of homemade hackish i2c adapter > locking code. That looks good on a first glance, and I'm sure it felt good to get rid of the locking workaround :-) However, is this safe? From looking at the short datasheet of the si2168, it seems that the mux is used to open up the channel to the tuner? But what happens is there are two parallel accesses, one to the tuner and one to the si2168 chip? With your change, it could happen that the access to the si2168 happens while the gate to the tuner is open. Can that break anything? I.e. thread one thread two ---------- ---------- open gate access si2168 access tuner close gate If that is safe, then I don't understand why the gate isn't left open at all times? The short datasheet is too short to answer my questions... Also, my series needs some Tested-by (and Reviewed-by for that matter), and I assume that you have tested it? Is it ok to add something like that from you? I understand that you may only be able to test your corner of the series, but that would still be very helpful. Thanks! Cheers, Peter > Cc: Peter Rosin <peda@axentia.se> > Cc: Peter Rosin <peda@lysator.liu.se> > Signed-off-by: Antti Palosaari <crope@iki.fi> > --- > drivers/media/dvb-frontends/si2168.c | 61 ++++-------------------------------- > 1 file changed, 6 insertions(+), 55 deletions(-) > > diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c > index ae217b5..d2a5608 100644 > --- a/drivers/media/dvb-frontends/si2168.c > +++ b/drivers/media/dvb-frontends/si2168.c > @@ -18,48 +18,15 @@ > > static const struct dvb_frontend_ops si2168_ops; > > -/* Own I2C adapter locking is needed because of I2C gate logic. */ > -static int si2168_i2c_master_send_unlocked(const struct i2c_client *client, > - const char *buf, int count) > -{ > - int ret; > - struct i2c_msg msg = { > - .addr = client->addr, > - .flags = 0, > - .len = count, > - .buf = (char *)buf, > - }; > - > - ret = __i2c_transfer(client->adapter, &msg, 1); > - return (ret == 1) ? count : ret; > -} > - > -static int si2168_i2c_master_recv_unlocked(const struct i2c_client *client, > - char *buf, int count) > -{ > - int ret; > - struct i2c_msg msg = { > - .addr = client->addr, > - .flags = I2C_M_RD, > - .len = count, > - .buf = buf, > - }; > - > - ret = __i2c_transfer(client->adapter, &msg, 1); > - return (ret == 1) ? count : ret; > -} > - > /* execute firmware command */ > -static int si2168_cmd_execute_unlocked(struct i2c_client *client, > - struct si2168_cmd *cmd) > +static int si2168_cmd_execute(struct i2c_client *client, struct si2168_cmd *cmd) > { > int ret; > unsigned long timeout; > > if (cmd->wlen) { > /* write cmd and args for firmware */ > - ret = si2168_i2c_master_send_unlocked(client, cmd->args, > - cmd->wlen); > + ret = i2c_master_send(client, cmd->args, cmd->wlen); > if (ret < 0) { > goto err; > } else if (ret != cmd->wlen) { > @@ -73,8 +40,7 @@ static int si2168_cmd_execute_unlocked(struct i2c_client *client, > #define TIMEOUT 70 > timeout = jiffies + msecs_to_jiffies(TIMEOUT); > while (!time_after(jiffies, timeout)) { > - ret = si2168_i2c_master_recv_unlocked(client, cmd->args, > - cmd->rlen); > + ret = i2c_master_recv(client, cmd->args, cmd->rlen); > if (ret < 0) { > goto err; > } else if (ret != cmd->rlen) { > @@ -109,17 +75,6 @@ err: > return ret; > } > > -static int si2168_cmd_execute(struct i2c_client *client, struct si2168_cmd *cmd) > -{ > - int ret; > - > - i2c_lock_adapter(client->adapter); > - ret = si2168_cmd_execute_unlocked(client, cmd); > - i2c_unlock_adapter(client->adapter); > - > - return ret; > -} > - > static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status) > { > struct i2c_client *client = fe->demodulator_priv; > @@ -610,11 +565,6 @@ static int si2168_get_tune_settings(struct dvb_frontend *fe, > return 0; > } > > -/* > - * I2C gate logic > - * We must use unlocked I2C I/O because I2C adapter lock is already taken > - * by the caller (usually tuner driver). > - */ > static int si2168_select(struct i2c_mux_core *muxc, u32 chan) > { > struct i2c_client *client = i2c_mux_priv(muxc); > @@ -625,7 +575,7 @@ static int si2168_select(struct i2c_mux_core *muxc, u32 chan) > memcpy(cmd.args, "\xc0\x0d\x01", 3); > cmd.wlen = 3; > cmd.rlen = 0; > - ret = si2168_cmd_execute_unlocked(client, &cmd); > + ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > @@ -645,7 +595,7 @@ static int si2168_deselect(struct i2c_mux_core *muxc, u32 chan) > memcpy(cmd.args, "\xc0\x0d\x00", 3); > cmd.wlen = 3; > cmd.rlen = 0; > - ret = si2168_cmd_execute_unlocked(client, &cmd); > + ret = si2168_cmd_execute(client, &cmd); > if (ret) > goto err; > > @@ -717,6 +667,7 @@ static int si2168_probe(struct i2c_client *client, > dev->muxc->parent = client->adapter; > dev->muxc->select = si2168_select; > dev->muxc->deselect = si2168_deselect; > + dev->muxc->i2c_controlled = true; > > /* create mux i2c adapter for tuner */ > ret = i2c_add_mux_adapter(dev->muxc, 0, 0, 0); > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Moikka! On 01/06/2016 09:14 AM, Peter Rosin wrote: > Hi Antti, > > On 2016-01-06 06:42, Antti Palosaari wrote: >> Recent i2c mux locking update offers support for i2c controlled i2c >> muxes. Use it and get the rid of homemade hackish i2c adapter >> locking code. > > That looks good on a first glance, and I'm sure it felt good to get rid > of the locking workaround :-) And I forgot mutext to protect si2168_cmd_execute()... > However, is this safe? From looking at the short datasheet of the si2168, > it seems that the mux is used to open up the channel to the tuner? But > what happens is there are two parallel accesses, one to the tuner and one > to the si2168 chip? With your change, it could happen that the access to > the si2168 happens while the gate to the tuner is open. Can that break > anything? > > I.e. > thread one thread two > ---------- ---------- > open gate > access si2168 > access tuner > close gate > > If that is safe, then I don't understand why the gate isn't left open > at all times? The short datasheet is too short to answer my questions... It is often called I2C Gate or repeater, and yes it is there to block noise traveling to tuner. I think that noise could be unnecessary I2C traffic as well some digital noise travelling via I2C wires. Tuners are usually build using pure digital logic + analog circuits (RF mixers, RF filters, RF amplifiers). Leaving gate open usually does not harm and there is even some designs it is connected directly to main bus (but almost all demodulators still has that kind of gate to connect tuner). All in all, I don't see it very big issue if only few unwanted I2C messages are sent to bus tuner is connected. > Also, my series needs some Tested-by (and Reviewed-by for that matter), > and I assume that you have tested it? Is it ok to add something like > that from you? I understand that you may only be able to test your > corner of the series, but that would still be very helpful. Thanks! Yes it worked, but I haven't examined it care enough yet. However, I think I see one problem: i2c muxes that deselects channel automatically, usually after the first i2c stop (P) condition is seen. regards Antti > > Cheers, > Peter > >> Cc: Peter Rosin <peda@axentia.se> >> Cc: Peter Rosin <peda@lysator.liu.se> >> Signed-off-by: Antti Palosaari <crope@iki.fi> >> --- >> drivers/media/dvb-frontends/si2168.c | 61 ++++-------------------------------- >> 1 file changed, 6 insertions(+), 55 deletions(-) >> >> diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c >> index ae217b5..d2a5608 100644 >> --- a/drivers/media/dvb-frontends/si2168.c >> +++ b/drivers/media/dvb-frontends/si2168.c >> @@ -18,48 +18,15 @@ >> >> static const struct dvb_frontend_ops si2168_ops; >> >> -/* Own I2C adapter locking is needed because of I2C gate logic. */ >> -static int si2168_i2c_master_send_unlocked(const struct i2c_client *client, >> - const char *buf, int count) >> -{ >> - int ret; >> - struct i2c_msg msg = { >> - .addr = client->addr, >> - .flags = 0, >> - .len = count, >> - .buf = (char *)buf, >> - }; >> - >> - ret = __i2c_transfer(client->adapter, &msg, 1); >> - return (ret == 1) ? count : ret; >> -} >> - >> -static int si2168_i2c_master_recv_unlocked(const struct i2c_client *client, >> - char *buf, int count) >> -{ >> - int ret; >> - struct i2c_msg msg = { >> - .addr = client->addr, >> - .flags = I2C_M_RD, >> - .len = count, >> - .buf = buf, >> - }; >> - >> - ret = __i2c_transfer(client->adapter, &msg, 1); >> - return (ret == 1) ? count : ret; >> -} >> - >> /* execute firmware command */ >> -static int si2168_cmd_execute_unlocked(struct i2c_client *client, >> - struct si2168_cmd *cmd) >> +static int si2168_cmd_execute(struct i2c_client *client, struct si2168_cmd *cmd) >> { >> int ret; >> unsigned long timeout; >> >> if (cmd->wlen) { >> /* write cmd and args for firmware */ >> - ret = si2168_i2c_master_send_unlocked(client, cmd->args, >> - cmd->wlen); >> + ret = i2c_master_send(client, cmd->args, cmd->wlen); >> if (ret < 0) { >> goto err; >> } else if (ret != cmd->wlen) { >> @@ -73,8 +40,7 @@ static int si2168_cmd_execute_unlocked(struct i2c_client *client, >> #define TIMEOUT 70 >> timeout = jiffies + msecs_to_jiffies(TIMEOUT); >> while (!time_after(jiffies, timeout)) { >> - ret = si2168_i2c_master_recv_unlocked(client, cmd->args, >> - cmd->rlen); >> + ret = i2c_master_recv(client, cmd->args, cmd->rlen); >> if (ret < 0) { >> goto err; >> } else if (ret != cmd->rlen) { >> @@ -109,17 +75,6 @@ err: >> return ret; >> } >> >> -static int si2168_cmd_execute(struct i2c_client *client, struct si2168_cmd *cmd) >> -{ >> - int ret; >> - >> - i2c_lock_adapter(client->adapter); >> - ret = si2168_cmd_execute_unlocked(client, cmd); >> - i2c_unlock_adapter(client->adapter); >> - >> - return ret; >> -} >> - >> static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status) >> { >> struct i2c_client *client = fe->demodulator_priv; >> @@ -610,11 +565,6 @@ static int si2168_get_tune_settings(struct dvb_frontend *fe, >> return 0; >> } >> >> -/* >> - * I2C gate logic >> - * We must use unlocked I2C I/O because I2C adapter lock is already taken >> - * by the caller (usually tuner driver). >> - */ >> static int si2168_select(struct i2c_mux_core *muxc, u32 chan) >> { >> struct i2c_client *client = i2c_mux_priv(muxc); >> @@ -625,7 +575,7 @@ static int si2168_select(struct i2c_mux_core *muxc, u32 chan) >> memcpy(cmd.args, "\xc0\x0d\x01", 3); >> cmd.wlen = 3; >> cmd.rlen = 0; >> - ret = si2168_cmd_execute_unlocked(client, &cmd); >> + ret = si2168_cmd_execute(client, &cmd); >> if (ret) >> goto err; >> >> @@ -645,7 +595,7 @@ static int si2168_deselect(struct i2c_mux_core *muxc, u32 chan) >> memcpy(cmd.args, "\xc0\x0d\x00", 3); >> cmd.wlen = 3; >> cmd.rlen = 0; >> - ret = si2168_cmd_execute_unlocked(client, &cmd); >> + ret = si2168_cmd_execute(client, &cmd); >> if (ret) >> goto err; >> >> @@ -717,6 +667,7 @@ static int si2168_probe(struct i2c_client *client, >> dev->muxc->parent = client->adapter; >> dev->muxc->select = si2168_select; >> dev->muxc->deselect = si2168_deselect; >> + dev->muxc->i2c_controlled = true; >> >> /* create mux i2c adapter for tuner */ >> ret = i2c_add_mux_adapter(dev->muxc, 0, 0, 0); >>
On 2016-01-06 06:42, Antti Palosaari wrote: > Recent i2c mux locking update offers support for i2c controlled i2c > muxes. Use it and get the rid of homemade hackish i2c adapter > locking code. [actual patch elided] I had a 2nd look and it seems that the saa7164 driver has support for a HVR2055 card with dual si2168 chips. These two chips appear to sit on the same i2c-bus with different i2c-addresses (0x64 and 0x66) and with gates (implemented as muxes) to two identical tuners with the same i2c-address (0x60). Do I read it right? With the current i2c-mux-locking (parent-locked muxes), this works fine as an access to one of the tuners locks the root i2c adapter and thus the other tuner is also locked out. But with the upcoming i2c-mux-locking for i2c-controlled muxes (self-locked muxes), the root i2c adapter would no longer be locked for the full transaction when one of the tuners is accessed. This means that accesses to the two tuners may interleave and cause all kinds of trouble, should both gates be open at the same time. So, is it really correct and safe to change the si2168 driver to use a self-locked mux? Unless there is some other mechanism that prevents the two tuners from being accessed in parallel, I think not. But maybe there is such a mechanism? Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/23/2016 06:58 PM, Peter Rosin wrote: > On 2016-01-06 06:42, Antti Palosaari wrote: >> Recent i2c mux locking update offers support for i2c controlled i2c >> muxes. Use it and get the rid of homemade hackish i2c adapter >> locking code. > > [actual patch elided] > > I had a 2nd look and it seems that the saa7164 driver has support for > a HVR2055 card with dual si2168 chips. These two chips appear to sit > on the same i2c-bus with different i2c-addresses (0x64 and 0x66) and > with gates (implemented as muxes) to two identical tuners with the > same i2c-address (0x60). Do I read it right? saa7164 has 3 different I2C adapters. saa7164 I2C bus #0: * eeprom * Si2157 #1 saa7164 I2C bus #1: * Si2157 #2 saa7164 I2C bus #2: * Si2168 #1 * Si2168 #2 So both of the Si2157 tuners could have same addresses. (It is Hauppauge WinTV-HVR2205, not HVR-2055). > With the current i2c-mux-locking (parent-locked muxes), this works > fine as an access to one of the tuners locks the root i2c adapter > and thus the other tuner is also locked out. But with the upcoming > i2c-mux-locking for i2c-controlled muxes (self-locked muxes), the > root i2c adapter would no longer be locked for the full transaction > when one of the tuners is accessed. This means that accesses to the > two tuners may interleave and cause all kinds of trouble, should > both gates be open at the same time. So, is it really correct and > safe to change the si2168 driver to use a self-locked mux? > > Unless there is some other mechanism that prevents the two tuners > from being accessed in parallel, I think not. But maybe there is such > a mechanism? Good point. Actually there is pretty often this kind of configuration used for those dual tuner devices and it will cause problems... Currently all of those implements hackish i2c_gate_ctrl() callback to switch mux. ____________ ____________ ____________ |I2C-adapter | | I2C-mux | | I2C-client | |------------| |------------| |------------| | | | addr 0x1c | | addr 0x60 | | | | | | | | |-+-I2C---|-----/ -----|---I2C---| | |____________| | |____________| |____________| | ____________ ____________ | | I2C-mux | | I2C-client | | |------------| |------------| | | addr 0x1d | | addr 0x60 | | | | | | +-I2C---|-----/ -----|---I2C---| | |____________| |____________| regards Antti
On 2016-04-05 16:50, Antti Palosaari wrote: > On 03/23/2016 06:58 PM, Peter Rosin wrote: >> On 2016-01-06 06:42, Antti Palosaari wrote: >>> Recent i2c mux locking update offers support for i2c controlled i2c >>> muxes. Use it and get the rid of homemade hackish i2c adapter >>> locking code. >> >> [actual patch elided] >> >> I had a 2nd look and it seems that the saa7164 driver has support for >> a HVR2055 card with dual si2168 chips. These two chips appear to sit >> on the same i2c-bus with different i2c-addresses (0x64 and 0x66) and >> with gates (implemented as muxes) to two identical tuners with the >> same i2c-address (0x60). Do I read it right? > > saa7164 has 3 different I2C adapters. > > saa7164 I2C bus #0: > * eeprom > * Si2157 #1 > > saa7164 I2C bus #1: > * Si2157 #2 > > saa7164 I2C bus #2: > * Si2168 #1 > * Si2168 #2 > > So both of the Si2157 tuners could have same addresses. > > (It is Hauppauge WinTV-HVR2205, not HVR-2055). Ok, so I did read it right. >> With the current i2c-mux-locking (parent-locked muxes), this works >> fine as an access to one of the tuners locks the root i2c adapter >> and thus the other tuner is also locked out. But with the upcoming >> i2c-mux-locking for i2c-controlled muxes (self-locked muxes), the >> root i2c adapter would no longer be locked for the full transaction >> when one of the tuners is accessed. This means that accesses to the >> two tuners may interleave and cause all kinds of trouble, should >> both gates be open at the same time. So, is it really correct and >> safe to change the si2168 driver to use a self-locked mux? >> >> Unless there is some other mechanism that prevents the two tuners >> from being accessed in parallel, I think not. But maybe there is such >> a mechanism? > > Good point. Actually there is pretty often this kind of configuration used for those dual tuner devices and it will cause problems... Currently all of those implements hackish i2c_gate_ctrl() callback to switch mux. > > ____________ ____________ ____________ > |I2C-adapter | | I2C-mux | | I2C-client | > |------------| |------------| |------------| > | | | addr 0x1c | | addr 0x60 | > | | | | | | > | |-+-I2C---|-----/ -----|---I2C---| | > |____________| | |____________| |____________| > | ____________ ____________ > | | I2C-mux | | I2C-client | > | |------------| |------------| > | | addr 0x1d | | addr 0x60 | > | | | | | > +-I2C---|-----/ -----|---I2C---| | > |____________| |____________| Right, so self-locked muxes (as in v5 [1]) can't do the job, but mux-locked muxes (as in v6 [2]) should be fine. The v6 mux- locked muxes grab a new mux_lock in the parent adapter where the v5 self-locked muxes grabbed a lock in the mux itself. Which means that sibling muxes will lock each other out with v6 when they go for the same lock, which is what we want. Cheers, Peter (v5 and v6 refer to the mux locking update patch series) [1] http://marc.info/?l=linux-i2c&m=145805103325611&w=2 [2] http://marc.info/?l=linux-i2c&m=145967417924732&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c index ae217b5..d2a5608 100644 --- a/drivers/media/dvb-frontends/si2168.c +++ b/drivers/media/dvb-frontends/si2168.c @@ -18,48 +18,15 @@ static const struct dvb_frontend_ops si2168_ops; -/* Own I2C adapter locking is needed because of I2C gate logic. */ -static int si2168_i2c_master_send_unlocked(const struct i2c_client *client, - const char *buf, int count) -{ - int ret; - struct i2c_msg msg = { - .addr = client->addr, - .flags = 0, - .len = count, - .buf = (char *)buf, - }; - - ret = __i2c_transfer(client->adapter, &msg, 1); - return (ret == 1) ? count : ret; -} - -static int si2168_i2c_master_recv_unlocked(const struct i2c_client *client, - char *buf, int count) -{ - int ret; - struct i2c_msg msg = { - .addr = client->addr, - .flags = I2C_M_RD, - .len = count, - .buf = buf, - }; - - ret = __i2c_transfer(client->adapter, &msg, 1); - return (ret == 1) ? count : ret; -} - /* execute firmware command */ -static int si2168_cmd_execute_unlocked(struct i2c_client *client, - struct si2168_cmd *cmd) +static int si2168_cmd_execute(struct i2c_client *client, struct si2168_cmd *cmd) { int ret; unsigned long timeout; if (cmd->wlen) { /* write cmd and args for firmware */ - ret = si2168_i2c_master_send_unlocked(client, cmd->args, - cmd->wlen); + ret = i2c_master_send(client, cmd->args, cmd->wlen); if (ret < 0) { goto err; } else if (ret != cmd->wlen) { @@ -73,8 +40,7 @@ static int si2168_cmd_execute_unlocked(struct i2c_client *client, #define TIMEOUT 70 timeout = jiffies + msecs_to_jiffies(TIMEOUT); while (!time_after(jiffies, timeout)) { - ret = si2168_i2c_master_recv_unlocked(client, cmd->args, - cmd->rlen); + ret = i2c_master_recv(client, cmd->args, cmd->rlen); if (ret < 0) { goto err; } else if (ret != cmd->rlen) { @@ -109,17 +75,6 @@ err: return ret; } -static int si2168_cmd_execute(struct i2c_client *client, struct si2168_cmd *cmd) -{ - int ret; - - i2c_lock_adapter(client->adapter); - ret = si2168_cmd_execute_unlocked(client, cmd); - i2c_unlock_adapter(client->adapter); - - return ret; -} - static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status) { struct i2c_client *client = fe->demodulator_priv; @@ -610,11 +565,6 @@ static int si2168_get_tune_settings(struct dvb_frontend *fe, return 0; } -/* - * I2C gate logic - * We must use unlocked I2C I/O because I2C adapter lock is already taken - * by the caller (usually tuner driver). - */ static int si2168_select(struct i2c_mux_core *muxc, u32 chan) { struct i2c_client *client = i2c_mux_priv(muxc); @@ -625,7 +575,7 @@ static int si2168_select(struct i2c_mux_core *muxc, u32 chan) memcpy(cmd.args, "\xc0\x0d\x01", 3); cmd.wlen = 3; cmd.rlen = 0; - ret = si2168_cmd_execute_unlocked(client, &cmd); + ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -645,7 +595,7 @@ static int si2168_deselect(struct i2c_mux_core *muxc, u32 chan) memcpy(cmd.args, "\xc0\x0d\x00", 3); cmd.wlen = 3; cmd.rlen = 0; - ret = si2168_cmd_execute_unlocked(client, &cmd); + ret = si2168_cmd_execute(client, &cmd); if (ret) goto err; @@ -717,6 +667,7 @@ static int si2168_probe(struct i2c_client *client, dev->muxc->parent = client->adapter; dev->muxc->select = si2168_select; dev->muxc->deselect = si2168_deselect; + dev->muxc->i2c_controlled = true; /* create mux i2c adapter for tuner */ ret = i2c_add_mux_adapter(dev->muxc, 0, 0, 0);
Recent i2c mux locking update offers support for i2c controlled i2c muxes. Use it and get the rid of homemade hackish i2c adapter locking code. Cc: Peter Rosin <peda@axentia.se> Cc: Peter Rosin <peda@lysator.liu.se> Signed-off-by: Antti Palosaari <crope@iki.fi> --- drivers/media/dvb-frontends/si2168.c | 61 ++++-------------------------------- 1 file changed, 6 insertions(+), 55 deletions(-)