| Message ID | 20240508183605.955341-1-alexander.sverdlin@siemens.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | net: ti: am65-cpsw-nuss: don't touch DMA after stop | expand |
Hi Alexander, On 08/05/2024 21:36, A. Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > Contrary to doc/develop/driver-model/ethernet.rst contract, eth_ops > .free_pkt can be called after .stop, there are several error paths in TFTP, > for instance: Doesn't this mean we need to fix TFTP instead of patching the Ethernet driver? I'm sure the issue is present for all Ethernet drivers as none of them are checking if Ethernet is stopped. Just that most of them don't print any error message so it goes unnoticed. > > eth_halt() <= tftp_handler() <= net_process_received_packet() <= eth_rx() > ... > am65_cpsw_free_pkt() <= eth_rx()> > Which results in (deliberately "tftpboot"ing non-existing file): > > TFTP error: 'File not found' (1) > Not retrying... > am65_cpsw_nuss_port ethernet@8000000port@1: RX dma free_pkt failed -22 > > Avoid the error message by checking that the interface is still not stopped > in am65_cpsw_free_pkt(). > > Fixes: 9d0dca1199d1 ("net: ethernet: ti: Introduce am654 gigabit eth switch subsystem driver") > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > --- > drivers/net/ti/am65-cpsw-nuss.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c > index 65ade1afd05..646f618afcf 100644 > --- a/drivers/net/ti/am65-cpsw-nuss.c > +++ b/drivers/net/ti/am65-cpsw-nuss.c > @@ -523,6 +523,9 @@ static int am65_cpsw_free_pkt(struct udevice *dev, uchar *packet, int length) > struct am65_cpsw_common *common = priv->cpsw_common; > int ret; > > + if (!common->started) > + return -ENETDOWN; > + > if (length > 0) { > u32 pkt = common->rx_next % UDMA_RX_DESC_NUM; >
Hi Roger! On Fri, 2024-05-10 at 12:11 +0300, Roger Quadros wrote: > > Contrary to doc/develop/driver-model/ethernet.rst contract, eth_ops > > .free_pkt can be called after .stop, there are several error paths in TFTP, > > for instance: > > Doesn't this mean we need to fix TFTP instead of patching the Ethernet > driver? I'm sure the issue is present for all Ethernet drivers as none > of them are checking if Ethernet is stopped. Just that most of them > don't print any error message so it goes unnoticed. Thanks for looking into this, your proposal makes sense to me! I'll rework! Best regards,
diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c index 65ade1afd05..646f618afcf 100644 --- a/drivers/net/ti/am65-cpsw-nuss.c +++ b/drivers/net/ti/am65-cpsw-nuss.c @@ -523,6 +523,9 @@ static int am65_cpsw_free_pkt(struct udevice *dev, uchar *packet, int length) struct am65_cpsw_common *common = priv->cpsw_common; int ret; + if (!common->started) + return -ENETDOWN; + if (length > 0) { u32 pkt = common->rx_next % UDMA_RX_DESC_NUM;