linux-kernel - Re: [PATCH v5 08/10] i2c: npcm: Remove own slave addresses 2:10

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] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 21 May 2022 07:58:52 +0200
From:   Wolfram Sang <wsa@...nel.org>
To:     Tyrone Ting <warp5tw@...il.com>
Cc:     avifishman70@...il.com, tmaimon77@...il.com, tali.perry1@...il.com,
        venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        andriy.shevchenko@...ux.intel.com, jarkko.nikula@...ux.intel.com,
        semen.protsenko@...aro.org, rafal@...ecki.pl, sven@...npeter.dev,
        jsd@...ihalf.com, jie.deng@...el.com, lukas.bulwahn@...il.com,
        arnd@...db.de, olof@...om.net, tali.perry@...oton.com,
        Avi.Fishman@...oton.com, tomer.maimon@...oton.com,
        KWLIU@...oton.com, JJLIU0@...oton.com, kfting@...oton.com,
        openbmc@...ts.ozlabs.org, linux-i2c@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 08/10] i2c: npcm: Remove own slave addresses 2:10


> NPCM can support up to 10 own slave addresses. In practice, only one
> address is actually being used. In order to access addresses 2 and above,
> need to switch register banks. The switch needs spinlock.
> To avoid using spinlock for this useless feature removed support of SA >=
> 2. Also fix returned slave event enum.

Is the spinlock contention so high? The code paths do not really look
like hot paths to me. A bit sad to see this feature go.

>  static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
>  	NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4,
>  	NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8,

Why do we keep this array if we drop the support?

> @@ -604,8 +602,7 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
>  			i2cctl1 &= ~NPCM_I2CCTL1_GCMEN;
>  		iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
>  		return 0;
> -	}
> -	if (addr_type == I2C_ARP_ADDR) {
> +	} else if (addr_type == I2C_ARP_ADDR) {

I might be wrong but this looks like a seperate change?

> @@ -924,11 +918,15 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
>  	for (i = 0; i < I2C_HW_FIFO_SIZE; i++) {
>  		if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
>  			break;
> -		i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
> +		if (bus->state == I2C_SLAVE_MATCH) {
> +			i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
> +			bus->state = I2C_OPER_STARTED;
> +		} else {
> +			i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
> +		}
>  		ind = (bus->slv_wr_ind + bus->slv_wr_size) % I2C_HW_FIFO_SIZE;
>  		bus->slv_wr_buf[ind] = value;
>  		bus->slv_wr_size++;
> -		i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
>  	}
>  	return I2C_HW_FIFO_SIZE - ret;
>  }
> @@ -976,7 +974,6 @@ static void npcm_i2c_slave_xmit(struct npcm_i2c *bus, u16 nwrite,
>  	if (nwrite == 0)
>  		return;
>  
> -	bus->state = I2C_OPER_STARTED;
>  	bus->operation = I2C_WRITE_OPER;

This is definately a seperate change!

All the best!

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)