Re: [PATCH 1/2] i2c: piix4: Use request_muxed_region

[Jean Delvare <jdelvare@suse.de>]

Hi Guneter,

Sorry for the delay :(

On Sat, 30 Dec 2017 08:50:57 -0800, Guenter Roeck wrote:
> Accesses to SB800_PIIX4_SMB_IDX can occur from multiple drivers.
> Use request_muxed_region() to ensure synchronization.

Which ones? Documenting it, at least in the commit message, would seem
useful. Out of curiosity, have these other drivers been converted to
use request_muxed_region already?

> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 50 ++++++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 462948e2c535..78dd5951d6e7 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -153,10 +153,7 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
>  
>  /*
>   * SB800 globals
> - * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
> - * of I/O ports at SB800_PIIX4_SMB_IDX.
>   */
> -static DEFINE_MUTEX(piix4_mutex_sb800);

With this gone, you can remove #include <linux/mutex.h>.

>  static u8 piix4_port_sel_sb800;
>  static u8 piix4_port_mask_sb800;
>  static u8 piix4_port_shift_sb800;
> @@ -298,12 +295,15 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>  	else
>  		smb_en = (aux) ? 0x28 : 0x2c;
>  
> -	mutex_lock(&piix4_mutex_sb800);
> +	if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2, "sb800_piix4_smb"))
> +		return -EBUSY;

This would happen if and only if another driver has requested the
region already but without IORESOURCE_MUXED, right? Don't you want to
write an error message then? I don't think request_muxed_region() will
do, and probe failing with -EBUSY but no error message logged would be
hard to diagnose.

> +
>  	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
>  	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
>  	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
>  	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
> -	mutex_unlock(&piix4_mutex_sb800);
> +
> +	release_region(SB800_PIIX4_SMB_IDX, 2);
>  
>  	if (!smb_en) {
>  		smb_en_status = smba_en_lo & 0x10;
> @@ -373,7 +373,12 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>  			break;
>  		}
>  	} else {
> -		mutex_lock(&piix4_mutex_sb800);
> +		if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2,
> +					  "sb800_piix4_smb")) {
> +			release_region(piix4_smba, SMBIOSIZE);
> +			return -EBUSY;
> +		}
> +
>  		outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
>  		port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
>  		piix4_port_sel_sb800 = (port_sel & 0x01) ?
> @@ -381,7 +386,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>  				       SB800_PIIX4_PORT_IDX;
>  		piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
>  		piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
> -		mutex_unlock(&piix4_mutex_sb800);
> +		release_region(SB800_PIIX4_SMB_IDX, 2);
>  	}
>  
>  	dev_info(&PIIX4_dev->dev,
> @@ -679,7 +684,8 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>  	u8 port;
>  	int retval;
>  
> -	mutex_lock(&piix4_mutex_sb800);
> +	if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2, "sb800_piix4_smb"))
> +		return -EBUSY;

Did you check the performance cost? I thought that
request_muxed_region() was meant for driver setup, I did not expect it
to be used at driver run-time. Requesting the region again for every
transaction seems quite costly?

That being said, being slow is certainly better than failing, as is
currently the case, so I'm fine with this change anyway. Just curious.

>  
>  	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
>  	smbslvcnt  = inb_p(SMBSLVCNT);
> @@ -695,8 +701,8 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>  	} while (--retries);
>  	/* SMBus is still owned by the IMC, we give up */
>  	if (!retries) {
> -		mutex_unlock(&piix4_mutex_sb800);
> -		return -EBUSY;
> +		retval = -EBUSY;
> +		goto release;
>  	}
>  
>  	/*
> @@ -753,8 +759,8 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>  	if ((size == I2C_SMBUS_BLOCK_DATA) && adapdata->notify_imc)
>  		piix4_imc_wakeup();
>  
> -	mutex_unlock(&piix4_mutex_sb800);
> -
> +release:
> +	release_region(SB800_PIIX4_SMB_IDX, 2);
>  	return retval;
>  }
>  
> @@ -899,13 +905,6 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  		bool notify_imc = false;
>  		is_sb800 = true;
>  
> -		if (!request_region(SB800_PIIX4_SMB_IDX, 2, "smba_idx")) {
> -			dev_err(&dev->dev,
> -			"SMBus base address index region 0x%x already in use!\n",
> -			SB800_PIIX4_SMB_IDX);
> -			return -EBUSY;
> -		}
> -
>  		if (dev->vendor == PCI_VENDOR_ID_AMD &&
>  		    dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
>  			u8 imc;
> @@ -922,20 +921,16 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  
>  		/* base address location etc changed in SB800 */
>  		retval = piix4_setup_sb800(dev, id, 0);
> -		if (retval < 0) {
> -			release_region(SB800_PIIX4_SMB_IDX, 2);
> +		if (retval < 0)
>  			return retval;
> -		}
>  
>  		/*
>  		 * Try to register multiplexed main SMBus adapter,
>  		 * give up if we can't
>  		 */
>  		retval = piix4_add_adapters_sb800(dev, retval, notify_imc);
> -		if (retval < 0) {
> -			release_region(SB800_PIIX4_SMB_IDX, 2);
> +		if (retval < 0)
>  			return retval;
> -		}
>  	} else {
>  		retval = piix4_setup(dev, id);
>  		if (retval < 0)
> @@ -983,11 +978,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>  
>  	if (adapdata->smba) {
>  		i2c_del_adapter(adap);
> -		if (adapdata->port == (0 << piix4_port_shift_sb800)) {
> +		if (adapdata->port == (0 << piix4_port_shift_sb800))
>  			release_region(adapdata->smba, SMBIOSIZE);
> -			if (adapdata->sb800_main)
> -				release_region(SB800_PIIX4_SMB_IDX, 2);
> -		}
>  		kfree(adapdata);
>  		kfree(adap);
>  	}

Everything else looks good to me, thanks.

I assume you have tested this patch on real hardware?

-- 
Jean Delvare
SUSE L3 Support