On 23/10/2023 11:20, Zhenhua Huang wrote: > Qualcomm memory dump driver initializes system memory dump table. > Firmware dumps system cache, internal memory, peripheral registers > to DDR as per this table after system crashes and warm resets. The > driver reserves memory, populates ids and sizes for firmware dumping > according to the configuration. > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@xxxxxxxxxxx> > --- ... > + > +/* populate allocated region */ > +static int __init mem_dump_populate_mem(struct device *dev, > + struct page *start_page, > + size_t total_size) > +{ > + struct qcom_memory_dump *memdump = dev_get_drvdata(dev); > + struct qcom_dump_entry dump_entry; > + struct qcom_dump_data *dump_data; > + struct xbc_node *linked_list; > + phys_addr_t phys_end_addr; > + phys_addr_t phys_addr; > + const char *size_p; > + void *dump_vaddr; > + const char *id_p; > + int ret = 0; > + int size; > + int id; > + > + phys_addr = page_to_phys(start_page); > + phys_end_addr = phys_addr + total_size; > + dump_vaddr = page_to_virt(start_page); > + > + ret = mem_dump_register_data_table(dev, dump_vaddr, phys_addr); > + if (ret) { > + dev_err_probe(dev, ret, "Mem Dump table set up is failed\n"); > + return ret; That's not the syntax. Syntax is return dev_err_probe > + } > + > + ret = qcom_init_memdump_imem_area(dev, total_size); > + if (ret) > + return ret; > + > + /* Apply two tables: QCOM_DUMP_TYPE_TABLE and QCOM_DUMP_TYPE_DATA */ > + mem_dump_apply_offset(&dump_vaddr, &phys_addr, > + sizeof(struct qcom_dump_table) * 2); > + > + /* Both "id" and "size" must be present */ > + xbc_node_for_each_subkey(memdump->mem_dump_node, linked_list) { > + const char *name = xbc_node_get_data(linked_list); > + > + if (!name) > + continue; > + > + id_p = xbc_node_find_value(linked_list, "id", NULL); > + size_p = xbc_node_find_value(linked_list, "size", NULL); > + > + if (id_p && size_p) { > + ret = kstrtoint(id_p, 0, &id); > + if (ret) > + continue; > + > + ret = kstrtoint(size_p, 0, &size); > + > + if (ret) > + continue; > + > + /* > + * Physical layout: starting from two qcom_dump_data. > + * Following are respective dump meta data and reserved regions. > + * Qcom_dump_data is populated by the driver, fw parse it > + * and dump respective info into dump mem. > + * Illustrate the layout: > + * > + * +------------------------+------------------------+ > + * | qcom_dump_table(TABLE) | qcom_dump_table(DATA) | > + * +------------------------+------------------------+ > + * +-------------+----------+-------------+----------+ > + * |qcom_dump_data| dump mem|qcom_dump_data| dump mem | > + * +-------------+----------+-------------+----------+ > + * +-------------+----------+-------------+----------+ > + * |qcom_dump_data| dump mem|qcom_dump_data| dump mem | > + * +-------------+----------+-------------+----------+ > + * ... > + */ You have wrong indentation here. > + dump_data = dump_vaddr; > + dump_data->addr = phys_addr + QCOM_DUMP_DATA_SIZE; > + dump_data->len = size; > + dump_entry.id = id; > + strscpy(dump_data->name, name, > + sizeof(dump_data->name)); > + dump_entry.addr = phys_addr; > + ret = mem_dump_data_register(dev, QCOM_DUMP_TABLE_LINUX, > + &dump_entry); > + if (ret) { > + dev_err_probe(dev, ret, "Dump data setup failed, id = %d\n", > + id); Syntax is return dev_err_probe > + return ret; > + } > + > + mem_dump_apply_offset(&dump_vaddr, &phys_addr, > + size + QCOM_DUMP_DATA_SIZE); > + if (phys_addr > phys_end_addr) { > + dev_err_probe(dev, -ENOMEM, "Exceeding allocated region\n"); ENOMEM? Does not look right then. > + return -ENOMEM; > + } > + } else { > + continue; > + } > + } > + > + return ret; > +} > + > +static int __init mem_dump_probe(struct platform_device *pdev) > +{ > + struct qcom_memory_dump *memdump; > + struct device *dev = &pdev->dev; > + struct page *page; > + size_t total_size; > + int ret = 0; > + > + memdump = devm_kzalloc(dev, sizeof(struct qcom_memory_dump), > + GFP_KERNEL); > + if (!memdump) > + return -ENOMEM; > + > + dev_set_drvdata(dev, memdump); > + > + /* check and initiate CMA region */ > + ret = mem_dump_reserve_mem(dev); > + if (ret) > + return ret; > + > + /* allocate and populate */ > + page = mem_dump_alloc_mem(dev, &total_size); > + if (IS_ERR(page)) { > + ret = PTR_ERR(page); > + dev_err_probe(dev, ret, "mem dump alloc failed\n"); No, the syntax is: ret = dev_err_probe But why do you print messgaes for memory allocations? > + goto release; > + } > + > + ret = mem_dump_populate_mem(dev, page, total_size); > + if (!ret) > + dev_info(dev, "Mem dump region populated successfully\n"); Drop simple probe success messages. Not needed. > + else > + goto free; > + > + return 0; > + > +free: > + cma_release(dev_get_cma_area(dev), page, (total_size / PAGE_SIZE)); > + > +release: > + of_reserved_mem_device_release(dev); Where is cleanup on unbind? > + return ret; > +} > + > +static const struct of_device_id mem_dump_match_table[] = { > + {.compatible = "qcom,mem-dump",}, > + {} > +}; > + Best regards, Krzysztof