> +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.
> +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?
It seems a counter would be more appropriate, and maybe one rate
limited message if the problem persists.
Please look at all your netdev_err() calls and consider if they are
really needed, should they be netdev_dbg(), or statistics counters.
> +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.
> +
> 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.
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?
Andrew
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
> >> +#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.
Please try to express this using ETH_DATA_LEN + sizeof(struct
oa_tc6_overhead). Doing it like this will avoid errors like this since
it is also part documentation.
Andrew
Hi Andrew,
On 18/09/23 6:31 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>>>> +#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.
>
> Please try to express this using ETH_DATA_LEN + sizeof(struct
> oa_tc6_overhead). Doing it like this will avoid errors like this since
> it is also part documentation.
Ok, in my case the define would be,
#define MAX_ETH_LEN (ETH_DATA_LEN + ETH_HLEN + ETH_FCS_LEN)
Best Regards,
Parthiban V
>
> Andrew