On 4/24/24 6:43 AM, Dan Carpenter wrote:
> Hello Tanmay Shah,
>
> Commit 6b291e8020a8 ("drivers: remoteproc: Add Xilinx r5 remoteproc
> driver") from Nov 14, 2022 (linux-next), leads to the following
> Smatch static checker warning:
>
> drivers/remoteproc/xlnx_r5_remoteproc.c:1088 zynqmp_r5_cluster_init()
> error: uninitialized symbol 'tcm_mode'.
>
> drivers/remoteproc/xlnx_r5_remoteproc.c:917 zynqmp_r5_core_init()
> error: uninitialized symbol 'ret'.
Hello,
Second warning was fixed with this patch:
https://lore.kernel.org/all/20240423170210.1035957-1-tanmay.shah@xxxxxxx/
>
> drivers/remoteproc/xlnx_r5_remoteproc.c
> 961 static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
> 962 {
> 963 enum zynqmp_r5_cluster_mode cluster_mode = LOCKSTEP_MODE;
> 964 struct device *dev = cluster->dev;
> 965 struct device_node *dev_node = dev_of_node(dev);
> 966 struct platform_device *child_pdev;
> 967 struct zynqmp_r5_core **r5_cores;
> 968 enum rpu_oper_mode fw_reg_val;
> 969 struct device **child_devs;
> 970 struct device_node *child;
> 971 enum rpu_tcm_comb tcm_mode;
> 972 int core_count, ret, i;
> 973 struct mbox_info *ipi;
> 974
> 975 ret = of_property_read_u32(dev_node, "xlnx,cluster-mode", &cluster_mode);
> 976
> 977 /*
> 978 * on success returns 0, if not defined then returns -EINVAL,
> 979 * In that case, default is LOCKSTEP mode. Other than that
> 980 * returns relative error code < 0.
> 981 */
> 982 if (ret != -EINVAL && ret != 0) {
> 983 dev_err(dev, "Invalid xlnx,cluster-mode property\n");
> 984 return ret;
> 985 }
> 986
> 987 /*
> 988 * For now driver only supports split mode and lockstep mode.
> 989 * fail driver probe if either of that is not set in dts.
> 990 */
> 991 if (cluster_mode == LOCKSTEP_MODE) {
> 992 fw_reg_val = PM_RPU_MODE_LOCKSTEP;
> 993 } else if (cluster_mode == SPLIT_MODE) {
> 994 fw_reg_val = PM_RPU_MODE_SPLIT;
> 995 } else {
> 996 dev_err(dev, "driver does not support cluster mode %d\n", cluster_mode);
> 997 return -EINVAL;
> 998 }
> 999
> 1000 if (of_find_property(dev_node, "xlnx,tcm-mode", NULL)) {
> 1001 ret = of_property_read_u32(dev_node, "xlnx,tcm-mode", (u32 *)&tcm_mode);
> 1002 if (ret)
> 1003 return ret;
> 1004 } else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss")) {
> 1005 if (cluster_mode == LOCKSTEP_MODE)
> 1006 tcm_mode = PM_RPU_TCM_COMB;
> 1007 else
> 1008 tcm_mode = PM_RPU_TCM_SPLIT;
> 1009 }
>
> tcm_mode not initialized if device_is_compatible() is false.
Thanks,
I will fix this with else case.
>
> 1010
> 1011 /*
> 1012 * Number of cores is decided by number of child nodes of
> 1013 * r5f subsystem node in dts. If Split mode is used in dts
> 1014 * 2 child nodes are expected.
> 1015 * In lockstep mode if two child nodes are available,
> 1016 * only use first child node and consider it as core0
> 1017 * and ignore core1 dt node.
> 1018 */
> 1019 core_count = of_get_available_child_count(dev_node);
> 1020 if (core_count == 0) {
> 1021 dev_err(dev, "Invalid number of r5 cores %d", core_count);
> 1022 return -EINVAL;
> 1023 } else if (cluster_mode == SPLIT_MODE && core_count != 2) {
> 1024 dev_err(dev, "Invalid number of r5 cores for split mode\n");
> 1025 return -EINVAL;
> 1026 } else if (cluster_mode == LOCKSTEP_MODE && core_count == 2) {
> 1027 dev_warn(dev, "Only r5 core0 will be used\n");
> 1028 core_count = 1;
> 1029 }
> 1030
> 1031 child_devs = kcalloc(core_count, sizeof(struct device *), GFP_KERNEL);
> 1032 if (!child_devs)
> 1033 return -ENOMEM;
> 1034
> 1035 r5_cores = kcalloc(core_count,
> 1036 sizeof(struct zynqmp_r5_core *), GFP_KERNEL);
> 1037 if (!r5_cores) {
> 1038 kfree(child_devs);
> 1039 return -ENOMEM;
> 1040 }
> 1041
> 1042 i = 0;
> 1043 for_each_available_child_of_node(dev_node, child) {
> 1044 child_pdev = of_find_device_by_node(child);
> 1045 if (!child_pdev) {
> 1046 of_node_put(child);
> 1047 ret = -ENODEV;
> 1048 goto release_r5_cores;
> 1049 }
> 1050
> 1051 child_devs[i] = &child_pdev->dev;
> 1052
> 1053 /* create and add remoteproc instance of type struct rproc */
> 1054 r5_cores[i] = zynqmp_r5_add_rproc_core(&child_pdev->dev);
> 1055 if (IS_ERR(r5_cores[i])) {
> 1056 of_node_put(child);
> 1057 ret = PTR_ERR(r5_cores[i]);
> 1058 r5_cores[i] = NULL;
> 1059 goto release_r5_cores;
> 1060 }
> 1061
> 1062 /*
> 1063 * If mailbox nodes are disabled using "status" property then
> 1064 * setting up mailbox channels will fail.
> 1065 */
> 1066 ipi = zynqmp_r5_setup_mbox(&child_pdev->dev);
> 1067 if (ipi) {
> 1068 r5_cores[i]->ipi = ipi;
> 1069 ipi->r5_core = r5_cores[i];
> 1070 }
> 1071
> 1072 /*
> 1073 * If two child nodes are available in dts in lockstep mode,
> 1074 * then ignore second child node.
> 1075 */
> 1076 if (cluster_mode == LOCKSTEP_MODE) {
> 1077 of_node_put(child);
> 1078 break;
> 1079 }
> 1080
> 1081 i++;
> 1082 }
> 1083
> 1084 cluster->mode = cluster_mode;
> 1085 cluster->core_count = core_count;
> 1086 cluster->r5_cores = r5_cores;
> 1087
> --> 1088 ret = zynqmp_r5_core_init(cluster, fw_reg_val, tcm_mode);
> 1089 if (ret < 0) {
> 1090 dev_err(dev, "failed to init r5 core err %d\n", ret);
> 1091 cluster->core_count = 0;
> 1092 cluster->r5_cores = NULL;
> 1093
> 1094 /*
> 1095 * at this point rproc resources for each core are allocated.
> 1096 * adjust index to free resources in reverse order
> 1097 */
> 1098 i = core_count - 1;
> 1099 goto release_r5_cores;
> 1100 }
> 1101
> 1102 kfree(child_devs);
> 1103 return 0;
> 1104
> 1105 release_r5_cores:
> 1106 while (i >= 0) {
> 1107 put_device(child_devs[i]);
> 1108 if (r5_cores[i]) {
> 1109 zynqmp_r5_free_mbox(r5_cores[i]->ipi);
> 1110 of_reserved_mem_device_release(r5_cores[i]->dev);
> 1111 rproc_del(r5_cores[i]->rproc);
> 1112 rproc_free(r5_cores[i]->rproc);
> 1113 }
> 1114 i--;
> 1115 }
> 1116 kfree(r5_cores);
> 1117 kfree(child_devs);
> 1118 return ret;
> 1119 }
>
> regards,
> dan carpenter
