linux-kernel - Re: [v2,02/10] i2c-i801: Move hostcfg set/reset to i801

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]

Message-ID: <20160609093945.GD24234@mail.corp.redhat.com>
Date:	Thu, 9 Jun 2016 11:39:45 +0200
From:	Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:	Corey Minyard <minyard@....org>
Cc:	Jean Delvare <jdelvare@...e.com>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org, Corey Minyard <cminyard@...sta.com>
Subject: Re: [v2,02/10] i2c-i801: Move hostcfg set/reset to i801_access()

On May 29 2016 or thereabouts, Corey Minyard wrote:
> From: Corey Minyard <cminyard@...sta.com>
> 
> The HSTCFG register save/restore was done in i2c_block_transaction,
> but all the checks were already done in i801_access, so move it into
> those checks.
> 
> This results in a small savings of code, and moves some special
> handing for I2C transactions into code that is already handling
> special things for I2C transactions.
> 
> Signed-off-by: Corey Minyard <cminyard@...sta.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 818c0c8..205f9d0 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -661,20 +661,6 @@ static int i801_block_transaction(struct i801_priv *priv,
>  				  int command, int hwpec)
>  {
>  	int result = 0;
> -	unsigned char hostc;
> -
> -	if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
> -		if (read_write == I2C_SMBUS_WRITE) {
> -			/* set I2C_EN bit in configuration register */
> -			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
> -			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
> -					      hostc | SMBHSTCFG_I2C_EN);
> -		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
> -			dev_err(&priv->pci_dev->dev,
> -				"I2C block read is unsupported!\n");
> -			return -EOPNOTSUPP;
> -		}
> -	}
>  
>  	if (read_write == I2C_SMBUS_WRITE
>  	 || command == I2C_SMBUS_I2C_BLOCK_DATA) {
> @@ -699,11 +685,6 @@ static int i801_block_transaction(struct i801_priv *priv,
>  							     read_write,
>  							     command);
>  
> -	if (command == I2C_SMBUS_I2C_BLOCK_DATA
> -	 && read_write == I2C_SMBUS_WRITE) {
> -		/* restore saved configuration register value */
> -		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
> -	}
>  	return result;
>  }
>  
> @@ -715,6 +696,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	int hwpec;
>  	int block = 0;
>  	int ret = 0, xact = 0;
> +	int hostc = -1;
>  	struct i801_priv *priv = i2c_get_adapdata(adap);
>  
>  	pm_runtime_get_sync(&priv->pci_dev->dev);
> @@ -764,12 +746,24 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		/* NB: page 240 of ICH5 datasheet shows that the R/#W
>  		 * bit should be cleared here, even when reading */
>  		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
> -		if (read_write == I2C_SMBUS_READ) {
> +		if (read_write == I2C_SMBUS_WRITE) {
> +			unsigned char thostc;
> +
> +			outb_p(command, SMBHSTCMD(priv));
> +			/* set I2C_EN bit in configuration register */
> +			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &thostc);
> +			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
> +					      thostc | SMBHSTCFG_I2C_EN);
> +			hostc = thostc;
> +		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {

nitpick: I'd prefer seeing:
if (write) {
  foo
} else {
  if (!FEATURE_I2C_BLOCK_READ) {
    goto out
  }
  bar
}

It's equivalent but I feel like it's easier to understand.

Other than that, no strong opinion on the patch itself, but it's correct
(the code path stays the same):
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>

Cheers,
Benjamin

> +			dev_err(&priv->pci_dev->dev,
> +				"I2C block read is unsupported!\n");
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		} else
>  			/* NB: page 240 of ICH5 datasheet also shows
>  			 * that DATA1 is the cmd field when reading */
>  			outb_p(command, SMBHSTDAT1(priv));
> -		} else
> -			outb_p(command, SMBHSTCMD(priv));
>  		block = 1;
>  		break;
>  	default:
> @@ -798,6 +792,9 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		outb_p(inb_p(SMBAUXCTL(priv)) &
>  		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
>  
> +	if (hostc >= 0)
> +		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
> +
>  	if (block)
>  		goto out;
>  	if (ret)