[PATCH] net: ravb: Fix NULL pointer access

[Biju Das]

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