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

["lihuisong (C)" <lihuisong@huawei.com>]

在 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.
>
>
>