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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 7 Mar 2018 16:51:26 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Andrey Smirnov <andrew.smirnov@...il.com>
Cc:     linux-kernel@...r.kernel.org, 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 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.

-- 
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog