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

[lihuisong (C)]

在 2025/6/3 4:51, Adam Young 写道:
> On 5/30/25 02:19, lihuisong (C) wrote:
> > 在 2025/4/29 2:48, Adam Young 写道:
> > > On 4/24/25 09:03, lihuisong (C) wrote:
> > > > > +    rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
> > > > > +                     context.inbox_index);
> > > > > +    if (rc)
> > > > > +        goto free_netdev;
> > > > > +    mctp_pcc_ndev->inbox.client.rx_callback = 
> > > > > mctp_pcc_client_rx_callback;
> > > > It is good to move the assignemnt of  rx_callback pointer to 
> > > > initialize inbox mailbox.
> > >
> > >
> > > The other changes are fine, but this one I do not agree with.
> > >
> > > The rx callback only makes sense for one of the two mailboxes, and 
> > > thus is not appropriate for a generic function.
> > >
> > > Either  initialize_mailbox needs more complex logic, or would 
> > > blindly assign the callback to both mailboxes, neither of which 
> > > simplifies or streamlines the code.  That function emerged as a way 
> > > to reduce duplication.  Lets keep it that way.
> > It depends on you. But please reply my below comment. I didn't see 
> > any change about it in next version.
> >
> > -->
> >
> > > +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct 
> > > net_device *ndev)
> > > +{
> > > +    struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
> > > +    struct mctp_pcc_hdr *mctp_pcc_header;
> > > +    void __iomem *buffer;
> > > +    unsigned long flags;
> > > +    int len = skb->len;
> > > +    int rc;
> > > +
> > > +    rc = skb_cow_head(skb, sizeof(struct mctp_pcc_hdr));
> > > +    if (rc)
> > > +        goto err_drop;
> > > +
> > > +    mctp_pcc_header = skb_push(skb, sizeof(struct mctp_pcc_hdr));
> > > +    mctp_pcc_header->signature = cpu_to_le32(PCC_MAGIC | 
> > > mpnd->outbox.index);
> > > +    mctp_pcc_header->flags = cpu_to_le32(PCC_HEADER_FLAGS);
> > > +    memcpy(mctp_pcc_header->mctp_signature, MCTP_SIGNATURE,
> > > +           MCTP_SIGNATURE_LENGTH);
> > > +    mctp_pcc_header->length = cpu_to_le32(len + 
> > > MCTP_SIGNATURE_LENGTH);
> > > +
> > > +    spin_lock_irqsave(&mpnd->lock, flags);
> > > +    buffer = mpnd->outbox.chan->shmem;
> > > +    memcpy_toio(buffer, skb->data, skb->len);
> > > + 
> > > mpnd->outbox.chan->mchan->mbox->ops->send_data(mpnd->outbox.chan->mchan,
> > > +                            NULL);
> > > +    spin_unlock_irqrestore(&mpnd->lock, flags);
> > > +
> > Why does it not need to know if the packet is sent successfully?
> > It's possible for the platform not to finish to send the packet after 
> > executing this unlock.
> > In this moment, the previous packet may be modified by the new packet 
> > to be sent.
>
> I think you missed version  21.
>
> Version 21 of this function ends with:
>         memcpy_toio(buffer, skb->data, skb->len);
>         rc = mpnd->outbox.chan->mchan->mbox->ops->send_data
>                 (mpnd->outbox.chan->mchan, NULL);
>         spin_unlock_irqrestore(&mpnd->lock, flags);
>         if ACPI_FAILURE(rc)
>                 goto err_drop;
>         dev_dstats_tx_add(ndev, len);
>         dev_consume_skb_any(skb);
>         return NETDEV_TX_OK;
> err_drop:
>         dev_dstats_tx_dropped(ndev);
>         kfree_skb(skb);
>         return NETDEV_TX_OK;
>
>
> Once the memcpy_toio completes, the driver will not look at the packet 
> again.  if the Kernel did change it at this point, it would not affect 
> the flow.  The send of the packet is checked vi rc returned from 
> send_data, and it tags the packet as dropped.  Is this not sufficient?
Yes, it is not enough.
Once send_data() return success, platform can receive an interrupt，but 
the processing of the platform has not ended.
This processing includes handling data and then triggering an interrupt 
to notify OS.
>