Re: [PATCH 4/5] platform/x86: msi-ec: Add EC bit operation functions

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

Hi,

On 10/11/23 14:59, Ilpo Järvinen wrote:
> On Tue, 10 Oct 2023, Nikita Kravets wrote:
> 
>> The EC of MSI laptops supports several features represented by a single
>> bit. Add ec_set_bit and ec_check_bit functions to operate on these bits.
>>
>> Cc: Aakash Singh <mail@singhaakash.dev>
>> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
>> Signed-off-by: Nikita Kravets <teackot@gmail.com>
>> ---
>>  drivers/platform/x86/msi-ec.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/platform/x86/msi-ec.c b/drivers/platform/x86/msi-ec.c
>> index 09472b21e093..ae73dcf01d09 100644
>> --- a/drivers/platform/x86/msi-ec.c
>> +++ b/drivers/platform/x86/msi-ec.c
>> @@ -699,6 +699,34 @@ static int ec_read_seq(u8 addr, u8 *buf, u8 len)
>>  	return 0;
>>  }
>>  
>> +static int ec_set_bit(u8 addr, u8 bit, bool value)
>> +{
>> +	int result;
>> +	u8 stored;
>> +
>> +	result = ec_read(addr, &stored);
>> +	if (result < 0)
>> +		return result;
>> +
>> +	stored ^= (-(u8) value ^ stored) & (1 << bit);
> 
> So first you case bool to u8 and then take negation of that unsigned 
> number? ...My head is already hurting even without all the other logic.
> 
> This has to be rewritten to something that mere mortals can understand 
> which doesn't explore all those odd corners of C spec. :-)
> 
> I didn't try to parse that logic through but I assuming it's the usual 
> construct perhaps this could be simplified with (please be sure to check 
> this throughoutly as I didn't try to understand what the original really 
> does):
> 
> 	bit = 1 << bit;
> 	stored &= ~bit;
> 	stored |= value ? bit : 0;

Right instead of using a bit variable I would suggest
using the BIT(x) macro here:

 	stored &= ~BIT(bit);
 	stored |= value ? BIT(bit) : 0;

Also since you are exposing multiple userspace
entry points into the kernel this function may
race with itself, so I think you need to add
a mutex and lock this while doing the
read-modify-write to avoid 2 read-modify-write
cycles from racing with each other.

Regards,

Hans






> 
>> +
>> +	return ec_write(addr, stored);
>> +}
>> +
>> +static int ec_check_bit(u8 addr, u8 bit, bool *output)
>> +{
>> +	int result;
>> +	u8 stored;
>> +
>> +	result = ec_read(addr, &stored);
>> +	if (result < 0)
>> +		return result;
>> +
>> +	*output = (stored >> bit) & 1;
>> +
>> +	return 0;
>> +}
>> +
>>  static int ec_get_firmware_version(u8 buf[MSI_EC_FW_VERSION_LENGTH + 1])
>>  {
>>  	int result;
>>
>