Hi Andrew, On 14/09/23 6:48 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> +static void oa_tc6_rx_eth_ready(struct oa_tc6 *tc6) >> +{ >> + struct sk_buff *skb = NULL; >> + >> + /* Send the received ethernet packet to network layer */ >> + skb = netdev_alloc_skb(tc6->netdev, tc6->rxd_bytes + NET_IP_ALIGN); >> + if (!skb) { >> + tc6->netdev->stats.rx_dropped++; >> + netdev_err(tc6->netdev, "Out of memory for rx'd frame"); > > Being out of memory is not likely to go away quickly. So i can see > this spamming the kernel log. At minimum make it rate limited, or just > rely on the counter and do not print anything. Ok then I will use rate limited print like below, if (printk_ratelimit()) netdev_err(tc6->netdev, "Out of memory for rx'd frame"); > >> +static int oa_tc6_process_exst(struct oa_tc6 *tc6) >> +{ >> + u32 regval; >> + int ret; >> + >> + ret = oa_tc6_read_register(tc6, OA_TC6_STS0, ®val, 1); >> + if (ret) { >> + netdev_err(tc6->netdev, "STS0 register read failed.\n"); >> + return ret; >> + } >> + if (regval & TXPE) >> + netdev_err(tc6->netdev, "Transmit protocol error\n"); >> + if (regval & TXBOE) >> + netdev_err(tc6->netdev, "Transmit buffer overflow\n"); >> + if (regval & TXBUE) >> + netdev_err(tc6->netdev, "Transmit buffer underflow\n"); >> + if (regval & RXBOE) >> + netdev_err(tc6->netdev, "Receive buffer overflow\n"); >> + if (regval & LOFE) >> + netdev_err(tc6->netdev, "Loss of frame\n"); >> + if (regval & HDRE) >> + netdev_err(tc6->netdev, "Header error\n"); >> + if (regval & TXFCSE) >> + netdev_err(tc6->netdev, "Transmit Frame Check Sequence Error\n"); > > Do you expect these problems to magically fix themselves, or is this > going to spam the kernel log until the machine is rebooted? Yes, with the current implementation the system need to be rebooted. I will keep a rate limited print for the user reference so that it will not spam the kernel. > > It seems a counter would be more appropriate, and maybe one rate > limited message if the problem persists. I prefer a rate limited print. > > Please look at all your netdev_err() calls and consider if they are > really needed, should they be netdev_dbg(), or statistics counters. Sure, I will fix all the prints in the next version with the required approach. > >> +static int oa_tc6_process_rx_chunks(struct oa_tc6 *tc6, u8 *buf, u16 len) >> +{ >> + u8 cp_count; >> + u8 *payload; >> + u32 ftr; >> + u16 ebo; >> + u16 sbo; >> + >> + /* Calculate the number of chunks received */ >> + cp_count = len / (tc6->cps + TC6_FTR_SIZE); >> + >> + for (u8 i = 0; i < cp_count; i++) { >> + /* Get the footer and payload */ >> + ftr = *(u32 *)&buf[tc6->cps + (i * (tc6->cps + TC6_FTR_SIZE))]; >> + ftr = be32_to_cpu(ftr); >> + payload = &buf[(i * (tc6->cps + TC6_FTR_SIZE))]; >> + /* Check for footer parity error */ >> + if (oa_tc6_get_parity(ftr)) { >> + netdev_err(tc6->netdev, "Footer: Parity error\n"); >> + goto err_exit; >> + } >> + /* If EXST set in the footer then read STS0 register to get the >> + * status information. >> + */ >> + if (FIELD_GET(DATA_FTR_EXST, ftr)) { >> + if (oa_tc6_process_exst(tc6)) >> + netdev_err(tc6->netdev, "Failed to process EXST\n"); >> + goto err_exit; >> + } >> + if (FIELD_GET(DATA_FTR_HDRB, ftr)) { >> + netdev_err(tc6->netdev, "Footer: Received header bad\n"); >> + goto err_exit; >> + } >> + if (!FIELD_GET(DATA_FTR_SYNC, ftr)) { >> + netdev_err(tc6->netdev, "Footer: Configuration unsync\n"); >> + goto err_exit; >> + } >> + /* If Frame Drop is set, indicates that the MAC has detected a >> + * condition for which the SPI host should drop the received >> + * ethernet frame. >> + */ >> + if (FIELD_GET(DATA_FTR_FD, ftr) && FIELD_GET(DATA_FTR_EV, ftr)) { >> + netdev_warn(tc6->netdev, "Footer: Frame drop\n"); >> + if (FIELD_GET(DATA_FTR_SV, ftr)) { >> + goto start_new_frame; >> + } else { >> + if (tc6->rx_eth_started) { >> + tc6->rxd_bytes = 0; >> + tc6->rx_eth_started = false; >> + tc6->netdev->stats.rx_dropped++; >> + } >> + continue; >> + } >> + } >> + /* Check for data valid */ >> + if (FIELD_GET(DATA_FTR_DV, ftr)) { >> + /* Check whether both start valid and end valid are in a >> + * single chunk payload means a single chunk payload may >> + * contain an entire ethernet frame. >> + */ >> + if (FIELD_GET(DATA_FTR_SV, ftr) && >> + FIELD_GET(DATA_FTR_EV, ftr)) { >> + sbo = FIELD_GET(DATA_FTR_SWO, ftr) * 4; >> + ebo = FIELD_GET(DATA_FTR_EBO, ftr) + 1; >> + if (ebo <= sbo) { >> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], >> + &payload[0], ebo); >> + tc6->rxd_bytes += ebo; >> + oa_tc6_rx_eth_ready(tc6); >> + tc6->rxd_bytes = 0; >> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], >> + &payload[sbo], tc6->cps - sbo); >> + tc6->rxd_bytes += (tc6->cps - sbo); >> + goto exit; >> + } else { >> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], >> + &payload[sbo], ebo - sbo); >> + tc6->rxd_bytes += (ebo - sbo); >> + oa_tc6_rx_eth_ready(tc6); >> + tc6->rxd_bytes = 0; >> + goto exit; >> + } >> + } >> +start_new_frame: >> + /* Check for start valid to start capturing the incoming >> + * ethernet frame. >> + */ >> + if (FIELD_GET(DATA_FTR_SV, ftr) && !tc6->rx_eth_started) { >> + tc6->rxd_bytes = 0; >> + tc6->rx_eth_started = true; >> + sbo = FIELD_GET(DATA_FTR_SWO, ftr) * 4; >> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], >> + &payload[sbo], tc6->cps - sbo); >> + tc6->rxd_bytes += (tc6->cps - sbo); >> + goto exit; >> + } >> + >> + /* Check for end valid and calculate the copy length */ >> + if (tc6->rx_eth_started) { >> + if (FIELD_GET(DATA_FTR_EV, ftr)) >> + ebo = FIELD_GET(DATA_FTR_EBO, ftr) + 1; >> + else >> + ebo = tc6->cps; >> + >> + memcpy(&tc6->eth_rx_buf[tc6->rxd_bytes], >> + &payload[0], ebo); >> + tc6->rxd_bytes += ebo; >> + if (FIELD_GET(DATA_FTR_EV, ftr)) { >> + /* If End Valid set then send the >> + * received ethernet frame to n/w. >> + */ >> + oa_tc6_rx_eth_ready(tc6); >> + tc6->rxd_bytes = 0; >> + tc6->rx_eth_started = false; >> + } >> + } >> + } >> + >> +exit: >> + tc6->txc = FIELD_GET(DATA_FTR_TXC, ftr); >> + tc6->rca = FIELD_GET(DATA_FTR_RCA, ftr); >> + } >> + return FTR_OK; >> + >> +err_exit: >> + if (tc6->rx_eth_started) { >> + tc6->rxd_bytes = 0; >> + tc6->rx_eth_started = false; >> + tc6->netdev->stats.rx_dropped++; >> + } >> + return FTR_ERR; >> +} > > This is quite a complex function, with a lot of gotos. Please try to > split it up into helpers. Yes sure, will do it. > >> + >> static int oa_tc6_handler(void *data) >> { >> struct oa_tc6 *tc6 = data; >> + bool txc_wait = false; >> + u16 tx_pos = 0; >> u32 regval; >> + u16 len; >> int ret; >> >> while (likely(!kthread_should_stop())) { >> - wait_event_interruptible(tc6->tc6_wq, tc6->int_flag || >> + wait_event_interruptible(tc6->tc6_wq, tc6->tx_flag || >> + tc6->int_flag || tc6->rca || >> kthread_should_stop()); >> - if (tc6->int_flag) { >> + if (tc6->int_flag && !tc6->reset) { >> tc6->int_flag = false; >> + tc6->reset = true; >> ret = oa_tc6_perform_ctrl(tc6, OA_TC6_STS0, ®val, 1, >> false, false); >> if (ret) { >> @@ -227,10 +435,170 @@ static int oa_tc6_handler(void *data) >> complete(&tc6->rst_complete); >> } >> } >> + >> + if (tc6->int_flag || tc6->rca) { >> + /* If rca is updated from the previous footer then >> + * prepare the empty chunks equal to rca and perform >> + * SPI transfer to receive the ethernet frame. >> + */ >> + if (tc6->rca) { >> + len = oa_tc6_prepare_empty_chunk(tc6, >> + tc6->spi_tx_buf, >> + tc6->rca); >> + } else { >> + /* If there is an interrupt then perform a SPI >> + * transfer with a empty chunk to get the >> + * details. >> + */ >> + tc6->int_flag = false; >> + len = oa_tc6_prepare_empty_chunk(tc6, >> + tc6->spi_tx_buf, >> + 1); >> + } >> + /* Perform SPI transfer */ >> + ret = oa_tc6_spi_transfer(tc6->spi, tc6->spi_tx_buf, >> + tc6->spi_rx_buf, len); >> + if (ret) { >> + netdev_err(tc6->netdev, "SPI transfer failed\n"); >> + continue; >> + } >> + /* Process the received chunks to get the ethernet frame >> + * or interrupt details. >> + */ >> + if (oa_tc6_process_rx_chunks(tc6, tc6->spi_rx_buf, len)) >> + continue; >> + } >> + >> + /* If there is a tx ethernet frame available */ >> + if (tc6->tx_flag || txc_wait) { >> + tc6->tx_flag = false; >> + txc_wait = false; >> + len = 0; >> + if (!tc6->txc) { >> + /* If there is no txc available to transport the >> + * tx ethernet frames then wait for the MAC-PHY >> + * interrupt to get the txc availability. >> + */ >> + txc_wait = true; >> + continue; >> + } else if (tc6->txc >= tc6->txc_needed) { >> + len = tc6->txc_needed * (tc6->cps + TC6_HDR_SIZE); >> + } else { >> + len = tc6->txc * (tc6->cps + TC6_HDR_SIZE); >> + } >> + memcpy(&tc6->spi_tx_buf[0], &tc6->eth_tx_buf[tx_pos], >> + len); >> + ret = oa_tc6_spi_transfer(tc6->spi, tc6->spi_tx_buf, >> + tc6->spi_rx_buf, len); >> + if (ret) { >> + netdev_err(tc6->netdev, "SPI transfer failed\n"); >> + continue; >> + } >> + /* Process the received chunks to get the ethernet frame >> + * or status. >> + */ >> + if (oa_tc6_process_rx_chunks(tc6, tc6->spi_rx_buf, >> + len)) { >> + /* In case of error while processing rx chunks >> + * discard the incomplete tx ethernet frame and >> + * resend it. >> + */ >> + tx_pos = 0; >> + tc6->txc_needed = tc6->total_txc_needed; >> + } else { >> + tx_pos += len; >> + tc6->txc_needed = tc6->txc_needed - >> + (len / (tc6->cps + TC6_HDR_SIZE)); >> + /* If the complete ethernet frame is transmitted >> + * then return the skb and update the details to >> + * n/w layer. >> + */ >> + if (!tc6->txc_needed) { >> + tc6->netdev->stats.tx_packets++; >> + tc6->netdev->stats.tx_bytes += tc6->tx_skb->len; >> + dev_kfree_skb(tc6->tx_skb); >> + tx_pos = 0; >> + tc6->tx_skb = NULL; >> + if (netif_queue_stopped(tc6->netdev)) >> + netif_wake_queue(tc6->netdev); >> + } else if (tc6->txc) { >> + /* If txc is available again and updated >> + * from the previous footer then perform >> + * tx again. >> + */ >> + tc6->tx_flag = true; >> + } else { >> + /* If there is no txc then wait for the >> + * interrupt to indicate txc >> + * availability. >> + */ >> + txc_wait = true; >> + } >> + } >> + } >> } >> return 0; >> } > > This is also a huge function. The Linux coding style says: > > Functions should be short and sweet, and do just one thing. They > should fit on one or two screenfuls of text (the ISO/ANSI screen > size is 80x24, as we all know), and do one thing and do that > well. > Thanks for the info. Sure, will do it. > Please break this up into lots of smaller functions which do just one > thing. > >> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi) >> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev) >> { >> struct oa_tc6 *tc6; >> int ret; >> @@ -334,11 +710,39 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi) >> if (!spi) >> return NULL; >> >> + if (!netdev) >> + return NULL; > > Hos can this happen? Let is explode if some developer is dumb enough > to pass a NULL. > >> +#define MAX_ETH_LEN 1536 > > Where do 1536 come from? Maybe this needs an OA_TC6 prefix to make it > clear this is specific to this protocol? Ah it is a mistake. It is supposed to be an ethernet packet size which is 1518 (1500 bytes MTU size + 18 bytes overhead) and it is not from OA. It is a mistake and will correct it in the next version. Best Regards, Parthiban V > > Andrew