Hi Jonas,
On Sun, 6 Apr 2025 at 21:02, Jonas Karlman <jo...@kwiboo.se> wrote:
>
> Hi Simon,
>
> On 2025-04-06 00:12, Simon Glass wrote:
> > Several drivers make use of the designware Ethernet driver but do not
> > implement the remove() method. Add this so that DMA is stopped when the
> > OS is booted to avoid memory corruption, etc.
>
> The designware Ethernet driver core should not need the remove() ops as
> the eth_ops.stop() ops already should stop DMA. And the eth-uclass
> pre_remove() ops already call the eth_ops.stop() ops.
>
> All these drivers seem to use designware_eth_ops and thus have correct
> stop() ops configured and should really not need the remove() ops.
>
That's interesting. I wrote that code about 9 years ago so perhaps can
be forgiven for forgetting.
Very few drivers set the DM_FLAG_ACTIVE_DMA flag, certainly not the
designware ones. So how would they be removed?
> Side note, this also misses the gmac rockchip driver that uses its own
> eth_ops instead of designware_eth_ops, however it also use same stop()
> ops as all these drivers.
Hmm yes I saw that but then apparently forgot to include it.
I can perhaps redo this patch to add the DMA flag.
>
> >
> > Signed-off-by: Simon Glass <s...@chromium.org>
> > Reported-by: Christian Kohlschütter <christ...@kohlschutter.com>
> > ---
> >
> > drivers/net/designware.c | 2 +-
> > drivers/net/designware.h | 12 ++++++++++++
> > drivers/net/dwmac_meson8b.c | 6 ++++++
> > drivers/net/dwmac_s700.c | 6 ++++++
> > drivers/net/dwmac_socfpga.c | 6 ++++++
> > 5 files changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> > index eebf14bd51a..d4d31925fca 100644
> > --- a/drivers/net/designware.c
> > +++ b/drivers/net/designware.c
> > @@ -805,7 +805,7 @@ clk_err:
> > return err;
> > }
> >
> > -static int designware_eth_remove(struct udevice *dev)
> > +int designware_eth_remove(struct udevice *dev)
> > {
> > struct dw_eth_dev *priv = dev_get_priv(dev);
> >
> > diff --git a/drivers/net/designware.h b/drivers/net/designware.h
> > index e47101ccaf6..8c9d0190e03 100644
> > --- a/drivers/net/designware.h
> > +++ b/drivers/net/designware.h
> > @@ -247,6 +247,18 @@ struct dw_eth_dev {
> >
> > int designware_eth_of_to_plat(struct udevice *dev);
> > int designware_eth_probe(struct udevice *dev);
> > +
> > +/**
> > + * designware_eth_remove() - Remove the device
> > + *
> > + * Disables DMA and marks the device as remove. This must be called before
> > + * booting an OS, to ensure that DMA is inactive.
> > + *
> > + * @dev: Device to remove
> > + * Return 0 if OK, -ve on error
> > + */
> > +int designware_eth_remove(struct udevice *dev);
> > +
> > extern const struct eth_ops designware_eth_ops;
> >
> > struct dw_eth_pdata {
> > diff --git a/drivers/net/dwmac_meson8b.c b/drivers/net/dwmac_meson8b.c
> > index fde4aabbace..b2152f8da9b 100644
> > --- a/drivers/net/dwmac_meson8b.c
> > +++ b/drivers/net/dwmac_meson8b.c
> > @@ -145,6 +145,11 @@ static int dwmac_meson8b_probe(struct udevice *dev)
> > return designware_eth_probe(dev);
> > }
> >
> > +static int dwmac_meson8b_remove(struct udevice *dev)
> > +{
> > + return designware_eth_remove(dev);
> > +}
> > +
> > static const struct udevice_id dwmac_meson8b_ids[] = {
> > { .compatible = "amlogic,meson-gxbb-dwmac", .data =
> > (ulong)dwmac_setup_gx },
> > { .compatible = "amlogic,meson-g12a-dwmac", .data =
> > (ulong)dwmac_setup_axg },
> > @@ -158,6 +163,7 @@ U_BOOT_DRIVER(dwmac_meson8b) = {
> > .of_match = dwmac_meson8b_ids,
> > .of_to_plat = dwmac_meson8b_of_to_plat,
> > .probe = dwmac_meson8b_probe,
> > + .remove = dwmac_meson8b_remove,
>
> There is no reason to add the dwmac_meson8b_remove function above, you
> could just use designware_eth_remove directly here.
>
> Same for remaining drivers below.
Yes, I wasn't sure which way to go on that. I'll remove them.
>
> Regards,
> Jonas
>
> > .ops = &designware_eth_ops,
> > .priv_auto = sizeof(struct dw_eth_dev),
> > .plat_auto = sizeof(struct dwmac_meson8b_plat),
> > diff --git a/drivers/net/dwmac_s700.c b/drivers/net/dwmac_s700.c
> > index 969d247b4f3..cfb37c3aa71 100644
> > --- a/drivers/net/dwmac_s700.c
> > +++ b/drivers/net/dwmac_s700.c
> > @@ -44,6 +44,11 @@ static int dwmac_s700_probe(struct udevice *dev)
> > return designware_eth_probe(dev);
> > }
> >
> > +static int dwmac_s700_remove(struct udevice *dev)
> > +{
> > + return designware_eth_remove(dev);
> > +}
> > +
> > static int dwmac_s700_of_to_plat(struct udevice *dev)
> > {
> > return designware_eth_of_to_plat(dev);
> > @@ -60,6 +65,7 @@ U_BOOT_DRIVER(dwmac_s700) = {
> > .of_match = dwmac_s700_ids,
> > .of_to_plat = dwmac_s700_of_to_plat,
> > .probe = dwmac_s700_probe,
> > + .remove = dwmac_s700_remove,
> > .ops = &designware_eth_ops,
> > .priv_auto = sizeof(struct dw_eth_dev),
> > .plat_auto = sizeof(struct eth_pdata),
> > diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c
> > index a9e2d8c0972..8e7a9975d28 100644
> > --- a/drivers/net/dwmac_socfpga.c
> > +++ b/drivers/net/dwmac_socfpga.c
> > @@ -130,6 +130,11 @@ static int dwmac_socfpga_probe(struct udevice *dev)
> > return designware_eth_probe(dev);
> > }
> >
> > +static int dwmac_socfpga_remove(struct udevice *dev)
> > +{
> > + return designware_eth_remove(dev);
> > +}
> > +
> > static const struct udevice_id dwmac_socfpga_ids[] = {
> > { .compatible = "altr,socfpga-stmmac" },
> > { }
> > @@ -141,6 +146,7 @@ U_BOOT_DRIVER(dwmac_socfpga) = {
> > .of_match = dwmac_socfpga_ids,
> > .of_to_plat = dwmac_socfpga_of_to_plat,
> > .probe = dwmac_socfpga_probe,
> > + .remove = dwmac_socfpga_remove,
> > .ops = &designware_eth_ops,
> > .priv_auto = sizeof(struct dw_eth_dev),
> > .plat_auto = sizeof(struct dwmac_socfpga_plat),
>
Regards,
Simon
