On 29/04/2024 15:50, Markus Elfring wrote: > I would usually expect a corresponding cover letter for patch series. > > >> construct_region() could return a PTR_ERR() which cannot be derefernced. > > I hope that a typo will be avoided in the last word of this sentence. Thanks, hate my fat fingers, I will fix it later. > > >> Moving the dereference behind the error checking to make sure the >> pointer is valid. > > Please choose an imperative wording for an improved change description. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n94 > > > … >> +++ b/drivers/cxl/core/region.c >> @@ -3086,10 +3086,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled) >> mutex_lock(&cxlrd->range_lock); >> region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa, >> match_region_by_range); >> - if (!region_dev) { >> + if (!region_dev) >> cxlr = construct_region(cxlrd, cxled); >> - region_dev = &cxlr->dev; >> - } else >> + else >> cxlr = to_cxl_region(region_dev); >> mutex_unlock(&cxlrd->range_lock); > > I suggest to simplify such source code by using a conditional operator expression. Thanks for your suggestion. Do you mean something like: cxlr = region_dev ? to_cxl_region(region_dev) : construct_region(cxlrd, cxled); If so, I'm open to this option, but the kernel does not always obey this convention. For example, static inline int __must_check PTR_ERR_OR_ZERO(__force const void *ptr) { if (IS_ERR(ptr)) return PTR_ERR(ptr); else return 0; } Thanks Zhijian > > Regards, > Markus
