Re: [PATCH] i2c: i801: Remove i801_set_block_buffer_mode

[Jean Delvare <jdelvare@suse.de>]

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