Re: [PATCH 21/66] rtl2830: implement own I2C locking
On Tue, Feb 03, 2015 at 08:34:01PM +0200, Antti Palosaari wrote: On 02/03/2015 07:53 PM, Mauro Carvalho Chehab wrote: If latter a better way to lock the I2C mux appears, we can reverse this change. More I am worried about next patch in a serie, which converts all that to regmap API... Same recursive mux register access comes to problem there, which I work-arounded by defining own I2C IO... And in that case I used i2c_lock_adapter/i2c_unlock_adapter so adapter is locked properly. Opne coding the register I/O is a terrible solution, we should allow people to keep this code factored out. signature.asc Description: Digital signature
Re: [PATCH 21/66] rtl2830: implement own I2C locking
On 02/03/2015 07:53 PM, Mauro Carvalho Chehab wrote: Em Mon, 02 Feb 2015 21:33:24 +0100 Wolfram Sang w...@the-dreams.de escreveu: Ok, this may eventually work ok for now, but a further change at the I2C core could easily break it. So, we need to double check about such patch with the I2C maintainer. Jean, Are you ok with such patch? If so, please ack. Jean handed over I2C to me in late 2012 :) Sorry for the mess... I mis-read MAINTAINERS. Basic problem here is that I2C-mux itself is controlled by same I2C device which implements I2C adapter for tuner. Here is what connections looks like: ___ | USB IF | | demod| |tuner | |---| || || | |--I2C--|-/ -|--I2C--|| |I2C master | | I2C mux | | I2C slave | |___| || || So when tuner is called via I2C, it needs recursively call same I2C adapter which is already locked. More elegant solution would be indeed nice. So, AFAIU this is the same problem that I2C based mux devices have (like drivers/i2c/muxes/i2c-mux-pca954x.c)? They also use the unlocked transfers... If I understood your comment correct, you're ok with this approach, right? I'll then merge the remaining of this 66-patch series. If latter a better way to lock the I2C mux appears, we can reverse this change. More I am worried about next patch in a serie, which converts all that to regmap API... Same recursive mux register access comes to problem there, which I work-arounded by defining own I2C IO... And in that case I used i2c_lock_adapter/i2c_unlock_adapter so adapter is locked properly. [PATCH 22/66] rtl2830: convert to regmap API http://www.spinics.net/lists/linux-media/msg84969.html regards Antti -- http://palosaari.fi/ -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/66] rtl2830: implement own I2C locking
Em Mon, 02 Feb 2015 21:33:24 +0100 Wolfram Sang w...@the-dreams.de escreveu: Ok, this may eventually work ok for now, but a further change at the I2C core could easily break it. So, we need to double check about such patch with the I2C maintainer. Jean, Are you ok with such patch? If so, please ack. Jean handed over I2C to me in late 2012 :) Sorry for the mess... I mis-read MAINTAINERS. Basic problem here is that I2C-mux itself is controlled by same I2C device which implements I2C adapter for tuner. Here is what connections looks like: ___ | USB IF | | demod| |tuner | |---| || || | |--I2C--|-/ -|--I2C--|| |I2C master | | I2C mux | | I2C slave | |___| || || So when tuner is called via I2C, it needs recursively call same I2C adapter which is already locked. More elegant solution would be indeed nice. So, AFAIU this is the same problem that I2C based mux devices have (like drivers/i2c/muxes/i2c-mux-pca954x.c)? They also use the unlocked transfers... If I understood your comment correct, you're ok with this approach, right? I'll then merge the remaining of this 66-patch series. If latter a better way to lock the I2C mux appears, we can reverse this change. Regards, Mauro pgpcIQSSqAgoP.pgp Description: Assinatura digital OpenPGP
Re: [PATCH 21/66] rtl2830: implement own I2C locking
On 02/02/2015 10:56 PM, Jean Delvare wrote: Hi Mauro, Antti, On Mon, 2 Feb 2015 18:07:26 -0200, Mauro Carvalho Chehab wrote: Em Tue, 23 Dec 2014 22:49:14 +0200 Antti Palosaari cr...@iki.fi escreveu: Own I2C locking is needed due to two special reasons: 1) Chips uses multiple register pages/banks on single I2C slave. Page is changed via I2C register access. This is no good reason to implement your own i2c bus locking. Lots of i2c slave device work that way, and the way to handle it is through a dedicated lock at the i2c slave device level. This is in addition to the standard i2c bus locking and not a replacement. Patch description is bit misleading as it does not implement own I2C bus lock but own 'I2C lock' is there to warrant none will interrupt I/O operation as it needs multiple I2C calls. *** take own I2C lock here 1) I2C mux read to read current register page 2) I2C mux write to switch register page (if needed) 3) I2C mux write to change mux (open gate/repeater for I2C bus tuner is) 4) perform tuner I2C access *** release own I2C lock here Mux is closed automatically after tuner I2C in that case, but very often there is I2C commands needed for that too. 2) Chip offers muxed/gated I2C adapter for tuner. Gate/mux is controlled by I2C register access. This, OTOH, is a valid reason for calling __i2c_transfer, and as a matter of fact a number of dvb frontend drivers already do so. Due to these reasons, I2C locking did not fit very well. I don't like the idea of calling __i2c_transfer() without calling first i2c_lock_adapter(). This can be dangerous, as the I2C core itself uses the lock for its own usage. I think the idea is that the i2c bus lock is already held at the time the muxing code is called. This happens each time the I2C muxing chip is an I2C chip itself. Ok, this may eventually work ok for now, but a further change at the I2C core could easily break it. So, we need to double check about such patch with the I2C maintainer. If it breaks than it'll break a dozen drivers which are already doing that, not just this one. But it's OK, I don't see this happening soon. Jean, Are you ok with such patch? If so, please ack. First of all: I am no longer the maintainer of the I2C subsystem. That being said... The changes look OK to me. I think it's how they are presented which make them look suspect. As I understand it, the extra locking at device level is unrelated with calling unlocked i2c transfer functions. The former change is to address the multi-page/bank register mapping, while the latter is to solve the deadlock due to the i2c bus topology and i2c-based muxing. If I am correct then it would be clearer to make that two separate patches with better descriptions. And if I'm wrong then the patch needs a better description too ;-) regards Antti -- http://palosaari.fi/ -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/66] rtl2830: implement own I2C locking
On 02/02/2015 09:33 PM, Wolfram Sang wrote: Ok, this may eventually work ok for now, but a further change at the I2C core could easily break it. So, we need to double check about such patch with the I2C maintainer. Jean, Are you ok with such patch? If so, please ack. Jean handed over I2C to me in late 2012 :) Basic problem here is that I2C-mux itself is controlled by same I2C device which implements I2C adapter for tuner. Here is what connections looks like: ___ | USB IF | | demod| |tuner | |---| || || | |--I2C--|-/ -|--I2C--|| |I2C master | | I2C mux | | I2C slave | |___| || || So when tuner is called via I2C, it needs recursively call same I2C adapter which is already locked. More elegant solution would be indeed nice. So, AFAIU this is the same problem that I2C based mux devices have (like drivers/i2c/muxes/i2c-mux-pca954x.c)? They also use the unlocked transfers... But those are all called with the lock for the adapter being held. I'm not convinced this is the same for this patch. This patch seems to add a device mutex which protects against concurrent access to the bus with the device itself, but not against concurrent access with other devices on the same bus. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/66] rtl2830: implement own I2C locking
Em Tue, 23 Dec 2014 22:49:14 +0200
Antti Palosaari cr...@iki.fi escreveu:
Own I2C locking is needed due to two special reasons:
1) Chips uses multiple register pages/banks on single I2C slave.
Page is changed via I2C register access.
2) Chip offers muxed/gated I2C adapter for tuner. Gate/mux is
controlled by I2C register access.
Due to these reasons, I2C locking did not fit very well.
I don't like the idea of calling __i2c_transfer() without calling first
i2c_lock_adapter(). This can be dangerous, as the I2C core itself uses
the lock for its own usage.
Ok, this may eventually work ok for now, but a further change at the I2C
core could easily break it. So, we need to double check about such
patch with the I2C maintainer.
Jean,
Are you ok with such patch? If so, please ack.
Regards,
Mauro
Signed-off-by: Antti Palosaari cr...@iki.fi
---
drivers/media/dvb-frontends/rtl2830.c | 45
+-
drivers/media/dvb-frontends/rtl2830_priv.h | 1 +
2 files changed, 39 insertions(+), 7 deletions(-)
diff --git a/drivers/media/dvb-frontends/rtl2830.c
b/drivers/media/dvb-frontends/rtl2830.c
index 8abaca6..3a9e4e9 100644
--- a/drivers/media/dvb-frontends/rtl2830.c
+++ b/drivers/media/dvb-frontends/rtl2830.c
@@ -43,7 +43,7 @@ static int rtl2830_wr(struct i2c_client *client, u8 reg,
const u8 *val, int len)
buf[0] = reg;
memcpy(buf[1], val, len);
- ret = i2c_transfer(client-adapter, msg, 1);
+ ret = __i2c_transfer(client-adapter, msg, 1);
if (ret == 1) {
ret = 0;
} else {
@@ -73,7 +73,7 @@ static int rtl2830_rd(struct i2c_client *client, u8 reg, u8
*val, int len)
}
};
- ret = i2c_transfer(client-adapter, msg, 2);
+ ret = __i2c_transfer(client-adapter, msg, 2);
if (ret == 2) {
ret = 0;
} else {
@@ -93,16 +93,23 @@ static int rtl2830_wr_regs(struct i2c_client *client, u16
reg, const u8 *val, in
u8 reg2 = (reg 0) 0xff;
u8 page = (reg 8) 0xff;
+ mutex_lock(dev-i2c_mutex);
+
/* switch bank if needed */
if (page != dev-page) {
ret = rtl2830_wr(client, 0x00, page, 1);
if (ret)
- return ret;
+ goto err_mutex_unlock;
dev-page = page;
}
- return rtl2830_wr(client, reg2, val, len);
+ ret = rtl2830_wr(client, reg2, val, len);
+
+err_mutex_unlock:
+ mutex_unlock(dev-i2c_mutex);
+
+ return ret;
}
/* read multiple registers */
@@ -113,16 +120,23 @@ static int rtl2830_rd_regs(struct i2c_client *client,
u16 reg, u8 *val, int len)
u8 reg2 = (reg 0) 0xff;
u8 page = (reg 8) 0xff;
+ mutex_lock(dev-i2c_mutex);
+
/* switch bank if needed */
if (page != dev-page) {
ret = rtl2830_wr(client, 0x00, page, 1);
if (ret)
- return ret;
+ goto err_mutex_unlock;
dev-page = page;
}
- return rtl2830_rd(client, reg2, val, len);
+ ret = rtl2830_rd(client, reg2, val, len);
+
+err_mutex_unlock:
+ mutex_unlock(dev-i2c_mutex);
+
+ return ret;
}
/* read single register */
@@ -815,6 +829,10 @@ static int rtl2830_select(struct i2c_adapter *adap, void
*mux_priv, u32 chan_id)
};
int ret;
+ dev_dbg(client-dev, \n);
+
+ mutex_lock(dev-i2c_mutex);
+
/* select register page */
ret = __i2c_transfer(client-adapter, select_reg_page_msg, 1);
if (ret != 1) {
@@ -841,6 +859,18 @@ err:
return ret;
}
+static int rtl2830_deselect(struct i2c_adapter *adap, void *mux_priv, u32
chan)
+{
+ struct i2c_client *client = mux_priv;
+ struct rtl2830_dev *dev = i2c_get_clientdata(client);
+
+ dev_dbg(client-dev, \n);
+
+ mutex_unlock(dev-i2c_mutex);
+
+ return 0;
+}
+
static struct dvb_frontend *rtl2830_get_dvb_frontend(struct i2c_client
*client)
{
struct rtl2830_dev *dev = i2c_get_clientdata(client);
@@ -886,6 +916,7 @@ static int rtl2830_probe(struct i2c_client *client,
dev-client = client;
dev-pdata = client-dev.platform_data;
dev-sleeping = true;
+ mutex_init(dev-i2c_mutex);
INIT_DELAYED_WORK(dev-stat_work, rtl2830_stat_work);
/* check if the demod is there */
@@ -895,7 +926,7 @@ static int rtl2830_probe(struct i2c_client *client,
/* create muxed i2c adapter for tuner */
dev-adapter = i2c_add_mux_adapter(client-adapter, client-dev,
- client, 0, 0, 0, rtl2830_select, NULL);
+ client, 0, 0, 0, rtl2830_select, rtl2830_deselect);
if (dev-adapter == NULL) {
ret = -ENODEV;
goto err_kfree;
diff --git a/drivers/media/dvb-frontends/rtl2830_priv.h
b/drivers/media/dvb-frontends/rtl2830_priv.h
index 2931889..517758a 100644
---
Re: [PATCH 21/66] rtl2830: implement own I2C locking
On 02/02/2015 10:07 PM, Mauro Carvalho Chehab wrote:
Em Tue, 23 Dec 2014 22:49:14 +0200
Antti Palosaari cr...@iki.fi escreveu:
Own I2C locking is needed due to two special reasons:
1) Chips uses multiple register pages/banks on single I2C slave.
Page is changed via I2C register access.
2) Chip offers muxed/gated I2C adapter for tuner. Gate/mux is
controlled by I2C register access.
Due to these reasons, I2C locking did not fit very well.
I don't like the idea of calling __i2c_transfer() without calling first
i2c_lock_adapter(). This can be dangerous, as the I2C core itself uses
the lock for its own usage.
Ok, this may eventually work ok for now, but a further change at the I2C
core could easily break it. So, we need to double check about such
patch with the I2C maintainer.
Jean,
Are you ok with such patch? If so, please ack.
Basic problem here is that I2C-mux itself is controlled by same I2C
device which implements I2C adapter for tuner.
Here is what connections looks like:
___
| USB IF | | demod| |tuner |
|---| || ||
| |--I2C--|-/ -|--I2C--||
|I2C master | | I2C mux | | I2C slave |
|___| || ||
So when tuner is called via I2C, it needs recursively call same I2C
adapter which is already locked. More elegant solution would be indeed nice.
regards
Antti
Regards,
Mauro
Signed-off-by: Antti Palosaari cr...@iki.fi
---
drivers/media/dvb-frontends/rtl2830.c | 45 +-
drivers/media/dvb-frontends/rtl2830_priv.h | 1 +
2 files changed, 39 insertions(+), 7 deletions(-)
diff --git a/drivers/media/dvb-frontends/rtl2830.c
b/drivers/media/dvb-frontends/rtl2830.c
index 8abaca6..3a9e4e9 100644
--- a/drivers/media/dvb-frontends/rtl2830.c
+++ b/drivers/media/dvb-frontends/rtl2830.c
@@ -43,7 +43,7 @@ static int rtl2830_wr(struct i2c_client *client, u8 reg,
const u8 *val, int len)
buf[0] = reg;
memcpy(buf[1], val, len);
- ret = i2c_transfer(client-adapter, msg, 1);
+ ret = __i2c_transfer(client-adapter, msg, 1);
if (ret == 1) {
ret = 0;
} else {
@@ -73,7 +73,7 @@ static int rtl2830_rd(struct i2c_client *client, u8 reg, u8
*val, int len)
}
};
- ret = i2c_transfer(client-adapter, msg, 2);
+ ret = __i2c_transfer(client-adapter, msg, 2);
if (ret == 2) {
ret = 0;
} else {
@@ -93,16 +93,23 @@ static int rtl2830_wr_regs(struct i2c_client *client, u16
reg, const u8 *val, in
u8 reg2 = (reg 0) 0xff;
u8 page = (reg 8) 0xff;
+ mutex_lock(dev-i2c_mutex);
+
/* switch bank if needed */
if (page != dev-page) {
ret = rtl2830_wr(client, 0x00, page, 1);
if (ret)
- return ret;
+ goto err_mutex_unlock;
dev-page = page;
}
- return rtl2830_wr(client, reg2, val, len);
+ ret = rtl2830_wr(client, reg2, val, len);
+
+err_mutex_unlock:
+ mutex_unlock(dev-i2c_mutex);
+
+ return ret;
}
/* read multiple registers */
@@ -113,16 +120,23 @@ static int rtl2830_rd_regs(struct i2c_client *client, u16
reg, u8 *val, int len)
u8 reg2 = (reg 0) 0xff;
u8 page = (reg 8) 0xff;
+ mutex_lock(dev-i2c_mutex);
+
/* switch bank if needed */
if (page != dev-page) {
ret = rtl2830_wr(client, 0x00, page, 1);
if (ret)
- return ret;
+ goto err_mutex_unlock;
dev-page = page;
}
- return rtl2830_rd(client, reg2, val, len);
+ ret = rtl2830_rd(client, reg2, val, len);
+
+err_mutex_unlock:
+ mutex_unlock(dev-i2c_mutex);
+
+ return ret;
}
/* read single register */
@@ -815,6 +829,10 @@ static int rtl2830_select(struct i2c_adapter *adap, void
*mux_priv, u32 chan_id)
};
int ret;
+ dev_dbg(client-dev, \n);
+
+ mutex_lock(dev-i2c_mutex);
+
/* select register page */
ret = __i2c_transfer(client-adapter, select_reg_page_msg, 1);
if (ret != 1) {
@@ -841,6 +859,18 @@ err:
return ret;
}
+static int rtl2830_deselect(struct i2c_adapter *adap, void *mux_priv, u32 chan)
+{
+ struct i2c_client *client = mux_priv;
+ struct rtl2830_dev *dev = i2c_get_clientdata(client);
+
+ dev_dbg(client-dev, \n);
+
+ mutex_unlock(dev-i2c_mutex);
+
+ return 0;
+}
+
static struct dvb_frontend *rtl2830_get_dvb_frontend(struct i2c_client
*client)
{
struct rtl2830_dev *dev = i2c_get_clientdata(client);
@@ -886,6 +916,7 @@ static int rtl2830_probe(struct i2c_client *client,
dev-client = client;
dev-pdata = client-dev.platform_data;
dev-sleeping = true;
Re: [PATCH 21/66] rtl2830: implement own I2C locking
Ok, this may eventually work ok for now, but a further change at the I2C core could easily break it. So, we need to double check about such patch with the I2C maintainer. Jean, Are you ok with such patch? If so, please ack. Jean handed over I2C to me in late 2012 :) Basic problem here is that I2C-mux itself is controlled by same I2C device which implements I2C adapter for tuner. Here is what connections looks like: ___ | USB IF | | demod| |tuner | |---| || || | |--I2C--|-/ -|--I2C--|| |I2C master | | I2C mux | | I2C slave | |___| || || So when tuner is called via I2C, it needs recursively call same I2C adapter which is already locked. More elegant solution would be indeed nice. So, AFAIU this is the same problem that I2C based mux devices have (like drivers/i2c/muxes/i2c-mux-pca954x.c)? They also use the unlocked transfers... signature.asc Description: Digital signature
Re: [PATCH 21/66] rtl2830: implement own I2C locking
Hi Mauro, Antti, On Mon, 2 Feb 2015 18:07:26 -0200, Mauro Carvalho Chehab wrote: Em Tue, 23 Dec 2014 22:49:14 +0200 Antti Palosaari cr...@iki.fi escreveu: Own I2C locking is needed due to two special reasons: 1) Chips uses multiple register pages/banks on single I2C slave. Page is changed via I2C register access. This is no good reason to implement your own i2c bus locking. Lots of i2c slave device work that way, and the way to handle it is through a dedicated lock at the i2c slave device level. This is in addition to the standard i2c bus locking and not a replacement. 2) Chip offers muxed/gated I2C adapter for tuner. Gate/mux is controlled by I2C register access. This, OTOH, is a valid reason for calling __i2c_transfer, and as a matter of fact a number of dvb frontend drivers already do so. Due to these reasons, I2C locking did not fit very well. I don't like the idea of calling __i2c_transfer() without calling first i2c_lock_adapter(). This can be dangerous, as the I2C core itself uses the lock for its own usage. I think the idea is that the i2c bus lock is already held at the time the muxing code is called. This happens each time the I2C muxing chip is an I2C chip itself. Ok, this may eventually work ok for now, but a further change at the I2C core could easily break it. So, we need to double check about such patch with the I2C maintainer. If it breaks than it'll break a dozen drivers which are already doing that, not just this one. But it's OK, I don't see this happening soon. Jean, Are you ok with such patch? If so, please ack. First of all: I am no longer the maintainer of the I2C subsystem. That being said... The changes look OK to me. I think it's how they are presented which make them look suspect. As I understand it, the extra locking at device level is unrelated with calling unlocked i2c transfer functions. The former change is to address the multi-page/bank register mapping, while the latter is to solve the deadlock due to the i2c bus topology and i2c-based muxing. If I am correct then it would be clearer to make that two separate patches with better descriptions. And if I'm wrong then the patch needs a better description too ;-) -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
