| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <021df8b3-bf89-42a8-9f83-f3c935dd9364@microchip.com>
Date: Tue, 31 Oct 2023 08:26:29 +0000
From: <Parthiban.Veerasooran@...rochip.com>
To: <andrew@...n.ch>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
<pabeni@...hat.com>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<corbet@....net>, <Steen.Hegelund@...rochip.com>,
<rdunlap@...radead.org>, <horms@...nel.org>,
<casper.casan@...il.com>, <netdev@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-doc@...r.kernel.org>, <Horatiu.Vultur@...rochip.com>,
<Woojung.Huh@...rochip.com>, <Nicolas.Ferre@...rochip.com>,
<UNGLinuxDriver@...rochip.com>, <Thorsten.Kummermehr@...rochip.com>
Subject: Re: [PATCH net-next v2 7/9] net: ethernet: oa_tc6: implement data
transaction interface
Hi Andrew,
On 24/10/23 7:37 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +static u16 oa_tc6_prepare_empty_chunk(struct oa_tc6 *tc6, u8 *buf, u8 cp_count)
>> +{
>> + u32 hdr;
>> +
>> + /* Prepare empty chunks used for getting interrupt information or if
>> + * receive data available.
>> + */
>> + for (u8 i = 0; i < cp_count; i++) {
>> + hdr = FIELD_PREP(DATA_HDR_DNC, 1);
>> + hdr |= FIELD_PREP(DATA_HDR_P, oa_tc6_get_parity(hdr));
>> + *(__be32 *)&buf[i * (tc6->cps + TC6_HDR_SIZE)] = cpu_to_be32(hdr);
>> + memset(&buf[TC6_HDR_SIZE + (i * (tc6->cps + TC6_HDR_SIZE))], 0,
>> + tc6->cps);
>> + }
>
> This is not simple, and its the sort of code which makes me wounder if
> its gone off the end of the buffer. It would be good to find somebody
> internally within Microchip to review this code.
I think, I don't need to do memset here as the header itself doesn't
describe any valid information about the payload except data not control
as it is an empty chunk and no matter what the payload contains. Apart
from that I don't get what's wrong here, anyway I will ask our internal
reviewers to review the code.
>
>> +static void oa_tc6_rx_eth_ready(struct oa_tc6 *tc6)
>> +{
>> + struct sk_buff *skb;
>> +
>> + /* 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_dbg(tc6->netdev, "Out of memory for rx'd frame");
>
> You can just return here, and skip the else. Less indentation is
> better, it generally makes the code more readable.
Ah yes. Noted.
>
>> + } else {
>> + skb_reserve(skb, NET_IP_ALIGN);
>> + memcpy(skb_put(skb, tc6->rxd_bytes), &tc6->eth_rx_buf[0],
>> + tc6->rxd_bytes);
>> + skb->protocol = eth_type_trans(skb, tc6->netdev);
>> + tc6->netdev->stats.rx_packets++;
>> + tc6->netdev->stats.rx_bytes += tc6->rxd_bytes;
>> + /* 0 for NET_RX_SUCCESS and 1 for NET_RX_DROP */
>> + if (netif_rx(skb))
>> + tc6->netdev->stats.rx_dropped++;
>
> Rather than have a comment do:
Ah ok, will do it.
>
> if (netif_rx(skb) == NET_RX_DROP)
> tc6->netdev->stats.rx_dropped++;
>
>
>> +static void oa_tc6_rx_eth_complete2(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
>> +{
>> + u16 ebo;
>
> What does ftr and ebo mean? Its really hard to read this code because
> the names are not really meaningful.
Ok, ftr -> footer and ebo -> end_byte_offset. Will update in the next
revision.
>
>> +
>> + 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 EV 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;
>> + }
>> +}
>> +
>> +static void oa_tc6_rx_eth_complete1(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
>> +{
>> + u16 ebo;
>> + u16 sbo;
>> +
>> + 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);
>> + } 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;
>> + }
>> +}
>> +
>> +static void oa_tc6_start_rx_eth(struct oa_tc6 *tc6, u8 *payload, u32 ftr)
>> +{
>> + u16 sbo;
>> +
>> + 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);
>> +}
>> +
>> +static u32 oa_tc6_get_footer(struct oa_tc6 *tc6, u8 *buf, u8 cp_num)
>> +{
>> + __be32 ftr;
>> +
>> + ftr = *(__be32 *)&buf[tc6->cps + (cp_num * (tc6->cps + TC6_FTR_SIZE))];
>> +
>> + return be32_to_cpu(ftr);
>> +}
>> +
>> +static void oa_tc6_update_txc_rca(struct oa_tc6 *tc6, u32 ftr)
>> +{
>> + tc6->txc = FIELD_GET(DATA_FTR_TXC, ftr);
>> + tc6->rca = FIELD_GET(DATA_FTR_RCA, ftr);
>> +}
>> +
>> +static int oa_tc6_check_ftr_errors(struct oa_tc6 *tc6, u32 ftr)
>> +{
>> + /* Check for footer parity error */
>> + if (oa_tc6_get_parity(ftr)) {
>> + net_err_ratelimited("%s: Footer parity error\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + /* 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))
>> + net_err_ratelimited("%s: Failed to process EXST\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + if (FIELD_GET(DATA_FTR_HDRB, ftr)) {
>> + net_err_ratelimited("%s: Footer eeceived header bad\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + if (!FIELD_GET(DATA_FTR_SYNC, ftr)) {
>> + net_err_ratelimited("%s: Footer configuration unsync\n",
>> + tc6->netdev->name);
>> + return FTR_ERR;
>> + }
>> + return FTR_OK;
>> +}
>> +
>> +static void oa_tc6_drop_rx_eth(struct oa_tc6 *tc6)
>> +{
>> + tc6->rxd_bytes = 0;
>> + tc6->rx_eth_started = false;
>> + tc6->netdev->stats.rx_dropped++;
>> + net_err_ratelimited("%s: Footer frame drop\n",
>> + tc6->netdev->name);
>> +}
>> +
>> +static int oa_tc6_process_rx_chunks(struct oa_tc6 *tc6, u8 *buf, u16 len)
>> +{
>> + u8 cp_count;
>> + u8 *payload;
>> + u32 ftr;
>> + int ret;
>> +
>> + /* 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 = oa_tc6_get_footer(tc6, buf, i);
>> + payload = &buf[(i * (tc6->cps + TC6_FTR_SIZE))];
>
> This would be more readable:
>
> /* Calculate the number of chunks received */
> chunks = len / (tc6->cps + TC6_FTR_SIZE);
>
> for (u8 chunk = 0; chunk < chunks; chunk++) {
> /* Get the footer and payload */
> ftr = oa_tc6_get_footer(tc6, buf, chunk);
> payload = &buf[(chunk * (tc6->cps + TC6_FTR_SIZE))];
>
> etc.
Ok thanks for the input. Will do it.
>
> And maybe move most of this code into a function
> oa_tc6_process_rx_chunk(). With lots of small functions with good
> names, you need less comments.
Yes sure. Will do it.
>
>
>> + /* 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)) {
>
>
> if (FIELD_GET(DATA_FOOTER_START_VALID, footer) &&
> FIELD_GET(DATA_FOOTER_END_VALID, footer)) {
>
> Don't you think that is more readable?
Yes it is more readable now. Will update in the next revision.
>
>> +static void oa_tc6_prepare_tx_chunks(struct oa_tc6 *tc6, u8 *buf,
>> + struct sk_buff *skb)
>> +{
>> + bool frame_started = false;
>> + u16 copied_bytes = 0;
>> + u16 copy_len;
>> + u32 hdr;
>> +
>> + /* Calculate the number tx credit counts needed to transport the tx
>> + * ethernet frame.
>> + */
>> + tc6->txc_needed = (skb->len / tc6->cps) + ((skb->len % tc6->cps) ? 1 : 0);
>
> Why call it a credit here, but a chunk when receiving?
I named as the tx path always gives the number of tx credits counts can
be enqueued for transfer. So used txc needed name to represent. Ok I
will change it as chunks_needed.
>
>> +static int oa_tc6_perform_spi_xfer(struct oa_tc6 *tc6)
>> +{
>> + bool do_tx_again;
>> + u16 total_len;
>> + u16 rca_len;
>> + u16 tx_len;
>> + int ret;
>> +
>> + do {
>> + do_tx_again = false;
>> + rca_len = 0;
>> + tx_len = 0;
>> +
>> + /* In case of an interrupt, perform an empty chunk transfer to
>> + * know the purpose of the interrupt. Interrupt may occur in
>> + * case of RCA (Receive Chunk Available) and TXC (Transmit
>> + * Credit Count). Both will occur if they are not indicated
>> + * through the previous footer.
>> + */
>> + if (tc6->int_flag) {
>> + tc6->int_flag = false;
>> + total_len = oa_tc6_prepare_empty_chunk(tc6,
>> + tc6->spi_tx_buf,
>> + 1);
>> + } else {
>> + /* Calculate the transfer length */
>> + if (tc6->tx_flag && tc6->txc) {
>> + tx_len = oa_tc6_calculate_tx_len(tc6);
>> + memcpy(&tc6->spi_tx_buf[0],
>> + &tc6->eth_tx_buf[tc6->tx_pos], tx_len);
>> + }
>> +
>> + if (tc6->rca)
>> + rca_len = oa_tc6_calculate_rca_len(tc6, tx_len);
>> +
>> + total_len = tx_len + rca_len;
>> + }
>> + ret = oa_tc6_spi_transfer(tc6->spi, tc6->spi_tx_buf,
>> + tc6->spi_rx_buf, total_len);
>> + if (ret)
>> + return ret;
>> + /* Process the rxd chunks to get the ethernet frame or status */
>> + ret = oa_tc6_process_rx_chunks(tc6, tc6->spi_rx_buf, total_len);
>> + if (ret)
>> + return ret;
>> + if (tc6->tx_flag) {
>> + tc6->tx_pos += tx_len;
>> + tc6->txc_needed = tc6->txc_needed -
>> + (tx_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)
>> + oa_tc6_tx_eth_complete(tc6);
>> + else if (tc6->txc)
>> + /* If txc is available again and updated from
>> + * the previous footer then perform tx again.
>> + */
>> + do_tx_again = true;
>> + }
>> +
>> + /* If rca is updated from the previous footer then perform empty
>> + * tx to receive ethernet frame.
>> + */
>> + if (tc6->rca)
>> + do_tx_again = true;
>> + } while (do_tx_again);
>
> The coding standard say:
>
> 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.
>
> This is too long, and does too many things.
Sure, will split into multiple small functions.
Best Regards,
Parthiban V
>
> Andrew
Powered by blists - more mailing lists