From:   Kalle Valo <kvalo@kernel.org>
To:     David Mosberger-Tang <davidm@egauge.net>
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>
Subject: Re: RFC: wilc1000: refactor TX path to use sk_buff queue
References: <e3502ecffe0c4c01b263ada8deed814d5135c24c.camel@egauge.net>
        <8735mvhyvk.fsf@codeaurora.org>
        <57d8cdfe5c22bf3df2727a18a6096026c59729da.camel@egauge.net>
Date:   Mon, 20 Dec 2021 10:43:13 +0200
In-Reply-To: <57d8cdfe5c22bf3df2727a18a6096026c59729da.camel@egauge.net>
        (David Mosberger-Tang's message of "Tue, 14 Dec 2021 10:43:35 -0700")
Message-ID: <87fsqnmztq.fsf@tynnyri.adurom.net>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

David Mosberger-Tang <davidm@egauge.net> writes:

> 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)
>
> ?

I don't have a good answer. IIRC I have never seen anyone doing anything
like this either, so I'm not sure if it's even worth it? In general
casting is a much bigger problem in upstream and I tend to check those
carefully.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches