netdev - Re: [PATCH net-next v6 08/13] net: wwan: t7xx: Add data path interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 26 Apr 2022 11:00:54 +0300
From:   Sergey Ryazanov <ryazanov.s.a@...il.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     Ricardo Martinez <ricardo.martinez@...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>,
        chandrashekar.devegowda@...el.com,
        Intel Corporation <linuxwwan@...el.com>,
        chiranjeevi.rapolu@...ux.intel.com,
        Haijun Liu (刘海军) 
        <haijun.liu@...iatek.com>, amir.hanania@...el.com,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        dinesh.sharma@...el.com, eliot.lee@...el.com,
        moises.veleta@...el.com, pierre-louis.bossart@...el.com,
        muralidharan.sethuraman@...el.com, Soumya.Prakash.Mishra@...el.com,
        sreehari.kancharla@...el.com, madhusmita.sahu@...el.com
Subject: Re: [PATCH net-next v6 08/13] net: wwan: t7xx: Add data path interface

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.

--
Sergey