Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

Simon Horman
2024-05-13T18:31:16Z

On Mon, May 13, 2024 at 01:35:44PM -0400, admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@os.amperecomputing.com>
>
> Implementation of DMTF DSP:0292
> Management Control Transport Protocol(MCTP) over
> Platform Communication Channel(PCC)
>
> MCTP devices are specified by entries in DSDT/SDST and
> reference channels specified in the PCCT.
>
> Communication with other devices use the PCC based
> doorbell mechanism.
>
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>

Hi Adam,

Some minor feedback from my side.

...

> +static struct mctp_pcc_packet *mctp_pcc_extract_data(struct sk_buff *old_skb,
> + void *buffer, int outbox_index)
> +{
> + struct mctp_pcc_packet *mpp;
> +
> + mpp = buffer;
> + writel(PCC_MAGIC | outbox_index, &mpp->pcc_header.signature);
> + writel(0x1, &mpp->pcc_header.flags);
> + memcpy_toio(mpp->pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH);
> + writel(old_skb->len + SIGNATURE_LENGTH, &mpp->pcc_header.length);
> + memcpy_toio(mpp->header_data, old_skb->data, old_skb->len);
> + return mpp;
> +}
> +
> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *)

Please include a name for all function parameters.

Flagged by W=1 builds.

> +{
> + struct sk_buff *skb;
> + struct mctp_pcc_packet *mpp;
> + struct mctp_skb_cb *cb;
> + int data_len;
> + unsigned long buf_ptr_val;

buf_ptr_val is assigned but otherwise unused.

Flagged by W=1 builds.

> + struct mctp_pcc_ndev *mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
> + void *skb_buf;

For Networking code please consider:

1. Using reverse xmas tree order - longest line to shortest - for local
variable declarations.

This tool can be of assistance: https://github.com/ecree-solarflare/xmastree

2. Restricting lines to 80 columns wide where this can be trivially achieved.

./scripts/checkpatch.pl --max-line-length=80

In this case, perhaps:

struct mctp_pcc_ndev *mctp_pcc_dev;
struct mctp_pcc_packet *mpp;
struct mctp_skb_cb *cb;
struct sk_buff *skb;
void *skb_buf;
int data_len;

mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);

> +
> + mpp = (struct mctp_pcc_packet *)mctp_pcc_dev->pcc_comm_inbox_addr;
> + buf_ptr_val = (unsigned long)mpp;
> + data_len = readl(&mpp->pcc_header.length) + MCTP_HEADER_LENGTH;
> + skb = netdev_alloc_skb(mctp_pcc_dev->mdev.dev, data_len);
> + if (!skb) {
> + mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
> + return;
> + }
> + skb->protocol = htons(ETH_P_MCTP);
> + skb_buf = skb_put(skb, data_len);
> + memcpy_fromio(skb_buf, mpp, data_len);
> + skb_reset_mac_header(skb);
> + skb_pull(skb, sizeof(struct mctp_pcc_hdr));
> + skb_reset_network_header(skb);
> + cb = __mctp_cb(skb);
> + cb->halen = 0;
> + skb->dev = mctp_pcc_dev->mdev.dev;
> + netif_rx(skb);
> +}
> +
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> + unsigned char *buffer;
> + struct mctp_pcc_ndev *mpnd;
> + struct mctp_pcc_packet *mpp;
> + unsigned long flags;
> + int rc;
> +
> + netif_stop_queue(ndev);
> + ndev->stats.tx_bytes += skb->len;
> + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev);
> + spin_lock_irqsave(&mpnd->lock, flags);
> + buffer = mpnd->pcc_comm_outbox_addr;

buffer is assigned but otherwise unused in this function.

Flagged by W=1 builds.

> + mpp = mctp_pcc_extract_data(skb, mpnd->pcc_comm_outbox_addr, mpnd->hw_addr.outbox_index);
> + rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan, mpp);
> + spin_unlock_irqrestore(&mpnd->lock, flags);
> +
> + dev_consume_skb_any(skb);
> + netif_start_queue(ndev);
> + if (!rc)
> + return NETDEV_TX_OK;
> + return NETDEV_TX_BUSY;
> +}

...

--
pw-bot: changes-requested