* [bug report] drivers: remoteproc: Add Xilinx r5 remoteproc driver
@ 2024-04-24 11:43 Dan Carpenter
2024-04-24 16:05 ` Tanmay Shah
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2024-04-24 11:43 UTC (permalink / raw)
To: tanmay.shah; +Cc: linux-remoteproc
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'.
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.
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] drivers: remoteproc: Add Xilinx r5 remoteproc driver
2024-04-24 11:43 [bug report] drivers: remoteproc: Add Xilinx r5 remoteproc driver Dan Carpenter
@ 2024-04-24 16:05 ` Tanmay Shah
0 siblings, 0 replies; 2+ messages in thread
From: Tanmay Shah @ 2024-04-24 16:05 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-remoteproc, Mathieu Poirier, Bjorn Andersson
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@amd.com/
>
> 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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-04-24 16:05 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-24 11:43 [bug report] drivers: remoteproc: Add Xilinx r5 remoteproc driver Dan Carpenter
2024-04-24 16:05 ` Tanmay Shah
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).