Date:   Mon, 2 May 2022 20:40:01 +0300
From:   Sergey Ryazanov <ryazanov.s.a@...il.com>
To:     "Martinez, Ricardo" <ricardo.martinez@...ux.intel.com>
Cc:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Netdev <netdev@...r.kernel.org>, linux-wireless@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Johannes Berg <johannes@...solutions.net>,
        Loic Poulain <loic.poulain@...aro.org>,
        M Chetan Kumar <m.chetan.kumar@...el.com>,
        "Devegowda, Chandrashekar" <chandrashekar.devegowda@...el.com>,
        Intel Corporation <linuxwwan@...el.com>,
        chiranjeevi.rapolu@...ux.intel.com,
        Haijun Liu (刘海军) 
        <haijun.liu@...iatek.com>,
        "Hanania, Amir" <amir.hanania@...el.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "Sharma, Dinesh" <dinesh.sharma@...el.com>,
        "Lee, Eliot" <eliot.lee@...el.com>,
        "Veleta, Moises" <moises.veleta@...el.com>,
        "Bossart, Pierre-louis" <pierre-louis.bossart@...el.com>,
        "Sethuraman, Muralidharan" <muralidharan.sethuraman@...el.com>,
        "Mishra, Soumya Prakash" <Soumya.Prakash.Mishra@...el.com>,
        "Kancharla, Sreehari" <sreehari.kancharla@...el.com>,
        "Sahu, Madhusmita" <madhusmita.sahu@...el.com>
Subject: Re: [PATCH net-next v6 08/13] net: wwan: t7xx: Add data path interface

Hello Ricardo,

On Mon, May 2, 2022 at 7:51 PM Martinez, Ricardo
<ricardo.martinez@...ux.intel.com> wrote:
> On 4/26/2022 1:00 AM, Sergey Ryazanov wrote:
>> On Tue, Apr 26, 2022 at 10:30 AM Ilpo Järvinen
>> <ilpo.jarvinen@...ux.intel.com> wrote:
>>> On Tue, 26 Apr 2022, Sergey Ryazanov wrote:
>>>> On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez
>>>> <ricardo.martinez@...ux.intel.com> wrote:
>>>>> Data Path Modem AP Interface (DPMAIF) HIF layer provides methods
>>>>> for initialization, ISR, control and event handling of TX/RX flows.
>>>>>
>>>>> DPMAIF TX
>>>>> Exposes the 'dmpaif_tx_send_skb' function which can be used by the
>>>>> network device to transmit packets.
>>>>> The uplink data management uses a Descriptor Ring Buffer (DRB).
>>>>> First DRB entry is a message type that will be followed by 1 or more
>>>>> normal DRB entries. Message type DRB will hold the skb information
>>>>> and each normal DRB entry holds a pointer to the skb payload.
>>>>>
>>>>> DPMAIF RX
>>>>> The downlink buffer management uses Buffer Address Table (BAT) and
>>>>> Packet Information Table (PIT) rings.
>>>>> The BAT ring holds the address of skb data buffer for the HW to use,
>>>>> while the PIT contains metadata about a whole network packet including
>>>>> a reference to the BAT entry holding the data buffer address.
>>>>> The driver reads the PIT and BAT entries written by the modem, when
>>>>> reaching a threshold, the driver will reload the PIT and BAT rings.
>>>>>
>>>>> Signed-off-by: Haijun Liu <haijun.liu@...iatek.com>
>>>>> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@...el.com>
>>>>> Co-developed-by: Ricardo Martinez <ricardo.martinez@...ux.intel.com>
>>>>> Signed-off-by: Ricardo Martinez <ricardo.martinez@...ux.intel.com>
>>>>>
>>>>>  From a WWAN framework perspective:
>>>>> Reviewed-by: Loic Poulain <loic.poulain@...aro.org>
>>>> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
>>>>
>>>> and a small question below.
>>>>
>>>>> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
>>>>> ...
>>>>> +static bool t7xx_alloc_and_map_skb_info(const struct dpmaif_ctrl *dpmaif_ctrl,
>>>>> +                                       const unsigned int size, struct dpmaif_bat_skb *cur_skb)
>>>>> +{
>>>>> +       dma_addr_t data_bus_addr;
>>>>> +       struct sk_buff *skb;
>>>>> +       size_t data_len;
>>>>> +
>>>>> +       skb = __dev_alloc_skb(size, GFP_KERNEL);
>>>>> +       if (!skb)
>>>>> +               return false;
>>>>> +
>>>>> +       data_len = skb_end_pointer(skb) - skb->data;
>>>>
>>>> Earlier you use a nice t7xx_skb_data_area_size() function here, but
>>>> now you opencode it. Is it a consequence of t7xx_common.h removing?
>>>>
>>>> I would even encourage you to make this function common and place it
>>>> into include/linux/skbuff.h with a dedicated patch and call it
>>>> something like skb_data_size(). Surprisingly, there is no such helper
>>>> function in the kernel, and several other drivers will benefit from
>>>> it:
>>>
>>> I agree other than the name. IMHO, skb_data_size sounds too much "data
>>> size" which it exactly isn't but just how large the memory area is (we
>>> already have "datalen" anyway and on language level, those two don't sound
>>> different at all). The memory area allocated may or may not have actual
>>> data in it, I suggested adding "area" into it.
>>
>> I agree, using the "area" word in the helper name gives more clue
>> about its purpose, thanks.
>>
>
> Sounds good. I'll add a patch to introduce skb_data_area_size(),
> I'm not planning to update other drivers to use it, at least in this series.

Sure. Introduction of the helper function will be enough in the
context of this series.

This will make life easier for future contributors. And perhaps over
time the earlier opencoded places will be updated to the new API.

-- 
Sergey

Powered by blists - more mailing lists