Hi,
On Wed, Oct 18, 2023 at 5:34 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> 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.
I am working on this set of patches again, and I think using %pt and rtc_time
would be a little cleaner because rtc_time has all the required fields.
I noticed that vsprintf.c doesn't use the tm_yday and tm_wday fields,
however, is it safe to ignore them and assume they won't be used in the future?
If not, having to calculate them will probably make the code worse.
Regards,
Nikita
