Re: [RFC 2/7] ath10k: Add support to process rx packet in thread

[Felix Fietkau]

On 2021-03-25 10:45, Rakesh Pillai wrote:
> Hi Felix / Ben,
> 
> In case of ath10k (snoc based targets), we have a lot of processing in the 
> NAPI context.
> Even moving this to threaded NAPI is not helping much due to the load.
> 
> Breaking the tasks into multiple context (with the patch series I posted) is 
> helping in improving the throughput.
> With the current rx_thread based approach, the rx processing is broken into 
> two parallel contexts
> 1) reaping the packets from the HW
> 2) processing these packets list and handing it over to mac80211 (and later 
> to the network stack)
> 
> This is the primary reason for choosing the rx thread approach.
Have you considered the possibility that maybe the problem is that the
driver doing too much work?
One example is that you could take advantage of the new 802.3 decap
offload to simplify rx processing. Worked for me on mt76 where a
dual-core 1.3 GHz A64 can easily handle >1.8 Gbps local TCP rx on a
single card, without the rx NAPI thread being the biggest consumer of
CPU cycles.

And if you can't do that and still consider all of the metric tons of
processing work necessary, you could still do this:
On interrupts, spawn a processing thread that traverses the ring and
does the preparation work (instead of NAPI).
>From that thread you schedule the threaded NAPI handler that processes
these packets further and hands them to mac80211.
To keep the load somewhat balanced, you can limit the number of
pre-processed packets in the ring.

- Felix

Re: [RFC 2/7] ath10k: Add support to process rx packet in thread

[Felix Fietkau]

On 2021-03-23 04:01, Ben Greear wrote:
> On 3/22/21 6:20 PM, Brian Norris wrote:
>> On Mon, Mar 22, 2021 at 4:58 PM Ben Greear  wrote:
>>> On 7/22/20 6:00 AM, Felix Fietkau wrote:
>>>> On 2020-07-22 14:55, Johannes Berg wrote:
>>>>> On Wed, 2020-07-22 at 14:27 +0200, Felix Fietkau wrote:
>>>>>
>>>>>> I'm considering testing a different approach (with mt76 initially):
>>>>>> - Add a mac80211 rx function that puts processed skbs into a list
>>>>>> instead of handing them to the network stack directly.
>>>>>
>>>>> Would this be *after* all the mac80211 processing, i.e. in place of the
>>>>> rx-up-to-stack?
>>>> Yes, it would run all the rx handlers normally and then put the
>>>> resulting skbs into a list instead of calling netif_receive_skb or
>>>> napi_gro_frags.
>>>
>>> Whatever came of this?  I realized I'm running Felix's patch since his mt76
>>> driver needs it.  Any chance it will go upstream?
>> 
>> If you're asking about $subject (moving NAPI/RX to a thread), this
>> landed upstream recently:
>> http://git.kernel.org/linus/adbb4fb028452b1b0488a1a7b66ab856cdf20715
>> 
>> It needs a bit of coaxing to work on a WiFi driver (including: WiFi
>> drivers tend to have a different netdev for NAPI than they expose to
>> /sys/class/net/), but it's there.
>> 
>> I'm not sure if people had something else in mind in the stuff you're
>> quoting though.
> 
> No, I got it confused with something Felix did:
> 
> https://github.com/greearb/mt76/blob/master/patches/0001-net-add-support-for-threaded-NAPI-polling.patch
> 
> Maybe the NAPI/RX to a thread thing superceded Felix's patch?
Yes, it did and it's in linux-next already.
I sent the following change to make mt76 use it:
https://github.com/nbd168/wireless/commit/1d4ff31437e5aaa999bd7a

- Felix

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

[Felix Fietkau]

> On 17. Feb 2021, at 21:28, Shuah Khan  wrote:
> 
> On 2/17/21 7:56 AM, Shuah Khan wrote:
>>> On 2/17/21 12:30 AM, Kalle Valo wrote:
>>> Shuah Khan  writes:
>>> 
>>>> On 2/16/21 12:53 AM, Felix Fietkau wrote:
>>>>> 
>>>>> On 2021-02-16 08:03, Kalle Valo wrote:
>>>>>> Shuah Khan  wrote:
>>>>>> 
>>>>>>> ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr()
>>>>>>> return pointer (sta) outside null check. Fix it by moving the code
>>>>>>> block under the null check.
>>>>>>> 
>>>>>>> This problem was found while reviewing code to debug RCU warn from
>>>>>>> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
>>>>>>> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
>>>>>>> RCU read lock.
>>>>>>> 
>>>>>>> Signed-off-by: Shuah Khan 
>>>>>>> Signed-off-by: Kalle Valo 
>>>>>> 
>>>>>> Patch applied to ath-next branch of ath.git, thanks.
>>>>>> 
>>>>>> a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr 
>>>>>> dereference
>>>>> I just took another look at this patch, and it is completely bogus.
>>>>> Not only does the stated reason not make any sense (sta is simply passed
>>>>> to other functions, not dereferenced without checks), but this also
>>>>> introduces a horrible memory leak by skipping buffer completion if sta
>>>>> is NULL.
>>>>> Please drop it, the code is fine as-is.
>>>> 
> 
> Felix,
> 
> I looked at the code path again and found the following path that
> can become a potential dereference downstream. My concern is
> about potential dereference downstream.
> 
> First path: ath_tx_complete_buf()
> 
> 1. ath_tx_process_buffer() passes sta to ath_tx_complete_buf()
> 2. ath_tx_complete_buf() doesn't check or dereference sta
>   Passes it on to ath_tx_complete()
> 3. ath_tx_complete() doesn't check or dereference sta, but assigns
>   it to tx_info->status.status_driver_data[0]
>   tx_info->status.status_driver_data[0] = sta;
> 
> ath_tx_complete_buf() should be fixed to check sta perhaps?
> 
> This assignment without checking could lead to dereference at some
> point in the future.
The assignment is fine, no check needed here. If there was any invalid 
dereference here, we would see reports of crashes with NULL pointer 
dereference. Sending packets with sta==NULL is quite common, especially in AP 
mode.

> Second path: ath_tx_complete_aggr()
> 
> 1. ath_tx_process_buffer() passes sta to ath_tx_complete_aggr()
> 2. No problems in this path as ath_tx_complete_aggr() checks
>   sta before use.
> 
> I can send the revert as it moves more code than necessary under
> the null check. As you pointed out, it could lead to memory leak.
> Not knowing this code well, I can't really tell where. However,
> my original concern is valid for ath_tx_complete_buf
I still don’t see anything to be concerned about. I don’t even think passing a 
pointer that could be NULL to another function even deserves a comment in the 
code. Stuff like that is used in many other places as well.

- Felix

Re: [PATCH 2/2] ath9k: fix ath_tx_process_buffer() potential null ptr dereference

[Felix Fietkau]

On 2021-02-16 08:03, Kalle Valo wrote:
> Shuah Khan  wrote:
> 
>> ath_tx_process_buffer() references ieee80211_find_sta_by_ifaddr()
>> return pointer (sta) outside null check. Fix it by moving the code
>> block under the null check.
>> 
>> This problem was found while reviewing code to debug RCU warn from
>> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
>> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
>> RCU read lock.
>> 
>> Signed-off-by: Shuah Khan 
>> Signed-off-by: Kalle Valo 
> 
> Patch applied to ath-next branch of ath.git, thanks.
> 
> a56c14bb21b2 ath9k: fix ath_tx_process_buffer() potential null ptr dereference
I just took another look at this patch, and it is completely bogus.
Not only does the stated reason not make any sense (sta is simply passed
to other functions, not dereferenced without checks), but this also
introduces a horrible memory leak by skipping buffer completion if sta
is NULL.
Please drop it, the code is fine as-is.

- Felix

Re: [PATCH] rtw88: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

[Felix Fietkau]

On 2021-02-12 03:13, Shuah Khan wrote:
> ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
> the resulting pointer is only valid under RCU lock as well.
> 
> Fix rtw_rx_addr_match_iter() to hold RCU read lock before it calls
> ieee80211_find_sta_by_ifaddr() and release it when the resulting
> pointer is no longer needed.
> 
> This problem was found while reviewing code to debug RCU warn from
> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
> RCU read lock.
> 
> Signed-off-by: Shuah Khan 
This one also seems unnecessary. rtw_rx_addr_match_iter is called by
ieee80211_iterate_active_interfaces_atomic, which acquires the RCU read
lock before calling it.

- Felix

Re: [PATCH 1/2] ath9k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

[Felix Fietkau]

On 2021-02-12 03:13, Shuah Khan wrote:
> ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
> the resulting pointer is only valid under RCU lock as well.
> 
> Fix ath_tx_process_buffer() to hold RCU read lock before it calls
> ieee80211_find_sta_by_ifaddr() and release it when the resulting
> pointer is no longer needed.
> 
> This problem was found while reviewing code to debug RCU warn from
> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
> RCU read lock.
> 
> Signed-off-by: Shuah Khan 
This patch seems unnecessary as well. All callers of
ath_tx_process_buffer seem to hold the RCU read lock already.

- Felix

Re: [PATCH] mt76: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

[Felix Fietkau]

On 2021-02-12 03:13, Shuah Khan wrote:
> ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
> the resulting pointer is only valid under RCU lock as well.
> 
> Fix mt76_check_sta() to hold RCU read lock before it calls
> ieee80211_find_sta_by_ifaddr() and release it when the resulting
> pointer is no longer needed.
> 
> This problem was found while reviewing code to debug RCU warn from
> ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
> of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
> RCU read lock.
> 
> Signed-off-by: Shuah Khan 
If I'm not mistaken, this patch is unnecessary. mt76_check_sta is only
called from mt76_rx_poll_complete, which itself is only called under RCU
lock.

- Felix

Re: [PATCH] mt76: Fix queue ID variable types after mcu queue split

[Felix Fietkau]

On 2021-01-11 09:06, Kalle Valo wrote:
> Lorenzo Bianconi  writes:
> 
>>> Clang warns in both mt7615 and mt7915:
>>> 
>>> drivers/net/wireless/mediatek/mt76/mt7915/mcu.c:271:9: warning: implicit
>>> conversion from enumeration type 'enum mt76_mcuq_id' to different
>>> enumeration type 'enum mt76_txq_id' [-Wenum-conversion]
>>> txq = MT_MCUQ_FWDL;
>>> ~ ^~~~
>>> drivers/net/wireless/mediatek/mt76/mt7915/mcu.c:278:9: warning: implicit
>>> conversion from enumeration type 'enum mt76_mcuq_id' to different
>>> enumeration type 'enum mt76_txq_id' [-Wenum-conversion]
>>> txq = MT_MCUQ_WA;
>>> ~ ^~
>>> drivers/net/wireless/mediatek/mt76/mt7915/mcu.c:282:9: warning: implicit
>>> conversion from enumeration type 'enum mt76_mcuq_id' to different
>>> enumeration type 'enum mt76_txq_id' [-Wenum-conversion]
>>> txq = MT_MCUQ_WM;
>>> ~ ^~
>>> 3 warnings generated.
>>> 
>>> drivers/net/wireless/mediatek/mt76/mt7615/mcu.c:238:9: warning: implicit
>>> conversion from enumeration type 'enum mt76_mcuq_id' to different
>>> enumeration type 'enum mt76_txq_id' [-Wenum-conversion]
>>> qid = MT_MCUQ_WM;
>>> ~ ^~
>>> drivers/net/wireless/mediatek/mt76/mt7615/mcu.c:240:9: warning: implicit
>>> conversion from enumeration type 'enum mt76_mcuq_id' to different
>>> enumeration type 'enum mt76_txq_id' [-Wenum-conversion]
>>> qid = MT_MCUQ_FWDL;
>>> ~ ^~~~
>>> 2 warnings generated.
>>> 
>>> Use the proper type for the queue ID variables to fix these warnings.
>>> Additionally, rename the txq variable in mt7915_mcu_send_message to be
>>> more neutral like mt7615_mcu_send_message.
>>> 
>>> Fixes: e637763b606b ("mt76: move mcu queues to mt76_dev q_mcu array")
>>> Link: https://github.com/ClangBuiltLinux/linux/issues/1229
>>> Signed-off-by: Nathan Chancellor 
>>
>> Acked-by: Lorenzo Bianconi 
> 
> I see that Felix already applied this, but as this is a regression
> starting from v5.11-rc1 I think it should be applied to
> wireless-drivers. Felix, can you drop this from your tree so that I
> could apply it to wireless-drivers?
Sure, will do.

- Felix

Re: [PATCH 2/3] mac80211: Add support to trigger sta disconnect on hardware restart

[Felix Fietkau]

On 2020-12-15 18:23, Youghandhar Chintala wrote:
> Currently in case of target hardware restart, we just reconfig and
> re-enable the security keys and enable the network queues to start
> data traffic back from where it was interrupted.
> 
> Many ath10k wifi chipsets have sequence numbers for the data
> packets assigned by firmware and the mac sequence number will
> restart from zero after target hardware restart leading to mismatch
> in the sequence number expected by the remote peer vs the sequence
> number of the frame sent by the target firmware.
> 
> This mismatch in sequence number will cause out-of-order packets
> on the remote peer and all the frames sent by the device are dropped
> until we reach the sequence number which was sent before we restarted
> the target hardware
> 
> In order to fix this, we trigger a sta disconnect, for the targets
> which expose this corresponding wiphy flag, in case of target hw
> restart. After this there will be a fresh connection and thereby
> avoiding the dropping of frames by remote peer.
> 
> The right fix would be to pull the entire data path into the host
> which is not feasible or would need lots of complex changes and
> will still be inefficient.
How about simply tracking which tids have aggregation enabled and send
DELBA frames for those after the restart?
It would mean less disruption for affected stations and less ugly hacks
in the stack for unreliable hardware.

- Felix

Re: [PATCH -next] mt76: Convert to DEFINE_SHOW_ATTRIBUTE

[Felix Fietkau]

On 2020-07-16 10:58, Qinglang Miao wrote:
> Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
> 
> Signed-off-by: Qinglang Miao 
This does not seem to apply to the current tree, please rebase.

Thanks,

- Felix

Re: perf regression after "genirq/affinity: Handle affinity setting on inactive interrupts correctly"

[Felix Fietkau]

On 2020-08-21 19:38, Felix Fietkau wrote:
> Hi Thomas,
> 
> after updating my MT7622 ARM64 device from Linux 5.4.52 to 5.4.59, perf
> stopped working. Whenever I started it, I got an IRQ storm on the second
> arm-pmu IRQ. After some digging, I figured out that reverting commit
> 9f8d3d2f79ba189ecc122d214d32396e5737963b ("genirq/affinity: Handle
> affinity setting on inactive interrupts correctly") made it work again.
Never mind, I just found your fix in 5.4.60 and it works for me.
Sorry for the noise :)

- Felix

perf regression after "genirq/affinity: Handle affinity setting on inactive interrupts correctly"

[Felix Fietkau]

Hi Thomas,

after updating my MT7622 ARM64 device from Linux 5.4.52 to 5.4.59, perf
stopped working. Whenever I started it, I got an IRQ storm on the second
arm-pmu IRQ. After some digging, I figured out that reverting commit
9f8d3d2f79ba189ecc122d214d32396e5737963b ("genirq/affinity: Handle
affinity setting on inactive interrupts correctly") made it work again.

Without the revert, the IRQ storm triggered on CPU0 (no interrupts on
CPU1).
With the revert, IRQs fire normally on CPU1 and perf works again.

It seems that setting the IRQ affinity for the arm-pmu IRQs does not
work anymore.

Please let me know if you need me to test anything.

- Felix

Re: [RFC 0/7] Add support to process rx packets in thread

[Felix Fietkau]

On 2020-07-26 10:32, Hillf Danton wrote:
> 
> On Sun, 26 Jul 2020 10:10:15 +0200 Felix Fietkau wrote:
>> On 2020-07-26 03:22, Hillf Danton wrote:
>> > 
>> > Feel free to do that. Is it likely for me to select a Cc?
>> > 
>> Shall I use Signed-off-by: Hillf Danton ?
> 
> s/Signed-off-by/Cc/
> 
>> What Cc do you want me to add?
> 
> I prefer Cc over other tags.
Ah, okay. I was planning on adding you as the author of the patch, since
you did most of the work on it. If you want to be attributed as author
(or in Co-developed-by), I'd need your Signed-off-by.

If you don't want that, I can submit it under my name and leave you in
as Cc only.

- Felix

Re: [RFC 0/7] Add support to process rx packets in thread

[Felix Fietkau]

On 2020-07-26 03:22, Hillf Danton wrote:
>> - add a state bit for threaded NAPI
>> - make netif_threaded_napi_add inline
>> - run queue_work outside of local_irq_save/restore (it does that
>> internally already)
>>
>> If you don't mind, I'd like to propose this to netdev soon. Can I have
>> your Signed-off-by for that?
> 
> Feel free to do that. Is it likely for me to select a Cc?
Shall I use Signed-off-by: Hillf Danton ?
What Cc do you want me to add?

>> ---
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -347,6 +347,7 @@ struct napi_struct {
>>  struct list_headdev_list;
>>  struct hlist_node   napi_hash_node;
>>  unsigned intnapi_id;
>> +struct work_struct  work;
>>  };
>>  
>>  enum {
>> @@ -357,6 +358,7 @@ enum {
>>  NAPI_STATE_HASHED,  /* In NAPI hash (busy polling possible) */
>>  NAPI_STATE_NO_BUSY_POLL,/* Do not add in napi_hash, no busy polling */
>>  NAPI_STATE_IN_BUSY_POLL,/* sk_busy_loop() owns this NAPI */
>> +NAPI_STATE_THREADED,/* Use threaded NAPI */
>>  };
>>  
>>  enum {
>> @@ -367,6 +369,7 @@ enum {
>>  NAPIF_STATE_HASHED   = BIT(NAPI_STATE_HASHED),
>>  NAPIF_STATE_NO_BUSY_POLL = BIT(NAPI_STATE_NO_BUSY_POLL),
>>  NAPIF_STATE_IN_BUSY_POLL = BIT(NAPI_STATE_IN_BUSY_POLL),
>> +NAPIF_STATE_THREADED = BIT(NAPI_STATE_THREADED),
>>  };
>>  
>>  enum gro_result {
>> @@ -2315,6 +2318,26 @@ static inline void *netdev_priv(const struct 
>> net_device *dev)
>>  void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
>>  int (*poll)(struct napi_struct *, int), int weight);
>>  
>> +/**
>> + *  netif_threaded_napi_add - initialize a NAPI context
>> + *  @dev:  network device
>> + *  @napi: NAPI context
>> + *  @poll: polling function
>> + *  @weight: default weight
>> + *
>> + * This variant of netif_napi_add() should be used from drivers using NAPI
>> + * with CPU intensive poll functions.
>> + * This will schedule polling from a high priority workqueue that
>> + */
>> +static inline void netif_threaded_napi_add(struct net_device *dev,
>> +   struct napi_struct *napi,
>> +   int (*poll)(struct napi_struct *, 
>> int),
>> +   int weight)
>> +{
>> +set_bit(NAPI_STATE_THREADED, >state);
>> +netif_napi_add(dev, napi, poll, weight);
>> +}
>> +
>>  /**
>>   *  netif_tx_napi_add - initialize a NAPI context
>>   *  @dev:  network device
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -158,6 +158,7 @@ static DEFINE_SPINLOCK(offload_lock);
>>  struct list_head ptype_base[PTYPE_HASH_SIZE] __read_mostly;
>>  struct list_head ptype_all __read_mostly;   /* Taps */
>>  static struct list_head offload_base __read_mostly;
>> +static struct workqueue_struct *napi_workq;
> 
> Is __read_mostly missing?
Yes, thanks. I will add that before sending the patch.

- Felix

Re: [RFC 0/7] Add support to process rx packets in thread

[Felix Fietkau]

On 2020-07-25 10:16, Hillf Danton wrote:
> Hi folks
> 
> Below is a minimunm poc implementation I can imagine on top of workqueue
> to make napi threaded. Thoughts are appreciated.
Hi Hillf,

For some reason I don't see your mails on linux-wireless/netdev.
I've cleaned up your implementation a bit and I ran some tests with mt76
on an mt7621 embedded board. The results look pretty nice, performance
is a lot more consistent in my tests now.

Here are the changes that I've made compared to your version:

- remove the #ifdef, I think it's unnecessary
- add a state bit for threaded NAPI
- make netif_threaded_napi_add inline
- run queue_work outside of local_irq_save/restore (it does that
internally already)

If you don't mind, I'd like to propose this to netdev soon. Can I have
your Signed-off-by for that?

Thanks,

- Felix

---
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -347,6 +347,7 @@ struct napi_struct {
struct list_headdev_list;
struct hlist_node   napi_hash_node;
unsigned intnapi_id;
+   struct work_struct  work;
 };
 
 enum {
@@ -357,6 +358,7 @@ enum {
NAPI_STATE_HASHED,  /* In NAPI hash (busy polling possible) */
NAPI_STATE_NO_BUSY_POLL,/* Do not add in napi_hash, no busy polling */
NAPI_STATE_IN_BUSY_POLL,/* sk_busy_loop() owns this NAPI */
+   NAPI_STATE_THREADED,/* Use threaded NAPI */
 };
 
 enum {
@@ -367,6 +369,7 @@ enum {
NAPIF_STATE_HASHED   = BIT(NAPI_STATE_HASHED),
NAPIF_STATE_NO_BUSY_POLL = BIT(NAPI_STATE_NO_BUSY_POLL),
NAPIF_STATE_IN_BUSY_POLL = BIT(NAPI_STATE_IN_BUSY_POLL),
+   NAPIF_STATE_THREADED = BIT(NAPI_STATE_THREADED),
 };
 
 enum gro_result {
@@ -2315,6 +2318,26 @@ static inline void *netdev_priv(const struct net_device 
*dev)
 void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
int (*poll)(struct napi_struct *, int), int weight);
 
+/**
+ * netif_threaded_napi_add - initialize a NAPI context
+ * @dev:  network device
+ * @napi: NAPI context
+ * @poll: polling function
+ * @weight: default weight
+ *
+ * This variant of netif_napi_add() should be used from drivers using NAPI
+ * with CPU intensive poll functions.
+ * This will schedule polling from a high priority workqueue that
+ */
+static inline void netif_threaded_napi_add(struct net_device *dev,
+  struct napi_struct *napi,
+  int (*poll)(struct napi_struct *, 
int),
+  int weight)
+{
+   set_bit(NAPI_STATE_THREADED, >state);
+   netif_napi_add(dev, napi, poll, weight);
+}
+
 /**
  * netif_tx_napi_add - initialize a NAPI context
  * @dev:  network device
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -158,6 +158,7 @@ static DEFINE_SPINLOCK(offload_lock);
 struct list_head ptype_base[PTYPE_HASH_SIZE] __read_mostly;
 struct list_head ptype_all __read_mostly;  /* Taps */
 static struct list_head offload_base __read_mostly;
+static struct workqueue_struct *napi_workq;
 
 static int netif_rx_internal(struct sk_buff *skb);
 static int call_netdevice_notifiers_info(unsigned long val,
@@ -6286,6 +6287,11 @@ void __napi_schedule(struct napi_struct *n)
 {
unsigned long flags;
 
+   if (test_bit(NAPI_STATE_THREADED, >state)) {
+   queue_work(napi_workq, >work);
+   return;
+   }
+
local_irq_save(flags);
napi_schedule(this_cpu_ptr(_data), n);
local_irq_restore(flags);
@@ -6333,6 +6339,11 @@ EXPORT_SYMBOL(napi_schedule_prep);
  */
 void __napi_schedule_irqoff(struct napi_struct *n)
 {
+   if (test_bit(NAPI_STATE_THREADED, >state)) {
+   queue_work(napi_workq, >work);
+   return;
+   }
+
napi_schedule(this_cpu_ptr(_data), n);
 }
 EXPORT_SYMBOL(__napi_schedule_irqoff);
@@ -6601,6 +6612,29 @@ static void init_gro_hash(struct napi_struct *napi)
napi->gro_bitmask = 0;
 }
 
+static void napi_workfn(struct work_struct *work)
+{
+   struct napi_struct *n = container_of(work, struct napi_struct, work);
+
+   for (;;) {
+   if (!test_bit(NAPI_STATE_SCHED, >state))
+   return;
+
+   if (n->poll(n, n->weight) < n->weight)
+   return;
+
+   if (need_resched()) {
+   /*
+* have to pay for the latency of task switch even if
+* napi is scheduled
+*/
+   if (test_bit(NAPI_STATE_SCHED, >state))
+   queue_work(napi_workq, work);
+   return;
+   }
+   }
+}
+
 void netif_napi_add(struct net_device *dev, struct napi_struct *napi,
int (*poll)(struct napi_struct *, int), int weight)
 {
@@ -6621,6 +6655,7 @@ void 

Re: [RFC 2/7] ath10k: Add support to process rx packet in thread

[Felix Fietkau]

On 2020-07-22 14:55, Johannes Berg wrote:
> On Wed, 2020-07-22 at 14:27 +0200, Felix Fietkau wrote:
> 
>> I'm considering testing a different approach (with mt76 initially):
>> - Add a mac80211 rx function that puts processed skbs into a list
>> instead of handing them to the network stack directly.
> 
> Would this be *after* all the mac80211 processing, i.e. in place of the
> rx-up-to-stack?
Yes, it would run all the rx handlers normally and then put the
resulting skbs into a list instead of calling netif_receive_skb or
napi_gro_frags.

- Felix

Re: [RFC 2/7] ath10k: Add support to process rx packet in thread

[Felix Fietkau]

On 2020-07-21 23:53, Rajkumar Manoharan wrote:
> On 2020-07-21 10:14, Rakesh Pillai wrote:
>> NAPI instance gets scheduled on a CPU core on which
>> the IRQ was triggered. The processing of rx packets
>> can be CPU intensive and since NAPI cannot be moved
>> to a different CPU core, to get better performance,
>> its better to move the gist of rx packet processing
>> in a high priority thread.
>> 
>> Add the init/deinit part for a thread to process the
>> receive packets.
>> 
> IMHO this defeat the whole purpose of NAPI. Originally in ath10k
> irq processing happened in tasklet (high priority) context which in
> turn push more data to net core even though net is unable to process
> driver data as both happen in different context (fast producer - slow 
> consumer)
> issue. Why can't CPU governor schedule the interrupts in less loaded CPU 
> core?
> Otherwise you can play with different RPS and affinity settings to meet 
> your
> requirement.
> 
> IMO introducing high priority tasklets/threads is not viable solution.
I'm beginning to think that the main problem with NAPI here is that the
work done by poll functions on 802.11 drivers is significantly more CPU
intensive compared to ethernet drivers, possibly more than what NAPI was
designed for.

I'm considering testing a different approach (with mt76 initially):
- Add a mac80211 rx function that puts processed skbs into a list
instead of handing them to the network stack directly.
- Move all rx processing to a high priority thread, keep a driver
internal queue for fully processed packets.
- Schedule NAPI poll on completion.
- NAPI poll function pulls from the internal queue and passes to the
network stack.

With this approach, the network stack retains some control over the
processing rate of rx packets, while the scheduler can move the CPU
intensive processing around to where it fits best.

What do you think?

- Felix

Re: [PATCH 4.19 157/267] mt76: avoid rx reorder buffer overflow

[Felix Fietkau]

On 2020-06-19 16:32, Greg Kroah-Hartman wrote:
> From: Ryder Lee 
> 
> [ Upstream commit 7c4f744d6703757be959f521a7a441bf34745d99 ]
> 
> Enlarge slot to support 11ax 256 BA (256 MPDUs in an AMPDU)
> 
> Signed-off-by: Chih-Min Chen 
> Signed-off-by: Ryder Lee 
> Signed-off-by: Felix Fietkau 
> Signed-off-by: Sasha Levin 
It does not make sense to backport this commit. It doesn't fix anything,
it's just preparation work for the mt7915 driver.

- Felix

Re: [PATCH][next] mac80211: minstrel_ht: fix infinite loop because supported is not being shifted

[Felix Fietkau]

On 2019-08-22 14:20, Colin King wrote:
> From: Colin Ian King 
> 
> Currently the for-loop will spin forever if variable supported is
> non-zero because supported is never changed.  Fix this by adding in
> the missing right shift of supported.
> 
> Addresses-Coverity: ("Infinite loop")
> Fixes: 48cb39522a9d ("mac80211: minstrel_ht: improve rate probing for devices 
> with static fallback")
> Signed-off-by: Colin Ian King 
Acked-by: Felix Fietkau 

Thanks,

- Felix

Re: [PATCH v1 5/6] mt76: fix some checkpatch warnings

[Felix Fietkau]

On 2019-07-24 10:58, Ryder Lee wrote:
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -537,25 +537,25 @@ struct mt76_rx_status {
>   s8 chain_signal[IEEE80211_MAX_CHAINS];
>  };
>  
> -#define __mt76_rr(dev, ...)  (dev)->bus->rr((dev), __VA_ARGS__)
> -#define __mt76_wr(dev, ...)  (dev)->bus->wr((dev), __VA_ARGS__)
> -#define __mt76_rmw(dev, ...) (dev)->bus->rmw((dev), __VA_ARGS__)
> -#define __mt76_wr_copy(dev, ...) (dev)->bus->copy((dev), __VA_ARGS__)
> +#define __mt76_rr(dev, ...)  ((dev)->bus->rr((dev), __VA_ARGS__))
> +#define __mt76_wr(dev, ...)  ((dev)->bus->wr((dev), __VA_ARGS__))
> +#define __mt76_rmw(dev, ...) ((dev)->bus->rmw((dev), __VA_ARGS__))
> +#define __mt76_wr_copy(dev, ...) ((dev)->bus->copy((dev), __VA_ARGS__))
>  
>  #define __mt76_set(dev, offset, val) __mt76_rmw(dev, offset, 0, val)
>  #define __mt76_clear(dev, offset, val)   __mt76_rmw(dev, offset, val, 0)
>  
> -#define mt76_rr(dev, ...)(dev)->mt76.bus->rr(&((dev)->mt76), __VA_ARGS__)
> -#define mt76_wr(dev, ...)(dev)->mt76.bus->wr(&((dev)->mt76), __VA_ARGS__)
> -#define mt76_rmw(dev, ...)   (dev)->mt76.bus->rmw(&((dev)->mt76), 
> __VA_ARGS__)
> -#define mt76_wr_copy(dev, ...)   (dev)->mt76.bus->copy(&((dev)->mt76), 
> __VA_ARGS__)
> -#define mt76_wr_rp(dev, ...) (dev)->mt76.bus->wr_rp(&((dev)->mt76), 
> __VA_ARGS__)
> -#define mt76_rd_rp(dev, ...) (dev)->mt76.bus->rd_rp(&((dev)->mt76), 
> __VA_ARGS__)
> +#define mt76_rr(dev, ...)((dev)->mt76.bus->rr(&((dev)->mt76), 
> __VA_ARGS__))
> +#define mt76_wr(dev, ...)((dev)->mt76.bus->wr(&((dev)->mt76), 
> __VA_ARGS__))
> +#define mt76_rmw(dev, ...)   ((dev)->mt76.bus->rmw(&((dev)->mt76), 
> __VA_ARGS__))
> +#define mt76_wr_copy(dev, ...)   ((dev)->mt76.bus->copy(&((dev)->mt76), 
> __VA_ARGS__))
> +#define mt76_wr_rp(dev, ...) ((dev)->mt76.bus->wr_rp(&((dev)->mt76), 
> __VA_ARGS__))
> +#define mt76_rd_rp(dev, ...) ((dev)->mt76.bus->rd_rp(&((dev)->mt76), 
> __VA_ARGS__))
>  
> -#define mt76_mcu_send_msg(dev, ...)  
> (dev)->mt76.mcu_ops->mcu_send_msg(&((dev)->mt76), __VA_ARGS__)
> -#define __mt76_mcu_send_msg(dev, ...)
> (dev)->mcu_ops->mcu_send_msg((dev), __VA_ARGS__)
> -#define mt76_mcu_restart(dev, ...)   
> (dev)->mt76.mcu_ops->mcu_restart(&((dev)->mt76))
> -#define __mt76_mcu_restart(dev, ...) (dev)->mcu_ops->mcu_restart((dev))
> +#define mt76_mcu_send_msg(dev, ...)  
> ((dev)->mt76.mcu_ops->mcu_send_msg(&((dev)->mt76), __VA_ARGS__))
> +#define __mt76_mcu_send_msg(dev, ...)
> ((dev)->mcu_ops->mcu_send_msg((dev), __VA_ARGS__))
> +#define mt76_mcu_restart(dev, ...)   
> ((dev)->mt76.mcu_ops->mcu_restart(&((dev)->mt76)))
> +#define __mt76_mcu_restart(dev, ...) ((dev)->mcu_ops->mcu_restart((dev)))
>  
>  #define mt76_set(dev, offset, val)   mt76_rmw(dev, offset, 0, val)
>  #define mt76_clear(dev, offset, val) mt76_rmw(dev, offset, val, 0)
> @@ -569,7 +569,7 @@ struct mt76_rx_status {
>  #define __mt76_rmw_field(_dev, _reg, _field, _val)   \
>   __mt76_rmw(_dev, _reg, _field, FIELD_PREP(_field, _val))
>  
> -#define mt76_hw(dev) (dev)->mt76.hw
> +#define mt76_hw(dev) ((dev)->mt76.hw)
>  
>  bool __mt76_poll(struct mt76_dev *dev, u32 offset, u32 mask, u32 val,
>int timeout);
> @@ -596,13 +596,13 @@ static inline u16 mt76_rev(struct mt76_dev *dev)
>  #define mt76xx_chip(dev) mt76_chip(&((dev)->mt76))
>  #define mt76xx_rev(dev) mt76_rev(&((dev)->mt76))
>  
> -#define mt76_init_queues(dev)
> (dev)->mt76.queue_ops->init(&((dev)->mt76))
> -#define mt76_queue_alloc(dev, ...)   
> (dev)->mt76.queue_ops->alloc(&((dev)->mt76), __VA_ARGS__)
> -#define mt76_tx_queue_skb_raw(dev, ...)  
> (dev)->mt76.queue_ops->tx_queue_skb_raw(&((dev)->mt76), __VA_ARGS__)
> -#define mt76_tx_queue_skb(dev, ...)  
> (dev)->mt76.queue_ops->tx_queue_skb(&((dev)->mt76), __VA_ARGS__)
> -#define mt76_queue_rx_reset(dev, ...)
> (dev)->mt76.queue_ops->rx_reset(&((dev)->mt76), __VA_ARGS__)
> -#define mt76_queue_tx_cleanup(dev, ...)  
> (dev)->mt76.queue_ops->tx_cleanup(&((dev)->mt76), __VA_ARGS__)
> -#define mt76_queue_kick(dev, ...)
> (dev)->mt76.queue_ops->kick(&((dev)->mt76), __VA_ARGS__)
> +#define mt76_init_queues(dev)
> ((dev)->mt76.queue_ops->init(&((dev)->mt76)))
> +#define mt76_queue_alloc(dev, ...)   
> ((dev)->mt76.queue_ops->alloc(&((dev)->mt76), __VA_ARGS__))
> +#define mt76_tx_queue_skb_raw(dev, ...)  
> ((dev)->mt76.queue_ops->tx_queue_skb_raw(&((dev)->mt76), __VA_ARGS__))
> +#define mt76_tx_queue_skb(dev, ...)  
> ((dev)->mt76.queue_ops->tx_queue_skb(&((dev)->mt76), __VA_ARGS__))
> +#define mt76_queue_rx_reset(dev, ...)
> ((dev)->mt76.queue_ops->rx_reset(&((dev)->mt76), __VA_ARGS__))
> +#define mt76_queue_tx_cleanup(dev, ...)  
> ((dev)->mt76.queue_ops->tx_cleanup(&((dev)->mt76), __VA_ARGS__))
> +#define mt76_queue_kick(dev, ...)
> ((dev)->mt76.queue_ops->kick(&((dev)->mt76), __VA_ARGS__))
>  
>  static inline 

Re: [PATCH v3 2/2] mt76: mt7615: fix slow performance when enable encryption

[Felix Fietkau]

On 2019-06-03 08:08, Ryder Lee wrote:
> Fix wrong WCID assignment and add RKV (RX Key of this entry is valid)
> flag to check if peer uses the same configuration with previous
> handshaking.
> 
> If the configuration is mismatch, WTBL indicates a “cipher mismatch”
> to stop SEC decryption to prevent the packet from damage.
> 
> Suggested-by: YF Luo 
> Suggested-by: Yiwei Chung 
> Signed-off-by: Ryder Lee 

Applied, thanks.

- Felix

Re: [PATCH] mt76: Remove set but not used variables 'pid' and 'final_mpdu'

[Felix Fietkau]

On 2019-05-29 16:53, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warnings:
> 
> drivers/net/wireless/mediatek/mt76/mt7603/mac.c: In function mt7603_fill_txs:
> drivers/net/wireless/mediatek/mt76/mt7603/mac.c:969:5: warning: variable pid 
> set but not used [-Wunused-but-set-variable]
> drivers/net/wireless/mediatek/mt76/mt7603/mac.c:961:7: warning: variable 
> final_mpdu set but not used [-Wunused-but-set-variable]
> drivers/net/wireless/mediatek/mt76/mt7615/mac.c: In function mt7615_fill_txs:
> drivers/net/wireless/mediatek/mt76/mt7615/mac.c:555:5: warning: variable pid 
> set but not used [-Wunused-but-set-variable]
> drivers/net/wireless/mediatek/mt76/mt7615/mac.c:552:19: warning: variable 
> final_mpdu set but not used [-Wunused-but-set-variable]
> 
> They are never used, so can be removed.
> 
> Signed-off-by: YueHaibing 

Applied, thanks.

- Felix

Re: [PATCH v2] mt76: mt7615: add TX/RX antenna pattern capabilities

[Felix Fietkau]

On 2019-04-26 07:23, Ryder Lee wrote:
> Announce antenna pattern cap to adapt PHY and baseband settings.
> 
> Signed-off-by: Ryder Lee 
> ---
> Changes since v2:
> - Add a prefix mt76 in the title.
> ---
>  drivers/net/wireless/mediatek/mt76/mt7615/init.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/init.c 
> b/drivers/net/wireless/mediatek/mt76/mt7615/init.c
> index 3ab3ff553ef2..122f7a565540 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7615/init.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7615/init.c
> @@ -190,6 +190,8 @@ int mt7615_register_device(struct mt7615_dev *dev)
>   IEEE80211_VHT_CAP_SHORT_GI_160 |
>   IEEE80211_VHT_CAP_MAX_MPDU_LENGTH_11454 |
>   IEEE80211_VHT_CAP_MAX_A_MPDU_LENGTH_EXPONENT_MASK |
> + IEEE80211_VHT_CAP_RX_ANTENNA_PATTERN |
> + IEEE80211_VHT_CAP_TX_ANTENNA_PATTERN |
If I read the standard correctly, these flags indicate that the rx/tx
antenna pattern does NOT change during association.
Doesn't that mean that we should set it in mac80211.c instead, so that
it also applies to MT76x2?

Thanks,

- Felix

Re: [mt76/mt7603/mac] Question about missing variable assignment

[Felix Fietkau]

On 2019-03-02 22:10, Gustavo A. R. Silva wrote:
> Hi all,
> 
> The following piece of code in drivers/net/wireless/mediatek/mt76/mt7603/mac.c
> is missing a variable assignment before line 1058.  Notice that there
> is a potential execution path in which variable *i* is compared against
> magic number 15 at line 1075 without being initialized previously
> (this was reported by Coverity):
> 
> 1055 out:
> 1056 final_rate_flags = info->status.rates[final_idx].flags;
> 1057 
> 1058 switch (FIELD_GET(MT_TX_RATE_MODE, final_rate)) {
> 1059 case MT_PHY_TYPE_CCK:
> 1060 cck = true;
> 1061 /* fall through */
> 1062 case MT_PHY_TYPE_OFDM:
> 1063 if (dev->mt76.chandef.chan->band == NL80211_BAND_5GHZ)
> 1064 sband = >mt76.sband_5g.sband;
> 1065 else
> 1066 sband = >mt76.sband_2g.sband;
> 1067 final_rate &= GENMASK(5, 0);
> 1068 final_rate = mt7603_get_rate(dev, sband, final_rate, 
> cck);
> 1069 final_rate_flags = 0;
> 1070 break;
> 1071 case MT_PHY_TYPE_HT_GF:
> 1072 case MT_PHY_TYPE_HT:
> 1073 final_rate_flags |= IEEE80211_TX_RC_MCS;
> 1074 final_rate &= GENMASK(5, 0);
> 1075 if (i > 15)
> 1076 return false;
> 1077 break;
> 1078 default:
> 1079 return false;
> 1080 }
> 
> My guess is that such missing assignment should be something similar
> to the one at line 566:
> 
>   i = FIELD_GET(MT_RXV1_TX_RATE, rxdg0);
> 
> but I'm not sure what the proper arguments for macro FIELD_GET should
> be.
> 
> This code was introduced by commit c8846e1015022d2531ac4c895783e400b3e5babe
> 
> What do you think?
Thanks for reporting this. The fix is simpler than that, the check
should be: if (final_rate > 15)
I will send a fix.

- Felix

Re: [PATCH] mt76: change the retun type of mt76_dma_attach()

[Felix Fietkau]

On 2019-02-11 03:13, Ryder Lee wrote:
> There is no need to retun 0 in mt76_dma_attach(), so switch it to void.
> 
> Signed-off-by: Ryder Lee 
Applied, thanks.

- Felix

Re: [PATCH] mt76: convert to DEFINE_SHOW_ATTRIBUTE

[Felix Fietkau]

On 2018-12-03 14:40, Yangtao Li wrote:
> Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
> 
> Signed-off-by: Yangtao Li Please rebase this patch 
> onto the mt76 branch of
https://github.com/nbd168/wireless

Thanks,

- Felix

Re: [PATCH] mt76: make const array 'data' static, shrinks object size

[Felix Fietkau]

On 2018-12-30 14:26, Colin King wrote:
> From: Colin Ian King 
> 
> Don't populate the const array 'data' on the stack but instead
> make it static. Makes the object code smaller by 78 bytes:
> 
> Before:
>textdata bss dec hex filename
>54381080   065181976 mediatek/mt76/mt76x2/usb_mcu.o
> 
> After:
>textdata bss dec hex filename
>52961144   064401928 mediatek/mt76/mt76x2/usb_mcu.o
> 
> (gcc version 8.2.0 x86_64)
> 
> Signed-off-by: Colin Ian King 
Applied, thanks.

- Felix

Re: [PATCH 00/12] Ethernet: Add and use ether__addr globals

[Felix Fietkau]

On 2018-04-05 15:51, Joe Perches wrote:
>>  You have to factor in
>> not just the .text size, but the fact that referencing an exported
>> symbol needs a .reloc entry as well, which also eats up some space (at
>> least when the code is being built as module).
> 
> Thanks, the modules I built got smaller.
Please post some numbers to show this. By the way, on other
architectures the numbers will probably be different, especially on
ARM/MIPS.
>> In my opinion, your series probably causes more bloat in common
>> configurations instead of reducing it.
>> 
>> You're also touching several places that could easily use
>> eth_broadcast_addr and eth_zero_addr. I think making those changes would
>> be more productive than what you did in this series.
> 
> Doubtful. AFAIK: possible unaligned addresses.
Those two are just memset calls, alignment does not matter.

- Felix

Re: [PATCH 00/12] Ethernet: Add and use ether__addr globals

[Felix Fietkau]

On 2018-04-05 15:51, Joe Perches wrote:
>>  You have to factor in
>> not just the .text size, but the fact that referencing an exported
>> symbol needs a .reloc entry as well, which also eats up some space (at
>> least when the code is being built as module).
> 
> Thanks, the modules I built got smaller.
Please post some numbers to show this. By the way, on other
architectures the numbers will probably be different, especially on
ARM/MIPS.
>> In my opinion, your series probably causes more bloat in common
>> configurations instead of reducing it.
>> 
>> You're also touching several places that could easily use
>> eth_broadcast_addr and eth_zero_addr. I think making those changes would
>> be more productive than what you did in this series.
> 
> Doubtful. AFAIK: possible unaligned addresses.
Those two are just memset calls, alignment does not matter.

- Felix

Re: [PATCH 00/12] Ethernet: Add and use ether__addr globals

[Felix Fietkau]

On 2018-03-31 09:05, Joe Perches wrote:
> There are many local static and non-static arrays that are used for
> Ethernet broadcast address output or comparison.
> 
> Centralize the array into a single separate file and remove the local
> arrays.
I suspect that for many targets and configurations, the local arrays
might actually be smaller than exporting a global. You have to factor in
not just the .text size, but the fact that referencing an exported
symbol needs a .reloc entry as well, which also eats up some space (at
least when the code is being built as module).

In my opinion, your series probably causes more bloat in common
configurations instead of reducing it.

You're also touching several places that could easily use
eth_broadcast_addr and eth_zero_addr. I think making those changes would
be more productive than what you did in this series.

- Felix

Re: [PATCH 00/12] Ethernet: Add and use ether__addr globals

[Felix Fietkau]

On 2018-03-31 09:05, Joe Perches wrote:
> There are many local static and non-static arrays that are used for
> Ethernet broadcast address output or comparison.
> 
> Centralize the array into a single separate file and remove the local
> arrays.
I suspect that for many targets and configurations, the local arrays
might actually be smaller than exporting a global. You have to factor in
not just the .text size, but the fact that referencing an exported
symbol needs a .reloc entry as well, which also eats up some space (at
least when the code is being built as module).

In my opinion, your series probably causes more bloat in common
configurations instead of reducing it.

You're also touching several places that could easily use
eth_broadcast_addr and eth_zero_addr. I think making those changes would
be more productive than what you did in this series.

- Felix

[tip:timers/urgent] clocksource/drivers/mips-gic-timer: Use correct shift count to extract data

[tip-bot for Felix Fietkau]

Commit-ID:  5753405e27f8fe4c42c1537d3ddbd9e058e54cdc
Gitweb: https://git.kernel.org/tip/5753405e27f8fe4c42c1537d3ddbd9e058e54cdc
Author: Felix Fietkau <n...@nbd.name>
AuthorDate: Wed, 28 Feb 2018 10:56:10 +0100
Committer:  Thomas Gleixner <t...@linutronix.de>
CommitDate: Wed, 28 Feb 2018 13:55:14 +0100

clocksource/drivers/mips-gic-timer: Use correct shift count to extract data

__gic_clocksource_init() extracts the GIC_CONFIG_COUNTBITS field from
read_gic_config() by right shifting the register value. The shift count is
determined by the most significant bit (__fls) of the bitmask which is
wrong as it shifts out the complete bitfield.

Use the least significant bit (__ffs) instead to shift the bitfield down to
bit 0.

Fixes: e07127a077c7 ("clocksource: mips-gic-timer: Use new GIC accessor 
functions")
Signed-off-by: Felix Fietkau <n...@nbd.name>
Signed-off-by: Thomas Gleixner <t...@linutronix.de>
Cc: daniel.lezc...@linaro.org
Cc: paul.bur...@imgtec.com
Link: https://lkml.kernel.org/r/20180228095610.50341-1-...@nbd.name

---
 drivers/clocksource/mips-gic-timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/mips-gic-timer.c 
b/drivers/clocksource/mips-gic-timer.c
index 65e18c86d9b9..986b6796b631 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -166,7 +166,7 @@ static int __init __gic_clocksource_init(void)
 
/* Set clocksource mask. */
count_width = read_gic_config() & GIC_CONFIG_COUNTBITS;
-   count_width >>= __fls(GIC_CONFIG_COUNTBITS);
+   count_width >>= __ffs(GIC_CONFIG_COUNTBITS);
count_width *= 4;
count_width += 32;
gic_clocksource.mask = CLOCKSOURCE_MASK(count_width);

[tip:timers/urgent] clocksource/drivers/mips-gic-timer: Use correct shift count to extract data

[tip-bot for Felix Fietkau]

Commit-ID:  5753405e27f8fe4c42c1537d3ddbd9e058e54cdc
Gitweb: https://git.kernel.org/tip/5753405e27f8fe4c42c1537d3ddbd9e058e54cdc
Author: Felix Fietkau 
AuthorDate: Wed, 28 Feb 2018 10:56:10 +0100
Committer:  Thomas Gleixner 
CommitDate: Wed, 28 Feb 2018 13:55:14 +0100

clocksource/drivers/mips-gic-timer: Use correct shift count to extract data

__gic_clocksource_init() extracts the GIC_CONFIG_COUNTBITS field from
read_gic_config() by right shifting the register value. The shift count is
determined by the most significant bit (__fls) of the bitmask which is
wrong as it shifts out the complete bitfield.

Use the least significant bit (__ffs) instead to shift the bitfield down to
bit 0.

Fixes: e07127a077c7 ("clocksource: mips-gic-timer: Use new GIC accessor 
functions")
Signed-off-by: Felix Fietkau 
Signed-off-by: Thomas Gleixner 
Cc: daniel.lezc...@linaro.org
Cc: paul.bur...@imgtec.com
Link: https://lkml.kernel.org/r/20180228095610.50341-1-...@nbd.name

---
 drivers/clocksource/mips-gic-timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/mips-gic-timer.c 
b/drivers/clocksource/mips-gic-timer.c
index 65e18c86d9b9..986b6796b631 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -166,7 +166,7 @@ static int __init __gic_clocksource_init(void)
 
/* Set clocksource mask. */
count_width = read_gic_config() & GIC_CONFIG_COUNTBITS;
-   count_width >>= __fls(GIC_CONFIG_COUNTBITS);
+   count_width >>= __ffs(GIC_CONFIG_COUNTBITS);
count_width *= 4;
count_width += 32;
gic_clocksource.mask = CLOCKSOURCE_MASK(count_width);

[PATCH v2] clocksource: mips-gic-timer: fix clocksource counter width

[Felix Fietkau]

This code is trying to extract the GIC_CONFIG_COUNTBITS field from
read_gic_config(), so it needs to shift count_width right by the index
of the least significant bit (__ffs) instead of the most significant bit
(__fls) of the field mask.

Fixes: e07127a077c7 ("clocksource: mips-gic-timer: Use new GIC accessor 
functions")
Cc: Paul Burton <paul.bur...@imgtec.com>
Cc: sta...@vger.kernel.org
Signed-off-by: Felix Fietkau <n...@nbd.name>
---
 drivers/clocksource/mips-gic-timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/mips-gic-timer.c 
b/drivers/clocksource/mips-gic-timer.c
index a04808a21d4e..bf0eee78c6ef 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -166,7 +166,7 @@ static int __init __gic_clocksource_init(void)
 
/* Set clocksource mask. */
count_width = read_gic_config() & GIC_CONFIG_COUNTBITS;
-   count_width >>= __fls(GIC_CONFIG_COUNTBITS);
+   count_width >>= __ffs(GIC_CONFIG_COUNTBITS);
count_width *= 4;
count_width += 32;
gic_clocksource.mask = CLOCKSOURCE_MASK(count_width);
-- 
2.14.2

[PATCH v2] clocksource: mips-gic-timer: fix clocksource counter width

[Felix Fietkau]

This code is trying to extract the GIC_CONFIG_COUNTBITS field from
read_gic_config(), so it needs to shift count_width right by the index
of the least significant bit (__ffs) instead of the most significant bit
(__fls) of the field mask.

Fixes: e07127a077c7 ("clocksource: mips-gic-timer: Use new GIC accessor 
functions")
Cc: Paul Burton 
Cc: sta...@vger.kernel.org
Signed-off-by: Felix Fietkau 
---
 drivers/clocksource/mips-gic-timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/mips-gic-timer.c 
b/drivers/clocksource/mips-gic-timer.c
index a04808a21d4e..bf0eee78c6ef 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -166,7 +166,7 @@ static int __init __gic_clocksource_init(void)
 
/* Set clocksource mask. */
count_width = read_gic_config() & GIC_CONFIG_COUNTBITS;
-   count_width >>= __fls(GIC_CONFIG_COUNTBITS);
+   count_width >>= __ffs(GIC_CONFIG_COUNTBITS);
count_width *= 4;
count_width += 32;
gic_clocksource.mask = CLOCKSOURCE_MASK(count_width);
-- 
2.14.2

Re: [PATCH] clocksource: mips-gic-timer: fix clocksource counter width

[Felix Fietkau]

On 2018-02-28 09:56, Thomas Gleixner wrote:
> On Wed, 21 Feb 2018, Felix Fietkau wrote:
> 
>> This code needs to use ffs instead of fls on the mask to determine the
>> shift for reading the GIC_CONFIG_COUNTBITS field.
> 
> Why?
>>  count_width = read_gic_config() & GIC_CONFIG_COUNTBITS;
>> -count_width >>= __fls(GIC_CONFIG_COUNTBITS);
>> +count_width >>= __ffs(GIC_CONFIG_COUNTBITS);
This code is trying to extract the GIC_CONFIG_COUNTBITS field from
read_gic_config(), so it needs to shift count_width right by the index
of the least significant bit (__ffs) instead of the most significant bit
(__fls) of the field mask.

- Felix

Re: [PATCH] clocksource: mips-gic-timer: fix clocksource counter width

[Felix Fietkau]

On 2018-02-28 09:56, Thomas Gleixner wrote:
> On Wed, 21 Feb 2018, Felix Fietkau wrote:
> 
>> This code needs to use ffs instead of fls on the mask to determine the
>> shift for reading the GIC_CONFIG_COUNTBITS field.
> 
> Why?
>>  count_width = read_gic_config() & GIC_CONFIG_COUNTBITS;
>> -count_width >>= __fls(GIC_CONFIG_COUNTBITS);
>> +count_width >>= __ffs(GIC_CONFIG_COUNTBITS);
This code is trying to extract the GIC_CONFIG_COUNTBITS field from
read_gic_config(), so it needs to shift count_width right by the index
of the least significant bit (__ffs) instead of the most significant bit
(__fls) of the field mask.

- Felix

[PATCH] clocksource: mips-gic-timer: fix clocksource counter width

[Felix Fietkau]

This code needs to use ffs instead of fls on the mask to determine the
shift for reading the GIC_CONFIG_COUNTBITS field.

Fixes: e07127a077c7 ("clocksource: mips-gic-timer: Use new GIC accessor 
functions")
Cc: Paul Burton <paul.bur...@imgtec.com>
Cc: sta...@vger.kernel.org
Signed-off-by: Felix Fietkau <n...@nbd.name>
---
 drivers/clocksource/mips-gic-timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/mips-gic-timer.c 
b/drivers/clocksource/mips-gic-timer.c
index a04808a21d4e..bf0eee78c6ef 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -166,7 +166,7 @@ static int __init __gic_clocksource_init(void)
 
/* Set clocksource mask. */
count_width = read_gic_config() & GIC_CONFIG_COUNTBITS;
-   count_width >>= __fls(GIC_CONFIG_COUNTBITS);
+   count_width >>= __ffs(GIC_CONFIG_COUNTBITS);
count_width *= 4;
count_width += 32;
gic_clocksource.mask = CLOCKSOURCE_MASK(count_width);
-- 
2.14.2

[PATCH] clocksource: mips-gic-timer: fix clocksource counter width

[Felix Fietkau]

This code needs to use ffs instead of fls on the mask to determine the
shift for reading the GIC_CONFIG_COUNTBITS field.

Fixes: e07127a077c7 ("clocksource: mips-gic-timer: Use new GIC accessor 
functions")
Cc: Paul Burton 
Cc: sta...@vger.kernel.org
Signed-off-by: Felix Fietkau 
---
 drivers/clocksource/mips-gic-timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/mips-gic-timer.c 
b/drivers/clocksource/mips-gic-timer.c
index a04808a21d4e..bf0eee78c6ef 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -166,7 +166,7 @@ static int __init __gic_clocksource_init(void)
 
/* Set clocksource mask. */
count_width = read_gic_config() & GIC_CONFIG_COUNTBITS;
-   count_width >>= __fls(GIC_CONFIG_COUNTBITS);
+   count_width >>= __ffs(GIC_CONFIG_COUNTBITS);
count_width *= 4;
count_width += 32;
gic_clocksource.mask = CLOCKSOURCE_MASK(count_width);
-- 
2.14.2

Re: [PATCH] ath9k: turn on btcoex_enable as default

[Felix Fietkau]

On 2018-02-10 14:56, Kai Heng Feng wrote:
> 
>> On 9 Feb 2018, at 3:16 PM, Kalle Valo  wrote:
>> Sure, but we have to make sure that we don't create regressions on
>> existing systems. For example, did you test this with any system which
>> don't support btcoex? (just asking, haven't tested this myself)
> 
> No not really, but I will definitely test it.
> The only module I have that uses ath9k is Dell’s DW1707.
> How do I check if it support btcoex or not?
I just reviewed the code again, and I am sure that we cannot merge this
patch. Enabling the btcoex parameter makes the driver enable a whole
bunch of code starting timers, listening to some GPIOs, etc.

On non-btcoex systems, some of those GPIOs might be floating or even
connected to different things, which could cause a lot of undefined
behavior.

This is simply too big a risk, so there absolutely needs to be a
whitelist for systems that need this, otherwise it has to remain
disabled by default.

- Felix

Re: [PATCH] ath9k: turn on btcoex_enable as default

[Felix Fietkau]

On 2018-02-10 14:56, Kai Heng Feng wrote:
> 
>> On 9 Feb 2018, at 3:16 PM, Kalle Valo  wrote:
>> Sure, but we have to make sure that we don't create regressions on
>> existing systems. For example, did you test this with any system which
>> don't support btcoex? (just asking, haven't tested this myself)
> 
> No not really, but I will definitely test it.
> The only module I have that uses ath9k is Dell’s DW1707.
> How do I check if it support btcoex or not?
I just reviewed the code again, and I am sure that we cannot merge this
patch. Enabling the btcoex parameter makes the driver enable a whole
bunch of code starting timers, listening to some GPIOs, etc.

On non-btcoex systems, some of those GPIOs might be floating or even
connected to different things, which could cause a lot of undefined
behavior.

This is simply too big a risk, so there absolutely needs to be a
whitelist for systems that need this, otherwise it has to remain
disabled by default.

- Felix

Re: [PATCH] ath9k: turn on btcoex_enable as default

[Felix Fietkau]

On 2018-02-08 06:28, Kai-Heng Feng wrote:
> Without btcoex_enable, WiFi activies make both WiFi and Bluetooth
> unstable if there's a bluetooth connection.
> 
> Enable this option when bt_ant_diversity is disabled.
> 
> BugLink: https://bugs.launchpad.net/bugs/1746164
> Signed-off-by: Kai-Heng Feng 
I think this might cause regressions on devices that don't have
bluetooth. This probably either needs more EEPROM checks, or something
to selectively enable it only on affected platforms.

- Felix

Re: [PATCH] ath9k: turn on btcoex_enable as default

[Felix Fietkau]

On 2018-02-08 06:28, Kai-Heng Feng wrote:
> Without btcoex_enable, WiFi activies make both WiFi and Bluetooth
> unstable if there's a bluetooth connection.
> 
> Enable this option when bt_ant_diversity is disabled.
> 
> BugLink: https://bugs.launchpad.net/bugs/1746164
> Signed-off-by: Kai-Heng Feng 
I think this might cause regressions on devices that don't have
bluetooth. This probably either needs more EEPROM checks, or something
to selectively enable it only on affected platforms.

- Felix

Re: [PATCH] [wireless-next] mt76: fix building without CONFIG_LEDS_CLASS

[Felix Fietkau]

On 2018-01-18 14:14, Arnd Bergmann wrote:
> When CONFIG_LEDS_CLASS is disabled, or it is a loadable module while
> mt76 is built-in, we run into a link error:
> 
> drivers/net/wireless/mediatek/mt76/mac80211.o: In function 
> `mt76_register_device':
> mac80211.c:(.text+0xb78): relocation truncated to fit: R_AARCH64_CALL26 
> against undefined symbol `devm_of_led_classdev_register'
> 
> We don't really need a hard dependency here as the driver can presumably
> work just fine without LEDs, so this follows the iwlwifi example and
> adds a separate Kconfig option for the LED support, this will be available
> whenever it will link, and otherwise the respective code gets left out from
> the driver object.
> 
> Fixes: 17f1de56df05 ("mt76: add common code shared between multiple chipsets")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/net/wireless/mediatek/mt76/Kconfig   | 5 +
>  drivers/net/wireless/mediatek/mt76/mac80211.c| 8 +---
>  drivers/net/wireless/mediatek/mt76/mt76x2_init.c | 6 --
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/Kconfig 
> b/drivers/net/wireless/mediatek/mt76/Kconfig
> index fc05d79c80d0..9d12c1f5284e 100644
> --- a/drivers/net/wireless/mediatek/mt76/Kconfig
> +++ b/drivers/net/wireless/mediatek/mt76/Kconfig
> @@ -8,3 +8,8 @@ config MT76x2E
>   depends on PCI
>   ---help---
> This adds support for MT7612/MT7602/MT7662-based wireless PCIe 
> devices.
> +
> +config MT76_LEDS
> + bool "LED support for MT76"
> + depends on MT76_CORE
> + depends on LEDS_CLASS=y || MT76_CORE=LEDS_CLASS
I think this should have a 'default y'

- Felix

Re: [PATCH] [wireless-next] mt76: fix building without CONFIG_LEDS_CLASS

[Felix Fietkau]

On 2018-01-18 14:14, Arnd Bergmann wrote:
> When CONFIG_LEDS_CLASS is disabled, or it is a loadable module while
> mt76 is built-in, we run into a link error:
> 
> drivers/net/wireless/mediatek/mt76/mac80211.o: In function 
> `mt76_register_device':
> mac80211.c:(.text+0xb78): relocation truncated to fit: R_AARCH64_CALL26 
> against undefined symbol `devm_of_led_classdev_register'
> 
> We don't really need a hard dependency here as the driver can presumably
> work just fine without LEDs, so this follows the iwlwifi example and
> adds a separate Kconfig option for the LED support, this will be available
> whenever it will link, and otherwise the respective code gets left out from
> the driver object.
> 
> Fixes: 17f1de56df05 ("mt76: add common code shared between multiple chipsets")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/net/wireless/mediatek/mt76/Kconfig   | 5 +
>  drivers/net/wireless/mediatek/mt76/mac80211.c| 8 +---
>  drivers/net/wireless/mediatek/mt76/mt76x2_init.c | 6 --
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/Kconfig 
> b/drivers/net/wireless/mediatek/mt76/Kconfig
> index fc05d79c80d0..9d12c1f5284e 100644
> --- a/drivers/net/wireless/mediatek/mt76/Kconfig
> +++ b/drivers/net/wireless/mediatek/mt76/Kconfig
> @@ -8,3 +8,8 @@ config MT76x2E
>   depends on PCI
>   ---help---
> This adds support for MT7612/MT7602/MT7662-based wireless PCIe 
> devices.
> +
> +config MT76_LEDS
> + bool "LED support for MT76"
> + depends on MT76_CORE
> + depends on LEDS_CLASS=y || MT76_CORE=LEDS_CLASS
I think this should have a 'default y'

- Felix

Re: [PATCH 1/3] kallsyms: don't leak address when symbol not found

[Felix Fietkau]

On 2017-12-18 00:53, Tobin C. Harding wrote:
> Currently if kallsyms_lookup() fails to find the symbol then the address
> is printed. This potentially leaks sensitive information. Instead of
> printing the address we can return an error, giving the calling code the
> option to print the address or print some sanitized message.
> 
> Return error instead of printing address to argument buffer. Leave
> buffer in a sane state.
> 
> Signed-off-by: Tobin C. Harding 
I think there should be a way to keep the old behavior for debugging.

- Felix

Re: [PATCH 1/3] kallsyms: don't leak address when symbol not found

[Felix Fietkau]

On 2017-12-18 00:53, Tobin C. Harding wrote:
> Currently if kallsyms_lookup() fails to find the symbol then the address
> is printed. This potentially leaks sensitive information. Instead of
> printing the address we can return an error, giving the calling code the
> option to print the address or print some sanitized message.
> 
> Return error instead of printing address to argument buffer. Leave
> buffer in a sane state.
> 
> Signed-off-by: Tobin C. Harding 
I think there should be a way to keep the old behavior for debugging.

- Felix

Re: [PATCH] mt76: fix memcpy to potential null pointer on failed allocation

[Felix Fietkau]

On 2017-12-14 11:13, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> Currently if the allocation of skb fails and returns NULL then the
> call to skb_put will cause a null pointer dereference. Fix this by
> checking for a null skb and returning NULL.  Note that calls to
> function mt76x2_mcu_msg_alloc don't directly check the null return
> but instead pass the NULL pointer to mt76x2_mcu_msg_send which
> checks for the NULL and returns ENOMEM in this case.
> 
> Detected by CoverityScan, CID#1462624 ("Dereference null return value")
> 
> Fixes: 7bc04215a66b ("mt76: add driver code for MT76x2e")
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
Acked-by: Felix Fietkau <n...@nbd.name>

Re: [PATCH] mt76: fix memcpy to potential null pointer on failed allocation

[Felix Fietkau]

On 2017-12-14 11:13, Colin King wrote:
> From: Colin Ian King 
> 
> Currently if the allocation of skb fails and returns NULL then the
> call to skb_put will cause a null pointer dereference. Fix this by
> checking for a null skb and returning NULL.  Note that calls to
> function mt76x2_mcu_msg_alloc don't directly check the null return
> but instead pass the NULL pointer to mt76x2_mcu_msg_send which
> checks for the NULL and returns ENOMEM in this case.
> 
> Detected by CoverityScan, CID#1462624 ("Dereference null return value")
> 
> Fixes: 7bc04215a66b ("mt76: add driver code for MT76x2e")
> Signed-off-by: Colin Ian King 
Acked-by: Felix Fietkau 

Re: [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[Felix Fietkau]

On 2017-12-07 07:06, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> MT7530 can treat each port as either VLAN-unware port or VLAN-ware port
Shouldn't that be VLAN-unaware/VLAN-aware (in the code as well)?

- Felix

Re: [PATCH net-next 1/3] net: dsa: mediatek: add VLAN support for MT7530

[Felix Fietkau]

On 2017-12-07 07:06, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> MT7530 can treat each port as either VLAN-unware port or VLAN-ware port
Shouldn't that be VLAN-unaware/VLAN-aware (in the code as well)?

- Felix

Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy

[Felix Fietkau]

On 2017-03-27 12:47, Johannes Berg wrote:
> On Fri, 2017-03-24 at 18:06 -0700, Matthias Kaehlcke wrote:
>> __ieee80211_amsdu_copy_frag intentionally initializes a pointer to
>> array[-1] to increment it later to valid values. clang rightfully
>> generates an array-bounds warning on the initialization statement.
>> Work around this by initializing the pointer to array[0] and
>> decrementing it later, which allows to leave the rest of the
>> algorithm untouched.
>> 
>> Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
>> ---
>>  net/wireless/util.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/wireless/util.c b/net/wireless/util.c
>> index 68e5f2ecee1a..d3d459e4a070 100644
>> --- a/net/wireless/util.c
>> +++ b/net/wireless/util.c
>> @@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
>> struct sk_buff *frame,
>>  int offset, int len)
>>  {
>>  struct skb_shared_info *sh = skb_shinfo(skb);
>> -const skb_frag_t *frag = >frags[-1];
>> +const skb_frag_t *frag = >frags[0];
>>  struct page *frag_page;
>>  void *frag_ptr;
>>  int frag_len, frag_size;
>> @@ -669,6 +669,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
>> struct sk_buff *frame,
>>  frag_page = virt_to_head_page(skb->head);
>>  frag_ptr = skb->data;
>>  frag_size = head_size;
>> +frag--;
> 
> Isn't it just a question of time until the compiler will see through
> this trick and warn about it?
Frag is incremented again before being accessed, so there is nothing for
the compiler to see through here.

Acked-by: Felix Fietkau <n...@nbd.name>

- Felix

Re: [PATCH] cfg80211: Fix array-bounds warning in fragment copy

[Felix Fietkau]

On 2017-03-27 12:47, Johannes Berg wrote:
> On Fri, 2017-03-24 at 18:06 -0700, Matthias Kaehlcke wrote:
>> __ieee80211_amsdu_copy_frag intentionally initializes a pointer to
>> array[-1] to increment it later to valid values. clang rightfully
>> generates an array-bounds warning on the initialization statement.
>> Work around this by initializing the pointer to array[0] and
>> decrementing it later, which allows to leave the rest of the
>> algorithm untouched.
>> 
>> Signed-off-by: Matthias Kaehlcke 
>> ---
>>  net/wireless/util.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/wireless/util.c b/net/wireless/util.c
>> index 68e5f2ecee1a..d3d459e4a070 100644
>> --- a/net/wireless/util.c
>> +++ b/net/wireless/util.c
>> @@ -659,7 +659,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
>> struct sk_buff *frame,
>>  int offset, int len)
>>  {
>>  struct skb_shared_info *sh = skb_shinfo(skb);
>> -const skb_frag_t *frag = >frags[-1];
>> +const skb_frag_t *frag = >frags[0];
>>  struct page *frag_page;
>>  void *frag_ptr;
>>  int frag_len, frag_size;
>> @@ -669,6 +669,7 @@ __ieee80211_amsdu_copy_frag(struct sk_buff *skb,
>> struct sk_buff *frame,
>>  frag_page = virt_to_head_page(skb->head);
>>  frag_ptr = skb->data;
>>  frag_size = head_size;
>> +frag--;
> 
> Isn't it just a question of time until the compiler will see through
> this trick and warn about it?
Frag is incremented again before being accessed, so there is nothing for
the compiler to see through here.

Acked-by: Felix Fietkau 

- Felix

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Felix Fietkau]

On 2017-03-23 15:25, John Crispin wrote:
> 
> 
> On 23/03/17 15:09, Felix Fietkau wrote:
>> On 2017-03-23 09:06, Sean Wang wrote:
>>> Hi Andrew,
>>>
>>> The purpose for the regmap table registered is to
>>>
>>> provide a way which helps us to look up a specific
>>>
>>> register on the switch through regmap-debugfs.
>>>
>>>
>>> And not all ranges of register is defined
>>>
>>> so I only include the meaningful ones in a sparse way
>>>
>>> for the table.
>> I think in that case it might be nice to make regmap support optional in
>> order to avoid pulling in bloat on platforms that don't need it.
>>
>> - Felix
>>
> The 2 relevant platforms are mips/ralink and arm/mediatek. both require 
> regmap for the eth_sysctl syscon if they want to utilize the mtk_soc_eth 
> driver which is a prereq for mt7530. so regmap cannot be optional here.
Makes sense, thanks.

- Felix

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Felix Fietkau]

On 2017-03-23 15:25, John Crispin wrote:
> 
> 
> On 23/03/17 15:09, Felix Fietkau wrote:
>> On 2017-03-23 09:06, Sean Wang wrote:
>>> Hi Andrew,
>>>
>>> The purpose for the regmap table registered is to
>>>
>>> provide a way which helps us to look up a specific
>>>
>>> register on the switch through regmap-debugfs.
>>>
>>>
>>> And not all ranges of register is defined
>>>
>>> so I only include the meaningful ones in a sparse way
>>>
>>> for the table.
>> I think in that case it might be nice to make regmap support optional in
>> order to avoid pulling in bloat on platforms that don't need it.
>>
>> - Felix
>>
> The 2 relevant platforms are mips/ralink and arm/mediatek. both require 
> regmap for the eth_sysctl syscon if they want to utilize the mtk_soc_eth 
> driver which is a prereq for mt7530. so regmap cannot be optional here.
Makes sense, thanks.

- Felix

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Felix Fietkau]

On 2017-03-23 09:06, Sean Wang wrote:
> Hi Andrew,
> 
> The purpose for the regmap table registered is to 
> 
> provide a way which helps us to look up a specific 
> 
> register on the switch through regmap-debugfs.
> 
> 
> And not all ranges of register is defined
> 
> so I only include the meaningful ones in a sparse way 
> 
> for the table.
I think in that case it might be nice to make regmap support optional in
order to avoid pulling in bloat on platforms that don't need it.

- Felix

Re: [PATCH net-next v2 5/5] net-next: dsa: add dsa support for Mediatek MT7530 switch

[Felix Fietkau]

On 2017-03-23 09:06, Sean Wang wrote:
> Hi Andrew,
> 
> The purpose for the regmap table registered is to 
> 
> provide a way which helps us to look up a specific 
> 
> register on the switch through regmap-debugfs.
> 
> 
> And not all ranges of register is defined
> 
> so I only include the meaningful ones in a sparse way 
> 
> for the table.
I think in that case it might be nice to make regmap support optional in
order to avoid pulling in bloat on platforms that don't need it.

- Felix

[PATCH] kbuild: export SUBARCH

[Felix Fietkau]

Fixes the following build error on x86:

  gcc -Wp,-MD,arch/x86/entry/vdso/.vdso2c.d -O2 
-I/Users/nbd/lede/staging_dir/host/include 
-I/Users/nbd/lede/staging_dir/host/usr/include  -Wall -Wmissing-prototypes 
-Wstrict-prototypes   -I/Users/nbd/lede/staging_dir/host/include 
-I./tools/include -I./include/uapi -I./arch//include/uapi  -o 
arch/x86/entry/vdso/vdso2c arch/x86/entry/vdso/vdso2c.c 
-L/Users/nbd/lede/staging_dir/host/lib
  In file included from arch/x86/entry/vdso/vdso2c.c:66:
  In file included from ./include/uapi/linux/elf.h:4:
  ./tools/include/linux/types.h:9:10: fatal error: 'asm/types.h' file not found
  #include 

Fixes: d51306f1a3bc ("x86: Make the vdso2c compiler use the host architecture 
headers")
Signed-off-by: Felix Fietkau <n...@nbd.name>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 96b27a888285..492143bad81e 100644
--- a/Makefile
+++ b/Makefile
@@ -414,8 +414,8 @@ KERNELRELEASE = $(shell cat include/config/kernel.release 
2> /dev/null)
 KERNELVERSION = $(VERSION)$(if $(PATCHLEVEL),.$(PATCHLEVEL)$(if 
$(SUBLEVEL),.$(SUBLEVEL)))$(EXTRAVERSION)
 
 export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION
-export ARCH SRCARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
-export CPP AR NM STRIP OBJCOPY OBJDUMP
+export ARCH SRCARCH SUBARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD
+export CC CPP AR NM STRIP OBJCOPY OBJDUMP
 export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
 export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
 
-- 
2.11.0

[PATCH] kbuild: export SUBARCH

[Felix Fietkau]

Fixes the following build error on x86:

  gcc -Wp,-MD,arch/x86/entry/vdso/.vdso2c.d -O2 
-I/Users/nbd/lede/staging_dir/host/include 
-I/Users/nbd/lede/staging_dir/host/usr/include  -Wall -Wmissing-prototypes 
-Wstrict-prototypes   -I/Users/nbd/lede/staging_dir/host/include 
-I./tools/include -I./include/uapi -I./arch//include/uapi  -o 
arch/x86/entry/vdso/vdso2c arch/x86/entry/vdso/vdso2c.c 
-L/Users/nbd/lede/staging_dir/host/lib
  In file included from arch/x86/entry/vdso/vdso2c.c:66:
  In file included from ./include/uapi/linux/elf.h:4:
  ./tools/include/linux/types.h:9:10: fatal error: 'asm/types.h' file not found
  #include 

Fixes: d51306f1a3bc ("x86: Make the vdso2c compiler use the host architecture 
headers")
Signed-off-by: Felix Fietkau 
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 96b27a888285..492143bad81e 100644
--- a/Makefile
+++ b/Makefile
@@ -414,8 +414,8 @@ KERNELRELEASE = $(shell cat include/config/kernel.release 
2> /dev/null)
 KERNELVERSION = $(VERSION)$(if $(PATCHLEVEL),.$(PATCHLEVEL)$(if 
$(SUBLEVEL),.$(SUBLEVEL)))$(EXTRAVERSION)
 
 export VERSION PATCHLEVEL SUBLEVEL KERNELRELEASE KERNELVERSION
-export ARCH SRCARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD CC
-export CPP AR NM STRIP OBJCOPY OBJDUMP
+export ARCH SRCARCH SUBARCH CONFIG_SHELL HOSTCC HOSTCFLAGS CROSS_COMPILE AS LD
+export CC CPP AR NM STRIP OBJCOPY OBJDUMP
 export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE
 export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS
 
-- 
2.11.0

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-11 13:15, IgorMitsyanko wrote:
> On 01/11/2017 02:30 PM, Felix Fietkau wrote:
>> On 2017-01-11 12:26, IgorMitsyanko wrote:
>>> On 01/11/2017 12:27 AM, Felix Fietkau wrote:
>>>> On 2017-01-10 11:56, Johannes Berg wrote:
>>>>> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
>>>>>> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
>>>>>>> I wonder if MAC80211 should be doing IGMP snooping and not bridge
>>>>>>> in this environment.
>>>>>> In the long term, yes. For now, not quite sure.
>>>>> There's no "for now" in the kernel. Code added now will have to be
>>>>> maintained essentially forever.
>>>> I'm not sure that putting the IGMP snooping code in mac80211 is a good
>>>> idea, that would be quite a bit of code duplication.
>>>> This implementation works, it's very simple, and it's quite flexible for
>>>> a number of use cases.
>>>>
>>>> Is there any remaining objection to merging this in principle (aside
>>>> from potential issues with the code)?
>>>>
>>>> - Felix
>>>>
>>>
>>> Hi Felix, can we consider two examples configurations with multicast
>>> traffic:
>>>
>>> 1. AP is a source of multicast traffic itself, no bridge on AP. For
>>> example, wireless video server streaming to several clients.
>>> In this situation, we can not make use of possible advantages given by
>>> mc-to-uc conversion?
>> You could simply put the AP interface in a bridge, no need to have any
>> other bridge members present.
>>
>>> 2. A configuration with AP + STA + 3 client devices behind STA.
>>>   |client 1|
>>>  |
>>> |  mc  ||AP||STA|---|---|client 2|
>>> |server||
>>>   |client 3|
>>>
>>> Multicast server behind AP streams MC video traffic. All 3 clients
>>> behind the STA have joined the multicast group.
>>> I'm not sure if this case will be handled correctly with mc-to-uc
>>> conversion in bridge on AP?
>> What do you mean by "3 client devices behind STA"? Are you using a
>> 4-addr STA, multicast routing, or some kind of vendor specific "client
>> bridge" hackery?
> 
> 3 client devices connected by backbone Ethernet network. Generic
> case is probably STA/AP operating in 4-addr mode (more or less standard
> solution as far as I know).
If the AP is running in 4-addr mode, it will need to have a bridge
interface anyway, because the link to the STA will be split out into a
separate virtual interface (AP_VLAN iftype).

In this case you don't actually need any multicast-to-unicast
conversion, because the multicast traffic will be unicast on 802.11
already (due to use of 4-addr mode).

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-11 13:15, IgorMitsyanko wrote:
> On 01/11/2017 02:30 PM, Felix Fietkau wrote:
>> On 2017-01-11 12:26, IgorMitsyanko wrote:
>>> On 01/11/2017 12:27 AM, Felix Fietkau wrote:
>>>> On 2017-01-10 11:56, Johannes Berg wrote:
>>>>> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
>>>>>> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
>>>>>>> I wonder if MAC80211 should be doing IGMP snooping and not bridge
>>>>>>> in this environment.
>>>>>> In the long term, yes. For now, not quite sure.
>>>>> There's no "for now" in the kernel. Code added now will have to be
>>>>> maintained essentially forever.
>>>> I'm not sure that putting the IGMP snooping code in mac80211 is a good
>>>> idea, that would be quite a bit of code duplication.
>>>> This implementation works, it's very simple, and it's quite flexible for
>>>> a number of use cases.
>>>>
>>>> Is there any remaining objection to merging this in principle (aside
>>>> from potential issues with the code)?
>>>>
>>>> - Felix
>>>>
>>>
>>> Hi Felix, can we consider two examples configurations with multicast
>>> traffic:
>>>
>>> 1. AP is a source of multicast traffic itself, no bridge on AP. For
>>> example, wireless video server streaming to several clients.
>>> In this situation, we can not make use of possible advantages given by
>>> mc-to-uc conversion?
>> You could simply put the AP interface in a bridge, no need to have any
>> other bridge members present.
>>
>>> 2. A configuration with AP + STA + 3 client devices behind STA.
>>>   |client 1|
>>>  |
>>> |  mc  ||AP||STA|---|---|client 2|
>>> |server||
>>>   |client 3|
>>>
>>> Multicast server behind AP streams MC video traffic. All 3 clients
>>> behind the STA have joined the multicast group.
>>> I'm not sure if this case will be handled correctly with mc-to-uc
>>> conversion in bridge on AP?
>> What do you mean by "3 client devices behind STA"? Are you using a
>> 4-addr STA, multicast routing, or some kind of vendor specific "client
>> bridge" hackery?
> 
> 3 client devices connected by backbone Ethernet network. Generic
> case is probably STA/AP operating in 4-addr mode (more or less standard
> solution as far as I know).
If the AP is running in 4-addr mode, it will need to have a bridge
interface anyway, because the link to the STA will be split out into a
separate virtual interface (AP_VLAN iftype).

In this case you don't actually need any multicast-to-unicast
conversion, because the multicast traffic will be unicast on 802.11
already (due to use of 4-addr mode).

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-11 12:26, IgorMitsyanko wrote:
> On 01/11/2017 12:27 AM, Felix Fietkau wrote:
>> On 2017-01-10 11:56, Johannes Berg wrote:
>>> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
>>>> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
>>>>> I wonder if MAC80211 should be doing IGMP snooping and not bridge
>>>>> in this environment.
>>>>
>>>> In the long term, yes. For now, not quite sure.
>>>
>>> There's no "for now" in the kernel. Code added now will have to be
>>> maintained essentially forever.
>> I'm not sure that putting the IGMP snooping code in mac80211 is a good
>> idea, that would be quite a bit of code duplication.
>> This implementation works, it's very simple, and it's quite flexible for
>> a number of use cases.
>>
>> Is there any remaining objection to merging this in principle (aside
>> from potential issues with the code)?
>>
>> - Felix
>>
> 
> 
> Hi Felix, can we consider two examples configurations with multicast 
> traffic:
> 
> 1. AP is a source of multicast traffic itself, no bridge on AP. For 
> example, wireless video server streaming to several clients.
> In this situation, we can not make use of possible advantages given by 
> mc-to-uc conversion?
You could simply put the AP interface in a bridge, no need to have any
other bridge members present.

> 2. A configuration with AP + STA + 3 client devices behind STA.
>  |client 1|
>  |
> |  mc  ||AP||STA|---|---|client 2|
> |server||
>  |client 3|
> 
> Multicast server behind AP streams MC video traffic. All 3 clients 
> behind the STA have joined the multicast group.
> I'm not sure if this case will be handled correctly with mc-to-uc 
> conversion in bridge on AP?
What do you mean by "3 client devices behind STA"? Are you using a
4-addr STA, multicast routing, or some kind of vendor specific "client
bridge" hackery?

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-11 12:26, IgorMitsyanko wrote:
> On 01/11/2017 12:27 AM, Felix Fietkau wrote:
>> On 2017-01-10 11:56, Johannes Berg wrote:
>>> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
>>>> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
>>>>> I wonder if MAC80211 should be doing IGMP snooping and not bridge
>>>>> in this environment.
>>>>
>>>> In the long term, yes. For now, not quite sure.
>>>
>>> There's no "for now" in the kernel. Code added now will have to be
>>> maintained essentially forever.
>> I'm not sure that putting the IGMP snooping code in mac80211 is a good
>> idea, that would be quite a bit of code duplication.
>> This implementation works, it's very simple, and it's quite flexible for
>> a number of use cases.
>>
>> Is there any remaining objection to merging this in principle (aside
>> from potential issues with the code)?
>>
>> - Felix
>>
> 
> 
> Hi Felix, can we consider two examples configurations with multicast 
> traffic:
> 
> 1. AP is a source of multicast traffic itself, no bridge on AP. For 
> example, wireless video server streaming to several clients.
> In this situation, we can not make use of possible advantages given by 
> mc-to-uc conversion?
You could simply put the AP interface in a bridge, no need to have any
other bridge members present.

> 2. A configuration with AP + STA + 3 client devices behind STA.
>  |client 1|
>  |
> |  mc  ||AP||STA|---|---|client 2|
> |server||
>  |client 3|
> 
> Multicast server behind AP streams MC video traffic. All 3 clients 
> behind the STA have joined the multicast group.
> I'm not sure if this case will be handled correctly with mc-to-uc 
> conversion in bridge on AP?
What do you mean by "3 client devices behind STA"? Are you using a
4-addr STA, multicast routing, or some kind of vendor specific "client
bridge" hackery?

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-10 11:56, Johannes Berg wrote:
> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
>> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
>> > I wonder if MAC80211 should be doing IGMP snooping and not bridge
>> > in this environment.
>> 
>> In the long term, yes. For now, not quite sure.
> 
> There's no "for now" in the kernel. Code added now will have to be
> maintained essentially forever.
I'm not sure that putting the IGMP snooping code in mac80211 is a good
idea, that would be quite a bit of code duplication.
This implementation works, it's very simple, and it's quite flexible for
a number of use cases.

Is there any remaining objection to merging this in principle (aside
from potential issues with the code)?

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-10 11:56, Johannes Berg wrote:
> On Tue, 2017-01-10 at 05:18 +0100, Linus Lüssing wrote:
>> On Mon, Jan 09, 2017 at 01:30:32PM -0800, Stephen Hemminger wrote:
>> > I wonder if MAC80211 should be doing IGMP snooping and not bridge
>> > in this environment.
>> 
>> In the long term, yes. For now, not quite sure.
> 
> There's no "for now" in the kernel. Code added now will have to be
> maintained essentially forever.
I'm not sure that putting the IGMP snooping code in mac80211 is a good
idea, that would be quite a bit of code duplication.
This implementation works, it's very simple, and it's quite flexible for
a number of use cases.

Is there any remaining objection to merging this in principle (aside
from potential issues with the code)?

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-10 18:17, Dave Taht wrote:
> In the case of wifi I have 3 issues with this line of thought.
> 
> multicast in wifi has generally supposed to be unreliable. This makes
> it reliable. reliability comes at a cost -
> 
> multicast is typically set at a fixed low rate today. unicast is
> retried at different rates until it succeeds - for every station
> listening. If one station is already at the lowest rate, the total
> cost of the transmit increases, rather than decreases.
> 
> unicast gets block acks until it succeeds. Again, more delay.
> 
> I think there is something like 31 soft-retries in the ath9k driver
If I remember correctly, hardware retries are counted here as well.

> what happens to diffserv markings here? for unicast CS1 goes into the
> BE queue, CS6, the VO queue. Do we go from one flat queue for all of
> multicast to punching it through one of the hardware queues based on
> the diffserv mark now with this patch?
> 
> I would like it if there was a way to preserve the unreliability
> (which multiple mesh protocols depend on), send stuff with QoSNoack,
> etc - or dynamically choose (based on the rates of the stations)
> between conventional multicast and unicast.
> 
> Or - better, IMHO, keep sending multicast as is but pick the best of
> the rates available to all the listening stations for it.
The advantage of the multicast-to-unicast conversion goes beyond simply
selecting a better rate - aggregation matters a lot as well, and that is
simply incompatible with normal multicast.

Some multicast streams use lots of small-ish packets, the airtime impact
of those is vastly reduced, even if the transmission has to be
duplicated for a few stations.

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-10 18:17, Dave Taht wrote:
> In the case of wifi I have 3 issues with this line of thought.
> 
> multicast in wifi has generally supposed to be unreliable. This makes
> it reliable. reliability comes at a cost -
> 
> multicast is typically set at a fixed low rate today. unicast is
> retried at different rates until it succeeds - for every station
> listening. If one station is already at the lowest rate, the total
> cost of the transmit increases, rather than decreases.
> 
> unicast gets block acks until it succeeds. Again, more delay.
> 
> I think there is something like 31 soft-retries in the ath9k driver
If I remember correctly, hardware retries are counted here as well.

> what happens to diffserv markings here? for unicast CS1 goes into the
> BE queue, CS6, the VO queue. Do we go from one flat queue for all of
> multicast to punching it through one of the hardware queues based on
> the diffserv mark now with this patch?
> 
> I would like it if there was a way to preserve the unreliability
> (which multiple mesh protocols depend on), send stuff with QoSNoack,
> etc - or dynamically choose (based on the rates of the stations)
> between conventional multicast and unicast.
> 
> Or - better, IMHO, keep sending multicast as is but pick the best of
> the rates available to all the listening stations for it.
The advantage of the multicast-to-unicast conversion goes beyond simply
selecting a better rate - aggregation matters a lot as well, and that is
simply incompatible with normal multicast.

Some multicast streams use lots of small-ish packets, the airtime impact
of those is vastly reduced, even if the transmission has to be
duplicated for a few stations.

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-06 14:54, Johannes Berg wrote:
> 
>> The bridge layer can use IGMP snooping to ensure that the multicast
>> stream is only transmitted to clients that are actually a member of
>> the group. Can the mac80211 feature do the same?
> 
> No, it'll convert the packet for all clients that are behind that
> netdev. But that's an argument for dropping the mac80211 feature, which
> hasn't been merged upstream yet, no?
Right.

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-06 14:54, Johannes Berg wrote:
> 
>> The bridge layer can use IGMP snooping to ensure that the multicast
>> stream is only transmitted to clients that are actually a member of
>> the group. Can the mac80211 feature do the same?
> 
> No, it'll convert the packet for all clients that are behind that
> netdev. But that's an argument for dropping the mac80211 feature, which
> hasn't been merged upstream yet, no?
Right.

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-06 13:47, Johannes Berg wrote:
> On Mon, 2017-01-02 at 20:32 +0100, Linus Lüssing wrote:
>> Implements an optional, per bridge port flag and feature to deliver
>> multicast packets to any host on the according port via unicast
>> individually. This is done by copying the packet per host and
>> changing the multicast destination MAC to a unicast one accordingly.
> 
> How does this compare and/or relate to the multicast-to-unicast feature
> we were going to add to the wifi stack, particularly mac80211? Do we
> perhaps not need that feature at all, if bridging will have it?
> 
> I suppose that the feature there could apply also to locally generated
> traffic when the AP interface isn't in a bridge, but I think I could
> live with requiring the AP to be put into a bridge to achieve a similar
> configuration?
> 
> Additionally, on an unrelated note, this seems to apply generically to
> all kinds of frames, losing information by replacing the address.
> Shouldn't it have similar limitations as the wifi stack feature has
> then, like only applying to ARP, IPv4, IPv6 and not general protocols?
> 
> Also, it should probably come with the same caveat as we documented for
> the wifi feature:
> 
> Note that this may break certain expectations of the receiver,
> such as the ability to drop unicast IP packets received within
> multicast L2 frames, or the ability to not send ICMP destination
> unreachable messages for packets received in L2 multicast (which
> is required, but the receiver can't tell the difference if this
> new option is enabled.)
> 
> 
> I'll hold off sending my tree in until we see that we really need both
> features, or decide that we want the wifi feature *instead* of the
> bridge feature.
The bridge layer can use IGMP snooping to ensure that the multicast
stream is only transmitted to clients that are actually a member of the
group. Can the mac80211 feature do the same?

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-06 13:47, Johannes Berg wrote:
> On Mon, 2017-01-02 at 20:32 +0100, Linus Lüssing wrote:
>> Implements an optional, per bridge port flag and feature to deliver
>> multicast packets to any host on the according port via unicast
>> individually. This is done by copying the packet per host and
>> changing the multicast destination MAC to a unicast one accordingly.
> 
> How does this compare and/or relate to the multicast-to-unicast feature
> we were going to add to the wifi stack, particularly mac80211? Do we
> perhaps not need that feature at all, if bridging will have it?
> 
> I suppose that the feature there could apply also to locally generated
> traffic when the AP interface isn't in a bridge, but I think I could
> live with requiring the AP to be put into a bridge to achieve a similar
> configuration?
> 
> Additionally, on an unrelated note, this seems to apply generically to
> all kinds of frames, losing information by replacing the address.
> Shouldn't it have similar limitations as the wifi stack feature has
> then, like only applying to ARP, IPv4, IPv6 and not general protocols?
> 
> Also, it should probably come with the same caveat as we documented for
> the wifi feature:
> 
> Note that this may break certain expectations of the receiver,
> such as the ability to drop unicast IP packets received within
> multicast L2 frames, or the ability to not send ICMP destination
> unreachable messages for packets received in L2 multicast (which
> is required, but the receiver can't tell the difference if this
> new option is enabled.)
> 
> 
> I'll hold off sending my tree in until we see that we really need both
> features, or decide that we want the wifi feature *instead* of the
> bridge feature.
The bridge layer can use IGMP snooping to ensure that the multicast
stream is only transmitted to clients that are actually a member of the
group. Can the mac80211 feature do the same?

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-02 20:32, Linus Lüssing wrote:
> Implements an optional, per bridge port flag and feature to deliver
> multicast packets to any host on the according port via unicast
> individually. This is done by copying the packet per host and
> changing the multicast destination MAC to a unicast one accordingly.
> 
> multicast-to-unicast works on top of the multicast snooping feature of
> the bridge. Which means unicast copies are only delivered to hosts which
> are interested in it and signalized this via IGMP/MLD reports
> previously.
> 
> This feature is intended for interface types which have a more reliable
> and/or efficient way to deliver unicast packets than broadcast ones
> (e.g. wifi).
> 
> However, it should only be enabled on interfaces where no IGMPv2/MLDv1
> report suppression takes place. This feature is disabled by default.
> 
> The initial patch and idea is from Felix Fietkau.
> 
> Cc: Felix Fietkau <n...@nbd.name>
> Signed-off-by: Linus Lüssing <linus.luess...@c0d3.blue>
Please add Signed-off-by: Felix Fietkau <n...@nbd.name>
in the next version, and maybe also From:

Thanks,

- Felix

Re: [PATCH net-next] bridge: multicast to unicast

[Felix Fietkau]

On 2017-01-02 20:32, Linus Lüssing wrote:
> Implements an optional, per bridge port flag and feature to deliver
> multicast packets to any host on the according port via unicast
> individually. This is done by copying the packet per host and
> changing the multicast destination MAC to a unicast one accordingly.
> 
> multicast-to-unicast works on top of the multicast snooping feature of
> the bridge. Which means unicast copies are only delivered to hosts which
> are interested in it and signalized this via IGMP/MLD reports
> previously.
> 
> This feature is intended for interface types which have a more reliable
> and/or efficient way to deliver unicast packets than broadcast ones
> (e.g. wifi).
> 
> However, it should only be enabled on interfaces where no IGMPv2/MLDv1
> report suppression takes place. This feature is disabled by default.
> 
> The initial patch and idea is from Felix Fietkau.
> 
> Cc: Felix Fietkau 
> Signed-off-by: Linus Lüssing 
Please add Signed-off-by: Felix Fietkau 
in the next version, and maybe also From:

Thanks,

- Felix

Re: [PATCH v2] ath9k: do not return early to fix rcu unlocking

[Felix Fietkau]

On 2016-12-13 18:08, Tobias Klausmann wrote:
> Starting with commit d94a461d7a7d ("ath9k: use ieee80211_tx_status_noskb
> where possible") the driver uses rcu_read_lock() && rcu_read_unlock(), yet on
> returning early in ath_tx_edma_tasklet() the unlock is missing leading to 
> stalls
> and suspicious RCU usage:
> 
>  ===
>  [ INFO: suspicious RCU usage. ]
>  4.9.0-rc8 #11 Not tainted
>  ---
>  kernel/rcu/tree.c:705 Illegal idle entry in RCU read-side critical section.!
> 
>  other info that might help us debug this:
> 
>  RCU used illegally from idle CPU!
>  rcu_scheduler_active = 1, debug_locks = 0
>  RCU used illegally from extended quiescent state!
>  1 lock held by swapper/7/0:
>  #0:
>   (
>  rcu_read_lock
>  ){..}
>  , at:
>  [] ath_tx_edma_tasklet+0x0/0x450 [ath9k]
> 
>  stack backtrace:
>  CPU: 7 PID: 0 Comm: swapper/7 Not tainted 4.9.0-rc8 #11
>  Hardware name: Acer Aspire V3-571G/VA50_HC_CR, BIOS V2.21 12/16/2013
>   88025efc3f38 8132b1e5 88017ede4540 0001
>   88025efc3f68 810a25f7 88025efcee60 88017edebdd8
>   88025eeb5400 0091 88025efc3f88 810c3cd4
>  Call Trace:
>   
>   [] dump_stack+0x68/0x93
>   [] lockdep_rcu_suspicious+0xd7/0x110
>   [] rcu_eqs_enter_common.constprop.85+0x154/0x200
>   [] rcu_irq_exit+0x44/0xa0
>   [] irq_exit+0x61/0xd0
>   [] do_IRQ+0x65/0x110
>   [] common_interrupt+0x89/0x89
>   
>   [] ? cpuidle_enter_state+0x151/0x200
>   [] cpuidle_enter+0x12/0x20
>   [] call_cpuidle+0x1e/0x40
>   [] cpu_startup_entry+0x146/0x220
>   [] start_secondary+0x148/0x170
> 
> Signed-off-by: Tobias Klausmann <tobias.johannes.klausm...@mni.thm.de>
> Fixes: d94a461d7a7d ("ath9k: use ieee80211_tx_status_noskb where possible")
> Cc: <sta...@vger.kernel.org> # v4.9
Acked-by: Felix Fietkau <n...@nbd.name>

Re: [PATCH v2] ath9k: do not return early to fix rcu unlocking

[Felix Fietkau]

On 2016-12-13 18:08, Tobias Klausmann wrote:
> Starting with commit d94a461d7a7d ("ath9k: use ieee80211_tx_status_noskb
> where possible") the driver uses rcu_read_lock() && rcu_read_unlock(), yet on
> returning early in ath_tx_edma_tasklet() the unlock is missing leading to 
> stalls
> and suspicious RCU usage:
> 
>  ===
>  [ INFO: suspicious RCU usage. ]
>  4.9.0-rc8 #11 Not tainted
>  ---
>  kernel/rcu/tree.c:705 Illegal idle entry in RCU read-side critical section.!
> 
>  other info that might help us debug this:
> 
>  RCU used illegally from idle CPU!
>  rcu_scheduler_active = 1, debug_locks = 0
>  RCU used illegally from extended quiescent state!
>  1 lock held by swapper/7/0:
>  #0:
>   (
>  rcu_read_lock
>  ){..}
>  , at:
>  [] ath_tx_edma_tasklet+0x0/0x450 [ath9k]
> 
>  stack backtrace:
>  CPU: 7 PID: 0 Comm: swapper/7 Not tainted 4.9.0-rc8 #11
>  Hardware name: Acer Aspire V3-571G/VA50_HC_CR, BIOS V2.21 12/16/2013
>   88025efc3f38 8132b1e5 88017ede4540 0001
>   88025efc3f68 810a25f7 88025efcee60 88017edebdd8
>   88025eeb5400 0091 88025efc3f88 810c3cd4
>  Call Trace:
>   
>   [] dump_stack+0x68/0x93
>   [] lockdep_rcu_suspicious+0xd7/0x110
>   [] rcu_eqs_enter_common.constprop.85+0x154/0x200
>   [] rcu_irq_exit+0x44/0xa0
>   [] irq_exit+0x61/0xd0
>   [] do_IRQ+0x65/0x110
>   [] common_interrupt+0x89/0x89
>   
>   [] ? cpuidle_enter_state+0x151/0x200
>   [] cpuidle_enter+0x12/0x20
>   [] call_cpuidle+0x1e/0x40
>   [] cpu_startup_entry+0x146/0x220
>   [] start_secondary+0x148/0x170
> 
> Signed-off-by: Tobias Klausmann 
> Fixes: d94a461d7a7d ("ath9k: use ieee80211_tx_status_noskb where possible")
> Cc:  # v4.9
Acked-by: Felix Fietkau 

Re: [PATCH] ath9k: unlock rcu read when returning early

[Felix Fietkau]

On 2016-12-13 14:41, Tobias Klausmann wrote:
> On 13.12.2016 11:41, Felix Fietkau wrote:
>> On 2016-12-12 19:50, Tobias Klausmann wrote:
>>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c 
>>> b/drivers/net/wireless/ath/ath9k/xmit.c
>>> index 52bfbb988611..857d5ae09a1d 100644
>>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>>> @@ -2787,6 +2787,7 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
>>> fifo_list = >txq_fifo[txq->txq_tailidx];
>>> if (list_empty(fifo_list)) {
>>> ath_txq_unlock(sc, txq);
>>> +   rcu_read_unlock();
>> Technically this is fine as well, but I'd prefer a fix where you replace
>> the 'return' with 'break', thus avoiding the duplication of
>> rcu_read_unlock()
> 
> Actually if you want to avoid it, maybe skipping over the rest is better 
> (as originally intended):
> 
> ...
> 
> ath_txq_unlock(sc, txq);
> 
> 
> goto unlock;
> }
> ...
> 
> unlock:
> rcu_read_unlock();
There are already other places that skip to the rcu_read_unlock() part
by using 'break'. I don't see how adding an unnecessary goto makes
things any better.

- Felix

Re: [PATCH] ath9k: unlock rcu read when returning early

[Felix Fietkau]

On 2016-12-13 14:41, Tobias Klausmann wrote:
> On 13.12.2016 11:41, Felix Fietkau wrote:
>> On 2016-12-12 19:50, Tobias Klausmann wrote:
>>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c 
>>> b/drivers/net/wireless/ath/ath9k/xmit.c
>>> index 52bfbb988611..857d5ae09a1d 100644
>>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>>> @@ -2787,6 +2787,7 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
>>> fifo_list = >txq_fifo[txq->txq_tailidx];
>>> if (list_empty(fifo_list)) {
>>> ath_txq_unlock(sc, txq);
>>> +   rcu_read_unlock();
>> Technically this is fine as well, but I'd prefer a fix where you replace
>> the 'return' with 'break', thus avoiding the duplication of
>> rcu_read_unlock()
> 
> Actually if you want to avoid it, maybe skipping over the rest is better 
> (as originally intended):
> 
> ...
> 
> ath_txq_unlock(sc, txq);
> 
> 
> goto unlock;
> }
> ...
> 
> unlock:
> rcu_read_unlock();
There are already other places that skip to the rcu_read_unlock() part
by using 'break'. I don't see how adding an unnecessary goto makes
things any better.

- Felix

Re: [PATCH] ath9k: unlock rcu read when returning early

[Felix Fietkau]

On 2016-12-12 19:50, Tobias Klausmann wrote:
> Starting with ath9k: use ieee80211_tx_status_noskb where possible
> [d94a461d7a7df68991fb9663531173f60ef89c68] the driver uses rcu_read_lock() &&
> rcu_read_unlock() yet on returning early in ath_tx_edma_tasklet() the unlock 
> is
> missing leading to stalls and suspicious RCU usage:
> 
>  ===
>  [ INFO: suspicious RCU usage. ]
>  4.9.0-rc8 #11 Not tainted
>  ---
>  kernel/rcu/tree.c:705 Illegal idle entry in RCU read-side critical section.!
> 
>  other info that might help us debug this:
> 
>  RCU used illegally from idle CPU!
>  rcu_scheduler_active = 1, debug_locks = 0
>  RCU used illegally from extended quiescent state!
>  1 lock held by swapper/7/0:
>  #0:
>   (
>  rcu_read_lock
>  ){..}
>  , at:
>  [] ath_tx_edma_tasklet+0x0/0x450 [ath9k]
> 
>  stack backtrace:
>  CPU: 7 PID: 0 Comm: swapper/7 Not tainted 4.9.0-rc8 #11
>  Hardware name: Acer Aspire V3-571G/VA50_HC_CR, BIOS V2.21 12/16/2013
>   88025efc3f38 8132b1e5 88017ede4540 0001
>   88025efc3f68 810a25f7 88025efcee60 88017edebdd8
>   88025eeb5400 0091 88025efc3f88 810c3cd4
>  Call Trace:
>   
>   [] dump_stack+0x68/0x93
>   [] lockdep_rcu_suspicious+0xd7/0x110
>   [] rcu_eqs_enter_common.constprop.85+0x154/0x200
>   [] rcu_irq_exit+0x44/0xa0
>   [] irq_exit+0x61/0xd0
>   [] do_IRQ+0x65/0x110
>   [] common_interrupt+0x89/0x89
>   
>   [] ? cpuidle_enter_state+0x151/0x200
>   [] cpuidle_enter+0x12/0x20
>   [] call_cpuidle+0x1e/0x40
>   [] cpu_startup_entry+0x146/0x220
>   [] start_secondary+0x148/0x170
> 
> Signed-off-by: Tobias Klausmann 
> ---
>  drivers/net/wireless/ath/ath9k/xmit.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c 
> b/drivers/net/wireless/ath/ath9k/xmit.c
> index 52bfbb988611..857d5ae09a1d 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -2787,6 +2787,7 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
>   fifo_list = >txq_fifo[txq->txq_tailidx];
>   if (list_empty(fifo_list)) {
>   ath_txq_unlock(sc, txq);
> + rcu_read_unlock();
Technically this is fine as well, but I'd prefer a fix where you replace
the 'return' with 'break', thus avoiding the duplication of
rcu_read_unlock()

Thanks,

- Felix

Re: [PATCH] ath9k: unlock rcu read when returning early

[Felix Fietkau]

On 2016-12-12 19:50, Tobias Klausmann wrote:
> Starting with ath9k: use ieee80211_tx_status_noskb where possible
> [d94a461d7a7df68991fb9663531173f60ef89c68] the driver uses rcu_read_lock() &&
> rcu_read_unlock() yet on returning early in ath_tx_edma_tasklet() the unlock 
> is
> missing leading to stalls and suspicious RCU usage:
> 
>  ===
>  [ INFO: suspicious RCU usage. ]
>  4.9.0-rc8 #11 Not tainted
>  ---
>  kernel/rcu/tree.c:705 Illegal idle entry in RCU read-side critical section.!
> 
>  other info that might help us debug this:
> 
>  RCU used illegally from idle CPU!
>  rcu_scheduler_active = 1, debug_locks = 0
>  RCU used illegally from extended quiescent state!
>  1 lock held by swapper/7/0:
>  #0:
>   (
>  rcu_read_lock
>  ){..}
>  , at:
>  [] ath_tx_edma_tasklet+0x0/0x450 [ath9k]
> 
>  stack backtrace:
>  CPU: 7 PID: 0 Comm: swapper/7 Not tainted 4.9.0-rc8 #11
>  Hardware name: Acer Aspire V3-571G/VA50_HC_CR, BIOS V2.21 12/16/2013
>   88025efc3f38 8132b1e5 88017ede4540 0001
>   88025efc3f68 810a25f7 88025efcee60 88017edebdd8
>   88025eeb5400 0091 88025efc3f88 810c3cd4
>  Call Trace:
>   
>   [] dump_stack+0x68/0x93
>   [] lockdep_rcu_suspicious+0xd7/0x110
>   [] rcu_eqs_enter_common.constprop.85+0x154/0x200
>   [] rcu_irq_exit+0x44/0xa0
>   [] irq_exit+0x61/0xd0
>   [] do_IRQ+0x65/0x110
>   [] common_interrupt+0x89/0x89
>   
>   [] ? cpuidle_enter_state+0x151/0x200
>   [] cpuidle_enter+0x12/0x20
>   [] call_cpuidle+0x1e/0x40
>   [] cpu_startup_entry+0x146/0x220
>   [] start_secondary+0x148/0x170
> 
> Signed-off-by: Tobias Klausmann 
> ---
>  drivers/net/wireless/ath/ath9k/xmit.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c 
> b/drivers/net/wireless/ath/ath9k/xmit.c
> index 52bfbb988611..857d5ae09a1d 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -2787,6 +2787,7 @@ void ath_tx_edma_tasklet(struct ath_softc *sc)
>   fifo_list = >txq_fifo[txq->txq_tailidx];
>   if (list_empty(fifo_list)) {
>   ath_txq_unlock(sc, txq);
> + rcu_read_unlock();
Technically this is fine as well, but I'd prefer a fix where you replace
the 'return' with 'break', thus avoiding the duplication of
rcu_read_unlock()

Thanks,

- Felix

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Felix Fietkau]

On 2016-12-10 21:32, Måns Rullgård wrote:
> Felix Fietkau <n...@nbd.name> writes:
> 
>> On 2016-12-10 14:25, Måns Rullgård wrote:
>>> Felix Fietkau <n...@nbd.name> writes:
>>> 
>>>> On 2016-12-07 19:54, Jason A. Donenfeld wrote:
>>>>> On Wed, Dec 7, 2016 at 7:51 PM, David Miller <da...@davemloft.net> wrote:
>>>>>> It's so much better to analyze properly where the misalignment comes from
>>>>>> and address it at the source, as we have for various cases that trip up
>>>>>> Sparc too.
>>>>> 
>>>>> That's sort of my attitude too, hence starting this thread. Any
>>>>> pointers you have about this would be most welcome, so as not to
>>>>> perpetuate what already seems like an issue in other parts of the
>>>>> stack.
>>>> Hi Jason,
>>>>
>>>> I'm the author of that hackish LEDE/OpenWrt patch that works around the
>>>> misalignment issues. Here's some context regarding that patch:
>>>>
>>>> I intentionally put it in the target specific patches for only one of
>>>> our MIPS targets. There are a few ar71xx devices where the misalignment
>>>> cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment
>>>> requirement, and does not support inserting 2 bytes of padding to
>>>> correct the IP header misalignment.
>>>>
>>>> With these limitations the choice was between this ugly network stack
>>>> patch or inserting a very expensive memmove in the data path (which is
>>>> better than taking the mis-alignment traps, but still hurts routing
>>>> performance significantly).
>>> 
>>> I solved this problem in an Ethernet driver by copying the initial part
>>> of the packet to an aligned skb and appending the remainder using
>>> skb_add_rx_frag().  The kernel network stack only cares about the
>>> headers, so the alignment of the packet payload doesn't matter.
>>
>> I considered that as well, but it's bad for routing performance if the
>> ethernet MAC does not support scatter/gather for xmit.
>> Unfortunately that limitation is quite common on embedded hardware.
> 
> Yes, I can see that being an issue.  However, if you're doing zero-copy
> routing, the header part of the original buffer should still be there,
> unused, so you could presumably copy the header of the outgoing packet
> there and then do dma as usual.  Maybe there's something in the network
> stack that makes this impossible though.
That still puts more pressure on the ridiculously small dcache sizes
that are typical for embedded MIPS routers.

- Felix

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Felix Fietkau]

On 2016-12-10 21:32, Måns Rullgård wrote:
> Felix Fietkau  writes:
> 
>> On 2016-12-10 14:25, Måns Rullgård wrote:
>>> Felix Fietkau  writes:
>>> 
>>>> On 2016-12-07 19:54, Jason A. Donenfeld wrote:
>>>>> On Wed, Dec 7, 2016 at 7:51 PM, David Miller  wrote:
>>>>>> It's so much better to analyze properly where the misalignment comes from
>>>>>> and address it at the source, as we have for various cases that trip up
>>>>>> Sparc too.
>>>>> 
>>>>> That's sort of my attitude too, hence starting this thread. Any
>>>>> pointers you have about this would be most welcome, so as not to
>>>>> perpetuate what already seems like an issue in other parts of the
>>>>> stack.
>>>> Hi Jason,
>>>>
>>>> I'm the author of that hackish LEDE/OpenWrt patch that works around the
>>>> misalignment issues. Here's some context regarding that patch:
>>>>
>>>> I intentionally put it in the target specific patches for only one of
>>>> our MIPS targets. There are a few ar71xx devices where the misalignment
>>>> cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment
>>>> requirement, and does not support inserting 2 bytes of padding to
>>>> correct the IP header misalignment.
>>>>
>>>> With these limitations the choice was between this ugly network stack
>>>> patch or inserting a very expensive memmove in the data path (which is
>>>> better than taking the mis-alignment traps, but still hurts routing
>>>> performance significantly).
>>> 
>>> I solved this problem in an Ethernet driver by copying the initial part
>>> of the packet to an aligned skb and appending the remainder using
>>> skb_add_rx_frag().  The kernel network stack only cares about the
>>> headers, so the alignment of the packet payload doesn't matter.
>>
>> I considered that as well, but it's bad for routing performance if the
>> ethernet MAC does not support scatter/gather for xmit.
>> Unfortunately that limitation is quite common on embedded hardware.
> 
> Yes, I can see that being an issue.  However, if you're doing zero-copy
> routing, the header part of the original buffer should still be there,
> unused, so you could presumably copy the header of the outgoing packet
> there and then do dma as usual.  Maybe there's something in the network
> stack that makes this impossible though.
That still puts more pressure on the ridiculously small dcache sizes
that are typical for embedded MIPS routers.

- Felix

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Felix Fietkau]

On 2016-12-10 14:25, Måns Rullgård wrote:
> Felix Fietkau <n...@nbd.name> writes:
> 
>> On 2016-12-07 19:54, Jason A. Donenfeld wrote:
>>> On Wed, Dec 7, 2016 at 7:51 PM, David Miller <da...@davemloft.net> wrote:
>>>> It's so much better to analyze properly where the misalignment comes from
>>>> and address it at the source, as we have for various cases that trip up
>>>> Sparc too.
>>> 
>>> That's sort of my attitude too, hence starting this thread. Any
>>> pointers you have about this would be most welcome, so as not to
>>> perpetuate what already seems like an issue in other parts of the
>>> stack.
>> Hi Jason,
>>
>> I'm the author of that hackish LEDE/OpenWrt patch that works around the
>> misalignment issues. Here's some context regarding that patch:
>>
>> I intentionally put it in the target specific patches for only one of
>> our MIPS targets. There are a few ar71xx devices where the misalignment
>> cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment
>> requirement, and does not support inserting 2 bytes of padding to
>> correct the IP header misalignment.
>>
>> With these limitations the choice was between this ugly network stack
>> patch or inserting a very expensive memmove in the data path (which is
>> better than taking the mis-alignment traps, but still hurts routing
>> performance significantly).
> 
> I solved this problem in an Ethernet driver by copying the initial part
> of the packet to an aligned skb and appending the remainder using
> skb_add_rx_frag().  The kernel network stack only cares about the
> headers, so the alignment of the packet payload doesn't matter.
I considered that as well, but it's bad for routing performance if the
ethernet MAC does not support scatter/gather for xmit.
Unfortunately that limitation is quite common on embedded hardware.

- Felix

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Felix Fietkau]

On 2016-12-10 14:25, Måns Rullgård wrote:
> Felix Fietkau  writes:
> 
>> On 2016-12-07 19:54, Jason A. Donenfeld wrote:
>>> On Wed, Dec 7, 2016 at 7:51 PM, David Miller  wrote:
>>>> It's so much better to analyze properly where the misalignment comes from
>>>> and address it at the source, as we have for various cases that trip up
>>>> Sparc too.
>>> 
>>> That's sort of my attitude too, hence starting this thread. Any
>>> pointers you have about this would be most welcome, so as not to
>>> perpetuate what already seems like an issue in other parts of the
>>> stack.
>> Hi Jason,
>>
>> I'm the author of that hackish LEDE/OpenWrt patch that works around the
>> misalignment issues. Here's some context regarding that patch:
>>
>> I intentionally put it in the target specific patches for only one of
>> our MIPS targets. There are a few ar71xx devices where the misalignment
>> cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment
>> requirement, and does not support inserting 2 bytes of padding to
>> correct the IP header misalignment.
>>
>> With these limitations the choice was between this ugly network stack
>> patch or inserting a very expensive memmove in the data path (which is
>> better than taking the mis-alignment traps, but still hurts routing
>> performance significantly).
> 
> I solved this problem in an Ethernet driver by copying the initial part
> of the packet to an aligned skb and appending the remainder using
> skb_add_rx_frag().  The kernel network stack only cares about the
> headers, so the alignment of the packet payload doesn't matter.
I considered that as well, but it's bad for routing performance if the
ethernet MAC does not support scatter/gather for xmit.
Unfortunately that limitation is quite common on embedded hardware.

- Felix

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Felix Fietkau]

On 2016-12-07 19:54, Jason A. Donenfeld wrote:
> On Wed, Dec 7, 2016 at 7:51 PM, David Miller  wrote:
>> It's so much better to analyze properly where the misalignment comes from
>> and address it at the source, as we have for various cases that trip up
>> Sparc too.
> 
> That's sort of my attitude too, hence starting this thread. Any
> pointers you have about this would be most welcome, so as not to
> perpetuate what already seems like an issue in other parts of the
> stack.
Hi Jason,

I'm the author of that hackish LEDE/OpenWrt patch that works around the
misalignment issues. Here's some context regarding that patch:

I intentionally put it in the target specific patches for only one of
our MIPS targets. There are a few ar71xx devices where the misalignment
cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment
requirement, and does not support inserting 2 bytes of padding to
correct the IP header misalignment.

With these limitations the choice was between this ugly network stack
patch or inserting a very expensive memmove in the data path (which is
better than taking the mis-alignment traps, but still hurts routing
performance significantly).

There are a lot of places in the network stack that assume full 32 bit
alignment, and you only get to see those once you start using more of
netfilter, play with various tunnel encapsulations, etc.

I think you have 3 options to deal with this properly:
1. add 3 bytes of padding
2. allocate a separate skb for decryption (might be more expensive)
3. save the header and decrypt to the start of the packet data
(overwriting the misaligned header).

I'm not sure what the performance impact of 2 and 3 is, so it's probably
best to stick with the padding.

I've taken a quick look at the wireguard message headers, and my
recommendation would be to insert the 3-byte padding in struct
message_header and remove __packed from your structs.
This will also remove misaligment of your own protocol fields.

- Felix

Re: Misalignment, MIPS, and ip_hdr(skb)->version

[Felix Fietkau]

On 2016-12-07 19:54, Jason A. Donenfeld wrote:
> On Wed, Dec 7, 2016 at 7:51 PM, David Miller  wrote:
>> It's so much better to analyze properly where the misalignment comes from
>> and address it at the source, as we have for various cases that trip up
>> Sparc too.
> 
> That's sort of my attitude too, hence starting this thread. Any
> pointers you have about this would be most welcome, so as not to
> perpetuate what already seems like an issue in other parts of the
> stack.
Hi Jason,

I'm the author of that hackish LEDE/OpenWrt patch that works around the
misalignment issues. Here's some context regarding that patch:

I intentionally put it in the target specific patches for only one of
our MIPS targets. There are a few ar71xx devices where the misalignment
cannot be fixed, because the Ethernet MAC has a 4-byte DMA alignment
requirement, and does not support inserting 2 bytes of padding to
correct the IP header misalignment.

With these limitations the choice was between this ugly network stack
patch or inserting a very expensive memmove in the data path (which is
better than taking the mis-alignment traps, but still hurts routing
performance significantly).

There are a lot of places in the network stack that assume full 32 bit
alignment, and you only get to see those once you start using more of
netfilter, play with various tunnel encapsulations, etc.

I think you have 3 options to deal with this properly:
1. add 3 bytes of padding
2. allocate a separate skb for decryption (might be more expensive)
3. save the header and decrypt to the start of the packet data
(overwriting the misaligned header).

I'm not sure what the performance impact of 2 and 3 is, so it's probably
best to stick with the padding.

I've taken a quick look at the wireguard message headers, and my
recommendation would be to insert the 3-byte padding in struct
message_header and remove __packed from your structs.
This will also remove misaligment of your own protocol fields.

- Felix

Re: [PATCH v2 0/4] ubifs: Add overlayfs support

[Felix Fietkau]

On 2016-09-14 22:28, Richard Weinberger wrote:
> To support overlayfs in a proper way a filesystem has to offer support
> for DT_CHR, RENAME_WHITEOUT and RENAME_EXCHANGE.
> While UBIFS supports DT_CHR it lacks support for RENAME_WHITEOUT and
> RENAME_EXCHANGE. This patch series implement the missing bits.
> RENAME_WHITEOUT itself depends on O_TMPFILE, therefore O_TMPFILE is now
> also supported.
> 
> Changes since v1:
>  - Build fix for CONFIG_LOCKDEP=y
>  - Fix for wrong dentry name length calculation in ubifs_jnl_xrename()
>  - Fix for wrong directory link count after RENAME_EXCHANGE
> 
> Richard Weinberger (4):
>   ubifs: Implement O_TMPFILE
>   ubifs: Implement RENAME_WHITEOUT
>   ubifs: Implement RENAME_EXCHANGE
>   ubifs: Use move variable in ubifs_rename()
Tested-by: Felix Fietkau <n...@nbd.name>

Re: [PATCH v2 0/4] ubifs: Add overlayfs support

[Felix Fietkau]

On 2016-09-14 22:28, Richard Weinberger wrote:
> To support overlayfs in a proper way a filesystem has to offer support
> for DT_CHR, RENAME_WHITEOUT and RENAME_EXCHANGE.
> While UBIFS supports DT_CHR it lacks support for RENAME_WHITEOUT and
> RENAME_EXCHANGE. This patch series implement the missing bits.
> RENAME_WHITEOUT itself depends on O_TMPFILE, therefore O_TMPFILE is now
> also supported.
> 
> Changes since v1:
>  - Build fix for CONFIG_LOCKDEP=y
>  - Fix for wrong dentry name length calculation in ubifs_jnl_xrename()
>  - Fix for wrong directory link count after RENAME_EXCHANGE
> 
> Richard Weinberger (4):
>   ubifs: Implement O_TMPFILE
>   ubifs: Implement RENAME_WHITEOUT
>   ubifs: Implement RENAME_EXCHANGE
>   ubifs: Use move variable in ubifs_rename()
Tested-by: Felix Fietkau 

Re: [PATCH 1/2] add basic register-field manipulation macros

[Felix Fietkau]

On 2016-06-13 15:29, Jakub Kicinski wrote:
> C bitfields are problematic and best avoided.  Developers
> interacting with hardware registers find themselves searching
> for easy-to-use alternatives.  Common approach is to define
> structures or sets of macros containing mask and shift pair.
> Operations on the register are then performed as follows:
> 
>  field = (reg >> shift) & mask;
> 
>  reg &= ~(mask << shift);
>  reg |= (field & mask) << shift;
> 
> Defining shift and mask separately is tedious.  Ivo van Doorn
> came up with an idea of computing them at compilation time
> based on a single shifted mask (later refined by Felix) which
> can be used like this:
> 
>  #define X_REG_FIELD 0x000ff000
> 
>  field = FIELD_GET(X_REG_FIELD, reg);
> 
>  reg &= ~X_REG_FIELD;
>  reg |= FIELD_PUT(X_REG_FIELD, field);
> 
> FIELD_{GET,PUT} macros take care of finding out what the
> appropriate shift is based on compilation time ffs operation.
> 
> GENMASK can be used to define registers (which is usually
> less error-prone and easier to match with datasheets).
> 
> This approach is the most convenient I've seen so to limit code
> multiplication let's move the macros to a global header file.
> 
> Compared to Felix Fietkau's implementation from mt76 this one
> uses standard Linux and GCC functions such as is_power_of_2()
> and __builtin_ffsll().
> 
> Signed-off-by: Jakub Kicinski <jakub.kicin...@netronome.com>
> ---
>  include/linux/bitfield.h | 58 
> 
>  include/linux/log2.h |  6 +
>  2 files changed, 64 insertions(+)
>  create mode 100644 include/linux/bitfield.h
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> new file mode 100644
> index ..ae2224464523
> --- /dev/null
> +++ b/include/linux/bitfield.h
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (C) 2014 Felix Fietkau <n...@openwrt.org>
Please change my email address to n...@nbd.name here

- Felix

Re: [PATCH 1/2] add basic register-field manipulation macros

[Felix Fietkau]

On 2016-06-13 15:29, Jakub Kicinski wrote:
> C bitfields are problematic and best avoided.  Developers
> interacting with hardware registers find themselves searching
> for easy-to-use alternatives.  Common approach is to define
> structures or sets of macros containing mask and shift pair.
> Operations on the register are then performed as follows:
> 
>  field = (reg >> shift) & mask;
> 
>  reg &= ~(mask << shift);
>  reg |= (field & mask) << shift;
> 
> Defining shift and mask separately is tedious.  Ivo van Doorn
> came up with an idea of computing them at compilation time
> based on a single shifted mask (later refined by Felix) which
> can be used like this:
> 
>  #define X_REG_FIELD 0x000ff000
> 
>  field = FIELD_GET(X_REG_FIELD, reg);
> 
>  reg &= ~X_REG_FIELD;
>  reg |= FIELD_PUT(X_REG_FIELD, field);
> 
> FIELD_{GET,PUT} macros take care of finding out what the
> appropriate shift is based on compilation time ffs operation.
> 
> GENMASK can be used to define registers (which is usually
> less error-prone and easier to match with datasheets).
> 
> This approach is the most convenient I've seen so to limit code
> multiplication let's move the macros to a global header file.
> 
> Compared to Felix Fietkau's implementation from mt76 this one
> uses standard Linux and GCC functions such as is_power_of_2()
> and __builtin_ffsll().
> 
> Signed-off-by: Jakub Kicinski 
> ---
>  include/linux/bitfield.h | 58 
> 
>  include/linux/log2.h |  6 +
>  2 files changed, 64 insertions(+)
>  create mode 100644 include/linux/bitfield.h
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> new file mode 100644
> index ..ae2224464523
> --- /dev/null
> +++ b/include/linux/bitfield.h
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (C) 2014 Felix Fietkau 
Please change my email address to n...@nbd.name here

- Felix

[PATCH] treewide: update my email address

[Felix Fietkau]

My n...@openwrt.org address is no longer valid and bounces

Signed-off-by: Felix Fietkau <n...@nbd.name>
---
 MAINTAINERS  | 2 +-
 arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts| 2 +-
 arch/mips/ar7/clock.c| 2 +-
 arch/mips/ar7/gpio.c | 2 +-
 arch/mips/ar7/irq.c  | 2 +-
 arch/mips/ar7/memory.c   | 2 +-
 arch/mips/ar7/platform.c | 2 +-
 arch/mips/ath25/Makefile | 2 +-
 arch/mips/ath25/ar2315.c | 2 +-
 arch/mips/ath25/ar2315_regs.h| 2 +-
 arch/mips/ath25/ar5312.c | 2 +-
 arch/mips/ath25/ar5312_regs.h| 2 +-
 arch/mips/ath25/board.c  | 2 +-
 arch/mips/ath25/prom.c   | 2 +-
 arch/mips/bcm47xx/setup.c| 2 +-
 arch/mips/include/asm/mach-ar7/ar7.h | 2 +-
 arch/mips/include/asm/mach-ath25/dma-coherence.h | 2 +-
 arch/mips/include/asm/mach-ralink/mt7621/cpu-feature-overrides.h | 2 +-
 arch/mips/include/asm/mach-rc32434/rb.h  | 2 +-
 arch/mips/pci/ops-rc32434.c  | 2 +-
 arch/mips/rb532/devices.c| 2 +-
 arch/mips/rb532/prom.c   | 2 +-
 drivers/firmware/broadcom/bcm47xx_nvram.c| 2 +-
 drivers/firmware/broadcom/bcm47xx_sprom.c| 2 +-
 drivers/mtd/ar7part.c| 4 ++--
 drivers/net/ethernet/broadcom/b44.c  | 2 +-
 drivers/net/ethernet/korina.c| 4 ++--
 drivers/net/ethernet/mediatek/mtk_eth_soc.c  | 2 +-
 drivers/net/ethernet/mediatek/mtk_eth_soc.h  | 2 +-
 drivers/net/wireless/ath/ath5k/eeprom.c  | 2 +-
 drivers/net/wireless/ath/ath5k/phy.c | 2 +-
 drivers/net/wireless/mediatek/mt7601u/core.c | 2 +-
 drivers/net/wireless/mediatek/mt7601u/debugfs.c  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.h  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/eeprom.c   | 2 +-
 drivers/net/wireless/mediatek/mt7601u/eeprom.h   | 2 +-
 drivers/net/wireless/mediatek/mt7601u/init.c | 2 +-
 drivers/net/wireless/mediatek/mt7601u/mac.c  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/mac.h  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/main.c | 2 +-
 drivers/net/wireless/mediatek/mt7601u/mcu.c  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/mcu.h  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/phy.c  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/regs.h | 2 +-
 drivers/net/wireless/mediatek/mt7601u/trace.c| 2 +-
 drivers/net/wireless/mediatek/mt7601u/trace.h| 2 +-
 drivers/net/wireless/mediatek/mt7601u/tx.c   | 2 +-
 drivers/net/wireless/mediatek/mt7601u/util.c | 2 +-
 drivers/net/wireless/mediatek/mt7601u/util.h | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800.h  | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800lib.c   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800mmio.c  | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800mmio.h  | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800pci.c   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800pci.h   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800soc.c   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800usb.c   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800usb.h   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2x00soc.c   | 2 +-
 drivers/ssb/driver_extif.c   | 2 +-
 drivers/tty/serial/lantiq.c  | 2 +-
 drivers/usb/host/bcma-hcd.c  | 2 +-
 net/mac80211/rc80211_minstrel.c  | 2 +-
 net/mac80211/rc80211_minstrel.h  | 2 +-
 net/mac80211/rc80211_minstrel_debugfs.c  | 2 +-
 net/mac80211/rc80211_minstre

[PATCH] treewide: update my email address

[Felix Fietkau]

My n...@openwrt.org address is no longer valid and bounces

Signed-off-by: Felix Fietkau 
---
 MAINTAINERS  | 2 +-
 arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts| 2 +-
 arch/mips/ar7/clock.c| 2 +-
 arch/mips/ar7/gpio.c | 2 +-
 arch/mips/ar7/irq.c  | 2 +-
 arch/mips/ar7/memory.c   | 2 +-
 arch/mips/ar7/platform.c | 2 +-
 arch/mips/ath25/Makefile | 2 +-
 arch/mips/ath25/ar2315.c | 2 +-
 arch/mips/ath25/ar2315_regs.h| 2 +-
 arch/mips/ath25/ar5312.c | 2 +-
 arch/mips/ath25/ar5312_regs.h| 2 +-
 arch/mips/ath25/board.c  | 2 +-
 arch/mips/ath25/prom.c   | 2 +-
 arch/mips/bcm47xx/setup.c| 2 +-
 arch/mips/include/asm/mach-ar7/ar7.h | 2 +-
 arch/mips/include/asm/mach-ath25/dma-coherence.h | 2 +-
 arch/mips/include/asm/mach-ralink/mt7621/cpu-feature-overrides.h | 2 +-
 arch/mips/include/asm/mach-rc32434/rb.h  | 2 +-
 arch/mips/pci/ops-rc32434.c  | 2 +-
 arch/mips/rb532/devices.c| 2 +-
 arch/mips/rb532/prom.c   | 2 +-
 drivers/firmware/broadcom/bcm47xx_nvram.c| 2 +-
 drivers/firmware/broadcom/bcm47xx_sprom.c| 2 +-
 drivers/mtd/ar7part.c| 4 ++--
 drivers/net/ethernet/broadcom/b44.c  | 2 +-
 drivers/net/ethernet/korina.c| 4 ++--
 drivers/net/ethernet/mediatek/mtk_eth_soc.c  | 2 +-
 drivers/net/ethernet/mediatek/mtk_eth_soc.h  | 2 +-
 drivers/net/wireless/ath/ath5k/eeprom.c  | 2 +-
 drivers/net/wireless/ath/ath5k/phy.c | 2 +-
 drivers/net/wireless/mediatek/mt7601u/core.c | 2 +-
 drivers/net/wireless/mediatek/mt7601u/debugfs.c  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.h  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/eeprom.c   | 2 +-
 drivers/net/wireless/mediatek/mt7601u/eeprom.h   | 2 +-
 drivers/net/wireless/mediatek/mt7601u/init.c | 2 +-
 drivers/net/wireless/mediatek/mt7601u/mac.c  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/mac.h  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/main.c | 2 +-
 drivers/net/wireless/mediatek/mt7601u/mcu.c  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/mcu.h  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/phy.c  | 2 +-
 drivers/net/wireless/mediatek/mt7601u/regs.h | 2 +-
 drivers/net/wireless/mediatek/mt7601u/trace.c| 2 +-
 drivers/net/wireless/mediatek/mt7601u/trace.h| 2 +-
 drivers/net/wireless/mediatek/mt7601u/tx.c   | 2 +-
 drivers/net/wireless/mediatek/mt7601u/util.c | 2 +-
 drivers/net/wireless/mediatek/mt7601u/util.h | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800.h  | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800lib.c   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800mmio.c  | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800mmio.h  | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800pci.c   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800pci.h   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800soc.c   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800usb.c   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2800usb.h   | 2 +-
 drivers/net/wireless/ralink/rt2x00/rt2x00soc.c   | 2 +-
 drivers/ssb/driver_extif.c   | 2 +-
 drivers/tty/serial/lantiq.c  | 2 +-
 drivers/usb/host/bcma-hcd.c  | 2 +-
 net/mac80211/rc80211_minstrel.c  | 2 +-
 net/mac80211/rc80211_minstrel.h  | 2 +-
 net/mac80211/rc80211_minstrel_debugfs.c  | 2 +-
 net/mac80211/rc80211_minstrel_ht.c   | 2 +-
 net

Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[Felix Fietkau]

On 2016-02-26 16:18, Andrew Lunn wrote:
> On Fri, Feb 26, 2016 at 03:21:35PM +0100, John Crispin wrote:
>> The ESW is found in many of the old 100mbit MIPS based SoCs. it has 5
>> external ports, 1 cpu port and 1 further port that the internal HW
>> offloading engine connects to.
>> 
>> This driver is very basic and only provides basic init and irq support.
>> The SoC and switch core both have support for a special tag making DSA
>> support possible.
> 
> Hi Crispin
> 
> There was recently a discussion about adding switches without using
> DSA or switchdev. It was pretty much decided we would not accept such
> drivers.
For exactly this reason, the code does not provide any non-standard API
for allowing the user to configure the switch. The hardware needs to be
programmed with some defaults for the driver to be functional (since the
switch logic is built into the SoC).

In my opinion, leaving this part out does not make much sense and
neither does deferring the entire patch series until we have a
switchdev/DSA capable driver. This is just a starting point, which will
be turned into a proper driver with the right APIs later.

- Felix

Re: [PATCH V2 03/12] net-next: mediatek: add embedded switch driver (ESW)

[Felix Fietkau]

On 2016-02-26 16:18, Andrew Lunn wrote:
> On Fri, Feb 26, 2016 at 03:21:35PM +0100, John Crispin wrote:
>> The ESW is found in many of the old 100mbit MIPS based SoCs. it has 5
>> external ports, 1 cpu port and 1 further port that the internal HW
>> offloading engine connects to.
>> 
>> This driver is very basic and only provides basic init and irq support.
>> The SoC and switch core both have support for a special tag making DSA
>> support possible.
> 
> Hi Crispin
> 
> There was recently a discussion about adding switches without using
> DSA or switchdev. It was pretty much decided we would not accept such
> drivers.
For exactly this reason, the code does not provide any non-standard API
for allowing the user to configure the switch. The hardware needs to be
programmed with some defaults for the driver to be functional (since the
switch logic is built into the SoC).

In my opinion, leaving this part out does not make much sense and
neither does deferring the entire patch series until we have a
switchdev/DSA capable driver. This is just a starting point, which will
be turned into a proper driver with the right APIs later.

- Felix

Re: [PATCH for-4.4 1/2] mtd: spi-nor: fix Spansion regressions (aliased with Winbond)

[Felix Fietkau]

On 2016-01-06 01:02, Brian Norris wrote:
> On Mon, Jan 04, 2016 at 06:29:28PM -0800, Brian Norris wrote:
>> On Tue, Dec 15, 2015 at 10:48:20AM -0800, Brian Norris wrote:
>> > Spansion and Winbond have occasionally used the same manufacturer ID,
>> > and they don't support the same features. Particularly, writing SR=0
>> > seems to break read access for Spansion's s25fl064k. Unfortunately, we
>> > don't currently have a way to differentiate these Spansion and Winbond
>> > parts, so rather than regressing support for these Spansion flash, let's
>> > drop the new Winbond lock/unlock support for now. We can try to address
>> > Winbond support during the next release cycle.
>> > 
>> > Original discussion:
>> > 
>> > http://patchwork.ozlabs.org/patch/549173/
>> > http://patchwork.ozlabs.org/patch/553683/
>> > 
>> > Fixes: 357ca38d4751 ("mtd: spi-nor: support lock/unlock/is_locked for 
>> > Winbond")
>> > Fixes: c6fc2171b249 ("mtd: spi-nor: disable protection for Winbond flash 
>> > at startup")
>> > Signed-off-by: Brian Norris 
>> > Reported-by: Felix Fietkau 
>> > Cc: Felix Fietkau 
>> 
>> Felix,
>> 
>> Can I get a Tested-by? I'm going to send this for 4.4 still, if
>> possible.
> 
> Despite the lack of response, pushed both to linux-mtd.git, as they are
> obvious responses to the reported regressions/bugs.
Sorry for the delay, I don't have time to test that at the moment. I'll
try to find the time for it soon.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH for-4.4 1/2] mtd: spi-nor: fix Spansion regressions (aliased with Winbond)

[Felix Fietkau]

On 2016-01-06 01:02, Brian Norris wrote:
> On Mon, Jan 04, 2016 at 06:29:28PM -0800, Brian Norris wrote:
>> On Tue, Dec 15, 2015 at 10:48:20AM -0800, Brian Norris wrote:
>> > Spansion and Winbond have occasionally used the same manufacturer ID,
>> > and they don't support the same features. Particularly, writing SR=0
>> > seems to break read access for Spansion's s25fl064k. Unfortunately, we
>> > don't currently have a way to differentiate these Spansion and Winbond
>> > parts, so rather than regressing support for these Spansion flash, let's
>> > drop the new Winbond lock/unlock support for now. We can try to address
>> > Winbond support during the next release cycle.
>> > 
>> > Original discussion:
>> > 
>> > http://patchwork.ozlabs.org/patch/549173/
>> > http://patchwork.ozlabs.org/patch/553683/
>> > 
>> > Fixes: 357ca38d4751 ("mtd: spi-nor: support lock/unlock/is_locked for 
>> > Winbond")
>> > Fixes: c6fc2171b249 ("mtd: spi-nor: disable protection for Winbond flash 
>> > at startup")
>> > Signed-off-by: Brian Norris <computersforpe...@gmail.com>
>> > Reported-by: Felix Fietkau <n...@openwrt.org>
>> > Cc: Felix Fietkau <n...@openwrt.org>
>> 
>> Felix,
>> 
>> Can I get a Tested-by? I'm going to send this for 4.4 still, if
>> possible.
> 
> Despite the lack of response, pushed both to linux-mtd.git, as they are
> obvious responses to the reported regressions/bugs.
Sorry for the delay, I don't have time to test that at the moment. I'll
try to find the time for it soon.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

[Felix Fietkau]

On 2015-12-07 23:58, Gilad Avidov wrote:
> +/* RRD (Receive Return Descriptor) */
> +union emac_rrd {
> + struct {
> + /* 32bit word 0 */
> + u32  xsum:16;
> + u32  nor:4;   /* number of RFD */
> + u32  si:12;   /* start index of rfd-ring */
> + /* 32bit word 1 */
> + u32  hash;
> + /* 32bit word 2 */
You should never use bitfields for hardware structs.
I think in general, kernel code should be made endian safe, even if you
only care about one particular endian type for your platform.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] net: emac: emac gigabit ethernet controller driver

[Felix Fietkau]

On 2015-12-07 23:58, Gilad Avidov wrote:
> +/* RRD (Receive Return Descriptor) */
> +union emac_rrd {
> + struct {
> + /* 32bit word 0 */
> + u32  xsum:16;
> + u32  nor:4;   /* number of RFD */
> + u32  si:12;   /* start index of rfd-ring */
> + /* 32bit word 1 */
> + u32  hash;
> + /* 32bit word 2 */
You should never use bitfields for hardware structs.
I think in general, kernel code should be made endian safe, even if you
only care about one particular endian type for your platform.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] net: dsa: Use devm_ prefixed allocations

[Felix Fietkau]

On 2015-10-02 15:30, Neil Armstrong wrote:
> On 10/02/2015 03:29 PM, Sergei Shtylyov wrote:
>> On 10/2/2015 1:48 PM, Neil Armstrong wrote:
>> 
>>> To simplify and prevent memory leakage when unbinding, use
>>> the devm_ memory allocation calls.
>>>
>>> Tested-by: Andrew Lunn 
>>> Tested-by: Florian Fainelli 
>>> Signed-off-by: Neil Armstrong 
>>> ---
>>>   net/dsa/dsa.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
>>> index c59fa5d..98f94c2 100644
>>> --- a/net/dsa/dsa.c
>>> +++ b/net/dsa/dsa.c
>>> @@ -305,7 +305,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, 
>>> struct device *parent)
>>>   if (ret < 0)
>>>   goto out;
>>>
>>> -ds->slave_mii_bus = mdiobus_alloc();
>>> +ds->slave_mii_bus = devm_mdiobus_alloc(parent);
>>>   if (ds->slave_mii_bus == NULL) {
>>>   ret = -ENOMEM;
>>>   goto out;
>>> @@ -400,7 +400,7 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
>>>   /*
>>>* Allocate and initialise switch state.
>>>*/
>>> -ds = kzalloc(sizeof(*ds) + drv->priv_size, GFP_KERNEL);
>>> +ds = devm_kzalloc(parent, sizeof(*ds) + drv->priv_size, GFP_KERNEL);
>>>   if (ds == NULL)
>>>   return ERR_PTR(-ENOMEM);
>>>
>>> @@ -883,7 +883,7 @@ static int dsa_probe(struct platform_device *pdev)
>>>   goto out;
>>>   }
>>>
>>> -dst = kzalloc(sizeof(*dst), GFP_KERNEL);
>>> +dst = devm_kzalloc(>dev, sizeof(*dst), GFP_KERNEL);
>>>   if (dst == NULL) {
>>>   dev_put(dev);
>>>   ret = -ENOMEM;
>>>
>> 
>>Shouldn't you remove the correspoding kfree(), etc. calls?
>> 
>> MBR, Sergei
>> 
> The corresponding kfree() calls were all missing.
Not in the error handling path. mdiobus_alloc has a corresponding
mdiobus_free in the same function.
The ds kzalloc in dsa_switch_setup has a kfree in dsa_switch_setup_one.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] net: dsa: Use devm_ prefixed allocations

[Felix Fietkau]

On 2015-10-02 15:30, Neil Armstrong wrote:
> On 10/02/2015 03:29 PM, Sergei Shtylyov wrote:
>> On 10/2/2015 1:48 PM, Neil Armstrong wrote:
>> 
>>> To simplify and prevent memory leakage when unbinding, use
>>> the devm_ memory allocation calls.
>>>
>>> Tested-by: Andrew Lunn 
>>> Tested-by: Florian Fainelli 
>>> Signed-off-by: Neil Armstrong 
>>> ---
>>>   net/dsa/dsa.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
>>> index c59fa5d..98f94c2 100644
>>> --- a/net/dsa/dsa.c
>>> +++ b/net/dsa/dsa.c
>>> @@ -305,7 +305,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, 
>>> struct device *parent)
>>>   if (ret < 0)
>>>   goto out;
>>>
>>> -ds->slave_mii_bus = mdiobus_alloc();
>>> +ds->slave_mii_bus = devm_mdiobus_alloc(parent);
>>>   if (ds->slave_mii_bus == NULL) {
>>>   ret = -ENOMEM;
>>>   goto out;
>>> @@ -400,7 +400,7 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
>>>   /*
>>>* Allocate and initialise switch state.
>>>*/
>>> -ds = kzalloc(sizeof(*ds) + drv->priv_size, GFP_KERNEL);
>>> +ds = devm_kzalloc(parent, sizeof(*ds) + drv->priv_size, GFP_KERNEL);
>>>   if (ds == NULL)
>>>   return ERR_PTR(-ENOMEM);
>>>
>>> @@ -883,7 +883,7 @@ static int dsa_probe(struct platform_device *pdev)
>>>   goto out;
>>>   }
>>>
>>> -dst = kzalloc(sizeof(*dst), GFP_KERNEL);
>>> +dst = devm_kzalloc(>dev, sizeof(*dst), GFP_KERNEL);
>>>   if (dst == NULL) {
>>>   dev_put(dev);
>>>   ret = -ENOMEM;
>>>
>> 
>>Shouldn't you remove the correspoding kfree(), etc. calls?
>> 
>> MBR, Sergei
>> 
> The corresponding kfree() calls were all missing.
Not in the error handling path. mdiobus_alloc has a corresponding
mdiobus_free in the same function.
The ds kzalloc in dsa_switch_setup has a kfree in dsa_switch_setup_one.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] net: dsa: Use devm_ prefixed allocations

[Felix Fietkau]

On 2015-10-02 12:48, Neil Armstrong wrote:
> To simplify and prevent memory leakage when unbinding, use
> the devm_ memory allocation calls.
> 
> Tested-by: Andrew Lunn 
> Tested-by: Florian Fainelli 
> Signed-off-by: Neil Armstrong 
I think you also need to get rid of the corresponding free calls in the
error path, otherwise it will probably crash at some point.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/