| 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: <5hjezq5ag4etru6suzbntvg2fwn45acckiyxsujmsjxsrgqxrd@asub7zr2t3gd>
Date: Tue, 10 Sep 2024 15:15:57 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Manikanta Guntupalli <manikanta.guntupalli@....com>
Cc: git@....com, linux-arm-kernel@...ts.infradead.org,
linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org, michal.simek@....com,
radhey.shyam.pandey@....com, srinivas.goud@....com, shubhrajyoti.datta@....com,
manion05gk@...il.com
Subject: Re: [PATCH 3/3] i2c: cadence: Add atomic transfer support for
controller version 1.4
Hi Manikata,
Sorry for the delay in reviewing this patch. Looks good, just a
few notes below.
...
> +static bool cdns_i2c_error_check(struct cdns_i2c *id)
> +{
> + unsigned int isr_status;
> +
> + id->err_status = 0;
> +
> + isr_status = cdns_i2c_readreg(CDNS_I2C_ISR_OFFSET);
> + cdns_i2c_writereg(isr_status & CDNS_I2C_IXR_ERR_INTR_MASK, CDNS_I2C_ISR_OFFSET);
> +
> + id->err_status = isr_status & CDNS_I2C_IXR_ERR_INTR_MASK;
> + if (id->err_status)
> + return true;
> +
> + return false;
return !!id->err_status;
> +}
> +
> +static void cdns_i2c_mrecv_atomic(struct cdns_i2c *id)
> +{
> + bool updatetx;
Please move the udatex declaration inside the while loop.
> + while (id->recv_count > 0) {
> + /*
> + * Check if transfer size register needs to be updated again for a
> + * large data receive operation.
> + */
> + updatetx = id->recv_count > id->curr_recv_count;
> +
> + while (id->curr_recv_count > 0) {
> + if (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) & CDNS_I2C_SR_RXDV) {
> + *id->p_recv_buf++ = cdns_i2c_readreg(CDNS_I2C_DATA_OFFSET);
Can you please expand this operation to be a bit more clearer,
without asking people to check on operation precedence?
> + id->recv_count--;
> + id->curr_recv_count--;
> +
> + /*
> + * Clear hold bit that was set for FIFO control
> + * if RX data left is less than or equal to
> + * FIFO DEPTH unless repeated start is selected
mmhhh... the lack of punctuation makes this comment difficult to
understand.
> + */
> + if (id->recv_count <= id->fifo_depth && !id->bus_hold_flag)
> + cdns_i2c_clear_bus_hold(id);
> + }
> + if (cdns_i2c_error_check(id))
> + return;
> + if (cdns_is_holdquirk(id, updatetx))
> + break;
> + }
> +
> + /*
> + * The controller sends NACK to the slave when transfer size
/slave/target/
> + * register reaches zero without considering the HOLD bit.
> + * This workaround is implemented for large data transfers to
> + * maintain transfer size non-zero while performing a large
> + * receive operation.
> + */
> + if (cdns_is_holdquirk(id, updatetx)) {
> + /* wait while fifo is full */
> + while (cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET) !=
> + (id->curr_recv_count - id->fifo_depth))
> + ;
> +
> + /*
> + * Check number of bytes to be received against maximum
> + * transfer size and update register accordingly.
> + */
> + if (((int)(id->recv_count) - id->fifo_depth) >
The cast is not needed here.
> + id->transfer_size) {
> + cdns_i2c_writereg(id->transfer_size,
> + CDNS_I2C_XFER_SIZE_OFFSET);
> + id->curr_recv_count = id->transfer_size +
> + id->fifo_depth;
> + } else {
> + cdns_i2c_writereg(id->recv_count -
> + id->fifo_depth,
> + CDNS_I2C_XFER_SIZE_OFFSET);
> + id->curr_recv_count = id->recv_count;
> + }
> + }
> + }
> +
> + /* Clear hold (if not repeated start) */
> + if (!id->recv_count && !id->bus_hold_flag)
> + cdns_i2c_clear_bus_hold(id);
> +}
> +
> /**
> * cdns_i2c_mrecv - Prepare and start a master receive operation
> * @id: pointer to the i2c device structure
> @@ -715,7 +804,34 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
> cdns_i2c_writereg(addr, CDNS_I2C_ADDR_OFFSET);
> }
>
> - cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
> + if (!id->atomic)
> + cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
> + else
> + cdns_i2c_mrecv_atomic(id);
> +}
> +
> +static void cdns_i2c_msend_rem_atomic(struct cdns_i2c *id)
> +{
> + unsigned int avail_bytes;
> + unsigned int bytes_to_send;
Please move these inside the while.
> +
> + while (id->send_count) {
> + avail_bytes = id->fifo_depth - cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
> + if (id->send_count > avail_bytes)
> + bytes_to_send = avail_bytes;
> + else
> + bytes_to_send = id->send_count;
> +
> + while (bytes_to_send--) {
> + cdns_i2c_writereg((*id->p_send_buf++), CDNS_I2C_DATA_OFFSET);
> + id->send_count--;
> + }
> + if (cdns_i2c_error_check(id))
> + return;
> + }
> +
> + if (!id->send_count && !id->bus_hold_flag)
> + cdns_i2c_clear_bus_hold(id);
> }
>
> /**
> @@ -778,7 +894,12 @@ static void cdns_i2c_msend(struct cdns_i2c *id)
> cdns_i2c_writereg(id->p_msg->addr & CDNS_I2C_ADDR_MASK,
> CDNS_I2C_ADDR_OFFSET);
>
> - cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
> + if (!id->atomic) {
> + cdns_i2c_writereg(CDNS_I2C_ENABLED_INTR_MASK, CDNS_I2C_IER_OFFSET);
> + } else {
> + if (id->send_count > 0)
If you do:
} else if (id->send_count > 0) {
we save a level of indentation.
> + cdns_i2c_msend_rem_atomic(id);
> + }
> }
>
> /**
> @@ -818,7 +939,8 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
>
> id->p_msg = msg;
> id->err_status = 0;
> - reinit_completion(&id->xfer_done);
> + if (!id->atomic)
> + reinit_completion(&id->xfer_done);
>
> /* Check for the TEN Bit mode on each msg */
> reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
> @@ -841,13 +963,24 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
> /* Minimal time to execute this message */
> msg_timeout = msecs_to_jiffies((1000 * msg->len * BITS_PER_BYTE) / id->i2c_clk);
> /* Plus some wiggle room */
> - msg_timeout += msecs_to_jiffies(500);
> + if (!id->atomic)
> + msg_timeout += msecs_to_jiffies(500);
> + else
> + msg_timeout += msecs_to_jiffies(2000);
You explained this in the commit log, can you add it in a
comment, as well?
>
> if (msg_timeout < adap->timeout)
> msg_timeout = adap->timeout;
>
> - /* Wait for the signal of completion */
> - time_left = wait_for_completion_timeout(&id->xfer_done, msg_timeout);
> + if (!id->atomic) {
> + /* Wait for the signal of completion */
> + time_left = wait_for_completion_timeout(&id->xfer_done, msg_timeout);
> + } else {
> + /* 0 is success, -ETIMEDOUT is error */
> + time_left = !readl_poll_timeout_atomic(id->membase + CDNS_I2C_ISR_OFFSET,
> + reg, (reg & CDNS_I2C_IXR_COMP),
> + CDNS_I2C_POLL_US_ATOMIC, msg_timeout);
> + }
You can merge this if/else with the one above, to save some code.
> +
Thanks,
Andi
Powered by blists - more mailing lists