> > > > > > > > + enable_vmx = cpu_has(c, X86_FEATURE_VMX) && > > > > + IS_ENABLED(CONFIG_KVM_INTEL); > > > > > > It's less than 100 characters: > > > > Just carious, shouldn't be 80 characters to wrap a new line, instead of 100? > > Try with checkpatch.pl. Checkpatch.pl has default value 100, but it can be overwritten. I found below document explicitly said 80 should be the length: https://www.kernel.org/doc/html/v5.11-rc7/process/coding-style.html 2) Breaking long lines and strings Coding style is all about readability and maintainability using commonly available tools. The preferred limit on the length of a single line is 80 columns. Statements longer than 80 columns should be broken into sensible chunks, unless exceeding 80 columns significantly increases readability and does not hide information. [...] > > > > update_sgx: > > > > - if (!(msr & FEAT_CTL_SGX_ENABLED) || > > > > - !(msr & FEAT_CTL_SGX_LC_ENABLED) || !enable_sgx) { > > > > - if (enable_sgx) > > > > - pr_err_once("SGX disabled by BIOS\n"); > > > > + if (!(msr & FEAT_CTL_SGX_ENABLED)) { > > > > + if (enable_sgx_kvm || enable_sgx_driver) > > > > + pr_err_once("SGX disabled by BIOS.\n"); > > > > clear_cpu_cap(c, X86_FEATURE_SGX); > > > > > > Empty line before return statement. > > > > It's just two statements inside the if() {} statement. Putting a new line here is > too sparse IMHO. > > > > I'd like to hear more. > > This was a common review comment in original SGX series, so I'm sticking to the > pattern. Well if you insist, I can do that. But I am not that convinced. In fact, I also believe that in most cases, having empty line before 'return' is good practice, for instance, when 'return' is the very last statement in the function. I am also glad to do it if it is a x86 patch convention that we even need to put a new empty line when there are only very few statements inside if() {}. However it seems it is not the case. For example, I just did a search in SGX driver code, and below examples all DONOT have empty line before return (and I don't think I captured them all): sgx/driver.c: static int sgx_release(struct inode *inode, struct file *file) { ...... kref_put(&encl->refcount, sgx_encl_release); return 0; } sgx/ioctl.c: static struct sgx_va_page *sgx_encl_grow(struct sgx_encl *encl) { ...... va_page->epc_page = sgx_alloc_va_page(); if (IS_ERR(va_page->epc_page)) { err = ERR_CAST(va_page->epc_page); kfree(va_page); return err; } WARN_ON_ONCE(encl->page_cnt % SGX_VA_SLOT_COUNT); } encl->page_cnt++; return va_page; } static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg) { ...... kfree(secs); return ret; } static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src, unsigned long offset, struct sgx_secinfo *secinfo, unsigned long flags) { ...... epc_page = sgx_alloc_epc_page(encl_page, true); if (IS_ERR(epc_page)) { kfree(encl_page); return PTR_ERR(epc_page); } ...... sgx_mark_page_reclaimable(encl_page->epc_page); mutex_unlock(&encl->lock); mmap_read_unlock(current->mm); return ret; ...... } And in cpu/feat_ctl.c: void init_ia32_feat_ctl(struct cpuinfo_x86 *c) { ...... if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) { clear_cpu_cap(c, X86_FEATURE_VMX); clear_sgx_caps(); return; } ...... }
