[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT RFC PATCH] Implement PL031 RTC library for Arm
Hi, > -----Original Message----- > From: Jianyong Wu (Arm Technology China) > Sent: Thursday, November 15, 2018 1:37 PM > To: Julien Grall <julien.grall@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx; > simon.kuenzer@xxxxxxxxx > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd > <nd@xxxxxxx>; Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx> > Subject: RE: [Minios-devel] [UNIKRAFT RFC PATCH] Implement PL031 RTC > library for Arm > > Hi, > > > -----Original Message----- > > From: Julien Grall <julien.grall@xxxxxxx> > > Sent: Monday, November 12, 2018 6:37 PM > > To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@xxxxxxx>; > minios- > > devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx > > Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd > > <nd@xxxxxxx>; Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx> > > Subject: Re: [Minios-devel] [UNIKRAFT RFC PATCH] Implement PL031 RTC > > library for Arm > > > > > > > > On 11/11/18 1:59 AM, Jianyong Wu (Arm Technology China) wrote: > > > Hi, > > > > Hi, > > > > >>> +void rtc_clear_intr(void) > > >>> +{ > > >>> + ioreg_write32(RTC_REG(RTC_REG_ICR), 1); } > > >>> + > > >>> +int _dtb_init_rtc(void *dtb) > > >>> +{ > > >>> + uint32_t idx; > > >>> + int fdt_rtc, naddr, nsize, size, prop_len, prop_min_len; > > >>> + const uint64_t *regs; > > >>> + > > >>> + uk_printd(DLVL_INFO, "Probing RTC...\n"); > > >>> + fdt_rtc = uk_dtb_find_device(dtb, rtc_device_list, > > >>> + sizeof(rtc_device_list)); > > >> > > >> I would not assume the RTC is always present. > > > > > > The check is inside uk_dtb_read_region. > > > > Your check is an ASSERT(...). This means that if an RTC is not present > > then you will get Unikraft crashed in debug build. > > > > On non-debug build, it will just continue silently. > > > > In both case, I don't think this is the correct behavior. If the RTC > > is a mandatory device, then it should be an error/crash in appropriate > > place (from uk_dtb_read_region it is not very obvious what failed). > > > > If the RTC is not mandatory, then you should return here and not > > configured the RTC. > > > > Ok, I will check fdt_rtc and give a crash if there is no that device. > > > >> > > >>> + naddr = uk_dtb_read_region(dtb, fdt_rtc, &nsize, ®s); > > >>> + rtc_base_addr = uk_dtb_read_term(regs, 0, naddr, nsize, &size); > > >> > > >> Similarly here, I would not assume uk_dtb_read_term() will always > > succeed. > > >> It would make the code more safe if the DT passed is wrong. > > > > > > > > > Check has been done in this function. > > > > I am afraid this is not the case. If you look at your function there > > are no check whether the region N you are trying to access is in the device- > tree. > > > > There are an ASSERT(...) in uk_dtb_read_region but that's only make > > sure "regs" describes at least one region. > > > Ok, I will check rtc_base_addr. > Sorry rtc_base_addr is difficult to check. I will check regs in uk_dtb_read_term() as I say in fdt part of mail list. > > >> > > >>> + > > >>> + /* Record the boot seconds */ > > >>> + rtc_boot_seconds = rtc_read(); > > >>> + > > >>> + uk_printd(DLVL_INFO, "Found RTC on: %p\n", rtc_base_addr); > > >> > > >> I am slightly surprised this compile in Unikraft. This function is > > >> meant to return an integer but I don't see any return here. > > > > > > Which function? Rtc_read()? Check it again. > > > > Please tone down your writing if you want people to continue reviewing > > your code. > > > > In that particular case, my comment was at the end of _dtb_init_rtc. > > The prototype of the function is: > > > > int _dtb_init_rtc(void *dtb) > > > > That function return an int but I can't see a 'return X' in the code. > > It seems Unikraft does not use -Werror flags, which means this kind of > > error would be only a warning. Yet the return value will be unknown. > > > > Regarding -Werror, I think Unikraft community should consider use it > > as a lot of warning can be harmful in long run. > > Oh, I'm sorry. It's not my intention to offend you. Words does not convey the > exact emotion of the talker. > Anyway I will watch out of my words. I am really happy to have my patch > reviewed by someone like you. Thanks. > > Yeah, there is really my fault to miss that return statement. I will add it. > > Bests > Jianyong wu > > > > Cheers, > > > > -- > > Julien Grall _______________________________________________ Minios-devel mailing list Minios-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/minios-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |