linux-kernel - Re: [PATCH v4] usb: phy: samsung: Add support to set pmu isolation

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:	Thu, 27 Dec 2012 00:26:46 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Vivek Gautam <gautam.vivek@...sung.com>
Cc:	linux-usb@...r.kernel.org, l.majewski@...sung.com,
	linux-samsung-soc@...r.kernel.org, heiko@...ech.de,
	p.paneri@...sung.com, gregkh@...uxfoundation.org,
	devicetree-discuss@...ts.ozlabs.org,
	broonie@...nsource.wolfsonmicro.com, linux-kernel@...r.kernel.org,
	balbi@...com, dianders@...omium.org, grant.likely@...retlab.ca,
	kyungmin.park@...sung.com, kgene.kim@...sung.com,
	thomas.abraham@...aro.org, ben-linux@...ff.org,
	sylvester.nawrocki@...il.com, t.figa@...sung.com,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4] usb: phy: samsung: Add support to set pmu isolation

On Wed, Dec 26, 2012 at 05:58:32PM +0530, Vivek Gautam wrote:
> +	if (!ret)
> +		sphy->phyctrl_pmureg = ioremap(reg[0], reg[1]);
> +
> +	of_node_put(usbphy_pmu);
> +
> +	if (IS_ERR_OR_NULL(sphy->phyctrl_pmureg)) {

No.  Learn what the error return values are from functions.  Using the
wrong ones is buggy.  ioremap() only ever returns NULL on error.  You
must check against NULL, and not use the IS_ERR stuff.

> +/*
> + * Set isolation here for phy.
> + * SOCs control this by controlling corresponding PMU registers
> + */
> +static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, int on)
> +{
> +	u32 reg;
> +	int en_mask;
> +
> +	if (!sphy->phyctrl_pmureg) {
> +		dev_warn(sphy->dev, "Can't set pmu isolation\n");
> +		return;
> +	}
> +
> +	reg = readl(sphy->phyctrl_pmureg);
> +
> +	en_mask = sphy->drv_data->devphy_en_mask;
> +
> +	if (on)
> +		writel(reg & ~en_mask, sphy->phyctrl_pmureg);
> +	else
> +		writel(reg | en_mask, sphy->phyctrl_pmureg);

What guarantees that this read-modify-write sequence of this register safe?
And, btw, this can't be optimised very well because of the barrier inside
writel().  This would be better:

	if (on)
		reg &= ~en_mask;
	else
		reg |= en_mask;

	writel(reg, sphy->phyctrl_pmureg);

> +static inline struct samsung_usbphy_drvdata
> +*samsung_usbphy_get_driver_data(struct platform_device *pdev)
>  {
>  	if (pdev->dev.of_node) {
>  		const struct of_device_id *match;
>  		match = of_match_node(samsung_usbphy_dt_match,
>  							pdev->dev.of_node);
> -		return (int) match->data;
> +		return (struct samsung_usbphy_drvdata *) match->data;

match->data is a const void pointer.  Is there a reason you need this
cast here?  What if you made the returned pointer from this function
also const and fixed up all its users (no user should modify this
data.)

>  #ifdef CONFIG_OF
>  static const struct of_device_id samsung_usbphy_dt_match[] = {
>  	{
>  		.compatible = "samsung,s3c64xx-usbphy",
> -		.data = (void *)TYPE_S3C64XX,
> +		.data = (void *)&usbphy_s3c64xx,

Why do you need this cast?

>  	}, {
>  		.compatible = "samsung,exynos4210-usbphy",
> -		.data = (void *)TYPE_EXYNOS4210,
> +		.data = (void *)&usbphy_exynos4,

Ditto.
--
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/