| 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
| ||
|
Message-ID: <ccfb841c-b518-3107-eb9b-a62169970bb1@wanyeetech.com>
Date: Thu, 27 Aug 2020 23:35:30 +0800
From: Zhou Yanjie <zhouyanjie@...yeetech.com>
To: Paul Cercueil <paul@...pouillou.net>,
Felipe Balbi <balbi@...nel.org>
Cc: 周正 <sernia.zhou@...mail.com>,
漆鹏振 <aric.pzqi@...enic.com>, od@...c.me,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to
HW register
Hi
在 2020/8/27 下午9:50, Paul Cercueil 写道:
>
>
> Le jeu. 27 août 2020 à 16:25, Felipe Balbi <balbi@...nel.org> a écrit :
>>
>> Hi,
>>
>> Paul Cercueil <paul@...pouillou.net> writes:
>>>>> @@ -172,7 +172,8 @@ static int ingenic_usb_phy_init(struct usb_phy
>>>>> *phy)
>>>>> return err;
>>>>> }
>>>>>
>>>>> - priv->soc_info->usb_phy_init(phy);
>>>>> + reg = priv->soc_info->usb_phy_init(phy);
>>>>> + writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>>
>>>> not fixing any bug.
>>>>
>>>> Looking at the code, the bug follows after this line. It would
>>>> suffice
>>>> to read REG_USBPCR_OFFSET in order to initialize reg. This bug fix
>>>> could
>>>> have been a one liner.
>>>
>>> There's no need to re-read a register when you have the value readily
>>> available. It just needs to be returned from the usb_phy_init
>>> callbacks. But yes, it's not a one-liner.
>>
>> there's a difference between making a bug fix and reworking the behavior
>> of the driver ;-)
>
> The one-liner is actually what changes the behavior of the driver,
> since previously the code did not read back the register.
>
> In this case I guess it's fine, because the register does not have
> volatile bits.
>
>>>>> @@ -195,19 +196,15 @@ static void ingenic_usb_phy_remove(void *phy)
>>>>> usb_remove_phy(phy);
>>>>> }
>>>>>
>>>>> -static void jz4770_usb_phy_init(struct usb_phy *phy)
>>>>> +static u32 jz4770_usb_phy_init(struct usb_phy *phy)
>>>>
>>>> not a bug fix
>>>>
>>>>> {
>>>>> - struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>>> - u32 reg;
>>>>> -
>>>>> - reg = USBPCR_AVLD_REG | USBPCR_COMMONONN |
>>>>> USBPCR_IDPULLUP_ALWAYS
>>>>> |
>>>>> + return USBPCR_AVLD_REG | USBPCR_COMMONONN |
>>>>> USBPCR_IDPULLUP_ALWAYS |
>>>>> USBPCR_COMPDISTUNE_DFT | USBPCR_OTGTUNE_DFT |
>>>>> USBPCR_SQRXTUNE_DFT |
>>>>> USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT |
>>>>> USBPCR_TXVREFTUNE_DFT |
>>>>> USBPCR_POR;
>>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>>
>>>> not a bug fix
>>>>
>>>>> }
>>>>>
>>>>> -static void jz4780_usb_phy_init(struct usb_phy *phy)
>>>>> +static u32 jz4780_usb_phy_init(struct usb_phy *phy)
>>>>
>>>> not a bug fix
>>>>
>>>>> @@ -216,11 +213,10 @@ static void jz4780_usb_phy_init(struct
>>>>> usb_phy *phy)
>>>>> USBPCR1_WORD_IF_16BIT;
>>>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>>>
>>>>> - reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>>> + return USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>>>
>>>> not a bug fix
>>>>
>>>>> }
>>>>>
>>>>> -static void x1000_usb_phy_init(struct usb_phy *phy)
>>>>> +static u32 x1000_usb_phy_init(struct usb_phy *phy)
>>>>
>>>> not a bug fix
>>>>
>>>>> {
>>>>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>>> u32 reg;
>>>>> @@ -228,13 +224,12 @@ static void x1000_usb_phy_init(struct usb_phy
>>>>> *phy)
>>>>> reg = readl(priv->base + REG_USBPCR1_OFFSET) |
>>>>> USBPCR1_WORD_IF_16BIT;
>>>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>>>
>>>>> - reg = USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>>>>> + return USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>>>>> USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
>>>>> USBPCR_COMMONONN | USBPCR_POR;
>>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>>
>>>> not a bug fix
>>>>
>>>>> }
>>>>>
>>>>> -static void x1830_usb_phy_init(struct usb_phy *phy)
>>>>> +static u32 x1830_usb_phy_init(struct usb_phy *phy)
>>>>
>>>> not a bug fix
>>>>
>>>>> {
>>>>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>>>> u32 reg;
>>>>> @@ -246,9 +241,8 @@ static void x1830_usb_phy_init(struct usb_phy
>>>>> *phy)
>>>>> USBPCR1_DMPD | USBPCR1_DPPD;
>>>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>>>
>>>>> - reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT
>>>>> | USBPCR_TXPREEMPHTUNE |
>>>>> + return USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT |
>>>>> USBPCR_TXPREEMPHTUNE |
>>>>> USBPCR_COMMONONN | USBPCR_POR;
>>>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>>>
>>>> not a bug fix
>>>
>>> Well, if you don't like my bug fix, next time wait for my Reviewed-by.
>>
>> why so angry? Take a break every once in a while. Besides, someone else
>> already sent the oneliner before you ;-)
>
> I'm just pissed that this patch has not been tested. I don't like
> sloppy work.
>
This is my fault. This error occurred in the v5 version, but was
corrected in the v6 version, but I don't know why v5 was merged into the
mainline and v6 was not merged, which caused this problem.
>> In any case, why should I wait for your Reviewed-by? Get maintainer
>> doesn't list you as the maintainer for it. Do you want to update
>> MAINTAINERS by any chance?
>
> Yes, I thought I was (I'm maintainer of all Ingenic drivers), that
> also explains why I wasn't Cc'd for the oneliner patch you mentioned...
>
I checked again, get_maintainer did not give the correct information,
which caused my script to not add Paul to the cc list.
> IIRC Zhou has a patch to move the driver to drivers/phy/, I'll add
> myself as maintainer once it's moved there.
>
I'll resend it soon.
Thanks and best regards!
> -Paul
>
Powered by blists - more mailing lists