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 14:50:47 +0530
From:	Vivek Gautam <gautamvivek1987@...il.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Vivek Gautam <gautam.vivek@...sung.com>, 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

Hi Russell,


On Thu, Dec 27, 2012 at 5:56 AM, Russell King - ARM Linux
<linux@....linux.org.uk> wrote:
> 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.
>
True, i should have checked the things. :-(
ioremap() won't return error. Will amend this to check against NULL.

>> +/*
>> + * 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);
>
Sure will amend this.
A similar way suggested by Sylwester in the earlier mail in this thread:

reg = on ? reg & ~mask : reg | mask;
writel(reg, sphy->phyctrl_pmureg);

Does this look fine ?

>> +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.)
>
Right, we won't need this cast since match->data is a void pointer.
Will also make the returned pointer const.

>>  #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?
>
True we don't need this cast. Will remove this one.

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

True we don't need this cast. Will remove this one.

Thanks for the review :-)


-- 
Regards
Vivek
--
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/