Re: [net-next v33 1/1] mctp pcc: Implement MCTP over PCC Transport

[Adam Young <admiyo@amperemail.onmicrosoft.com>]

On 3/17/26 00:51, Jeremy Kerr wrote:
> Hi Adam,
>
> Some comments inline.
>
>> +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;
>> +       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));
>> +       size = pcc_header.length - sizeof(u32);
> Since size is signed, this may be negative...

Which would be a sign that something is wrong, and thus that should be 
handled as well.  OK. Will do.


>
> (also, why sizeof(u32) here? from the spec, it seems like you're
> trimming the signature here, so MCTP_SIGNATURE_LENGTH would be more
> appropriate? or sizeof(pcc_header.command)?)

command is the right option.  I think I have just copied this around so 
many places where it is renamed that I mentally have just gone with the 
underlying size, but explicitly it is the command size.



>
>> +
>> +       if (size == 0)
>> +               return;
> ... which won't get picked up here
>
>> +
>> +       if (strncmp((unsigned char *)&pcc_header.command, MCTP_SIGNATURE, 4) != 0) {
>> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
>> +               return;
>> +       }
> We shouldn't really be treating the signature as a string, just memcmp()
> instead of strncmp(). You could also consider using a u32 (appropriately
> commented) instead of the string for MCTP_SIGNATURE, and doing a direct
> comparison with pcc_header.command.
>
> 4 -> MCTP_SIGNATURE_LENGTH, or whatever you chose for the above.

I think I like the memcmp option, as it is defined by the ASCII string.



>
>> +
>> +       if (size > mctp_pcc_ndev->ndev->mtu)
>> +               dev_dbg(cl->dev, "MCTP_PCC bytes available exceeds MTU");
> (I assume we're fine to receive larger-than-MTU packets, in which
> case probably don't need the dev_dbg?)
They get dropped by the MCTP layer in the Kernel, apparently, so the 
dev-dbg is to help someone debug why.
PCC currently lacks a negotiation mechanism for setting a larger MTU, so 
we are setting it manually, as our backend uses a non-standard size.

>
>> +
>> +       skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
> ... with the negative size, this will end up as a huge attempted
> allocation here.
>
> Maybe change the above check for pcc_header.length > sizeof(whatever),
> instead of subtracting the sizeof() first, so you cannot underflow.
> Then, use an unsigned type for size.

Yeah, there is certainly some better logic for this.


>
>> +       if (!skb) {
>> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
>> +               return;
>> +       }
>> +       skb_put(skb, size);
>> +       skb->protocol = htons(ETH_P_MCTP);
>> +       memcpy_fromio(skb->data, inbox->chan->shmem + sizeof(pcc_header),
>> +                     size);
> Minor: This seems oddly-wrapped, as you have longer lines above.
I was trying to get everything down to under 80.  Looks like I missed 
some above, but this section had my attention.
>
>> +       dev_dstats_rx_add(mctp_pcc_ndev->ndev, skb->len);
> Your RX stats do not include the PCC header, but your TX stats do.
>
> I would suggest making the SKB layout consistent (ie., presence of the
> transport header) across TX and RX. Include the pcc header in
> the RX skb data (which you do in the TX path), and use skb->len for both
> TX and RX accounting.

Again, a vestige of debugging the disappearing packets which was due to 
exceeding the MTU, but since stats don't effect that, I can add it back in.

I'll get this....Thanks so much for your patience and diligence.


>
> Cheers,
>
>
> Jeremy