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
