| Message ID | 20170101201521.12364-5-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, 2017-01-01 at 21:15 +0100, Hans de Goede wrote: > Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so that > we keep the iosf_mbi_lock locked during the read-modify-write done to > reset the semaphore. > While patch itself looks good to me, I think it reduces a probability to race and doesn't eliminate an issue completely. Can you check i915 code how it's done there? > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v5: > -New patch in v5 of this patch-set > --- > drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c > b/drivers/i2c/busses/i2c-designware-baytrail.c > index 650a700..8df529c 100644 > --- a/drivers/i2c/busses/i2c-designware-baytrail.c > +++ b/drivers/i2c/busses/i2c-designware-baytrail.c > @@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32 > *sem) > > static void reset_semaphore(struct dw_i2c_dev *dev) > { > - u32 data; > - > - if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, > PUNIT_SEMAPHORE, &data)) { > - dev_err(dev->dev, "iosf failed to reset punit > semaphore during read\n"); > - return; > - } > - > - data &= ~PUNIT_SEMAPHORE_BIT; > - if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, > PUNIT_SEMAPHORE, data)) > + if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, > PUNIT_SEMAPHORE, > + 0, PUNIT_SEMAPHORE_BIT)) > dev_err(dev->dev, "iosf failed to reset punit > semaphore during write\n"); > > pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
Hi, On 02-01-17 10:36, Andy Shevchenko wrote: > On Sun, 2017-01-01 at 21:15 +0100, Hans de Goede wrote: >> Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so that >> we keep the iosf_mbi_lock locked during the read-modify-write done to >> reset the semaphore. >> > > While patch itself looks good to me, I think it reduces a probability to > race and doesn't eliminate an issue completely. All accesses through this register are done through the iosf_mbi_ helpers, and the modify helper holds the iosf_mbi_lock during the entire read-modify-wrtie cycle, so I don't see how there can still be any issue left. > Can you check i915 code > how it's done there? AFAIK the i915 code never takes the pmic i2c bus semaphore. Regards, Hans p.s. Since you're interested in this set, you may also be interested in this RFC patch-set: https://www.spinics.net/lists/intel-gfx/msg115833.html > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v5: >> -New patch in v5 of this patch-set >> --- >> drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++--------- >> 1 file changed, 2 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c >> b/drivers/i2c/busses/i2c-designware-baytrail.c >> index 650a700..8df529c 100644 >> --- a/drivers/i2c/busses/i2c-designware-baytrail.c >> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c >> @@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32 >> *sem) >> >> static void reset_semaphore(struct dw_i2c_dev *dev) >> { >> - u32 data; >> - >> - if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, >> PUNIT_SEMAPHORE, &data)) { >> - dev_err(dev->dev, "iosf failed to reset punit >> semaphore during read\n"); >> - return; >> - } >> - >> - data &= ~PUNIT_SEMAPHORE_BIT; >> - if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, >> PUNIT_SEMAPHORE, data)) >> + if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, >> PUNIT_SEMAPHORE, >> + 0, PUNIT_SEMAPHORE_BIT)) >> dev_err(dev->dev, "iosf failed to reset punit >> semaphore during write\n"); >> >> pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE); > -- 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
On Mon, 2017-01-02 at 10:55 +0100, Hans de Goede wrote: > Hi, > > On 02-01-17 10:36, Andy Shevchenko wrote: > > On Sun, 2017-01-01 at 21:15 +0100, Hans de Goede wrote: > > > Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so > > > that > > > we keep the iosf_mbi_lock locked during the read-modify-write done > > > to > > > reset the semaphore. > > > > > > > While patch itself looks good to me, I think it reduces a > > probability to > > race and doesn't eliminate an issue completely. > > All accesses through this register are done through the > iosf_mbi_ helpers, and the modify helper holds the > iosf_mbi_lock during the entire read-modify-wrtie cycle, > so I don't see how there can still be any issue left. Ah, seems you are right. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Can you check i915 code > > how it's done there? > > AFAIK the i915 code never takes the pmic i2c bus semaphore. > > Regards, > > Hans > > p.s. > > Since you're interested in this set, you may also be interested > in this RFC patch-set: > > https://www.spinics.net/lists/intel-gfx/msg115833.html > > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > Changes in v5: > > > -New patch in v5 of this patch-set > > > --- > > > drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++--------- > > > 1 file changed, 2 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c > > > b/drivers/i2c/busses/i2c-designware-baytrail.c > > > index 650a700..8df529c 100644 > > > --- a/drivers/i2c/busses/i2c-designware-baytrail.c > > > +++ b/drivers/i2c/busses/i2c-designware-baytrail.c > > > @@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32 > > > *sem) > > > > > > static void reset_semaphore(struct dw_i2c_dev *dev) > > > { > > > - u32 data; > > > - > > > - if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, > > > PUNIT_SEMAPHORE, &data)) { > > > - dev_err(dev->dev, "iosf failed to reset punit > > > semaphore during read\n"); > > > - return; > > > - } > > > - > > > - data &= ~PUNIT_SEMAPHORE_BIT; > > > - if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, > > > PUNIT_SEMAPHORE, data)) > > > + if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, > > > PUNIT_SEMAPHORE, > > > + 0, PUNIT_SEMAPHORE_BIT)) > > > dev_err(dev->dev, "iosf failed to reset punit > > > semaphore during write\n"); > > > > > > pm_qos_update_request(&dev->pm_qos, > > > PM_QOS_DEFAULT_VALUE);
On 01/02/2017 12:06 PM, Andy Shevchenko wrote: > On Mon, 2017-01-02 at 10:55 +0100, Hans de Goede wrote: >> Hi, >> >> On 02-01-17 10:36, Andy Shevchenko wrote: >>> On Sun, 2017-01-01 at 21:15 +0100, Hans de Goede wrote: >>>> Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so >>>> that >>>> we keep the iosf_mbi_lock locked during the read-modify-write done >>>> to >>>> reset the semaphore. >>>> >>> >>> While patch itself looks good to me, I think it reduces a >>> probability to >>> race and doesn't eliminate an issue completely. >> >> All accesses through this register are done through the >> iosf_mbi_ helpers, and the modify helper holds the >> iosf_mbi_lock during the entire read-modify-wrtie cycle, >> so I don't see how there can still be any issue left. > > Ah, seems you are right. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> -- 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
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c index 650a700..8df529c 100644 --- a/drivers/i2c/busses/i2c-designware-baytrail.c +++ b/drivers/i2c/busses/i2c-designware-baytrail.c @@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32 *sem) static void reset_semaphore(struct dw_i2c_dev *dev) { - u32 data; - - if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data)) { - dev_err(dev->dev, "iosf failed to reset punit semaphore during read\n"); - return; - } - - data &= ~PUNIT_SEMAPHORE_BIT; - if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, data)) + if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, + 0, PUNIT_SEMAPHORE_BIT)) dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n"); pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so that we keep the iosf_mbi_lock locked during the read-modify-write done to reset the semaphore. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v5: -New patch in v5 of this patch-set --- drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)