On 7/14/23 07:27, Dmitry Antipov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Not a formal description but some thoughts on Ajay's review:
>
> 1. An initial intention was to remove intermediate data structures
> used only to pass parameters, and related 'kmalloc()/kfree()' calls
> used to manage these structures.
This version of patch, which has no dummy variables and few function
arguments removed, looks better.
>
> 2. Why not use the common cleanup function for all type of frames?
> Since the type is always known (it's recorded in 'struct txq_entry_t'),
> cleanup function may call 'dev_kfree_skb()' or 'kfree()' for plain
> buffers. This also shrinks 'struct txq_entry_t' by one pointer field.>
> 3. IMHO it makes sense to make 'struct txq_entry_t' as small as
> possible. To avoid the redundancy (of storing 'skb' nearby to raw
> buffer and its size) for WILC_NET_PKT frames, it's possible to use
> a union, e.g.:
>
> struct txq_entry_t {
> int type;
> ...
> union {
> /* Used by WILC_NET_PKT entries */
> struct sk_buff *skb;
> /* Used by WILC_MGMT_PKT and WILC_CFG_PKT entries */
> struct {
> u8 *buffer;
> u32 size;
> } data;
> } u;
> ...
> };
>
> but this will require some wrappers to access frame data and size,
> e.g.:
>
> static inline u8 *txq_entry_data(struct txq_entry_t *tqe)
> {
> return (tqe->type == WILC_NET_PKT
> ? tqe->u.skb->data : tqe->u.data.buffer);
> }
>
> static inline u32 txq_entry_size(struct txq_entry_t *tqe)
> {
> return (tqe->type == WILC_NET_PKT
> ? tqe->u.skb->len : tqe->u.data.size);
> }
>
> I'm not sure that this makes sense just to shrink 'struct txq_entry_t'
> by one more pointer field.
>
I think the code without the union, which requires extra wrapper API's,
is clean. The tx_complete function takes care of freeing up the
appropriate buffer so skb buffer would have no impact for mgmt/cfg frames.
> Signed-off-by: Dmitry Antipov <dmantipov@xxxxxxxxx>
> ---
> .../wireless/microchip/wilc1000/cfg80211.c | 37 ++-------
> drivers/net/wireless/microchip/wilc1000/mon.c | 47 +++--------
> .../net/wireless/microchip/wilc1000/netdev.c | 34 ++++----
> .../net/wireless/microchip/wilc1000/netdev.h | 1 +
> .../net/wireless/microchip/wilc1000/wlan.c | 80 +++++--------------
> .../net/wireless/microchip/wilc1000/wlan.h | 25 ++----
> 6 files changed, 66 insertions(+), 158 deletions(-)
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> index b545d93c6e37..7d244c6c1a92 100644
> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> @@ -54,11 +54,6 @@ static const struct wiphy_wowlan_support wowlan_support = {
> };
> #endif
>
> -struct wilc_p2p_mgmt_data {
> - int size;
> - u8 *buff;
> -};
> -
> struct wilc_p2p_pub_act_frame {
> u8 category;
> u8 action;
> @@ -1086,14 +1081,6 @@ void wilc_wfi_p2p_rx(struct wilc_vif *vif, u8 *buff, u32 size)
> cfg80211_rx_mgmt(&priv->wdev, freq, 0, buff, size, 0);
> }
>
> -static void wilc_wfi_mgmt_tx_complete(void *priv, int status)
> -{
> - struct wilc_p2p_mgmt_data *pv_data = priv;
> -
> - kfree(pv_data->buff);
> - kfree(pv_data);
> -}
> -
> static void wilc_wfi_remain_on_channel_expired(void *data, u64 cookie)
> {
> struct wilc_vif *vif = data;
> @@ -1172,7 +1159,6 @@ static int mgmt_tx(struct wiphy *wiphy,
> const u8 *buf = params->buf;
> size_t len = params->len;
> const struct ieee80211_mgmt *mgmt;
> - struct wilc_p2p_mgmt_data *mgmt_tx;
> struct wilc_vif *vif = netdev_priv(wdev->netdev);
> struct wilc_priv *priv = &vif->priv;
> struct host_if_drv *wfi_drv = priv->hif_drv;
> @@ -1181,6 +1167,7 @@ static int mgmt_tx(struct wiphy *wiphy,
> int ie_offset = offsetof(struct ieee80211_mgmt, u) + sizeof(*d);
> const u8 *vendor_ie;
> int ret = 0;
> + u8 *copy;
>
> *cookie = get_random_u32();
> priv->tx_cookie = *cookie;
> @@ -1189,21 +1176,12 @@ static int mgmt_tx(struct wiphy *wiphy,
> if (!ieee80211_is_mgmt(mgmt->frame_control))
> goto out;
>
> - mgmt_tx = kmalloc(sizeof(*mgmt_tx), GFP_KERNEL);
> - if (!mgmt_tx) {
> + copy = kmemdup(buf, len, GFP_KERNEL);
> + if (!copy) {
> ret = -ENOMEM;
> goto out;
> }
>
> - mgmt_tx->buff = kmemdup(buf, len, GFP_KERNEL);
> - if (!mgmt_tx->buff) {
> - ret = -ENOMEM;
> - kfree(mgmt_tx);
> - goto out;
> - }
> -
> - mgmt_tx->size = len;
> -
> if (ieee80211_is_probe_resp(mgmt->frame_control)) {
> wilc_set_mac_chnl_num(vif, chan->hw_value);
> vif->wilc->op_ch = chan->hw_value;
> @@ -1230,8 +1208,7 @@ static int mgmt_tx(struct wiphy *wiphy,
> goto out_set_timeout;
>
> vendor_ie = cfg80211_find_vendor_ie(WLAN_OUI_WFA, WLAN_OUI_TYPE_WFA_P2P,
> - mgmt_tx->buff + ie_offset,
> - len - ie_offset);
> + copy + ie_offset, len - ie_offset);
> if (!vendor_ie)
> goto out_set_timeout;
>
> @@ -1243,10 +1220,8 @@ static int mgmt_tx(struct wiphy *wiphy,
>
> out_txq_add_pkt:
>
> - wilc_wlan_txq_add_mgmt_pkt(wdev->netdev, mgmt_tx,
> - mgmt_tx->buff, mgmt_tx->size,
> - wilc_wfi_mgmt_tx_complete);
> -
> + if (wilc_wlan_txq_add_mgmt_pkt(wdev->netdev, copy, len) < 0)
> + kfree(copy);
> out:
>
> return ret;
> diff --git a/drivers/net/wireless/microchip/wilc1000/mon.c b/drivers/net/wireless/microchip/wilc1000/mon.c
> index 03b7229a0ff5..28c642af943d 100644
> --- a/drivers/net/wireless/microchip/wilc1000/mon.c
> +++ b/drivers/net/wireless/microchip/wilc1000/mon.c
> @@ -95,48 +95,25 @@ void wilc_wfi_monitor_rx(struct net_device *mon_dev, u8 *buff, u32 size)
> netif_rx(skb);
> }
>
> -struct tx_complete_mon_data {
> - int size;
> - void *buff;
> -};
> -
> -static void mgmt_tx_complete(void *priv, int status)
> -{
> - struct tx_complete_mon_data *pv_data = priv;
> - /*
> - * in case of fully hosting mode, the freeing will be done
> - * in response to the cfg packet
> - */
> - kfree(pv_data->buff);
> -
> - kfree(pv_data);
> -}
> -
> -static int mon_mgmt_tx(struct net_device *dev, const u8 *buf, size_t len)
> +static int mon_mgmt_tx(struct net_device *dev, const u8 *buf, u32 len)
> {
> - struct tx_complete_mon_data *mgmt_tx = NULL;
> + u8 *copy;
> + int ret = -EFAULT;
>
> if (!dev)
> - return -EFAULT;
> + return ret;
>
> netif_stop_queue(dev);
> - mgmt_tx = kmalloc(sizeof(*mgmt_tx), GFP_ATOMIC);
> - if (!mgmt_tx)
> - return -ENOMEM;
> -
> - mgmt_tx->buff = kmemdup(buf, len, GFP_ATOMIC);
> - if (!mgmt_tx->buff) {
> - kfree(mgmt_tx);
> - return -ENOMEM;
> + copy = kmemdup(buf, len, GFP_ATOMIC);
> + if (!copy) {
> + ret = -ENOMEM;
> + } else {
> + if (wilc_wlan_txq_add_mgmt_pkt(dev, copy, len) < 0)
> + kfree(copy);
> + ret = 0;
> }
> -
> - mgmt_tx->size = len;
> -
> - wilc_wlan_txq_add_mgmt_pkt(dev, mgmt_tx, mgmt_tx->buff, mgmt_tx->size,
> - mgmt_tx_complete);
> -
> netif_wake_queue(dev);
> - return 0;
> + return ret;
> }
'mon_mgmt_tx' can be structed as below
{
u8 *copy;
int ret;
if (!dev)
return -EFAULT;
netif_stop_queue(dev);
copy = kmemdup(buf, len, GFP_ATOMIC);
if (!copy) {
netif_wake_queue(dev);
return -ENOMEM;
}
ret = wilc_wlan_txq_add_mgmt_pkt(dev, copy, len);
if (ret < 0)
kfree(copy);
netif_wake_queue(dev);
}
>
> static netdev_tx_t wilc_wfi_mon_xmit(struct sk_buff *skb,
> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
> index e9f59de31b0b..82143d4d16e1 100644
> --- a/drivers/net/wireless/microchip/wilc1000/netdev.c
> +++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
> @@ -713,19 +713,28 @@ static void wilc_set_multicast_list(struct net_device *dev)
> kfree(mc_list);
> }
>
> -static void wilc_tx_complete(void *priv, int status)
> +void wilc_tx_complete(struct txq_entry_t *tqe)
> {
> - struct tx_complete_data *pv_data = priv;
> -
> - dev_kfree_skb(pv_data->skb);
> - kfree(pv_data);
> + switch (tqe->type) {
> + case WILC_NET_PKT:
> + dev_kfree_skb(tqe->skb);
> + break;
> + case WILC_MGMT_PKT:
> + kfree(tqe->buffer);
> + break;
> + case WILC_CFG_PKT:
> + /* nothing */
> + break;
> + default:
> + netdev_err(tqe->vif->ndev, "bad packet type %d\n", tqe->type);
> + break;
> + }
> }
>
> netdev_tx_t wilc_mac_xmit(struct sk_buff *skb, struct net_device *ndev)
> {
> struct wilc_vif *vif = netdev_priv(ndev);
> struct wilc *wilc = vif->wilc;
> - struct tx_complete_data *tx_data = NULL;
> int queue_count;
>
> if (skb->dev != ndev) {
> @@ -734,22 +743,15 @@ netdev_tx_t wilc_mac_xmit(struct sk_buff *skb, struct net_device *ndev)
> return NETDEV_TX_OK;
> }
>
> - tx_data = kmalloc(sizeof(*tx_data), GFP_ATOMIC);
> - if (!tx_data) {
> + queue_count = wilc_wlan_txq_add_net_pkt(ndev, skb);
> + if (queue_count < 0) {
> dev_kfree_skb(skb);
> netif_wake_queue(ndev);
> return NETDEV_TX_OK;
> }
>
> - tx_data->buff = skb->data;
> - tx_data->size = skb->len;
> - tx_data->skb = skb;
> -
> vif->netstats.tx_packets++;
> - vif->netstats.tx_bytes += tx_data->size;
> - queue_count = wilc_wlan_txq_add_net_pkt(ndev, tx_data,
> - tx_data->buff, tx_data->size,
> - wilc_tx_complete);
> + vif->netstats.tx_bytes += skb->len;
>
> if (queue_count > FLOW_CONTROL_UPPER_THRESHOLD) {
> int srcu_idx;
> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h
> index bb1a315a7b7e..b3ba87bd3581 100644
> --- a/drivers/net/wireless/microchip/wilc1000/netdev.h
> +++ b/drivers/net/wireless/microchip/wilc1000/netdev.h
> @@ -277,6 +277,7 @@ struct wilc_wfi_mon_priv {
> struct net_device *real_ndev;
> };
>
> +void wilc_tx_complete(struct txq_entry_t *tqe);
> void wilc_frmw_to_host(struct wilc *wilc, u8 *buff, u32 size, u32 pkt_offset);
> void wilc_mac_indicate(struct wilc *wilc);
> void wilc_netdev_cleanup(struct wilc *wilc);
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 58bbf50081e4..d594178d05d0 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -219,10 +219,7 @@ static void wilc_wlan_txq_filter_dup_tcp_ack(struct net_device *dev)
> tqe = f->pending_acks[i].txqe;
> if (tqe) {
> wilc_wlan_txq_remove(wilc, tqe->q_num, tqe);
> - tqe->status = 1;
> - if (tqe->tx_complete_func)
> - tqe->tx_complete_func(tqe->priv,
> - tqe->status);
> + wilc_tx_complete(tqe);
> kfree(tqe);
> dropped++;
> }
> @@ -272,8 +269,6 @@ static int wilc_wlan_txq_add_cfg_pkt(struct wilc_vif *vif, u8 *buffer,
> tqe->type = WILC_CFG_PKT;
> tqe->buffer = buffer;
> tqe->buffer_size = buffer_size;
> - tqe->tx_complete_func = NULL;
> - tqe->priv = NULL;
> tqe->q_num = AC_VO_Q;
> tqe->ack_idx = NOT_TCP_ACK;
> tqe->vif = vif;
> @@ -410,45 +405,30 @@ static inline u8 ac_change(struct wilc *wilc, u8 *ac)
> return 1;
> }
>
> -int wilc_wlan_txq_add_net_pkt(struct net_device *dev,
> - struct tx_complete_data *tx_data, u8 *buffer,
> - u32 buffer_size,
> - void (*tx_complete_fn)(void *, int))
> +int wilc_wlan_txq_add_net_pkt(struct net_device *dev, struct sk_buff *skb)
> {
> struct txq_entry_t *tqe;
> struct wilc_vif *vif = netdev_priv(dev);
> - struct wilc *wilc;
> + struct wilc *wilc = vif->wilc;
> u8 q_num;
>
> - wilc = vif->wilc;
> -
> - if (wilc->quit) {
> - tx_complete_fn(tx_data, 0);
> - return 0;
> - }
> -
> - if (!wilc->initialized) {
> - tx_complete_fn(tx_data, 0);
> - return 0;
> - }
> + if (wilc->quit || !wilc->initialized)
> + return -1;
Use macro's to return the error. like -EINVAL.
>
> tqe = kmalloc(sizeof(*tqe), GFP_ATOMIC);
> + if (!tqe)
> + return -1;
>
Same as above.
> - if (!tqe) {
> - tx_complete_fn(tx_data, 0);
> - return 0;
> - }
> tqe->type = WILC_NET_PKT;
> - tqe->buffer = buffer;
> - tqe->buffer_size = buffer_size;
> - tqe->tx_complete_func = tx_complete_fn;
> - tqe->priv = tx_data;
> + tqe->skb = skb;
> + tqe->buffer = skb->data;
> + tqe->buffer_size = skb->len;
> tqe->vif = vif;
>
> - q_num = ac_classify(wilc, tx_data->skb);
> + q_num = ac_classify(wilc, skb);
> tqe->q_num = q_num;
> if (ac_change(wilc, &q_num)) {
> - tx_complete_fn(tx_data, 0);
> + wilc_tx_complete(tqe);
> kfree(tqe);
> return 0;
> }
> @@ -459,43 +439,30 @@ int wilc_wlan_txq_add_net_pkt(struct net_device *dev,
> tcp_process(dev, tqe);
> wilc_wlan_txq_add_to_tail(dev, q_num, tqe);
> } else {
> - tx_complete_fn(tx_data, 0);
> + wilc_tx_complete(tqe);
> kfree(tqe);
> }
>
> return wilc->txq_entries;
> }
>
> -int wilc_wlan_txq_add_mgmt_pkt(struct net_device *dev, void *priv, u8 *buffer,
> - u32 buffer_size,
> - void (*tx_complete_fn)(void *, int))
> +int wilc_wlan_txq_add_mgmt_pkt(struct net_device *dev, u8 *buffer,
> + u32 buffer_size)
> {
> struct txq_entry_t *tqe;
> struct wilc_vif *vif = netdev_priv(dev);
> - struct wilc *wilc;
> -
> - wilc = vif->wilc;
> + struct wilc *wilc = vif->wilc;
>
> - if (wilc->quit) {
> - tx_complete_fn(priv, 0);
> - return 0;
> - }
> + if (wilc->quit || !wilc->initialized)
> + return -1;
Same as above.
>
> - if (!wilc->initialized) {
> - tx_complete_fn(priv, 0);
> - return 0;
> - }
> tqe = kmalloc(sizeof(*tqe), GFP_ATOMIC);
> + if (!tqe)
> + return -1;
Same as above.
>
> - if (!tqe) {
> - tx_complete_fn(priv, 0);
> - return 0;
> - }
> tqe->type = WILC_MGMT_PKT;
> tqe->buffer = buffer;
> tqe->buffer_size = buffer_size;
> - tqe->tx_complete_func = tx_complete_fn;
> - tqe->priv = priv;
> tqe->q_num = AC_BE_Q;
> tqe->ack_idx = NOT_TCP_ACK;
> tqe->vif = vif;
> @@ -916,9 +883,7 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32 *txq_count)
> tqe->buffer, tqe->buffer_size);
> offset += vmm_sz;
> i++;
> - tqe->status = 1;
> - if (tqe->tx_complete_func)
> - tqe->tx_complete_func(tqe->priv, tqe->status);
> + wilc_tx_complete(tqe);
> if (tqe->ack_idx != NOT_TCP_ACK &&
> tqe->ack_idx < MAX_PENDING_ACKS)
> vif->ack_filter.pending_acks[tqe->ack_idx].txqe = NULL;
> @@ -1243,8 +1208,7 @@ void wilc_wlan_cleanup(struct net_device *dev)
> wilc->quit = 1;
> for (ac = 0; ac < NQUEUES; ac++) {
> while ((tqe = wilc_wlan_txq_remove_from_head(wilc, ac))) {
> - if (tqe->tx_complete_func)
> - tqe->tx_complete_func(tqe->priv, 0);
> + wilc_tx_complete(tqe);
> kfree(tqe);
> }
> }
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h b/drivers/net/wireless/microchip/wilc1000/wlan.h
> index a72cd5cac81d..f5ba836c595a 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.h
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
> @@ -323,15 +323,13 @@ enum ip_pkt_priority {
>
> struct txq_entry_t {
> struct list_head list;
> - int type;
> + u8 type;
> u8 q_num;
> - int ack_idx;
> + s16 ack_idx;
> + struct sk_buff *skb;
> u8 *buffer;
> - int buffer_size;
> - void *priv;
> - int status;
> + u32 buffer_size;
> struct wilc_vif *vif;
> - void (*tx_complete_func)(void *priv, int status);
> };
>
> struct txq_fw_recv_queue_stat {
> @@ -378,12 +376,6 @@ struct wilc_hif_func {
>
> #define WILC_MAX_CFG_FRAME_SIZE 1468
>
> -struct tx_complete_data {
> - int size;
> - void *buff;
> - struct sk_buff *skb;
> -};
> -
> struct wilc_cfg_cmd_hdr {
> u8 cmd_type;
> u8 seq_no;
> @@ -407,10 +399,7 @@ int wilc_wlan_firmware_download(struct wilc *wilc, const u8 *buffer,
> u32 buffer_size);
> int wilc_wlan_start(struct wilc *wilc);
> int wilc_wlan_stop(struct wilc *wilc, struct wilc_vif *vif);
> -int wilc_wlan_txq_add_net_pkt(struct net_device *dev,
> - struct tx_complete_data *tx_data, u8 *buffer,
> - u32 buffer_size,
> - void (*tx_complete_fn)(void *, int));
> +int wilc_wlan_txq_add_net_pkt(struct net_device *dev, struct sk_buff *skb);
> int wilc_wlan_handle_txq(struct wilc *wl, u32 *txq_count);
> void wilc_handle_isr(struct wilc *wilc);
> void wilc_wlan_cleanup(struct net_device *dev);
> @@ -418,8 +407,8 @@ int wilc_wlan_cfg_set(struct wilc_vif *vif, int start, u16 wid, u8 *buffer,
> u32 buffer_size, int commit, u32 drv_handler);
> int wilc_wlan_cfg_get(struct wilc_vif *vif, int start, u16 wid, int commit,
> u32 drv_handler);
> -int wilc_wlan_txq_add_mgmt_pkt(struct net_device *dev, void *priv, u8 *buffer,
> - u32 buffer_size, void (*func)(void *, int));
> +int wilc_wlan_txq_add_mgmt_pkt(struct net_device *dev, u8 *buffer,
> + u32 buffer_size);
> void wilc_enable_tcp_ack_filter(struct wilc_vif *vif, bool value);
> int wilc_wlan_get_num_conn_ifcs(struct wilc *wilc);
> netdev_tx_t wilc_mac_xmit(struct sk_buff *skb, struct net_device *dev);
> --
> 2.41.0
>
