Re: [PATCH v1 3/5] soc: qcom: memory_dump: Add memory dump driver

[Zhenhua Huang]

On 2023/10/23 19:46, Krzysztof Kozlowski wrote:
> On 23/10/2023 13:43, Zhenhua Huang wrote:
> > > > +
> > > > +			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.
> >
> > ENOMEM means the memory allocated not enough? any suggestion? Thanks.
>
> The error code is okay, but we rarely need to print error messages for
> memory allocation failures. Core prints it already.

It's not same as below case. Allocation is successful here, while the 
driver used more than allocated size.

> > > > +				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?
> >
> > Do you think it's useless because mm codes should print error as well if
> > failure ?
>
> We fixed this in kernel long, long, long time ago so we have even
> coccicheck scripts to find misuses.

Thanks, yeah.. I know mm codes already have for example warn_alloc() to 
print error messages. Thanks for pointing out.

> Best regards,
> Krzysztof

Thanks,
Zhenhua