[3/7] i2c: designware-baytrail: Take punit lock on bus acquire
diff mbox

Message ID 20170108134427.8392-4-hdegoede@redhat.com
State Awaiting Upstream
Headers show

Commit Message

Hans de Goede Jan. 8, 2017, 1:44 p.m. UTC
Take the punit lock to stop others from accessing the punit while the
pmic i2c bus is in use. This is necessary because accessing the punit
from the kernel may result in the punit trying to access the pmic i2c
bus, which results in a hang when it happens while we own the pmic i2c
bus semaphore.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Andy Shevchenko Jan. 8, 2017, 3:27 p.m. UTC | #1
On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> Take the punit lock to stop others from accessing the punit while the
> pmic i2c bus is in use. This is necessary because accessing the punit
> from the kernel may result in the punit trying to access the pmic i2c
> bus, which results in a hang when it happens while we own the pmic i2c
> bus semaphore.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 3effc9a..cf7a2a0 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> 

> @@ -62,6 +62,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)

> +	iosf_mbi_punit_unlock();

> @@ -79,6 +81,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
> +	iosf_mbi_punit_lock();

For me it looks a bit asymmetrical.

Can we use acquire()/release()?
Hans de Goede Jan. 8, 2017, 3:39 p.m. UTC | #2
Hi,

On 08-01-17 16:27, Andy Shevchenko wrote:
> On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
>> Take the punit lock to stop others from accessing the punit while the
>> pmic i2c bus is in use. This is necessary because accessing the punit
>> from the kernel may result in the punit trying to access the pmic i2c
>> bus, which results in a hang when it happens while we own the pmic i2c
>> bus semaphore.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
>> b/drivers/i2c/busses/i2c-designware-baytrail.c
>> index 3effc9a..cf7a2a0 100644
>> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
>> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
>>
>
>> @@ -62,6 +62,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
>
>> +	iosf_mbi_punit_unlock();
>
>> @@ -79,6 +81,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
>> +	iosf_mbi_punit_lock();
>
> For me it looks a bit asymmetrical.
>
> Can we use acquire()/release()?

Sure I can change things to use acquire()/release() instead. I will
change this for v2.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Jan. 12, 2017, 6:45 p.m. UTC | #3
On Sun, Jan 08, 2017 at 02:44:23PM +0100, Hans de Goede wrote:
> Take the punit lock to stop others from accessing the punit while the
> pmic i2c bus is in use. This is necessary because accessing the punit
> from the kernel may result in the punit trying to access the pmic i2c
> bus, which results in a hang when it happens while we own the pmic i2c
> bus semaphore.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>

I don't think the I2C patches need to go via I2C tree, so:

Acked-by: Wolfram Sang <wsa@the-dreams.de>

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede Jan. 15, 2017, 11:21 a.m. UTC | #4
Hi,

On 12-01-17 19:45, Wolfram Sang wrote:
> On Sun, Jan 08, 2017 at 02:44:23PM +0100, Hans de Goede wrote:
>> Take the punit lock to stop others from accessing the punit while the
>> pmic i2c bus is in use. This is necessary because accessing the punit
>> from the kernel may result in the punit trying to access the pmic i2c
>> bus, which results in a hang when it happens while we own the pmic i2c
>> bus semaphore.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>
> I don't think the I2C patches need to go via I2C tree, so:
>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>

Note that as mentioned in the cover-letter, these 2 i2c patches depend
on these:

https://patchwork.ozlabs.org/patch/710067/
https://patchwork.ozlabs.org/patch/710068/
https://patchwork.ozlabs.org/patch/710069/
https://patchwork.ozlabs.org/patch/710070/
https://patchwork.ozlabs.org/patch/710071/
https://patchwork.ozlabs.org/patch/710072/

Which all have many reviews / acks. Given the race between i915
and designware-i2c opened up by the last patch in this series
these not having been merged yet is a good thing, but patch 1-5
are ready for merging.

So Wolfram, what is the plan with these ? As said they are necessary
for the 2 i2c patches in this patch-set, so do you want the
entire set of 8 i2c patches to go through an other tree to avoid
inter tree dependencies ?

And if so, can we then have you're Acked-by for the 6 above too ?

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Jan. 15, 2017, 11:45 a.m. UTC | #5
Hi Hans,

> So Wolfram, what is the plan with these ? As said they are necessary
> for the 2 i2c patches in this patch-set, so do you want the
> entire set of 8 i2c patches to go through an other tree to avoid
> inter tree dependencies ?

Thanks for the heads up. So, my plan was that I send a pull request for
4.10 any minute now, and start pulling in patches for 4.11 based on
shiny new rc4 from tomorrow on. And your series would have been one of
the fist to get merged because I think it is ready.

Reading this though, my feeling is now that it should be merged via some
other tree so you can get these nasty issues fixed without too many
dependencies. An immutable branch for me to pull in would be great,
though.

Makes sense?

Regards,

   Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede Jan. 15, 2017, 3:11 p.m. UTC | #6
Hi,

On 15-01-17 12:45, Wolfram Sang wrote:
> Hi Hans,
>
>> So Wolfram, what is the plan with these ? As said they are necessary
>> for the 2 i2c patches in this patch-set, so do you want the
>> entire set of 8 i2c patches to go through an other tree to avoid
>> inter tree dependencies ?
>
> Thanks for the heads up. So, my plan was that I send a pull request for
> 4.10 any minute now, and start pulling in patches for 4.11 based on
> shiny new rc4 from tomorrow on. And your series would have been one of
> the fist to get merged because I think it is ready.
>
> Reading this though, my feeling is now that it should be merged via some
> other tree so you can get these nasty issues fixed without too many
> dependencies. An immutable branch for me to pull in would be great,
> though.
>
> Makes sense?

Sounds fine to me, step one is to get consensus on how to deal with
coordinating the kernel directly accessing to the pmic-i2c-bus vs punit
accesses which also end up needing to use the same i2c-bus from the
punit side.

Once I've a patch-set everyone likes I will start talking to people
to coordinate the merging, I believe it is probably best for all this
to be merged through the drm-intel tree. But we'll see about that
when the patch-set is ready.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Jan. 15, 2017, 3:17 p.m. UTC | #7
> Once I've a patch-set everyone likes I will start talking to people
> to coordinate the merging, I believe it is probably best for all this
> to be merged through the drm-intel tree. But we'll see about that
> when the patch-set is ready.

Agreed.

Patch
diff mbox

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 3effc9a..cf7a2a0 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -62,6 +62,8 @@  static void reset_semaphore(struct dw_i2c_dev *dev)
 		dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
 
 	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
+
+	iosf_mbi_punit_unlock();
 }
 
 static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
@@ -79,6 +81,8 @@  static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	if (!dev->release_lock)
 		return 0;
 
+	iosf_mbi_punit_lock();
+
 	/*
 	 * Disallow the CPU to enter C6 or C7 state, entering these states
 	 * requires the punit to talk to the pmic and if this happens while