Messages in this thread Patch in this message | | | Subject | Re: [PATCH V3 XRT Alveo 02/18] fpga: xrt: driver metadata helper functions | From | Lizhi Hou <> | Date | Mon, 22 Feb 2021 22:05:29 -0800 |
| |
Hi Tom,
On 02/20/2021 09:07 AM, Tom Rix wrote: > On 2/17/21 10:40 PM, Lizhi Hou wrote: >> XRT drivers use device tree as metadata format to discover HW subsystems >> behind PCIe BAR. Thus libfdt functions are called for driver to parse > for the driver to parse the will fix. >> device tree blob. >> >> Signed-off-by: Sonal Santan <sonal.santan@xilinx.com> >> Signed-off-by: Max Zhen <max.zhen@xilinx.com> >> Signed-off-by: Lizhi Hou <lizhih@xilinx.com> >> --- >> drivers/fpga/xrt/include/metadata.h | 229 ++++++++++++ >> drivers/fpga/xrt/metadata/metadata.c | 524 +++++++++++++++++++++++++++ >> 2 files changed, 753 insertions(+) >> create mode 100644 drivers/fpga/xrt/include/metadata.h >> create mode 100644 drivers/fpga/xrt/metadata/metadata.c >> >> diff --git a/drivers/fpga/xrt/include/metadata.h b/drivers/fpga/xrt/include/metadata.h >> new file mode 100644 >> index 000000000000..b929bc469b73 >> --- /dev/null >> +++ b/drivers/fpga/xrt/include/metadata.h >> @@ -0,0 +1,229 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Header file for Xilinx Runtime (XRT) driver >> + * >> + * Copyright (C) 2020-2021 Xilinx, Inc. >> + * >> + * Authors: >> + * Lizhi Hou <Lizhi.Hou@xilinx.com> >> + */ >> + >> +#ifndef _XRT_METADATA_H >> +#define _XRT_METADATA_H >> + >> +#include <linux/device.h> >> +#include <linux/vmalloc.h> >> +#include <linux/uuid.h> >> + >> +#define XRT_MD_INVALID_LENGTH (~0UL) >> + > These #defines could be in alphabetical order > > Some #define with embedded acronyms could be expanded > > ex/ XRT_MD_NODE_CMC_REG , what is CMC ? Will reorder. Expanding might make macro name too long. I will add comment as below: /* * IP nodes * AF: AXI Firewall * CMC: Card Management Controller * ERT: Embedded Runtime * PLP: Provider Reconfigurable Partition * ULP: User Reconfigurable Partition */ > >> +#define XRT_MD_PROP_COMPATIBLE "compatible" >> +#define XRT_MD_PROP_PF_NUM "pcie_physical_function" >> +#define XRT_MD_PROP_BAR_IDX "pcie_bar_mapping" >> +#define XRT_MD_PROP_IO_OFFSET "reg" >> +#define XRT_MD_PROP_INTERRUPTS "interrupts" >> +#define XRT_MD_PROP_INTERFACE_UUID "interface_uuid" >> +#define XRT_MD_PROP_LOGIC_UUID "logic_uuid" >> +#define XRT_MD_PROP_VERSION_MAJOR "firmware_version_major" >> + >> +#define XRT_MD_PROP_HWICAP "axi_hwicap" >> +#define XRT_MD_PROP_PDI_CONFIG "pdi_config_mem" >> + >> +#define XRT_MD_NODE_ENDPOINTS "addressable_endpoints" >> +#define XRT_MD_INTERFACES_PATH "/interfaces" >> + >> +#define XRT_MD_NODE_FIRMWARE "firmware" >> +#define XRT_MD_NODE_INTERFACES "interfaces" >> +#define XRT_MD_NODE_PARTITION_INFO "partition_info" >> + >> +#define XRT_MD_NODE_FLASH "ep_card_flash_program_00" >> +#define XRT_MD_NODE_XVC_PUB "ep_debug_bscan_user_00" >> +#define XRT_MD_NODE_XVC_PRI "ep_debug_bscan_mgmt_00" >> +#define XRT_MD_NODE_SYSMON "ep_cmp_sysmon_00" >> +#define XRT_MD_NODE_AF_BLP_CTRL_MGMT "ep_firewall_blp_ctrl_mgmt_00" >> +#define XRT_MD_NODE_AF_BLP_CTRL_USER "ep_firewall_blp_ctrl_user_00" >> +#define XRT_MD_NODE_AF_CTRL_MGMT "ep_firewall_ctrl_mgmt_00" >> +#define XRT_MD_NODE_AF_CTRL_USER "ep_firewall_ctrl_user_00" >> +#define XRT_MD_NODE_AF_CTRL_DEBUG "ep_firewall_ctrl_debug_00" >> +#define XRT_MD_NODE_AF_DATA_H2C "ep_firewall_data_h2c_00" >> +#define XRT_MD_NODE_AF_DATA_C2H "ep_firewall_data_c2h_00" >> +#define XRT_MD_NODE_AF_DATA_P2P "ep_firewall_data_p2p_00" >> +#define XRT_MD_NODE_AF_DATA_M2M "ep_firewall_data_m2m_00" >> +#define XRT_MD_NODE_CMC_REG "ep_cmc_regmap_00" >> +#define XRT_MD_NODE_CMC_RESET "ep_cmc_reset_00" >> +#define XRT_MD_NODE_CMC_MUTEX "ep_cmc_mutex_00" >> +#define XRT_MD_NODE_CMC_FW_MEM "ep_cmc_firmware_mem_00" >> +#define XRT_MD_NODE_ERT_FW_MEM "ep_ert_firmware_mem_00" >> +#define XRT_MD_NODE_ERT_CQ_MGMT "ep_ert_command_queue_mgmt_00" >> +#define XRT_MD_NODE_ERT_CQ_USER "ep_ert_command_queue_user_00" >> +#define XRT_MD_NODE_MAILBOX_MGMT "ep_mailbox_mgmt_00" >> +#define XRT_MD_NODE_MAILBOX_USER "ep_mailbox_user_00" >> +#define XRT_MD_NODE_GATE_PLP "ep_pr_isolate_plp_00" >> +#define XRT_MD_NODE_GATE_ULP "ep_pr_isolate_ulp_00" >> +#define XRT_MD_NODE_PCIE_MON "ep_pcie_link_mon_00" >> +#define XRT_MD_NODE_DDR_CALIB "ep_ddr_mem_calib_00" >> +#define XRT_MD_NODE_CLK_KERNEL1 "ep_aclk_kernel_00" >> +#define XRT_MD_NODE_CLK_KERNEL2 "ep_aclk_kernel_01" >> +#define XRT_MD_NODE_CLK_KERNEL3 "ep_aclk_hbm_00" >> +#define XRT_MD_NODE_KDMA_CTRL "ep_kdma_ctrl_00" >> +#define XRT_MD_NODE_FPGA_CONFIG "ep_fpga_configuration_00" >> +#define XRT_MD_NODE_ERT_SCHED "ep_ert_sched_00" >> +#define XRT_MD_NODE_XDMA "ep_xdma_00" >> +#define XRT_MD_NODE_MSIX "ep_msix_00" >> +#define XRT_MD_NODE_QDMA "ep_qdma_00" >> +#define XRT_MD_XRT_MD_NODE_QDMA4 "ep_qdma4_00" >> +#define XRT_MD_NODE_STM "ep_stream_traffic_manager_00" >> +#define XRT_MD_NODE_STM4 "ep_stream_traffic_manager4_00" >> +#define XRT_MD_NODE_CLK_SHUTDOWN "ep_aclk_shutdown_00" >> +#define XRT_MD_NODE_ERT_BASE "ep_ert_base_address_00" >> +#define XRT_MD_NODE_ERT_RESET "ep_ert_reset_00" >> +#define XRT_MD_NODE_CLKFREQ_K1 "ep_freq_cnt_aclk_kernel_00" >> +#define XRT_MD_NODE_CLKFREQ_K2 "ep_freq_cnt_aclk_kernel_01" >> +#define XRT_MD_NODE_CLKFREQ_HBM "ep_freq_cnt_aclk_hbm_00" >> +#define XRT_MD_NODE_GAPPING "ep_gapping_demand_00" >> +#define XRT_MD_NODE_UCS_CONTROL_STATUS "ep_ucs_control_status_00" >> +#define XRT_MD_NODE_P2P "ep_p2p_00" >> +#define XRT_MD_NODE_REMAP_P2P "ep_remap_p2p_00" >> +#define XRT_MD_NODE_DDR4_RESET_GATE "ep_ddr_mem_srsr_gate_00" >> +#define XRT_MD_NODE_ADDR_TRANSLATOR "ep_remap_data_c2h_00" >> +#define XRT_MD_NODE_MAILBOX_XRT "ep_mailbox_user_to_ert_00" >> +#define XRT_MD_NODE_PMC_INTR "ep_pmc_intr_00" >> +#define XRT_MD_NODE_PMC_MUX "ep_pmc_mux_00" >> + >> +/* driver defined endpoints */ >> +#define XRT_MD_NODE_VSEC "drv_ep_vsec_00" >> +#define XRT_MD_NODE_VSEC_GOLDEN "drv_ep_vsec_golden_00" >> +#define XRT_MD_NODE_BLP_ROM "drv_ep_blp_rom_00" >> +#define XRT_MD_NODE_MAILBOX_VSEC "ep_mailbox_vsec_00" >> +#define XRT_MD_NODE_PLAT_INFO "drv_ep_platform_info_mgmt_00" >> +#define XRT_MD_NODE_TEST "drv_ep_test_00" >> +#define XRT_MD_NODE_MGMT_MAIN "drv_ep_mgmt_main_00" >> +#define XRT_MD_NODE_FLASH_VSEC "drv_ep_card_flash_program_00" >> +#define XRT_MD_NODE_GOLDEN_VER "drv_ep_golden_ver_00" >> +#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_BLP "partition_info_0" >> +#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_PLP "partition_info_1" >> + >> +#define XRT_MD_NODE_DDR_SRSR "drv_ep_ddr_srsr" >> +#define XRT_MD_REGMAP_DDR_SRSR "drv_ddr_srsr" >> + >> +#define XRT_MD_PROP_OFFSET "drv_offset" >> +#define XRT_MD_PROP_CLK_FREQ "drv_clock_frequency" >> +#define XRT_MD_PROP_CLK_CNT "drv_clock_frequency_counter" >> +#define XRT_MD_PROP_VBNV "vbnv" >> +#define XRT_MD_PROP_VROM "vrom" >> +#define XRT_MD_PROP_PARTITION_LEVEL "partition_level" >> + >> +struct xrt_md_endpoint { >> + const char *ep_name; >> + u32 bar; >> + long bar_off; >> + ulong size; >> + char *regmap; >> + char *regmap_ver; >> +}; > could the existing 'struct regmap' be used here ? This structure is just a direct translation of firmware node. It is simple description of hardware endpoint. >> + >> +/* Note: res_id is defined by leaf driver and must start with 0. */ >> +struct xrt_iores_map { >> + char *res_name; >> + int res_id; >> +}; >> + >> +static inline int xrt_md_res_name2id(const struct xrt_iores_map *res_map, >> + int entry_num, const char *res_name) >> +{ >> + int i; >> + >> + for (i = 0; i < entry_num; i++) { >> + if (!strcmp(res_name, res_map->res_name)) > Use the 'n' variant strncmp for better safety. > > Fix generally. Will replace all strcmp() > >> + return res_map->res_id; >> + res_map++; >> + } >> + return -1; >> +} >> + >> +static inline const char * >> +xrt_md_res_id2name(const struct xrt_iores_map *res_map, int entry_num, int id) >> +{ >> + int i; >> + >> + for (i = 0; i < entry_num; i++) { >> + if (res_map->res_id == id) >> + return res_map->res_name; >> + res_map++; >> + } >> + return NULL; >> +} >> + >> +unsigned long xrt_md_size(struct device *dev, const char *blob); >> +int xrt_md_create(struct device *dev, char **blob); >> +int xrt_md_add_endpoint(struct device *dev, char *blob, >> + struct xrt_md_endpoint *ep); >> +int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name, >> + char *regmap_name); >> +int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name, >> + const char *regmap_name, const char *prop, >> + const void **val, int *size); >> +int xrt_md_set_prop(struct device *dev, char *blob, const char *ep_name, >> + const char *regmap_name, const char *prop, >> + const void *val, int size); >> +int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob, >> + const char *ep_name, const char *regmap_name, >> + const char *new_ep_name); >> +int xrt_md_get_next_endpoint(struct device *dev, const char *blob, >> + const char *ep_name, const char *regmap_name, >> + char **next_ep, char **next_regmap); >> +int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob, >> + const char *regmap_name, const char **ep_name); >> +int xrt_md_find_endpoint(struct device *dev, const char *blob, >> + const char *ep_name, const char *regmap_name, >> + const char **epname); >> +void xrt_md_pack(struct device *dev, char *blob); >> +char *xrt_md_dup(struct device *dev, const char *blob); >> +int xrt_md_get_intf_uuids(struct device *dev, const char *blob, >> + u32 *num_uuids, uuid_t *intf_uuids); >> +static inline int xrt_md_copy_all_endpoints(struct device *dev, char *blob, const char *src_blob) >> +{ >> + return xrt_md_copy_endpoint(dev, blob, src_blob, XRT_MD_NODE_ENDPOINTS, >> + NULL, NULL); > A wrapping a single call seems like an unnecessary abstraction layer. > > can this be reduced/removed ? Will remove. > >> +} >> + >> +/* >> + * Firmware provides 128 bit hash string as unque id of partition/interface. > The firmware provides a 128 bit hash string as a unique id to the partition/interface. > > Existing hw does not yet use the cononical form, so it is necessary to use a translation function. Will fix > >> + * This string will be canonical textual representation in the future. >> + * Before that, introducing these two functions below to translate >> + * hash string to uuid_t for released hardware. > Is there an existing version string the new hw will use to check which way to go ? We do not have the new hw yet. >> + */ >> +static inline void xrt_md_trans_uuid2str(const uuid_t *uuid, char *uuidstr) >> +{ >> + int i, p; >> + u8 *u = (u8 *)uuid; >> + >> + for (p = 0, i = sizeof(uuid_t) - 1; i >= 0; p++, i--) > This loop needs to be improved. > > Consider if sizeof(uuid_t) changed, accessing u[] would overflow. Will fix all the issues of uuid2str()/str2uuid() you pointed out. The rough idea is to use export_uuid() and import_uuid(). > >> + (void)snprintf(&uuidstr[p * 2], 3, "%02x", u[i]); > (void) cast isn't needed. >> +} >> + >> +static inline int xrt_md_trans_str2uuid(struct device *dev, const char *uuidstr, uuid_t *p_uuid) >> +{ >> + char *p; >> + const char *str; >> + char tmp[3] = { 0 }; >> + int i, ret; >> + >> + memset(p_uuid, 0, sizeof(*p_uuid)); >> + p = (char *)p_uuid; > access with p_uuid->b[] would remove use of 'p' >> + str = uuidstr + strlen(uuidstr) - 2; >> + >> + for (i = 0; i < sizeof(*p_uuid) && str >= uuidstr; i++) { > is not filling p_uuid completely really ok ? >> + tmp[0] = *str; >> + tmp[1] = *(str + 1); >> + ret = kstrtou8(tmp, 16, p); >> + if (ret) >> + return -EINVAL; >> + p++; >> + str -= 2; >> + } >> + >> + return 0; >> +} >> + >> +#endif >> diff --git a/drivers/fpga/xrt/metadata/metadata.c b/drivers/fpga/xrt/metadata/metadata.c >> new file mode 100644 >> index 000000000000..5d106396f438 >> --- /dev/null >> +++ b/drivers/fpga/xrt/metadata/metadata.c >> @@ -0,0 +1,524 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Xilinx Alveo FPGA Metadata parse APIs >> + * >> + * Copyright (C) 2020-2021 Xilinx, Inc. >> + * >> + * Authors: >> + * Lizhi Hou <Lizhi.Hou@xilinx.com> >> + */ >> + >> +#include <linux/libfdt_env.h> >> +#include "libfdt.h" >> +#include "metadata.h" >> + >> +#define MAX_BLOB_SIZE (4096 * 25) >> + >> +static int xrt_md_setprop(struct device *dev, char *blob, int offset, >> + const char *prop, const void *val, int size); >> +static int xrt_md_overlay(struct device *dev, char *blob, int target, >> + const char *overlay_blob, int overlay_offset); >> +static int xrt_md_get_endpoint(struct device *dev, const char *blob, >> + const char *ep_name, const char *regmap_name, >> + int *ep_offset); > consider reordering functions so these fwd decl's are not needed Will remove decl's. >> + >> +unsigned long xrt_md_size(struct device *dev, const char *blob) >> +{ >> + unsigned long len = (long)fdt_totalsize(blob); >> + >> + len = (len > MAX_BLOB_SIZE) ? XRT_MD_INVALID_LENGTH : len; >> + return len; >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_size); >> + >> +int xrt_md_create(struct device *dev, char **blob) >> +{ >> + int ret = 0; >> + >> + WARN_ON(!blob); >> + >> + *blob = vmalloc(MAX_BLOB_SIZE); > why vmalloc instead of vzalloc ? Will use vzalloc. >> + if (!*blob) >> + return -ENOMEM; >> + >> + ret = fdt_create_empty_tree(*blob, MAX_BLOB_SIZE); >> + if (ret) { >> + dev_err(dev, "format blob failed, ret = %d", ret); >> + goto failed; >> + } >> + >> + ret = fdt_next_node(*blob, -1, NULL); > A variable called 'offset' would make more sense here because it is used later in fdt_add_subnode as the parentoffset parameter. the following fdt_add_subnode should use 0 as the parentoffset parameter. So I will keep 'ret' here. >> + if (ret < 0) { >> + dev_err(dev, "No Node, ret = %d", ret); >> + goto failed; >> + } >> + >> + ret = fdt_add_subnode(*blob, ret, XRT_MD_NODE_ENDPOINTS); > why isn't the parentoffset '0' ? Yes, 0 should be used as parentoffset. > > >> + if (ret < 0) { >> + dev_err(dev, "add node failed, ret = %d", ret); >> + goto failed; >> + } >> + >> + return 0; >> + >> +failed: >> + vfree(*blob); >> + *blob = NULL; >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_create); >> + >> +static int xrt_md_add_node(struct device *dev, char *blob, int parent_offset, >> + const char *ep_name) >> +{ >> + int ret; >> + >> + ret = fdt_add_subnode(blob, parent_offset, ep_name); >> + if (ret < 0 && ret != -FDT_ERR_EXISTS) >> + dev_err(dev, "failed to add node %s. %d", ep_name, ret); >> + >> + return ret; >> +} >> + >> +int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name, >> + char *regmap_name) >> +{ >> + int ret; >> + int ep_offset; >> + >> + ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name, &ep_offset); >> + if (ret) { >> + dev_err(dev, "can not find ep %s", ep_name); >> + return -EINVAL; >> + } >> + >> + ret = fdt_del_node(blob, ep_offset); >> + if (ret) >> + dev_err(dev, "delete node %s failed, ret %d", ep_name, ret); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_del_endpoint); >> + >> +static int __xrt_md_add_endpoint(struct device *dev, char *blob, >> + struct xrt_md_endpoint *ep, int *offset, bool root) >> +{ >> + int ret = 0; >> + int ep_offset = 0; >> + u32 val, count = 0; >> + u64 io_range[2]; >> + char comp[128]; >> + >> + if (!ep->ep_name) { >> + dev_err(dev, "empty name"); >> + return -EINVAL; >> + } >> + >> + if (!root) { >> + ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL, >> + &ep_offset); >> + if (ret) { >> + dev_err(dev, "invalid blob, ret = %d", ret); >> + return -EINVAL; >> + } >> + } >> + >> + ep_offset = xrt_md_add_node(dev, blob, ep_offset, ep->ep_name); >> + if (ep_offset < 0) { >> + dev_err(dev, "add endpoint failed, ret = %d", ret); >> + return -EINVAL; >> + } >> + if (offset) >> + *offset = ep_offset; >> + >> + if (ep->size != 0) { >> + val = cpu_to_be32(ep->bar); >> + ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_BAR_IDX, >> + &val, sizeof(u32)); >> + if (ret) { >> + dev_err(dev, "set %s failed, ret %d", >> + XRT_MD_PROP_BAR_IDX, ret); >> + goto failed; >> + } >> + io_range[0] = cpu_to_be64((u64)ep->bar_off); >> + io_range[1] = cpu_to_be64((u64)ep->size); > is there a type mismatch between bar (u32) and size (u64) ? Do you mean ep->bar? 'ep->bar' is bar index. So it does not have type mismatch issue. >> + ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_IO_OFFSET, >> + io_range, sizeof(io_range)); >> + if (ret) { >> + dev_err(dev, "set %s failed, ret %d", >> + XRT_MD_PROP_IO_OFFSET, ret); >> + goto failed; >> + } >> + } >> + >> + if (ep->regmap) { >> + if (ep->regmap_ver) { >> + count = snprintf(comp, sizeof(comp), >> + "%s-%s", ep->regmap, ep->regmap_ver); >> + count++; >> + } >> + >> + count += snprintf(comp + count, sizeof(comp) - count, >> + "%s", ep->regmap); >> + count++; > unlikely, but overflow is not checked. Will add overflow check. > > are multiple null's in this string ok ? yes. > >> + >> + ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_COMPATIBLE, >> + comp, count); >> + if (ret) { >> + dev_err(dev, "set %s failed, ret %d", >> + XRT_MD_PROP_COMPATIBLE, ret); >> + goto failed; >> + } >> + } >> + >> +failed: >> + if (ret) >> + xrt_md_del_endpoint(dev, blob, ep->ep_name, NULL); >> + >> + return ret; >> +} >> + >> +int xrt_md_add_endpoint(struct device *dev, char *blob, >> + struct xrt_md_endpoint *ep) >> +{ >> + return __xrt_md_add_endpoint(dev, blob, ep, NULL, false); > ok, user doesn't add root's endpoint. > > could an assert be added ? The word 'root' is misleading here. Will change this argument to 'char *parent' > >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_add_endpoint); >> + >> +static int xrt_md_get_endpoint(struct device *dev, const char *blob, >> + const char *ep_name, const char *regmap_name, >> + int *ep_offset) >> +{ >> + int offset; >> + const char *name; >> + >> + for (offset = fdt_next_node(blob, -1, NULL); >> + offset >= 0; >> + offset = fdt_next_node(blob, offset, NULL)) { >> + name = fdt_get_name(blob, offset, NULL); >> + if (!name || strncmp(name, ep_name, strlen(ep_name) + 1)) > strlen() + 1 ? This is to make sure "12345" and "123" are different. >> + continue; >> + if (!regmap_name || >> + !fdt_node_check_compatible(blob, offset, regmap_name)) >> + break; >> + } >> + if (offset < 0) >> + return -ENODEV; >> + >> + *ep_offset = offset; >> + >> + return 0; >> +} >> + >> +int xrt_md_find_endpoint(struct device *dev, const char *blob, >> + const char *ep_name, const char *regmap_name, >> + const char **epname) >> +{ >> + int offset; >> + int ret; >> + >> + ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name, >> + &offset); >> + if (!ret && epname && offset >= 0) > The offset >= 0 check isn't needed, !ret is enough Will fix. >> + *epname = fdt_get_name(blob, offset, NULL); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_find_endpoint); >> + >> +int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name, >> + const char *regmap_name, const char *prop, >> + const void **val, int *size) >> +{ >> + int offset; >> + int ret; >> + >> + if (val) >> + *val = NULL; > Seems like no point in making this call if !val > > Return -EINVAL if !val and remove the if (val) check below. Will fix > >> + if (ep_name) { >> + ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name, >> + &offset); >> + if (ret) { >> + dev_err(dev, "cannot get ep %s, regmap %s, ret = %d", >> + ep_name, regmap_name, ret); >> + return -EINVAL; >> + } >> + } else { >> + offset = fdt_next_node(blob, -1, NULL); >> + if (offset < 0) { >> + dev_err(dev, "internal error, ret = %d", offset); >> + return -EINVAL; >> + } >> + } >> + >> + if (val) { >> + *val = fdt_getprop(blob, offset, prop, size); >> + if (!*val) { >> + dev_dbg(dev, "get ep %s, prop %s failed", ep_name, prop); >> + return -EINVAL; >> + } >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_get_prop); >> + >> +static int xrt_md_setprop(struct device *dev, char *blob, int offset, >> + const char *prop, const void *val, int size) >> +{ >> + int ret; >> + >> + ret = fdt_setprop(blob, offset, prop, val, size); >> + if (ret) >> + dev_err(dev, "failed to set prop %d", ret); >> + >> + return ret; >> +} >> + >> +int xrt_md_set_prop(struct device *dev, char *blob, >> + const char *ep_name, const char *regmap_name, >> + const char *prop, const void *val, int size) >> +{ >> + int offset; >> + int ret; >> + >> + if (ep_name) { >> + ret = xrt_md_get_endpoint(dev, blob, ep_name, >> + regmap_name, &offset); >> + if (ret) { >> + dev_err(dev, "cannot get node %s, ret = %d", >> + ep_name, ret); >> + return -EINVAL; >> + } >> + } else { >> + offset = fdt_next_node(blob, -1, NULL); >> + if (offset < 0) { >> + dev_err(dev, "internal error, ret = %d", offset); >> + return -EINVAL; >> + } >> + } > This if-else block is a cut-n-paste from above. > > It is good to convert common logic blocks to macros or inline functions. Will define a in-line function. > >> + >> + ret = xrt_md_setprop(dev, blob, offset, prop, val, size); >> + if (ret) >> + dev_err(dev, "set prop %s failed, ret = %d", prop, ret); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_set_prop); >> + >> +int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob, >> + const char *ep_name, const char *regmap_name, >> + const char *new_ep_name) >> +{ >> + int offset, target; >> + int ret; >> + struct xrt_md_endpoint ep = {0}; >> + const char *newepnm = new_ep_name ? new_ep_name : ep_name; > How is this valid ? > > The xrt_md_get_endpoint searches by ep_name and if there names are not unique the second one will never be found. ep_name in the firmware will always be unique. It is documented in xrt.rst > >> + >> + ret = xrt_md_get_endpoint(dev, src_blob, ep_name, regmap_name, >> + &offset); >> + if (ret) >> + return -EINVAL; >> + >> + ret = xrt_md_get_endpoint(dev, blob, newepnm, regmap_name, &target); >> + if (ret) { >> + ep.ep_name = newepnm; >> + ret = __xrt_md_add_endpoint(dev, blob, &ep, &target, >> + fdt_parent_offset(src_blob, offset) == 0); >> + if (ret) >> + return -EINVAL; >> + } >> + >> + ret = xrt_md_overlay(dev, blob, target, src_blob, offset); >> + if (ret) >> + dev_err(dev, "overlay failed, ret = %d", ret); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_copy_endpoint); >> + >> +char *xrt_md_dup(struct device *dev, const char *blob) >> +{ >> + int ret; >> + char *dup_blob; >> + >> + ret = xrt_md_create(dev, &dup_blob); >> + if (ret) >> + return NULL; >> + ret = xrt_md_overlay(dev, dup_blob, -1, blob, -1); > would memcpy-ing the blob work ? Sure. Memcpy works. I will remove xrt_md_dup() and use memcpy. >> + if (ret) { >> + vfree(dup_blob); >> + return NULL; >> + } >> + >> + return dup_blob; >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_dup); >> + >> +static int xrt_md_overlay(struct device *dev, char *blob, int target, >> + const char *overlay_blob, int overlay_offset) >> +{ >> + int property, subnode; >> + int ret; >> + >> + WARN_ON(!blob || !overlay_blob); >> + >> + if (!blob) { >> + dev_err(dev, "blob is NULL"); >> + return -EINVAL; >> + } >> + >> + if (target < 0) { >> + target = fdt_next_node(blob, -1, NULL); >> + if (target < 0) { >> + dev_err(dev, "invalid target"); >> + return -EINVAL; >> + } >> + } >> + if (overlay_offset < 0) { >> + overlay_offset = fdt_next_node(overlay_blob, -1, NULL); >> + if (overlay_offset < 0) { >> + dev_err(dev, "invalid overlay"); >> + return -EINVAL; >> + } >> + } >> + >> + fdt_for_each_property_offset(property, overlay_blob, overlay_offset) { >> + const char *name; >> + const void *prop; >> + int prop_len; >> + >> + prop = fdt_getprop_by_offset(overlay_blob, property, &name, >> + &prop_len); >> + if (!prop || prop_len >= MAX_BLOB_SIZE) { > could add prop_len < 0 as another sanity check Will add. >> + dev_err(dev, "internal error"); >> + return -EINVAL; >> + } >> + >> + ret = xrt_md_setprop(dev, blob, target, name, prop, >> + prop_len); >> + if (ret) { >> + dev_err(dev, "setprop failed, ret = %d", ret); >> + return ret; > overlay_blob is half done, as an error handling shouldn't it be undone ? The error will only cause a half done blob. Nothing need to be undone. >> + } >> + } >> + >> + fdt_for_each_subnode(subnode, overlay_blob, overlay_offset) { >> + const char *name = fdt_get_name(overlay_blob, subnode, NULL); >> + int nnode; >> + >> + nnode = xrt_md_add_node(dev, blob, target, name); >> + if (nnode == -FDT_ERR_EXISTS) >> + nnode = fdt_subnode_offset(blob, target, name); >> + if (nnode < 0) { >> + dev_err(dev, "add node failed, ret = %d", nnode); >> + return nnode; >> + } >> + >> + ret = xrt_md_overlay(dev, blob, nnode, overlay_blob, subnode); > eek, recursion. > > Any chance this will blow the stack ? Will add max recursive depth (5). > >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +int xrt_md_get_next_endpoint(struct device *dev, const char *blob, >> + const char *ep_name, const char *regmap_name, >> + char **next_ep, char **next_regmap) >> +{ >> + int offset, ret; >> + >> + if (!ep_name) { >> + ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL, >> + &offset); >> + } else { >> + ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name, >> + &offset); >> + } >> + >> + if (ret) { >> + *next_ep = NULL; >> + *next_regmap = NULL; > could initialize next_ep and next_regmap to NULL outside the check. Will move outside. >> + return -EINVAL; >> + } >> + >> + offset = ep_name ? fdt_next_subnode(blob, offset) : >> + fdt_first_subnode(blob, offset); >> + if (offset < 0) { >> + *next_ep = NULL; >> + *next_regmap = NULL; >> + return -EINVAL; >> + } >> + >> + *next_ep = (char *)fdt_get_name(blob, offset, NULL); >> + *next_regmap = (char *)fdt_stringlist_get(blob, offset, XRT_MD_PROP_COMPATIBLE, >> + 0, NULL); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_get_next_endpoint); >> + >> +int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob, >> + const char *regmap_name, const char **ep_name) >> +{ >> + int ep_offset; >> + >> + ep_offset = fdt_node_offset_by_compatible(blob, -1, regmap_name); >> + if (ep_offset < 0) { >> + *ep_name = NULL; >> + return -ENOENT; >> + } >> + >> + *ep_name = (char *)fdt_get_name(blob, ep_offset, NULL); > why the cast ? Will remove. >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_get_compatible_endpoint); >> + >> +void xrt_md_pack(struct device *dev, char *blob) >> +{ >> + int ret; >> + >> + ret = fdt_pack(blob); >> + if (ret) >> + dev_err(dev, "pack failed %d", ret); > maybe return int Will return int. >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_pack); >> + > The input/output of num_uuids parameter is tricky add a comment Will modify to 'u32 num_uuids' and return the actual number >> +int xrt_md_get_intf_uuids(struct device *dev, const char *blob, >> + u32 *num_uuids, uuid_t *intf_uuids) > what is intf ? change to 'interface' ? yes. And will change to 'interface'. >> +{ >> + int offset, count = 0; >> + int ret; >> + const char *uuid_str; >> + >> + ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_INTERFACES, NULL, &offset); >> + if (ret) >> + return -ENOENT; >> + >> + for (offset = fdt_first_subnode(blob, offset); >> + offset >= 0; >> + offset = fdt_next_subnode(blob, offset)) { >> + uuid_str = fdt_getprop(blob, offset, XRT_MD_PROP_INTERFACE_UUID, >> + NULL); >> + if (!uuid_str) { >> + dev_err(dev, "empty intf uuid node"); >> + return -EINVAL; >> + } >> + >> + if (intf_uuids && count < *num_uuids) { >> + ret = xrt_md_trans_str2uuid(dev, uuid_str, >> + &intf_uuids[count]); >> + if (ret) >> + return -EINVAL; >> + } >> + count++; > keep going even when count > num_uuids ? > > that seems like an error. Will add check.
Thanks, Lizhi > > Tom > >> + } >> + >> + *num_uuids = count; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(xrt_md_get_intf_uuids);
| |