On Sat, Mar 05, 2022 at 03:25:56AM +0530, Kumar Kartikeya Dwivedi wrote: > On Sat, Mar 05, 2022 at 03:13:33AM IST, Martin KaFai Lau wrote: > > On Sat, Mar 05, 2022 at 02:18:56AM +0530, Kumar Kartikeya Dwivedi wrote: > > > On Sat, Mar 05, 2022 at 01:58:30AM IST, Martin KaFai Lau wrote: > > > > > + verbose(env, "R%d must have zero offset when passed to release func\n", > > > > > + regno); > > > > > + return -EINVAL; > > > > > + } > > > > > + fixed_off_ok = release_reg ? false : true; > > > > nit. > > > > fixed_off_ok = !release_reg; > > > > > > > > but this is a bit moot here considering the reg->off > > > > check has already been done for the release_reg case. > > > > > > > > > > Yes, it would be a redundant check inside __check_ptr_off_reg, but we still need > > > to call it for checking bad var_off. > > Redundant check is fine. > > > > The intention and the net effect here is fixed_off is always > > allowed for the remaining case, so may as well directly set > > fixed_off_ok to true. "fixed_off_ok = !release_reg;" > > made me go back to re-read what else has not been handled > > for the release_reg case but it could be just me being > > slow here. > > > > Right, I can see why that may be confusing. I just set it to !release_reg to > disable any other code that may be added using that bool later in the future. hmm... If the concern is on future code, how about using a comment to remind future cases instead and directly set it to true? /* All special cases were handled above, the remaining * PTR_TO_BTF_ID case always allows fixed off. */ fixed_off_ok = true; > > > It will be useful to at least leave a comment here > > on the redundant check and the remaining cases for > > PTR_TO_BTF_ID actually always allow fixed_off. > > > > Yes, I will add a comment to make it clearer.
