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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]

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.