| Message ID | 20200915141038.3371-1-biju.das.jz@bp.renesas.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | net: ravb: Fix NULL pointer access | expand |
On 15. 09. 20 16:10, Biju Das wrote: > Some phy's like rtl8211e do not support writeext() callback. Add a check to > avoid null pointer access before calling writeext() callback. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/net/ravb.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c > index 886f53ee82..623d377897 100644 > --- a/drivers/net/ravb.c > +++ b/drivers/net/ravb.c > @@ -438,7 +438,8 @@ static int ravb_config(struct udevice *dev) > > writel(mask, eth->iobase + RAVB_REG_ECMR); > > - phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); > + if (phy->drv->writeext) > + phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); > > return 0; > } > Make sense. Reviewed-by: Michal Simek <michal.simek@xilinx.com> Thanks, Michal
On 9/15/20 4:10 PM, Biju Das wrote: [...] > +++ b/drivers/net/ravb.c > @@ -438,7 +438,8 @@ static int ravb_config(struct udevice *dev) > > writel(mask, eth->iobase + RAVB_REG_ECMR); > > - phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); > + if (phy->drv->writeext) > + phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); Shouldn't we rather move this into the PHY driver altogether ? I _think_ this might be specific to some Micrel PHY.
Hi Marek, Thanks for the review. > Subject: Re: [PATCH] net: ravb: Fix NULL pointer access > > On 9/15/20 4:10 PM, Biju Das wrote: > [...] > > +++ b/drivers/net/ravb.c > > @@ -438,7 +438,8 @@ static int ravb_config(struct udevice *dev) > > > > writel(mask, eth->iobase + RAVB_REG_ECMR); > > > > -phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); > > +if (phy->drv->writeext) > > +phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); > > Shouldn't we rather move this into the PHY driver altogether ? If I fix, the phydriver with empty function, compiler will complain about unused parameter right? What about other phy's which doesn't have this callback. > I _think_ this might be specific to some Micrel PHY. As mentioned in the commit message, it is seen with Realtek phy(rtl8211e) and crash seen on Hihope RZ/G2M board. Thanks and regards, biju Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
On 9/16/20 5:43 PM, Biju Das wrote: > Hi Marek, Hi, [...] >>> +++ b/drivers/net/ravb.c >>> @@ -438,7 +438,8 @@ static int ravb_config(struct udevice *dev) >>> >>> writel(mask, eth->iobase + RAVB_REG_ECMR); >>> >>> -phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); >>> +if (phy->drv->writeext) >>> +phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); >> >> Shouldn't we rather move this into the PHY driver altogether ? > > If I fix, the phydriver with empty function, compiler will complain about unused parameter right? > What about other phy's which doesn't have this callback. I mean, should we not move this entire code which configures something in a Micrel PHY, and is specific to Micrel PHY, into the Micrel PHY driver and remove the whole call of writeext from this ethernet driver ? >> I _think_ this might be specific to some Micrel PHY. > As mentioned in the commit message, it is seen with Realtek phy(rtl8211e) and crash seen on Hihope RZ/G2M board. Right, because that writeext seems to be micrel-phy specific.
Hi Marek, Thanks for the feedback. > Subject: Re: [PATCH] net: ravb: Fix NULL pointer access > > On 9/16/20 5:43 PM, Biju Das wrote: > > Hi Marek, > > Hi, > > [...] > > >>> +++ b/drivers/net/ravb.c > >>> @@ -438,7 +438,8 @@ static int ravb_config(struct udevice *dev) > >>> > >>> writel(mask, eth->iobase + RAVB_REG_ECMR); > >>> > >>> -phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); > >>> +if (phy->drv->writeext) > >>> +phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); > >> > >> Shouldn't we rather move this into the PHY driver altogether ? > > > > If I fix, the phydriver with empty function, compiler will complain about > unused parameter right? > > What about other phy's which doesn't have this callback. > > I mean, should we not move this entire code which configures something in a > Micrel PHY, and is specific to Micrel PHY, into the Micrel PHY driver and > remove the whole call of writeext from this ethernet driver ? Ok , this function is invoked during data transfer. We could remove this function and test on all RCar boards to see any issues present or not? By looking at [1], only this driver is using writeext. [1]https://elixir.bootlin.com/u-boot/v2020.10-rc4/A/ident/writeext Cheers, Biju Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
On 9/18/20 5:26 PM, Biju Das wrote: [...] >>>>> +++ b/drivers/net/ravb.c >>>>> @@ -438,7 +438,8 @@ static int ravb_config(struct udevice *dev) >>>>> >>>>> writel(mask, eth->iobase + RAVB_REG_ECMR); >>>>> >>>>> -phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); >>>>> +if (phy->drv->writeext) >>>>> +phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); >>>> >>>> Shouldn't we rather move this into the PHY driver altogether ? >>> >>> If I fix, the phydriver with empty function, compiler will complain about >> unused parameter right? >>> What about other phy's which doesn't have this callback. >> >> I mean, should we not move this entire code which configures something in a >> Micrel PHY, and is specific to Micrel PHY, into the Micrel PHY driver and >> remove the whole call of writeext from this ethernet driver ? > > Ok , this function is invoked during data transfer. The ravb_config() is invoked when starting the interface, see ravb_start(), it is not repeatedly called during data transfer. > We could remove this function and test on all RCar boards to see any issues present or not? No, because calling the writeext configures something in the Micrel KSZ90x1 PHY, and by removing it, I suspect the Micrel PHY would stop working. > By looking at [1], only this driver is using writeext. > [1]https://elixir.bootlin.com/u-boot/v2020.10-rc4/A/ident/writeext git grep indicates a couple more sites where the writeext is called. But look into the KSZ9031 datasheet, that particular writeext call seems to be setting up RGMII Clock Pad Skew (MMD Address 2, Register 8), and I think there is a matching DT binding to set those up too, rxc-skew-ps and txc-skew-ps I think. So I think if you set those two DT properties in rcar2/3 DTs accordingly (and that might already be the case, but please double-check), you can even drop this entire writeext altogether. [...]
Hi Marek, Thanks for the feedback. > Subject: Re: [PATCH] net: ravb: Fix NULL pointer access > > On 9/18/20 5:26 PM, Biju Das wrote: > [...] > > >>>>> +++ b/drivers/net/ravb.c > >>>>> @@ -438,7 +438,8 @@ static int ravb_config(struct udevice *dev) > >>>>> > >>>>> writel(mask, eth->iobase + RAVB_REG_ECMR); > >>>>> > >>>>> -phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); > >>>>> +if (phy->drv->writeext) > >>>>> +phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); > >>>> > >>>> Shouldn't we rather move this into the PHY driver altogether ? > >>> > >>> If I fix, the phydriver with empty function, compiler will complain > >>> about > >> unused parameter right? > >>> What about other phy's which doesn't have this callback. > >> > >> I mean, should we not move this entire code which configures > >> something in a Micrel PHY, and is specific to Micrel PHY, into the > >> Micrel PHY driver and remove the whole call of writeext from this > ethernet driver ? > > > > Ok , this function is invoked during data transfer. > > The ravb_config() is invoked when starting the interface, see ravb_start(), it > is not repeatedly called during data transfer. Yep you are correct , it will be called during starting of the interface. I have seen the crash when I started doing tftp or ping, which in turn calls ravb_start. > > We could remove this function and test on all RCar boards to see any issues > present or not? > > No, because calling the writeext configures something in the Micrel > KSZ90x1 PHY, and by removing it, I suspect the Micrel PHY would stop > working. I agree, otherwise it will use default values which may cause issues. > > By looking at [1], only this driver is using writeext. > > [1]https://elixir.bootlin.com/u-boot/v2020.10-rc4/A/ident/writeext > > git grep indicates a couple more sites where the writeext is called. > But look into the KSZ9031 datasheet, that particular writeext call seems to be > setting up RGMII Clock Pad Skew (MMD Address 2, Register 8), and I think > there is a matching DT binding to set those up too, rxc-skew-ps and txc- > skew-ps I think. Thanks for the pointers. I checked the configs[2] which uses renesas ravb driver and found that we are defining only rxc-skew-ps in all dts. since CONFIG DM_ETH is defined it is already picking the value corresponding to "rxc-skew-ps". For txc-skew-ps anyway the value is default one. So we don't care. > So I think if you set those two DT properties in rcar2/3 DTs accordingly (and > that might already be the case, but please double-check), you can even drop > this entire writeext altogether. I did a grep and the below configs enables renesas ravb driver .Only Gen3 uses renesas ravb driver and all of them have "rxc-skew-ps" in Devicetree. So I agree with you, it is safe to remove writephyext. I will post V2, removing this [2] u-boot/configs/rcar3_ulcb_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/r8a77990_ebisu_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/r8a774a1_beacon_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/hihope_rzg2_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/r8a77995_draak_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/r8a77970_eagle_defconfig:CONFIG_RENESAS_RAVB=y u-boot/configs/rcar3_salvator-x_defconfig:CONFIG_RENESAS_RAVB=y Cheers, Biju Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
On 9/19/20 1:14 PM, Biju Das wrote: [...] >>> By looking at [1], only this driver is using writeext. >>> [1]https://elixir.bootlin.com/u-boot/v2020.10-rc4/A/ident/writeext >> >> git grep indicates a couple more sites where the writeext is called. >> But look into the KSZ9031 datasheet, that particular writeext call seems to be >> setting up RGMII Clock Pad Skew (MMD Address 2, Register 8), and I think >> there is a matching DT binding to set those up too, rxc-skew-ps and txc- >> skew-ps I think. > > Thanks for the pointers. I checked the configs[2] which uses renesas ravb driver > and found that we are defining only rxc-skew-ps in all dts. > > since CONFIG DM_ETH is defined it is already picking the value corresponding to "rxc-skew-ps". > > For txc-skew-ps anyway the value is default one. So we don't care. Are you sure (0xf << 5) | 0x19 is the same as the default value of the clock pad skew register ? [...]
Hi Marek, Thanks for the feedback. > Subject: Re: [PATCH] net: ravb: Fix NULL pointer access > > On 9/19/20 1:14 PM, Biju Das wrote: > [...] > >>> By looking at [1], only this driver is using writeext. > >>> [1]https://elixir.bootlin.com/u-boot/v2020.10-rc4/A/ident/writeext > >> > >> git grep indicates a couple more sites where the writeext is called. > >> But look into the KSZ9031 datasheet, that particular writeext call > >> seems to be setting up RGMII Clock Pad Skew (MMD Address 2, Register > >> 8), and I think there is a matching DT binding to set those up too, > >> rxc-skew-ps and txc- skew-ps I think. > > > > Thanks for the pointers. I checked the configs[2] which uses renesas > > ravb driver and found that we are defining only rxc-skew-ps in all dts. > > > > since CONFIG DM_ETH is defined it is already picking the value > corresponding to "rxc-skew-ps". > > > > For txc-skew-ps anyway the value is default one. So we don't care. > > Are you sure (0xf << 5) | 0x19 is the same as the default value of the clock > pad skew register ? No. As per [1] & [2], the default values for this registers are 0xf [1] https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/drivers/net/phy/micrel_ksz90x1.c#L105 [2] http://ww1.microchip.com/downloads/en/devicedoc/00002117f.pdf if we remove writephyext, by looking the code at [1], rxc-skew-ps will be taken from the device tree[3] and "txc-skew-pc" will be the default value(0xf). [3]https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/arch/arm/dts/salvator-common.dtsi#L331 I will check this and let you know the results after checking on RCar board. Unfortunately currently I don't have RCar board. Cheers, Biju Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
On 9/19/20 8:14 PM, Biju Das wrote: Hi, [...] >>>>> By looking at [1], only this driver is using writeext. >>>>> [1]https://elixir.bootlin.com/u-boot/v2020.10-rc4/A/ident/writeext >>>> >>>> git grep indicates a couple more sites where the writeext is called. >>>> But look into the KSZ9031 datasheet, that particular writeext call >>>> seems to be setting up RGMII Clock Pad Skew (MMD Address 2, Register >>>> 8), and I think there is a matching DT binding to set those up too, >>>> rxc-skew-ps and txc- skew-ps I think. >>> >>> Thanks for the pointers. I checked the configs[2] which uses renesas >>> ravb driver and found that we are defining only rxc-skew-ps in all dts. >>> >>> since CONFIG DM_ETH is defined it is already picking the value >> corresponding to "rxc-skew-ps". >>> >>> For txc-skew-ps anyway the value is default one. So we don't care. >> >> Are you sure (0xf << 5) | 0x19 is the same as the default value of the clock >> pad skew register ? > > No. > As per [1] & [2], the default values for this registers are 0xf So the combined value of the MMD 2-8 register is (0xf << 5) | (0xf << 0) , right. > [1] https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/drivers/net/phy/micrel_ksz90x1.c#L105 > [2] http://ww1.microchip.com/downloads/en/devicedoc/00002117f.pdf > > if we remove writephyext, by looking the code at [1], rxc-skew-ps will be taken from the device tree[3] and "txc-skew-pc" will be the default value(0xf). > [3]https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/arch/arm/dts/salvator-common.dtsi#L331 So you want to check whether each RCar3 DT contains a PHY node and that PHY node has rxc-skew-ps and txc-skew-ps , which combined then results into a register value (0xf << 5) | (0x19 << 0) . > I will check this and let you know the results after checking on RCar board. Unfortunately currently I don't have RCar board. It's enough to just check the DTs and verify that they set the matching correct values of rxc-skew-ps/txc-skew-ps . I can test it on the real hardware. If you want, you can add the txc-skew-ps into the Linux R-Car3 DTs too. btw unrelated, you seem to have rxc-skew-ps in your hihope-rzg2-ex.dtsi, but I think you don't have KSZ9031 PHY, so maybe you want to remove it form your DT too. Thanks
Hi Marek, Thanks for the feedback. > Subject: Re: [PATCH] net: ravb: Fix NULL pointer access > > On 9/19/20 8:14 PM, Biju Das wrote: > > Hi, > > [...] > > >>>>> By looking at [1], only this driver is using writeext. > >>>>> [1]https://elixir.bootlin.com/u-boot/v2020.10-rc4/A/ident/writeext > >>>> > >>>> git grep indicates a couple more sites where the writeext is called. > >>>> But look into the KSZ9031 datasheet, that particular writeext call > >>>> seems to be setting up RGMII Clock Pad Skew (MMD Address 2, > >>>> Register 8), and I think there is a matching DT binding to set > >>>> those up too, rxc-skew-ps and txc- skew-ps I think. > >>> > >>> Thanks for the pointers. I checked the configs[2] which uses > >>> renesas ravb driver and found that we are defining only rxc-skew-ps in > all dts. > >>> > >>> since CONFIG DM_ETH is defined it is already picking the value > >> corresponding to "rxc-skew-ps". > >>> > >>> For txc-skew-ps anyway the value is default one. So we don't care. > >> > >> Are you sure (0xf << 5) | 0x19 is the same as the default value of > >> the clock pad skew register ? > > > > No. > > As per [1] & [2], the default values for this registers are 0xf > > So the combined value of the MMD 2-8 register is (0xf << 5) | (0xf << 0) , > right. > > > [1] > > https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/drivers/net/phy/ > > micrel_ksz90x1.c#L105 [2] > > http://ww1.microchip.com/downloads/en/devicedoc/00002117f.pdf > > > > if we remove writephyext, by looking the code at [1], rxc-skew-ps will be > taken from the device tree[3] and "txc-skew-pc" will be the default > value(0xf). > > [3]https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/arch/arm/dts/ > > salvator-common.dtsi#L331 > > So you want to check whether each RCar3 DT contains a PHY node and that > PHY node has rxc-skew-ps and txc-skew-ps , which combined then results > into a register value (0xf << 5) | (0x19 << 0) . rxc-skew-ps set in DTS is 1500. 1 step is 60 ps, so 1500/60 = 25 which is 0x19 and this value will be overridden and stored in ofcfg->grp's val[0]. > > I will check this and let you know the results after checking on RCar board. > Unfortunately currently I don't have RCar board. > > It's enough to just check the DTs and verify that they set the matching > correct values of rxc-skew-ps/txc-skew-ps . I can test it on the real hardware. Yes, that way we can make sure the mapping operation is correct for this phy. 1500 in dts after mapping operation should override ofcfg->grp's val[0] with 0x19. > If you want, you can add the txc-skew-ps into the Linux R-Car3 DTs too. We don't need to set txc-skew-ps in dts, since it is same as default value and is filled in ofcfg->grp's val[1]. We can avoid unnecessary mapping operations by not specifying in device tree, for default values. > btw unrelated, you seem to have rxc-skew-ps in your hihope-rzg2-ex.dtsi, > but I think you don't have KSZ9031 PHY, so maybe you want to remove it > form your DT too. OK. Thanks for reporting this. Will fix this in kernel as well as here. Cheers, Biju Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647
On 9/20/20 9:34 AM, Biju Das wrote: Hi, [...] >>> if we remove writephyext, by looking the code at [1], rxc-skew-ps will be >> taken from the device tree[3] and "txc-skew-pc" will be the default >> value(0xf). >>> [3]https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/arch/arm/dts/ >>> salvator-common.dtsi#L331 >> >> So you want to check whether each RCar3 DT contains a PHY node and that >> PHY node has rxc-skew-ps and txc-skew-ps , which combined then results >> into a register value (0xf << 5) | (0x19 << 0) . > > rxc-skew-ps set in DTS is 1500. 1 step is 60 ps, so 1500/60 = 25 which is 0x19 and this value will be overridden and stored in ofcfg->grp's val[0]. OK, good, thanks. >>> I will check this and let you know the results after checking on RCar board. >> Unfortunately currently I don't have RCar board. >> >> It's enough to just check the DTs and verify that they set the matching >> correct values of rxc-skew-ps/txc-skew-ps . I can test it on the real hardware. > > Yes, that way we can make sure the mapping operation is correct for this phy. 1500 in dts after mapping operation should override > ofcfg->grp's val[0] with 0x19. > >> If you want, you can add the txc-skew-ps into the Linux R-Car3 DTs too. > > We don't need to set txc-skew-ps in dts, since it is same as default value and is filled in ofcfg->grp's val[1]. > We can avoid unnecessary mapping operations by not specifying in device tree, for default values. Hopefully the default setting will not change in some new revision of the PHY then.
diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c index 886f53ee82..623d377897 100644 --- a/drivers/net/ravb.c +++ b/drivers/net/ravb.c @@ -438,7 +438,8 @@ static int ravb_config(struct udevice *dev) writel(mask, eth->iobase + RAVB_REG_ECMR); - phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); + if (phy->drv->writeext) + phy->drv->writeext(phy, -1, 0x02, 0x08, (0x0f << 5) | 0x19); return 0; }