Re: [PATCH] [RFC] net: phy: Fix reboot crash if CONFIG_IP_PNP is not set

[Geert Uytterhoeven]

Hi Andrew,

On Tue, Jan 5, 2021 at 3:10 PM Andrew Lunn <andrew@xxxxxxx> wrote:
> > I added a statically-linked ethtool binary to my initramfs, and can
> > confirm that retrieving the PHY statistics does not access the PHY
> > registers when the device is suspended:
> >
> >     # ethtool --phy-statistics eth0
> >     no stats available
> >     # ifconfig eth0 up
> >     # ethtool --phy-statistics eth0
> >     PHY statistics:
> >        phy_receive_errors: 0
> >        phy_idle_errors: 0
> >     #
> >
> > In the past, we've gone to great lengths to avoid accessing the PHY
> > registers when the device is suspended, usually in the statistics
> > handling (see e.g. [1][2]).
>
> I would argue that is the wrong approach. The PHY device is a
> device. It has its own lifetime. You would not suspend a PCI bus
> controller without first suspending all PCI devices on the bus etc.

Makes sense.  So perhaps the PHY devices should become full citizens
instead, and start using Runtime PM theirselves? Then the device
framework will take care of it automatically through the devices'
parent/child relations.

This would be similar to e.g. commit 3a611e26e958b037 ("net/smsc911x:
Add minimal runtime PM support"), but now for PHYs w.r.t. their parent
network controller device, instead of for the network controller device
w.r.t. its parent bus.

> > +static int sh_mdiobb_read(struct mii_bus *bus, int phy, int reg)
> > +{
> > +     struct bb_info *bb = container_of(bus->priv, struct bb_info, ctrl);
>
> mii_bus->parent should give you dev, so there is no need to add it to
> bb_info.

OK.

> > +     /* Wrap accessors with Runtime PM-aware ops */
> > +     bitbang->read = mdp->mii_bus->read;
> > +     bitbang->write = mdp->mii_bus->write;
> > +     mdp->mii_bus->read = sh_mdiobb_read;
> > +     mdp->mii_bus->write = sh_mdiobb_write;
>
> I did wonder about just exporting the two functions so you can
> directly call them.

I did consider that. Do you prefer exporting the functions?

> Otherwise, this looks good.

Thanks. Do you want me to submit (with the above changed) as an interim
solution?

Note that the same issue seems to be present in the Renesas EtherAVB
driver.  But that is more difficult to reproduce, as I don't have any arm32
boards that use RAVB, and on arm64 register access while a device is
suspended doesn't cause a crash, but continues silently without any effect.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds