| Message ID | 20170108134427.8392-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote: > One some systems the punit accesses the pmic to change various > voltages > through the same bus as other kernel drivers use for e.g. battery > monitoring. > > If a driver sends requests to the punit which require the punit to > access > the pmic bus while another driver is also accessing the pmic bus > various > bad things happen. > > This commit adds a mutex to protect the punit against simultaneous > accesses > and 2 functions to lock / unlock this mutex. > > Note on these systems the i2c-bus driver will request a sempahore from > the > punit for exclusive access to the pmic bus when i2c drivers are > accessing > it, but this does not appear to be sufficient, we still need to avoid > making certain punit requests during the access window to avoid > problems. I'm fine with the patch, but please spell P-Unit PMIC Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > 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> > --- > arch/x86/include/asm/iosf_mbi.h | 31 > +++++++++++++++++++++++++++++++ > arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/arch/x86/include/asm/iosf_mbi.h > b/arch/x86/include/asm/iosf_mbi.h > index b41ee16..91f5d16 100644 > --- a/arch/x86/include/asm/iosf_mbi.h > +++ b/arch/x86/include/asm/iosf_mbi.h > @@ -88,6 +88,33 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, > u32 mdr); > */ > int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 > mask); > > +/** > + * iosf_mbi_punit_lock() - Lock the punit mutex > + * > + * One some systems the punit accesses the pmic to change various > voltages > + * through the same bus as other kernel drivers use for e.g. battery > monitoring. > + * > + * If a driver sends requests to the punit which require the punit to > access the > + * pmic bus while another driver is also accessing the pmic bus > various bad > + * things happen. > + * > + * To avoid these problems this function must be called before > accessing the > + * punit or the pmic, be it through iosf_mbi* functions or through > other means. > + * > + * Note on these systems the i2c-bus driver will request a sempahore > from the > + * punit for exclusive access to the pmic bus when i2c drivers are > accessing it, > + * but this does not appear to be sufficient, we still need to avoid > making > + * certain punit requests during the access window to avoid problems. > + * > + * This function locks a mutex, as such it may sleep. > + */ > +void iosf_mbi_punit_lock(void); > + > +/** > + * iosf_mbi_punit_unlock() - Unlock the punit mutex > + */ > +void iosf_mbi_punit_unlock(void); > + > #else /* CONFIG_IOSF_MBI is not enabled */ > static inline > bool iosf_mbi_available(void) > @@ -115,6 +142,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 > offset, u32 mdr, u32 mask) > WARN(1, "IOSF_MBI driver not available"); > return -EPERM; > } > + > +static inline void iosf_mbi_punit_lock(void) {} > +static inline void iosf_mbi_punit_unlock(void) {} > + > #endif /* CONFIG_IOSF_MBI */ > > #endif /* IOSF_MBI_SYMS_H */ > diff --git a/arch/x86/platform/intel/iosf_mbi.c > b/arch/x86/platform/intel/iosf_mbi.c > index edf2c54..75d8135 100644 > --- a/arch/x86/platform/intel/iosf_mbi.c > +++ b/arch/x86/platform/intel/iosf_mbi.c > @@ -34,6 +34,7 @@ > > static struct pci_dev *mbi_pdev; > static DEFINE_SPINLOCK(iosf_mbi_lock); > +static DEFINE_MUTEX(iosf_mbi_punit_mutex); > > static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset) > { > @@ -190,6 +191,18 @@ bool iosf_mbi_available(void) > } > EXPORT_SYMBOL(iosf_mbi_available); > > +void iosf_mbi_punit_lock(void) > +{ > + mutex_lock(&iosf_mbi_punit_mutex); > +} > +EXPORT_SYMBOL(iosf_mbi_punit_lock); > + > +void iosf_mbi_punit_unlock(void) > +{ > + mutex_unlock(&iosf_mbi_punit_mutex); > +} > +EXPORT_SYMBOL(iosf_mbi_punit_unlock); > + > #ifdef CONFIG_IOSF_MBI_DEBUG > static u32 dbg_mdr; > static u32 dbg_mcr;
Hi, On 08-01-17 16:16, Andy Shevchenko wrote: > On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote: >> One some systems the punit accesses the pmic to change various >> voltages >> through the same bus as other kernel drivers use for e.g. battery >> monitoring. >> >> If a driver sends requests to the punit which require the punit to >> access >> the pmic bus while another driver is also accessing the pmic bus >> various >> bad things happen. >> >> This commit adds a mutex to protect the punit against simultaneous >> accesses >> and 2 functions to lock / unlock this mutex. >> >> Note on these systems the i2c-bus driver will request a sempahore from >> the >> punit for exclusive access to the pmic bus when i2c drivers are >> accessing >> it, but this does not appear to be sufficient, we still need to avoid >> making certain punit requests during the access window to avoid >> problems. > > I'm fine with the patch, but please spell > P-Unit > PMIC In the commit msg and comments, not in code you mean I assume ? Regards, Hans > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >> >> 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> >> --- >> arch/x86/include/asm/iosf_mbi.h | 31 >> +++++++++++++++++++++++++++++++ >> arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++ >> 2 files changed, 44 insertions(+) >> >> diff --git a/arch/x86/include/asm/iosf_mbi.h >> b/arch/x86/include/asm/iosf_mbi.h >> index b41ee16..91f5d16 100644 >> --- a/arch/x86/include/asm/iosf_mbi.h >> +++ b/arch/x86/include/asm/iosf_mbi.h >> @@ -88,6 +88,33 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, >> u32 mdr); >> */ >> int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 >> mask); >> >> +/** >> + * iosf_mbi_punit_lock() - Lock the punit mutex >> + * >> + * One some systems the punit accesses the pmic to change various >> voltages >> + * through the same bus as other kernel drivers use for e.g. battery >> monitoring. >> + * >> + * If a driver sends requests to the punit which require the punit to >> access the >> + * pmic bus while another driver is also accessing the pmic bus >> various bad >> + * things happen. >> + * >> + * To avoid these problems this function must be called before >> accessing the >> + * punit or the pmic, be it through iosf_mbi* functions or through >> other means. >> + * >> + * Note on these systems the i2c-bus driver will request a sempahore >> from the >> + * punit for exclusive access to the pmic bus when i2c drivers are >> accessing it, >> + * but this does not appear to be sufficient, we still need to avoid >> making >> + * certain punit requests during the access window to avoid problems. >> + * >> + * This function locks a mutex, as such it may sleep. >> + */ >> +void iosf_mbi_punit_lock(void); >> + >> +/** >> + * iosf_mbi_punit_unlock() - Unlock the punit mutex >> + */ >> +void iosf_mbi_punit_unlock(void); >> + >> #else /* CONFIG_IOSF_MBI is not enabled */ >> static inline >> bool iosf_mbi_available(void) >> @@ -115,6 +142,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 >> offset, u32 mdr, u32 mask) >> WARN(1, "IOSF_MBI driver not available"); >> return -EPERM; >> } >> + >> +static inline void iosf_mbi_punit_lock(void) {} >> +static inline void iosf_mbi_punit_unlock(void) {} >> + >> #endif /* CONFIG_IOSF_MBI */ >> >> #endif /* IOSF_MBI_SYMS_H */ >> diff --git a/arch/x86/platform/intel/iosf_mbi.c >> b/arch/x86/platform/intel/iosf_mbi.c >> index edf2c54..75d8135 100644 >> --- a/arch/x86/platform/intel/iosf_mbi.c >> +++ b/arch/x86/platform/intel/iosf_mbi.c >> @@ -34,6 +34,7 @@ >> >> static struct pci_dev *mbi_pdev; >> static DEFINE_SPINLOCK(iosf_mbi_lock); >> +static DEFINE_MUTEX(iosf_mbi_punit_mutex); >> >> static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset) >> { >> @@ -190,6 +191,18 @@ bool iosf_mbi_available(void) >> } >> EXPORT_SYMBOL(iosf_mbi_available); >> >> +void iosf_mbi_punit_lock(void) >> +{ >> + mutex_lock(&iosf_mbi_punit_mutex); >> +} >> +EXPORT_SYMBOL(iosf_mbi_punit_lock); >> + >> +void iosf_mbi_punit_unlock(void) >> +{ >> + mutex_unlock(&iosf_mbi_punit_mutex); >> +} >> +EXPORT_SYMBOL(iosf_mbi_punit_unlock); >> + >> #ifdef CONFIG_IOSF_MBI_DEBUG >> static u32 dbg_mdr; >> static u32 dbg_mcr; >
On Sun, 2017-01-08 at 16:30 +0100, Hans de Goede wrote: > Hi, > > On 08-01-17 16:16, Andy Shevchenko wrote: > > On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote: > > > One some systems the punit accesses the pmic to change various > > > voltages > > > through the same bus as other kernel drivers use for e.g. battery > > > monitoring. > > > > > > If a driver sends requests to the punit which require the punit to > > > access > > > the pmic bus while another driver is also accessing the pmic bus > > > various > > > bad things happen. > > > > > > This commit adds a mutex to protect the punit against simultaneous > > > accesses > > > and 2 functions to lock / unlock this mutex. > > > > > > Note on these systems the i2c-bus driver will request a sempahore > > > from > > > the > > > punit for exclusive access to the pmic bus when i2c drivers are > > > accessing > > > it, but this does not appear to be sufficient, we still need to > > > avoid > > > making certain punit requests during the access window to avoid > > > problems. > > > > I'm fine with the patch, but please spell > > P-Unit > > PMIC > > In the commit msg and comments, not in code you mean I assume ? Correct.
diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h index b41ee16..91f5d16 100644 --- a/arch/x86/include/asm/iosf_mbi.h +++ b/arch/x86/include/asm/iosf_mbi.h @@ -88,6 +88,33 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr); */ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask); +/** + * iosf_mbi_punit_lock() - Lock the punit mutex + * + * One some systems the punit accesses the pmic to change various voltages + * through the same bus as other kernel drivers use for e.g. battery monitoring. + * + * If a driver sends requests to the punit which require the punit to access the + * pmic bus while another driver is also accessing the pmic bus various bad + * things happen. + * + * To avoid these problems this function must be called before accessing the + * punit or the pmic, be it through iosf_mbi* functions or through other means. + * + * Note on these systems the i2c-bus driver will request a sempahore from the + * punit for exclusive access to the pmic bus when i2c drivers are accessing it, + * but this does not appear to be sufficient, we still need to avoid making + * certain punit requests during the access window to avoid problems. + * + * This function locks a mutex, as such it may sleep. + */ +void iosf_mbi_punit_lock(void); + +/** + * iosf_mbi_punit_unlock() - Unlock the punit mutex + */ +void iosf_mbi_punit_unlock(void); + #else /* CONFIG_IOSF_MBI is not enabled */ static inline bool iosf_mbi_available(void) @@ -115,6 +142,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask) WARN(1, "IOSF_MBI driver not available"); return -EPERM; } + +static inline void iosf_mbi_punit_lock(void) {} +static inline void iosf_mbi_punit_unlock(void) {} + #endif /* CONFIG_IOSF_MBI */ #endif /* IOSF_MBI_SYMS_H */ diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c index edf2c54..75d8135 100644 --- a/arch/x86/platform/intel/iosf_mbi.c +++ b/arch/x86/platform/intel/iosf_mbi.c @@ -34,6 +34,7 @@ static struct pci_dev *mbi_pdev; static DEFINE_SPINLOCK(iosf_mbi_lock); +static DEFINE_MUTEX(iosf_mbi_punit_mutex); static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset) { @@ -190,6 +191,18 @@ bool iosf_mbi_available(void) } EXPORT_SYMBOL(iosf_mbi_available); +void iosf_mbi_punit_lock(void) +{ + mutex_lock(&iosf_mbi_punit_mutex); +} +EXPORT_SYMBOL(iosf_mbi_punit_lock); + +void iosf_mbi_punit_unlock(void) +{ + mutex_unlock(&iosf_mbi_punit_mutex); +} +EXPORT_SYMBOL(iosf_mbi_punit_unlock); + #ifdef CONFIG_IOSF_MBI_DEBUG static u32 dbg_mdr; static u32 dbg_mcr;