Re: [PATCH 3/8] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER

[Jean Delvare <jdelvare@suse.de>]

Hi Heiner,

On Fri, 15 Apr 2022 18:55:46 +0200, Heiner Kallweit wrote:
> According to the datasheet the block process call requires block
> buffer mode. The user may disable block buffer mode by module
> parameter disable_features, in such a case we have to clear
> FEATURE_BLOCK_PROC.

In which datasheet are you seeing this? Can you point me to the
specific section and/or quote the statement? I can't find it in the
datasheet I'm looking at (ICH9, Intel document 316972-002) but it is
huge and I may just be missing it.

Also, same request as previous patch, I'd like a comment in the code,
so that developers don't have to read the git log to figure out why this
piece of code is there.

Furthermore, as far as I can see, the FEATURE_BLOCK_PROC flag only
affects the value returned by i801_func(). i801_access() does not
verify whether this flag is set before processing a command where size
== I2C_SMBUS_BLOCK_PROC_CALL. I think it should? Otherwise your fix is
only partial (will work if the device driver calls .functionality as it
is supposed to, will fail with - I suppose - unpredictable results if
the device driver calls .smbus_xfer directly).

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index eccdc7035..1d8182901 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1675,6 +1675,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	}
>  	priv->features &= ~disable_features;
>  
> +	if (!(priv->features & FEATURE_BLOCK_BUFFER))
> +		priv->features &= ~FEATURE_BLOCK_PROC;
> +
>  	err = pcim_enable_device(dev);
>  	if (err) {
>  		dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",

Thanks,
-- 
Jean Delvare
SUSE L3 Support