linux-kernel - RE: [PATCH 02/10] i2c: xiic: Add standard mode support for

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]

Date:   Tue, 8 Jun 2021 09:46:37 +0000
From:   Raviteja Narayanam <rna@...inx.com>
To:     Marek Vasut <marex@...x.de>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        Michal Simek <michals@...inx.com>
CC:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        git <git@...inx.com>
Subject: RE: [PATCH 02/10] i2c: xiic: Add standard mode support for > 255 byte
 read transfers



> -----Original Message-----
> From: Marek Vasut <marex@...x.de>
> Sent: Thursday, June 3, 2021 3:27 PM
> To: Raviteja Narayanam <rna@...inx.com>; linux-i2c@...r.kernel.org; Michal
> Simek <michals@...inx.com>
> Cc: linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; git
> <git@...inx.com>
> Subject: Re: [PATCH 02/10] i2c: xiic: Add standard mode support for > 255
> byte read transfers
> 
> On 6/3/21 7:33 AM, Raviteja Narayanam wrote:
> [...]
> >>> +	if (i2c->dynamic) {
> >>> +		u8 bytes;
> >>> +		u16 val;
> >>> +
> >>> +		/* Clear and enable Rx full interrupt. */
> >>> +		xiic_irq_clr_en(i2c, XIIC_INTR_RX_FULL_MASK |
> >>> +				XIIC_INTR_TX_ERROR_MASK);
> >>> +
> >>> +		/*
> >>> +		 * We want to get all but last byte, because the TX_ERROR
> >> IRQ
> >>> +		 * is used to indicate error ACK on the address, and
> >>> +		 * negative ack on the last received byte, so to not mix
> >>> +		 * them receive all but last.
> >>> +		 * In the case where there is only one byte to receive
> >>> +		 * we can check if ERROR and RX full is set at the same time
> >>> +		 */
> >>> +		rx_watermark = msg->len;
> >>> +		bytes = min_t(u8, rx_watermark, IIC_RX_FIFO_DEPTH);
> >>> +		bytes--;
> >>> +
> >>> +		xiic_setreg8(i2c, XIIC_RFD_REG_OFFSET, bytes);
> >>> +
> >>> +		local_irq_save(flags);
> >>> +		if (!(msg->flags & I2C_M_NOSTART))
> >>> +			/* write the address */
> >>> +			xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET,
> >>> +				      i2c_8bit_addr_from_msg(msg) |
> >>> +				      XIIC_TX_DYN_START_MASK);
> >>> +
> >>> +		xiic_irq_clr_en(i2c, XIIC_INTR_BNB_MASK);
> >>> +
> >>> +		/* If last message, include dynamic stop bit with length */
> >>> +		val = (i2c->nmsgs == 1) ? XIIC_TX_DYN_STOP_MASK : 0;
> >>> +		val |= msg->len;
> >>> +
> >>> +		xiic_setreg16(i2c, XIIC_DTR_REG_OFFSET, val);
> >>> +		local_irq_restore(flags);
> >>
> >> Is local_irq_save()/local_irq_restore() used here to prevent
> >> concurrent access to the XIIC ?
> >
> > These were used to fix the timing constraint between two register writes
> to the IP.
> > As we have discussed last time, these are removed in " [PATCH 08/10] i2c:
> xiic: Remove interrupt enable/disable in Rx path".
> > Now they are no longer needed since our IP is fixed.
> > For legacy IP versions, driver is switching to 'AXI I2C standard mode'
> > of operation in " [PATCH 07/10] i2c: xiic: Switch to Xiic standard mode for
> i2c-read".
> 
> I see, I would expect such fixes to be at the beginning of the series, so they
> can be picked into linux-stable.

Yes, I will send a v2 with only fixes by removing 0004, 0005, 0006, 0009 and 0010 patches.
I can send these five separately afterwards.

> 
> In fact, now that you mention the bugfixes which I posted a year ago, they
> also fixed a multitude of locking issues on SMP in the xiic driver.

This series addressed the issue found in IP about the timing constraint in register
writes. Removed the usage of local_irq_save/restore and gave fix (xiic standard mode)
for the affected IP versions.
All the i2c app requests are serialized through the i2c core into the driver.

> Has this series been tested on SMP Zynq or ZynqMP extensively ?

Yes, we have tested this on Zynqmp with multiple applications running, and
no issues are observed. 

Regards,
Raviteja N