Re: [PATCH V4 08/16] soc: tegra: pmc: Fix checking of valid partitions

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3948 bytes --]

On Fri, Dec 04, 2015 at 02:57:09PM +0000, Jon Hunter wrote:
> The tegra power partitions are referenced by a numerical ID which are
> the same values programmed into the PMC registers for controlling the
> partition. For a given device, the valid partition IDs may not be
> contiguous and so simply checking that an ID is not greater than the
> maximum ID supported may not mean it is valid. Fix this by computing
> a bit-mask of the valid partition IDs for a device and add a macro that
> will test if the partition is valid based upon this mask.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 28d3106d3add..0967bba13947 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -103,6 +103,7 @@
>  #define GPU_RG_CNTRL			0x2d4
>  
>  #define PMC_PWRGATE_STATE(status, id)	((status & BIT(id)) != 0)
> +#define PMC_PWRGATE_IS_VALID(id)	(pmc->powergates_mask & BIT(id))

I'd prefer a static inline function here, too. If you do that you will
also need to move the static inline below the struct tegra_pmc to avoid
build errors. Also don't forget to check for id < 0, because it
technically could be. It may also be worth checking for an upper bound
just in case anybody was going to pass in a value other that the ones
defined in include/soc/tegra/pmc.h.

>  struct tegra_pmc_soc {
>  	unsigned int num_powergates;
> @@ -134,6 +135,7 @@ struct tegra_pmc_soc {
>   * @cpu_pwr_good_en: CPU power good signal is enabled
>   * @lp0_vec_phys: physical base address of the LP0 warm boot code
>   * @lp0_vec_size: size of the LP0 warm boot code
> + * @powergates_mask: Bit mask of valid power gates
>   * @powergates_lock: mutex for power gate register access
>   */
>  struct tegra_pmc {
> @@ -158,6 +160,7 @@ struct tegra_pmc {
>  	bool cpu_pwr_good_en;
>  	u32 lp0_vec_phys;
>  	u32 lp0_vec_size;
> +	u32 powergates_mask;

This seems rather risky. The highest partition ID that we currently have
is 29, so there is potential that 32 bits will be exceeded in the near
future. It'd be more future-proof to turn this into a bitmap from the
start so that we never have to worry about it.

>  
>  	struct mutex powergates_lock;
>  };
> @@ -213,7 +216,7 @@ static int tegra_powergate_set(int id, bool new_state)
>   */
>  int tegra_powergate_power_on(int id)
>  {
> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
> +	if (!PMC_PWRGATE_IS_VALID(id))
>  		return -EINVAL;

In a similar vein I've been meaning, for a long time, to make the ID an
unsigned integer. It might be worth doing that, even with this patch
applied, but it can be a separate patch.

>  	return tegra_powergate_set(id, true);
> @@ -225,7 +228,7 @@ int tegra_powergate_power_on(int id)
>   */
>  int tegra_powergate_power_off(int id)
>  {
> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
> +	if (!PMC_PWRGATE_IS_VALID(id))
>  		return -EINVAL;
>  
>  	return tegra_powergate_set(id, false);
> @@ -240,7 +243,7 @@ int tegra_powergate_is_powered(int id)
>  {
>  	u32 status;
>  
> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
> +	if (!PMC_PWRGATE_IS_VALID(id))
>  		return -EINVAL;
>  
>  	status = tegra_pmc_readl(PWRGATE_STATUS);
> @@ -256,7 +259,7 @@ int tegra_powergate_remove_clamping(int id)
>  {
>  	u32 mask;
>  
> -	if (!pmc->soc || id < 0 || id >= pmc->soc->num_powergates)
> +	if (!PMC_PWRGATE_IS_VALID(id))
>  		return -EINVAL;
>  
>  	/*
> @@ -1084,7 +1087,7 @@ static int __init tegra_pmc_early_init(void)
>  	struct device_node *np;
>  	struct resource regs;
>  	bool invert;
> -	u32 value;
> +	u32 value, i;

I'd prefer an unsized type (unsigned int) for i because there is no
reason why it should be sized.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]