* [PATCH 1/5] i2c: Don't start transfers when suspended
@ 2014-07-12 11:49 Bastian Hecht
[not found] ` <1405165771-8732-1-git-send-email-hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Bastian Hecht @ 2014-07-12 11:49 UTC (permalink / raw)
To: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang
Cc: Linux-SH, Tomoya MORINAGA, Naveen Krishna Chatradhi, Taekgyun Ko,
Ben Dooks, Magnus Damm
i2c transfer requests come in very uncontrolled, like from interrupt routines.
We might be suspended when this happens. To avoid i2c timeouts caused by
powered down busses we check for suspension.
Signed-off-by: Bastian Hecht <hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
I ended up getting i2c timeouts when suspending and provoking touchscreen
IRQs on an Armadillo board with an sh_mobile i2c bus. Instead of copying
again the protection mechanism from other bus drivers, we can clean things
up and move this into the core.
I assume that all bus drivers use kzalloc or equivalent for the
struct i2c_adapter. So adap->suspended can't end up uninitialized,
right?
We could consider using this scheme for freeze/restore too.
drivers/i2c/i2c-core.c | 24 +++++++++++++++++++++++-
include/linux/i2c.h | 1 +
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7c7f4b8..9fe9581 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -343,6 +343,13 @@ static int i2c_legacy_resume(struct device *dev)
static int i2c_device_pm_suspend(struct device *dev)
{
const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+ struct i2c_adapter *adap = i2c_verify_adapter(dev);
+
+ if (adap) {
+ i2c_lock_adapter(adap);
+ adap->suspended = true;
+ i2c_unlock_adapter(adap);
+ }
if (pm)
return pm_generic_suspend(dev);
@@ -353,6 +360,13 @@ static int i2c_device_pm_suspend(struct device *dev)
static int i2c_device_pm_resume(struct device *dev)
{
const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+ struct i2c_adapter *adap = i2c_verify_adapter(dev);
+
+ if (adap) {
+ i2c_lock_adapter(adap);
+ adap->suspended = false;
+ i2c_unlock_adapter(adap);
+ }
if (pm)
return pm_generic_resume(dev);
@@ -1819,7 +1833,10 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
i2c_lock_adapter(adap);
}
- ret = __i2c_transfer(adap, msgs, num);
+ if (adap->suspended)
+ ret = -EIO;
+ else
+ ret = __i2c_transfer(adap, msgs, num);
i2c_unlock_adapter(adap);
return ret;
@@ -2577,6 +2594,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
if (adapter->algo->smbus_xfer) {
i2c_lock_adapter(adapter);
+ if (adapter->suspended) {
+ res = -EIO;
+ goto unlock;
+ }
/* Retry automatically on arbitration loss */
orig_jiffies = jiffies;
@@ -2590,6 +2611,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
orig_jiffies + adapter->timeout))
break;
}
+unlock:
i2c_unlock_adapter(adapter);
if (res != -EOPNOTSUPP || !adapter->algo->master_xfer)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..af08c75 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -434,6 +434,7 @@ struct i2c_adapter {
int timeout; /* in jiffies */
int retries;
struct device dev; /* the adapter device */
+ unsigned int suspended:1;
int nr;
char name[48];
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/5] i2c: eg20t: Remove suspension check
[not found] ` <1405165771-8732-1-git-send-email-hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2014-07-12 11:49 ` Bastian Hecht
2014-07-12 11:49 ` [PATCH 3/5] i2c: exynos5: " Bastian Hecht
2014-07-12 11:49 ` [PATCH 5/5] i2c: tegra: " Bastian Hecht
2 siblings, 0 replies; 8+ messages in thread
From: Bastian Hecht @ 2014-07-12 11:49 UTC (permalink / raw)
To: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang
Cc: Linux-SH, Tomoya MORINAGA, Naveen Krishna Chatradhi, Taekgyun Ko,
Ben Dooks, Magnus Damm
We now take care of suspension in the i2c core code. So we can remove this
check here.
Signed-off-by: Bastian Hecht <hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/i2c/busses/i2c-eg20t.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
index a44ea13..aae0413 100644
--- a/drivers/i2c/busses/i2c-eg20t.c
+++ b/drivers/i2c/busses/i2c-eg20t.c
@@ -162,15 +162,12 @@ struct i2c_algo_pch_data {
* struct adapter_info - This structure holds the adapter information for the
PCH i2c controller
* @pch_data: stores a list of i2c_algo_pch_data
- * @pch_i2c_suspended: specifies whether the system is suspended or not
- * perhaps with more lines and words.
* @ch_num: specifies the number of i2c instance
*
* pch_data has as many elements as maximum I2C channels
*/
struct adapter_info {
struct i2c_algo_pch_data pch_data[PCH_I2C_MAX_DEV];
- bool pch_i2c_suspended;
int ch_num;
};
@@ -677,13 +674,6 @@ static s32 pch_i2c_xfer(struct i2c_adapter *i2c_adap,
if (ret)
return ret;
- if (adap->p_adapter_info->pch_i2c_suspended) {
- mutex_unlock(&pch_mutex);
- return -EBUSY;
- }
-
- pch_dbg(adap, "adap->p_adapter_info->pch_i2c_suspended is %d\n",
- adap->p_adapter_info->pch_i2c_suspended);
/* transfer not completed */
adap->pch_i2c_xfer_in_progress = true;
@@ -786,7 +776,6 @@ static int pch_i2c_probe(struct pci_dev *pdev,
for (i = 0; i < adap_info->ch_num; i++) {
pch_adap = &adap_info->pch_data[i].pch_adapter;
- adap_info->pch_i2c_suspended = false;
adap_info->pch_data[i].p_adapter_info = adap_info;
@@ -862,8 +851,6 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
struct adapter_info *adap_info = pci_get_drvdata(pdev);
void __iomem *p = adap_info->pch_data[0].pch_base_address;
- adap_info->pch_i2c_suspended = true;
-
for (i = 0; i < adap_info->ch_num; i++) {
while ((adap_info->pch_data[i].pch_i2c_xfer_in_progress)) {
/* Wait until all channel transfers are completed */
@@ -912,8 +899,6 @@ static int pch_i2c_resume(struct pci_dev *pdev)
for (i = 0; i < adap_info->ch_num; i++)
pch_i2c_init(&adap_info->pch_data[i]);
- adap_info->pch_i2c_suspended = false;
-
return 0;
}
#else
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/5] i2c: exynos5: Remove suspension check
[not found] ` <1405165771-8732-1-git-send-email-hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-07-12 11:49 ` [PATCH 2/5] i2c: eg20t: Remove suspension check Bastian Hecht
@ 2014-07-12 11:49 ` Bastian Hecht
2014-07-12 11:49 ` [PATCH 5/5] i2c: tegra: " Bastian Hecht
2 siblings, 0 replies; 8+ messages in thread
From: Bastian Hecht @ 2014-07-12 11:49 UTC (permalink / raw)
To: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang
Cc: Linux-SH, Tomoya MORINAGA, Naveen Krishna Chatradhi, Taekgyun Ko,
Ben Dooks, Magnus Damm
We now take care of suspension in the i2c core code. So we can remove this
check here.
Signed-off-by: Bastian Hecht <hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/i2c/busses/i2c-exynos5.c | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index 63d2292..a80cf28 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -145,7 +145,6 @@
struct exynos5_i2c {
struct i2c_adapter adap;
- unsigned int suspended:1;
struct i2c_msg *msg;
struct completion msg_complete;
@@ -610,11 +609,6 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap,
struct exynos5_i2c *i2c = adap->algo_data;
int i = 0, ret = 0, stop = 0;
- if (i2c->suspended) {
- dev_err(i2c->dev, "HS-I2C is not initialized.\n");
- return -EIO;
- }
-
clk_prepare_enable(i2c->clk);
for (i = 0; i < num; i++, msgs++) {
@@ -757,16 +751,6 @@ static int exynos5_i2c_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM_SLEEP
-static int exynos5_i2c_suspend_noirq(struct device *dev)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
-
- i2c->suspended = 1;
-
- return 0;
-}
-
static int exynos5_i2c_resume_noirq(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -783,14 +767,12 @@ static int exynos5_i2c_resume_noirq(struct device *dev)
exynos5_i2c_init(i2c);
clk_disable_unprepare(i2c->clk);
- i2c->suspended = 0;
return 0;
}
#endif
-static SIMPLE_DEV_PM_OPS(exynos5_i2c_dev_pm_ops, exynos5_i2c_suspend_noirq,
- exynos5_i2c_resume_noirq);
+static SIMPLE_DEV_PM_OPS(exynos5_i2c_dev_pm_ops, exynos5_i2c_resume_noirq);
static struct platform_driver exynos5_i2c_driver = {
.probe = exynos5_i2c_probe,
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/5] i2c: sc3c2410: Remove suspension check
2014-07-12 11:49 [PATCH 1/5] i2c: Don't start transfers when suspended Bastian Hecht
[not found] ` <1405165771-8732-1-git-send-email-hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2014-07-12 11:49 ` Bastian Hecht
2014-07-17 13:00 ` [PATCH 1/5] i2c: Don't start transfers when suspended Wolfram Sang
2 siblings, 0 replies; 8+ messages in thread
From: Bastian Hecht @ 2014-07-12 11:49 UTC (permalink / raw)
To: linux-i2c, Wolfram Sang
Cc: Linux-SH, Tomoya MORINAGA, Naveen Krishna Chatradhi, Taekgyun Ko,
Ben Dooks, Magnus Damm
We now take care of suspension in the i2c core code. So we can remove this
check here.
Signed-off-by: Bastian Hecht <hechtb@gmail.com>
---
drivers/i2c/busses/i2c-s3c2410.c | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index e828a1d..568b993 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -103,7 +103,6 @@ enum s3c24xx_i2c_state {
struct s3c24xx_i2c {
wait_queue_head_t wait;
kernel_ulong_t quirks;
- unsigned int suspended:1;
struct i2c_msg *msg;
unsigned int msg_num;
@@ -714,9 +713,6 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
unsigned long timeout;
int ret;
- if (i2c->suspended)
- return -EIO;
-
ret = s3c24xx_i2c_set_master(i2c);
if (ret != 0) {
dev_err(i2c->dev, "cannot get bus (error %d)\n", ret);
@@ -1257,16 +1253,6 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM_SLEEP
-static int s3c24xx_i2c_suspend_noirq(struct device *dev)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
-
- i2c->suspended = 1;
-
- return 0;
-}
-
static int s3c24xx_i2c_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -1275,7 +1261,6 @@ static int s3c24xx_i2c_resume(struct device *dev)
clk_prepare_enable(i2c->clk);
s3c24xx_i2c_init(i2c);
clk_disable_unprepare(i2c->clk);
- i2c->suspended = 0;
return 0;
}
@@ -1284,7 +1269,6 @@ static int s3c24xx_i2c_resume(struct device *dev)
#ifdef CONFIG_PM
static const struct dev_pm_ops s3c24xx_i2c_dev_pm_ops = {
#ifdef CONFIG_PM_SLEEP
- .suspend_noirq = s3c24xx_i2c_suspend_noirq,
.resume = s3c24xx_i2c_resume,
#endif
};
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 5/5] i2c: tegra: Remove suspension check
[not found] ` <1405165771-8732-1-git-send-email-hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-07-12 11:49 ` [PATCH 2/5] i2c: eg20t: Remove suspension check Bastian Hecht
2014-07-12 11:49 ` [PATCH 3/5] i2c: exynos5: " Bastian Hecht
@ 2014-07-12 11:49 ` Bastian Hecht
2 siblings, 0 replies; 8+ messages in thread
From: Bastian Hecht @ 2014-07-12 11:49 UTC (permalink / raw)
To: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang
Cc: Linux-SH, Tomoya MORINAGA, Naveen Krishna Chatradhi, Taekgyun Ko,
Ben Dooks, Magnus Damm
We now take care of suspension in the i2c core code. So we can remove this
check here.
Signed-off-by: Bastian Hecht <hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
drivers/i2c/busses/i2c-tegra.c | 19 +------------------
1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index f1bb2fc..c279d85 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -172,7 +172,6 @@ struct tegra_i2c_dev {
size_t msg_buf_remaining;
int msg_read;
u32 bus_clk_rate;
- bool is_suspended;
};
static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
@@ -628,9 +627,6 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
int i;
int ret = 0;
- if (i2c_dev->is_suspended)
- return -EBUSY;
-
ret = tegra_i2c_clock_enable(i2c_dev);
if (ret < 0) {
dev_err(i2c_dev->dev, "Clock enable failed %d\n", ret);
@@ -817,17 +813,6 @@ static int tegra_i2c_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM_SLEEP
-static int tegra_i2c_suspend(struct device *dev)
-{
- struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
-
- i2c_lock_adapter(&i2c_dev->adapter);
- i2c_dev->is_suspended = true;
- i2c_unlock_adapter(&i2c_dev->adapter);
-
- return 0;
-}
-
static int tegra_i2c_resume(struct device *dev)
{
struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
@@ -842,14 +827,12 @@ static int tegra_i2c_resume(struct device *dev)
return ret;
}
- i2c_dev->is_suspended = false;
-
i2c_unlock_adapter(&i2c_dev->adapter);
return 0;
}
-static SIMPLE_DEV_PM_OPS(tegra_i2c_pm, tegra_i2c_suspend, tegra_i2c_resume);
+static SIMPLE_DEV_PM_OPS(tegra_i2c_pm, tegra_i2c_resume);
#define TEGRA_I2C_PM (&tegra_i2c_pm)
#else
#define TEGRA_I2C_PM NULL
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/5] i2c: Don't start transfers when suspended
2014-07-12 11:49 [PATCH 1/5] i2c: Don't start transfers when suspended Bastian Hecht
[not found] ` <1405165771-8732-1-git-send-email-hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-07-12 11:49 ` [PATCH 4/5] i2c: sc3c2410: " Bastian Hecht
@ 2014-07-17 13:00 ` Wolfram Sang
2014-07-17 13:04 ` Wolfram Sang
2014-07-17 14:34 ` Bastian Hecht
2 siblings, 2 replies; 8+ messages in thread
From: Wolfram Sang @ 2014-07-17 13:00 UTC (permalink / raw)
To: Bastian Hecht
Cc: linux-i2c, Linux-SH, Tomoya MORINAGA, Naveen Krishna Chatradhi,
Taekgyun Ko, Ben Dooks, Magnus Damm
[-- Attachment #1: Type: text/plain, Size: 3996 bytes --]
Hi Bastian,
On Sat, Jul 12, 2014 at 01:49:27PM +0200, Bastian Hecht wrote:
> i2c transfer requests come in very uncontrolled, like from interrupt routines.
> We might be suspended when this happens. To avoid i2c timeouts caused by
> powered down busses we check for suspension.
>
> Signed-off-by: Bastian Hecht <hechtb@gmail.com>
Thanks for doing this! I add linux-pm to CC because of two questions.
> ---
> I ended up getting i2c timeouts when suspending and provoking touchscreen
> IRQs on an Armadillo board with an sh_mobile i2c bus. Instead of copying
> again the protection mechanism from other bus drivers, we can clean things
> up and move this into the core.
This could be part of the commit message IMO.
>
> I assume that all bus drivers use kzalloc or equivalent for the
> struct i2c_adapter. So adap->suspended can't end up uninitialized,
> right?
Better safe than sorry, I'd say. Please add the initialization.
> We could consider using this scheme for freeze/restore too.
linux-pm: opinions?
>
>
> drivers/i2c/i2c-core.c | 24 +++++++++++++++++++++++-
> include/linux/i2c.h | 1 +
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 7c7f4b8..9fe9581 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -343,6 +343,13 @@ static int i2c_legacy_resume(struct device *dev)
> static int i2c_device_pm_suspend(struct device *dev)
> {
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> + struct i2c_adapter *adap = i2c_verify_adapter(dev);
> +
> + if (adap) {
> + i2c_lock_adapter(adap);
> + adap->suspended = true;
> + i2c_unlock_adapter(adap);
> + }
>
> if (pm)
> return pm_generic_suspend(dev);
> @@ -353,6 +360,13 @@ static int i2c_device_pm_suspend(struct device *dev)
> static int i2c_device_pm_resume(struct device *dev)
> {
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> + struct i2c_adapter *adap = i2c_verify_adapter(dev);
> +
> + if (adap) {
> + i2c_lock_adapter(adap);
> + adap->suspended = false;
> + i2c_unlock_adapter(adap);
> + }
>
> if (pm)
> return pm_generic_resume(dev);
> @@ -1819,7 +1833,10 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> i2c_lock_adapter(adap);
> }
>
> - ret = __i2c_transfer(adap, msgs, num);
> + if (adap->suspended)
> + ret = -EIO;
> + else
> + ret = __i2c_transfer(adap, msgs, num);
a) Minor: I'd swap the branches and invert the condition, so that the
more expected code path comes first
b) linux-pm: I wonder about the return code. Currently, some drivers
return -EIO, some -EBUSY. Do you care which? Since we try to have
designated fault codes in the i2c subsystem, I wonder about -ESHUTDOWN?
Or is this too much of hijacking fault codes?
> i2c_unlock_adapter(adap);
>
> return ret;
> @@ -2577,6 +2594,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
>
> if (adapter->algo->smbus_xfer) {
> i2c_lock_adapter(adapter);
> + if (adapter->suspended) {
> + res = -EIO;
> + goto unlock;
> + }
>
> /* Retry automatically on arbitration loss */
> orig_jiffies = jiffies;
> @@ -2590,6 +2611,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
> orig_jiffies + adapter->timeout))
> break;
> }
> +unlock:
> i2c_unlock_adapter(adapter);
>
> if (res != -EOPNOTSUPP || !adapter->algo->master_xfer)
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index b556e0a..af08c75 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -434,6 +434,7 @@ struct i2c_adapter {
> int timeout; /* in jiffies */
> int retries;
> struct device dev; /* the adapter device */
> + unsigned int suspended:1;
>
> int nr;
> char name[48];
> --
> 1.9.1
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/5] i2c: Don't start transfers when suspended
2014-07-17 13:00 ` [PATCH 1/5] i2c: Don't start transfers when suspended Wolfram Sang
@ 2014-07-17 13:04 ` Wolfram Sang
2014-07-17 14:34 ` Bastian Hecht
1 sibling, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2014-07-17 13:04 UTC (permalink / raw)
To: Bastian Hecht
Cc: linux-i2c, Linux-SH, Tomoya MORINAGA, Naveen Krishna Chatradhi,
Taekgyun Ko, Ben Dooks, Magnus Damm
[-- Attachment #1: Type: text/plain, Size: 636 bytes --]
On Thu, Jul 17, 2014 at 03:00:48PM +0200, Wolfram Sang wrote:
> Hi Bastian,
>
> On Sat, Jul 12, 2014 at 01:49:27PM +0200, Bastian Hecht wrote:
> > i2c transfer requests come in very uncontrolled, like from interrupt routines.
> > We might be suspended when this happens. To avoid i2c timeouts caused by
> > powered down busses we check for suspension.
> >
> > Signed-off-by: Bastian Hecht <hechtb@gmail.com>
>
> Thanks for doing this! I add linux-pm to CC because of two questions.
And in case of resending, please add at least alkml to the CC list. Or
better: use 'get_maintainer.pl' as cc-cmd with git-send-email.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/5] i2c: Don't start transfers when suspended
2014-07-17 13:00 ` [PATCH 1/5] i2c: Don't start transfers when suspended Wolfram Sang
2014-07-17 13:04 ` Wolfram Sang
@ 2014-07-17 14:34 ` Bastian Hecht
1 sibling, 0 replies; 8+ messages in thread
From: Bastian Hecht @ 2014-07-17 14:34 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Linux-SH, Tomoya MORINAGA,
Naveen Krishna Chatradhi, Taekgyun Ko, Ben Dooks, Magnus Damm
Hi Wolfram,
thanks for the review.
2014-07-17 15:00 GMT+02:00 Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>:
> Hi Bastian,
>
> On Sat, Jul 12, 2014 at 01:49:27PM +0200, Bastian Hecht wrote:
>> i2c transfer requests come in very uncontrolled, like from interrupt routines.
>> We might be suspended when this happens. To avoid i2c timeouts caused by
>> powered down busses we check for suspension.
>>
>> Signed-off-by: Bastian Hecht <hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Thanks for doing this! I add linux-pm to CC because of two questions.
>
> And in case of resending, please add at least alkml to the CC list. Or
> better: use 'get_maintainer.pl' as cc-cmd with git-send-email.
Wow these scripts! So useful - thanks for the hint. I looked up the
few people I added manually in the drivers.
>> ---
>> I ended up getting i2c timeouts when suspending and provoking touchscreen
>> IRQs on an Armadillo board with an sh_mobile i2c bus. Instead of copying
>> again the protection mechanism from other bus drivers, we can clean things
>> up and move this into the core.
>
> This could be part of the commit message IMO.
Ok I added it partly. I didn't include the Armadillo specific part,
but added the general
idea to move existing protection schemes to the core.
>>
>> I assume that all bus drivers use kzalloc or equivalent for the
>> struct i2c_adapter. So adap->suspended can't end up uninitialized,
>> right?
>
> Better safe than sorry, I'd say. Please add the initialization.
Ok done.
>> We could consider using this scheme for freeze/restore too.
>
> linux-pm: opinions?
>
>>
>>
>> drivers/i2c/i2c-core.c | 24 +++++++++++++++++++++++-
>> include/linux/i2c.h | 1 +
>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 7c7f4b8..9fe9581 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -343,6 +343,13 @@ static int i2c_legacy_resume(struct device *dev)
>> static int i2c_device_pm_suspend(struct device *dev)
>> {
>> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> + struct i2c_adapter *adap = i2c_verify_adapter(dev);
>> +
>> + if (adap) {
>> + i2c_lock_adapter(adap);
>> + adap->suspended = true;
>> + i2c_unlock_adapter(adap);
>> + }
>>
>> if (pm)
>> return pm_generic_suspend(dev);
>> @@ -353,6 +360,13 @@ static int i2c_device_pm_suspend(struct device *dev)
>> static int i2c_device_pm_resume(struct device *dev)
>> {
>> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> + struct i2c_adapter *adap = i2c_verify_adapter(dev);
>> +
>> + if (adap) {
>> + i2c_lock_adapter(adap);
>> + adap->suspended = false;
>> + i2c_unlock_adapter(adap);
>> + }
>>
>> if (pm)
>> return pm_generic_resume(dev);
>> @@ -1819,7 +1833,10 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>> i2c_lock_adapter(adap);
>> }
>>
>> - ret = __i2c_transfer(adap, msgs, num);
>> + if (adap->suspended)
>> + ret = -EIO;
>> + else
>> + ret = __i2c_transfer(adap, msgs, num);
>
> a) Minor: I'd swap the branches and invert the condition, so that the
> more expected code path comes first
Oh yes, well seen. I swapped it.
> b) linux-pm: I wonder about the return code. Currently, some drivers
> return -EIO, some -EBUSY. Do you care which? Since we try to have
> designated fault codes in the i2c subsystem, I wonder about -ESHUTDOWN?
> Or is this too much of hijacking fault codes?
I looked up Documentation/i2c/fault-codes and -EIO currently is the
closest blurrily defined fault code to our problem. I agree that
-ESHUTDOWN would look clearer, but as well agree that I'm unsure about
getting too pedantic. I left -EIO for the v2 series for now. If we opt
for -ESHUTDOWN we shouldn't forget to update the docs too.
I will post v2 now but be on the road for probably a month and can't
participate further here. If needed feel free to completely modify
these patches - it's a very basic patchset anyway.
Cheers,
Bastian
>> i2c_unlock_adapter(adap);
>>
>> return ret;
>> @@ -2577,6 +2594,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
>>
>> if (adapter->algo->smbus_xfer) {
>> i2c_lock_adapter(adapter);
>> + if (adapter->suspended) {
>> + res = -EIO;
>> + goto unlock;
>> + }
>>
>> /* Retry automatically on arbitration loss */
>> orig_jiffies = jiffies;
>> @@ -2590,6 +2611,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
>> orig_jiffies + adapter->timeout))
>> break;
>> }
>> +unlock:
>> i2c_unlock_adapter(adapter);
>>
>> if (res != -EOPNOTSUPP || !adapter->algo->master_xfer)
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index b556e0a..af08c75 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -434,6 +434,7 @@ struct i2c_adapter {
>> int timeout; /* in jiffies */
>> int retries;
>> struct device dev; /* the adapter device */
>> + unsigned int suspended:1;
>>
>> int nr;
>> char name[48];
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-07-17 14:34 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-12 11:49 [PATCH 1/5] i2c: Don't start transfers when suspended Bastian Hecht
[not found] ` <1405165771-8732-1-git-send-email-hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2014-07-12 11:49 ` [PATCH 2/5] i2c: eg20t: Remove suspension check Bastian Hecht
2014-07-12 11:49 ` [PATCH 3/5] i2c: exynos5: " Bastian Hecht
2014-07-12 11:49 ` [PATCH 5/5] i2c: tegra: " Bastian Hecht
2014-07-12 11:49 ` [PATCH 4/5] i2c: sc3c2410: " Bastian Hecht
2014-07-17 13:00 ` [PATCH 1/5] i2c: Don't start transfers when suspended Wolfram Sang
2014-07-17 13:04 ` Wolfram Sang
2014-07-17 14:34 ` Bastian Hecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).