[PATCH] i2c: i801: Remove i801_set_block_buffer_mode

[PATCH] i2c: i801: Remove i801_set_block_buffer_mode

[Heiner Kallweit]

If FEATURE_BLOCK_BUFFER is set I don't see how setting this bit could
fail. Reading it back seems to be overly paranoid. Origin of this
check seems to be 14 yrs ago when people were not completely sure
which chip versions support block buffer mode.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 4c96f1b47..608e928e9 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -521,9 +521,11 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 		return -EOPNOTSUPP;
 	}
 
+	/* Set block buffer mode */
+	outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
+
 	inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */
 
-	/* Use 32-byte buffer to process this transaction */
 	if (read_write == I2C_SMBUS_WRITE) {
 		len = data->block[0];
 		outb_p(len, SMBHSTDAT0(priv));
@@ -750,14 +752,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	return i801_check_post(priv, status);
 }
 
-static int i801_set_block_buffer_mode(struct i801_priv *priv)
-{
-	outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
-	if ((inb_p(SMBAUXCTL(priv)) & SMBAUXCTL_E32B) == 0)
-		return -EIO;
-	return 0;
-}
-
 /* Block transaction function */
 static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
 				  char read_write, int command)
@@ -786,9 +780,8 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
 	/* Experience has shown that the block buffer can only be used for
 	   SMBus (not I2C) block transactions, even though the datasheet
 	   doesn't mention this limitation. */
-	if ((priv->features & FEATURE_BLOCK_BUFFER)
-	 && command != I2C_SMBUS_I2C_BLOCK_DATA
-	 && i801_set_block_buffer_mode(priv) == 0)
+	if (priv->features & FEATURE_BLOCK_BUFFER &&
+	    command != I2C_SMBUS_I2C_BLOCK_DATA)
 		result = i801_block_transaction_by_block(priv, data,
 							 read_write,
 							 command);
-- 
2.33.1

Re: [PATCH] i2c: i801: Remove i801_set_block_buffer_mode

[Jean Delvare]

Hi Heiner,

On Thu, 11 Nov 2021 22:43:35 +0100, Heiner Kallweit wrote:
> If FEATURE_BLOCK_BUFFER is set I don't see how setting this bit could
> fail. Reading it back seems to be overly paranoid. Origin of this
> check seems to be 14 yrs ago when people were not completely sure
> which chip versions support block buffer mode.

Your reading of the history is correct, although "overly paranoid"
might be a somewhat exaggerated statement. When you modify a driver
used by millions and have been bitten by undocumented restrictions in
the same area, being cautious not to cause a regression doesn't seem
that bad to me.

What was wrong in that approach, I would think retrospectively, is that
i801_set_block_buffer_mode() should have been made verbose on failure,
so that we learned over time if any chipset actually failed to support
the feature in question. Because 14 years later we in fact still don't
know if the test was needed or not.

I'm fine with your change nevertheless, it should be fine, and if
anything breaks then we'll fix it.

I'll test it on my system later today.

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 4c96f1b47..608e928e9 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -521,9 +521,11 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	/* Set block buffer mode */
> +	outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
> +
>  	inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */
>  
> -	/* Use 32-byte buffer to process this transaction */
>  	if (read_write == I2C_SMBUS_WRITE) {
>  		len = data->block[0];
>  		outb_p(len, SMBHSTDAT0(priv));
> @@ -750,14 +752,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  	return i801_check_post(priv, status);
>  }
>  
> -static int i801_set_block_buffer_mode(struct i801_priv *priv)
> -{
> -	outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
> -	if ((inb_p(SMBAUXCTL(priv)) & SMBAUXCTL_E32B) == 0)
> -		return -EIO;
> -	return 0;
> -}
> -
>  /* Block transaction function */
>  static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
>  				  char read_write, int command)
> @@ -786,9 +780,8 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>  	/* Experience has shown that the block buffer can only be used for
>  	   SMBus (not I2C) block transactions, even though the datasheet
>  	   doesn't mention this limitation. */
> -	if ((priv->features & FEATURE_BLOCK_BUFFER)
> -	 && command != I2C_SMBUS_I2C_BLOCK_DATA
> -	 && i801_set_block_buffer_mode(priv) == 0)
> +	if (priv->features & FEATURE_BLOCK_BUFFER &&

No, please preserve the parentheses. Mixing "&" and "&&" without
parentheses is highly confusing (to me at least, but I suspect I'm not
alone).

> +	    command != I2C_SMBUS_I2C_BLOCK_DATA)
>  		result = i801_block_transaction_by_block(priv, data,
>  							 read_write,
>  							 command);


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Remove i801_set_block_buffer_mode

[Heiner Kallweit]

On 18.11.2021 11:23, Jean Delvare wrote:
> Hi Heiner,
> 
> On Thu, 11 Nov 2021 22:43:35 +0100, Heiner Kallweit wrote:
>> If FEATURE_BLOCK_BUFFER is set I don't see how setting this bit could
>> fail. Reading it back seems to be overly paranoid. Origin of this
>> check seems to be 14 yrs ago when people were not completely sure
>> which chip versions support block buffer mode.
> 
> Your reading of the history is correct, although "overly paranoid"
> might be a somewhat exaggerated statement. When you modify a driver
> used by millions and have been bitten by undocumented restrictions in
> the same area, being cautious not to cause a regression doesn't seem
> that bad to me.

Indeed my statement could be read as: The guys back then didn't know
what they were doing. It definitely wasn't meant this way.

> 
> What was wrong in that approach, I would think retrospectively, is that
> i801_set_block_buffer_mode() should have been made verbose on failure,
> so that we learned over time if any chipset actually failed to support
> the feature in question. Because 14 years later we in fact still don't
> know if the test was needed or not.
> 
ICH4 spec mentions the block buffer mode and it's hard to imagine
(even though not impossible) that single later versions dropped
this feature.

> I'm fine with your change nevertheless, it should be fine, and if
> anything breaks then we'll fix it.
> 
> I'll test it on my system later today.
> 
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 17 +++++------------
>>  1 file changed, 5 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 4c96f1b47..608e928e9 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -521,9 +521,11 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>>  		return -EOPNOTSUPP;
>>  	}
>>  
>> +	/* Set block buffer mode */
>> +	outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
>> +
>>  	inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */
>>  
>> -	/* Use 32-byte buffer to process this transaction */
>>  	if (read_write == I2C_SMBUS_WRITE) {
>>  		len = data->block[0];
>>  		outb_p(len, SMBHSTDAT0(priv));
>> @@ -750,14 +752,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>  	return i801_check_post(priv, status);
>>  }
>>  
>> -static int i801_set_block_buffer_mode(struct i801_priv *priv)
>> -{
>> -	outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
>> -	if ((inb_p(SMBAUXCTL(priv)) & SMBAUXCTL_E32B) == 0)
>> -		return -EIO;
>> -	return 0;
>> -}
>> -
>>  /* Block transaction function */
>>  static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
>>  				  char read_write, int command)
>> @@ -786,9 +780,8 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>>  	/* Experience has shown that the block buffer can only be used for
>>  	   SMBus (not I2C) block transactions, even though the datasheet
>>  	   doesn't mention this limitation. */
>> -	if ((priv->features & FEATURE_BLOCK_BUFFER)
>> -	 && command != I2C_SMBUS_I2C_BLOCK_DATA
>> -	 && i801_set_block_buffer_mode(priv) == 0)
>> +	if (priv->features & FEATURE_BLOCK_BUFFER &&
> 
> No, please preserve the parentheses. Mixing "&" and "&&" without
> parentheses is highly confusing (to me at least, but I suspect I'm not
> alone).
> 
Shall I send a v2 with an adjusted commit message and these
parentheses re-added?

>> +	    command != I2C_SMBUS_I2C_BLOCK_DATA)
>>  		result = i801_block_transaction_by_block(priv, data,
>>  							 read_write,
>>  							 command);
> 
> 

Re: [PATCH] i2c: i801: Remove i801_set_block_buffer_mode

[Jean Delvare]

On Thu, 18 Nov 2021 21:58:02 +0100, Heiner Kallweit wrote:
> On 18.11.2021 11:23, Jean Delvare wrote:
> > On Thu, 11 Nov 2021 22:43:35 +0100, Heiner Kallweit wrote:  
> >> If FEATURE_BLOCK_BUFFER is set I don't see how setting this bit could
> >> fail. Reading it back seems to be overly paranoid. Origin of this
> >> check seems to be 14 yrs ago when people were not completely sure
> >> which chip versions support block buffer mode.  
> > 
> > Your reading of the history is correct, although "overly paranoid"
> > might be a somewhat exaggerated statement. When you modify a driver
> > used by millions and have been bitten by undocumented restrictions in
> > the same area, being cautious not to cause a regression doesn't seem
> > that bad to me.  
> 
> Indeed my statement could be read as: The guys back then didn't know
> what they were doing. It definitely wasn't meant this way.

That would be me ;-) well not alone but I remember participating.

And no worry, I did not take it personally, I just wanted to give some
more background.

> > What was wrong in that approach, I would think retrospectively, is that
> > i801_set_block_buffer_mode() should have been made verbose on failure,
> > so that we learned over time if any chipset actually failed to support
> > the feature in question. Because 14 years later we in fact still don't
> > know if the test was needed or not.
>
> ICH4 spec mentions the block buffer mode and it's hard to imagine
> (even though not impossible) that single later versions dropped
> this feature.

The history of computer hardware is paved with examples of exactly this
or even more unexpected changes. I no longer take anything for granted.

Plus the ICH4 itself was probably the problem, I seem to recall none of
us had any sample to test the new code on it, and freshly added
features are sometimes subject to hardware errata.

But looking at it now, the most surprising really is that we thought
the extra, probably unneeded check would help. It might as well be that
the bit *would* be set properly but not do anything, or not what was
documented. So all in all I suppose we should have just let it fail if it
had too, and come up with a workaround later if actually needed. It was
deliciously naive from us to believe we could anticipate *how* the
hardware would fail if it did. Oh well :-)

> > I'm fine with your change nevertheless, it should be fine, and if
> > anything breaks then we'll fix it.
> > 
> > I'll test it on my system later today.

For the record, I did, and it worked :-)

> >> (...)
> >> @@ -786,9 +780,8 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
> >>  	/* Experience has shown that the block buffer can only be used for
> >>  	   SMBus (not I2C) block transactions, even though the datasheet
> >>  	   doesn't mention this limitation. */
> >> -	if ((priv->features & FEATURE_BLOCK_BUFFER)
> >> -	 && command != I2C_SMBUS_I2C_BLOCK_DATA
> >> -	 && i801_set_block_buffer_mode(priv) == 0)
> >> +	if (priv->features & FEATURE_BLOCK_BUFFER &&  
> > 
> > No, please preserve the parentheses. Mixing "&" and "&&" without
> > parentheses is highly confusing (to me at least, but I suspect I'm not
> > alone).
> >   
> Shall I send a v2 with an adjusted commit message and these
> parentheses re-added?
> 
> >> +	    command != I2C_SMBUS_I2C_BLOCK_DATA)
> >>  		result = i801_block_transaction_by_block(priv, data,
> >>  							 read_write,
> >>  							 command);  

Yes please!

-- 
Jean Delvare
SUSE L3 Support