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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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().

> +		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

-- 
Ville Syrjälä
Intel