| 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
| ||
|
Date: Tue, 28 May 2024 19:45:57 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: admiyo@...amperecomputing.com
Cc: Jeremy Kerr <jk@...econstruct.com.au>, Matt Johnston
<matt@...econstruct.com.au>, "David S. Miller" <davem@...emloft.net>, Eric
Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport
On Tue, 28 May 2024 15:18:23 -0400 admiyo@...amperecomputing.com wrote:
> From: Adam Young <admiyo@...erecomputing.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.
Missing your SoB, but please wait for more feedback before reposting.
> +#include <net/pkt_sched.h>
Hm, what do you need this include for?
> +#define SPDM_VERSION_OFFSET 1
> +#define SPDM_REQ_RESP_OFFSET 2
> +#define MCTP_PAYLOAD_LENGTH 256
> +#define MCTP_CMD_LENGTH 4
> +#define MCTP_PCC_VERSION 0x1 /* DSP0253 defines a single version: 1 */
> +#define MCTP_SIGNATURE "MCTP"
> +#define SIGNATURE_LENGTH 4
> +#define MCTP_HEADER_LENGTH 12
> +#define MCTP_MIN_MTU 68
> +#define PCC_MAGIC 0x50434300
> +#define PCC_DWORD_TYPE 0x0c
Could you align the values using tabs?
> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_dev;
> + struct mctp_skb_cb *cb;
> + struct sk_buff *skb;
> + u32 length_offset;
> + u32 flags_offset;
> + void *skb_buf;
> + u32 data_len;
> + u32 flags;
> +
> + mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
> + length_offset = offsetof(struct mctp_pcc_hdr, length);
> + data_len = readl(mctp_pcc_dev->pcc_comm_inbox_addr + length_offset) +
> + 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;
> + }
> + mctp_pcc_dev->mdev.dev->stats.rx_packets++;
> + mctp_pcc_dev->mdev.dev->stats.rx_bytes += data_len;
Please implement ndo_get_stats64, use of the core dev stats in drivers
is deprecated:
* @stats: Statistics struct, which was left as a legacy, use
* rtnl_link_stats64 instead
> + skb->protocol = htons(ETH_P_MCTP);
> + skb_buf = skb_put(skb, data_len);
> + memcpy_fromio(skb_buf, mctp_pcc_dev->pcc_comm_inbox_addr, 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;
netdev_alloc_skb() already sets dev
> + netif_rx(skb);
> +
> + flags_offset = offsetof(struct mctp_pcc_hdr, flags);
> + flags = readl(mctp_pcc_dev->pcc_comm_inbox_addr + flags_offset);
> + mctp_pcc_dev->in_chan->ack_rx = (flags & 1) > 0;
> +}
> +
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> + struct mctp_pcc_hdr pcc_header;
> + struct mctp_pcc_ndev *mpnd;
> + void __iomem *buffer;
> + unsigned long flags;
> + int rc;
> +
> + netif_stop_queue(ndev);
Why?
> + ndev->stats.tx_bytes += skb->len;
> + ndev->stats.tx_packets++;
> + mpnd = (struct mctp_pcc_ndev *)netdev_priv(ndev);
> +
> + spin_lock_irqsave(&mpnd->lock, flags);
> + buffer = mpnd->pcc_comm_outbox_addr;
> + pcc_header.signature = PCC_MAGIC;
> + pcc_header.flags = 0x1;
> + memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH);
> + pcc_header.length = skb->len + SIGNATURE_LENGTH;
> + memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr));
> + memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len);
> + rc = mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
> + NULL);
> + 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;
> +}
> +
> +static const struct net_device_ops mctp_pcc_netdev_ops = {
> + .ndo_start_xmit = mctp_pcc_tx,
> + .ndo_uninit = NULL
No need to init things to NULL
> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_dev = NULL;
> + struct list_head *ptr;
> + struct list_head *tmp;
> +
> + list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
> + struct net_device *ndev;
> +
> + mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, next);
> + if (adev && mctp_pcc_dev->acpi_device == adev)
> + continue;
> +
> + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
> + mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
> + ndev = mctp_pcc_dev->mdev.dev;
> + if (ndev)
> + mctp_unregister_netdev(ndev);
> + list_del(ptr);
> + if (adev)
> + break;
> + }
> +};
spurious ;
> + .owner = THIS_MODULE,
> +
suprious new line
> +};
> +
--
pw-bot: cr
Powered by blists - more mailing lists