linux-kernel - Re: [PATCH v2 2/5] mtd: rawnand: qcom: Add support for reset, readid, status exec

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20230526192733.003b5293@xps-13>
Date:   Fri, 26 May 2023 19:27:33 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Md Sadre Alam <quic_mdalam@...cinc.com>
Cc:     <mani@...nel.org>, <richard@....at>, <vigneshr@...com>,
        <linux-mtd@...ts.infradead.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_srichara@...cinc.com>
Subject: Re: [PATCH v2 2/5] mtd: rawnand: qcom: Add support for reset,
 readid, status exec_op

Hi Md,

quic_mdalam@...cinc.com wrote on Wed, 24 May 2023 14:54:34 +0530:

> On 5/22/2023 7:15 PM, Miquel Raynal wrote:
> > Hi Md,
> > 
> > quic_mdalam@...cinc.com wrote on Thu, 11 May 2023 19:00:14 +0530:
> >   
> >> This change will add exec_ops support for RESET , READ_ID, STATUS
> >> command.
> >>
> >> Co-developed-by: Sricharan Ramabadhran <quic_srichara@...cinc.com>
> >> Signed-off-by: Sricharan Ramabadhran <quic_srichara@...cinc.com>
> >> Signed-off-by: Md Sadre Alam <quic_mdalam@...cinc.com>
> >> ---
> >> Change in [v2]
> >>
> >> * Missed to post Cover-letter, so posting v2 patch with cover-letter
> >>
> >>   drivers/mtd/nand/raw/qcom_nandc.c | 166 +++++++++++++++++++++++++++++-
> >>   1 file changed, 163 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> >> index dae460e2aa0b..d2f2a8971907 100644
> >> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> >> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> >> @@ -384,6 +384,9 @@ struct nandc_regs {
> >>    * @reg_read_pos:		marker for data read in reg_read_buf
> >>    *
> >>    * @cmd1/vld:			some fixed controller register values
> >> + *
> >> + * @exec_opwrite:		flag to select correct number of code word
> >> + *				while reading status
> >>    */
> >>   struct qcom_nand_controller {
> >>   	struct device *dev;
> >> @@ -434,6 +437,7 @@ struct qcom_nand_controller {
> >>   	int reg_read_pos;  
> >>   >>   	u32 cmd1, vld;  
> >> +	bool exec_opwrite;
> >>   };  
> >>   >>   /*  
> >> @@ -2920,6 +2924,8 @@ static int qcom_op_cmd_mapping(struct qcom_nand_controller *nandc, u8 cmd,
> >>   		break;
> >>   	case NAND_CMD_PAGEPROG:
> >>   		ret = OP_PROGRAM_PAGE;
> >> +		q_op->flag = NAND_CMD_PAGEPROG;  
> > 
> > Just use the instruction value?  
> 
>    Sure , will fix this in next patch V3.
> >   
> >> +		nandc->exec_opwrite = true;
> >>   		break;
> >>   	default:
> >>   		break;
> >> @@ -2982,10 +2988,95 @@ static void qcom_parse_instructions(struct nand_chip *chip,
> >>   	}
> >>   }  
> >>   >> +static void qcom_delay_ns(unsigned int ns)  
> >> +{
> >> +	if (!ns)
> >> +		return;
> >> +
> >> +	if (ns < 10000)
> >> +		ndelay(ns);
> >> +	else
> >> +		udelay(DIV_ROUND_UP(ns, 1000));
> >> +}
> >> +
> >> +static int qcom_wait_rdy_poll(struct nand_chip *chip, unsigned int time_ms)
> >> +{
> >> +	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> >> +	unsigned long start = jiffies + msecs_to_jiffies(time_ms);
> >> +	u32 flash;
> >> +
> >> +	nandc_read_buffer_sync(nandc, true);
> >> +
> >> +	do {
> >> +		flash = le32_to_cpu(nandc->reg_read_buf[0]);
> >> +		if (flash & FS_READY_BSY_N)
> >> +			return 0;
> >> +		cpu_relax();
> >> +	} while (time_after(start, jiffies));
> >> +
> >> +	dev_err(nandc->dev, "Timeout waiting for device to be ready:0x%08x\n", flash);
> >> +
> >> +	return -ETIMEDOUT;
> >> +}
> >> +
> >>   static int qcom_read_status_exec(struct nand_chip *chip,
> >>   				 const struct nand_subop *subop)
> >>   {
> >> -	return 0;
> >> +	struct qcom_nand_host *host = to_qcom_nand_host(chip);
> >> +	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> >> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> >> +	struct qcom_op q_op;
> >> +	const struct nand_op_instr *instr = NULL;
> >> +	unsigned int op_id = 0;
> >> +	unsigned int len = 0;
> >> +	int ret = 0, num_cw = 1, i;
> >> +	u32 flash_status;
> >> +
> >> +	host->status = NAND_STATUS_READY | NAND_STATUS_WP;
> >> +
> >> +	qcom_parse_instructions(chip, subop, &q_op);
> >> +
> >> +	if (nandc->exec_opwrite) {  
> > 
> > I definitely don't understand this flag at all.  
> 
>    This flag is to get the status for all code word in case of program page operation.
>    Since this read status is common for reading status for all kind of operation.
>    so in page program operation it needs to get status for all code word i.e 4 in 2K page.
>    but for normal operation number of code word will be 1.

Then you don't need that dark flag, just ask for a number of CW to
check. It will always be 1 unless you're in a page helper and want as
many CW as chunks.

> >   
> >> +		num_cw = ecc->steps;
> >> +		nandc->exec_opwrite = false;
> >> +	}
> >> +

Thanks,
Miquèl