Message-ID: <87a4xwkzuv.fsf@bootlin.com>
Date: Thu, 29 Jan 2026 20:01:12 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Junhao Xie <bigfoot@...xa.com>
Cc: Bjorn Andersson <andersson@...nel.org>,  Konrad Dybcio
 <konradybcio@...nel.org>,  Richard Weinberger <richard@....at>,  Vignesh
 Raghavendra <vigneshr@...com>,  linux-arm-msm@...r.kernel.org,
  linux-kernel@...r.kernel.org,  linux-mtd@...ts.infradead.org,  Xilin Wu
 <sophon@...xa.com>
Subject: Re: [PATCH v2 2/2] mtd: devices: Add Qualcomm SCM storage driver

Hello Junhao,

> +static int qcom_scm_storage_erase(struct mtd_info *mtd,
> +				  struct erase_info *instr)
> +{
> +	struct qcom_scm_storage *host = mtd->priv;
> +	size_t block_size;
> +
> +	if (instr->addr % host->mtd.erasesize ||
> +	    instr->len % host->mtd.erasesize)
> +		return -EINVAL;

Can theses situations realistically happen? Erases are in theory
eraseblock aligned, so I doubt this case shall be checked in device drivers.

> +
> +	block_size = le32_to_cpu(host->info.block_size);

You seem to store the SCM info structure as-is and always make
conversions on the fly. May I suggest to store the fields you need
directly in the correct format/endianness?

> +
> +	guard(mutex)(&host->lock);
> +
> +	return qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
> +					 QCOM_SCM_STORAGE_ERASE,
> +					 instr->addr / block_size,

Actually what you need instead of block_size is to use mtd->erasesize
directly, no need to store "block_size" in your structure, you already
have it.

> +					 0, instr->len);
> +}
> +
> +static int qcom_scm_storage_read(struct mtd_info *mtd,
> +				 loff_t from, size_t len,
> +				 size_t *retlen, u_char *buf)

u_char? Can we use a more common type?

> +{
> +	struct qcom_scm_storage *host = mtd->priv;
> +	loff_t block_start, block_off, lba;
> +	size_t block_size, chunk, to_read;
> +	int ret;
> +
> +	if (retlen)
> +		*retlen = 0;

Are there cases where retlen can be absent?

> +
> +	if (from + len > mtd->size)
> +		return -EINVAL;

This check should be done at the core level already.

> +	if (len == 0)
> +		return 0;

Ditto (see mtdchar_read()).

> +	block_size = le32_to_cpu(host->info.block_size);
> +
> +	guard(mutex)(&host->lock);
> +
> +	while (len > 0) {
> +		block_start = round_down(from, block_size);
> +		block_off = from - block_start;
> +		lba = block_start / block_size;
> +
> +		if (block_off || len < block_size) {
> +			chunk = min_t(size_t, block_size - block_off, len);
> +			to_read = block_size;
> +		} else {
> +			chunk = round_down(len, block_size);
> +			chunk = min_t(size_t, chunk, host->buffer_size);
> +			to_read = chunk;
> +		}
> +
> +		ret = qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
> +						QCOM_SCM_STORAGE_READ,
> +						lba, host->buffer,
> +						to_read);
> +		if (ret)
> +			return ret;
> +
> +		memcpy(buf, host->buffer + block_off, chunk);
> +
> +		buf += chunk;
> +		from += chunk;
> +		len -= chunk;
> +		if (retlen)
> +			*retlen += chunk;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_scm_storage_write(struct mtd_info *mtd,
> +				  loff_t to, size_t len,
> +				  size_t *retlen, const u_char *buf)
> +{
> +	struct qcom_scm_storage *host = mtd->priv;
> +	loff_t block_start, block_off, lba;
> +	size_t block_size, chunk, to_write;
> +	int ret;
> +
> +	if (retlen)
> +		*retlen = 0;
> +
> +	if (to + len > mtd->size)
> +		return -EINVAL;
> +
> +	if (len == 0)
> +		return 0;
> +
> +	block_size = le32_to_cpu(host->info.block_size);
> +
> +	guard(mutex)(&host->lock);
> +
> +	while (len > 0) {
> +		block_start = round_down(to, block_size);
> +		block_off = to - block_start;
> +		lba = block_start / block_size;
> +
> +		if (block_off || len < block_size) {
> +			chunk = min_t(size_t, block_size - block_off, len);
> +			to_write = block_size;
> +
> +			ret = qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
> +							QCOM_SCM_STORAGE_READ,
> +							lba, host->buffer,
> +							block_size);

This is strange. Does it really work? My understanding: you're trying to
handle a non aligned write access (I'm not even sure this is possible as
you set writesize == erasesize) by reading the existing data before
writing. If that's what you intend to do, you'll forcibly get 1s instead
of actual data because erases are mandatory before writes, which means
your read will happen too late.

I might be totally wrong, but I believe this approach is
incorrect. Please explain what you're trying to do otherwise.

> +			if (ret)
> +				return ret;
> +		} else {
> +			chunk = round_down(len, block_size);
> +			chunk = min_t(size_t, chunk, host->buffer_size);
> +			to_write = chunk;
> +		}
> +
> +		memcpy(host->buffer + block_off, buf, chunk);
> +
> +		ret = qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
> +						QCOM_SCM_STORAGE_WRITE,
> +						lba, host->buffer,
> +						to_write);
> +		if (ret)
> +			return ret;
> +
> +		buf += chunk;
> +		to += chunk;
> +		len -= chunk;
> +		if (retlen)
> +			*retlen += chunk;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_scm_storage_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_scm_storage *host;
> +	u64 total_blocks, serial_num;
> +	u32 block_size;
> +	int ret;
> +
> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> +	if (!host)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, host);
> +	host->dev = dev;
> +
> +	ret = devm_mutex_init(dev, &host->lock);
> +	if (ret)
> +		return ret;
> +
> +	host->buffer_size = SZ_256K;
> +	host->buffer = devm_kzalloc(dev, host->buffer_size, GFP_KERNEL);

Do you really need 256K of adjacent memory? If this is a DMA-able
buffer, then yes. Also, the check (block_size vs. buffer_size) should
happen before this allocation, and you should not need to allocate 256k
blindly, just use the NOR geometry knowledge.

> +	if (!host->buffer)
> +		return -ENOMEM;
> +
> +	ret = qcom_scm_storage_send_cmd(QCOM_SCM_STORAGE_SPINOR,
> +					QCOM_SCM_STORAGE_GET_INFO,
> +					0, &host->info,
> +					sizeof(host->info));
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "failed to get storage info\n");

Maybe this print can be enhanced, it may also mean the firmware does not
implement the required system call?

> +
> +	total_blocks = le64_to_cpu(host->info.total_blocks);
> +	serial_num = le64_to_cpu(host->info.serial_num);
> +	block_size = le32_to_cpu(host->info.block_size);
> +
> +	if (!block_size || !total_blocks)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "invalid storage geometry\n");
> +
> +	if (block_size > host->buffer_size)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "block size %u exceeds buffer size\n",
> +				     block_size);

I am now in favour of reducing logs in the kernel, maybe you can group
these two prints into one, indicating the info you get from firmware is
incorrect? Also, what is the point of using dev_err_probe() here, as you
cannot have an EPROBE_DEFER?

> +
> +	host->mtd.priv = host;
> +	host->mtd.name = dev_name(dev);
> +	host->mtd.owner = THIS_MODULE;
> +	host->mtd.dev.parent = dev;
> +	host->mtd.size = total_blocks * block_size;
> +	host->mtd.erasesize = block_size;
> +	host->mtd.writesize = block_size;
> +	host->mtd.writebufsize = block_size;
> +	host->mtd.type = MTD_NORFLASH;
> +	host->mtd.flags = MTD_WRITEABLE;
> +	host->mtd._erase = qcom_scm_storage_erase;
> +	host->mtd._read = qcom_scm_storage_read;
> +	host->mtd._write = qcom_scm_storage_write;
> +
> +	dev_info(dev, "scm storage 0x%llx registered with size %llu bytes\n",
> +		 serial_num, host->mtd.size);

Please drop this message, it is not useful.

...

> +static struct platform_driver qcom_scm_storage_driver = {
> +	.probe	= qcom_scm_storage_probe,
> +	.remove	= qcom_scm_storage_remove,
> +	.driver = {
> +		.name	= "qcom_scm_storage",
> +	},
> +	.id_table = qcom_scm_storage_ids,

I am surprised you have an id_table here, this is likely not for an OF
based platform, do you confirm?

Thanks,
Miquèl

Powered by blists - more mailing lists