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

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Adam,

Still some minor things, and locking query, inline.

You'll want to address the kernel-test-robot failure too; it looks like
the config it's hitting ends up without the acpi definitions available;
possibly CONFIG_ACPI is not set. Maybe you need a depends instead of a
select?

> diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
> index ce9d2d2ccf3b..ff4effd8e99c 100644
> --- a/drivers/net/mctp/Kconfig
> +++ b/drivers/net/mctp/Kconfig
> @@ -42,6 +42,19 @@ config MCTP_TRANSPORT_I3C
>           A MCTP protocol network device is created for each I3C bus
>           having a "mctp-controller" devicetree property.
>  
> +config MCTP_TRANSPORT_PCC
> +       tristate "MCTP  PCC transport"

Nit: you have two spaces there.

> +struct mctp_pcc_hdr {
> +       u32 signature;
> +       u32 flags;
> +       u32 length;
> +       char mctp_signature[4];
> +};

I see you've added the __le annotations that I mentioned, but to a
different patch in the series. Was that intentional?

> +
> +struct mctp_pcc_hw_addr {
> +       u32 inbox_index;
> +       u32 outbox_index;
> +};
> +
> +/* The netdev structure. One of these per PCC adapter. */
> +struct mctp_pcc_ndev {
> +       struct list_head next;

This is now unused.

> +       /* spinlock to serialize access to pcc buffer and registers*/
> +       spinlock_t lock;

The addition of the comment is good, but I'm still not clear on what
data this lock is serialising access to. Does "pcc buffer" refer to both
the pcc_comm_inbox_addr and pcc_comm_outbox_addr buffer?

If so, you may be missing locking on the client_rx_callback.

And what are the "registers" referring to here? everything seems to be
accessed through the mbox interface. Does that require serialisation?

(if you can list actual struct members that are only accessed under the
lock, that may make things more clear - but it can be a bit of a balance
in not going *too* overboard with the description!)

> +       struct mctp_dev mdev;
> +       struct acpi_device *acpi_device;
> +       struct pcc_mbox_chan *in_chan;
> +       struct pcc_mbox_chan *out_chan;
> +       struct mbox_client outbox_client;
> +       struct mbox_client inbox_client;
> +       void __iomem *pcc_comm_inbox_addr;
> +       void __iomem *pcc_comm_outbox_addr;
> +       struct mctp_pcc_hw_addr hw_addr;
> +};
> +
> +static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_dev;
> +       struct mctp_pcc_hdr mctp_pcc_hdr;
> +       struct mctp_skb_cb *cb;
> +       struct sk_buff *skb;
> +       void *skb_buf;
> +       u32 data_len;
> +
> +       mctp_pcc_dev = container_of(c, struct mctp_pcc_ndev, inbox_client);
> +       memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_dev->pcc_comm_inbox_addr,
> +                     sizeof(struct mctp_pcc_hdr));
> +       data_len = mctp_pcc_hdr.length + MCTP_HEADER_LENGTH;
> +
> +       if (data_len > mctp_pcc_dev->mdev.dev->max_mtu) {
> +               mctp_pcc_dev->mdev.dev->stats.rx_dropped++;
> +               return;
> +       }

Shouldn't this be comparing on the currently-set MTU, rather than the
max?

[in either case, this assumes that we want to enforce RX packets to be
within the transmit limit, which may be reasonable, but maybe not
strictly necessary]

> +
> +       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;
> +       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;
> +       netif_rx(skb);
> +}
> +
> +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;
> +
> +       ndev->stats.tx_bytes += skb->len;
> +       ndev->stats.tx_packets++;
> +       mpnd = netdev_priv(ndev);
> +
> +       spin_lock_irqsave(&mpnd->lock, flags);
> +       buffer = mpnd->pcc_comm_outbox_addr;
> +       pcc_header.signature = PCC_MAGIC | mpnd->hw_addr.outbox_index;
> +       pcc_header.flags = PCC_HEADER_FLAGS;
> +       memcpy(pcc_header.mctp_signature, MCTP_SIGNATURE, SIGNATURE_LENGTH);
> +       pcc_header.length = skb->len + SIGNATURE_LENGTH;

Are there any constraints on this length?

> +       memcpy_toio(buffer, &pcc_header, sizeof(struct mctp_pcc_hdr));
> +       memcpy_toio(buffer + sizeof(struct mctp_pcc_hdr), skb->data, skb->len);
> +       mpnd->out_chan->mchan->mbox->ops->send_data(mpnd->out_chan->mchan,
> +                                                   NULL);
> +       spin_unlock_irqrestore(&mpnd->lock, flags);
> +
> +       dev_consume_skb_any(skb);
> +       return NETDEV_TX_OK;
> +}
> +
> +static void
> +mctp_pcc_net_stats(struct net_device *net_dev,
> +                  struct rtnl_link_stats64 *stats)
> +{
> +       struct mctp_pcc_ndev *mpnd;
> +
> +       mpnd = netdev_priv(net_dev);
> +       stats->rx_errors = 0;
> +       stats->rx_packets = mpnd->mdev.dev->stats.rx_packets;
> +       stats->tx_packets = mpnd->mdev.dev->stats.tx_packets;
> +       stats->rx_dropped = 0;
> +       stats->tx_bytes = mpnd->mdev.dev->stats.tx_bytes;
> +       stats->rx_bytes = mpnd->mdev.dev->stats.rx_bytes;

Isn't mpnd->mdev.dev just net_dev?

Should we be doing this (as well as the stats updates) with mpnd->lock
held? 

> +static void mctp_pcc_driver_remove(struct acpi_device *adev)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_ndev = acpi_driver_data(adev);
> +
> +       pcc_mbox_free_channel(mctp_pcc_ndev->out_chan);
> +       pcc_mbox_free_channel(mctp_pcc_ndev->in_chan);
> +       mctp_unregister_netdev(mctp_pcc_ndev->mdev.dev);
> +}

Nice!

Cheers,


Jeremy