[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/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