| 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
| ||
|
Date: Wed, 8 Jul 2020 13:59:46 +0530
From: Anand Moon <linux.amoon@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
linux-samsung-soc@...r.kernel.org,
Linux Kernel <linux-kernel@...r.kernel.org>,
Kishon Vijay Abraham I <kishon@...com>,
Vinod Koul <vkoul@...nel.org>, Kukjin Kim <kgene@...nel.org>,
Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH v3] phy: samsung: Use readl_poll_timeout function
Hi Krzysztof,
On Tue, 7 Jul 2020 at 17:06, Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On Tue, Jul 07, 2020 at 09:59:08AM +0000, Anand Moon wrote:
> > Instead of a busy waiting loop while loop using udelay
>
> You doubled "loop".
>
I will fix this in the next version.
> > use readl_poll_timeout function to check the condition
> > is met or timeout occurs in crport_handshake function.
>
> Still you did not mention that you convert the function to use sleeping
> primitive. You also did not mention whether it is actually allowed in
> this context and I am not sure if you investigated it.
>
OK, I am not sure how to resolve your query.
I learned some new things.
So here are some points.
-- Yes read_poll_timeout internally used might_sleep if sleep_us != 0
under some condition.
-- None of the code in phy-exynos5-usbdrd.c is called using kernel
synchronization
methods like spinlock / mutex.
-- I have checked this function is called non atomic context.
see below.
[ 6.006395] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.013042] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.019646] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.026174] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.032699] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.039246] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.045707] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.052276] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.058760] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.065189] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.071631] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.078033] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.084433] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.090812] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.097102] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.103413] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.109670] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
[ 6.115871] exynos5_usb3drd_phy 12500000.phy: Not In atomic context
> Best regards,
> Krzysztof
>
> >
> > Fixes: d8c80bb3b55b ("phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800")
> > Signed-off-by: Anand Moon <linux.amoon@...il.com>
> > ---
> > Changes v3:
> > --Fix the commit message.
> > --Drop the variable, used the value directly.
> > Changes v2:
> > --used the default timeout values.
> > --Added missing Fixed tags.
> > ---
> > drivers/phy/samsung/phy-exynos5-usbdrd.c | 35 +++++++-----------------
> > 1 file changed, 10 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > index e510732afb8b..fa75fa88da33 100644
> > --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> > @@ -16,6 +16,7 @@
> > #include <linux/of.h>
> > #include <linux/of_address.h>
> > #include <linux/of_device.h>
> > +#include <linux/iopoll.h>
> > #include <linux/phy/phy.h>
> > #include <linux/platform_device.h>
> > #include <linux/mutex.h>
> > @@ -556,40 +557,24 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
> > static int crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
> > u32 val, u32 cmd)
> > {
> > - u32 usec = 100;
> > unsigned int result;
> > + int err;
> >
> > writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
> >
> > - do {
> > - result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
> > - if (result & PHYREG1_CR_ACK)
> > - break;
> > -
> > - udelay(1);
> > - } while (usec-- > 0);
> > -
> > - if (!usec) {
> > - dev_err(phy_drd->dev,
> > - "CRPORT handshake timeout1 (0x%08x)\n", val);
> > + err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
> > + result, (result & PHYREG1_CR_ACK), 1, 100);
> > + if (err) {
> > + dev_err(phy_drd->dev, "CRPORT handshake timeout1 (0x%08x)\n", val);
> > return -ETIME;
Here return should be -ETIMEDOUT;
> > }
> >
> > - usec = 100;
> > -
> > writel(val, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
> >
> > - do {
> > - result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
> > - if (!(result & PHYREG1_CR_ACK))
> > - break;
> > -
> > - udelay(1);
> > - } while (usec-- > 0);
> > -
> > - if (!usec) {
> > - dev_err(phy_drd->dev,
> > - "CRPORT handshake timeout2 (0x%08x)\n", val);
> > + err = readl_poll_timeout(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1,
> > + result, !(result & PHYREG1_CR_ACK), 1, 100);
> > + if (err) {
> > + dev_err(phy_drd->dev, "CRPORT handshake timeout2 (0x%08x)\n", val);
> > return -ETIME;
> > }
> >
Best Regards
-Anand
Powered by blists - more mailing lists