* Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
From: Lorenzo Bianconi @ 2019-06-14 10:22 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, kvalo, linux-wireless, nbd
In-Reply-To: <20190614075303.GB3395@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1937 bytes --]
> On Thu, Jun 13, 2019 at 11:43:13PM +0200, Lorenzo Bianconi wrote:
> > Set usb buffer size taking into account skb_shared_info in order to
> > not always copy the first part of received frames if A-MSDU is enabled
> > for SG capable devices. Moreover align usb buffer size to max_ep
> > boundaries and set buf_size to PAGE_SIZE even for sg case
>
> I think this should not be applied to wirless-drivers, only first patch
> that fix the bug and optimizations should be done in -next.
ack, right. I think patch 2/3 and 3/3 can go directly in Felix's tree
>
> > + int i, data_size;
> >
> > + data_size = rounddown(SKB_WITH_OVERHEAD(q->buf_size),
> > + dev->usb.in_ep[MT_EP_IN_PKT_RX].max_packet);
> > for (i = 0; i < nsgs; i++) {
> > struct page *page;
> > void *data;
> > @@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb,
> >
> > page = virt_to_head_page(data);
> > offset = data - page_address(page);
> > - sg_set_page(&urb->sg[i], page, q->buf_size, offset);
> > + sg_set_page(&urb->sg[i], page, data_size, offset);
> <snip>
> > - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> > q->ndesc = MT_NUM_RX_ENTRIES;
> > + q->buf_size = PAGE_SIZE;
> > +
>
> This should be associated with decrease of MT_SG_MAX_SIZE to value that
> is actually needed and currently this is 2 for 4k AMSDU.
MT_SG_MAX_SIZE is used even on tx side and I do not think we will end up with a
huge difference here
>
> However I don't think allocating 2 pages to avoid ieee80211 header and SNAP
> copy is worth to do. For me best approach would be allocate 1 page for
> 4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without data_size
> change to avoid 32B copying).
From my point of view it is better to avoid copying if it is possible. Are you
sure there is no difference?
Regards,
Lorenzo
>
> Stanislaw
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [PATCH] mac80211: suppress kernel warning message
From: Yibo Zhao @ 2019-06-14 11:01 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Yibo Zhao, Zhi Chen
In multiple SSID cases, it takes time to prepare every AP interface
to be ready in initializing phase. If a sta already knows everything it
needs to join one of the APs and sends authentication to the AP which
is not fully prepared at this point of time, AP's channel context
could be NULL. As a result, warning message occurs.
Even worse, if the AP is under attack via tools such as MDK3 and massive
authentication requests are received in a very short time, console will
be hung due to kernel warning messages.
WARN_ON_ONCE() could be a better way for indicating warning messages
without duplicate messages to flood the console.
Signed-off-by: Zhi Chen <zhichen@codeaurora.org>
Signed-off-by: Yibo Zhao <yiboz@codeaurora.org>
---
net/mac80211/ieee80211_i.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a8af4aa..682d0ab 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1435,7 +1435,7 @@ struct ieee80211_local {
rcu_read_lock();
chanctx_conf = rcu_dereference(sdata->vif.chanctx_conf);
- if (WARN_ON(!chanctx_conf)) {
+ if (WARN_ON_ONCE(!chanctx_conf)) {
rcu_read_unlock();
return NULL;
}
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
From: Stanislaw Gruszka @ 2019-06-14 11:04 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, kvalo, linux-wireless, nbd
In-Reply-To: <20190614102247.GB2669@localhost.localdomain>
On Fri, Jun 14, 2019 at 12:22:48PM +0200, Lorenzo Bianconi wrote:
> > On Thu, Jun 13, 2019 at 11:43:13PM +0200, Lorenzo Bianconi wrote:
> > > Set usb buffer size taking into account skb_shared_info in order to
> > > not always copy the first part of received frames if A-MSDU is enabled
> > > for SG capable devices. Moreover align usb buffer size to max_ep
> > > boundaries and set buf_size to PAGE_SIZE even for sg case
> >
> > I think this should not be applied to wirless-drivers, only first patch
> > that fix the bug and optimizations should be done in -next.
>
> ack, right. I think patch 2/3 and 3/3 can go directly in Felix's tree
>
> >
> > > + int i, data_size;
> > >
> > > + data_size = rounddown(SKB_WITH_OVERHEAD(q->buf_size),
> > > + dev->usb.in_ep[MT_EP_IN_PKT_RX].max_packet);
> > > for (i = 0; i < nsgs; i++) {
> > > struct page *page;
> > > void *data;
> > > @@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb,
> > >
> > > page = virt_to_head_page(data);
> > > offset = data - page_address(page);
> > > - sg_set_page(&urb->sg[i], page, q->buf_size, offset);
> > > + sg_set_page(&urb->sg[i], page, data_size, offset);
> > <snip>
> > > - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> > > q->ndesc = MT_NUM_RX_ENTRIES;
> > > + q->buf_size = PAGE_SIZE;
> > > +
> >
> > This should be associated with decrease of MT_SG_MAX_SIZE to value that
> > is actually needed and currently this is 2 for 4k AMSDU.
>
> MT_SG_MAX_SIZE is used even on tx side and I do not think we will end up with a
> huge difference here
So use different value as argument for mt76u_fill_rx_sg() in
mt76u_rx_urb_alloc(). After changing buf_size to PAGE_SIZE we will
allocate 8 pages per rx queue entry, but only 2 pages will be used
(with data_size change, 1 without data_size change). Or I'm wrong?
> > However I don't think allocating 2 pages to avoid ieee80211 header and SNAP
> > copy is worth to do. For me best approach would be allocate 1 page for
> > 4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without data_size
> > change to avoid 32B copying).
>
> From my point of view it is better to avoid copying if it is possible. Are you
> sure there is no difference?
I do not understand what you mean by difference here.
Stanislaw
^ permalink raw reply
* Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
From: Stanislaw Gruszka @ 2019-06-14 11:14 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, kvalo, linux-wireless, nbd, johannes
In-Reply-To: <20190614101115.GA2669@localhost.localdomain>
On Fri, Jun 14, 2019 at 12:11:17PM +0200, Lorenzo Bianconi wrote:
> > On Thu, Jun 13, 2019 at 11:43:11PM +0200, Lorenzo Bianconi wrote:
> > > Commit f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes
> > > for rx") breaks A-MSDU support. When A-MSDU is enable the device can
> > > receive frames up to q->buf_size but they will be discarded in
> > > mt76u_process_rx_entry since there is no enough room for
> > > skb_shared_info. Fix the issue reallocating the skb and copying in the
> > > linear area the first 128B of the received frames and in the frag_list
> > > the remaining part.
> > >
> > > Fixes: f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for rx")
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > > drivers/net/wireless/mediatek/mt76/mt76.h | 1 +
> > > drivers/net/wireless/mediatek/mt76/usb.c | 49 ++++++++++++++++++-----
> > > 2 files changed, 41 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> > > index 8ecbf81a906f..889b76deb703 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> > > +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> > > @@ -30,6 +30,7 @@
> > > #define MT_TX_RING_SIZE 256
> > > #define MT_MCU_RING_SIZE 32
> > > #define MT_RX_BUF_SIZE 2048
> > > +#define MT_SKB_HEAD_LEN 128
> > >
> > > struct mt76_dev;
> > > struct mt76_wcid;
> > > diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> > > index bbaa1365bbda..12d60d31cb51 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/usb.c
> > > +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> > > @@ -429,6 +429,45 @@ static int mt76u_get_rx_entry_len(u8 *data, u32 data_len)
> > > return dma_len;
> > > }
> > >
> > > +static struct sk_buff *
> > > +mt76u_build_rx_skb(u8 *data, int len, int buf_size)
> > > +{
> > > + struct sk_buff *skb;
> > > +
> > > + if (SKB_WITH_OVERHEAD(buf_size) < MT_DMA_HDR_LEN + len) {
> > > + struct page *page;
> > > + int offset;
> > > +
> > > + /* slow path, not enough space for data and
> > > + * skb_shared_info
> > > + */
> > > + skb = alloc_skb(MT_SKB_HEAD_LEN, GFP_ATOMIC);
> > > + if (!skb)
> > > + return NULL;
> > > +
> > > + skb_put_data(skb, data + MT_DMA_HDR_LEN, MT_SKB_HEAD_LEN);
> >
> > I looked how rx amsdu is processed in mac80211 and it is decomposed and
> > copied into newly allocated individual skb's, see ieee80211_amsdu_to_8023s()
> >
> > So copy L3 & L4 headers doesn't do anything good here, actually seems to
> > be better to have them in frag as __ieee80211_amsdu_copy_frag() do some
> > magic to avid copy.
>
> Looking at __ieee80211_amsdu_copy() now I got why other drivers copy hdrlen +
> 8, thx :)
> In our case reuse_frag is true in __ieee80211_amsdu_copy, so we will end up
I don't think reuse_frag is true in our case since skb->head_frag is
not set when we use alloc_skb().
Stanislaw
^ permalink raw reply
* Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
From: Stanislaw Gruszka @ 2019-06-14 11:31 UTC (permalink / raw)
To: Johannes Berg
Cc: Lorenzo Bianconi, Lorenzo Bianconi, kvalo, linux-wireless, nbd
In-Reply-To: <a50de3e52ece8a636ae902b1a5b901d0d3cd068f.camel@sipsolutions.net>
On Fri, Jun 14, 2019 at 12:20:59PM +0200, Johannes Berg wrote:
> On Fri, 2019-06-14 at 12:11 +0200, Lorenzo Bianconi wrote:
>
> > Looking at __ieee80211_amsdu_copy() now I got why other drivers copy hdrlen +
> > 8, thx :)
> > In our case reuse_frag is true in __ieee80211_amsdu_copy, so we will end up
> > copying 32B + ether_len. Anyway I think 32 is a little bit too low and we could get
> > better performances increasing it a little bit.
> > A typical use case (e.g IPv6 + TCP):
> >
> > IPv6 = 40B, TCP = 32B --> so 72B..I guess 128B is a good value :)
> > @Felix, Johannes: what do you think?
>
> I think while we might *allocate* more, I don't think we should *copy*
> more, since then the TCP payload will no longer be in pages.
>
> It'd probably be better to implement leaving enough tailroom (allocate
> 128), but copying nothing, unless the *entire* packet fits.
iwl4965 put entire packet in fragment in il4965_pass_packet_to_mac80211() .
Initially I thought this is a bug, since mac80211 require header be
in the linear area, but looks like ieee80211_rx_monitor() copy header
before rest of mac80211 check it, so 4965 is fine.
Anyway I think the driver should put ieee80211 header in linear area
and iwlwifi & mt7601u implementation is somewhat optimal.
Stanislaw
^ permalink raw reply
* Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
From: Johannes Berg @ 2019-06-14 11:34 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Lorenzo Bianconi, Lorenzo Bianconi, kvalo, linux-wireless, nbd
In-Reply-To: <20190614113120.GC17298@redhat.com>
On Fri, 2019-06-14 at 13:31 +0200, Stanislaw Gruszka wrote:
> On Fri, Jun 14, 2019 at 12:20:59PM +0200, Johannes Berg wrote:
> > On Fri, 2019-06-14 at 12:11 +0200, Lorenzo Bianconi wrote:
> >
> > > Looking at __ieee80211_amsdu_copy() now I got why other drivers copy hdrlen +
> > > 8, thx :)
> > > In our case reuse_frag is true in __ieee80211_amsdu_copy, so we will end up
> > > copying 32B + ether_len. Anyway I think 32 is a little bit too low and we could get
> > > better performances increasing it a little bit.
> > > A typical use case (e.g IPv6 + TCP):
> > >
> > > IPv6 = 40B, TCP = 32B --> so 72B..I guess 128B is a good value :)
> > > @Felix, Johannes: what do you think?
> >
> > I think while we might *allocate* more, I don't think we should *copy*
> > more, since then the TCP payload will no longer be in pages.
> >
> > It'd probably be better to implement leaving enough tailroom (allocate
> > 128), but copying nothing, unless the *entire* packet fits.
>
> iwl4965 put entire packet in fragment in il4965_pass_packet_to_mac80211() .
> Initially I thought this is a bug, since mac80211 require header be
> in the linear area, but looks like ieee80211_rx_monitor() copy header
> before rest of mac80211 check it, so 4965 is fine.
Mac80211 should not assume anything about header being present or not,
just like the rest of the network stack.
> Anyway I think the driver should put ieee80211 header in linear area
> and iwlwifi & mt7601u implementation is somewhat optimal.
Yes, it's just an optimisation to do the copy-break (copy small packets
completely) or to copy the header already (since we may have better ways
to do so than skb_copy_bits if we still have a virt pointer to the page,
rather than just the page pointer).
johannes
^ permalink raw reply
* Re: [PATCH] mmc: core: Prevent processing SDIO IRQs when the card is suspended
From: Ulf Hansson @ 2019-06-14 11:55 UTC (permalink / raw)
To: Doug Anderson
Cc: Linux MMC List, Adrian Hunter, Brian Norris, Shawn Lin,
Guenter Roeck, Heiko Stuebner, Kalle Valo, linux-wireless, # 4.0+
In-Reply-To: <CAD=FV=WODbZa1fBrLbJBsd77xn5ekHWjks-ydxOSzjdBK83Rmg@mail.gmail.com>
On Thu, 13 Jun 2019 at 20:05, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Jun 13, 2019 at 2:30 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > > > @@ -937,6 +937,10 @@ static int mmc_sdio_pre_suspend(struct mmc_host *host)
> > > > */
> > > > static int mmc_sdio_suspend(struct mmc_host *host)
> > > > {
> > > > + /* Prevent processing of SDIO IRQs in suspended state. */
> > > > + mmc_card_set_suspended(host->card);
> > >
> > > Do you need to claim / release the host around the call to
> > > mmc_card_set_suspended() to avoid races?
> >
> > The intent is that the races should be taken care of like this:
> > 1) In MMC_CAP2_SDIO_IRQ_NOTHREAD case, the call to
> > cancel_delayed_work_sync() below, will make sure that if there are any
> > new work scheduled beyond that point, mmc_card_suspended() will be set
> > and process_sdio_pending_irqs() will bail out.
> >
> > 2. In the non MMC_CAP2_SDIO_IRQ_NOTHREAD case, the call to
> > mmc_claim_host() below will make sure if there is any new attempt to
> > claim the host from the kthread, mmc_card_suspended() will be set and
> > process_sdio_pending_irqs() bails out.
> >
> > Ideally in the long run and want to remove the SDIO kthread, so
> > perhaps this is good enough for now, what do you think?
>
> I was more worried about the safety of mmc_card_set_suspended()
> itself. That is:
>
> #define mmc_card_set_suspended(c) ((c)->state |= MMC_STATE_SUSPENDED)
>
> ...so it's doing a read-modify-write of "state". Is that safe to do
> without any type of locking?
In this case, yes I think so.
The point is, it really doesn't matter if the reader (work or thread),
reads a non-updated value, because the synchronization is managed by
the later mmc_claim_host() and the cancel_delayed_work_sync().
>
>
> > BTW, the important point is that the call to
> > cancel_delayed_work_sync(), must not be done while keeping the host
> > claimed, as then it could deadlock.
>
> Definitely. I was thinking of this sequence:
>
> mmc_claim_host(host);
> mmc_card_set_suspended(host->card);
> mmc_release_host(host);
>
This would work, but claiming/releasing the host is just unnecessary
and "expensive".
> cancel_delayed_work_sync(&host->sdio_irq_work);
>
> mmc_claim_host(host);
>
>
> > > > if (!err && host->sdio_irqs) {
> > > > if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD))
> > > > wake_up_process(host->sdio_irq_thread);
> > > > diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> > > > index 931e6226c0b3..9f54a259a1b3 100644
> > > > --- a/drivers/mmc/core/sdio_irq.c
> > > > +++ b/drivers/mmc/core/sdio_irq.c
> > > > @@ -34,6 +34,10 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
> > > > unsigned char pending;
> > > > struct sdio_func *func;
> > > >
> > > > + /* Don't process SDIO IRQs if the card is suspended. */
> > > > + if (mmc_card_suspended(card))
> > > > + return 0;
> > > > +
> > >
> > > Is it really OK to just return like this? I guess there are two
> > > (somewhat opposite) worries I'd have. See A) and B) below:
> >
> > Let me comment on A) and B) below, for sure there are more problems to address.
> >
> > The main reason to why I think it's okay to bail out here, is because
> > I think it still improves the current behavior a lot. So, rather than
> > solving all problems at once, I wanted to take a step by step
> > approach.
> >
> > >
> > > A) Do we need to do anything extra to make sure we actually call the
> > > interrupt handler after we've resumed? I guess we can't actually
> > > "lose" the interrupt since it will be sitting asserted in CCCR_INTx
> > > until we deal with it (right?), but maybe we need to do something to
> > > ensure the handler gets called once we're done resuming?
> >
> > Good point!
> >
> > Although, it also depends on if we are going to power off the SDIO
> > card or not. In other words, if the SDIO IRQ are configured as a
> > system wakeup.
>
> Even if it's not a system wakeup, we still don't want to drop the
> interrupt on the ground though, do we? For instance, think about a
> level-triggered GPIO interrupt that's _not_ a wakeup interrupt. If
> that gets asserted in suspend then we won't wakeup the system, but as
> soon as the system gets to a certain point in the resume sequence then
> we should pass the interrupt on to the handler. If an edge triggered
> (but non-wakeup) interrupt fires when the system is resuming then we
> should similarly not drop it, should we?
GPIOs is clearly different.
When it comes to SDIO cards, re-playing/re-kicking an SDIO IRQ doesn't
make sense in case the SDIO card was powered off during system
suspend. The reason is simply because the internal state inside the
SDIO card gets lost at a power off. For example, the used SDIO func
number, needs to re-enabled before any SDIO IRQs can be delivered for
it.
So yes, I really think we should drop the SDIO IRQ, unless it's
configured as a wakeup. Simply because it's not valid after the system
has resumed.
>
>
> > Moreover there is another related problem, if the SDIO IRQ are
> > configured as a system wakeup, and if there is an IRQ raised during
> > the system suspend process, the system suspend process should be
> > aborted but it may not. This is another issue that currently isn't
> > supported. The PM core helps to deals with this, but to take advantage
> > of that, the host controller device device must be configured via the
> > common wakeup interfaces, such as the device_init_wakeup(), for
> > example.
>
> As per earlier discussions I don't have any good examples of SDIO IRQs
> being able to wakeup the device to poke at. ...but from GPIO-based
> wakeups I'm used to the suspend code masking the interrupt (so it
> doesn't fire anymore after the suspend call) but leaving it enabled
> and configured as a wakeup. I guess we'd have to think about how that
> translates. Your patch seems to be acting as a "mask" of the
> interrupt, at least on my dw_mmc tests where the hardware presents the
> interrupt like it was edge triggered. ...so it would work OK there
> I'd guess.
>
>
> > > A2): new MMC_CAP2_SDIO_IRQ_NOTHREAD case
> > >
> > > Should we do something to re-kick things? We could call
> > > sdio_signal_irq() in mmc_sdio_resume() I guess? I was worried that
> > > might conflict with those that call sdio_run_irqs() directly but it
> > > seems like that's nobody as of commit 89f3c365f3e1 ("mmc: sdhci: Fix
> > > SDIO IRQ thread deadlock").
> >
> > Good point!
> >
> > Again, whether we should re-kick things depends on if the SDIO IRQ is
> > configured as wakeup, but in general using sdio_signal_irq() should
> > work.
> >
> > The other part I am considering is to disable the SDIO irq, in case of
> > "mmc_card_keep_power() && !mmc_card_wake_sdio_irq()".
> >
> > Moreover, if !mmc_card_keep_power(), then there really shouldn't be
> > any IRQs registered so perhaps we should add a special check for that
> > and return an error code.
>
> I haven't looked through all the details here but I can dig if you
> want. On other drivers it's generally OK to leave your interrupt
> registered (just disabled and/or masked) across suspend/resume, but
> maybe that's not OK for SDIO cards without keep power?
In the end this boils done to dividing the responsibility of whom does
what, at the different layers.
Restoring power to an SDIO card, often ends up with the SDIO func
driver having to re-load a FW and perhaps apply some initial HW
settings for the corresponding chip.
That said, having the mmc core via mmc_sdio_suspend|resume(), to deal
with disable/enable of the SDIO IRQs (and the SDIO func number), seems
like doing things upside-down. It may not even work correctly, until
the chip have been programmed with a new FW.
>
>
> > In regards to other callers of sdio_run_irqs(). I have a patch that
> > makes it this function static, as it really should not need to be used
> > other than from the work queue path. Let me post it asap to cover that
> > gap. Again, thanks for pointing this out!
>
> Yeah, I was thinking of posting that too, but happy to have you do it! :-)
>
>
> > > ...side note: overall looking at this code path, two additional
> > > questions come up for me. One is why sdio_run_irqs() hardcodes
> > > "sdio_irq_pending" as true. That means we won't _ever_ poll CCCR_INTx
> > > in the 1-function case, right? That seems wrong. The other is why
> >
> > In the 1-function case, the idea is that we don't have to read the
> > CCCR_INTx to find out what func number the IRQ belongs to.
> >
> > This is the same behavior consistent as with the kthread case, see
> > mmc_signal_sdio_irq(), unless I am mistaken.
>
> I think there's at least the bug that nothing will ever set
> "sdio_irq_pending" to false in the MMC_CAP2_SDIO_IRQ_NOTHREAD case,
> right? So we'll set it to true the first time and from then on out it
> will never be false again?
Right.
Although, I consider this a different problem (dealing with the things
we have discussed above) and it seems like we should address that as
additional changes on top.
>
>
> > > mmc_sdio_resume() always calls host->ops->enable_sdio_irq(host, 1) at
> > > resume time when nobody ever turned the IRQs off.
> >
> > That's correct and it leads to unbalanced calls of
> > host->ops->enable_sdio_irq(). This needs to be fixed as well.
> >
> > >
> > > ===
> > >
> > > B) Are there any instances where the interrupt will just keep firing
> > > over and over again because we don't handle it?
> > >
> > > As per above, this _isn't_ happening on dw_mmc on my setup because
> > > dw_mmc seems to treat the SDIO interrupt as edge triggered. ...but is
> > > this true everywhere? If we were using SDIO in 1-bit mode on dw_mmc,
> > > would the interrupt re-assert right away? If dw_mmc were configured
> > > to use a dedicated pin would it re-assert right away? What about
> > > other host controllers?
> > >
> > > If you're sure no host controllers will keep asserting the interrupt
> > > over and over then I guess we don't need to worry about it?
> > > ...otherwise we'd need to find some way to mask the interrupt and we'd
> > > need to make sure whatever we do doesn't interfere with anyone who
> > > supports the SDIO interrupt as a wake source, right?
> >
> > For the MMC_CAP2_SDIO_IRQ_NOTHREAD case, the expected behavior by the
> > host driver is to prior calling sdio_signal_irq(), is should temporary
> > disable the SDIO IRQ. Then, when the host->ops->ack_sdio_irq is called
> > from the work, the IRQ has been processed, which tells the host driver
> > to re-enable the SDIO IRQ.
>
> So what I'm imagining is this:
>
> 1. mmc_sdio_suspend() starts; calls mmc_card_set_suspended() and
> cancel_delayed_work_sync().
>
> 2. SDIO interrupt comes in; host controller calls sdio_signal_irq()
>
> 3. sdio_signal_irq() queues delayed work, which gets scheduled right away.
>
> 4. sdio_run_irqs() calls process_sdio_pending_irqs() which is a no-op
> (because we're suspended)
>
> 5. sdio_run_irqs() calls host->ops->ack_sdio_irq(), which re-enables
> more interrupts.
>
> 6. If SDIO interrupt was truly level triggered, we'll go straight back
> to #2 because we never actually removed the true source of the
> level-triggered interrupt by handling it.
>
>
> We'll run steps #2 - #6 above ad nauseam until we finally manage to
> get to the point in the suspend process where the system actually
> masks/disables all driver interrupts. This happens sometime _after_
> the host controller's suspend call happens. Technically this might
> not really hurt anything (other than burning CPU cycles) because the
> system workqueue isn't all that high priority so I think the suspend
> can continue happening while we're looping. ...but it still doesn't
> seem great.
You are right, thanks for point this out so clearly. I have already
started drafting additional patches on top, but allow me a few more
days before I can post them.
In regards to $subject patch, even if it may introduce the behavior
you described above, I still think it's an improvement comparing to
the existing behavior. Allowing to process the SDIO IRQ is just far
worse.
>
> We don't end up in the above situation in my tests because the SDIO
> interrupt was acting as an edge triggered interrupt. ...and because,
> as per below, we eventually turn the clock off.
>
>
> > In the kthread case, this is managed by mmc_signal_sdio_irq() and the
> > sdio_irq_thread() that calls host->ops->enable_sdio_irq() both to
> > enable/disable the IRQ (but there are other problems with that).
> >
> > >
> > > ======
> > >
> > > Overall, I can confirm that on my system your patch actually does
> > > work. ...so if all of the above concerns are moot and won't cause
> > > anyone else problems then I can say that they don't seem to cause any
> > > problems on my system. On rk3288-veyron-jerry:
> > >
> > > - Before your patch, I got failures at iteration 18, then 32, then 55,
> > > then 7, then 26.
> > >
> > > - After your patch I could do 100 iterations of suspend/resume with no
> > > failures. I also put printouts to confirm your patch was having an
> > > effect.
> >
> > Great news, thanks a lot for testing and sharing these result.
> >
> > One more thing to consider. After the system suspend callback have
> > been called for the mmc host driver (assuming SDIO IRQ isn't
> > configured as system wakeup), the host driver shouldn't really receive
> > SDIO IRQs and nor should it signal them via sdio_signal_irq(), simply
> > because it has suspended its device/controller and beyond that point,
> > the behavior might be undefined. Can you check to see if this is
> > happening, or possibly you already know that this is the case and that
> > we are "lucky"?
>
> It's happening fine as long as we're loose with the term "after". :-)
> Most certainly when we just finished executing the last line of the
> host controller's suspend call then the system can't have done
> anything to prevent interrupts from going off. Even if the very next
> thing that the core OS did was to disable interrupts there would still
> be at least a few CPU instructions in there where we could have
> finished the suspend call and interrupts were still enabled at the
> system level.
>
> It looks like the actual suspension of interrupts is in
> suspend_device_irqs() which is called right before the "no irq" calls
> are made. ...so in theory we could still get interrupts for quite a
> while after the host controller's suspend call.
Yes, this is common problem, not specific to this one.
>
> In practice it actually looks to be impossible for dw_mmc, though.
> ...part of dw_mmc's suspend call turns off both the ciu (card clock)
> and biu (bus clock). I believe this means that the controller is
> fully unclocked and there's no way it could give an interrupt.
>
> In fact, the only time we actually get into trouble in dw_mmc is right
> at the beginning of the resume code where we start re-initting the
> host controller (and turning its clocks on) and then the interrupt
> fires before we're quite ready.
Okay, so that we may need to look into sooner or later. Anyway, let's
see what my following patches can help with in regards to this as
well.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v4 2/5] mmc: core: API to temporarily disable retuning for SDIO CRC errors
From: Ulf Hansson @ 2019-06-14 12:04 UTC (permalink / raw)
To: Douglas Anderson
Cc: Kalle Valo, Adrian Hunter, Arend van Spriel,
brcm80211-dev-list.pdl, open list:ARM/Rockchip SoC..., Double Lo,
Brian Norris, linux-wireless, Naveen Gupta, Madhan Mohan R,
Matthias Kaehlcke, Wright Feng, Chi-Hsien Lin, netdev,
brcm80211-dev-list, Jiong Wu, Ritesh Harjani, Allison Randal,
linux-mmc@vger.kernel.org, Linux Kernel Mailing List,
Thomas Gleixner, Greg Kroah-Hartman, Shawn Lin, Wolfram Sang,
Avri Altman
In-Reply-To: <20190613234153.59309-3-dianders@chromium.org>
On Fri, 14 Jun 2019 at 01:42, Douglas Anderson <dianders@chromium.org> wrote:
>
> Normally when the MMC core sees an "-EILSEQ" error returned by a host
> controller then it will trigger a retuning of the card. This is
> generally a good idea.
>
> However, if a command is expected to sometimes cause transfer errors
> then these transfer errors shouldn't cause a re-tuning. This
> re-tuning will be a needless waste of time. One example case where a
> transfer is expected to cause errors is when transitioning between
> idle (sometimes referred to as "sleep" in Broadcom code) and active
> state on certain Broadcom WiFi SDIO cards. Specifically if the card
> was already transitioning between states when the command was sent it
> could cause an error on the SDIO bus.
>
> Let's add an API that the SDIO function drivers can call that will
> temporarily disable the auto-tuning functionality. Then we can add a
> call to this in the Broadcom WiFi driver and any other driver that
> might have similar needs.
>
> NOTE: this makes the assumption that the card is already tuned well
> enough that it's OK to disable the auto-retuning during one of these
> error-prone situations. Presumably the driver code performing the
> error-prone transfer knows how to recover / retry from errors. ...and
> after we can get back to a state where transfers are no longer
> error-prone then we can enable the auto-retuning again. If we truly
> find ourselves in a case where the card needs to be retuned sometimes
> to handle one of these error-prone transfers then we can always try a
> few transfers first without auto-retuning and then re-try with
> auto-retuning if the first few fail.
>
> Without this change on rk3288-veyron-minnie I periodically see this in
> the logs of a machine just sitting there idle:
> dwmmc_rockchip ff0d0000.dwmmc: Successfully tuned phase to XYZ
>
> Fixes: bd11e8bd03ca ("mmc: core: Flag re-tuning is needed on CRC errors")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v4:
> - Moved to SDIO API only (Adrian, Ulf).
> - Renamed to make it less generic, now retune_crc_disable (Ulf).
> - Function header makes it clear host must be claimed (Ulf).
> - No more WARN_ON (Ulf).
>
> Changes in v3:
> - Took out the spinlock since I believe this is all in one context.
>
> Changes in v2:
> - Updated commit message to clarify based on discussion of v1.
>
> drivers/mmc/core/core.c | 5 +++--
> drivers/mmc/core/sdio_io.c | 36 +++++++++++++++++++++++++++++++++++
> include/linux/mmc/core.h | 2 ++
> include/linux/mmc/host.h | 1 +
> include/linux/mmc/sdio_func.h | 3 +++
> 5 files changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 6db36dc870b5..9020cb2490f7 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -144,8 +144,9 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
> int err = cmd->error;
>
> /* Flag re-tuning needed on CRC errors */
> - if ((cmd->opcode != MMC_SEND_TUNING_BLOCK &&
> - cmd->opcode != MMC_SEND_TUNING_BLOCK_HS200) &&
> + if (cmd->opcode != MMC_SEND_TUNING_BLOCK &&
> + cmd->opcode != MMC_SEND_TUNING_BLOCK_HS200 &&
> + !host->retune_crc_disable &&
> (err == -EILSEQ || (mrq->sbc && mrq->sbc->error == -EILSEQ) ||
> (mrq->data && mrq->data->error == -EILSEQ) ||
> (mrq->stop && mrq->stop->error == -EILSEQ)))
> diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
> index f79f0b0caab8..f822a9630b0e 100644
> --- a/drivers/mmc/core/sdio_io.c
> +++ b/drivers/mmc/core/sdio_io.c
> @@ -734,3 +734,39 @@ int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t flags)
> return 0;
> }
> EXPORT_SYMBOL_GPL(sdio_set_host_pm_flags);
> +
> +/**
> + * sdio_retune_crc_disable - temporarily disable retuning on CRC errors
> + * @func: SDIO function attached to host
> + *
> + * If the SDIO card is known to be in a state where it might produce
> + * CRC errors on the bus in response to commands (like if we know it is
> + * transitioning between power states), an SDIO function driver can
> + * call this function to temporarily disable the SD/MMC core behavior of
> + * triggering an automatic retuning.
> + *
> + * This function should be called while the host is claimed and the host
> + * should remain claimed until sdio_retune_crc_enable() is called.
> + * Specifically, the expected sequence of calls is:
> + * - sdio_claim_host()
> + * - sdio_retune_crc_disable()
> + * - some number of calls like sdio_writeb() and sdio_readb()
sdio_retune_crc_enable()
> + * - sdio_release_host()
> + */
> +void sdio_retune_crc_disable(struct sdio_func *func)
> +{
> + func->card->host->retune_crc_disable = true;
> +}
> +EXPORT_SYMBOL_GPL(sdio_retune_crc_disable);
> +
> +/**
> + * sdio_retune_crc_enable - reneable retuning on CRC errors
/s/reneable/re-enable
> + * @func: SDIO function attached to host
> + *
> + * This is the compement to sdio_retune_crc_disable().
> + */
> +void sdio_retune_crc_enable(struct sdio_func *func)
> +{
> + func->card->host->retune_crc_disable = false;
> +}
> +EXPORT_SYMBOL_GPL(sdio_retune_crc_enable);
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 134a6483347a..02a13abf0cda 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -178,6 +178,8 @@ int mmc_wait_for_cmd(struct mmc_host *host, struct mmc_command *cmd,
>
> int mmc_hw_reset(struct mmc_host *host);
> int mmc_sw_reset(struct mmc_host *host);
> +void mmc_expect_errors_begin(struct mmc_host *host);
> +void mmc_expect_errors_end(struct mmc_host *host);
Leftovers for earlier versions.
> void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card);
>
> #endif /* LINUX_MMC_CORE_H */
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 43d0f0c496f6..ecb7972e2423 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -398,6 +398,7 @@ struct mmc_host {
> unsigned int retune_now:1; /* do re-tuning at next req */
> unsigned int retune_paused:1; /* re-tuning is temporarily disabled */
> unsigned int use_blk_mq:1; /* use blk-mq */
> + unsigned int retune_crc_disable:1; /* don't trigger retune upon crc */
>
> int rescan_disable; /* disable card detection */
> int rescan_entered; /* used with nonremovable devices */
> diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
> index e9dfdd501cd1..4820e6d09dac 100644
> --- a/include/linux/mmc/sdio_func.h
> +++ b/include/linux/mmc/sdio_func.h
> @@ -167,4 +167,7 @@ extern void sdio_f0_writeb(struct sdio_func *func, unsigned char b,
> extern mmc_pm_flag_t sdio_get_host_pm_caps(struct sdio_func *func);
> extern int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t flags);
>
> +extern void sdio_retune_crc_disable(struct sdio_func *func);
> +extern void sdio_retune_crc_enable(struct sdio_func *func);
> +
> #endif /* LINUX_MMC_SDIO_FUNC_H */
> --
> 2.22.0.rc2.383.gf4fbbf30c2-goog
>
Besides the minor comments, this looks good to me.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v4 4/5] mmc: core: Add sdio_retune_hold_now() and sdio_retune_release()
From: Ulf Hansson @ 2019-06-14 12:09 UTC (permalink / raw)
To: Douglas Anderson
Cc: Kalle Valo, Adrian Hunter, Arend van Spriel,
brcm80211-dev-list.pdl, open list:ARM/Rockchip SoC..., Double Lo,
Brian Norris, linux-wireless, Naveen Gupta, Madhan Mohan R,
Matthias Kaehlcke, Wright Feng, Chi-Hsien Lin, netdev,
brcm80211-dev-list, linux-mmc@vger.kernel.org,
Linux Kernel Mailing List, Thomas Gleixner, Greg Kroah-Hartman,
Avri Altman
In-Reply-To: <20190613234153.59309-5-dianders@chromium.org>
On Fri, 14 Jun 2019 at 01:42, Douglas Anderson <dianders@chromium.org> wrote:
>
> We want SDIO drivers to be able to temporarily stop retuning when the
> driver knows that the SDIO card is not in a state where retuning will
> work (maybe because the card is asleep). We'll move the relevant
> functions to a place where drivers can call them.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
This looks good to me.
BTW, seems like this series is best funneled via my mmc tree, no? In
such case, I need acks for the brcmfmac driver patches.
Kind regards
Uffe
> ---
>
> Changes in v4:
> - Moved retune hold/release to SDIO API (Adrian).
>
> Changes in v3:
> - ("mmc: core: Export mmc_retune_hold_now() mmc_retune_release()") new for v3.
>
> Changes in v2: None
>
> drivers/mmc/core/sdio_io.c | 40 +++++++++++++++++++++++++++++++++++
> include/linux/mmc/sdio_func.h | 3 +++
> 2 files changed, 43 insertions(+)
>
> diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
> index f822a9630b0e..1b6fe737bd72 100644
> --- a/drivers/mmc/core/sdio_io.c
> +++ b/drivers/mmc/core/sdio_io.c
> @@ -15,6 +15,7 @@
> #include "sdio_ops.h"
> #include "core.h"
> #include "card.h"
> +#include "host.h"
>
> /**
> * sdio_claim_host - exclusively claim a bus for a certain SDIO function
> @@ -770,3 +771,42 @@ void sdio_retune_crc_enable(struct sdio_func *func)
> func->card->host->retune_crc_disable = false;
> }
> EXPORT_SYMBOL_GPL(sdio_retune_crc_enable);
> +
> +/**
> + * sdio_retune_hold_now - start deferring retuning requests till release
> + * @func: SDIO function attached to host
> + *
> + * This function can be called if it's currently a bad time to do
> + * a retune of the SDIO card. Retune requests made during this time
> + * will be held and we'll actually do the retune sometime after the
> + * release.
> + *
> + * This function could be useful if an SDIO card is in a power state
> + * where it can respond to a small subset of commands that doesn't
> + * include the retuning command. Care should be taken when using
> + * this function since (presumably) the retuning request we might be
> + * deferring was made for a good reason.
> + *
> + * This function should be called while the host is claimed.
> + */
> +void sdio_retune_hold_now(struct sdio_func *func)
> +{
> + mmc_retune_hold_now(func->card->host);
> +}
> +EXPORT_SYMBOL_GPL(sdio_retune_hold_now);
> +
> +/**
> + * sdio_retune_release - signal that it's OK to retune now
> + * @func: SDIO function attached to host
> + *
> + * This is the complement to sdio_retune_hold_now(). Calling this
> + * function won't make a retune happen right away but will allow
> + * them to be scheduled normally.
> + *
> + * This function should be called while the host is claimed.
> + */
> +void sdio_retune_release(struct sdio_func *func)
> +{
> + mmc_retune_release(func->card->host);
> +}
> +EXPORT_SYMBOL_GPL(sdio_retune_release);
> diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
> index 4820e6d09dac..5a177f7a83c3 100644
> --- a/include/linux/mmc/sdio_func.h
> +++ b/include/linux/mmc/sdio_func.h
> @@ -170,4 +170,7 @@ extern int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t flags);
> extern void sdio_retune_crc_disable(struct sdio_func *func);
> extern void sdio_retune_crc_enable(struct sdio_func *func);
>
> +extern void sdio_retune_hold_now(struct sdio_func *func);
> +extern void sdio_retune_release(struct sdio_func *func);
> +
> #endif /* LINUX_MMC_SDIO_FUNC_H */
> --
> 2.22.0.rc2.383.gf4fbbf30c2-goog
>
^ permalink raw reply
* Re: [PATCH v3 1/2] nl80211: Add support for EDMG channels
From: Johannes Berg @ 2019-06-14 12:31 UTC (permalink / raw)
To: Alexei Avshalom Lazar; +Cc: linux-wireless, wil6210
In-Reply-To: <1558364020-11064-2-git-send-email-ailizaro@codeaurora.org>
Hi Alexei,
Sorry for the long delay here.
I have a few questions.
Looking at this:
> /**
> + * struct ieee80211_sta_edmg_cap - EDMG capabilities
> + *
> + * This structure describes most essential parameters needed
> + * to describe 802.11ay EDMG capabilities
> + *
> + * @channels: bitmap that indicates the 2.16 GHz channel(s)
> + * that are allowed to be used for transmissions in the BSS.
> + * Set to 0 indicate EDMG not supported.
> + * @bw_config: Channel BW Configuration subfield encodes
> + * the allowed channel bandwidth configurations
> + */
> +struct ieee80211_sta_edmg_cap {
> + u8 channels;
> + u8 bw_config;
> +};
What are the bits actually? Seems you should define some enum or so that
shows which bits are used here?
> * @center_freq1: center frequency of first segment
> * @center_freq2: center frequency of second segment
> * (only with 80+80 MHz)
> + * @edmg_channels: bitmap that indicates the 2.16 GHz channel(s)
> + * that are allowed to be used for transmissions in the BSS.
> + * @edmg_bw_config: Channel BW Configuration subfield encodes
> + * the allowed channel bandwidth configurations
> */
> struct cfg80211_chan_def {
> struct ieee80211_channel *chan;
> enum nl80211_chan_width width;
> u32 center_freq1;
> u32 center_freq2;
> + u8 edmg_channels;
> + u8 edmg_bw_config;
> };
This isn't clear to me. How can the capability and the configuration be
exactly the same? In the capability, you should be able to capture
multiple possible things, and in the setting only choose one?
And if they really are the same, why not use the struct
ieee80211_sta_edmg_cap here? Seems weird to me, but I don't know 11ay.
Also, I think you should describe a bit more how this plays together
with the existing settings. Will ->chan still be set, to something like
the control channel?
> /**
> @@ -1144,15 +1169,17 @@ int cfg80211_check_station_change(struct wiphy *wiphy,
> * @RATE_INFO_FLAGS_MCS: mcs field filled with HT MCS
> * @RATE_INFO_FLAGS_VHT_MCS: mcs field filled with VHT MCS
> * @RATE_INFO_FLAGS_SHORT_GI: 400ns guard interval
> - * @RATE_INFO_FLAGS_60G: 60GHz MCS
> + * @RATE_INFO_FLAGS_DMG: 60GHz MCS
> * @RATE_INFO_FLAGS_HE_MCS: HE MCS information
> + * @RATE_INFO_FLAGS_EDMG: 60GHz MCS in EDMG mode
> */
> enum rate_info_flags {
> RATE_INFO_FLAGS_MCS = BIT(0),
> RATE_INFO_FLAGS_VHT_MCS = BIT(1),
> RATE_INFO_FLAGS_SHORT_GI = BIT(2),
> - RATE_INFO_FLAGS_60G = BIT(3),
> + RATE_INFO_FLAGS_DMG = BIT(3),
> RATE_INFO_FLAGS_HE_MCS = BIT(4),
> + RATE_INFO_FLAGS_EDMG = BIT(5),
> };
>
> /**
> @@ -1192,6 +1219,7 @@ enum rate_info_bw {
> * @he_dcm: HE DCM value
> * @he_ru_alloc: HE RU allocation (from &enum nl80211_he_ru_alloc,
> * only valid if bw is %RATE_INFO_BW_HE_RU)
> + * @n_bonded_ch: In case of EDMG the number of bonded channels (1-4)
> */
> struct rate_info {
> u8 flags;
> @@ -1202,6 +1230,7 @@ struct rate_info {
> u8 he_gi;
> u8 he_dcm;
> u8 he_ru_alloc;
> + u8 n_bonded_ch;
> };
It seems like this is missing corresponding nl80211.h changes?
> @@ -2436,6 +2469,8 @@ struct cfg80211_connect_params {
> const u8 *fils_erp_rrk;
> size_t fils_erp_rrk_len;
> bool want_1x;
> + u8 edmg_channels;
> + u8 edmg_bw_config;
> };
Same question as above, why not embed the struct if it's the same?
Again it seems like it shouldn't be the same though.
> + * @NL80211_ATTR_WIPHY_EDMG_CHANNELS: bitmap that indicates the 2.16 GHz
> + * channel(s) that are allowed to be used for EDMG transmissions in the
> + * BSS as defined by IEEE 802.11 section 9.4.2.251.
> + * @NL80211_ATTR_WIPHY_EDMG_BW_CONFIG: Channel BW Configuration subfield encodes
> + * the allowed channel bandwidth configurations as defined by IEEE 802.11
> + * section 9.4.2.251, Table 13.
This is unclear - "in the BSS" means nothing in this context, since
you're using this to advertise capabilities?
This isn't a BSS attribute, after all.
Ah - but looking further, you use this to *set* the channel, not for
capabilities... I guess that makes sense.
> * @NL80211_BAND_ATTR_VHT_CAPA: VHT capabilities, as in the HT information IE
> * @NL80211_BAND_ATTR_IFTYPE_DATA: nested array attribute, with each entry using
> * attributes from &enum nl80211_band_iftype_attr
> + * @NL80211_BAND_ATTR_EDMG_CHANNELS: bitmap that indicates the 2.16 GHz
> + * channel(s) that are allowed to be used for EDMG transmissions in the
> + * BSS as defined by IEEE 802.11 section 9.4.2.251.
> + * @NL80211_BAND_ATTR_EDMG_BW_CONFIG: Channel BW Configuration subfield
> + * encodes the allowed channel bandwidth configurations as defined by
> + * IEEE 802.11 section 9.4.2.251, Table 13.
> * @NL80211_BAND_ATTR_MAX: highest band attribute currently defined
> * @__NL80211_BAND_ATTR_AFTER_LAST: internal use
And ... that makes more sense than the global attribute I guess?
> +static bool cfg80211_edmg_chandef_valid(const struct cfg80211_chan_def *chandef)
> +{
> + int max_continuous = 0;
> + int num_of_enabled = 0;
> + int continuous = 0;
do you mean "contiguous"? "continuous" doesn't make much sense?
> + int i;
> +
> + if (!chandef->edmg_channels && !chandef->edmg_bw_config)
> + return true;
> +
> + if ((!chandef->edmg_channels && chandef->edmg_bw_config) ||
> + (chandef->edmg_channels && !chandef->edmg_bw_config))
> + return false;
There probably should be some kind of WARN_ON() check that validates you
get here only if the ->chan is actually 60GHz?
> +++ b/net/wireless/nl80211.c
> @@ -288,6 +288,9 @@ static int validate_ie_attr(const struct nlattr *attr,
>
> [NL80211_ATTR_WIPHY_FREQ] = { .type = NLA_U32 },
> [NL80211_ATTR_WIPHY_CHANNEL_TYPE] = { .type = NLA_U32 },
> + [NL80211_ATTR_WIPHY_EDMG_CHANNELS] = { .type = NLA_U8 },
> + [NL80211_ATTR_WIPHY_EDMG_BW_CONFIG] = { .type = NLA_U8 },
You probably want something like NLA_POLICY_RANGE() here? This was only
1-4 IIRC?
> + if (info->attrs[NL80211_ATTR_WIPHY_EDMG_CHANNELS]) {
> + chandef->edmg_channels =
> + nla_get_u8(info->attrs[NL80211_ATTR_WIPHY_EDMG_CHANNELS]);
> +
> + if (info->attrs[NL80211_ATTR_WIPHY_EDMG_BW_CONFIG])
> + chandef->edmg_bw_config =
> + nla_get_u8(info->attrs[NL80211_ATTR_WIPHY_EDMG_BW_CONFIG]);
> + } else {
> + chandef->edmg_bw_config = 0;
> + chandef->edmg_channels = 0;
> + }
> +
> if (!cfg80211_chandef_valid(chandef)) {
So I guess what I suggested above shouldn't actually be a WARN_ON() but
just a check w/o WARN_ON()?
johannes
^ permalink raw reply
* Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
From: Lorenzo Bianconi @ 2019-06-14 12:32 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, kvalo, linux-wireless, nbd, johannes
In-Reply-To: <20190614111414.GB17298@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 3429 bytes --]
> On Fri, Jun 14, 2019 at 12:11:17PM +0200, Lorenzo Bianconi wrote:
> > > On Thu, Jun 13, 2019 at 11:43:11PM +0200, Lorenzo Bianconi wrote:
> > > > Commit f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes
> > > > for rx") breaks A-MSDU support. When A-MSDU is enable the device can
> > > > receive frames up to q->buf_size but they will be discarded in
> > > > mt76u_process_rx_entry since there is no enough room for
> > > > skb_shared_info. Fix the issue reallocating the skb and copying in the
> > > > linear area the first 128B of the received frames and in the frag_list
> > > > the remaining part.
> > > >
> > > > Fixes: f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for rx")
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > > drivers/net/wireless/mediatek/mt76/mt76.h | 1 +
> > > > drivers/net/wireless/mediatek/mt76/usb.c | 49 ++++++++++++++++++-----
> > > > 2 files changed, 41 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> > > > index 8ecbf81a906f..889b76deb703 100644
> > > > --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> > > > +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> > > > @@ -30,6 +30,7 @@
> > > > #define MT_TX_RING_SIZE 256
> > > > #define MT_MCU_RING_SIZE 32
> > > > #define MT_RX_BUF_SIZE 2048
> > > > +#define MT_SKB_HEAD_LEN 128
> > > >
> > > > struct mt76_dev;
> > > > struct mt76_wcid;
> > > > diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> > > > index bbaa1365bbda..12d60d31cb51 100644
> > > > --- a/drivers/net/wireless/mediatek/mt76/usb.c
> > > > +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> > > > @@ -429,6 +429,45 @@ static int mt76u_get_rx_entry_len(u8 *data, u32 data_len)
> > > > return dma_len;
> > > > }
> > > >
> > > > +static struct sk_buff *
> > > > +mt76u_build_rx_skb(u8 *data, int len, int buf_size)
> > > > +{
> > > > + struct sk_buff *skb;
> > > > +
> > > > + if (SKB_WITH_OVERHEAD(buf_size) < MT_DMA_HDR_LEN + len) {
> > > > + struct page *page;
> > > > + int offset;
> > > > +
> > > > + /* slow path, not enough space for data and
> > > > + * skb_shared_info
> > > > + */
> > > > + skb = alloc_skb(MT_SKB_HEAD_LEN, GFP_ATOMIC);
> > > > + if (!skb)
> > > > + return NULL;
> > > > +
> > > > + skb_put_data(skb, data + MT_DMA_HDR_LEN, MT_SKB_HEAD_LEN);
> > >
> > > I looked how rx amsdu is processed in mac80211 and it is decomposed and
> > > copied into newly allocated individual skb's, see ieee80211_amsdu_to_8023s()
> > >
> > > So copy L3 & L4 headers doesn't do anything good here, actually seems to
> > > be better to have them in frag as __ieee80211_amsdu_copy_frag() do some
> > > magic to avid copy.
> >
> > Looking at __ieee80211_amsdu_copy() now I got why other drivers copy hdrlen +
> > 8, thx :)
> > In our case reuse_frag is true in __ieee80211_amsdu_copy, so we will end up
>
> I don't think reuse_frag is true in our case since skb->head_frag is
> not set when we use alloc_skb().
Oh, right. In this case it is probably better to use netdev_alloc_skb().
I will repost using the approach used in mt7601u since for the moment it will
not make any difference to copy more data.
Regards,
Lorenzo
>
> Stanislaw
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] mac80211: add support for the ADDBA extension element
From: Johannes Berg @ 2019-06-14 12:32 UTC (permalink / raw)
To: John Crispin; +Cc: linux-wireless, ath11k, Shashidhar Lakkavalli
In-Reply-To: <20190521150259.2414-2-john@phrozen.org>
>
> +#define IEEE80211_ADDBA_EXT_ELEM_ID 0x9f
That doesn't really belong here.
> +#define IEEE80211_ADDBA_EXT_ELEM_ID_LEN 0x01
And that's usually not a define?
> +#define IEEE80211_ADDBA_EXT_FRAG_LEVEL_MASK GENMASK(2, 1)
> +#define IEEE80211_ADDBA_EXT_FRAG_LEVEL_SHIFT 1
> +#define IEEE80211_ADDBA_EXT_NO_FRAG BIT(0)
> +
> +struct ieee80211_addbaext {
> + u8 eid;
> + u8 length;
> + u8 data;
> +} __packed;
I think we don't typically put the eid/len inside, and then we use
sizeof(struct ...) for the size.
johannes
^ permalink raw reply
* Re: [PATCH V2 4/6] mac80211: HE: add Spatial Reuse IE parsing support
From: Johannes Berg @ 2019-06-14 12:34 UTC (permalink / raw)
To: John Crispin; +Cc: linux-wireless, ath11k, Shashidhar Lakkavalli
In-Reply-To: <20190528114952.838-5-john@phrozen.org>
> +/*
> + * ieee80211_he_spr_size - calculate 802.11ax HE Spatial Reuse IE size
> + * @he_spr_ie: byte data of the He Spatial Reuse IE, stating from the the byte
> + * after the ext ID byte. It is assumed that he_spr_ie has at least
> + * sizeof(struct ieee80211_he_spr) bytes, checked already in
> + * ieee802_11_parse_elems_crc()
That reference to mac80211 isn't really quite appropriate here - should
say something like "the caller must have validated this". This is a
lower layer usable by non-mac80211 as well.
johannes
^ permalink raw reply
* Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
From: Lorenzo Bianconi @ 2019-06-14 12:46 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, kvalo, linux-wireless, nbd
In-Reply-To: <20190614110442.GA17298@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2738 bytes --]
> On Fri, Jun 14, 2019 at 12:22:48PM +0200, Lorenzo Bianconi wrote:
> > > On Thu, Jun 13, 2019 at 11:43:13PM +0200, Lorenzo Bianconi wrote:
> > > > Set usb buffer size taking into account skb_shared_info in order to
> > > > not always copy the first part of received frames if A-MSDU is enabled
> > > > for SG capable devices. Moreover align usb buffer size to max_ep
> > > > boundaries and set buf_size to PAGE_SIZE even for sg case
> > >
> > > I think this should not be applied to wirless-drivers, only first patch
> > > that fix the bug and optimizations should be done in -next.
> >
> > ack, right. I think patch 2/3 and 3/3 can go directly in Felix's tree
> >
> > >
> > > > + int i, data_size;
> > > >
> > > > + data_size = rounddown(SKB_WITH_OVERHEAD(q->buf_size),
> > > > + dev->usb.in_ep[MT_EP_IN_PKT_RX].max_packet);
> > > > for (i = 0; i < nsgs; i++) {
> > > > struct page *page;
> > > > void *data;
> > > > @@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb,
> > > >
> > > > page = virt_to_head_page(data);
> > > > offset = data - page_address(page);
> > > > - sg_set_page(&urb->sg[i], page, q->buf_size, offset);
> > > > + sg_set_page(&urb->sg[i], page, data_size, offset);
> > > <snip>
> > > > - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> > > > q->ndesc = MT_NUM_RX_ENTRIES;
> > > > + q->buf_size = PAGE_SIZE;
> > > > +
> > >
> > > This should be associated with decrease of MT_SG_MAX_SIZE to value that
> > > is actually needed and currently this is 2 for 4k AMSDU.
> >
> > MT_SG_MAX_SIZE is used even on tx side and I do not think we will end up with a
> > huge difference here
>
> So use different value as argument for mt76u_fill_rx_sg() in
> mt76u_rx_urb_alloc(). After changing buf_size to PAGE_SIZE we will
> allocate 8 pages per rx queue entry, but only 2 pages will be used
> (with data_size change, 1 without data_size change). Or I'm wrong?
yes, it is right (we will use two pages with data_size change). Maybe better to
use 4 pages for each rx queue entry? (otherwise we will probably change it in
the future)
>
> > > However I don't think allocating 2 pages to avoid ieee80211 header and SNAP
> > > copy is worth to do. For me best approach would be allocate 1 page for
> > > 4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without data_size
> > > change to avoid 32B copying).
> >
> > From my point of view it is better to avoid copying if it is possible. Are you
> > sure there is no difference?
>
> I do not understand what you mean by difference here.
tpt differences, not sure if there are any
Regards,
Lorenzo
>
> Stanislaw
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [PATCH v2] Revert "cfg80211: fix processing world regdomain when non modular"
From: Hodaszi, Robert @ 2019-06-14 13:16 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless@vger.kernel.org
This reverts commit 96cce12ff6e0bc9d9fcb2235e08b7fc150f96fd2.
Re-triggering a reg_process_hint with the last request on all events,
can make the regulatory domain fail in case of multiple WiFi modules. On
slower boards (espacially with mdev), enumeration of the WiFi modules
can end up in an intersected regulatory domain, and user cannot set it
with 'iw reg set' anymore.
This is happening, because:
- 1st module enumerates, queues up a regulatory request
- request gets processed by __reg_process_hint_driver():
- checks if previous was set by CORE -> yes
- checks if regulator domain changed -> yes, from '00' to e.g. 'US'
-> sends request to the 'crda'
- 2nd module enumerates, queues up a regulator request (which triggers
the reg_todo() work)
- reg_todo() -> reg_process_pending_hints() sees, that the last request
is not processed yet, so it tries to process it again.
__reg_process_hint driver() will run again, and:
- checks if the last request's initiator was the core -> no, it was
the driver (1st WiFi module)
- checks, if the previous initiator was the driver -> yes
- checks if the regulator domain changed -> yes, it was '00' (set by
core, and crda call did not return yet), and should be changed to 'US'
------> __reg_process_hint_driver calls an intersect
Besides, the reg_process_hint call with the last request is meaningless
since the crda call has a timeout work. If that timeout expires, the
first module's request will lost.
Signed-off-by: Robert Hodaszi <robert.hodaszi@digi.com>
---
net/wireless/reg.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 4831ad745f91..327479ce69f5 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -2788,7 +2788,7 @@ static void reg_process_pending_hints(void)
/* When last_request->processed becomes true this will be rescheduled */
if (lr && !lr->processed) {
- reg_process_hint(lr);
+ pr_debug("Pending regulatory request, waiting for it to be processed...\n");
return;
}
^ permalink raw reply related
* [mac80211-next:master 17/20] drivers/net/wireless/intel/iwlwifi/dvm/rs.c:3317:3: error: 'const struct rate_control_ops' has no member named 'remove_sta_debugfs'; did you mean 'add_sta_debugfs'?
From: kbuild test robot @ 2019-06-14 13:17 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: kbuild-all, linux-wireless, Johannes Berg
[-- Attachment #1: Type: text/plain, Size: 5088 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git master
head: 499169fb3a5a27b8322cdd559a09e79e390c211b
commit: d3f7e94bc458dd1b20bef591ee2b82e2ae631858 [17/20] mac80211: remove unused and unneeded remove_sta_debugfs callback
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout d3f7e94bc458dd1b20bef591ee2b82e2ae631858
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=xtensa
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/net/wireless/intel/iwlwifi/dvm/rs.c:3317:3: error: 'const struct rate_control_ops' has no member named 'remove_sta_debugfs'; did you mean 'add_sta_debugfs'?
.remove_sta_debugfs = rs_remove_debugfs,
^~~~~~~~~~~~~~~~~~
add_sta_debugfs
>> drivers/net/wireless/intel/iwlwifi/dvm/rs.c:3317:24: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.remove_sta_debugfs = rs_remove_debugfs,
^~~~~~~~~~~~~~~~~
drivers/net/wireless/intel/iwlwifi/dvm/rs.c:3317:24: note: (near initialization for 'rs_ops.get_expected_throughput')
cc1: some warnings being treated as errors
--
>> drivers/net/wireless/intel/iwlwifi/mvm/rs.c:4138:3: error: 'const struct rate_control_ops' has no member named 'remove_sta_debugfs'; did you mean 'add_sta_debugfs'?
.remove_sta_debugfs = rs_remove_sta_debugfs,
^~~~~~~~~~~~~~~~~~
add_sta_debugfs
>> drivers/net/wireless/intel/iwlwifi/mvm/rs.c:4138:24: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.remove_sta_debugfs = rs_remove_sta_debugfs,
^~~~~~~~~~~~~~~~~~~~~
drivers/net/wireless/intel/iwlwifi/mvm/rs.c:4138:24: note: (near initialization for 'rs_mvm_ops_drv.get_expected_throughput')
cc1: some warnings being treated as errors
--
>> drivers/net//wireless/intel/iwlegacy/4965-rs.c:2815:3: error: 'const struct rate_control_ops' has no member named 'remove_sta_debugfs'; did you mean 'add_sta_debugfs'?
.remove_sta_debugfs = il4965_rs_remove_debugfs,
^~~~~~~~~~~~~~~~~~
add_sta_debugfs
>> drivers/net//wireless/intel/iwlegacy/4965-rs.c:2815:24: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
.remove_sta_debugfs = il4965_rs_remove_debugfs,
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/net//wireless/intel/iwlegacy/4965-rs.c:2815:24: note: (near initialization for 'rs_4965_ops.get_expected_throughput')
cc1: some warnings being treated as errors
vim +3317 drivers/net/wireless/intel/iwlwifi/dvm/rs.c
631ad703b drivers/net/wireless/iwlwifi/dvm/rs.c Johannes Berg 2014-01-20 3305
631ad703b drivers/net/wireless/iwlwifi/dvm/rs.c Johannes Berg 2014-01-20 3306 static const struct rate_control_ops rs_ops = {
b481de9ca drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-25 3307 .name = RS_NAME,
b481de9ca drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-25 3308 .tx_status = rs_tx_status,
b481de9ca drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-25 3309 .get_rate = rs_get_rate,
fe6b23dd3 drivers/net/wireless/iwlwifi/iwl-agn-rs.c Reinette Chatre 2010-02-22 3310 .rate_init = rs_rate_init_stub,
b481de9ca drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-25 3311 .alloc = rs_alloc,
b481de9ca drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-25 3312 .free = rs_free,
b481de9ca drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-25 3313 .alloc_sta = rs_alloc_sta,
b481de9ca drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-25 3314 .free_sta = rs_free_sta,
93dc646ad drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-27 3315 #ifdef CONFIG_MAC80211_DEBUGFS
93dc646ad drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-27 3316 .add_sta_debugfs = rs_add_debugfs,
93dc646ad drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-27 @3317 .remove_sta_debugfs = rs_remove_debugfs,
93dc646ad drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-27 3318 #endif
b481de9ca drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-25 3319 };
b481de9ca drivers/net/wireless/iwlwifi/iwl-4965-rs.c Zhu Yi 2007-09-25 3320
:::::: The code at line 3317 was first introduced by commit
:::::: 93dc646adb94127ca1c2e74275a85265ec57b9af [PATCH] iwlwifi: add debugfs framework to rate scale
:::::: TO: Zhu Yi <yi.zhu@intel.com>
:::::: CC: David S. Miller <davem@sunset.davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58622 bytes --]
^ permalink raw reply
* Re: [PATCH v2] Revert "cfg80211: fix processing world regdomain when non modular"
From: Johannes Berg @ 2019-06-14 13:30 UTC (permalink / raw)
To: Hodaszi, Robert; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <20190614131600.GA13897@a1-hr>
On Fri, 2019-06-14 at 13:16 +0000, Hodaszi, Robert wrote:
> This reverts commit 96cce12ff6e0bc9d9fcb2235e08b7fc150f96fd2.
>
> Re-triggering a reg_process_hint with the last request on all events,
> can make the regulatory domain fail in case of multiple WiFi modules. On
> slower boards (espacially with mdev), enumeration of the WiFi modules
> can end up in an intersected regulatory domain, and user cannot set it
> with 'iw reg set' anymore.
>
> This is happening, because:
> - 1st module enumerates, queues up a regulatory request
> - request gets processed by __reg_process_hint_driver():
> - checks if previous was set by CORE -> yes
> - checks if regulator domain changed -> yes, from '00' to e.g. 'US'
> -> sends request to the 'crda'
> - 2nd module enumerates, queues up a regulator request (which triggers
> the reg_todo() work)
> - reg_todo() -> reg_process_pending_hints() sees, that the last request
> is not processed yet, so it tries to process it again.
> __reg_process_hint driver() will run again, and:
> - checks if the last request's initiator was the core -> no, it was
> the driver (1st WiFi module)
> - checks, if the previous initiator was the driver -> yes
> - checks if the regulator domain changed -> yes, it was '00' (set by
> core, and crda call did not return yet), and should be changed to 'US'
>
> ------> __reg_process_hint_driver calls an intersect
>
> Besides, the reg_process_hint call with the last request is meaningless
> since the crda call has a timeout work. If that timeout expires, the
> first module's request will lost.
It's pointless to resend when I still have the original patch pending,
at least without any changes.
That said, I looked at this today and I'm not sure how the code doesn't
now have the original issue again?
johannes
^ permalink raw reply
* Re: [PATCH-v3 1/2] wireless: Support assoc-at-ms timer in sta-info
From: Johannes Berg @ 2019-06-14 13:38 UTC (permalink / raw)
To: greearb, linux-wireless
In-Reply-To: <20190415172123.6532-1-greearb@candelatech.com>
On Mon, 2019-04-15 at 10:21 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> Report time stamp of when sta became associated.
I guess that makes sense.
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> include/net/cfg80211.h | 2 ++
> include/uapi/linux/nl80211.h | 2 ++
> net/wireless/nl80211.c | 1 +
> 3 files changed, 5 insertions(+)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index f49eb1464b7a..a3ad78b9d9f4 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1268,6 +1268,7 @@ struct cfg80211_tid_stats {
> * indicate the relevant values in this struct for them
> * @connected_time: time(in secs) since a station is last connected
> * @inactive_time: time since last station activity (tx/rx) in milliseconds
> + * @assoc_at_ms: time in ms of the last association
I think the "_at_ms" doesn't really make sense. "time in ms" also
doesn't make sense as documentation. I think you need to say what should
be assigned here.
Also, you're actually using ktime_get_real() for this, which again
doesn't make much sense. For exposing an absolute time, I think we're
better off with CLOCK_BOOTTIME like we use elsewhere:
* @boottime_ns: CLOCK_BOOTTIME timestamp the frame was received at, this is
* needed only for beacons and probe responses that update the scan cache.
> + * @NL80211_STA_INFO_ASSOC_AT_MS: Timestamp of last association
_ASSOC_TIMESTAMP or something would make more sense too as the attribute
name, and clearly the documentation needs to spell out the semantics
here too.
johannes
^ permalink raw reply
* Re: [PATCH 1/3] net: wireless: trace: add trace for tx_mgmt_expired
From: Johannes Berg @ 2019-06-14 13:42 UTC (permalink / raw)
To: James Prestwood, linux-wireless
In-Reply-To: <20190612193510.27680-1-james.prestwood@linux.intel.com>
Applied, but
* I squashed 1 and 2, there's no point in 1 standalone
* please be more careful with indentation in the future,
I fixed a few places.
johannes
^ permalink raw reply
* [PATCH 02/16] drm/ati_pcigart: stop using drm_pci_alloc
From: Christoph Hellwig @ 2019-06-14 13:47 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ian Abbott, H Hartley Sweeten
Cc: Intel Linux Wireless, moderated list:ARM PORT, dri-devel,
intel-gfx, linux-rdma, linux-media, netdev, linux-wireless,
linux-s390, devel, linux-mm, iommu, linux-kernel
In-Reply-To: <20190614134726.3827-1-hch@lst.de>
Remove usage of the legacy drm PCI DMA wrappers, and with that the
incorrect usage cocktail of __GFP_COMP, virt_to_page on DMA allocation
and SetPageReserved.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/gpu/drm/ati_pcigart.c | 27 +++++++++++----------------
include/drm/ati_pcigart.h | 5 ++++-
2 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/ati_pcigart.c b/drivers/gpu/drm/ati_pcigart.c
index 2362f07fe1fc..f66d4fccd812 100644
--- a/drivers/gpu/drm/ati_pcigart.c
+++ b/drivers/gpu/drm/ati_pcigart.c
@@ -41,21 +41,14 @@
static int drm_ati_alloc_pcigart_table(struct drm_device *dev,
struct drm_ati_pcigart_info *gart_info)
{
- gart_info->table_handle = drm_pci_alloc(dev, gart_info->table_size,
- PAGE_SIZE);
- if (gart_info->table_handle == NULL)
+ gart_info->table_vaddr = dma_alloc_coherent(&dev->pdev->dev,
+ gart_info->table_size, &gart_info->table_handle,
+ GFP_KERNEL);
+ if (!gart_info->table_vaddr)
return -ENOMEM;
-
return 0;
}
-static void drm_ati_free_pcigart_table(struct drm_device *dev,
- struct drm_ati_pcigart_info *gart_info)
-{
- drm_pci_free(dev, gart_info->table_handle);
- gart_info->table_handle = NULL;
-}
-
int drm_ati_pcigart_cleanup(struct drm_device *dev, struct drm_ati_pcigart_info *gart_info)
{
struct drm_sg_mem *entry = dev->sg;
@@ -87,8 +80,10 @@ int drm_ati_pcigart_cleanup(struct drm_device *dev, struct drm_ati_pcigart_info
}
if (gart_info->gart_table_location == DRM_ATI_GART_MAIN &&
- gart_info->table_handle) {
- drm_ati_free_pcigart_table(dev, gart_info);
+ gart_info->table_vaddr) {
+ dma_free_coherent(&dev->pdev->dev, gart_info->table_size,
+ gart_info->table_vaddr, gart_info->table_handle);
+ gart_info->table_vaddr = NULL;
}
return 1;
@@ -127,9 +122,9 @@ int drm_ati_pcigart_init(struct drm_device *dev, struct drm_ati_pcigart_info *ga
goto done;
}
- pci_gart = gart_info->table_handle->vaddr;
- address = gart_info->table_handle->vaddr;
- bus_address = gart_info->table_handle->busaddr;
+ pci_gart = gart_info->table_vaddr;
+ address = gart_info->table_vaddr;
+ bus_address = gart_info->table_handle;
} else {
address = gart_info->addr;
bus_address = gart_info->bus_addr;
diff --git a/include/drm/ati_pcigart.h b/include/drm/ati_pcigart.h
index a728a1364e66..2ffe278ba3b3 100644
--- a/include/drm/ati_pcigart.h
+++ b/include/drm/ati_pcigart.h
@@ -18,7 +18,10 @@ struct drm_ati_pcigart_info {
void *addr;
dma_addr_t bus_addr;
dma_addr_t table_mask;
- struct drm_dma_handle *table_handle;
+
+ dma_addr_t table_handle;
+ void *table_vaddr;
+
struct drm_local_map mapping;
int table_size;
};
--
2.20.1
^ permalink raw reply related
* use exact allocation for dma coherent memory
From: Christoph Hellwig @ 2019-06-14 13:47 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ian Abbott, H Hartley Sweeten
Cc: Intel Linux Wireless, moderated list:ARM PORT, dri-devel,
intel-gfx, linux-rdma, linux-media, netdev, linux-wireless,
linux-s390, devel, linux-mm, iommu, linux-kernel
Hi all,
various architectures have used exact memory allocations for dma
allocations for a long time, but x86 and thus the common code based
on it kept using our normal power of two allocator, which tends to
waste a lot of memory for certain allocations.
Switching to a slightly cleaned up alloc_pages_exact is pretty easy,
but it turns out that because we didn't filter valid gfp_t flags
on the DMA allocator, a bunch of drivers were passing __GFP_COMP
to it, which is rather bogus in too many ways to explain. Arm has
been filtering it for a while, but this series instead tries to fix
the drivers and warn when __GFP_COMP is passed, which makes it much
larger than just adding the functionality.
^ permalink raw reply
* [PATCH 06/16] drm: don't pass __GFP_COMP to dma_alloc_coherent in drm_pci_alloc
From: Christoph Hellwig @ 2019-06-14 13:47 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ian Abbott, H Hartley Sweeten
Cc: Intel Linux Wireless, moderated list:ARM PORT, dri-devel,
intel-gfx, linux-rdma, linux-media, netdev, linux-wireless,
linux-s390, devel, linux-mm, iommu, linux-kernel
In-Reply-To: <20190614134726.3827-1-hch@lst.de>
The memory returned from dma_alloc_coherent is opaqueue to the user,
thus the exact way of page refcounting shall not matter either.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/gpu/drm/drm_bufs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index b640437ce90f..7a79a16a055c 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -70,7 +70,7 @@ drm_dma_handle_t *drm_pci_alloc(struct drm_device * dev, size_t size, size_t ali
dmah->size = size;
dmah->vaddr = dma_alloc_coherent(&dev->pdev->dev, size,
&dmah->busaddr,
- GFP_KERNEL | __GFP_COMP);
+ GFP_KERNEL);
if (dmah->vaddr == NULL) {
kfree(dmah);
--
2.20.1
^ permalink raw reply related
* [PATCH 10/16] iwlwifi: stop passing bogus gfp flags arguments to dma_alloc_coherent
From: Christoph Hellwig @ 2019-06-14 13:47 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ian Abbott, H Hartley Sweeten
Cc: Intel Linux Wireless, moderated list:ARM PORT, dri-devel,
intel-gfx, linux-rdma, linux-media, netdev, linux-wireless,
linux-s390, devel, linux-mm, iommu, linux-kernel
In-Reply-To: <20190614134726.3827-1-hch@lst.de>
dma_alloc_coherent is not just the page allocator. The only valid
arguments to pass are either GFP_ATOMIC or GFP_ATOMIC with possible
modifiers of __GFP_NORETRY or __GFP_NOWARN.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/net/wireless/intel/iwlwifi/fw/dbg.c | 3 +--
drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
index 5f52e40a2903..323dc5d5ee88 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
+++ b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
@@ -2361,8 +2361,7 @@ iwl_fw_dbg_buffer_allocation(struct iwl_fw_runtime *fwrt, u32 size)
virtual_addr =
dma_alloc_coherent(fwrt->trans->dev, size, &phys_addr,
- GFP_KERNEL | __GFP_NOWARN | __GFP_ZERO |
- __GFP_COMP);
+ GFP_KERNEL | __GFP_NOWARN);
/* TODO: alloc fragments if needed */
if (!virtual_addr)
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index 803fcbac4152..22a47f928dc8 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -210,8 +210,7 @@ static void iwl_pcie_alloc_fw_monitor_block(struct iwl_trans *trans,
for (power = max_power; power >= min_power; power--) {
size = BIT(power);
cpu_addr = dma_alloc_coherent(trans->dev, size, &phys,
- GFP_KERNEL | __GFP_NOWARN |
- __GFP_ZERO | __GFP_COMP);
+ GFP_KERNEL | __GFP_NOWARN);
if (!cpu_addr)
continue;
--
2.20.1
^ permalink raw reply related
* [PATCH 08/16] IB/qib: stop passing bogus gfp flags arguments to dma_alloc_coherent
From: Christoph Hellwig @ 2019-06-14 13:47 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ian Abbott, H Hartley Sweeten
Cc: Intel Linux Wireless, moderated list:ARM PORT, dri-devel,
intel-gfx, linux-rdma, linux-media, netdev, linux-wireless,
linux-s390, devel, linux-mm, iommu, linux-kernel
In-Reply-To: <20190614134726.3827-1-hch@lst.de>
dma_alloc_coherent is not just the page allocator. The only valid
arguments to pass are either GFP_ATOMIC or GFP_ATOMIC with possible
modifiers of __GFP_NORETRY or __GFP_NOWARN.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/infiniband/hw/qib/qib_iba6120.c | 2 +-
drivers/infiniband/hw/qib/qib_init.c | 20 +++-----------------
2 files changed, 4 insertions(+), 18 deletions(-)
diff --git a/drivers/infiniband/hw/qib/qib_iba6120.c b/drivers/infiniband/hw/qib/qib_iba6120.c
index 531d8a1db2c3..d8a0b8993d22 100644
--- a/drivers/infiniband/hw/qib/qib_iba6120.c
+++ b/drivers/infiniband/hw/qib/qib_iba6120.c
@@ -2076,7 +2076,7 @@ static void alloc_dummy_hdrq(struct qib_devdata *dd)
dd->cspec->dummy_hdrq = dma_alloc_coherent(&dd->pcidev->dev,
dd->rcd[0]->rcvhdrq_size,
&dd->cspec->dummy_hdrq_phys,
- GFP_ATOMIC | __GFP_COMP);
+ GFP_ATOMIC);
if (!dd->cspec->dummy_hdrq) {
qib_devinfo(dd->pcidev, "Couldn't allocate dummy hdrq\n");
/* fallback to just 0'ing */
diff --git a/drivers/infiniband/hw/qib/qib_init.c b/drivers/infiniband/hw/qib/qib_init.c
index d4fd8a6cff7b..072885a6684d 100644
--- a/drivers/infiniband/hw/qib/qib_init.c
+++ b/drivers/infiniband/hw/qib/qib_init.c
@@ -1547,18 +1547,13 @@ int qib_create_rcvhdrq(struct qib_devdata *dd, struct qib_ctxtdata *rcd)
if (!rcd->rcvhdrq) {
dma_addr_t phys_hdrqtail;
- gfp_t gfp_flags;
-
amt = ALIGN(dd->rcvhdrcnt * dd->rcvhdrentsize *
sizeof(u32), PAGE_SIZE);
- gfp_flags = (rcd->ctxt >= dd->first_user_ctxt) ?
- GFP_USER : GFP_KERNEL;
old_node_id = dev_to_node(&dd->pcidev->dev);
set_dev_node(&dd->pcidev->dev, rcd->node_id);
rcd->rcvhdrq = dma_alloc_coherent(
- &dd->pcidev->dev, amt, &rcd->rcvhdrq_phys,
- gfp_flags | __GFP_COMP);
+ &dd->pcidev->dev, amt, &rcd->rcvhdrq_phys, GFP_KERNEL);
set_dev_node(&dd->pcidev->dev, old_node_id);
if (!rcd->rcvhdrq) {
@@ -1578,7 +1573,7 @@ int qib_create_rcvhdrq(struct qib_devdata *dd, struct qib_ctxtdata *rcd)
set_dev_node(&dd->pcidev->dev, rcd->node_id);
rcd->rcvhdrtail_kvaddr = dma_alloc_coherent(
&dd->pcidev->dev, PAGE_SIZE, &phys_hdrqtail,
- gfp_flags);
+ GFP_KERNEL);
set_dev_node(&dd->pcidev->dev, old_node_id);
if (!rcd->rcvhdrtail_kvaddr)
goto bail_free;
@@ -1622,17 +1617,8 @@ int qib_setup_eagerbufs(struct qib_ctxtdata *rcd)
struct qib_devdata *dd = rcd->dd;
unsigned e, egrcnt, egrperchunk, chunk, egrsize, egroff;
size_t size;
- gfp_t gfp_flags;
int old_node_id;
- /*
- * GFP_USER, but without GFP_FS, so buffer cache can be
- * coalesced (we hope); otherwise, even at order 4,
- * heavy filesystem activity makes these fail, and we can
- * use compound pages.
- */
- gfp_flags = __GFP_RECLAIM | __GFP_IO | __GFP_COMP;
-
egrcnt = rcd->rcvegrcnt;
egroff = rcd->rcvegr_tid_base;
egrsize = dd->rcvegrbufsize;
@@ -1664,7 +1650,7 @@ int qib_setup_eagerbufs(struct qib_ctxtdata *rcd)
rcd->rcvegrbuf[e] =
dma_alloc_coherent(&dd->pcidev->dev, size,
&rcd->rcvegrbuf_phys[e],
- gfp_flags);
+ GFP_KERNEL);
set_dev_node(&dd->pcidev->dev, old_node_id);
if (!rcd->rcvegrbuf[e])
goto bail_rcvegrbuf_phys;
--
2.20.1
^ permalink raw reply related
* [PATCH 16/16] dma-mapping: use exact allocation in dma_alloc_contiguous
From: Christoph Hellwig @ 2019-06-14 13:47 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
Daniel Vetter, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ian Abbott, H Hartley Sweeten
Cc: Intel Linux Wireless, moderated list:ARM PORT, dri-devel,
intel-gfx, linux-rdma, linux-media, netdev, linux-wireless,
linux-s390, devel, linux-mm, iommu, linux-kernel
In-Reply-To: <20190614134726.3827-1-hch@lst.de>
Many architectures (e.g. arm, m68 and sh) have always used exact
allocation in their dma coherent allocator, which avoids a lot of
memory waste especially for larger allocations. Lift this behavior
into the generic allocator so that dma-direct and the generic IOMMU
code benefit from this behavior as well.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
include/linux/dma-contiguous.h | 8 +++++---
kernel/dma/contiguous.c | 17 +++++++++++------
2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/include/linux/dma-contiguous.h b/include/linux/dma-contiguous.h
index c05d4e661489..2e542e314acf 100644
--- a/include/linux/dma-contiguous.h
+++ b/include/linux/dma-contiguous.h
@@ -161,15 +161,17 @@ static inline struct page *dma_alloc_contiguous(struct device *dev, size_t size,
gfp_t gfp)
{
int node = dev ? dev_to_node(dev) : NUMA_NO_NODE;
- size_t align = get_order(PAGE_ALIGN(size));
+ void *cpu_addr = alloc_pages_exact_node(node, size, gfp);
- return alloc_pages_node(node, gfp, align);
+ if (!cpu_addr)
+ return NULL;
+ return virt_to_page(p);
}
static inline void dma_free_contiguous(struct device *dev, struct page *page,
size_t size)
{
- __free_pages(page, get_order(size));
+ free_pages_exact(page_address(page), get_order(size));
}
#endif
diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
index bfc0c17f2a3d..84f41eea2741 100644
--- a/kernel/dma/contiguous.c
+++ b/kernel/dma/contiguous.c
@@ -232,9 +232,8 @@ struct page *dma_alloc_contiguous(struct device *dev, size_t size, gfp_t gfp)
{
int node = dev ? dev_to_node(dev) : NUMA_NO_NODE;
size_t count = PAGE_ALIGN(size) >> PAGE_SHIFT;
- size_t align = get_order(PAGE_ALIGN(size));
- struct page *page = NULL;
struct cma *cma = NULL;
+ void *cpu_addr;
if (dev && dev->cma_area)
cma = dev->cma_area;
@@ -243,14 +242,20 @@ struct page *dma_alloc_contiguous(struct device *dev, size_t size, gfp_t gfp)
/* CMA can be used only in the context which permits sleeping */
if (cma && gfpflags_allow_blocking(gfp)) {
+ size_t align = get_order(PAGE_ALIGN(size));
+ struct page *page;
+
align = min_t(size_t, align, CONFIG_CMA_ALIGNMENT);
page = cma_alloc(cma, count, align, gfp & __GFP_NOWARN);
+ if (page)
+ return page;
}
/* Fallback allocation of normal pages */
- if (!page)
- page = alloc_pages_node(node, gfp, align);
- return page;
+ cpu_addr = alloc_pages_exact_node(node, size, gfp);
+ if (!cpu_addr)
+ return NULL;
+ return virt_to_page(cpu_addr);
}
/**
@@ -267,7 +272,7 @@ struct page *dma_alloc_contiguous(struct device *dev, size_t size, gfp_t gfp)
void dma_free_contiguous(struct device *dev, struct page *page, size_t size)
{
if (!cma_release(dev_get_cma_area(dev), page, size >> PAGE_SHIFT))
- __free_pages(page, get_order(size));
+ free_pages_exact(page_address(page), get_order(size));
}
/*
--
2.20.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox