linux-kernel - Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets

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]

Message-ID: <20131205102740.GI29200@e106331-lin.cambridge.arm.com>
Date:	Thu, 5 Dec 2013 10:27:40 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Georgi Djakov <gdjakov@...sol.com>
Cc:	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	"cjb@...top.org" <cjb@...top.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	Pawel Moll <Pawel.Moll@....com>,
	"swarren@...dotorg.org" <swarren@...dotorg.org>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"galak@...eaurora.org" <galak@...eaurora.org>,
	"rob@...dley.net" <rob@...dley.net>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
	"subhashj@...eaurora.org" <subhashj@...eaurora.org>
Subject: Re: [PATCH v7 2/2] mmc: sdhci-msm: Initial support for MSM chipsets

On Wed, Nov 06, 2013 at 03:56:45PM +0000, Georgi Djakov wrote:
> This platform driver adds the initial support of Secure
> Digital Host Controller Interface compliant controller
> found in Qualcomm MSM chipsets.
> 
> Signed-off-by: Georgi Djakov <gdjakov@...sol.com>
> ---
>  drivers/mmc/host/Kconfig     |   13 +
>  drivers/mmc/host/Makefile    |    1 +
>  drivers/mmc/host/sdhci-msm.c |  651 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 665 insertions(+)
>  create mode 100644 drivers/mmc/host/sdhci-msm.c

[...]

> +static int sdhci_msm_dt_parse_vreg_info(struct device *dev,
> +                                       struct sdhci_msm_reg_data *vreg,
> +                                       const char *vreg_name)
> +{
> +       int len;
> +       const __be32 *prop;

Seeing raw property handling in drivers worries me. If there's a reason
to touch the raw DTB we should add helpers to do it rather than leaking
binary format issues into drivers.

> +       char prop_name[MAX_PROP_SIZE];
> +       struct device_node *np = dev->of_node;
> +
> +       snprintf(prop_name, MAX_PROP_SIZE, "%s-supply", vreg_name);
> +       if (!of_parse_phandle(np, prop_name, 0)) {
> +               dev_info(dev, "No vreg data found for %s\n", vreg_name);
> +               return -EINVAL;
> +       }
> +
> +       vreg->name = vreg_name;
> +
> +       snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-lpm-sup", vreg_name);
> +       if (of_get_property(np, prop_name, NULL))
> +               vreg->lpm_sup = true;
> +
> +       snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-voltage-level", vreg_name);
> +       prop = of_get_property(np, prop_name, &len);
> +       if (!prop || (len != (2 * sizeof(__be32)))) {
> +               dev_warn(dev, "%s %s property\n",
> +               prop ? "invalid format" : "no", prop_name);
> +       } else {
> +               vreg->low_vol_level = be32_to_cpup(&prop[0]);
> +               vreg->high_vol_level = be32_to_cpup(&prop[1]);
> +       }

You can use of_property_read_u32_array here.

> +
> +       snprintf(prop_name, MAX_PROP_SIZE, "qcom,%s-current-level", vreg_name);
> +       prop = of_get_property(np, prop_name, &len);
> +       if (!prop || (len != (2 * sizeof(__be32)))) {
> +               dev_warn(dev, "%s %s property\n",
> +                        prop ? "invalid format" : "no", prop_name);
> +       } else {
> +               vreg->lpm_uA = be32_to_cpup(&prop[0]);
> +               vreg->hpm_uA = be32_to_cpup(&prop[1]);
> +       }

Likewise.

[...]

> +       /*
> +        * CORE_SW_RST above may trigger power irq if previous status of PWRCTL
> +        * was either BUS_ON or IO_HIGH_V. So before we enable the power irq
> +        * interrupt in GIC (by registering the interrupt handler), we need to
> +        * ensure that any pending power irq interrupt status is acknowledged
> +        * otherwise power irq interrupt handler would be fired prematurely.
> +        */
> +       irq_status = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_STATUS);
> +       writel_relaxed(irq_status, (msm_host->core_mem + CORE_PWRCTL_CLEAR));
> +       irq_ctl = readl_relaxed(msm_host->core_mem + CORE_PWRCTL_CTL);
> +       if (irq_status & (CORE_PWRCTL_BUS_ON | CORE_PWRCTL_BUS_OFF))
> +               irq_ctl |= CORE_PWRCTL_BUS_SUCCESS;
> +       if (irq_status & (CORE_PWRCTL_IO_HIGH | CORE_PWRCTL_IO_LOW))
> +               irq_ctl |= CORE_PWRCTL_IO_SUCCESS;
> +       writel_relaxed(irq_ctl, (msm_host->core_mem + CORE_PWRCTL_CTL));
> +       /*
> +        * Ensure that above writes are propogated before interrupt enablement
> +        * in GIC.
> +        */
> +       mb();

Does this guarantee that the device has finished responding to the write
and deasserted the interrupt line (i.e. does the device only acknowledge
the write once that is true)?

Thanks,
Mark.
--
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/