Message-ID: <57d8cdfe5c22bf3df2727a18a6096026c59729da.camel@egauge.net>
Subject: Re: RFC: wilc1000: refactor TX path to use sk_buff queue
From:   David Mosberger-Tang <davidm@egauge.net>
To:     Kalle Valo <kvalo@kernel.org>
Cc:     Ajay Singh <ajay.kathat@microchip.com>,
        Claudiu Beznea <claudiu.beznea@microchip.com>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
Date:   Tue, 14 Dec 2021 10:43:35 -0700
In-Reply-To: <8735mvhyvk.fsf@codeaurora.org>
References: <e3502ecffe0c4c01b263ada8deed814d5135c24c.camel@egauge.net>
         <8735mvhyvk.fsf@codeaurora.org>
Organization: eGauge Systems LLC
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2021-12-14 at 19:36 +0200, Kalle Valo wrote:
> David Mosberger-Tang <davidm@egauge.net> writes:
> 
> > I'd like to propose to restructure the wilc1000 TX path to take
> > advantage of the existing sk_buff queuing and buffer operations rather
> > than using a driver-specific solution.  To me, the resulting code looks
> > simpler and the diffstat shows a fair amount of code-reduction:
> > 
> >  cfg80211.c |   35 ----
> >  mon.c      |   36 ----
> >  netdev.c   |   28 ---
> >  netdev.h   |   10 -
> >  wlan.c     |  499 ++++++++++++++++++++++++++-----------------------------------
> >  wlan.h     |   51 ++----
> >  6 files changed, 255 insertions(+), 404 deletions(-)
> 
> Looks like a very good cleanup.

Thanks!

> > +static void wilc_wlan_txq_drop_net_pkt(struct sk_buff *skb)
> > +{
> > +	struct wilc_vif *vif = netdev_priv(skb->dev);
> > +	struct wilc *wilc = vif->wilc;
> > +	struct wilc_skb_tx_cb *tx_cb = WILC_SKB_TX_CB(skb);
> > +
> > +	if ((u8)tx_cb->q_num >= NQUEUES) {
> > +		netdev_err(vif->ndev, "Invalid AC queue number %d",
> > +			   tx_cb->q_num);
> > +		return;
> > +	}
> 
> But why the cast here? Casting should be avoided as much as possible,
> they just create so many problems if used badly.

tx_cb->q_num is declared as:

       enum ip_pkt_priority q_num;     /* AC queue number */

so the cast to (u8) is to protect against negative values (which, of
course, should never really be the case).  Would you rather have the
code check explicitly for negative numbers, i.e.:

    if (tx_cb->q_num < 0 || tx_cb->q_num >= NQUEUES)

?

  --david