linux-kernel - Re: [PATCH V3 2/2] spmi: pmic_arb: add support for hw version 2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 18 Feb 2015 09:34:46 -0600
From:	Josh Cartwright <joshc@....teric.us>
To:	Gilad Avidov <gavidov@...eaurora.org>
Cc:	sdharia@...eaurora.org, mlocke@...eaurora.org,
	linux-arm-msm@...r.kernel.org, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org, iivanov@...sol.com,
	galak@...eaurora.org, agross@...eaurora.org
Subject: Re: [PATCH V3 2/2] spmi: pmic_arb: add support for hw version 2

Hey Gilad-

On Mon, Feb 09, 2015 at 03:51:12PM -0700, Gilad Avidov wrote:
> Qualcomm PMIC Arbiter version-2 changes from version-1 are:
> 
> - Some different register offsets.
> - New channel register space, one per PMIC peripheral (ppid).
>   All tx traffic uses these channels.
> - New observer register space. All rx trafic uses this space.
> - Different command format for spmi command registers.
> 
> Acked-by: Sagar Dharia <sdharia@...eaurora.org>
> Signed-off-by: Gilad Avidov <gavidov@...eaurora.org>
[..]
> +++ b/drivers/spmi/spmi-pmic-arb.c
[..]
> @@ -645,12 +795,65 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  	pa->spmic = ctrl;
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
> -	pa->base = devm_ioremap_resource(&ctrl->dev, res);
> -	if (IS_ERR(pa->base)) {
> -		err = PTR_ERR(pa->base);
> +	pa->rd_base = devm_ioremap_resource(&ctrl->dev, res);

This seems like an awkward way to do this, especially if you end up
remapping it...

> +	if (IS_ERR(pa->rd_base)) {
> +		err = PTR_ERR(pa->rd_base);
>  		goto err_put_ctrl;
>  	}
>  
> +	hw_ver = readl_relaxed(pa->rd_base + PMIC_ARB_VERSION);
> +	is_v1  = (hw_ver < PMIC_ARB_VERSION_V2_MIN);
> +
> +	dev_info(&ctrl->dev, "PMIC Arb Version-%d (0x%x)\n", (is_v1 ? 1 : 2),
> +		hw_ver);
> +
> +	if (is_v1) {
> +		pa->ver_ops = &pmic_arb_v1;
> +		pa->wr_base = pa->rd_base;
> +	} else {
> +		u8  chan;
> +		u16 ppid;
> +		u32 regval;
> +
> +		pa->ver_ops = &pmic_arb_v2;
> +
> +		pa->ppid_to_chan = devm_kzalloc(&ctrl->dev,
> +					PPID_TO_CHAN_TABLE_SZ, GFP_KERNEL);
> +		if (!pa->ppid_to_chan) {
> +			err = -ENOMEM;
> +			goto err_put_ctrl;
> +		}
> +		/*
> +		 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
> +		 * ppid_to_chan is an in-memory invert of that table.
> +		 */
> +		for (chan = 0; chan < PMIC_ARB_MAX_CHNL; ++chan) {
> +			regval = readl_relaxed(pa->rd_base +
> +					       PMIC_ARB_REG_CHNL(chan));
> +			if (!regval)
> +				continue;
> +
> +			ppid = (regval >> 8) & 0xFFF;
> +			pa->ppid_to_chan[ppid] = chan;
> +		}
> +
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						   "obsrvr");
> +		pa->rd_base = devm_ioremap_resource(&ctrl->dev, res);

...here.  Especially because now you have some loose mapping hanging around
for the lifetime of the device.  I'd suggest splitting the v1 and v2
probe routines out into their own functions.

  Josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/