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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Tue, 26 Apr 2022, Sergey Ryazanov wrote:

> On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez
> <ricardo.martinez@linux.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@mediatek.com>
> > Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.com>
> > Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
> > Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
> >
> > From a WWAN framework perspective:
> > Reviewed-by: Loic Poulain <loic.poulain@linaro.org>
> 
> Reviewed-by: Sergey Ryazanov <ryazanov.s.a@gmail.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.