David Mosberger-Tang <davidm@xxxxxxxxxx> writes:
> On Tue, 2021-12-14 at 19:36 +0200, Kalle Valo wrote:
>> David Mosberger-Tang <davidm@xxxxxxxxxx> 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
