* Re: cellular modem APIs - take 2
From: Johannes Berg @ 2019-05-29 20:16 UTC (permalink / raw)
To: Dan Williams, netdev, linux-wireless
Cc: Subash Abhinov Kasiviswanathan, Sean Tranchetti, Daniele Palmas,
Aleksander Morgado, Bjørn Mork
In-Reply-To: <cb2ef612be9e347a7cbceb26831f8d5ebea4eacf.camel@redhat.com>
Hi Dan,
> > Quite possibly there might be some additional (vendor-dependent?)
> > configuration for when such netdevs are created, but we need to
> > figure out if that really needs to be at creation time, or just
> > ethtool later or something like that. I guess it depends on how
> > generic it needs to be.
>
> I'm pretty sure it would have to be async via netlink or ethtool or
> whatever later, because the control plane (ModemManager,
> libmbim/libqmi, oFono, etc) is what sets up the PDP/EPS context and
> thus the data channel. A netdev (or a queue on that netdev) would be a
> representation of that data channel, but that's not something the
> kernel knows beforehand.
Right.
It just seemed that people do want to have a netdev (if only to see that
their driver loaded and use ethtool to dump the firmware version), and
then you can reassign it to some actual context later?
It doesn't really matter much. If you have a control channel and higher-
level abstraction (wwan device) then having the netdev is probably more
of a nuisance and mostly irrelevant, just might be useful for legacy
reasons.
> > 3) Separately, we need to have an ability to create "generalized control
> > channels". I'm thinking there would be a general command "create
> > control channel" with a given type (e.g. ATCMD, RPC, MBIM, GNSS) plus
> > a list of vendor-specific channels (e.g. for tracing).
> >
> > I'm unsure where this channel should really go - somehow it seems to
> > me that for many (most?) of these registering them as a serial line
> > would be most appropriate, but some, especially vendor-defined
> > channels like tracing, would probably better use a transport that's
> > higher bandwidth than, e.g. netdevs.
>
> There's only a couple protocols that are run over serial transport,
> namely AT, DM/DIAG, and Sierra's CnS.
Right.
> Most of the rest like QMI and MBIM are packet-based protocols that can
> use different transports. QMI for example can use CDC-WDM for USB
> devices but on SoCs will use Qualcomm's SMD I believe.
Right, though transport and protocol are sort of different issues.
> Should you really need to account for these specially, or would some
> kind of sysfs linkage like SET_NETDEV_DEV() be more appropriate?
>
> Really all you want to do is (a) identify which WWAN device a given
> control/data channel is for and (b) perhaps tag different control/data
> channels with attributes like primary/secondary/gps/sim/etc often
> through USB attributes or hardcoded data on SoCs.
Ah, that brings up something I completely forgot.
I was thinking only of the case that the control channel(s) to the
device is/are all managed by the *kernel* driver, i.e. you'd have some
device-specific driver that has an interface into userspace to talk to
the modem's control channel (and that we could abstract).
However, yes, that's not true - many will be like USB where the control
channel is driven by a generic kernel driver (e.g. maybe usb-serial) or
no kernel driver at all, and then this linkage is probably the right
approach. Need to think about it.
OTOH, there will be device-specific ways to add more control channels
(say for debug/trace purposes etc.) and those would not have a "natural"
interface to userspace like control channels with generic/no drivers.
> > One way I thought of doing this was to create an abstraction in the
> > wwan framework that lets the driver use SKBs anyway (i.e. TX and RX
> > on these channels using SKBs) and then translate them to some channel
> > in the framework - that way, we can even select at runtime if we want
> > a netdev (not really plugged into the network stack, ARPHDR_VOID?) or
> > some other kind of transport. Building that would allow us to add
> > transport types in the future too.
>
> I'm not quite sure what you mean here, can you explain?
I was just thinking of the mechanics of doing this in the driver (while,
like I said above, completely forgetting about the non-monolithic driver
case). It's not really that important.
> > I guess such a channel should also be created by default, if it's
> > not already created by the driver in some out-of-band way anyway (and
> > most likely it shouldn't be, but I guess drivers might have some
> > entirely different communication channels for AT CMDs?)
>
> For existing devices we're not talking about monolithic drivers here
> (excepting 'hso') which I guess is part of the issue.
Right, and doesn't help I forgot about this ;-)
> A given device
> might be driven by 2 or 3 different kernel drivers (usb-serial derived,
> netdev, cdc-wdm) and they all expose different channels and none of
> them coordinate. You have to do sysfs matching up the family tree to
> find out they are associated with each other. If the kernel can take
> over some of that responsibility great.
Right. I guess it's hard for the kernel to take responsibility unless we
teach all the components (usb-serial, ...) that certain devices are
modems and should get some (additional?) registration?
> But the diversity is large. Any given TTY representing an AT channel
> could be from USB drivers (usb-serial, cdc-wdm, vendor-specific driver
> like sierra/hso, option) or PCI (nozomi) or platform stuff (Qualcomm
> SoC ones). You can also do AT-over-QMI if you want.
Right. The linkage here is the interesting part - for platform stuff
that might be easier (devicetree?) but not sure how we could teach that
to e.g. usb-serial, and nozomi is interesting because ... where is the
data plane even?
> I think it's worth discussing how this could be better unified but
> maybe small steps to get there are more feasible.
Fair point.
> > 4) There was a question about something like pure IP channels that
> > belong to another PDN and apparently now separate netdevs might be
> > used, but it seems to me that could just be a queue reserved on the
> > regular netdevs and then when you say ("enable video streaming on
> > wwan1 interface") that can do some magic to classify the video
> > packets (DSCP?) to another hardware queue for better QoS.
>
> Most stuff is pure IP these days (both for QMI/rmnet and MBIM at least)
> and having ethernet encapsulation is kinda pointless.
Yeah, true, not really sure why I was distinguishing this in these
terms. I think the use case really was just giving some packets higher
priority, putting them into a different *hardware* queue so the device
can see them even if the "normal" hardware queue is completely
backlogged.
Kinda a typical multi-queue TX use case.
> But anyway you'd
> need some mechanism to map that particular queue to a given channel/PDN
> created by the control channel.
Well, I was thinking that mechanism was creating the netdev, but then
*within* that some QoS seems to be desired.
> But classification is mostly done in the hardware/network because
> setting different QoS for a given PDP/EPS context is basically saying
> how much airtime the queue gets. No amount of kernel involvement is
> going to change what the network lets you transmit.
Right.
> And I honestly
> don't know how the firmware responds when its internal queues for a
> given context are full that would tell a kernel queue to stop sending
> more.
Well, at the very least it'll stop pulling packets from DMA/whatever, so
the kernel has to back off, right?
johannes
^ permalink raw reply
* Re: cellular modem APIs - take 2
From: Johannes Berg @ 2019-05-29 19:59 UTC (permalink / raw)
To: Marcel Holtmann
Cc: netdev, linux-wireless, Subash Abhinov Kasiviswanathan,
Dan Williams, Sean Tranchetti, Daniele Palmas, Aleksander Morgado,
Bjørn Mork
In-Reply-To: <662BBC5C-D0C7-4B2C-A001-D6F490D0F36F@holtmann.org>
Hi Marcel,
> Have you actually looked at Phonet or CAIF.
Phonet is just a specific protocol spoken by a specific modem (family)
for their control plane. Not all modems are going to be speaking this.
Same for CAIF, really. I don't really see all that much that's generic
(enough) here. It's unfortunate that in all this time nobody ever
bothered to even try though, and just invented all their own mechanisms
to precisely mirror the hardware and firmware they were working with.
Theoretically now, you could build a driver that still speaks CAIF or
phonet and then translates to a specific modem, but what would the point
be?
Now, I'm looking more at the data plan than the control plane, so I
guess you could argue I should've not just mentioned MBIM and AT
commands, but also something like Phonet/CAIF.
> And netdev by default seems like repeating the same mistake we have
> done with WiFi. Your default context in LTE cases is only available
> when actually registered to the LTE bearer. It is pretty much
> pointless to have a netdev if you are not registered to the network.
I partially agree with this.
Of course, for WiFi, that's just wrong since the control path is on the
netdev. Without a netdev, nothing works, and so not having one by
default just adds something pointless to the setup necessary to bring up
anything at all. It can be argued whether not allowing to remove it is
right, but that just detracts from the discussion at hand and there's
also a lot of history here.
I do understand (and mostly agree) that having a netdev by default
that's not connected to anything and has no functionality until you've
done some out-of-band (as far as the netdev is concerned) setup is
fairly much pointless, but OTOH having a netdev is something that people
seem to want for various reasons (discovery, ethtool, ...).
> You have to do a lot of initial modem setup before you ever get to the
> having your default context connected. Have a look at oFono and what
> it does to bring up the modem.
Sure.
> > 2) Clearly, one needs to be able to create PDN netdevs, with the PDN
> > given to the command. Here's another advantage: If these are created
> > on top of another abstraction, not another netdev, they can have
> > their own queues, multiqueue RX etc. much more easily.
[...]
> I think you need to provide actually a lot more details on how queue
> control inside Linux would be helpful and actually work in the first
> place. I don’t see how Linux will be ever in charge and not the modem
> do this all for you.
I think you misunderstand. I wasn't really talking about *queue control*
(packet dequeue etc.) although this is *also* something that could be
interesting, since the modem can only control packets that ever made it
to the hardware.
I was more thinking of what I actually wrote - "have their own queues,
multiqueue RX etc." - i.e. control the software layer of the queues in
the driver, rather than having all of that managed in some stacked
netdev like VLAN.
For example, with stacked netdevs like VLANs it gets difficult (and
awkward from a network stack perspective) to put frames for different
connections (stacked netdevs) into different hardware queues and manage
flow control correctly.
Similarly, if one connection has maybe multiple hardware queues (say for
a specific video stream) disentangling that when you have stacked
netdevs is much harder than when they're all separate.
(And, of course, there's little point in having the underlying netdev to
start with since it speaks a device-specific protocol the network stack
will never understand.)
> > 3) Separately, we need to have an ability to create "generalized control
> > channels". I'm thinking there would be a general command "create
> > control channel" with a given type (e.g. ATCMD, RPC, MBIM, GNSS) plus
> > a list of vendor-specific channels (e.g. for tracing).
[...]
> > I guess such a channel should also be created by default, if it's
> > not already created by the driver in some out-of-band way anyway (and
> > most likely it shouldn't be, but I guess drivers might have some
> > entirely different communication channels for AT CMDs?)
>
> I would just use sockets like Phonet and CAIF.
It was in fact one of the options I considered, but it seems to have
very little traction with any other userspace, and having a separate
socket family yet again also seems a bit pointless. I guess for devices
that do already speak Phonet or CAIF that would make sense. Possible to
some extent, but not really useful, would be to terminate the Phonet or
CAIF protocol inside the driver or stack, but then you end up having to
emulate some specific firmware behaviour ...
So ultimately it boils down to what protocols you want to support and
what kind of API they typically use. I guess I don't have enough hubris
to propose unifying the whole command set and how you talk to your
device ;-)
I suppose you could have a socket type for AT commands, but is that
really all that useful? Or a socket type that muxes the different
control channels you might have, which gets close to phonet/caif.
> Frankly I have a problem if this is designed from the hardware point
> of view. Unless you are familiar with how 3GPP works and what a
> telephony stack like oFono has to do, there is really no point in
> trying to create a WWAN subsystem.
>
> Anything defined needs to be defined in terms of 3GPP and not what
> current drivers have hacked together.
That objection makes no sense to me. 3GPP doesn't define how the data
plane is implemented in your device/OS. There's a data plane, it carries
IP packets, but how you get those to your applications?
After all, I'm not really proposing that we put oFono or something like
it into the kernel - far from it! I'm only proposing that we kill the
many various ways of creating and managing the necessary netdevs (VLANs,
sysfs, rmnet, ...) from a piece of software like oFono (or libmbim or
whatever else).
Apart from CAIF and phonet, oFono doesn't even try to do this though,
afaict, so I guess it relies on the default netdev created, or some out-
of-band configuration is still needed?
johannes
^ permalink raw reply
* Re: cellular modem APIs - take 2
From: Dan Williams @ 2019-05-29 19:59 UTC (permalink / raw)
To: Johannes Berg, netdev, linux-wireless
Cc: Subash Abhinov Kasiviswanathan, Sean Tranchetti, Daniele Palmas,
Aleksander Morgado, Bjørn Mork
In-Reply-To: <b59be15f1d0caa4eaa4476bbd8457afc44d57089.camel@sipsolutions.net>
On Mon, 2019-05-27 at 15:20 +0200, Johannes Berg wrote:
> Hi all,
>
> Sorry for the long delay in getting back to this. I'm meaning to write
> some code soon also for this, to illustrate better, but I figured I'd
> still get some thoughts out before I do that.
>
> After more discussion (@Intel) and the previous thread(s), I've pretty
> much come to the conclusion that we should have a small subsystem for
> WWAN, rather than fudging everything like we previously did.
>
> We can debate whether or not that should use 'real' netlink or generic
> netlink - personally I know the latter better and I think it has some
> real advantages like easier message parsing (it's automatic more or
> less) including strict checking and automatic policy introspection (I
> recently wrote the code for this and it's plugged into generic netlink
> family, for other netlink families it needs more hand-written code). But
> I could possibly be convinced of doing something else, and/or perhaps
> building more infrastructure for 'real' netlink to realize those
> benefits there as well.
>
>
> In terms of what I APIs are needed, the kernel-driver side and userspace
> side go pretty much hand in hand (the wwan subsystem just providing the
> glue), so what I say below is pretty much both a method/function call
> (kernel internal API) or a netlink message (userspace API).
>
> 1) I think a generic abstraction of WWAN device that is not a netdev
> is needed. Yes, on the one hand it's quite nice to be able to work on
> top of a given netdev, but it's also limiting because it requires the
> data flow to go through there, and packets that are tagged in some
> way be exchanged there.
> For VLANs this can be out-of-band (in a sense) with hw-accel, but for
> rmnet-style it's in-band, and that limits what we can do.
>
> Now, of course this doesn't mean there shouldn't be a netdev created
> by default in most cases, but it gives us a way to do additional
> things that we cannot do with *just* a netdev.
>
> From a driver POV though, it would register a new "wwan_device", and
> then get some generic callback to create a netdev on top, maybe by
> default from the subsystem or from the user.
>
> 2) Clearly, one needs to be able to create PDN netdevs, with the PDN
> given to the command. Here's another advantage: If these are created
> on top of another abstraction, not another netdev, they can have
> their own queues, multiqueue RX etc. much more easily.
>
> Also, things like the "if I have just a single channel, drop the mux
> headers" can then be entirely done in the driver, and the default
> netdev no longer has the possibility of muxed and IP frames on the
> same datapath.
>
> This also enables more things like handling checksum offload directly
> in the driver, which doesn't behave so well with VLANs I think.
>
> All of that will just be easier for 5G too, I believe, with
> acceleration being handled per PDN, multi-queue working without
> ndo_select_queue, etc.
>
> Quite possibly there might be some additional (vendor-dependent?)
> configuration for when such netdevs are created, but we need to
> figure out if that really needs to be at creation time, or just
> ethtool later or something like that. I guess it depends on how
> generic it needs to be.
I'm pretty sure it would have to be async via netlink or ethtool or
whatever later, because the control plane (ModemManager,
libmbim/libqmi, oFono, etc) is what sets up the PDP/EPS context and
thus the data channel. A netdev (or a queue on that netdev) would be a
representation of that data channel, but that's not something the
kernel knows beforehand.
> 3) Separately, we need to have an ability to create "generalized control
> channels". I'm thinking there would be a general command "create
> control channel" with a given type (e.g. ATCMD, RPC, MBIM, GNSS) plus
> a list of vendor-specific channels (e.g. for tracing).
>
> I'm unsure where this channel should really go - somehow it seems to
> me that for many (most?) of these registering them as a serial line
> would be most appropriate, but some, especially vendor-defined
> channels like tracing, would probably better use a transport that's
> higher bandwidth than, e.g. netdevs.
There's only a couple protocols that are run over serial transport,
namely AT, DM/DIAG, and Sierra's CnS.
Most of the rest like QMI and MBIM are packet-based protocols that can
use different transports. QMI for example can use CDC-WDM for USB
devices but on SoCs will use Qualcomm's SMD I believe.
Should you really need to account for these specially, or would some
kind of sysfs linkage like SET_NETDEV_DEV() be more appropriate?
Really all you want to do is (a) identify which WWAN device a given
control/data channel is for and (b) perhaps tag different control/data
channels with attributes like primary/secondary/gps/sim/etc often
through USB attributes or hardcoded data on SoCs.
> One way I thought of doing this was to create an abstraction in the
> wwan framework that lets the driver use SKBs anyway (i.e. TX and RX
> on these channels using SKBs) and then translate them to some channel
> in the framework - that way, we can even select at runtime if we want
> a netdev (not really plugged into the network stack, ARPHDR_VOID?) or
> some other kind of transport. Building that would allow us to add
> transport types in the future too.
I'm not quite sure what you mean here, can you explain?
> I guess such a channel should also be created by default, if it's
> not already created by the driver in some out-of-band way anyway (and
> most likely it shouldn't be, but I guess drivers might have some
> entirely different communication channels for AT CMDs?)
For existing devices we're not talking about monolithic drivers here
(excepting 'hso') which I guess is part of the issue. A given device
might be driven by 2 or 3 different kernel drivers (usb-serial derived,
netdev, cdc-wdm) and they all expose different channels and none of
them coordinate. You have to do sysfs matching up the family tree to
find out they are associated with each other. If the kernel can take
over some of that responsibility great.
But the diversity is large. Any given TTY representing an AT channel
could be from USB drivers (usb-serial, cdc-wdm, vendor-specific driver
like sierra/hso, option) or PCI (nozomi) or platform stuff (Qualcomm
SoC ones). You can also do AT-over-QMI if you want.
I think it's worth discussing how this could be better unified but
maybe small steps to get there are more feasible.
> 4) There was a question about something like pure IP channels that
> belong to another PDN and apparently now separate netdevs might be
> used, but it seems to me that could just be a queue reserved on the
> regular netdevs and then when you say ("enable video streaming on
> wwan1 interface") that can do some magic to classify the video
> packets (DSCP?) to another hardware queue for better QoS.
Most stuff is pure IP these days (both for QMI/rmnet and MBIM at least)
and having ethernet encapsulation is kinda pointless. But anyway you'd
need some mechanism to map that particular queue to a given channel/PDN
created by the control channel.
But classification is mostly done in the hardware/network because
setting different QoS for a given PDP/EPS context is basically saying
how much airtime the queue gets. No amount of kernel involvement is
going to change what the network lets you transmit. And I honestly
don't know how the firmware responds when its internal queues for a
given context are full that would tell a kernel queue to stop sending
more.
Dan
>
>
> Anyway, if all of this doesn't seem completely outlandish I'll try to
> write some code to illustrate it (sooner, rather than later).
>
> Thanks,
> johannes
>
^ permalink raw reply
* Re: cellular modem APIs - take 2
From: Marcel Holtmann @ 2019-05-29 19:05 UTC (permalink / raw)
To: Johannes Berg
Cc: netdev, linux-wireless, Subash Abhinov Kasiviswanathan,
Dan Williams, Sean Tranchetti, Daniele Palmas, Aleksander Morgado,
Bjørn Mork
In-Reply-To: <b59be15f1d0caa4eaa4476bbd8457afc44d57089.camel@sipsolutions.net>
Hi Johannes,
> Sorry for the long delay in getting back to this. I'm meaning to write
> some code soon also for this, to illustrate better, but I figured I'd
> still get some thoughts out before I do that.
>
> After more discussion (@Intel) and the previous thread(s), I've pretty
> much come to the conclusion that we should have a small subsystem for
> WWAN, rather than fudging everything like we previously did.
>
> We can debate whether or not that should use 'real' netlink or generic
> netlink - personally I know the latter better and I think it has some
> real advantages like easier message parsing (it's automatic more or
> less) including strict checking and automatic policy introspection (I
> recently wrote the code for this and it's plugged into generic netlink
> family, for other netlink families it needs more hand-written code). But
> I could possibly be convinced of doing something else, and/or perhaps
> building more infrastructure for 'real' netlink to realize those
> benefits there as well.
>
>
> In terms of what I APIs are needed, the kernel-driver side and userspace
> side go pretty much hand in hand (the wwan subsystem just providing the
> glue), so what I say below is pretty much both a method/function call
> (kernel internal API) or a netlink message (userspace API).
>
> 1) I think a generic abstraction of WWAN device that is not a netdev
> is needed. Yes, on the one hand it's quite nice to be able to work on
> top of a given netdev, but it's also limiting because it requires the
> data flow to go through there, and packets that are tagged in some
> way be exchanged there.
> For VLANs this can be out-of-band (in a sense) with hw-accel, but for
> rmnet-style it's in-band, and that limits what we can do.
>
> Now, of course this doesn't mean there shouldn't be a netdev created
> by default in most cases, but it gives us a way to do additional
> things that we cannot do with *just* a netdev.
>
> From a driver POV though, it would register a new "wwan_device", and
> then get some generic callback to create a netdev on top, maybe by
> default from the subsystem or from the user.
Have you actually looked at Phonet or CAIF.
And netdev by default seems like repeating the same mistake we have done with WiFi. Your default context in LTE cases is only available when actually registered to the LTE bearer. It is pretty much pointless to have a netdev if you are not registered to the network.
You have to do a lot of initial modem setup before you ever get to the having your default context connected. Have a look at oFono and what it does to bring up the modem.
> 2) Clearly, one needs to be able to create PDN netdevs, with the PDN
> given to the command. Here's another advantage: If these are created
> on top of another abstraction, not another netdev, they can have
> their own queues, multiqueue RX etc. much more easily.
>
> Also, things like the "if I have just a single channel, drop the mux
> headers" can then be entirely done in the driver, and the default
> netdev no longer has the possibility of muxed and IP frames on the
> same datapath.
>
> This also enables more things like handling checksum offload directly
> in the driver, which doesn't behave so well with VLANs I think.
>
> All of that will just be easier for 5G too, I believe, with
> acceleration being handled per PDN, multi-queue working without
> ndo_select_queue, etc.
>
> Quite possibly there might be some additional (vendor-dependent?)
> configuration for when such netdevs are created, but we need to
> figure out if that really needs to be at creation time, or just
> ethtool later or something like that. I guess it depends on how
> generic it needs to be.
I think you need to provide actually a lot more details on how queue control inside Linux would be helpful and actually work in the first place. I don’t see how Linux will be ever in charge and not the modem do this all for you.
> 3) Separately, we need to have an ability to create "generalized control
> channels". I'm thinking there would be a general command "create
> control channel" with a given type (e.g. ATCMD, RPC, MBIM, GNSS) plus
> a list of vendor-specific channels (e.g. for tracing).
>
> I'm unsure where this channel should really go - somehow it seems to
> me that for many (most?) of these registering them as a serial line
> would be most appropriate, but some, especially vendor-defined
> channels like tracing, would probably better use a transport that's
> higher bandwidth than, e.g. netdevs.
>
> One way I thought of doing this was to create an abstraction in the
> wwan framework that lets the driver use SKBs anyway (i.e. TX and RX
> on these channels using SKBs) and then translate them to some channel
> in the framework - that way, we can even select at runtime if we want
> a netdev (not really plugged into the network stack, ARPHDR_VOID?) or
> some other kind of transport. Building that would allow us to add
> transport types in the future too.
>
> I guess such a channel should also be created by default, if it's
> not already created by the driver in some out-of-band way anyway (and
> most likely it shouldn't be, but I guess drivers might have some
> entirely different communication channels for AT CMDs?)
I would just use sockets like Phonet and CAIF.
> 4) There was a question about something like pure IP channels that
> belong to another PDN and apparently now separate netdevs might be
> used, but it seems to me that could just be a queue reserved on the
> regular netdevs and then when you say ("enable video streaming on
> wwan1 interface") that can do some magic to classify the video
> packets (DSCP?) to another hardware queue for better QoS.
>
>
>
> Anyway, if all of this doesn't seem completely outlandish I'll try to
> write some code to illustrate it (sooner, rather than later).
Frankly I have a problem if this is designed from the hardware point of view. Unless you are familiar with how 3GPP works and what a telephony stack like oFono has to do, there is really no point in trying to create a WWAN subsystem.
Anything defined needs to be defined in terms of 3GPP and not what current drivers have hacked together.
Regards
Marcel
^ permalink raw reply
* Re: [RFC PATCH v3] rtl8xxxu: Improve TX performance of RTL8723BU on rtl8xxxu driver
From: Larry Finger @ 2019-05-29 18:22 UTC (permalink / raw)
To: Chris Chiu, jes.sorensen, kvalo, davem
Cc: linux-wireless, netdev, linux-kernel, linux
In-Reply-To: <20190529050335.72061-1-chiu@endlessm.com>
On 5/29/19 12:03 AM, Chris Chiu wrote:
> We have 3 laptops which connect the wifi by the same RTL8723BU.
> The PCI VID/PID of the wifi chip is 10EC:B720 which is supported.
> They have the same problem with the in-kernel rtl8xxxu driver, the
> iperf (as a client to an ethernet-connected server) gets ~1Mbps.
> Nevertheless, the signal strength is reported as around -40dBm,
> which is quite good. From the wireshark capture, the tx rate for each
> data and qos data packet is only 1Mbps. Compare to the driver from
> https://github.com/lwfinger/rtl8723bu, the same iperf test gets ~12
> Mbps or more. The signal strength is reported similarly around
> -40dBm. That's why we want to improve.
The driver at GitHub was written by Realtek. I only published it in a prominent
location, and fix it for kernel API changes. I would say "the Realtek driver at
https://...", and every mention of "Larry's driver" should say "Realtek's
driver". That attribution is more correct.
>
> After reading the source code of the rtl8xxxu driver and Larry's, the
> major difference is that Larry's driver has a watchdog which will keep
> monitoring the signal quality and updating the rate mask just like the
> rtl8xxxu_gen2_update_rate_mask() does if signal quality changes.
> And this kind of watchdog also exists in rtlwifi driver of some specific
> chips, ex rtl8192ee, rtl8188ee, rtl8723ae, rtl8821ae...etc. They have
> the same member function named dm_watchdog and will invoke the
> corresponding dm_refresh_rate_adaptive_mask to adjust the tx rate
> mask.
>
> With this commit, the tx rate of each data and qos data packet will
> be 39Mbps (MCS4) with the 0xF00000 as the tx rate mask. The 20th bit
> to 23th bit means MCS4 to MCS7. It means that the firmware still picks
> the lowest rate from the rate mask and explains why the tx rate of
> data and qos data is always lowest 1Mbps because the default rate mask
> passed is always 0xFFFFFFF ranges from the basic CCK rate, OFDM rate,
> and MCS rate. However, with Larry's driver, the tx rate observed from
> wireshark under the same condition is almost 65Mbps or 72Mbps.
>
> I believe the firmware of RTL8723BU may need fix. And I think we
> can still bring in the dm_watchdog as rtlwifi to improve from the
> driver side. Please leave precious comments for my commits and
> suggest what I can do better. Or suggest if there's any better idea
> to fix this. Thanks.
>
> Signed-off-by: Chris Chiu <chiu@endlessm.com>
I have not tested this patch, but I plan to soon.
Larry
^ permalink raw reply
* Re: [RFC PATCH v3] rtl8xxxu: Improve TX performance of RTL8723BU on rtl8xxxu driver
From: Daniel Drake @ 2019-05-29 18:11 UTC (permalink / raw)
To: Chris Chiu
Cc: Jes Sorensen, Kalle Valo, David Miller, linux-wireless, netdev,
Linux Kernel, Linux Upstreaming Team
In-Reply-To: <20190529050335.72061-1-chiu@endlessm.com>
Hi Chris,
On Tue, May 28, 2019 at 11:03 PM Chris Chiu <chiu@endlessm.com> wrote:
> + /*
> + * Single virtual interface permitted since the driver supports STATION
> + * mode only.
I think you can be a bit more explicit by saying e.g.:
Only one virtual interface permitted because only STA mode is
supported and no iface_combinations are provided.
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 039e5ca9d2e4..2d612c2df5b2 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -4345,7 +4345,7 @@ void rtl8xxxu_gen2_update_rate_mask(struct rtl8xxxu_priv *priv,
> h2c.b_macid_cfg.ramask3 = (ramask >> 24) & 0xff;
>
> h2c.ramask.arg = 0x80;
> - h2c.b_macid_cfg.data1 = 0;
> + h2c.b_macid_cfg.data1 = priv->ratr_index;
I think ratr_index can be moved to be a function parameter of the
update_rate_mask function. It looks like all callsites already know
which value they want to set. Then you don't have to store it in the
priv structure.
> @@ -5471,6 +5509,10 @@ static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
>
> switch (vif->type) {
> case NL80211_IFTYPE_STATION:
> + if (!priv->vif)
> + priv->vif = vif;
> + else
> + return -EOPNOTSUPP;
> rtl8xxxu_stop_tx_beacon(priv);
rtl8xxxu_remove_interface should also set priv->vif back to NULL.
> @@ -6183,6 +6259,8 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
> mutex_destroy(&priv->usb_buf_mutex);
> mutex_destroy(&priv->h2c_mutex);
>
> + cancel_delayed_work_sync(&priv->ra_watchdog);
Given that the work was started in rtl8xxxu_start, I think it should
be cancelled in rtl8xxxu_stop() instead.
Daniel
^ permalink raw reply
* Re: [PATCH 02/10] cfg80211: use BIT_ULL in cfg80211_parse_mbssid_data()
From: Kalle Valo @ 2019-05-29 16:28 UTC (permalink / raw)
To: Luca Coelho; +Cc: johannes, linux-wireless, Luca Coelho, Dan Carpented
In-Reply-To: <20190529122537.8564-3-luca@coelho.fi>
Luca Coelho <luca@coelho.fi> writes:
> From: Luca Coelho <luciano.coelho@intel.com>
>
> The seen_indices variable is u64 and in other parts of the code we
> assume mbssid_index_ie[2] can be up to 45, so we should use the 64-bit
> versions of BIT, namely, BIT_ULL().
>
> Reported-by: Dan Carpented <dan.carpenter@oracle.com>
s/ted/ter/ :)
--
Kalle Valo
^ permalink raw reply
* Re: FYI: vendor specific nl80211 API upstream
From: Denis Kenzior @ 2019-05-29 15:49 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
In-Reply-To: <f3847d4efe6822bba3948993ddc4cde31615436f.camel@sipsolutions.net>
Hi Johannes,
On 05/29/2019 04:09 AM, Johannes Berg wrote:
> On Tue, 2019-05-28 at 12:36 -0500, Denis Kenzior wrote:
>>
>> I'm guessing that you guys considered and rejected the idea of pushing
>> these out to a separate, vendor specific genl family instead?
>
> We do actually use that internally (though mostly for cases where we
> don't have a cfg80211 connection like manufacturing support), but vendor
> commands are there and people do like to use them :-)
And herein lies the danger. If you make it too easy to add vendor APIs,
there's no incentive for the vendors to do anything else. In the end
this all becomes a mess for userspace to deal with.
One idea off the top of my head is to introduce a concept of
'experimental' APIs in NL80211, ones that are not guaranteed to be ABI
stable going forward. Specifically for dealing with such 'vendor' APIs.
The semantic difference might be subtle, but I think the effect will
be drastically different. E.g. people will approach this more seriously
and you will get more people reviewing the API.
>
> The idea with formalizing this is that they actually get more
> visibility, and I hope that this will lead to more forming of real
> nl80211 API too.
What about ABI guarantees (to tie it in with the discussion above) ?
If the vendor wants to change their API, can they? Are NL80211 APIs
stable unless they are vendor APIs?
Anyhow, speaking from experience with oFono, which has to deal with a
bazillion of wwan modem vendors, I suspect that the opposite will
actually happen. Any time we let through a vendor API, the vendor lost
any interest in generalizing it further. And it becomes a huge pain to
implement a proper generic one later. I get that there are cases where
something just cannot be generalized. In that case it belongs on a
separate genl family (or whatever) altogether.
So I would highly encourage you to reconsider this decision and
deprecate vendor APIs altogether. If someone really cares, they can
implement their own genl family. It is really not that hard. And then
they control the API, API stability policy, etc.
Regards,
-Denis
^ permalink raw reply
* Re: [PATCH] rtlwifi: Fix null-pointer dereferences in error handling code of rtl_pci_probe()
From: Larry Finger @ 2019-05-29 15:48 UTC (permalink / raw)
To: Jia-Ju Bai, Kalle Valo
Cc: pkshih, davem, linux-wireless, netdev, linux-kernel
In-Reply-To: <357682ba-d1ae-23b1-2372-b1a33d2ba1ac@gmail.com>
On 5/29/19 5:30 AM, Jia-Ju Bai wrote:
>
>
> On 2019/5/28 21:00, Larry Finger wrote:
>> On 5/28/19 6:55 AM, Kalle Valo wrote:
>>> Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
>>>
>>>> *BUG 1:
>>>> In rtl_pci_probe(), when rtlpriv->cfg->ops->init_sw_vars() fails,
>>>> rtl_deinit_core() in the error handling code is executed.
>>>> rtl_deinit_core() calls rtl_free_entries_from_scan_list(), which uses
>>>> rtlpriv->scan_list.list in list_for_each_entry_safe(), but it has been
>>>> initialized. Thus a null-pointer dereference occurs.
>>>> The reason is that rtlpriv->scan_list.list is initialized by
>>>> INIT_LIST_HEAD() in rtl_init_core(), which has not been called.
>>>>
>>>> To fix this bug, rtl_deinit_core() should not be called when
>>>> rtlpriv->cfg->ops->init_sw_vars() fails.
>>>>
>>>> *BUG 2:
>>>> In rtl_pci_probe(), rtl_init_core() can fail when rtl_regd_init() in
>>>> this function fails, and rtlpriv->scan_list.list has not been
>>>> initialized by INIT_LIST_HEAD(). Then, rtl_deinit_core() in the error
>>>> handling code of rtl_pci_probe() is executed. Finally, a null-pointer
>>>> dereference occurs due to the same reason of the above bug.
>>>>
>>>> To fix this bug, the initialization of lists in rtl_init_core() are
>>>> performed before the call to rtl_regd_init().
>>>>
>>>> These bugs are found by a runtime fuzzing tool named FIZZER written by
>>>> us.
>>>>
>>>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>>>
>>> Ping & Larry, is this ok to take?
>>>
>>
>> Kalle,
>>
>> Not at the moment. In reviewing the code, I was unable to see how this
>> situation could develop, and his backtrace did not mention any rtlwifi code.
>> For that reason, I asked him to add printk stat4ements to show the last part
>> of rtl_pci that executed correctly. In
>> https://marc.info/?l=linux-wireless&m=155788322631134&w=2, he promised to do
>> that, but I have not seen the result.
>>
>
> Hi Larry,
>
> This patch is not related to the message you mentioned.
> That message is about an occasional crash that I reported.
> That crash occurred when request_irq() in rtl_pci_intr_mode_legacy() in
> rtl_pci_intr_mode_decide() fails.
> I have added printk statements and try to reproduce and debug that crash, but
> that crash does not always occur, and I still do not know the root cause of that
> crash.
>
> The null-pointer dereferences fixed by this patch are different from that crash,
> and they always occur when the related functions fail.
> So please review these null-pointer dereferences, thanks :)
>
>
> Best wishes,
> Jia-Ju Bai
Sorry if I got confused. Kalle has dropped this patch, thus you will need to
submit a new version.
Larry
^ permalink raw reply
* Re: [PATCH 01/11] rtw88: resolve order of tx power setting routines
From: Larry Finger @ 2019-05-29 15:16 UTC (permalink / raw)
To: yhchuang, kvalo; +Cc: linux-wireless
In-Reply-To: <1559116487-5244-2-git-send-email-yhchuang@realtek.com>
On 5/29/19 2:54 AM, yhchuang@realtek.com wrote:
> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> Some functions that should be static are unnecessarily exposed, remove
> their declaration in header file phy.h.
>
> After resolving their declaration order, they can be declared as static.
> So this commit changes nothing except the order and marking them static.
>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
This patch does not apply. Using quilt to see what is wrong, there are 6 changes
that have already been applied.
Larry
^ permalink raw reply
* [PATCH] rtw88: Remove set but not used variable 'ip_sel' and 'orig'
From: YueHaibing @ 2019-05-29 14:57 UTC (permalink / raw)
To: yhchuang, kvalo, davem; +Cc: linux-kernel, netdev, linux-wireless, YueHaibing
Fixes gcc '-Wunused-but-set-variable' warnings:
drivers/net/wireless/realtek/rtw88/pci.c: In function rtw_pci_phy_cfg:
drivers/net/wireless/realtek/rtw88/pci.c:978:6: warning: variable ip_sel set but not used [-Wunused-but-set-variable]
drivers/net/wireless/realtek/rtw88/phy.c: In function phy_tx_power_limit_config:
drivers/net/wireless/realtek/rtw88/phy.c:1607:11: warning: variable orig set but not used [-Wunused-but-set-variable]
They are never used, so can be removed.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wireless/realtek/rtw88/pci.c | 3 ---
drivers/net/wireless/realtek/rtw88/phy.c | 3 +--
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index 353871c27779..8329f4e447b7 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -977,7 +977,6 @@ static void rtw_pci_phy_cfg(struct rtw_dev *rtwdev)
u16 cut;
u16 value;
u16 offset;
- u16 ip_sel;
int i;
cut = BIT(0) << rtwdev->hal.cut_version;
@@ -990,7 +989,6 @@ static void rtw_pci_phy_cfg(struct rtw_dev *rtwdev)
break;
offset = para->offset;
value = para->value;
- ip_sel = para->ip_sel;
if (para->ip_sel == RTW_IP_SEL_PHY)
rtw_mdio_write(rtwdev, offset, value, true);
else
@@ -1005,7 +1003,6 @@ static void rtw_pci_phy_cfg(struct rtw_dev *rtwdev)
break;
offset = para->offset;
value = para->value;
- ip_sel = para->ip_sel;
if (para->ip_sel == RTW_IP_SEL_PHY)
rtw_mdio_write(rtwdev, offset, value, false);
else
diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
index 404d89432c96..c3e75ffe27b5 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.c
+++ b/drivers/net/wireless/realtek/rtw88/phy.c
@@ -1604,12 +1604,11 @@ void rtw_phy_tx_power_by_rate_config(struct rtw_hal *hal)
static void
phy_tx_power_limit_config(struct rtw_hal *hal, u8 regd, u8 bw, u8 rs)
{
- s8 base, orig;
+ s8 base;
u8 ch;
for (ch = 0; ch < RTW_MAX_CHANNEL_NUM_2G; ch++) {
base = hal->tx_pwr_by_rate_base_2g[0][rs];
- orig = hal->tx_pwr_limit_2g[regd][bw][rs][ch];
hal->tx_pwr_limit_2g[regd][bw][rs][ch] -= base;
}
--
2.17.1
^ permalink raw reply related
* [PATCH] mt76: Remove set but not used variables 'pid' and 'final_mpdu'
From: YueHaibing @ 2019-05-29 14:53 UTC (permalink / raw)
To: nbd, lorenzo.bianconi83, ryder.lee, royluo, kvalo, matthias.bgg,
sgruszka
Cc: linux-kernel, netdev, linux-wireless, linux-arm-kernel,
linux-mediatek, davem, YueHaibing
Fixes gcc '-Wunused-but-set-variable' warnings:
drivers/net/wireless/mediatek/mt76/mt7603/mac.c: In function mt7603_fill_txs:
drivers/net/wireless/mediatek/mt76/mt7603/mac.c:969:5: warning: variable pid set but not used [-Wunused-but-set-variable]
drivers/net/wireless/mediatek/mt76/mt7603/mac.c:961:7: warning: variable final_mpdu set but not used [-Wunused-but-set-variable]
drivers/net/wireless/mediatek/mt76/mt7615/mac.c: In function mt7615_fill_txs:
drivers/net/wireless/mediatek/mt76/mt7615/mac.c:555:5: warning: variable pid set but not used [-Wunused-but-set-variable]
drivers/net/wireless/mediatek/mt76/mt7615/mac.c:552:19: warning: variable final_mpdu set but not used [-Wunused-but-set-variable]
They are never used, so can be removed.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wireless/mediatek/mt76/mt7603/mac.c | 4 ----
drivers/net/wireless/mediatek/mt76/mt7615/mac.c | 5 +----
2 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
index 6d506e34c3ee..5182a36276fc 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
@@ -958,7 +958,6 @@ mt7603_fill_txs(struct mt7603_dev *dev, struct mt7603_sta *sta,
int final_idx = 0;
u32 final_rate;
u32 final_rate_flags;
- bool final_mpdu;
bool ack_timeout;
bool fixed_rate;
bool probe;
@@ -966,7 +965,6 @@ mt7603_fill_txs(struct mt7603_dev *dev, struct mt7603_sta *sta,
bool cck = false;
int count;
u32 txs;
- u8 pid;
int idx;
int i;
@@ -974,9 +972,7 @@ mt7603_fill_txs(struct mt7603_dev *dev, struct mt7603_sta *sta,
probe = !!(info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE);
txs = le32_to_cpu(txs_data[4]);
- final_mpdu = txs & MT_TXS4_ACKED_MPDU;
ampdu = !fixed_rate && (txs & MT_TXS4_AMPDU);
- pid = FIELD_GET(MT_TXS4_PID, txs);
count = FIELD_GET(MT_TXS4_TX_COUNT, txs);
txs = le32_to_cpu(txs_data[0]);
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mac.c b/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
index b8f48d10f27a..a51bfb6990b3 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
@@ -549,23 +549,20 @@ static bool mt7615_fill_txs(struct mt7615_dev *dev, struct mt7615_sta *sta,
{
struct ieee80211_supported_band *sband;
int i, idx, count, final_idx = 0;
- bool fixed_rate, final_mpdu, ack_timeout;
+ bool fixed_rate, ack_timeout;
bool probe, ampdu, cck = false;
u32 final_rate, final_rate_flags, final_nss, txs;
- u8 pid;
fixed_rate = info->status.rates[0].count;
probe = !!(info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE);
txs = le32_to_cpu(txs_data[1]);
- final_mpdu = txs & MT_TXS1_ACKED_MPDU;
ampdu = !fixed_rate && (txs & MT_TXS1_AMPDU);
txs = le32_to_cpu(txs_data[3]);
count = FIELD_GET(MT_TXS3_TX_COUNT, txs);
txs = le32_to_cpu(txs_data[0]);
- pid = FIELD_GET(MT_TXS0_PID, txs);
final_rate = FIELD_GET(MT_TXS0_TX_RATE, txs);
ack_timeout = txs & MT_TXS0_ACK_TIMEOUT;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354
From: Kalle Valo @ 2019-05-29 14:51 UTC (permalink / raw)
To: Doug Anderson
Cc: Arend van Spriel, Madhan Mohan R, Ulf Hansson, LKML,
Hante Meuleman, David S. Miller, netdev, Chi-Hsien Lin,
Brian Norris, linux-wireless, YueHaibing, Adrian Hunter,
open list:ARM/Rockchip SoC..., brcm80211-dev-list.pdl,
Matthias Kaehlcke, Naveen Gupta, Wright Feng, brcm80211-dev-list,
Double Lo, Franky Lin
In-Reply-To: <CAD=FV=VtxdEeFQsdF=U7-_7R+TXfVmA2_JMB_-WYidGHTLDgLw@mail.gmail.com>
Doug Anderson <dianders@chromium.org> writes:
> Hi,
>
> On Tue, May 28, 2019 at 5:18 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> Douglas Anderson <dianders@chromium.org> wrote:
>>
>> > In commit 29f6589140a1 ("brcmfmac: disable command decode in
>> > sdio_aos") we disabled something called "command decode in sdio_aos"
>> > for a whole bunch of Broadcom SDIO WiFi parts.
>> >
>> > After that patch landed I find that my kernel log on
>> > rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
>> > brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep state -110
>> >
>> > This seems to happen every time the Broadcom WiFi transitions out of
>> > sleep mode. Reverting the part of the commit that affects the WiFi on
>> > my boards fixes the problem for me, so that's what this patch does.
>> >
>> > Note that, in general, the justification in the original commit seemed
>> > a little weak. It looked like someone was testing on a SD card
>> > controller that would sometimes die if there were CRC errors on the
>> > bus. This used to happen back in early days of dw_mmc (the controller
>> > on my boards), but we fixed it. Disabling a feature on all boards
>> > just because one SD card controller is broken seems bad. ...so
>> > instead of just this patch possibly the right thing to do is to fully
>> > revert the original commit.
>> >
>> > Fixes: 29f6589140a1 ("brcmfmac: disable command decode in sdio_aos")
>> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>
>> I don't see patch 2 in patchwork and I assume discussion continues.
>
> Apologies. I made sure to CC you individually on all the patches but
> didn't think about the fact that you use patchwork to manage and so
> didn't ensure all patches made it to all lists (by default each patch
> gets recipients individually from get_maintainer). I'll make sure to
> fix for patch set #2. If you want to see all the patches, you can at
> least find them on lore.kernel.org linked from the cover:
>
> https://lore.kernel.org/patchwork/cover/1075373/
No worries, I had the thread on my email but was just too busy to check.
So I instead wrote down my thought process so that somebode can correct
me in case I have misunderstood. I usually do that when it's not clear
what the next action should be.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] rtlwifi: rtl8192cu: fix error handle when usb probe failed
From: Larry Finger @ 2019-05-29 14:51 UTC (permalink / raw)
To: pkshih, kvalo; +Cc: linux-wireless, andreyknvl
In-Reply-To: <20190529065730.25951-1-pkshih@realtek.com>
On 5/29/19 1:57 AM, pkshih@realtek.com wrote:
> From: Ping-Ke Shih <pkshih@realtek.com>
>
> rtl_usb_probe() must do error handle rtl_deinit_core() only if
> rtl_init_core() is done, otherwise goto error_out2.
>
> | usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
> | rtl_usb: reg 0xf0, usbctrl_vendorreq TimeOut! status:0xffffffb9 value=0x0
> | rtl8192cu: Chip version 0x10
> | rtl_usb: reg 0xa, usbctrl_vendorreq TimeOut! status:0xffffffb9 value=0x0
> | rtl_usb: Too few input end points found
> | INFO: trying to register non-static key.
> | the code is fine but needs lockdep annotation.
> | turning off the locking correctness validator.
> | CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.1.0-rc4-319354-g9a33b36 #3
> | Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> | Google 01/01/2011
> | Workqueue: usb_hub_wq hub_event
> | Call Trace:
> | __dump_stack lib/dump_stack.c:77 [inline]
> | dump_stack+0xe8/0x16e lib/dump_stack.c:113
> | assign_lock_key kernel/locking/lockdep.c:786 [inline]
> | register_lock_class+0x11b8/0x1250 kernel/locking/lockdep.c:1095
> | __lock_acquire+0xfb/0x37c0 kernel/locking/lockdep.c:3582
> | lock_acquire+0x10d/0x2f0 kernel/locking/lockdep.c:4211
> | __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> | _raw_spin_lock_irqsave+0x44/0x60 kernel/locking/spinlock.c:152
> | rtl_c2hcmd_launcher+0xd1/0x390
> | drivers/net/wireless/realtek/rtlwifi/base.c:2344
> | rtl_deinit_core+0x25/0x2d0 drivers/net/wireless/realtek/rtlwifi/base.c:574
> | rtl_usb_probe.cold+0x861/0xa70
> | drivers/net/wireless/realtek/rtlwifi/usb.c:1093
> | usb_probe_interface+0x31d/0x820 drivers/usb/core/driver.c:361
> | really_probe+0x2da/0xb10 drivers/base/dd.c:509
> | driver_probe_device+0x21d/0x350 drivers/base/dd.c:671
> | __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778
> | bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454
> | __device_attach+0x223/0x3a0 drivers/base/dd.c:844
> | bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514
> | device_add+0xad2/0x16e0 drivers/base/core.c:2106
> | usb_set_configuration+0xdf7/0x1740 drivers/usb/core/message.c:2021
> | generic_probe+0xa2/0xda drivers/usb/core/generic.c:210
> | usb_probe_device+0xc0/0x150 drivers/usb/core/driver.c:266
> | really_probe+0x2da/0xb10 drivers/base/dd.c:509
> | driver_probe_device+0x21d/0x350 drivers/base/dd.c:671
> | __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778
> | bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454
> | __device_attach+0x223/0x3a0 drivers/base/dd.c:844
> | bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514
> | device_add+0xad2/0x16e0 drivers/base/core.c:2106
> | usb_new_device.cold+0x537/0xccf drivers/usb/core/hub.c:2534
> | hub_port_connect drivers/usb/core/hub.c:5089 [inline]
> | hub_port_connect_change drivers/usb/core/hub.c:5204 [inline]
> | port_event drivers/usb/core/hub.c:5350 [inline]
> | hub_event+0x138e/0x3b00 drivers/usb/core/hub.c:5432
> | process_one_work+0x90f/0x1580 kernel/workqueue.c:2269
> | worker_thread+0x9b/0xe20 kernel/workqueue.c:2415
> | kthread+0x313/0x420 kernel/kthread.c:253
> | ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352
>
> Reported-by: syzbot+1fcc5ef45175fc774231@syzkaller.appspotmail.com
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
I agree that this is a good fix.
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
Larry
> ---
> drivers/net/wireless/realtek/rtlwifi/usb.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> index e24fda5e9087..34d68dbf4b4c 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> @@ -1064,13 +1064,13 @@ int rtl_usb_probe(struct usb_interface *intf,
> rtlpriv->cfg->ops->read_eeprom_info(hw);
> err = _rtl_usb_init(hw);
> if (err)
> - goto error_out;
> + goto error_out2;
> rtl_usb_init_sw(hw);
> /* Init mac80211 sw */
> err = rtl_init_core(hw);
> if (err) {
> pr_err("Can't allocate sw for mac80211\n");
> - goto error_out;
> + goto error_out2;
> }
> if (rtlpriv->cfg->ops->init_sw_vars(hw)) {
> pr_err("Can't init_sw_vars\n");
> @@ -1091,6 +1091,7 @@ int rtl_usb_probe(struct usb_interface *intf,
>
> error_out:
> rtl_deinit_core(hw);
> +error_out2:
> _rtl_usb_io_handler_release(hw);
> usb_put_dev(udev);
> complete(&rtlpriv->firmware_loading_complete);
>
^ permalink raw reply
* Re: [PATCH 4/7] iwlwifi: print fseq info upon fw assert
From: Kalle Valo @ 2019-05-29 14:39 UTC (permalink / raw)
To: Luca Coelho; +Cc: linux-wireless, Shahar S Matityahu, Luca Coelho
In-Reply-To: <20190529133955.31082-5-luca@coelho.fi>
Luca Coelho <luca@coelho.fi> writes:
> From: Shahar S Matityahu <shahar.s.matityahu@intel.com>
>
> Read fseq info from FW registers and print it upon fw assert.
> The print is needed since the fseq version coming from the TLV might
> not be the actual version that is used.
>
> Signed-off-by: Shahar S Matityahu <shahar.s.matityahu@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
[...]
> +void iwl_fw_error_print_fseq_regs(struct iwl_fw_runtime *fwrt)
> +{
> + struct iwl_trans *trans = fwrt->trans;
> + unsigned long flags;
> + int i;
> + struct {
> + u32 addr;
> + const char *str;
> + } fseq_regs[] = {
> + FSEQ_REG(FSEQ_ERROR_CODE),
> + FSEQ_REG(FSEQ_TOP_INIT_VERSION),
> + FSEQ_REG(FSEQ_CNVIO_INIT_VERSION),
> + FSEQ_REG(FSEQ_OTP_VERSION),
> + FSEQ_REG(FSEQ_TOP_CONTENT_VERSION),
> + FSEQ_REG(FSEQ_ALIVE_TOKEN),
> + FSEQ_REG(FSEQ_CNVI_ID),
> + FSEQ_REG(FSEQ_CNVR_ID),
> + FSEQ_REG(CNVI_AUX_MISC_CHIP),
> + FSEQ_REG(CNVR_AUX_MISC_CHIP),
> + FSEQ_REG(CNVR_SCU_SD_REGS_SD_REG_DIG_DCDC_VTRIM),
> + FSEQ_REG(CNVR_SCU_SD_REGS_SD_REG_ACTIVE_VDIG_MIRROR),
> + };
Can fseq_regs be static const?
--
Kalle Valo
^ permalink raw reply
* [PATCH 0/7] wlwifi: fixes intended for 5.2 2019-05-29
From: Luca Coelho @ 2019-05-29 13:39 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Luca Coelho
From: Luca Coelho <luciano.coelho@intel.com>
Hi,
This is the first patchset with fixes for 5.2.
The changes are:
* Fix rfkill bug with newer devices;
* Fix a potential double-free;
* Remove an obsolete debugfs file that, when used with new devices ,
caused an oops;
* Fix a sleep with a spinlock held;
* Use the correct firmware when using a Killer NIC;
* Fix PCI persistence register for newer families;
* Print a firmware-related ID to help debug mismatch issues;
As usual, I'm pushing this to a pending branch, for kbuild bot, and
will send a pull-request later.
Please review.
Cheers,
Luca.
Emmanuel Grumbach (1):
iwlwifi: fix load in rfkill flow for unified firmware
Jia-Ju Bai (1):
iwlwifi: Fix double-free problems in iwl_req_fw_callback()
Johannes Berg (1):
iwlwifi: mvm: remove d3_sram debugfs file
Lior Cohen (1):
iwlwifi: mvm: change TLC config cmd sent by rs to be async
Matt Chen (1):
iwlwifi: fix AX201 killer sku loading firmware issue
Shahar S Matityahu (2):
iwlwifi: clear persistence bit according to device family
iwlwifi: print fseq info upon fw assert
drivers/net/wireless/intel/iwlwifi/fw/dbg.c | 39 +++++++++++++
drivers/net/wireless/intel/iwlwifi/fw/dbg.h | 2 +
drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 1 -
drivers/net/wireless/intel/iwlwifi/iwl-prph.h | 22 ++++++-
drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 22 -------
.../net/wireless/intel/iwlwifi/mvm/debugfs.c | 57 -------------------
drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 23 ++++++--
.../net/wireless/intel/iwlwifi/mvm/mac80211.c | 2 +-
drivers/net/wireless/intel/iwlwifi/mvm/mvm.h | 4 +-
drivers/net/wireless/intel/iwlwifi/mvm/ops.c | 20 ++++---
.../net/wireless/intel/iwlwifi/mvm/rs-fw.c | 3 +-
.../net/wireless/intel/iwlwifi/mvm/utils.c | 2 +
.../wireless/intel/iwlwifi/pcie/internal.h | 2 +-
.../net/wireless/intel/iwlwifi/pcie/trans.c | 53 ++++++++++++-----
14 files changed, 136 insertions(+), 116 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH 1/7] iwlwifi: mvm: remove d3_sram debugfs file
From: Luca Coelho @ 2019-05-29 13:39 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Johannes Berg, Luca Coelho
In-Reply-To: <20190529133955.31082-1-luca@coelho.fi>
From: Johannes Berg <johannes.berg@intel.com>
This debugfs file is really old, and cannot work properly since
the unified image support. Rather than trying to make it work,
which is difficult now due to multiple images (LMAC/UMAC etc.)
just remove it - we no longer need it since we properly do a FW
coredump even in D3 cases.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 22 -------
.../net/wireless/intel/iwlwifi/mvm/debugfs.c | 57 -------------------
drivers/net/wireless/intel/iwlwifi/mvm/mvm.h | 2 -
drivers/net/wireless/intel/iwlwifi/mvm/ops.c | 3 -
4 files changed, 84 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
index 60f5d337f16d..e7e68fb2bd29 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
@@ -1972,26 +1972,6 @@ static void iwl_mvm_query_netdetect_reasons(struct iwl_mvm *mvm,
}
}
-static void iwl_mvm_read_d3_sram(struct iwl_mvm *mvm)
-{
-#ifdef CONFIG_IWLWIFI_DEBUGFS
- const struct fw_img *img = &mvm->fw->img[IWL_UCODE_WOWLAN];
- u32 len = img->sec[IWL_UCODE_SECTION_DATA].len;
- u32 offs = img->sec[IWL_UCODE_SECTION_DATA].offset;
-
- if (!mvm->store_d3_resume_sram)
- return;
-
- if (!mvm->d3_resume_sram) {
- mvm->d3_resume_sram = kzalloc(len, GFP_KERNEL);
- if (!mvm->d3_resume_sram)
- return;
- }
-
- iwl_trans_read_mem_bytes(mvm->trans, offs, mvm->d3_resume_sram, len);
-#endif
-}
-
static void iwl_mvm_d3_disconnect_iter(void *data, u8 *mac,
struct ieee80211_vif *vif)
{
@@ -2054,8 +2034,6 @@ static int __iwl_mvm_resume(struct iwl_mvm *mvm, bool test)
}
iwl_fw_dbg_read_d3_debug_data(&mvm->fwrt);
- /* query SRAM first in case we want event logging */
- iwl_mvm_read_d3_sram(mvm);
if (iwl_mvm_check_rt_status(mvm, vif)) {
set_bit(STATUS_FW_ERROR, &mvm->trans->status);
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c
index d4ff6b44de2c..5b1bb76c5d28 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs.c
@@ -1557,59 +1557,6 @@ static ssize_t iwl_dbgfs_bcast_filters_macs_write(struct iwl_mvm *mvm,
}
#endif
-#ifdef CONFIG_PM_SLEEP
-static ssize_t iwl_dbgfs_d3_sram_write(struct iwl_mvm *mvm, char *buf,
- size_t count, loff_t *ppos)
-{
- int store;
-
- if (sscanf(buf, "%d", &store) != 1)
- return -EINVAL;
-
- mvm->store_d3_resume_sram = store;
-
- return count;
-}
-
-static ssize_t iwl_dbgfs_d3_sram_read(struct file *file, char __user *user_buf,
- size_t count, loff_t *ppos)
-{
- struct iwl_mvm *mvm = file->private_data;
- const struct fw_img *img;
- int ofs, len, pos = 0;
- size_t bufsz, ret;
- char *buf;
- u8 *ptr = mvm->d3_resume_sram;
-
- img = &mvm->fw->img[IWL_UCODE_WOWLAN];
- len = img->sec[IWL_UCODE_SECTION_DATA].len;
-
- bufsz = len * 4 + 256;
- buf = kzalloc(bufsz, GFP_KERNEL);
- if (!buf)
- return -ENOMEM;
-
- pos += scnprintf(buf, bufsz, "D3 SRAM capture: %sabled\n",
- mvm->store_d3_resume_sram ? "en" : "dis");
-
- if (ptr) {
- for (ofs = 0; ofs < len; ofs += 16) {
- pos += scnprintf(buf + pos, bufsz - pos,
- "0x%.4x %16ph\n", ofs, ptr + ofs);
- }
- } else {
- pos += scnprintf(buf + pos, bufsz - pos,
- "(no data captured)\n");
- }
-
- ret = simple_read_from_buffer(user_buf, count, ppos, buf, pos);
-
- kfree(buf);
-
- return ret;
-}
-#endif
-
#define PRINT_MVM_REF(ref) do { \
if (mvm->refs[ref]) \
pos += scnprintf(buf + pos, bufsz - pos, \
@@ -1940,9 +1887,6 @@ MVM_DEBUGFS_READ_WRITE_FILE_OPS(bcast_filters, 256);
MVM_DEBUGFS_READ_WRITE_FILE_OPS(bcast_filters_macs, 256);
#endif
-#ifdef CONFIG_PM_SLEEP
-MVM_DEBUGFS_READ_WRITE_FILE_OPS(d3_sram, 8);
-#endif
#ifdef CONFIG_ACPI
MVM_DEBUGFS_READ_FILE_OPS(sar_geo_profile);
#endif
@@ -2159,7 +2103,6 @@ void iwl_mvm_dbgfs_register(struct iwl_mvm *mvm, struct dentry *dbgfs_dir)
#endif
#ifdef CONFIG_PM_SLEEP
- MVM_DEBUGFS_ADD_FILE(d3_sram, mvm->debugfs_dir, 0600);
MVM_DEBUGFS_ADD_FILE(d3_test, mvm->debugfs_dir, 0400);
debugfs_create_bool("d3_wake_sysassert", 0600, mvm->debugfs_dir,
&mvm->d3_wake_sysassert);
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
index 8dc2a9850bc5..7b829a5be773 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
@@ -1039,8 +1039,6 @@ struct iwl_mvm {
#ifdef CONFIG_IWLWIFI_DEBUGFS
bool d3_wake_sysassert;
bool d3_test_active;
- bool store_d3_resume_sram;
- void *d3_resume_sram;
u32 d3_test_pme_ptr;
struct ieee80211_vif *keep_vif;
u32 last_netdetect_scans; /* no. of scans in the last net-detect wake */
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ops.c b/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
index acd2fda12466..004de67f9157 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
@@ -918,9 +918,6 @@ static void iwl_op_mode_mvm_stop(struct iwl_op_mode *op_mode)
kfree(mvm->error_recovery_buf);
mvm->error_recovery_buf = NULL;
-#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_IWLWIFI_DEBUGFS)
- kfree(mvm->d3_resume_sram);
-#endif
iwl_trans_op_mode_leave(mvm->trans);
iwl_phy_db_free(mvm->phy_db);
--
2.20.1
^ permalink raw reply related
* [PATCH 6/7] iwlwifi: Fix double-free problems in iwl_req_fw_callback()
From: Luca Coelho @ 2019-05-29 13:39 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Jia-Ju Bai, Luca Coelho
In-Reply-To: <20190529133955.31082-1-luca@coelho.fi>
From: Jia-Ju Bai <baijiaju1990@gmail.com>
In the error handling code of iwl_req_fw_callback(), iwl_dealloc_ucode()
is called to free data. In iwl_drv_stop(), iwl_dealloc_ucode() is called
again, which can cause double-free problems.
To fix this bug, the call to iwl_dealloc_ucode() in
iwl_req_fw_callback() is deleted.
This bug is found by a runtime fuzzing tool named FIZZER written by us.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index 852d3cbfc719..fba242284507 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -1597,7 +1597,6 @@ static void iwl_req_fw_callback(const struct firmware *ucode_raw, void *context)
goto free;
out_free_fw:
- iwl_dealloc_ucode(drv);
release_firmware(ucode_raw);
out_unbind:
complete(&drv->request_firmware_complete);
--
2.20.1
^ permalink raw reply related
* [PATCH 7/7] iwlwifi: mvm: change TLC config cmd sent by rs to be async
From: Luca Coelho @ 2019-05-29 13:39 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Lior Cohen, Luca Coelho
In-Reply-To: <20190529133955.31082-1-luca@coelho.fi>
From: Lior Cohen <lior2.cohen@intel.com>
The TLC_MNG_CONFIG sync cmd sent by the rs leads to a kernel warning
of sleeping while in rcu read-side critical section. The fix is to
change the command to be ASYNC (not blocking for the response anymore).
Signed-off-by: Lior Cohen <lior2.cohen@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/mvm/rs-fw.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rs-fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/rs-fw.c
index 659e21b2d4e7..be62f499c595 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rs-fw.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rs-fw.c
@@ -441,7 +441,8 @@ void rs_fw_rate_init(struct iwl_mvm *mvm, struct ieee80211_sta *sta,
*/
sta->max_amsdu_len = max_amsdu_len;
- ret = iwl_mvm_send_cmd_pdu(mvm, cmd_id, 0, sizeof(cfg_cmd), &cfg_cmd);
+ ret = iwl_mvm_send_cmd_pdu(mvm, cmd_id, CMD_ASYNC, sizeof(cfg_cmd),
+ &cfg_cmd);
if (ret)
IWL_ERR(mvm, "Failed to send rate scale config (%d)\n", ret);
}
--
2.20.1
^ permalink raw reply related
* [PATCH 5/7] iwlwifi: fix AX201 killer sku loading firmware issue
From: Luca Coelho @ 2019-05-29 13:39 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Matt Chen, Luca Coelho
In-Reply-To: <20190529133955.31082-1-luca@coelho.fi>
From: Matt Chen <matt.chen@intel.com>
When try to bring up the AX201 2 killer sku, we
run into:
[81261.392463] iwlwifi 0000:01:00.0: loaded firmware version 46.8c20f243.0 op_mode iwlmvm
[81261.407407] iwlwifi 0000:01:00.0: Detected Intel(R) Dual Band Wireless AX 22000, REV=0x340
[81262.424778] iwlwifi 0000:01:00.0: Collecting data: trigger 16 fired.
[81262.673359] iwlwifi 0000:01:00.0: Start IWL Error Log Dump:
[81262.673365] iwlwifi 0000:01:00.0: Status: 0x00000000, count: -906373681
[81262.673368] iwlwifi 0000:01:00.0: Loaded firmware version: 46.8c20f243.0
[81262.673371] iwlwifi 0000:01:00.0: 0x507C015D | ADVANCED_SYSASSERT
Fix this issue by adding 2 more cfg to avoid modifying the
original cfg configuration.
Signed-off-by: Matt Chen <matt.chen@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/pcie/trans.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index 21da18af0155..dfa1bed124aa 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -3598,7 +3598,9 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev,
}
} else if (CSR_HW_RF_ID_TYPE_CHIP_ID(trans->hw_rf_id) ==
CSR_HW_RF_ID_TYPE_CHIP_ID(CSR_HW_RF_ID_TYPE_HR) &&
- (trans->cfg != &iwl_ax200_cfg_cc ||
+ ((trans->cfg != &iwl_ax200_cfg_cc &&
+ trans->cfg != &killer1650x_2ax_cfg &&
+ trans->cfg != &killer1650w_2ax_cfg) ||
trans->hw_rev == CSR_HW_REV_TYPE_QNJ_B0)) {
u32 hw_status;
--
2.20.1
^ permalink raw reply related
* [PATCH 3/7] iwlwifi: clear persistence bit according to device family
From: Luca Coelho @ 2019-05-29 13:39 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Shahar S Matityahu, Luca Coelho
In-Reply-To: <20190529133955.31082-1-luca@coelho.fi>
From: Shahar S Matityahu <shahar.s.matityahu@intel.com>
The driver attempts to clear persistence bit on any device familiy even
though only 9000 and 22000 families require it. Clear the bit only on
the relevant device families.
Each HW has different address to the write protection register. Use the
right register for each HW
Signed-off-by: Shahar S Matityahu <shahar.s.matityahu@intel.com>
Fixes: 8954e1eb2270 ("iwlwifi: trans: Clear persistence bit when starting the FW")
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/iwl-prph.h | 7 ++-
.../net/wireless/intel/iwlwifi/pcie/trans.c | 46 +++++++++++++------
2 files changed, 39 insertions(+), 14 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-prph.h b/drivers/net/wireless/intel/iwlwifi/iwl-prph.h
index 8e6a0c363c0d..925f308764bf 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-prph.h
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-prph.h
@@ -408,7 +408,12 @@ enum aux_misc_master1_en {
#define AUX_MISC_MASTER1_SMPHR_STATUS 0xA20800
#define RSA_ENABLE 0xA24B08
#define PREG_AUX_BUS_WPROT_0 0xA04CC0
-#define PREG_PRPH_WPROT_0 0xA04CE0
+
+/* device family 9000 WPROT register */
+#define PREG_PRPH_WPROT_9000 0xA04CE0
+/* device family 22000 WPROT register */
+#define PREG_PRPH_WPROT_22000 0xA04D00
+
#define SB_CPU_1_STATUS 0xA01E30
#define SB_CPU_2_STATUS 0xA01E34
#define UMAG_SB_CPU_1_STATUS 0xA038C0
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index 803fcbac4152..e9d1075d91db 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -1698,26 +1698,26 @@ static int iwl_pcie_init_msix_handler(struct pci_dev *pdev,
return 0;
}
-static int _iwl_trans_pcie_start_hw(struct iwl_trans *trans, bool low_power)
+static int iwl_trans_pcie_clear_persistence_bit(struct iwl_trans *trans)
{
- struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans);
- u32 hpm;
- int err;
-
- lockdep_assert_held(&trans_pcie->mutex);
+ u32 hpm, wprot;
- err = iwl_pcie_prepare_card_hw(trans);
- if (err) {
- IWL_ERR(trans, "Error while preparing HW: %d\n", err);
- return err;
+ switch (trans->cfg->device_family) {
+ case IWL_DEVICE_FAMILY_9000:
+ wprot = PREG_PRPH_WPROT_9000;
+ break;
+ case IWL_DEVICE_FAMILY_22000:
+ wprot = PREG_PRPH_WPROT_22000;
+ break;
+ default:
+ return 0;
}
hpm = iwl_read_umac_prph_no_grab(trans, HPM_DEBUG);
if (hpm != 0xa5a5a5a0 && (hpm & PERSISTENCE_BIT)) {
- int wfpm_val = iwl_read_umac_prph_no_grab(trans,
- PREG_PRPH_WPROT_0);
+ u32 wprot_val = iwl_read_umac_prph_no_grab(trans, wprot);
- if (wfpm_val & PREG_WFPM_ACCESS) {
+ if (wprot_val & PREG_WFPM_ACCESS) {
IWL_ERR(trans,
"Error, can not clear persistence bit\n");
return -EPERM;
@@ -1726,6 +1726,26 @@ static int _iwl_trans_pcie_start_hw(struct iwl_trans *trans, bool low_power)
hpm & ~PERSISTENCE_BIT);
}
+ return 0;
+}
+
+static int _iwl_trans_pcie_start_hw(struct iwl_trans *trans, bool low_power)
+{
+ struct iwl_trans_pcie *trans_pcie = IWL_TRANS_GET_PCIE_TRANS(trans);
+ int err;
+
+ lockdep_assert_held(&trans_pcie->mutex);
+
+ err = iwl_pcie_prepare_card_hw(trans);
+ if (err) {
+ IWL_ERR(trans, "Error while preparing HW: %d\n", err);
+ return err;
+ }
+
+ err = iwl_trans_pcie_clear_persistence_bit(trans);
+ if (err)
+ return err;
+
iwl_trans_pcie_sw_reset(trans);
err = iwl_pcie_apm_init(trans);
--
2.20.1
^ permalink raw reply related
* [PATCH 4/7] iwlwifi: print fseq info upon fw assert
From: Luca Coelho @ 2019-05-29 13:39 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Shahar S Matityahu, Luca Coelho
In-Reply-To: <20190529133955.31082-1-luca@coelho.fi>
From: Shahar S Matityahu <shahar.s.matityahu@intel.com>
Read fseq info from FW registers and print it upon fw assert.
The print is needed since the fseq version coming from the TLV might
not be the actual version that is used.
Signed-off-by: Shahar S Matityahu <shahar.s.matityahu@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/fw/dbg.c | 39 +++++++++++++++++++
drivers/net/wireless/intel/iwlwifi/fw/dbg.h | 2 +
drivers/net/wireless/intel/iwlwifi/iwl-prph.h | 15 ++++++-
.../net/wireless/intel/iwlwifi/mvm/utils.c | 2 +
.../net/wireless/intel/iwlwifi/pcie/trans.c | 3 +-
5 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
index 5f52e40a2903..33d7bc5500db 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
+++ b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
@@ -2747,3 +2747,42 @@ void iwl_fw_dbg_periodic_trig_handler(struct timer_list *t)
jiffies + msecs_to_jiffies(collect_interval));
}
}
+
+#define FSEQ_REG(x) { .addr = (x), .str = #x, }
+
+void iwl_fw_error_print_fseq_regs(struct iwl_fw_runtime *fwrt)
+{
+ struct iwl_trans *trans = fwrt->trans;
+ unsigned long flags;
+ int i;
+ struct {
+ u32 addr;
+ const char *str;
+ } fseq_regs[] = {
+ FSEQ_REG(FSEQ_ERROR_CODE),
+ FSEQ_REG(FSEQ_TOP_INIT_VERSION),
+ FSEQ_REG(FSEQ_CNVIO_INIT_VERSION),
+ FSEQ_REG(FSEQ_OTP_VERSION),
+ FSEQ_REG(FSEQ_TOP_CONTENT_VERSION),
+ FSEQ_REG(FSEQ_ALIVE_TOKEN),
+ FSEQ_REG(FSEQ_CNVI_ID),
+ FSEQ_REG(FSEQ_CNVR_ID),
+ FSEQ_REG(CNVI_AUX_MISC_CHIP),
+ FSEQ_REG(CNVR_AUX_MISC_CHIP),
+ FSEQ_REG(CNVR_SCU_SD_REGS_SD_REG_DIG_DCDC_VTRIM),
+ FSEQ_REG(CNVR_SCU_SD_REGS_SD_REG_ACTIVE_VDIG_MIRROR),
+ };
+
+ if (!iwl_trans_grab_nic_access(trans, &flags))
+ return;
+
+ IWL_ERR(fwrt, "Fseq Registers:\n");
+
+ for (i = 0; i < ARRAY_SIZE(fseq_regs); i++)
+ IWL_ERR(fwrt, "0x%08X | %s\n",
+ iwl_read_prph_no_grab(trans, fseq_regs[i].addr),
+ fseq_regs[i].str);
+
+ iwl_trans_release_nic_access(trans, &flags);
+}
+IWL_EXPORT_SYMBOL(iwl_fw_error_print_fseq_regs);
diff --git a/drivers/net/wireless/intel/iwlwifi/fw/dbg.h b/drivers/net/wireless/intel/iwlwifi/fw/dbg.h
index 2a9e560a906b..fd0ad220e961 100644
--- a/drivers/net/wireless/intel/iwlwifi/fw/dbg.h
+++ b/drivers/net/wireless/intel/iwlwifi/fw/dbg.h
@@ -471,4 +471,6 @@ static inline void iwl_fw_error_collect(struct iwl_fw_runtime *fwrt)
}
void iwl_fw_dbg_periodic_trig_handler(struct timer_list *t);
+
+void iwl_fw_error_print_fseq_regs(struct iwl_fw_runtime *fwrt);
#endif /* __iwl_fw_dbg_h__ */
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-prph.h b/drivers/net/wireless/intel/iwlwifi/iwl-prph.h
index 925f308764bf..8d930bfe0727 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-prph.h
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-prph.h
@@ -395,7 +395,11 @@ enum {
WFPM_AUX_CTL_AUX_IF_MAC_OWNER_MSK = 0x80000000,
};
-#define AUX_MISC_REG 0xA200B0
+#define CNVI_AUX_MISC_CHIP 0xA200B0
+#define CNVR_AUX_MISC_CHIP 0xA2B800
+#define CNVR_SCU_SD_REGS_SD_REG_DIG_DCDC_VTRIM 0xA29890
+#define CNVR_SCU_SD_REGS_SD_REG_ACTIVE_VDIG_MIRROR 0xA29938
+
enum {
HW_STEP_LOCATION_BITS = 24,
};
@@ -447,4 +451,13 @@ enum {
#define UREG_DOORBELL_TO_ISR6 0xA05C04
#define UREG_DOORBELL_TO_ISR6_NMI_BIT BIT(0)
+
+#define FSEQ_ERROR_CODE 0xA340C8
+#define FSEQ_TOP_INIT_VERSION 0xA34038
+#define FSEQ_CNVIO_INIT_VERSION 0xA3403C
+#define FSEQ_OTP_VERSION 0xA340FC
+#define FSEQ_TOP_CONTENT_VERSION 0xA340F4
+#define FSEQ_ALIVE_TOKEN 0xA340F0
+#define FSEQ_CNVI_ID 0xA3408C
+#define FSEQ_CNVR_ID 0xA34090
#endif /* __iwl_prph_h__ */
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/utils.c b/drivers/net/wireless/intel/iwlwifi/mvm/utils.c
index b9914efc55c4..cc56ab88fb43 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/utils.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/utils.c
@@ -596,6 +596,8 @@ void iwl_mvm_dump_nic_error_log(struct iwl_mvm *mvm)
iwl_mvm_dump_lmac_error_log(mvm, 1);
iwl_mvm_dump_umac_error_log(mvm);
+
+ iwl_fw_error_print_fseq_regs(&mvm->fwrt);
}
int iwl_mvm_reconfig_scd(struct iwl_mvm *mvm, int queue, int fifo, int sta_id,
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
index e9d1075d91db..21da18af0155 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c
@@ -3546,7 +3546,8 @@ struct iwl_trans *iwl_trans_pcie_alloc(struct pci_dev *pdev,
hw_step |= ENABLE_WFPM;
iwl_write_umac_prph_no_grab(trans, WFPM_CTRL_REG,
hw_step);
- hw_step = iwl_read_prph_no_grab(trans, AUX_MISC_REG);
+ hw_step = iwl_read_prph_no_grab(trans,
+ CNVI_AUX_MISC_CHIP);
hw_step = (hw_step >> HW_STEP_LOCATION_BITS) & 0xF;
if (hw_step == 0x3)
trans->hw_rev = (trans->hw_rev & 0xFFFFFFF3) |
--
2.20.1
^ permalink raw reply related
* [PATCH 2/7] iwlwifi: fix load in rfkill flow for unified firmware
From: Luca Coelho @ 2019-05-29 13:39 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Emmanuel Grumbach, Luca Coelho
In-Reply-To: <20190529133955.31082-1-luca@coelho.fi>
From: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
When we have a single image (same firmware image for INIT and
OPERATIONAL), we couldn't load the driver and register to the
stack if we had hardware RF-Kill asserted.
Fix this. This required a few changes:
1) Run the firmware as part of the INIT phase even if its
ucode_type is not IWL_UCODE_INIT.
2) Send the commands that are sent to the unified image in
INIT flow even in RF-Kill.
3) Don't ask the transport to stop the hardware upon RF-Kill
interrupt if the RF-Kill is asserted.
4) Allow the RF-Kill interrupt to take us out of L1A so that
the RF-Kill interrupt will be received by the host (to
enable the radio).
Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 23 ++++++++++++++-----
.../net/wireless/intel/iwlwifi/mvm/mac80211.c | 2 +-
drivers/net/wireless/intel/iwlwifi/mvm/mvm.h | 2 +-
drivers/net/wireless/intel/iwlwifi/mvm/ops.c | 17 ++++++++++----
.../wireless/intel/iwlwifi/pcie/internal.h | 2 +-
5 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
index ab68b5d53ec9..153717587aeb 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c
@@ -311,6 +311,8 @@ static int iwl_mvm_load_ucode_wait_alive(struct iwl_mvm *mvm,
int ret;
enum iwl_ucode_type old_type = mvm->fwrt.cur_fw_img;
static const u16 alive_cmd[] = { MVM_ALIVE };
+ bool run_in_rfkill =
+ ucode_type == IWL_UCODE_INIT || iwl_mvm_has_unified_ucode(mvm);
if (ucode_type == IWL_UCODE_REGULAR &&
iwl_fw_dbg_conf_usniffer(mvm->fw, FW_DBG_START_FROM_ALIVE) &&
@@ -328,7 +330,12 @@ static int iwl_mvm_load_ucode_wait_alive(struct iwl_mvm *mvm,
alive_cmd, ARRAY_SIZE(alive_cmd),
iwl_alive_fn, &alive_data);
- ret = iwl_trans_start_fw(mvm->trans, fw, ucode_type == IWL_UCODE_INIT);
+ /*
+ * We want to load the INIT firmware even in RFKILL
+ * For the unified firmware case, the ucode_type is not
+ * INIT, but we still need to run it.
+ */
+ ret = iwl_trans_start_fw(mvm->trans, fw, run_in_rfkill);
if (ret) {
iwl_fw_set_current_image(&mvm->fwrt, old_type);
iwl_remove_notification(&mvm->notif_wait, &alive_wait);
@@ -433,7 +440,8 @@ static int iwl_run_unified_mvm_ucode(struct iwl_mvm *mvm, bool read_nvm)
* commands
*/
ret = iwl_mvm_send_cmd_pdu(mvm, WIDE_ID(SYSTEM_GROUP,
- INIT_EXTENDED_CFG_CMD), 0,
+ INIT_EXTENDED_CFG_CMD),
+ CMD_SEND_IN_RFKILL,
sizeof(init_cfg), &init_cfg);
if (ret) {
IWL_ERR(mvm, "Failed to run init config command: %d\n",
@@ -457,7 +465,8 @@ static int iwl_run_unified_mvm_ucode(struct iwl_mvm *mvm, bool read_nvm)
}
ret = iwl_mvm_send_cmd_pdu(mvm, WIDE_ID(REGULATORY_AND_NVM_GROUP,
- NVM_ACCESS_COMPLETE), 0,
+ NVM_ACCESS_COMPLETE),
+ CMD_SEND_IN_RFKILL,
sizeof(nvm_complete), &nvm_complete);
if (ret) {
IWL_ERR(mvm, "Failed to run complete NVM access: %d\n",
@@ -482,6 +491,8 @@ static int iwl_run_unified_mvm_ucode(struct iwl_mvm *mvm, bool read_nvm)
}
}
+ mvm->rfkill_safe_init_done = true;
+
return 0;
error:
@@ -526,7 +537,7 @@ int iwl_run_init_mvm_ucode(struct iwl_mvm *mvm, bool read_nvm)
lockdep_assert_held(&mvm->mutex);
- if (WARN_ON_ONCE(mvm->calibrating))
+ if (WARN_ON_ONCE(mvm->rfkill_safe_init_done))
return 0;
iwl_init_notification_wait(&mvm->notif_wait,
@@ -576,7 +587,7 @@ int iwl_run_init_mvm_ucode(struct iwl_mvm *mvm, bool read_nvm)
goto remove_notif;
}
- mvm->calibrating = true;
+ mvm->rfkill_safe_init_done = true;
/* Send TX valid antennas before triggering calibrations */
ret = iwl_send_tx_ant_cfg(mvm, iwl_mvm_get_valid_tx_ant(mvm));
@@ -612,7 +623,7 @@ int iwl_run_init_mvm_ucode(struct iwl_mvm *mvm, bool read_nvm)
remove_notif:
iwl_remove_notification(&mvm->notif_wait, &calib_wait);
out:
- mvm->calibrating = false;
+ mvm->rfkill_safe_init_done = false;
if (iwlmvm_mod_params.init_dbg && !mvm->nvm_data) {
/* we want to debug INIT and we have no NVM - fake */
mvm->nvm_data = kzalloc(sizeof(struct iwl_nvm_data) +
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
index 5c52469288be..fdbabca0280e 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
@@ -1209,7 +1209,7 @@ static void iwl_mvm_restart_cleanup(struct iwl_mvm *mvm)
mvm->scan_status = 0;
mvm->ps_disabled = false;
- mvm->calibrating = false;
+ mvm->rfkill_safe_init_done = false;
/* just in case one was running */
iwl_mvm_cleanup_roc_te(mvm);
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
index 7b829a5be773..02efcf2189c4 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
@@ -880,7 +880,7 @@ struct iwl_mvm {
struct iwl_mvm_vif *bf_allowed_vif;
bool hw_registered;
- bool calibrating;
+ bool rfkill_safe_init_done;
bool support_umac_log;
u32 ampdu_ref;
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/ops.c b/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
index 004de67f9157..fad3bf563712 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/ops.c
@@ -1209,7 +1209,8 @@ void iwl_mvm_set_hw_ctkill_state(struct iwl_mvm *mvm, bool state)
static bool iwl_mvm_set_hw_rfkill_state(struct iwl_op_mode *op_mode, bool state)
{
struct iwl_mvm *mvm = IWL_OP_MODE_GET_MVM(op_mode);
- bool calibrating = READ_ONCE(mvm->calibrating);
+ bool rfkill_safe_init_done = READ_ONCE(mvm->rfkill_safe_init_done);
+ bool unified = iwl_mvm_has_unified_ucode(mvm);
if (state)
set_bit(IWL_MVM_STATUS_HW_RFKILL, &mvm->status);
@@ -1218,15 +1219,23 @@ static bool iwl_mvm_set_hw_rfkill_state(struct iwl_op_mode *op_mode, bool state)
iwl_mvm_set_rfkill_state(mvm);
- /* iwl_run_init_mvm_ucode is waiting for results, abort it */
- if (calibrating)
+ /* iwl_run_init_mvm_ucode is waiting for results, abort it. */
+ if (rfkill_safe_init_done)
iwl_abort_notification_waits(&mvm->notif_wait);
+ /*
+ * Don't ask the transport to stop the firmware. We'll do it
+ * after cfg80211 takes us down.
+ */
+ if (unified)
+ return false;
+
/*
* Stop the device if we run OPERATIONAL firmware or if we are in the
* middle of the calibrations.
*/
- return state && (mvm->fwrt.cur_fw_img != IWL_UCODE_INIT || calibrating);
+ return state && (mvm->fwrt.cur_fw_img != IWL_UCODE_INIT ||
+ rfkill_safe_init_done);
}
static void iwl_mvm_free_skb(struct iwl_op_mode *op_mode, struct sk_buff *skb)
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
index b513037dc066..85973dd57234 100644
--- a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
+++ b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
@@ -928,7 +928,7 @@ static inline void iwl_enable_rfkill_int(struct iwl_trans *trans)
MSIX_HW_INT_CAUSES_REG_RF_KILL);
}
- if (trans->cfg->device_family == IWL_DEVICE_FAMILY_9000) {
+ if (trans->cfg->device_family >= IWL_DEVICE_FAMILY_9000) {
/*
* On 9000-series devices this bit isn't enabled by default, so
* when we power down the device we need set the bit to allow it
--
2.20.1
^ permalink raw reply related
* [PATCH 0/2] Buffer overflow / read checks in mwifiex
From: Takashi Iwai @ 2019-05-29 12:52 UTC (permalink / raw)
To: linux-wireless
Cc: Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat, Xinming Hu,
Kalle Valo, huangwen, Solar Designer, Marcus Meissner
Hi,
this is fixes for a few spots that perform memcpy() without checking
the source and the destination size in mwifiex driver, which may lead
to buffer overflows or read over boundary.
Takashi
===
Takashi Iwai (2):
mwifiex: Fix possible buffer overflows at parsing bss descriptor
mwifiex: Abort at too short BSS descriptor element
drivers/net/wireless/marvell/mwifiex/scan.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
--
2.16.4
^ permalink raw reply
* [PATCH 1/2] mwifiex: Fix possible buffer overflows at parsing bss descriptor
From: Takashi Iwai @ 2019-05-29 12:52 UTC (permalink / raw)
To: linux-wireless
Cc: Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat, Xinming Hu,
Kalle Valo, huangwen, Solar Designer, Marcus Meissner
In-Reply-To: <20190529125220.17066-1-tiwai@suse.de>
mwifiex_update_bss_desc_with_ie() calls memcpy() unconditionally in
a couple places without checking the destination size. Since the
source is given from user-space, this may trigger a heap buffer
overflow.
Fix it by putting the length check before performing memcpy().
This fix addresses CVE-2019-3846.
Reported-by: huangwen <huangwen@venustech.com.cn>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
drivers/net/wireless/marvell/mwifiex/scan.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
index 935778ec9a1b..64ab6fe78c0d 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -1247,6 +1247,8 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
}
switch (element_id) {
case WLAN_EID_SSID:
+ if (element_len > IEEE80211_MAX_SSID_LEN)
+ return -EINVAL;
bss_entry->ssid.ssid_len = element_len;
memcpy(bss_entry->ssid.ssid, (current_ptr + 2),
element_len);
@@ -1256,6 +1258,8 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
break;
case WLAN_EID_SUPP_RATES:
+ if (element_len > MWIFIEX_SUPPORTED_RATES)
+ return -EINVAL;
memcpy(bss_entry->data_rates, current_ptr + 2,
element_len);
memcpy(bss_entry->supported_rates, current_ptr + 2,
--
2.16.4
^ 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