On Tue, Jan 24, 2023 at 03:08:27PM -0500, Joe Lawrence wrote: > > + /* > > + * For a livepatch relocation, the restore r2 instruction might have > > + * been previously written if the relocation references a symbol in a > > + * module which was unloaded and is now being reloaded. In that case, > > + * skip the warning and instruction write. > > + */ > > + if (klp_sym && insn_val == PPC_INST_LD_TOC) > > + return 0; > > Hi Josh, > > Nit: shouldn't this return 1? > > And if you're willing to entertain a small refactor, wouldn't > restore_r2() be clearer if it returned -ESOMETHING on error? > > Maybe converting to a boolean could work, but then I'd suggest a name > that clearly implies success/fail given true/false return. Maybe > replace_nop_with_ld_toc() or replace_nop_to_restore_r2() ... still > -ESOMETHING is more intuitive to me as there are cases like this where > the function safely returns w/o replacing anything. Indeed, and I actually already discovered that and made such changes, just need to get around to posting the patches. -- Josh
