| Message ID | 20170108134427.8392-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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()?
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
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>
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
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
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
> 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.
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