Hi Jonas, On Mon, 7 Apr 2025 at 09:36, Jonas Karlman <jo...@kwiboo.se> wrote: > > 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?
I suppose so, yes. > > 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. But not all network drivers use DMA. It should really be set by the driver. > > There is probably no need to add all these remove() ops, as they would > still not have been called by dm_remove_devices_active(). Well, since we can rely on the eth uclass, that is correct, but I still think it is better to add it, as an example to other drivers. Regards, Simon
