Re: [Intel-gfx] [PATCH 4/5] drm/i915/gmbus: alloc intel_gmbus dynamically

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 03 Mar 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Mar 03, 2022 at 08:19:30PM +0200, Jani Nikula wrote:
>> Allocate the individual intel_gmbus structs dynamically. This lets us
>> hide struct intel_gmbus inside intel_gmbus.c completely. Also use the
>> cleanup function on the error path to avoid duplication.
>> 
>> Leave #include <linux/i2c.h> in i915_drv.h for now, as it pulls in a
>> bunch of implicit dependencies.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_gmbus.c | 42 +++++++++++++++-------
>>  drivers/gpu/drm/i915/i915_drv.h            | 14 ++------
>>  2 files changed, 31 insertions(+), 25 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> index fd908e524875..2bb3494b93e2 100644
>> --- a/drivers/gpu/drm/i915/display/intel_gmbus.c
>> +++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
>> @@ -38,6 +38,16 @@
>>  #include "intel_display_types.h"
>>  #include "intel_gmbus.h"
>>  
>> +struct intel_gmbus {
>> +	struct i2c_adapter adapter;
>> +#define GMBUS_FORCE_BIT_RETRY (1U << 31)
>> +	u32 force_bit;
>> +	u32 reg0;
>> +	i915_reg_t gpio_reg;
>> +	struct i2c_algo_bit_data bit_algo;
>> +	struct drm_i915_private *dev_priv;
>> +};
>> +
>>  struct gmbus_pin {
>>  	const char *name;
>>  	enum i915_gpio gpio;
>> @@ -881,7 +891,11 @@ int intel_gmbus_setup(struct drm_i915_private *dev_priv)
>>  		if (!gmbus_pin)
>>  			continue;
>>  
>> -		bus = &dev_priv->gmbus[pin];
>> +		bus = kzalloc(sizeof(*bus), GFP_KERNEL);
>> +		if (!bus) {
>> +			ret = -ENOMEM;
>> +			goto err;
>> +		}
>>  
>>  		bus->adapter.owner = THIS_MODULE;
>>  		bus->adapter.class = I2C_CLASS_DDC;
>> @@ -911,8 +925,12 @@ int intel_gmbus_setup(struct drm_i915_private *dev_priv)
>>  		intel_gpio_setup(bus, GPIO(gmbus_pin->gpio));
>>  
>>  		ret = i2c_add_adapter(&bus->adapter);
>> -		if (ret)
>> +		if (ret) {
>> +			kfree(bus);
>>  			goto err;
>> +		}
>> +
>> +		dev_priv->gmbus[pin] = bus;
>>  	}
>>  
>>  	intel_gmbus_reset(dev_priv);
>> @@ -920,24 +938,19 @@ int intel_gmbus_setup(struct drm_i915_private *dev_priv)
>>  	return 0;
>>  
>>  err:
>> -	while (pin--) {
>> -		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
>> -			continue;
>> +	intel_gmbus_teardown(dev_priv);
>>  
>> -		bus = &dev_priv->gmbus[pin];
>> -		i2c_del_adapter(&bus->adapter);
>> -	}
>>  	return ret;
>>  }
>>  
>>  struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv,
>>  					    unsigned int pin)
>>  {
>> -	if (drm_WARN_ON(&dev_priv->drm,
>> -			!intel_gmbus_is_valid_pin(dev_priv, pin)))
>> +	if (drm_WARN_ON(&dev_priv->drm, pin >= ARRAY_SIZE(dev_priv->gmbus) ||
>> +			!dev_priv->gmbus[pin]))
>>  		return NULL;
>>  
>> -	return &dev_priv->gmbus[pin].adapter;
>> +	return &dev_priv->gmbus[pin]->adapter;
>>  }
>>  
>>  void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
>> @@ -969,10 +982,13 @@ void intel_gmbus_teardown(struct drm_i915_private *dev_priv)
>>  	unsigned int pin;
>>  
>>  	for (pin = 0; pin < ARRAY_SIZE(dev_priv->gmbus); pin++) {
>> -		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
>> +		bus = dev_priv->gmbus[pin];
>
> nit: Would like the 'bus' variable to be declared inside the loop.
> Same for intel_gmbus_setup().

Thanks for the review, pushed to din. I'll follow up with a few
cleanups.

BR,
Jani.


>
>> +		if (!bus)
>>  			continue;
>>  
>> -		bus = &dev_priv->gmbus[pin];
>>  		i2c_del_adapter(&bus->adapter);
>> +
>> +		kfree(bus);
>> +		dev_priv->gmbus[pin] = NULL;
>
> I see we don't actually check intel_gmbus_setup() return value at all so
> intel_gmbus_teardown() can get called twice. So this NULLing is essential
> should intel_gmbus_setup() ever fail.
>
> Series is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>>  	}
>>  }
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 457bc1993d19..869a2bda347b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -35,7 +35,6 @@
>>  #include <asm/hypervisor.h>
>>  
>>  #include <linux/i2c.h>
>> -#include <linux/i2c-algo-bit.h>
>>  #include <linux/intel-iommu.h>
>>  #include <linux/pm_qos.h>
>>  
>> @@ -99,6 +98,7 @@ struct intel_dpll_funcs;
>>  struct intel_encoder;
>>  struct intel_fbdev;
>>  struct intel_fdi_funcs;
>> +struct intel_gmbus;
>>  struct intel_hotplug_funcs;
>>  struct intel_initial_plane_config;
>>  struct intel_limit;
>> @@ -231,16 +231,6 @@ struct i915_drrs {
>>  #define QUIRK_INCREASE_DDI_DISABLED_TIME (1<<7)
>>  #define QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK (1<<8)
>>  
>> -struct intel_gmbus {
>> -	struct i2c_adapter adapter;
>> -#define GMBUS_FORCE_BIT_RETRY (1U << 31)
>> -	u32 force_bit;
>> -	u32 reg0;
>> -	i915_reg_t gpio_reg;
>> -	struct i2c_algo_bit_data bit_algo;
>> -	struct drm_i915_private *dev_priv;
>> -};
>> -
>>  struct i915_suspend_saved_registers {
>>  	u32 saveDSPARB;
>>  	u32 saveSWF0[16];
>> @@ -510,7 +500,7 @@ struct drm_i915_private {
>>  
>>  	struct intel_dmc dmc;
>>  
>> -	struct intel_gmbus gmbus[GMBUS_NUM_PINS];
>> +	struct intel_gmbus *gmbus[GMBUS_NUM_PINS];
>>  
>>  	/** gmbus_mutex protects against concurrent usage of the single hw gmbus
>>  	 * controller on different i2c buses. */
>> -- 
>> 2.30.2

-- 
Jani Nikula, Intel Open Source Graphics Center