| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4a0e51cf-d29a-5843-9c61-1d31a9721a3d@quicinc.com>
Date: Tue, 20 Feb 2024 17:24:43 +0530
From: Md Sadre Alam <quic_mdalam@...cinc.com>
To: Mark Brown <broonie@...nel.org>
CC: <andersson@...nel.org>, <konrad.dybcio@...aro.org>, <robh@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<miquel.raynal@...tlin.com>, <richard@....at>, <vigneshr@...com>,
<manivannan.sadhasivam@...aro.org>, <linux-arm-msm@...r.kernel.org>,
<linux-spi@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-mtd@...ts.infradead.org>,
<quic_srichara@...cinc.com>, <quic_varada@...cinc.com>
Subject: Re: [PATCH 3/5] spi: spi-qpic: Add qpic spi nand driver support
On 2/15/2024 7:44 PM, Mark Brown wrote:
> On Thu, Feb 15, 2024 at 07:18:54PM +0530, Md Sadre Alam wrote:
>
>> +config SPI_QPIC_SNAND
>> + tristate "QPIC SNAND controller"
>> + default y
>
> Why is this driver so special it should be enabled by default?
Sorry by mistake I kept this enabled by default, will change in next
patch.
>
>> + depends on ARCH_QCOM
>
> Please add an || COMPILE_TEST so this gets some build coverage.
Ok
>
>> + help
>> + QPIC_SNAND (QPIC SPI NAND) driver for Qualcomm QPIC controller.
>> + QPIC controller supports both parallel nand and serial nand.
>> + This config will enable serial nand driver for QPIC controller.
>> +
>> config SPI_QUP
>> tristate "Qualcomm SPI controller with QUP interface"
>> depends on ARCH_QCOM || COMPILE_TEST
>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>> index 4ff8d725ba5e..1ac3bac35007 100644
>> --- a/drivers/spi/Makefile
>> +++ b/drivers/spi/Makefile
>> @@ -153,6 +153,7 @@ obj-$(CONFIG_SPI_XTENSA_XTFPGA) += spi-xtensa-xtfpga.o
>> obj-$(CONFIG_SPI_ZYNQ_QSPI) += spi-zynq-qspi.o
>> obj-$(CONFIG_SPI_ZYNQMP_GQSPI) += spi-zynqmp-gqspi.o
>> obj-$(CONFIG_SPI_AMD) += spi-amd.o
>> +obj-$(CONFIG_SPI_QPIC_SNAND) += spi-qpic-snand.o
>
> Please keep this sorted.
Ok
>
>> --- /dev/null
>> +++ b/drivers/spi/spi-qpic-snand.c
>> @@ -0,0 +1,1025 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>
> Please make the entire comment a C++ one so things look more
> intentional.
Ok
>
>> +#define snandc_set_read_loc_first(snandc, reg, cw_offset, read_size, is_last_read_loc) \
>> +snandc_set_reg(snandc, reg, \
>> + ((cw_offset) << READ_LOCATION_OFFSET) | \
>> + ((read_size) << READ_LOCATION_SIZE) | \
>> + ((is_last_read_loc) << READ_LOCATION_LAST))
>> +
>> +#define snandc_set_read_loc_last(snandc, reg, cw_offset, read_size, is_last_read_loc) \
>> +snandc_set_reg(snandc, reg, \
>> + ((cw_offset) << READ_LOCATION_OFFSET) | \
>> + ((read_size) << READ_LOCATION_SIZE) | \
>> + ((is_last_read_loc) << READ_LOCATION_LAST))
>
> For type safety and legibility please write these as functions, mark
> them as static inline if needed.
Ok
>
>> +void snandc_set_reg(struct qcom_nand_controller *snandc, int offset, u32 val)
>> +{
>> + struct nandc_regs *regs = snandc->regs;
>> + __le32 *reg;
>> +
>> + reg = offset_to_nandc_reg(regs, offset);
>> +
>> + if (reg)
>> + *reg = cpu_to_le32(val);
>> +}
>
> This silently ignores writes to invalid registers, that doesn't seem
> great.
Ok
>
>> + return snandc->ecc_stats.failed ? -EBADMSG : snandc->ecc_stats.bitflips;
>
> For legibility please just write normal conditional statements.
Ok
>
>> +static int qpic_snand_program_execute(struct qcom_nand_controller *snandc,
>> + const struct spi_mem_op *op)
>> +{
>
>> + int num_cw = 4;
>
>> + data_buf = (u8 *)snandc->wbuf;
>
> Why the cast? If it's needed that smells like it's masking a bug, it
> looks like it's casting from a u8 * to a u8 *.
Ok Will fix in next patch.
>
>> + for (i = 0; i < num_cw; i++) {
>> + int data_size;
>
> All these functions appear to hard code "num_cw" to 4. What is "num_cw"
> and why are we doing this per function?
QPIC controller internally works on code word size not on page and each
code word size is 512-bytes so if page size is 2K then num_cw = 4, if page
size is 4K then num_cw = 8.
Will not hard code this value to 4 or 8 , will fix this in next patch.
>
>> +static int qpic_snand_program_execute(struct qcom_nand_controller *snandc,
>> + const struct spi_mem_op *op)
>
>> + if (op->cmd.opcode == SPINAND_READID) {
>> + snandc->buf_count = 4;
>> + read_reg_dma(snandc, NAND_READ_ID, 1, NAND_BAM_NEXT_SGL);
>> +
>> + ret = submit_descs(snandc);
>> + if (ret)
>> + dev_err(snandc->dev, "failure in submitting descriptor for readid\n");
>> +
>> + nandc_read_buffer_sync(snandc, true);
>> + memcpy(op->data.buf.in, snandc->reg_read_buf, snandc->buf_count);
>
> These memcpy()s don't seem great, why aren't we just reading directly
> into the output buffer?
This reg_read_buf is being used in common API so that it will be used by both
serial nand as well raw nand, so I can't directly use the output buffer since
internally CW mechanism I have to maintain in common API.
>
>> + if (op->cmd.opcode == SPINAND_GET_FEATURE) {
>
> This function looks like it should be a switch statement.
Ok
>
>> +static bool qpic_snand_is_page_op(const struct spi_mem_op *op)
>> +{
>
>> + if (op->data.dir == SPI_MEM_DATA_IN) {
>> + if (op->addr.buswidth == 4 && op->data.buswidth == 4)
>> + return true;
>> +
>> + if (op->addr.nbytes == 2 && op->addr.buswidth == 1)
>> + return true;
>> +
>> + } else if (op->data.dir == SPI_MEM_DATA_OUT) {
>> + if (op->data.buswidth == 4)
>> + return true;
>> + if (op->addr.nbytes == 2 && op->addr.buswidth == 1)
>> + return true;
>> + }
>
> Again looks like a switch statement.
Ok
>
>> + ctlr = devm_spi_alloc_master(dev, sizeof(*snandc));
>> + if (!ctlr)
>> + return -ENOMEM;
>
> Please use _alloc_controller.
Ok
>
>> +static int qcom_snand_remove(struct platform_device *pdev)
>> +{
>> + struct spi_controller *ctlr = platform_get_drvdata(pdev);
>> +
>> + spi_unregister_controller(ctlr);
>> +
>> + return 0;
>> +}
>
> We don't disable any of the clocks in the remove path.
OK will fix in next patch.
Thanks for reviewing, will address all your comments in the next patch.
Regards,
Alam.
Powered by blists - more mailing lists