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@xxxxxxxxxxxxxxx>
>> Cc: Jose Angel Pastrana <japp0005@xxxxxxxxxxxx>
>> Signed-off-by: Nikita Kravets <teackot@xxxxxxxxx>
>> ---
>> 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;
>>
>
