Re: [net-next v42] mctp pcc: Implement MCTP over PCC Transport

[Adam Young]

I had just hit reesend on the older version of the patch when this came 
in.  Apologies for the churn.

On 5/19/26 00:06, Jeremy Kerr wrote:
> Hi Adam,
>
> > diff --git a/drivers/net/ethernet/intel/stYNFrCT b/drivers/net/ethernet/intel/stYNFrCT
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..9b01d1e909f20fe037eea685116d344a2165db52
> > GIT binary patch
> > literal 564073
> [1255 lines of b85 data]
>
> I think something leaked into your commit?

Yeah, and I did post that I was going to resubmit without that file 
included.  I guess we got into a real-world race condition. Sorry about 
the churn.



> > +static void mctp_pcc_client_rx_callback(struct mbox_client *cl, void *mssg)
> > +{
> > +       struct acpi_pcct_ext_pcc_shared_memory pcc_header;
> > +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> > +       struct mctp_pcc_mailbox *inbox;
> > +       struct mctp_skb_cb *cb;
> > +       struct sk_buff *skb;
> > +       u32 header_length;
> > +       int size;
> > +
> > +       mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, inbox.client);
> > +       inbox = &mctp_pcc_ndev->inbox;
> > +       memcpy_fromio(&pcc_header, inbox->chan->shmem, sizeof(pcc_header));
> > +
> > +       // The message must at least have the PCC command indicating it is an MCTP
> > +       // message followed by the MCTP header, or we have a malformed message.
> > +       // This may be run on big endian system, but the data in the buffer is
> > +       // explicitly little endian.
> > +       header_length = le32_to_cpu(pcc_header.length);
> Is this still OK with pcc_header.length being a u32 (and not a __le32?)
I will remove.  No longer doing any endian conversions.
> > +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> > +{
> > +       struct acpi_pcct_ext_pcc_shared_memory *pcc_header;
> > +       struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
> > +       int len = skb->len;
> > +
> > +       if (skb_cow_head(skb, sizeof(*pcc_header)))
> > +               goto error;
> > +
> > +       pcc_header = skb_push(skb, sizeof(*pcc_header));
> > +       pcc_header->signature = PCC_SIGNATURE | mpnd->outbox.index;
> > +       pcc_header->flags = PCC_CMD_COMPLETION_NOTIFY;
> > +       memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
> > +       pcc_header->length = len + MCTP_SIGNATURE_LENGTH;
> You're doing the endian conversion on read, but not on write. What was
> the decision on whether you need it or not?

The conversion needs to go.  The way the acpica imported headers are 
written they assume machine architecture only.  Hence enforcing little 
endianess in the config.  I thought I had gotten rid of all the 
conversions.  I missed this one.


> > +
> > +       if (skb->len > mpnd->outbox.chan->shmem_size)
> > +               goto error;
> > +
> > +       if (mbox_send_message(mpnd->outbox.chan->mchan, skb) < 0) {
> > +               netif_stop_queue(ndev);
> > +               /*
> > +                * There is a possibility that the mailbox was cleared on
> > +                * another thread between the failed send attempt and
> > +                * stopping the queue.  If that is the case, and we don't restart
> > +                * the queue, it will remain permanently stopped.  To test,
> > +                * try submitting the message again. If successful, restart the
> > +                * queue.
> > +                */
> > +               if (mbox_send_message(mpnd->outbox.chan->mchan, skb) >= 0) {
> > +                       netif_wake_queue(ndev);
> The retry is a bit odd; you can simplify this race handling by doing the
> stop_queue pessimistically, and the re-starting the queue on success:
>
>      netif_stop_queue();
>      if (mbox_send_message(...) >= 0) {
>          netif_start_queue();
>      }
>
> I don't think you need a wake (vs start), since you're running in the
> same softirq context as the stop.

I like that.  It is a better approach.  I will incorporate into the next 
version.


> Cheers,
>
>
> Jeremy