Re: [PATCH 2/5] platform/x86: msi-ec: Add fw version and release date attributes

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

Hi,

On 10/12/23 14:56, Ilpo Järvinen wrote:
> Hi Hans,
> 
> You missed the one question I had for you. I put it now conviniently at 
> the end of the quote block below...
> 
> On Thu, 12 Oct 2023, Hans de Goede wrote:
>>
>> Great to see that you are working on upstreaming more of the
>> out-of-tree msi-ec functionality. Thank you for working on this.
>>
>> On 10/11/23 14:41, Ilpo Järvinen wrote:
>>> On Tue, 10 Oct 2023, Nikita Kravets wrote:
>>>
>>>> Create a root attribute group and add the first platform device
>>>> attributes: firmware version and firmware release date. Firmware
>>>> version attribute uses an already present ec_get_firmware_version()
>>>> function. Both features are present on all supported laptops.
>>>>
>>>> Cc: Aakash Singh <mail@singhaakash.dev>
>>>> Cc: Jose Angel Pastrana <japp0005@red.ujaen.es>
>>>> Signed-off-by: Nikita Kravets <teackot@gmail.com>
>>>> ---
> 
>>>> +static ssize_t fw_release_date_show(struct device *device,
>>>> +				    struct device_attribute *attr, char *buf)
>>>> +{
>>>> +	u8 rdate[MSI_EC_FW_DATE_LENGTH + 1];
>>>> +	u8 rtime[MSI_EC_FW_TIME_LENGTH + 1];
>>>> +	int result;
>>>> +	int year, month, day, hour, minute, second;
>>>> +
>>>> +	memset(rdate, 0, MSI_EC_FW_DATE_LENGTH + 1);
>>>
>>> sizeof(*rdate) is safer so please use it.
>>>
>>>> +	result = ec_read_seq(MSI_EC_FW_DATE_ADDRESS,
>>>> +			     rdate,
>>>> +			     MSI_EC_FW_DATE_LENGTH);
>>>> +	if (result < 0)
>>>> +		return result;
>>>> +
>>>> +	result = sscanf(rdate, "%02d%02d%04d", &month, &day, &year);
>>>
>>> There fields would naturally be %u and unsigned but see the other comment 
>>> below before doing this change.
>>>
>>>> +	if (result != 3)
>>>> +		return -EINVAL;
>>>
>>> EINVAL should be returned if the input was invalid but here the data 
>>> itself is not okay so some other errno would be better.
>>>
>>>> +	memset(rtime, 0, MSI_EC_FW_TIME_LENGTH + 1);
>>>
>>> sizeof() like above.
>>>
>>>> +	result = ec_read_seq(MSI_EC_FW_TIME_ADDRESS,
>>>> +			     rtime,
>>>> +			     MSI_EC_FW_TIME_LENGTH);
>>>> +	if (result < 0)
>>>> +		return result;
>>>> +
>>>> +	result = sscanf(rtime, "%02d:%02d:%02d", &hour, &minute, &second);
>>>> +	if (result != 3)
>>>> +		return -EINVAL;
>>>
>>> Ditto.
>>>
>>>> +
>>>> +	return sysfs_emit(buf, "%04d/%02d/%02d %02d:%02d:%02d\n", year, month, day,
>>>> +			  hour, minute, second);
>>>
>>> It would be kind of nice to use %pt formatting here instead of custom
>>> datetime format, however, it would either require converting to time64_t 
>>> or using struct rtc_time. The latter would naturally have the right fields 
>>> but they're not unsigned so my comment above about %u is not going to work 
>>> well with it.
>>>
>>> I'm also a bit unsure whether it's appropriate to use that struct outside 
>>> of rtc realm. vsprintf.c seems to convert time64_t into rtc_time before 
>>> printing though.
>>>
>>> Hans, do you have any idea about the struct rtc_time?

I don't really have any good ideas how to handle this. I agree that
using %pt might be a good idea, but then as you say the data would first
need to be converted to a struct rtc_time. All in all I think it is
probably best to stick with the DIY formatting of the time.
But I've no objections to doing the rtc_time conversion if people
think that is cleaner / better.

Regards,

Hans