Hi Simon,
On 2025-04-06 23:10, Simon Glass wrote:
> 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?
Ahh, I see, remove() is only called for devices with ACTIVE_DMA or
OS_PREPARE. So I guess that was probably the issue all along?
There is some confusion that dm_remove_devices_active() does not remove
device_active()=true devices and instead only remove ACTIVE_DMA and/or
OS_PREPARE flagged devices.
>
>> 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.
Maybe eth_post_probe() or similar should add the OS_PREPARE or
ACTIVE_DMA flag for all eth-uclass, to ensure remove() and thus stop()
gets called when dm_remove_devices_active() is called.
There is probably no need to add all these remove() ops, as they would
still not have been called by dm_remove_devices_active().
Regards,
Jonas
>
>>
>>>
>>> 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
