On Tue Aug 8, 2023 at 6:26 AM EEST, Linus Torvalds wrote: > On Mon, 7 Aug 2023 at 17:39, Jason A. Donenfeld <Jason@xxxxxxxxx> wrote: > > > > I'm not sure what's best or what Linus prefers. Linus - Jarkko sent you > > the wrong version patch. Do you want a fixup patch that accounts for the > > difference, and then I'll address the stable@ metadata deficiency > > manually by talking to Greg, or would you rather some merge commit > > magic, or something else? > > Either works for me, whatever ends up being easiest. > > However, looking at that v3 patch, that "should we enable/disable the > hwrng" is now repeated *three* times, and that first one is > > if (!IS_ENABLED(CONFIG_HW_RANDOM_TPM) || tpm_is_firmware_upgrade(chip) || > - tpm_amd_is_rng_defective(chip)) > + chip->flags & TPM_CHIP_FLAG_HWRNG_DISABLED) > > and wants fixing anyway: you want parenthesis around the '&'. > > Yes, yes, it works (because bitwise ops have higher precedence than > logical ones), but let's not do that. > > But more importantly, can we just have a single helper inline function > for this and *not* repeat the same multi-line expression three times > (just in negated and then 2x non-negated format)? > > That test is ugly anyway. Why is "tpm_is_firmware_upgrade()" a wrapper > function around testing "chip->flags", but then right next to it it > tests them explicitly. > > So if we have to re-do this all, let's re-do it properly. Ok? > > Thinking about it, I do guess that makes it easier to just send an > incremental patch on top. > > Linus What if I just revert the commit, apply the correct one, and send a PR? BR, Jarkko
