* Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
From: Måns Rullgård @ 2017-07-31 15:28 UTC (permalink / raw)
To: Mason; +Cc: Florian Fainelli, Marc Gonzalez, netdev, Linux ARM
In-Reply-To: <b1a6cd1d-ce02-708f-6c8e-defe0d8f3cd8@free.fr>
Mason <slash.tmp@free.fr> writes:
> On 31/07/2017 16:08, Mason wrote:
>
>> Other things make no sense to me, for example in nb8800_dma_stop()
>> there is a polling loop:
>>
>> do {
>> mdelay(100);
>> nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>> wmb();
>> mdelay(100);
>> nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
>>
>> mdelay(5500);
>>
>> err = readl_poll_timeout_atomic(priv->base + NB8800_RXC_CR,
>> rxcr, !(rxcr & RCR_EN),
>> 1000, 100000);
>> printk("err=%d retry=%d\n", err, retry);
>> } while (err && --retry);
>>
>>
>> (It was me who added the delays.)
>>
>> *Whatever* delays I insert, it always goes 3 times through the loop.
>>
>> [ 29.654492] ++ETH++ gw32 reg=f002610c val=9ecc8000
>> [ 29.759320] ++ETH++ gw32 reg=f0026100 val=005c0aff
>> [ 35.364705] err=-110 retry=5
>> [ 35.467609] ++ETH++ gw32 reg=f002610c val=9ecc8000
>> [ 35.572436] ++ETH++ gw32 reg=f0026100 val=005c0aff
>> [ 41.177822] err=-110 retry=4
>> [ 41.280726] ++ETH++ gw32 reg=f002610c val=9ecc8000
>> [ 41.385553] ++ETH++ gw32 reg=f0026100 val=005c0aff
>> [ 46.890907] err=0 retry=3
>>
>> How is that possible?
>
> First time through the loop, it doesn't matter how long we poll,
> it *always* times out. Second time as well (only on BOARD B).
>
> Third time, it succeeds quickly (first or second poll).
> (This explains why various delays had no impact.)
>
> In fact, requesting the transfer 3 times *before* polling
> makes the polling succeed quickly:
>
> nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
> wmb();
> nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
>
> [ 16.464596] ++ETH++ gw32 reg=f002610c val=9ef28000
> [ 16.469414] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 16.474231] ++ETH++ gw32 reg=f002610c val=9ef28000
> [ 16.479048] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 16.483865] ++ETH++ gw32 reg=f002610c val=9ef28000
> [ 16.488682] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 16.493500] ++ETH++ POLL reg=f0026200 val=06100a8f
> [ 16.499317] ++ETH++ POLL reg=f0026200 val=06100a8e
> [ 16.504134] err=0 retry=5
That strengthens my theory that the hardware has an internal queue of
three descriptors that are pre-loaded from memory. Your hardware people
should be able to confirm this.
> With my changes, I get *exactly* the same logs on BOARD A
> and BOARD B (modulo the descriptors addresses).
>
> Yet BOARD A stays functional, but BOARD B is hosed...
What's the difference between board A and board B?
> Depressing. I've run out of ideas.
Get your hardware people involved. Perhaps they can run some test in a
simulator.
--
Måns Rullgård
^ permalink raw reply
* Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
From: Måns Rullgård @ 2017-07-31 15:18 UTC (permalink / raw)
To: Mason; +Cc: Florian Fainelli, Marc Gonzalez, netdev, Linux ARM
In-Reply-To: <31792688-7720-1d1e-4fd4-a0f96ac8af59@free.fr>
Mason <slash.tmp@free.fr> writes:
> On 31/07/2017 13:59, Måns Rullgård wrote:
>
>> Mason writes:
>>
>>> On 29/07/2017 17:18, Florian Fainelli wrote:
>>>
>>>> On 07/29/2017 05:02 AM, Mason wrote:
>>>>
>>>>> I have identified a 100% reproducible flaw.
>>>>> I have proposed a work-around that brings this down to 0
>>>>> (tested 1000 cycles of link up / ping / link down).
>>>>
>>>> Can you also try to get help from your HW resources to eventually help
>>>> you find out what is going on here?
>>>
>>> The patch I proposed /is/ based on the feedback from the HW team :-(
>>> "Just reset the HW block, and everything will work as expected."
>>
>> Nobody is saying a reset won't recover the lockup. The problem is that
>> we don't know what caused it to lock up in the first place. How do we
>> know it can't happen during normal operation? If we knew the cause, it
>> might also be possible to avoid the situation entirely.
>
> How does one prove that something "can't happen during normal operation"?
One figures out what conditions cause the something and ensures they
never arise.
> The "put adapter in loop-back mode so we can send ourselves fake packets"
> shenanigans seems completely insane, if you ask me.
Blame the hardware designers. The *only* way to stop the rx dma is to
have it receive a packet into a descriptor with the end of chain flag
set. Thankfully the loopback mode means this can be made to happen at
will rather than waiting for actual network traffic.
> Other things make no sense to me, for example in nb8800_dma_stop()
> there is a polling loop:
>
> do {
> mdelay(100);
> nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
> wmb();
> mdelay(100);
> nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
>
> mdelay(5500);
>
> err = readl_poll_timeout_atomic(priv->base + NB8800_RXC_CR,
> rxcr, !(rxcr & RCR_EN),
> 1000, 100000);
> printk("err=%d retry=%d\n", err, retry);
> } while (err && --retry);
>
> (It was me who added the delays.)
>
> *Whatever* delays I insert, it always goes 3 times through the loop.
>
> [ 29.654492] ++ETH++ gw32 reg=f002610c val=9ecc8000
> [ 29.759320] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 35.364705] err=-110 retry=5
> [ 35.467609] ++ETH++ gw32 reg=f002610c val=9ecc8000
> [ 35.572436] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 41.177822] err=-110 retry=4
> [ 41.280726] ++ETH++ gw32 reg=f002610c val=9ecc8000
> [ 41.385553] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 46.890907] err=0 retry=3
>
> How is that possible?
One possibility is that the hardware loads three descriptors in advance
and doesn't see the newly set end of chain flag until its internal queue
has been used up.
> I've tried using spinlocks and delays to get parallel execution
> down to a minimum, and have the same logs on both boards.
>
> Regards.
--
Måns Rullgård
^ permalink raw reply
* Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
From: Mason @ 2017-07-31 15:18 UTC (permalink / raw)
To: Mans Rullgard; +Cc: Florian Fainelli, Marc Gonzalez, netdev, Linux ARM
In-Reply-To: <31792688-7720-1d1e-4fd4-a0f96ac8af59@free.fr>
On 31/07/2017 16:08, Mason wrote:
> Other things make no sense to me, for example in nb8800_dma_stop()
> there is a polling loop:
>
> do {
> mdelay(100);
> nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
> wmb();
> mdelay(100);
> nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
>
> mdelay(5500);
>
> err = readl_poll_timeout_atomic(priv->base + NB8800_RXC_CR,
> rxcr, !(rxcr & RCR_EN),
> 1000, 100000);
> printk("err=%d retry=%d\n", err, retry);
> } while (err && --retry);
>
>
> (It was me who added the delays.)
>
> *Whatever* delays I insert, it always goes 3 times through the loop.
>
> [ 29.654492] ++ETH++ gw32 reg=f002610c val=9ecc8000
> [ 29.759320] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 35.364705] err=-110 retry=5
> [ 35.467609] ++ETH++ gw32 reg=f002610c val=9ecc8000
> [ 35.572436] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 41.177822] err=-110 retry=4
> [ 41.280726] ++ETH++ gw32 reg=f002610c val=9ecc8000
> [ 41.385553] ++ETH++ gw32 reg=f0026100 val=005c0aff
> [ 46.890907] err=0 retry=3
>
> How is that possible?
First time through the loop, it doesn't matter how long we poll,
it *always* times out. Second time as well (only on BOARD B).
Third time, it succeeds quickly (first or second poll).
(This explains why various delays had no impact.)
In fact, requesting the transfer 3 times *before* polling
makes the polling succeed quickly:
nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
wmb();
nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
[ 16.464596] ++ETH++ gw32 reg=f002610c val=9ef28000
[ 16.469414] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 16.474231] ++ETH++ gw32 reg=f002610c val=9ef28000
[ 16.479048] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 16.483865] ++ETH++ gw32 reg=f002610c val=9ef28000
[ 16.488682] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 16.493500] ++ETH++ POLL reg=f0026200 val=06100a8f
[ 16.499317] ++ETH++ POLL reg=f0026200 val=06100a8e
[ 16.504134] err=0 retry=5
With my changes, I get *exactly* the same logs on BOARD A
and BOARD B (modulo the descriptors addresses).
Yet BOARD A stays functional, but BOARD B is hosed...
Depressing. I've run out of ideas.
BOARD A LOGS:
# test_eth.sh
[ 18.037782] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
172.27.64.1 : xmt/rcv/%loss = 1/1/0%, min/avg/max = 0.39/0.39/0.39
[ 20.617917] nb8800_stop from __dev_close_many
[ 20.622314] ++ETH++ gw32 reg=f0026020 val=00920000
[ 20.627135] ++ETH++ gw32 reg=f0026020 val=80920000
[ 20.631973] ++ETH++ gr32 reg=f0026024 val=0000ec00
[ 20.636782] ++ETH++ gw32 reg=f0026020 val=04920000
[ 20.641601] ++ETH++ gw32 reg=f0026020 val=84920000
[ 20.646440] ++ETH++ gw32 reg=f0026020 val=00930000
[ 20.651258] ++ETH++ gw32 reg=f0026020 val=80930000
[ 20.656095] ++ETH++ gr32 reg=f0026024 val=00000000
[ 20.660909] ++ETH++ POLL reg=f0026100 val=005c0afe
[ 20.665743] ++ETH++ gr8 reg=f0026004 val=2b
[ 20.670028] ++ETH++ gw8 reg=f0026004 val=0b
[ 20.674313] ++ETH++ gr8 reg=f0026044 val=81
[ 20.678598] ++ETH++ gw8 reg=f0026044 val=85
[ 20.682883] ++ETH++ gw32 reg=f002610c val=9de0c000
[ 20.687693] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 20.692503] ++ETH++ gw32 reg=f002610c val=9de0c000
[ 20.697312] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 20.702121] ++ETH++ gw32 reg=f002610c val=9de0c000
[ 20.706929] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 20.712738] ++ETH++ POLL reg=f0026200 val=06100a8e
[ 20.717547] err=0 retry=5
[ 20.720172] ++ETH++ gr8 reg=f0026004 val=0b
[ 20.724456] ++ETH++ gw8 reg=f0026004 val=2b
[ 20.728742] ++ETH++ gr8 reg=f0026044 val=85
[ 20.733026] ++ETH++ gw8 reg=f0026044 val=81
[ 20.737349] ++ETH++ gw32 reg=f002620c val=9de04000
[ 20.742158] nb8800_dma_stop=0
[ 21.845224] ENTER nb8800_irq
[ 21.848116] ++ETH++ gr32 reg=f0026104 val=00000005
[ 21.852927] ++ETH++ gw32 reg=f0026104 val=00000005
[ 21.857738] ++ETH++ gr32 reg=f0026204 val=00000004
[ 21.862547] ++ETH++ gw32 reg=f0026204 val=00000004
[ 21.867356] ++ETH++ gw32 reg=f0026218 val=003cc4a4
[ 21.872164] EXIT nb8800_irq
[ 24.373448] ++ETH++ gr8 reg=f0026004 val=2b
[ 24.377755] ++ETH++ gw8 reg=f0026004 val=2a
[ 24.382054] ++ETH++ gr32 reg=f0026100 val=00080afe
[ 24.386876] ++ETH++ gr8 reg=f0026000 val=1d
[ 24.391192] ++ETH++ gw8 reg=f0026000 val=1c
[ 25.706747] ++ETH++ gw32 reg=f0026020 val=00920000
[ 25.711578] ++ETH++ gw32 reg=f0026020 val=80920000
[ 25.716427] ++ETH++ gr32 reg=f0026024 val=00000000
[ 25.721248] ++ETH++ gw32 reg=f0026020 val=04920000
[ 25.726091] ++ETH++ gw32 reg=f0026020 val=84920000
[ 25.730942] ++ETH++ gw32 reg=f0026020 val=00930000
[ 25.735782] ++ETH++ gw32 reg=f0026020 val=80930000
[ 25.740631] ++ETH++ gr32 reg=f0026024 val=00000000
[ 25.745604] nb8800 26000.ethernet eth0: Link is Down
[ 25.750617] ++ETH++ gw32 reg=f0026020 val=00920000
[ 25.755464] ++ETH++ gw32 reg=f0026020 val=80920000
[ 25.760377] ++ETH++ gr32 reg=f0026024 val=00000000
[ 25.765205] ++ETH++ gw32 reg=f0026020 val=00920000
[ 25.770085] ++ETH++ gw32 reg=f0026020 val=80920000
[ 25.774962] ++ETH++ gr32 reg=f0026024 val=00000000
[ 25.779784] ++ETH++ gw32 reg=f0026020 val=00800000
[ 25.784614] ++ETH++ gw32 reg=f0026020 val=80800000
[ 25.789510] ++ETH++ gr32 reg=f0026024 val=00001000
[ 25.794333] ++ETH++ gw32 reg=f0026020 val=04801800
[ 25.799199] ++ETH++ gw32 reg=f0026020 val=84801800
[ 25.804081] ++ETH++ gw32 reg=f0026020 val=00920000
[ 25.808918] ++ETH++ gw32 reg=f0026020 val=80920000
[ 25.813825] ++ETH++ gr32 reg=f0026024 val=00000000
[ 25.818684] ++ETH++ gw32 reg=f0026020 val=00920000
[ 25.823551] ++ETH++ gw32 reg=f0026020 val=80920000
[ 25.828435] ++ETH++ gr32 reg=f0026024 val=00000000
[ 25.833291] ++ETH++ gw32 reg=f0026020 val=00800000
[ 25.838156] ++ETH++ gw32 reg=f0026020 val=80800000
[ 25.843041] ++ETH++ gr32 reg=f0026024 val=00001800
[ 25.847897] ++ETH++ gw32 reg=f0026020 val=04801800
[ 25.852763] ++ETH++ gw32 reg=f0026020 val=84801800
[ 25.857719] EXIT nb8800_stop
[ 30.096432] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
172.27.64.1 : xmt/rcv/%loss = 1/1/0%, min/avg/max = 0.37/0.37/0.37
BOARD B LOGS:
# test_eth.sh
[ 13.796651] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
172.27.64.1 : xmt/rcv/%loss = 1/1/0%, min/avg/max = 0.34/0.34/0.34
[ 16.379613] nb8800_stop from __dev_close_many
[ 16.384016] ++ETH++ gw32 reg=f0026020 val=00920000
[ 16.388845] ++ETH++ gw32 reg=f0026020 val=80920000
[ 16.393691] ++ETH++ gr32 reg=f0026024 val=0000ec00
[ 16.398508] ++ETH++ gw32 reg=f0026020 val=04920000
[ 16.403335] ++ETH++ gw32 reg=f0026020 val=84920000
[ 16.408183] ++ETH++ gw32 reg=f0026020 val=00930000
[ 16.413009] ++ETH++ gw32 reg=f0026020 val=80930000
[ 16.417854] ++ETH++ gr32 reg=f0026024 val=00000000
[ 16.422673] ++ETH++ POLL reg=f0026100 val=005c0afe
[ 16.427514] ++ETH++ gr8 reg=f0026004 val=2b
[ 16.431806] ++ETH++ gw8 reg=f0026004 val=0b
[ 16.436100] ++ETH++ gr8 reg=f0026044 val=81
[ 16.440392] ++ETH++ gw8 reg=f0026044 val=85
[ 16.444684] ++ETH++ gw32 reg=f002610c val=9efa8000
[ 16.449501] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 16.454318] ++ETH++ gw32 reg=f002610c val=9efa8000
[ 16.459134] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 16.463951] ++ETH++ gw32 reg=f002610c val=9efa8000
[ 16.468768] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 16.474586] ++ETH++ POLL reg=f0026200 val=06100a8e
[ 16.479403] err=0 retry=5
[ 16.482034] ++ETH++ gr8 reg=f0026004 val=0b
[ 16.486327] ++ETH++ gw8 reg=f0026004 val=2b
[ 16.490619] ++ETH++ gr8 reg=f0026044 val=85
[ 16.494912] ++ETH++ gw8 reg=f0026044 val=81
[ 16.499217] ++ETH++ gw32 reg=f002620c val=9ed22000
[ 16.504035] nb8800_dma_stop=0
[ 17.607126] ENTER nb8800_irq
[ 17.610032] ++ETH++ gr32 reg=f0026104 val=00000005
[ 17.614851] ++ETH++ gw32 reg=f0026104 val=00000005
[ 17.619669] ++ETH++ gr32 reg=f0026204 val=00000004
[ 17.624486] ++ETH++ gw32 reg=f0026204 val=00000004
[ 17.629303] ++ETH++ gw32 reg=f0026218 val=003cc4a4
[ 17.634120] EXIT nb8800_irq
[ 20.108802] ++ETH++ gr8 reg=f0026004 val=2b
[ 20.113115] ++ETH++ gw8 reg=f0026004 val=2a
[ 20.117478] ++ETH++ gr32 reg=f0026100 val=00080afe
[ 20.122348] ++ETH++ gr8 reg=f0026000 val=1d
[ 20.126688] ++ETH++ gw8 reg=f0026000 val=1c
[ 21.442246] ++ETH++ gw32 reg=f0026020 val=00920000
[ 21.447107] ++ETH++ gw32 reg=f0026020 val=80920000
[ 21.452027] ++ETH++ gr32 reg=f0026024 val=00000000
[ 21.456907] ++ETH++ gw32 reg=f0026020 val=04920000
[ 21.461783] ++ETH++ gw32 reg=f0026020 val=84920000
[ 21.466684] ++ETH++ gw32 reg=f0026020 val=00930000
[ 21.471559] ++ETH++ gw32 reg=f0026020 val=80930000
[ 21.476457] ++ETH++ gr32 reg=f0026024 val=00000000
[ 21.481395] nb8800 26000.ethernet eth0: Link is Down
[ 21.486461] ++ETH++ gw32 reg=f0026020 val=00920000
[ 21.491338] ++ETH++ gw32 reg=f0026020 val=80920000
[ 21.496237] ++ETH++ gr32 reg=f0026024 val=00000000
[ 21.501102] ++ETH++ gw32 reg=f0026020 val=00920000
[ 21.505983] ++ETH++ gw32 reg=f0026020 val=80920000
[ 21.510877] ++ETH++ gr32 reg=f0026024 val=00000000
[ 21.515748] ++ETH++ gw32 reg=f0026020 val=00800000
[ 21.520621] ++ETH++ gw32 reg=f0026020 val=80800000
[ 21.525489] ++ETH++ gr32 reg=f0026024 val=00001000
[ 21.530319] ++ETH++ gw32 reg=f0026020 val=04801800
[ 21.535157] ++ETH++ gw32 reg=f0026020 val=84801800
[ 21.540025] ++ETH++ gw32 reg=f0026020 val=00920000
[ 21.544866] ++ETH++ gw32 reg=f0026020 val=80920000
[ 21.549730] ++ETH++ gr32 reg=f0026024 val=00000000
[ 21.554560] ++ETH++ gw32 reg=f0026020 val=00920000
[ 21.559400] ++ETH++ gw32 reg=f0026020 val=80920000
[ 21.564257] ++ETH++ gr32 reg=f0026024 val=00000000
[ 21.569083] ++ETH++ gw32 reg=f0026020 val=00800000
[ 21.573921] ++ETH++ gw32 reg=f0026020 val=80800000
[ 21.578777] ++ETH++ gr32 reg=f0026024 val=00001800
[ 21.583603] ++ETH++ gw32 reg=f0026020 val=04801800
[ 21.588441] ++ETH++ gw32 reg=f0026020 val=84801800
[ 21.593349] EXIT nb8800_stop
[ 25.811889] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
172.27.64.1 : xmt/rcv/%loss = 1/0/100%
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
From: Vivien Didelot @ 2017-07-31 15:15 UTC (permalink / raw)
To: Egil Hjelmeland, andrew, f.fainelli, netdev, linux-kernel, kernel
In-Reply-To: <6bc09c91-14b7-f28d-4f1f-f243f12b2940@egil-hjelmeland.no>
Hi Egil,
Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
>> It is indeed out of scope. You may want to add a first commit "net: dsa:
>> lan9303: introduce LAN9303_NUM_PORTS" for instance.
>
> In a later series I assume? Or is allowed to add patches to a series
> under review?
If it makes sense to include this first limit patch in this series under
review, you can totally add it. A simple "Changes in v2: - add
LAN9303_NUM_PORTS" list in the cover letter will do the job.
Thanks,
Vivien
^ permalink raw reply
* [PATCH V5 2/2] brcmfmac: don't warn user about NVRAM if fallback to the platform one succeeds
From: Rafał Miłecki @ 2017-07-31 15:09 UTC (permalink / raw)
To: Luis R . Rodriguez, Greg Kroah-Hartman
Cc: Bjorn Andersson, Daniel Wagner, David Woodhouse, Arend van Spriel,
Rafael J . Wysocki, yi1.li, atull, Moritz Fischer, pmladek,
Johannes Berg, emmanuel.grumbach, luciano.coelho, Kalle Valo,
luto, Linus Torvalds, Kees Cook, AKASHI Takahiro, David Howells,
pjones, Hans de Goede, alan, tytso, lkml,
Franky Lin <franky.li
In-Reply-To: <20170731150945.8925-1-zajec5@gmail.com>
From: Rafał Miłecki <rafal@milecki.pl>
Failing to load NVRAM *file* isn't critical if we manage to get platform
NVRAM in the fallback path. It means warnings like:
[ 10.801506] brcmfmac 0000:01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2
are unnecessary & disturbing for people with *platform* NVRAM as they
are not expected to have NVRAM file. This is a very common case for
Broadcom home routers.
Instead of printing warning immediately within the firmware subsystem
let's try our fallback code first. If that fails as well, then it's a
right moment to print an error.
This should reduce amount of false reports from users seeing this
warning while having wireless working perfectly fine with the platform
NVRAM.
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Update commit message as it wasn't clear enough (thanks Andy) & add extra
messages to the firmware.c.
V3: Set FW_OPT_UEVENT to don't change behavior
V4: Switch to the new request_firmware_async syntax
V5: Rebase, update commit message, resend after drvdata discussion
---
.../wireless/broadcom/brcm80211/brcmfmac/firmware.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index d231042f19d6..524442b3870f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
raw_nvram = false;
} else {
data = bcm47xx_nvram_get_contents(&data_len);
- if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
- goto fail;
+ if (!data) {
+ brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
+ if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
+ brcmf_err("Loading NVRAM from %s and using platform one both failed\n",
+ fwctx->nvram_name);
+ goto fail;
+ }
+ }
raw_nvram = true;
}
@@ -491,6 +497,9 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
{
struct brcmf_fw *fwctx = ctx;
+ struct firmware_opts fw_opts = {
+ .optional = true,
+ };
int ret = 0;
brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev));
@@ -503,9 +512,8 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
goto done;
fwctx->code = fw;
- ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
- fwctx->dev, GFP_KERNEL, fwctx,
- brcmf_fw_request_nvram_done);
+ ret = request_firmware_async(fwctx->nvram_name, &fw_opts, fwctx->dev,
+ fwctx, brcmf_fw_request_nvram_done);
/* pass NULL to nvram callback for bcm47xx fallback */
if (ret)
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 3/5] net: stmmac: Add Adaptrum Anarion GMAC glue layer
From: Alex @ 2017-07-31 15:11 UTC (permalink / raw)
To: David Miller
Cc: linux-snps-arc, linux-kernel, robh+dt, mark.rutland,
peppe.cavallaro, alexandre.torgue, netdev, devicetree
In-Reply-To: <20170728.190155.457209566729295513.davem@davemloft.net>
Hi David,
On 07/28/2017 07:01 PM, David Miller wrote:
> From: Alexandru Gagniuc <alex.g@adaptrum.com>
> Date: Fri, 28 Jul 2017 15:07:03 -0700
>
>> Before the GMAC on the Anarion chip can be used, the PHY interface
>> selection must be configured with the DWMAC block in reset.
>>
>> This layer covers a block containing only two registers. Although it
>> is possible to model this as a reset controller and use the "resets"
>> property of stmmac, it's much more intuitive to include this in the
>> glue layer instead.
>>
>> At this time only RGMII is supported, because it is the only mode
>> which has been validated hardware-wise.
>>
>> Signed-off-by: Alexandru Gagniuc <alex.g@adaptrum.com>
>
> I don't see how this fits into any patch series at all. If this is
> part of a series you posted elsewhere, you should keep netdev@ on
> all such postings so people there can review the change in-context.
I used the --cc-cmd option to send-email. I'll be sure to CC netdev@ on
[PATCH v2].
Alex
^ permalink raw reply
* [PATCH V5 1/2] firmware: add more flexible request_firmware_async function
From: Rafał Miłecki @ 2017-07-31 15:09 UTC (permalink / raw)
To: Luis R . Rodriguez, Greg Kroah-Hartman
Cc: Bjorn Andersson, Daniel Wagner, David Woodhouse, Arend van Spriel,
Rafael J . Wysocki, yi1.li, atull, Moritz Fischer, pmladek,
Johannes Berg, emmanuel.grumbach, luciano.coelho, Kalle Valo,
luto, Linus Torvalds, Kees Cook, AKASHI Takahiro, David Howells,
pjones, Hans de Goede, alan, tytso, lkml,
Franky Lin <franky.li
From: Rafał Miłecki <rafal@milecki.pl>
So far we got only one function for loading firmware asynchronously:
request_firmware_nowait. It didn't allow much customization of firmware
loading process - there is only one bool uevent argument. Moreover this
bool also controls user helper in an unclear way.
Some drivers need more flexible helper providing more options. This
includes control over uevent or warning for the missing firmware. Of
course this list may grow up in the future.
To handle this easily this patch adds a generic request_firmware_async
function. It takes struct with options as an argument which will allow
extending it in the future without massive changes.
This is a really cheap change (no new independent API) which works
nicely with the existing code. The old request_firmware_nowait is kept
as a simple helper calling new helper.
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V3: Don't expose all FW_OPT_* flags.
As Luis noted we want a struct so add struct firmware_opts for real
flexibility.
Thank you Luis for your review!
V5: Rebase, update commit message, resend after drvdata discussion
---
drivers/base/firmware_class.c | 43 ++++++++++++++++++++++++++++++++++---------
include/linux/firmware.h | 15 ++++++++++++++-
2 files changed, 48 insertions(+), 10 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index b9f907eedbf7..cde85b05e4cb 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1375,7 +1375,7 @@ static void request_firmware_work_func(struct work_struct *work)
_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
fw_work->opt_flags);
fw_work->cont(fw, fw_work->context);
- put_device(fw_work->device); /* taken in request_firmware_nowait() */
+ put_device(fw_work->device); /* taken in __request_firmware_nowait() */
module_put(fw_work->module);
kfree_const(fw_work->name);
@@ -1383,10 +1383,9 @@ static void request_firmware_work_func(struct work_struct *work)
}
/**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * __request_firmware_nowait - asynchronous version of request_firmware
* @module: module requesting the firmware
- * @uevent: sends uevent to copy the firmware image if this flag
- * is non-zero else the firmware copy must be done manually.
+ * @opt_flags: flags that control firmware loading process, see FW_OPT_*
* @name: name of firmware file
* @device: device for which firmware is being loaded
* @gfp: allocation flags
@@ -1405,9 +1404,9 @@ static void request_firmware_work_func(struct work_struct *work)
*
* - can't sleep at all if @gfp is GFP_ATOMIC.
**/
-int
-request_firmware_nowait(
- struct module *module, bool uevent,
+static int
+__request_firmware_nowait(
+ struct module *module, unsigned int opt_flags,
const char *name, struct device *device, gfp_t gfp, void *context,
void (*cont)(const struct firmware *fw, void *context))
{
@@ -1426,8 +1425,7 @@ request_firmware_nowait(
fw_work->device = device;
fw_work->context = context;
fw_work->cont = cont;
- fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
- (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+ fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
if (!try_module_get(module)) {
kfree_const(fw_work->name);
@@ -1440,8 +1438,35 @@ request_firmware_nowait(
schedule_work(&fw_work->work);
return 0;
}
+
+int request_firmware_nowait(struct module *module, bool uevent,
+ const char *name, struct device *device, gfp_t gfp,
+ void *context,
+ void (*cont)(const struct firmware *fw, void *context))
+{
+ unsigned int opt_flags = FW_OPT_FALLBACK |
+ (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
+
+ return __request_firmware_nowait(module, opt_flags, name, device, gfp,
+ context, cont);
+}
EXPORT_SYMBOL(request_firmware_nowait);
+int __request_firmware_async(struct module *module, const char *name,
+ struct firmware_opts *fw_opts, struct device *dev,
+ void *context,
+ void (*cont)(const struct firmware *fw, void *context))
+{
+ unsigned int opt_flags = FW_OPT_UEVENT;
+
+ if (fw_opts->optional)
+ opt_flags |= FW_OPT_NO_WARN;
+
+ return __request_firmware_nowait(module, opt_flags, name, dev,
+ GFP_KERNEL, context, cont);
+}
+EXPORT_SYMBOL(__request_firmware_async);
+
#ifdef CONFIG_PM_SLEEP
static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0ccb8ac..a32b6e67462d 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -82,6 +82,19 @@ static inline int request_firmware_into_buf(const struct firmware **firmware_p,
{
return -EINVAL;
}
-
#endif
+
+struct firmware_opts {
+ bool optional;
+};
+
+int __request_firmware_async(struct module *module, const char *name,
+ struct firmware_opts *fw_opts, struct device *dev,
+ void *context,
+ void (*cont)(const struct firmware *fw, void *context));
+
+#define request_firmware_async(name, fw_opts, dev, context, cont) \
+ __request_firmware_async(THIS_MODULE, name, fw_opts, dev, \
+ context, cont)
+
#endif
--
2.11.0
^ permalink raw reply related
* Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
From: Egil Hjelmeland @ 2017-07-31 15:08 UTC (permalink / raw)
To: Vivien Didelot, andrew, f.fainelli, netdev, linux-kernel, kernel
In-Reply-To: <87shhc6471.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
On 31. juli 2017 17:01, Vivien Didelot wrote:
> Hi Egil,
>
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
>
>> Would doing
>>
>> - chip->ds = dsa_switch_alloc(chip->dev, DSA_MAX_PORTS);
>> + chip->ds = dsa_switch_alloc(chip->dev, LAN9303_NUM_PORTS);
>>
>> at the same time be good, or breaking the scope of the patch?
>
> It is indeed out of scope. You may want to add a first commit "net: dsa:
> lan9303: introduce LAN9303_NUM_PORTS" for instance.
>
In a later series I assume? Or is allowed to add patches to a series
under review?
>
> Thanks,
>
> Vivien
>
Egil
^ permalink raw reply
* RE: [PATCH V5 net-next 7/8] net: hns3: Add Ethtool support to HNS3 driver
From: Salil Mehta @ 2017-07-31 15:05 UTC (permalink / raw)
To: Andrew Lunn
Cc: davem@davemloft.net, Zhuangyuzeng (Yisen), huangdaode, lipeng (Y),
mehta.salil.lnk@gmail.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
Linuxarm
In-Reply-To: <20170728234922.GC16080@lunn.ch>
Hi Andrew,
> -----Original Message-----
> From: Salil Mehta
> Sent: Saturday, July 29, 2017 12:57 AM
> To: 'Andrew Lunn'
> Cc: davem@davemloft.net; Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil.lnk@gmail.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-rdma@vger.kernel.org; Linuxarm
> Subject: RE: [PATCH V5 net-next 7/8] net: hns3: Add Ethtool support to
> HNS3 driver
>
> Hi Andrew,
>
> > -----Original Message-----
> > From: Andrew Lunn [mailto:andrew@lunn.ch]
> > Sent: Saturday, July 29, 2017 12:49 AM
> > To: Salil Mehta
> > Cc: davem@davemloft.net; Zhuangyuzeng (Yisen); huangdaode; lipeng
> (Y);
> > mehta.salil.lnk@gmail.com; netdev@vger.kernel.org; linux-
> > kernel@vger.kernel.org; linux-rdma@vger.kernel.org; Linuxarm
> > Subject: Re: [PATCH V5 net-next 7/8] net: hns3: Add Ethtool support
> to
> > HNS3 driver
> >
> > On Fri, Jul 28, 2017 at 11:26:51PM +0100, Salil Mehta wrote:
> > > +static const struct hns3_link_mode_mapping hns3_lm_map[] = {
> > > + {HNS3_LM_FIBRE_BIT, ETHTOOL_LINK_MODE_FIBRE_BIT, FLG},
> > > + {HNS3_LM_AUTONEG_BIT, ETHTOOL_LINK_MODE_Autoneg_BIT, FLG},
> > > + {HNS3_LM_TP_BIT, ETHTOOL_LINK_MODE_TP_BIT, FLG},
> > > + {HNS3_LM_PAUSE_BIT, ETHTOOL_LINK_MODE_Pause_BIT, FLG},
> > > + {HNS3_LM_BACKPLANE_BIT, ETHTOOL_LINK_MODE_Backplane_BIT, FLG},
> > > + {HNS3_LM_10BASET_HALF_BIT, ETHTOOL_LINK_MODE_10baseT_Half_BIT,
> > FLG},
> > > + {HNS3_LM_10BASET_FULL_BIT, ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> > FLG},
> > > + {HNS3_LM_100BASET_HALF_BIT, ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> > FLG},
> > > + {HNS3_LM_100BASET_FULL_BIT, ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> > FLG},
> > > + {HNS3_LM_1000BASET_FULL_BIT,
> > ETHTOOL_LINK_MODE_1000baseT_Full_BIT, FLG},
> > > +};
> >
> > It seems like all entries have link_mode_ksettings set to FLG. Which
> > probably indicates it is pointless.
> It is to support SUPPORTED and ADVERTISED link modes.
I can see what you are saying now. Will remove FLG in next patch.
Thanks
Salil
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
From: Vivien Didelot @ 2017-07-31 15:01 UTC (permalink / raw)
To: Egil Hjelmeland, andrew, f.fainelli, netdev, linux-kernel, kernel
In-Reply-To: <3eafb133-2b39-a1c5-6c01-917799fb6320@egil-hjelmeland.no>
Hi Egil,
Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> Would doing
>
> - chip->ds = dsa_switch_alloc(chip->dev, DSA_MAX_PORTS);
> + chip->ds = dsa_switch_alloc(chip->dev, LAN9303_NUM_PORTS);
>
> at the same time be good, or breaking the scope of the patch?
It is indeed out of scope. You may want to add a first commit "net: dsa:
lan9303: introduce LAN9303_NUM_PORTS" for instance.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
From: Egil Hjelmeland @ 2017-07-31 14:58 UTC (permalink / raw)
To: Vivien Didelot, andrew, f.fainelli, netdev, linux-kernel, kernel
In-Reply-To: <87o9s01xb3.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
On 31. juli 2017 16:43, Vivien Didelot wrote:
> Hi Egil,
>
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
>
>>>> + for (p = 0; p <= 2; p++) {
>>>
>>> Exclusive limits are often prefer, i.e. 'p < 3'.
>>>
>> OK, that can be nice when I later introduce LAN9303_NUM_PORTS = 3.
>
> This is indeed another reason what exclusive limits are prefered ;-)
>
>>>> + int ret;
>>>> +
>>>> + ret = lan9303_disable_packet_processing(chip, p);
>>>> + if (ret)
>>>> + return ret;
>>>
>>> When any non-zero return code means an error, we usually see 'err'
>>> instead of 'ret'.
>>>
>>
>> But 'ret' is used throughout the rest of the file. Is it not better to
>> be locally consistent?
>
> You are correct, I was missing a bit of context here.
>
>>>> case 1:
>>>> - return lan9303_enable_packet_processing(chip, port);
>>>
>>> Is this deletion intentional? The commit message does not explain this.
>>>
>>> When possible, it is appreciated to separate functional from
>>> non-functional changes. For example one commit adding the loop in
>>> lan9303_disable_processing and another one to not enable/disable packet
>>> processing on port 1.
>>>
>>
>> Case fall through, the change is purely non-functional.
>>
>> You are perhaps thinking of the patch in my first series where I removed
>> disable of port 0. I have put that on hold. Juergen says that the
>> mainline driver works out of the box for him. So I will investigate
>> that problem bit more.
>
> Correct! I misread, my bad. This is indeed cleaner with this patch. With
> the LAN9303_NUM_PORTS limit and detailed commit message, the patch LGTM.
>
>
> Thanks,
>
> Vivien
>
Would doing
- chip->ds = dsa_switch_alloc(chip->dev, DSA_MAX_PORTS);
+ chip->ds = dsa_switch_alloc(chip->dev, LAN9303_NUM_PORTS);
at the same time be good, or breaking the scope of the patch?
Egil
^ permalink raw reply
* [PATCH net] udp6: fix jumbogram reception
From: Paolo Abeni @ 2017-07-31 14:52 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Lorenzo Colitti, David Ahern, David Lebrun,
Hannes Frederic Sowa
Since commit 67a51780aebb ("ipv6: udp: leverage scratch area
helpers") udp6_recvmsg() read the skb len from the scratch area,
to avoid a cache miss.
But the UDP6 rx path support RFC 2675 UDPv6 jumbograms, and their
length exceeds the 16 bits available in the scratch area. As a side
effect the length returned by recvmsg() is:
<ingress datagram len> % (1<<16)
This commit addresses the issue allocating one more bit in the
IP6CB flags field and setting it for incoming jumbograms.
Such field is still in the first cacheline, so at recvmsg()
time we can check it and fallback to access skb->len if
required, without a measurable overhead.
Fixes: 67a51780aebb ("ipv6: udp: leverage scratch area helpers")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
This is possibly a theoretical only issue: I've been unable to
find an in kernel driver suitable to trigger the issue -
e.g. supporting a max mtu larger than 64K, and I have to resort
using a patched veth driver allowing very large mtu.
Anyway I think this is worth fixing, since pre 67a51780aebb the
IPv6/UDP rx path was able to cope correctly with jumbograms.
---
include/linux/ipv6.h | 6 ++++++
net/ipv6/exthdrs.c | 1 +
net/ipv6/udp.c | 11 ++++++++++-
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index e1b442996f81..474d6bbc158c 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -128,6 +128,7 @@ struct inet6_skb_parm {
#define IP6SKB_FRAGMENTED 16
#define IP6SKB_HOPBYHOP 32
#define IP6SKB_L3SLAVE 64
+#define IP6SKB_JUMBOGRAM 128
};
#if defined(CONFIG_NET_L3_MASTER_DEV)
@@ -152,6 +153,11 @@ static inline int inet6_iif(const struct sk_buff *skb)
return l3_slave ? skb->skb_iif : IP6CB(skb)->iif;
}
+static inline bool inet6_is_jumbogram(const struct sk_buff *skb)
+{
+ return !!(IP6CB(skb)->flags & IP6SKB_JUMBOGRAM);
+}
+
/* can not be used in TCP layer after tcp_v6_fill_cb */
static inline bool inet6_exact_dif_match(struct net *net, struct sk_buff *skb)
{
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 4996d734f1d2..3cec529c6113 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -756,6 +756,7 @@ static bool ipv6_hop_jumbo(struct sk_buff *skb, int optoff)
if (pskb_trim_rcsum(skb, pkt_len + sizeof(struct ipv6hdr)))
goto drop;
+ IP6CB(skb)->flags |= IP6SKB_JUMBOGRAM;
return true;
drop:
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 98fe4560e24c..578142b7ca3e 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -328,6 +328,15 @@ struct sock *udp6_lib_lookup(struct net *net, const struct in6_addr *saddr, __be
EXPORT_SYMBOL_GPL(udp6_lib_lookup);
#endif
+/* do not use the scratch area len for jumbogram: their length execeeds the
+ * scratch area space; note that the IP6CB flags is still in the first
+ * cacheline, so checking for jumbograms is cheap
+ */
+static int udp6_skb_len(struct sk_buff *skb)
+{
+ return unlikely(inet6_is_jumbogram(skb)) ? skb->len : udp_skb_len(skb);
+}
+
/*
* This should be easy, if there is something there we
* return it, otherwise we block.
@@ -358,7 +367,7 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
if (!skb)
return err;
- ulen = udp_skb_len(skb);
+ ulen = udp6_skb_len(skb);
copied = len;
if (copied > ulen - off)
copied = ulen - off;
--
2.13.3
^ permalink raw reply related
* Re: [net-next PATCH 11/12] net: add notifier hooks for devmap bpf map
From: John Fastabend @ 2017-07-31 14:47 UTC (permalink / raw)
To: Daniel Borkmann, Levin, Alexander (Sasha Levin)
Cc: davem@davemloft.net, ast@fb.com, netdev@vger.kernel.org,
brouer@redhat.com, andy@greyhouse.net
In-Reply-To: <597EF067.1090605@iogearbox.net>
On 07/31/2017 01:55 AM, Daniel Borkmann wrote:
> On 07/30/2017 03:28 PM, Levin, Alexander (Sasha Levin) wrote:
>> On Mon, Jul 17, 2017 at 09:30:02AM -0700, John Fastabend wrote:
>>> @@ -341,9 +368,11 @@ static int dev_map_update_elem(struct bpf_map *map, void *key, void *value,
>>> * Remembering the driver side flush operation will happen before the
>>> * net device is removed.
>>> */
>>> + mutex_lock(&dev_map_list_mutex);
>>> old_dev = xchg(&dtab->netdev_map[i], dev);
>>> if (old_dev)
>>> call_rcu(&old_dev->rcu, __dev_map_entry_free);
>>> + mutex_unlock(&dev_map_list_mutex);
>>>
>>> return 0;
>>> }
>>
>> This function gets called under rcu critical section, where we can't grab mutexes:
>
> Agree, same goes for the delete callback that mutex is not allowed
> in this context. If I recall, this was for the devmap netdev notifier
> in order to check whether we need to purge dev entries from the map,
> so that the device can be unregistered gracefully. Given that devmap
> ops like update/delete are only allowed from user space, we could
> look into whether this map type actually needs to hold RCU at all
> here, or other option is to try and get rid of the mutex altogether.
> John, could you take a look for a fix?
>
> Thanks a lot,
> Daniel
>
I'll work up a fix today/tomorrow. Thanks.
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
From: Andrew Lunn @ 2017-07-31 14:47 UTC (permalink / raw)
To: Egil Hjelmeland; +Cc: vivien.didelot, f.fainelli, netdev, linux-kernel, kernel
In-Reply-To: <83b3fb27-97dd-a9ea-e0db-017d616f93fe@egil-hjelmeland.no>
> >Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> >
> > Andrew
> >
>
> Hi Andrew
>
> This time I took the extra effort to split my original patch...
>
> Your lan9303_write_switch_port suggestion (in previous reply) is fine.
> And I can improve the coverletter.
>
> So I will do a v2 of the patch. But what is your advice:
> Should I squash the patch?
I already gave my Reviewed-by:, meaning i don't really care. Merged or
squashed is a minor point in this case.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
From: Vivien Didelot @ 2017-07-31 14:43 UTC (permalink / raw)
To: Egil Hjelmeland, andrew, f.fainelli, netdev, linux-kernel, kernel
In-Reply-To: <3207fcff-8318-9b17-c546-026ab1a1511b@egil-hjelmeland.no>
Hi Egil,
Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
>>> + for (p = 0; p <= 2; p++) {
>>
>> Exclusive limits are often prefer, i.e. 'p < 3'.
>>
> OK, that can be nice when I later introduce LAN9303_NUM_PORTS = 3.
This is indeed another reason what exclusive limits are prefered ;-)
>>> + int ret;
>>> +
>>> + ret = lan9303_disable_packet_processing(chip, p);
>>> + if (ret)
>>> + return ret;
>>
>> When any non-zero return code means an error, we usually see 'err'
>> instead of 'ret'.
>>
>
> But 'ret' is used throughout the rest of the file. Is it not better to
> be locally consistent?
You are correct, I was missing a bit of context here.
>>> case 1:
>>> - return lan9303_enable_packet_processing(chip, port);
>>
>> Is this deletion intentional? The commit message does not explain this.
>>
>> When possible, it is appreciated to separate functional from
>> non-functional changes. For example one commit adding the loop in
>> lan9303_disable_processing and another one to not enable/disable packet
>> processing on port 1.
>>
>
> Case fall through, the change is purely non-functional.
>
> You are perhaps thinking of the patch in my first series where I removed
> disable of port 0. I have put that on hold. Juergen says that the
> mainline driver works out of the box for him. So I will investigate
> that problem bit more.
Correct! I misread, my bad. This is indeed cleaner with this patch. With
the LAN9303_NUM_PORTS limit and detailed commit message, the patch LGTM.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
From: Egil Hjelmeland @ 2017-07-31 14:32 UTC (permalink / raw)
To: Vivien Didelot, andrew, f.fainelli, netdev, linux-kernel, kernel
In-Reply-To: <87379c4sfe.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me>
On 31. juli 2017 16:00, Vivien Didelot wrote:
> Hi Egil,
>
> A few nitpicks below for lan9303_disable_processing.
>
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
>
>> static int lan9303_disable_processing(struct lan9303 *chip)
>> {
>> - int ret;
>> + int p;
>>
>> - ret = lan9303_disable_packet_processing(chip, 0);
>> - if (ret)
>> - return ret;
>> - ret = lan9303_disable_packet_processing(chip, 1);
>> - if (ret)
>> - return ret;
>> - return lan9303_disable_packet_processing(chip, 2);
>> + for (p = 0; p <= 2; p++) {
>
> Exclusive limits are often prefer, i.e. 'p < 3'.
>
OK, that can be nice when I later introduce LAN9303_NUM_PORTS = 3.
>> + int ret;
>> +
>> + ret = lan9303_disable_packet_processing(chip, p);
>> + if (ret)
>> + return ret;
>
> When any non-zero return code means an error, we usually see 'err'
> instead of 'ret'.
>
But 'ret' is used throughout the rest of the file. Is it not better to
be locally consistent?
>> + }
>
> blank line before return statments are appreciated.
>
OK
>> + return 0;
>> }
>>
>> static int lan9303_check_device(struct lan9303 *chip)
>> @@ -760,7 +761,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
>> /* enable internal packet processing */
>> switch (port) {
>> case 1:
>> - return lan9303_enable_packet_processing(chip, port);
>
> Is this deletion intentional? The commit message does not explain this.
>
> When possible, it is appreciated to separate functional from
> non-functional changes. For example one commit adding the loop in
> lan9303_disable_processing and another one to not enable/disable packet
> processing on port 1.
>
Case fall through, the change is purely non-functional.
You are perhaps thinking of the patch in my first series where I removed
disable of port 0. I have put that on hold. Juergen says that the
mainline driver works out of the box for him. So I will investigate
that problem bit more.
>> case 2:
>> return lan9303_enable_packet_processing(chip, port);
>> default:
>> @@ -779,13 +779,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>> /* disable internal packet processing */
>> switch (port) {
>> case 1:
>> - lan9303_disable_packet_processing(chip, port);
>> - lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
>> - MII_BMCR, BMCR_PDOWN);
>> - break;
>> case 2:
>> lan9303_disable_packet_processing(chip, port);
>> - lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
>> + lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
>> MII_BMCR, BMCR_PDOWN);
>> break;
>
> Thanks,
>
> Vivien
>
Egil
^ permalink raw reply
* Re: Performance regression with virtio_net
From: Seth Forshee @ 2017-07-31 14:29 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Euan Kemp, netdev
In-Reply-To: <20170731161903-mutt-send-email-mst@kernel.org>
On Mon, Jul 31, 2017 at 04:26:08PM +0300, Michael S. Tsirkin wrote:
> On Sun, Jul 30, 2017 at 03:25:52PM -0700, Euan Kemp wrote:
> > I've also observed this performance regression.
> >
> > The minimal fix for me is removing the two
> > > if (unlikely(len > (unsigned long)ctx))
> > checks added in 680557c.
> >
> > After digging a little more, the reason that check can fail appears to
> > be that add_recvbuf_mergeable sometimes includes a hole at the end,
> > which is included in len but not ctx.
> >
> > I'd send a patch removing those conditions, but I'm not certain
> > whether "truesize" in receive_mergeable should also be changed back to
> > be the max of len/ctx, or should remain as-is.
> >
> > - Euan
>
> Thanks a lot for looking into it!
>
> I kept this around unchanged from
> ab7db91705e95ed1bba1304388936fccfa58c992. That commit had an internal
> reason not to account for that space: not enough bits to do it. No
> longer true so let's account for length exactly. I'll send a proper
> patch after a bit of testing, would appreciate reports reports of
> whether this helps very much.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
This fixes the issue for me, downloads are faster and rx_length_errors
does not show any errors.
Tested-by: Seth Forshee <seth.forshee@canonical.com>
Thanks!
Seth
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
From: Egil Hjelmeland @ 2017-07-31 14:09 UTC (permalink / raw)
To: Andrew Lunn; +Cc: vivien.didelot, f.fainelli, netdev, linux-kernel, kernel
In-Reply-To: <20170731134640.GD24562@lunn.ch>
On 31. juli 2017 15:46, Andrew Lunn wrote:
> On Mon, Jul 31, 2017 at 01:33:55PM +0200, Egil Hjelmeland wrote:
>> Simplify usage of lan9303_enable_packet_processing,
>> lan9303_disable_packet_processing()
>>
>> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
>> ---
>> drivers/net/dsa/lan9303-core.c | 24 ++++++++++--------------
>> 1 file changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
>> index 4d2bb8144c15..705267a1d2ba 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -559,15 +559,16 @@ static int lan9303_handle_reset(struct lan9303 *chip)
>> /* stop processing packets for all ports */
>> static int lan9303_disable_processing(struct lan9303 *chip)
>> {
>> - int ret;
>> + int p;
>>
>> - ret = lan9303_disable_packet_processing(chip, 0);
>> - if (ret)
>> - return ret;
>> - ret = lan9303_disable_packet_processing(chip, 1);
>> - if (ret)
>> - return ret;
>> - return lan9303_disable_packet_processing(chip, 2);
>> + for (p = 0; p <= 2; p++) {
>> + int ret;
>> +
>> + ret = lan9303_disable_packet_processing(chip, p);
>> + if (ret)
>> + return ret;
>> + }
>> + return 0;
>> }
>>
>> static int lan9303_check_device(struct lan9303 *chip)
>> @@ -760,7 +761,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
>> /* enable internal packet processing */
>> switch (port) {
>> case 1:
>> - return lan9303_enable_packet_processing(chip, port);
>> case 2:
>> return lan9303_enable_packet_processing(chip, port);
>> default:
>> @@ -779,13 +779,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>> /* disable internal packet processing */
>> switch (port) {
>> case 1:
>> - lan9303_disable_packet_processing(chip, port);
>> - lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
>> - MII_BMCR, BMCR_PDOWN);
>> - break;
>> case 2:
>> lan9303_disable_packet_processing(chip, port);
>> - lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
>> + lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
>> MII_BMCR, BMCR_PDOWN);
>> break;
>> default:
>
> :-)
>
> So maybe i would squash this part into the previous patch. You were
> touching the functions anyway, and the change is obvious, so easy to
> review.
>
> But it is also O.K. this way. The cover note could of helped. You
> could of said something like: "Changes made in the first patch allow
> some simplifications to be made in the same code in the second patch.
>
> Breaking changes up is hard, and you cannot please everybody, all the
> time.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
> Andrew
>
Hi Andrew
This time I took the extra effort to split my original patch...
Your lan9303_write_switch_port suggestion (in previous reply) is fine.
And I can improve the coverletter.
So I will do a v2 of the patch. But what is your advice:
Should I squash the patch?
Egil
^ permalink raw reply
* Re: [RFC PATCH v1] net: ethernet: nb8800: Reset HW block in ndo_open
From: Mason @ 2017-07-31 14:08 UTC (permalink / raw)
To: Mans Rullgard; +Cc: Florian Fainelli, Marc Gonzalez, netdev, Linux ARM
In-Reply-To: <yw1x60e8kebc.fsf@mansr.com>
On 31/07/2017 13:59, Måns Rullgård wrote:
> Mason writes:
>
>> On 29/07/2017 17:18, Florian Fainelli wrote:
>>
>>> On 07/29/2017 05:02 AM, Mason wrote:
>>>
>>>> I have identified a 100% reproducible flaw.
>>>> I have proposed a work-around that brings this down to 0
>>>> (tested 1000 cycles of link up / ping / link down).
>>>
>>> Can you also try to get help from your HW resources to eventually help
>>> you find out what is going on here?
>>
>> The patch I proposed /is/ based on the feedback from the HW team :-(
>> "Just reset the HW block, and everything will work as expected."
>
> Nobody is saying a reset won't recover the lockup. The problem is that
> we don't know what caused it to lock up in the first place. How do we
> know it can't happen during normal operation? If we knew the cause, it
> might also be possible to avoid the situation entirely.
How does one prove that something "can't happen during normal operation"?
The "put adapter in loop-back mode so we can send ourselves fake packets"
shenanigans seems completely insane, if you ask me.
Other things make no sense to me, for example in nb8800_dma_stop()
there is a polling loop:
do {
mdelay(100);
nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
wmb();
mdelay(100);
nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
mdelay(5500);
err = readl_poll_timeout_atomic(priv->base + NB8800_RXC_CR,
rxcr, !(rxcr & RCR_EN),
1000, 100000);
printk("err=%d retry=%d\n", err, retry);
} while (err && --retry);
(It was me who added the delays.)
*Whatever* delays I insert, it always goes 3 times through the loop.
[ 29.654492] ++ETH++ gw32 reg=f002610c val=9ecc8000
[ 29.759320] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 35.364705] err=-110 retry=5
[ 35.467609] ++ETH++ gw32 reg=f002610c val=9ecc8000
[ 35.572436] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 41.177822] err=-110 retry=4
[ 41.280726] ++ETH++ gw32 reg=f002610c val=9ecc8000
[ 41.385553] ++ETH++ gw32 reg=f0026100 val=005c0aff
[ 46.890907] err=0 retry=3
How is that possible?
I've tried using spinlocks and delays to get parallel execution
down to a minimum, and have the same logs on both boards.
Regards.
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
From: Vivien Didelot @ 2017-07-31 14:00 UTC (permalink / raw)
To: Egil Hjelmeland, andrew, f.fainelli, netdev, linux-kernel, kernel
Cc: Egil Hjelmeland
In-Reply-To: <20170731113355.4284-3-privat@egil-hjelmeland.no>
Hi Egil,
A few nitpicks below for lan9303_disable_processing.
Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> static int lan9303_disable_processing(struct lan9303 *chip)
> {
> - int ret;
> + int p;
>
> - ret = lan9303_disable_packet_processing(chip, 0);
> - if (ret)
> - return ret;
> - ret = lan9303_disable_packet_processing(chip, 1);
> - if (ret)
> - return ret;
> - return lan9303_disable_packet_processing(chip, 2);
> + for (p = 0; p <= 2; p++) {
Exclusive limits are often prefer, i.e. 'p < 3'.
> + int ret;
> +
> + ret = lan9303_disable_packet_processing(chip, p);
> + if (ret)
> + return ret;
When any non-zero return code means an error, we usually see 'err'
instead of 'ret'.
> + }
blank line before return statments are appreciated.
> + return 0;
> }
>
> static int lan9303_check_device(struct lan9303 *chip)
> @@ -760,7 +761,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
> /* enable internal packet processing */
> switch (port) {
> case 1:
> - return lan9303_enable_packet_processing(chip, port);
Is this deletion intentional? The commit message does not explain this.
When possible, it is appreciated to separate functional from
non-functional changes. For example one commit adding the loop in
lan9303_disable_processing and another one to not enable/disable packet
processing on port 1.
> case 2:
> return lan9303_enable_packet_processing(chip, port);
> default:
> @@ -779,13 +779,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
> /* disable internal packet processing */
> switch (port) {
> case 1:
> - lan9303_disable_packet_processing(chip, port);
> - lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
> - MII_BMCR, BMCR_PDOWN);
> - break;
> case 2:
> lan9303_disable_packet_processing(chip, port);
> - lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
> + lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
> MII_BMCR, BMCR_PDOWN);
> break;
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
From: Andrew Lunn @ 2017-07-31 13:46 UTC (permalink / raw)
To: Egil Hjelmeland; +Cc: vivien.didelot, f.fainelli, netdev, linux-kernel, kernel
In-Reply-To: <20170731113355.4284-3-privat@egil-hjelmeland.no>
On Mon, Jul 31, 2017 at 01:33:55PM +0200, Egil Hjelmeland wrote:
> Simplify usage of lan9303_enable_packet_processing,
> lan9303_disable_packet_processing()
>
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
> drivers/net/dsa/lan9303-core.c | 24 ++++++++++--------------
> 1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 4d2bb8144c15..705267a1d2ba 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -559,15 +559,16 @@ static int lan9303_handle_reset(struct lan9303 *chip)
> /* stop processing packets for all ports */
> static int lan9303_disable_processing(struct lan9303 *chip)
> {
> - int ret;
> + int p;
>
> - ret = lan9303_disable_packet_processing(chip, 0);
> - if (ret)
> - return ret;
> - ret = lan9303_disable_packet_processing(chip, 1);
> - if (ret)
> - return ret;
> - return lan9303_disable_packet_processing(chip, 2);
> + for (p = 0; p <= 2; p++) {
> + int ret;
> +
> + ret = lan9303_disable_packet_processing(chip, p);
> + if (ret)
> + return ret;
> + }
> + return 0;
> }
>
> static int lan9303_check_device(struct lan9303 *chip)
> @@ -760,7 +761,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
> /* enable internal packet processing */
> switch (port) {
> case 1:
> - return lan9303_enable_packet_processing(chip, port);
> case 2:
> return lan9303_enable_packet_processing(chip, port);
> default:
> @@ -779,13 +779,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
> /* disable internal packet processing */
> switch (port) {
> case 1:
> - lan9303_disable_packet_processing(chip, port);
> - lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
> - MII_BMCR, BMCR_PDOWN);
> - break;
> case 2:
> lan9303_disable_packet_processing(chip, port);
> - lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
> + lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
> MII_BMCR, BMCR_PDOWN);
> break;
> default:
:-)
So maybe i would squash this part into the previous patch. You were
touching the functions anyway, and the change is obvious, so easy to
review.
But it is also O.K. this way. The cover note could of helped. You
could of said something like: "Changes made in the first patch allow
some simplifications to be made in the same code in the second patch.
Breaking changes up is hard, and you cannot please everybody, all the
time.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* RE: [RFC PATCH v1 5/5] wave: Added basic version of TCP Wave
From: David Laight @ 2017-07-31 13:39 UTC (permalink / raw)
To: 'Natale Patriciello', David S . Miller, Alexey Kuznetsov,
James Morris, Hideaki YOSHIFUJI, Patrick McHardy
Cc: netdev, Ahmed Said, Francesco Zampognaro, Cesare Roseti
In-Reply-To: <20170728195919.10099-6-natale.patriciello@gmail.com>
From: Natale Patriciello
> Sent: 28 July 2017 20:59
..
> +static __always_inline bool test_flag(u8 value, const u8 *flags)
> +{
> + return (*flags & value) == value;
> +}
...
> + if (!test_flag(FLAG_INIT, &ca->flags))
> + return;
...
That is a completely unnecessary 'helper'.
It has its arguments in the wrong order.
Doesn't need to pass by reference.
Since you only ever check one bit you don't need the '=='.
Any error seems to be silently ignored.
I bet they can't actually happen at all.
David
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: dsa: lan9303: Refactor lan9303_xxx_packet_processing()
From: Andrew Lunn @ 2017-07-31 13:36 UTC (permalink / raw)
To: Egil Hjelmeland; +Cc: vivien.didelot, f.fainelli, netdev, linux-kernel, kernel
In-Reply-To: <20170731113355.4284-2-privat@egil-hjelmeland.no>
On Mon, Jul 31, 2017 at 01:33:54PM +0200, Egil Hjelmeland wrote:
> lan9303_enable_packet_processing, lan9303_disable_packet_processing()
> Pass port number (0,1,2) as parameter instead of port offset.
>
> Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG().
>
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
> drivers/net/dsa/lan9303-core.c | 59 +++++++++++++++++++++---------------------
> 1 file changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 8e430d1ee297..4d2bb8144c15 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -159,9 +159,7 @@
> # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT1 (BIT(9) | BIT(8))
> # define LAN9303_BM_EGRSS_PORT_TYPE_SPECIAL_TAG_PORT0 (BIT(1) | BIT(0))
>
> -#define LAN9303_PORT_0_OFFSET 0x400
> -#define LAN9303_PORT_1_OFFSET 0x800
> -#define LAN9303_PORT_2_OFFSET 0xc00
> +#define LAN9303_SWITCH_PORT_REG(port, reg0) (0x400 * (port) + (reg0))
>
> /* the built-in PHYs are of type LAN911X */
> #define MII_LAN911X_SPECIAL_MODES 0x12
> @@ -458,24 +456,25 @@ static int lan9303_detect_phy_setup(struct lan9303 *chip)
> return 0;
> }
>
> -#define LAN9303_MAC_RX_CFG_OFFS (LAN9303_MAC_RX_CFG_0 - LAN9303_PORT_0_OFFSET)
> -#define LAN9303_MAC_TX_CFG_OFFS (LAN9303_MAC_TX_CFG_0 - LAN9303_PORT_0_OFFSET)
> -
> static int lan9303_disable_packet_processing(struct lan9303 *chip,
> unsigned int port)
> {
> int ret;
>
> /* disable RX, but keep register reset default values else */
> - ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
> - LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
> + ret = lan9303_write_switch_reg(
> + chip,
> + LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_RX_CFG_0),
> + LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES);
Hi Egil
As you can see from the indentation change, this is rather long. What
we have in mv88e6xxx are functions which act on a port. You could have
a function:
lan9303_write_switch_port(struct lan9303 chip, unsigned int port, u16 reg,
u32 val)
> if (ret)
> return ret;
>
> /* disable TX, but keep register reset default values else */
> - return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
> - LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
> - LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
> + return lan9303_write_switch_reg(
> + chip,
> + LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_TX_CFG_0),
> + LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
> + LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE);
> }
>
> static int lan9303_enable_packet_processing(struct lan9303 *chip,
> @@ -484,17 +483,21 @@ static int lan9303_enable_packet_processing(struct lan9303 *chip,
> int ret;
>
> /* enable RX and keep register reset default values else */
> - ret = lan9303_write_switch_reg(chip, LAN9303_MAC_RX_CFG_OFFS + port,
> - LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
> - LAN9303_MAC_RX_CFG_X_RX_ENABLE);
> + ret = lan9303_write_switch_reg(
> + chip,
> + LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_RX_CFG_0),
> + LAN9303_MAC_RX_CFG_X_REJECT_MAC_TYPES |
> + LAN9303_MAC_RX_CFG_X_RX_ENABLE);
> if (ret)
> return ret;
>
> /* enable TX and keep register reset default values else */
> - return lan9303_write_switch_reg(chip, LAN9303_MAC_TX_CFG_OFFS + port,
> - LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
> - LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
> - LAN9303_MAC_TX_CFG_X_TX_ENABLE);
> + return lan9303_write_switch_reg(
> + chip,
> + LAN9303_SWITCH_PORT_REG(port, LAN9303_MAC_TX_CFG_0),
> + LAN9303_MAC_TX_CFG_X_TX_IFG_CONFIG_DEFAULT |
> + LAN9303_MAC_TX_CFG_X_TX_PAD_ENABLE |
> + LAN9303_MAC_TX_CFG_X_TX_ENABLE);
> }
>
> /* We want a special working switch:
> @@ -558,13 +561,13 @@ static int lan9303_disable_processing(struct lan9303 *chip)
> {
> int ret;
>
> - ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
> + ret = lan9303_disable_packet_processing(chip, 0);
> if (ret)
> return ret;
> - ret = lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
> + ret = lan9303_disable_packet_processing(chip, 1);
> if (ret)
> return ret;
> - return lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
> + return lan9303_disable_packet_processing(chip, 2);
> }
>
> static int lan9303_check_device(struct lan9303 *chip)
> @@ -634,7 +637,7 @@ static int lan9303_setup(struct dsa_switch *ds)
> if (ret)
> dev_err(chip->dev, "failed to separate ports %d\n", ret);
>
> - ret = lan9303_enable_packet_processing(chip, LAN9303_PORT_0_OFFSET);
> + ret = lan9303_enable_packet_processing(chip, 0);
> if (ret)
> dev_err(chip->dev, "failed to re-enable switching %d\n", ret);
>
> @@ -704,7 +707,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
> unsigned int u, poff;
> int ret;
>
> - poff = port * 0x400;
> + poff = LAN9303_SWITCH_PORT_REG(port, 0);
>
> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
> ret = lan9303_read_switch_reg(chip,
> @@ -757,11 +760,9 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
> /* enable internal packet processing */
> switch (port) {
> case 1:
> - return lan9303_enable_packet_processing(chip,
> - LAN9303_PORT_1_OFFSET);
> + return lan9303_enable_packet_processing(chip, port);
> case 2:
> - return lan9303_enable_packet_processing(chip,
> - LAN9303_PORT_2_OFFSET);
> + return lan9303_enable_packet_processing(chip, port);
case 1 and 2 are now identical. So you can simplify this.
> default:
> dev_dbg(chip->dev,
> "Error: request to power up invalid port %d\n", port);
> @@ -778,12 +779,12 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
> /* disable internal packet processing */
> switch (port) {
> case 1:
> - lan9303_disable_packet_processing(chip, LAN9303_PORT_1_OFFSET);
> + lan9303_disable_packet_processing(chip, port);
> lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
> MII_BMCR, BMCR_PDOWN);
> break;
> case 2:
> - lan9303_disable_packet_processing(chip, LAN9303_PORT_2_OFFSET);
> + lan9303_disable_packet_processing(chip, port);
> lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
> MII_BMCR, BMCR_PDOWN);
And this can also be simplified.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 0/2] Refactor lan9303_xxx_packet_processing
From: Egil Hjelmeland @ 2017-07-31 13:32 UTC (permalink / raw)
To: Andrew Lunn; +Cc: vivien.didelot, f.fainelli, netdev, linux-kernel, kernel
In-Reply-To: <20170731132236.GB24562@lunn.ch>
On 31. juli 2017 15:22, Andrew Lunn wrote:
> On Mon, Jul 31, 2017 at 01:33:53PM +0200, Egil Hjelmeland wrote:
>> First patch:
>> Change lan9303_enable_packet_processing,
>> lan9303_disable_packet_processing():
>> Pass port number (0,1,2) as parameter instead of port offset.
>> Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG()
>
> Hi Egil
>
> The cover note and the commit message should concentrate on the Why,
> not the What. I can see the What by reading the patch.
>
> Why do you want to pass port numbers?
>
Because other functions in the module pass port numbers. And to enable
the simplifications.
> Note that for netdev, the cover note also ends up in git as part of
> the history.
>
OK, I did not know that netdev does that.
> Andrew
>
^ permalink raw reply
* Re: Performance regression with virtio_net
From: Michael S. Tsirkin @ 2017-07-31 13:26 UTC (permalink / raw)
To: Euan Kemp; +Cc: seth.forshee, netdev
In-Reply-To: <20170730222552.4fxxnx3jxg4yv65g@multivac.euank.com>
On Sun, Jul 30, 2017 at 03:25:52PM -0700, Euan Kemp wrote:
> I've also observed this performance regression.
>
> The minimal fix for me is removing the two
> > if (unlikely(len > (unsigned long)ctx))
> checks added in 680557c.
>
> After digging a little more, the reason that check can fail appears to
> be that add_recvbuf_mergeable sometimes includes a hole at the end,
> which is included in len but not ctx.
>
> I'd send a patch removing those conditions, but I'm not certain
> whether "truesize" in receive_mergeable should also be changed back to
> be the max of len/ctx, or should remain as-is.
>
> - Euan
Thanks a lot for looking into it!
I kept this around unchanged from
ab7db91705e95ed1bba1304388936fccfa58c992. That commit had an internal
reason not to account for that space: not enough bits to do it. No
longer true so let's account for length exactly. I'll send a proper
patch after a bit of testing, would appreciate reports reports of
whether this helps very much.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f41ab0e..782c33f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -889,7 +889,6 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
buf += headroom; /* advance address leaving hole at front of pkt */
- ctx = (void *)(unsigned long)len;
get_page(alloc_frag->page);
alloc_frag->offset += len + headroom;
hole = alloc_frag->size - alloc_frag->offset;
@@ -904,6 +903,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
}
sg_init_one(rq->sg, buf, len);
+ ctx = (void *)(unsigned long)len;
err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
if (err < 0)
put_page(virt_to_head_page(buf));
--
MST
^ 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