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