Hi Tom, > > > ________________________________________________________________________________________________________ > > > *** CID 464361: Control flow issues (DEADCODE) > > > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 148 in ffa_print_error_log() > > > 142 > > > 143 if (ffa_id < FFA_FIRST_ID || ffa_id > FFA_LAST_ID) > > > 144 return -EINVAL; > > > 145 > > > 146 abi_idx = FFA_ID_TO_ERRMAP_ID(ffa_id); > > > 147 if (abi_idx < 0 || abi_idx >= FFA_ERRMAP_COUNT) > > > >>> CID 464361: Control flow issues (DEADCODE) > > > >>> Execution cannot reach this statement: "return -22;". > > > 148 return -EINVAL; > > > > This is a false positive. > > > > abi_idx value could end up matching this condition "(abi_idx < 0 || > > abi_idx >= FFA_ERRMAP_COUNT)". > > > > This happens when ffa_id value is above the allowed bounds. Example: when > > ffa_id is 0x50 or 0x80 > > > > ffa_print_error_log(0x50, ...); /* exceeding lower bound */ > > ffa_print_error_log(0x80, ...); /* exceeding upper bound */ > > > > In these cases "return -EINVAL;" is executed. > > So those invalid values aren't caught by the previous check that ffa_id > falls within FFA_FIRST_ID to FFA_LAST_ID ?
I had a closer look at that and I agree that the deadcode defect is legitimate. I already provided a fix [1]. [1]: https://lore.kernel.org/all/20231020131533.239591-1-abdellatif.elkhl...@arm.com/ > > > > ... > > > ________________________________________________________________________________________________________ > > > *** CID 464360: Control flow issues (NO_EFFECT) > > > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 207 in ffa_get_version_hdlr() > > > 201 major = GET_FFA_MAJOR_VERSION(res.a0); > > > 202 minor = GET_FFA_MINOR_VERSION(res.a0); > > > 203 > > > 204 log_debug("FF-A driver %d.%d\nFF-A framework %d.%d\n", > > > 205 FFA_MAJOR_VERSION, FFA_MINOR_VERSION, major, > > > minor); > > > 206 > > > >>> CID 464360: Control flow issues (NO_EFFECT) > > > >>> This greater-than-or-equal-to-zero comparison of an unsigned > > > >>> value is always true. "minor >= 0". > > > 207 if (major == FFA_MAJOR_VERSION && minor >= > > > FFA_MINOR_VERSION) { > > > > Providing the facts that: > > > > #define FFA_MINOR_VERSION (0) > > u16 minor; > > > > Yes, currently this condition is always true: minor >= FFA_MINOR_VERSION > > > > However, we might upgrade FFA_MINOR_VERSION in the future. If we remove the > > "minor >= FFA_MINOR_VERSION" , > > non compatible versions could pass which we don't want. > > > > To keep this code scalable, I think it's better to keep this condition. > > OK, thanks this makes sense as an intentional change for future sanity > checking. > > > > ________________________________________________________________________________________________________ > > > *** CID 464359: (PASS_BY_VALUE) > > > /drivers/firmware/arm-ffa/arm-ffa-uclass.c: 168 in invoke_ffa_fn() > > > 162 * @args: FF-A ABI arguments to be copied to Xn registers > > > 163 * @res: FF-A ABI return data to be copied from Xn registers > > > 164 * > > > 165 * Calls low level SMC implementation. > > > 166 * This function should be implemented by the user driver. > > > 167 */ > > > >>> CID 464359: (PASS_BY_VALUE) > > > >>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by > > > >>> value, which exceeds the low threshold of 128 bytes. > > > 168 void __weak invoke_ffa_fn(ffa_value_t args, ffa_value_t *res) > > > > We are using invoke_ffa_fn with the same arguments as in linux. The aim is > > to use the same interfaces as in the Linux FF-A > > driver to make porting code easier. > > > > In Linux, args is passed by value [1]. > > ffa_value_t is a structure with 18 "unsigned long" fields. So, the size is > > fixed. > > > > [1]: invoke_ffa_fn arguments in the Linux FF-A driver > > > > https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/driver.c#L115 > > https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/driver.c#L54 > > https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/firmware/arm_ffa/common.h#L15 > > > > [2]: include/linux/arm-smccc.h > > So this is intentional, OK. > > > > > > 169 { > > > 170 } > > > 171 > > > 172 /** > > > 173 * ffa_get_version_hdlr() - FFA_VERSION handler function > > > /drivers/firmware/arm-ffa/ffa-emul-uclass.c: 673 in invoke_ffa_fn() > > > 667 * invoke_ffa_fn() - SMC wrapper > > > 668 * @args: FF-A ABI arguments to be copied to Xn registers > > > 669 * @res: FF-A ABI return data to be copied from Xn registers > > > 670 * > > > 671 * Calls the emulated SMC call. > > > 672 */ > > > >>> CID 464359: (PASS_BY_VALUE) > > > >>> Passing parameter args of type "ffa_value_t" (size 144 bytes) by > > > >>> value, which exceeds the low threshold of 128 bytes. > > > 673 void invoke_ffa_fn(ffa_value_t args, ffa_value_t *res) > > > > Same feedback as above. > > Thanks. I'll update the last 3 CIDs shortly. Thanks Tom :) Cheers, Abdellatif
