Re: [PATCH 1/7] x86/platform/intel/iosf_mbi: Add a mutex for punit access

[Hans de Goede <hdegoede@redhat.com>]

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;
>