linux-kernel - Re: [PATCH 1/3] mfd: rave-sp: Add code to print firmware versions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 8 Mar 2018 09:18:58 -0800
From:   Andrey Smirnov <andrew.smirnov@...il.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Chris Healy <cphealy@...il.com>,
        Lucas Stach <l.stach@...gutronix.de>,
        Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH 1/3] mfd: rave-sp: Add code to print firmware versions

On Wed, Mar 7, 2018 at 8:51 AM, Lee Jones <lee.jones@...aro.org> wrote:
> On Mon, 26 Feb 2018, Andrey Smirnov wrote:
>
>> Add code that would query and print out bootloader and application
>> firmware version info.
>>
>> Cc: linux-kernel@...r.kernel.org
>> Cc: cphealy@...il.com
>> Cc: Lucas Stach <l.stach@...gutronix.de>
>> Cc: Lee Jones <lee.jones@...aro.org>
>> Cc: Guenter Roeck <linux@...ck-us.net>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@...il.com>
>> ---
>>
>> Lee:
>>
>> The reason 'part_number_firmware' and 'part_number_firmware' are a
>> part of struct rave_sp is because there exists another patch on top of
>> this one that exposes those fields via sysfs. The latter patch is not
>> currently being upstreamed (might be in the future), so if keeping
>> this arrangement is too ugly, let me know, and I'll get rid of those
>> fields in 'rave_sp'.
>
> That's okay.
>
>>  drivers/mfd/rave-sp.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 97 insertions(+)
>>
>> diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
>> index c8173de5653a..9e4c83ff2aec 100644
>> --- a/drivers/mfd/rave-sp.c
>> +++ b/drivers/mfd/rave-sp.c
>> @@ -160,6 +160,8 @@ struct rave_sp_variant {
>>   * @variant:                 Device variant specific information
>>   * @event_notifier_list:     Input event notification chain
>>   *
>> + * @part_number_firmware:    Firmware version
>> + * @part_number_bootloader:  Bootloader version
>>   */
>>  struct rave_sp {
>>       struct serdev_device *serdev;
>> @@ -171,8 +173,42 @@ struct rave_sp {
>>
>>       const struct rave_sp_variant *variant;
>>       struct blocking_notifier_head event_notifier_list;
>> +
>> +     const char *part_number_firmware;
>> +     const char *part_number_bootloader;
>>  };
>>
>> +struct rave_sp_version {
>> +     u8     hardware;
>> +     __le16 major;
>> +     u8     minor;
>> +     u8     letter[2];
>> +} __packed;
>> +
>> +struct rave_sp_status {
>> +     struct rave_sp_version bootloader_version;
>> +     struct rave_sp_version firmware_version;
>> +     u16 rdu_eeprom_flag;
>> +     u16 dds_eeprom_flag;
>> +     u8  pic_flag;
>> +     u8  orientation;
>> +     u32 etc;
>> +     s16 temp[2];
>> +     u8  backlight_current[3];
>> +     u8  dip_switch;
>> +     u8  host_interrupt;
>> +     u16 voltage_28;
>> +     u8  i2c_device_status;
>> +     u8  power_status;
>> +     u8  general_status;
>> +#define RAVE_SP_STATUS_GS_FIRMWARE_MODE      BIT(1)
>
> I'm more concerned with the seemingly unused #define shoved in the
> middle of a struct definition -- which I am not a fan of.
>
> Better to introduce it when you start using it and outside of the
> struct definition please.
>
> The remainder of the patch looks okay.

OK, will fix in v2.

Thanks,
Andrey Smirnov