* Re: [PATCH 0/9] rsi: code clean-up
From: Joe Perches @ 2016-09-12 18:50 UTC (permalink / raw)
To: Kalle Valo, Prameela Rani Garnepudi
Cc: linux-wireless, johannes.berg, hofrat, prameela.garnepudi
In-Reply-To: <87sht5mg86.fsf@kamboji.qca.qualcomm.com>
On Mon, 2016-09-12 at 15:53 +0300, Kalle Valo wrote:
> Also try to get more familiar how development is done, here are few
> links:
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> https://www.kernel.org/doc/Documentation/SubmittingPatches
> https://kernelnewbies.org/FirstKernelPatch
>
> Also mailing list archives and git logs are a great way to learn from
> how others do it.
And only working on drivers/staging files is a good way to start.
^ permalink raw reply
* Re: [PATCH 00/26] constify local structures
From: Jarkko Sakkinen @ 2016-09-12 18:50 UTC (permalink / raw)
To: Julia Lawall
Cc: Felipe Balbi, linux-renesas-soc, joe, kernel-janitors,
Sergei Shtylyov, linux-pm, platform-driver-x86, linux-media,
linux-can, Tatyana Nikolova, Shiraz Saleem, Mustafa Ismail,
Chien Tin Tung, linux-rdma, netdev, devel, alsa-devel,
linux-kernel, linux-fbdev, linux-wireless, Jason Gunthorpe,
tpmdd-devel, linux-scsi, linux-spi, linux-usb, linux-acpi
In-Reply-To: <alpine.DEB.2.10.1609121548200.29099@hadrien>
On Mon, Sep 12, 2016 at 03:52:08PM +0200, Julia Lawall wrote:
>
>
> On Mon, 12 Sep 2016, Felipe Balbi wrote:
>
> >
> > Hi,
> >
> > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> writes:
> > > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> > >>
> > >>
> > >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> > >>
> > >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > >> > > Constify local structures.
> > >> > >
> > >> > > The semantic patch that makes this change is as follows:
> > >> > > (http://coccinelle.lip6.fr/)
> > >> >
> > >> > Just my two cents but:
> > >> >
> > >> > 1. You *can* use a static analysis too to find bugs or other issues.
> > >> > 2. However, you should manually do the commits and proper commit
> > >> > messages to subsystems based on your findings. And I generally think
> > >> > that if one contributes code one should also at least smoke test changes
> > >> > somehow.
> > >> >
> > >> > I don't know if I'm alone with my opinion. I just think that one should
> > >> > also do the analysis part and not blindly create and submit patches.
> > >>
> > >> All of the patches are compile tested. And the individual patches are
> > >
> > > Compile-testing is not testing. If you are not able to test a commit,
> > > you should explain why.
> >
> > Dude, Julia has been doing semantic patching for years already and
> > nobody has raised any concerns so far. There's already an expectation
> > that Coccinelle *works* and Julia's sematic patches are sound.
> >
> > Besides, adding 'const' is something that causes virtually no functional
> > changes to the point that build-testing is really all you need. Any
> > problems caused by adding 'const' to a definition will be seen by build
> > errors or warnings.
> >
> > Really, just stop with the pointless discussion and go read a bit about
> > Coccinelle and what semantic patches are giving you. The work done by
> > Julia and her peers are INRIA have measurable benefits.
> >
> > You're really making a thunderstorm in a glass of water.
>
> Thanks for the defense, but since a lot of these patches torned out to be
> wrong, due to an incorrect parse by Coccinelle, combined with an
> unpleasantly lax compiler, Jarkko does have a point that I should have
> looked at the patches more carefully. In any case, I have written to the
> maintainers relevant to the patches that turned out to be incorrect.
Exactly. I'm not excepting that every commit would require extensive
analysis but it would be good to quickly at least skim through commits
and see if they make sense (or ask if unsure) :)
And I'm fine with compile testing if it is mentioned in the commit msg.
> julia
/Jarkko
^ permalink raw reply
* Re: [PATCH] mac80211: allow driver to handle packet-loss mechanism
From: Rajkumar Manoharan @ 2016-09-12 17:57 UTC (permalink / raw)
To: Johannes Berg; +Cc: Rajkumar Manoharan, linux-wireless
In-Reply-To: <1473684288.29016.38.camel@sipsolutions.net>
On 2016-09-12 18:14, Johannes Berg wrote:
> On Tue, 2016-09-06 at 12:26 +0530, Rajkumar Manoharan wrote:
>> mac80211 keeps track of missing acks and triggers CQM packet-loss
>> mechanism whenever consecutive msdu failure reaches threshold limit
>> (STA_LOST_PKT_THRESHOLD). Drivers like ath10k offlaoded rate countrol
>> and aggregation to firmware. Such drivers have its own connection
>> monitoring algorithm that is offloaded to firmware for triggering
>> station kickout due to excessive tries. In VHT mode, single PPDU can
>> have
>> more than 50 msdus at higher rates. Under noisy environment, single
>> ppdu
>> failure can cause station kickout by current mac80211 lost_packet
>> mechanism
>> while firmware is trying to adapt its rate table. This is causing
>> frequent
>> connect and disconnect iteration when station is roaming around.
>>
>> In such scenario, driver (or firmware) is not given enough chance to
>> tune its rate control. So for devices that report low ack events, add
>> a
>> hardware flag to rely on their mechnism.
>>
>
> The way you describe this it sounds like somehow you'll be reporting
> the indication to userspace from the driver; but you do not, and cannot
> do that. The description seems thus misleading?
>
Sorry for confusion.. The idea is that driver will report low ack status
by ieee80211_report_low_ack and if driver wants to completely rely on
firmware algorithm, then this flag will be used to bypass mac80211
packet loss mechanism. does it make sense?
-Rajkumar
^ permalink raw reply
* Re: [PATCH 2/3] ath10k: Grab rcu_read_lock before the txqs spinlock.
From: Ben Greear @ 2016-09-12 16:37 UTC (permalink / raw)
To: Johannes Berg, Valo, Kalle
Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <1473662509.4201.0.camel@sipsolutions.net>
On 09/11/2016 11:41 PM, Johannes Berg wrote:
>
>>>> - rcu_read_unlock();
>>>> spin_unlock_bh(&ar->txqs_lock);
>>>> + rcu_read_unlock();
>>>
>>> I'm no RCU expert but this isn't making any sense. Maybe it changes
>>> timings on your kernel so that it hides the real problem?
>>
>> I'm not sure this fixed anything or not, it just seemed weird so I
>> changed it.
>>
>> I was hoping someone that understood rcu locking would comment...
>>
>
> RCU is no "locking". The sooner you get over that notion, the better.
>
> This therefore make no sense whatsoever.
>
> In fact, you want to keep the RCU protected section *small*, so having
> the spinlock inside hurts overall system performance.
Ok, thanks for the review. I'll drop this patch from my tree.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH] cfg80211: cap 20MHz VHT bitrate at MCS 8
From: Ben Greear @ 2016-09-12 16:36 UTC (permalink / raw)
To: Johannes Berg, Pedersen, Thomas, linux-wireless
In-Reply-To: <1473662637.4201.2.camel@sipsolutions.net>
On 09/11/2016 11:43 PM, Johannes Berg wrote:
> On Wed, 2016-09-07 at 18:20 +0000, Pedersen, Thomas wrote:
>> On 09/06/2016 12:07 PM, Ben Greear wrote:
>>>
>>> On 09/06/2016 12:00 PM, Thomas Pedersen wrote:
>>>>
>>>> Some drivers (ath10k) report MCS 9 @ 20MHz, which
>>>> technically isn't allowed. To get more meaningful value
>>>> than 0 out of this however, just cap the bitrate for 20MHz
>>>> to MCS 8.
>>>
>>> If it is actually reporting MCS9, why lie about it? Report it up
>>> the stack as a proper value instead of hiding the issue?
>>
>> Good point, will send a v2 extrapolating the value to 86.5Mb/s.
>
> That makes no sense either, IMHO.
>
> Are you saying that ath10k actually somehow manages to use an invalid
> bitrate over the air?!
>
> It seems more likely that it's actually just misreporting what it's
> doing, and thus the issue should be fixed in ath10k.
I saw ath10k report the same value in my testing.
Hard to know what the actual on-air encoding rate is.
I do know that some ath10k firmware would run 1Mbps encoding
management frames on 5Mhz, which is also not per spec, for what that is worth.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: Support of rtl8723bs
From: Jes Sorensen @ 2016-09-12 16:07 UTC (permalink / raw)
To: Bastien Nocera; +Cc: Hanno Zulla, Larry Finger, anthony.wong, linux-wireless
In-Reply-To: <1473695458.6389.3.camel@hadess.net>
Bastien Nocera <hadess@hadess.net> writes:
> On Mon, 2016-09-12 at 17:48 +0200, Hanno Zulla wrote:
>> Hi,
>>
>> > Longer term I think it makes sense to add SDIO support to rtl8xxxu.
>> > The
>> > differences between the USB version and the SDIO version are rather
>> > small.
>>
>>
>> This is beyond my expertise, sadly.
>>
>> Is there a good blueprint / example of a previous case where a USB
>> driver had SDIO support added that one might learn from?
>
> I don't know on top of my head, sorry. Best look in the kernel sources
> directly.
The issues I am aware of is to start out changing the register access
macros into function pointers and stick them all into the fileops
structure. Provide a set of SDIO ones to match the USB ones. Then you
will need some code to detect the device, as that part will obviously be
different from the USB device detection.
I know there are a few issues to look out for in the hw register init
files to look out for too, that does some special casing for SDIO, but
that should be limited. I am happy to point someone in the right
direction on that when they get to that.
Cheers,
Jes
^ permalink raw reply
* Re: Support of rtl8723bs
From: Bastien Nocera @ 2016-09-12 15:50 UTC (permalink / raw)
To: Hanno Zulla, Larry Finger, Jes Sorensen; +Cc: anthony.wong, linux-wireless
In-Reply-To: <cd991918-0bfa-a9a3-15e5-aa8561db2c65@hanno.de>
On Mon, 2016-09-12 at 17:48 +0200, Hanno Zulla wrote:
> Hi,
>
> > Longer term I think it makes sense to add SDIO support to rtl8xxxu.
> > The
> > differences between the USB version and the SDIO version are rather
> > small.
>
>
> This is beyond my expertise, sadly.
>
> Is there a good blueprint / example of a previous case where a USB
> driver had SDIO support added that one might learn from?
I don't know on top of my head, sorry. Best look in the kernel sources
directly.
^ permalink raw reply
* Re: Support of rtl8723bs
From: Hanno Zulla @ 2016-09-12 15:48 UTC (permalink / raw)
To: Larry Finger, Jes Sorensen, Bastien Nocera; +Cc: anthony.wong, linux-wireless
In-Reply-To: <5b920339-abcd-0fc4-f054-039155c531a0@lwfinger.net>
Hi,
> Longer term I think it makes sense to add SDIO support to rtl8xxxu. The
> differences between the USB version and the SDIO version are rather
> small.
This is beyond my expertise, sadly.
Is there a good blueprint / example of a previous case where a USB
driver had SDIO support added that one might learn from?
Kind regards,
Hanno
^ permalink raw reply
* Re: [PATCH 2/2] ath10k: add platform regulatory domain support
From: Valo, Kalle @ 2016-09-12 15:35 UTC (permalink / raw)
To: bartosz.markowski@tieto.com
Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <1470140454-3899-3-git-send-email-bartosz.markowski@tieto.com>
Bartosz Markowski <bartosz.markowski@tieto.com> writes:
> This overrides whatever regulatory the device
> EEPROM contains and uses what the platform says
> instead - in this implementation the ACPI driver.
>
> In case the hint is not programmed or corrupted (0xffff)
> the device falls back to the eeprom programmed settings.
>
> Signed-off-by: Bartosz Markowski <bartosz.markowski@tieto.com>
[...]
> +#ifdef CONFIG_ACPI
> +#define WRD_METHOD "WRDD"
> +#define WRDD_WIFI (0x07)
> +
> +static u32 ath10k_mac_wrdd_get_mcc(struct ath10k *ar, union acpi_object =
*wrdd)
> +{
I don't think the ifdef is really necessary, acpi.h should handle that
(hopefully). Also I changed the error handling to use standard error
values and changed the info messages to dbg, they are too spammy in my
opinion. Please check carefully my changes in the pending branch:
https://git.kernel.org/cgit/linux/kernel/git/kvalo/ath.git/commit/?h=3Dpend=
ing&id=3Dfe91745381ec3999d8de6dedb07b396c82539717
--=20
Kalle Valo=
^ permalink raw reply
* Re: [PATCH] ath10k: Fix rfc1042 header retrieval in QCA4019 with eth decap mode
From: Valo, Kalle @ 2016-09-12 15:31 UTC (permalink / raw)
To: Thiagarajan, Vasanthakumar
Cc: ath10k@lists.infradead.org, linux-wireless@vger.kernel.org
In-Reply-To: <1469623085-13942-1-git-send-email-vthiagar@qti.qualcomm.com>
Vasanthakumar Thiagarajan <vthiagar@qti.qualcomm.com> writes:
> Chipset from QCA99X0 onwards (QCA99X0, QCA9984, QCA4019 & future)
> rx_hdr_status is not padded to align in 4-byte boundary. Define a
> new hw_params field to handle different alignment behaviour between
> different hw. This patch fixes improper retrieval of rfc1042 header
> with QCA4019. This patch along with "ath10k: Properly remove padding
> from the start of rx payload" will fix traffic failure in ethernet
> decap mode for QCA4019.
>
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qti.qualcomm.com>
There were some conflicts, I fixed them in the pending branch. Please
check my changes:
https://git.kernel.org/cgit/linux/kernel/git/kvalo/ath.git/commit/?h=3Dpend=
ing&id=3Dd0d9e40216e1a8edd2f3b387b96aad62c3a49add
--=20
Kalle Valo=
^ permalink raw reply
* Re: Support of rtl8723bs
From: Larry Finger @ 2016-09-12 15:09 UTC (permalink / raw)
To: Jes Sorensen, Bastien Nocera; +Cc: Hanno Zulla, linux-wireless, anthony.wong
In-Reply-To: <wrfjtwdli39u.fsf@redhat.com>
On 09/12/2016 09:47 AM, Jes Sorensen wrote:
> Bastien Nocera <hadess@hadess.net> writes:
>> On Mon, 2016-09-12 at 11:20 +0200, Hanno Zulla wrote:
>>> Hi Jes,
>>> hello linux-wireless,
>>> it as a guinea pig to test on my hardware. The newer vendor driver
>>> crashed on Ubuntu' 4.4 sources and doesn't compile yet on Ubuntu 4.8-
>>> rc
>>> sources.
>>> https://github.com/anthonywong/rtl8723bs/issues/1
>>
>> I really don't care about the vendor driver. There's no way to track
>> changes other than through code dumps.
>>
>> Longer term, it seems likely that improving Jes' driver is the way to
>> go. At least there's 170k less lines of code to read in the rtl8723bs
>> driver to get things going.
>
> Longer term I think it makes sense to add SDIO support to rtl8xxxu. The
> differences between the USB version and the SDIO version are rather
> small.
>
> That said, I have quite a lot of things on my TODO list so it is going
> to take some time until I can look at this. It may make sense to pull
> a cleaned up version of the vendor driver into staging until then if
> there is a desperate need for it, but otherwise any help would be
> appreciated :)
I agree with Jes that adding support for SDIO in rtl8xxxu would be a useful
step. If anyone wants to help, that would be the place to start. I disagree that
this driver should be dumped into staging. If one reads the various issues
associated with the GitHub repo, you will see that this driver has severe
problems that are unlikely to be fixed with a cleanup. The latest report is that
the driver causes the loss of clock interrupts, which is not surprising given
the sorry state of locking in the driver.
Complicating matters is that no one outside Realtek ever sees the details of any
commit. At intervals, they make a dump of their current code and release it. The
differences between two such releases amounts to many thousands of lines, and
finding any changes due to bug fixes is a "needle in the haystack" problem.
Larry
^ permalink raw reply
* Re: Support of rtl8723bs
From: Jes Sorensen @ 2016-09-12 14:47 UTC (permalink / raw)
To: Bastien Nocera; +Cc: Hanno Zulla, linux-wireless, Larry.Finger, anthony.wong
In-Reply-To: <1473683031.2930.3.camel@hadess.net>
Bastien Nocera <hadess@hadess.net> writes:
> On Mon, 2016-09-12 at 11:20 +0200, Hanno Zulla wrote:
>> Hi Jes,
>> hello linux-wireless,
>> it as a guinea pig to test on my hardware. The newer vendor driver
>> crashed on Ubuntu' 4.4 sources and doesn't compile yet on Ubuntu 4.8-
>> rc
>> sources.
>> https://github.com/anthonywong/rtl8723bs/issues/1
>
> I really don't care about the vendor driver. There's no way to track
> changes other than through code dumps.
>
> Longer term, it seems likely that improving Jes' driver is the way to
> go. At least there's 170k less lines of code to read in the rtl8723bs
> driver to get things going.
Longer term I think it makes sense to add SDIO support to rtl8xxxu. The
differences between the USB version and the SDIO version are rather
small.
That said, I have quite a lot of things on my TODO list so it is going
to take some time until I can look at this. It may make sense to pull
a cleaned up version of the vendor driver into staging until then if
there is a desperate need for it, but otherwise any help would be
appreciated :)
Jes
^ permalink raw reply
* Re: Support of rtl8723bs
From: Anthony Wong @ 2016-09-12 14:27 UTC (permalink / raw)
To: Bastien Nocera; +Cc: Hanno Zulla, linux-wireless, Jes.Sorensen, Larry Finger
In-Reply-To: <1473683031.2930.3.camel@hadess.net>
On 12 September 2016 at 20:23, Bastien Nocera <hadess@hadess.net> wrote:
>
> On Mon, 2016-09-12 at 11:20 +0200, Hanno Zulla wrote:
> > Hi Jes,
> > hello linux-wireless,
> >
> > I'd like to know more about the current progress of rtl8723bs support
> > for Linux.
> >
> > Googling and reading the mailing list archives found an older vendor
> > driver
> > https://github.com/hadess/rtl8723bs
>
> This is the canonical location.
>
> > plus patches at
> > https://github.com/lwfinger/rtl8723bs
>
> This is an old tree.
>
> > which both aren't ready for Linux according to
> > https://github.com/hadess/rtl8723bs/issues/96
> >
> > According to
> > https://github.com/hadess/rtl8723bs/issues/98
> >
> > there is a newer vendor driver from Realtek at
> > https://github.com/anthonywong/rtl8723bs
> > which Anthony wants to get ready for inclusion to the Ubuntu kernel.
> >
> > But after reading
> > https://marc.info/?l=linux-wireless&m=146902477024130&w=2
> > it appears that even that newer vendor driver isn't ready for Jes's
> > work.
>
> The "hadess" driver above is an older vendor driver with 2 years of
> cleanups, and lots of code removal:
> 430 files changed, 81387 insertions(+), 254812 deletions(-)
>
> Anthony's repo is just a dump of a newer version of the driver.
Indeed, the code in my git repo is simply the driver source code
extracted from what was provided by the vendor. It's pushed to github
in the hope that it can help anyone who works on supporting this wifi
device and can serve as a reference for future development.
Cheers,
Anthony
>
>
> What would be useful would be to compare the newer vendor driver with
> the last time the "hadess" driver was rebased
> (a2e5bb366f6d6c19dcab6db47b68a944256e8228) and pick out bug fixes.
>
> Anyway, as an owner of such a device, I'd like to help somehow, and
> be
> > it as a guinea pig to test on my hardware. The newer vendor driver
> > crashed on Ubuntu' 4.4 sources and doesn't compile yet on Ubuntu 4.8-
> > rc
> > sources.
> > https://github.com/anthonywong/rtl8723bs/issues/1
>
> I really don't care about the vendor driver. There's no way to track
> changes other than through code dumps.
>
> Longer term, it seems likely that improving Jes' driver is the way to
> go. At least there's 170k less lines of code to read in the rtl8723bs
> driver to get things going.
>
> Cheers
^ permalink raw reply
* [PATCH] ath10k: Add missing CONFIG_ATH10K_DEBUGFS check
From: Benjamin Berg @ 2016-09-12 14:11 UTC (permalink / raw)
To: Valo, Kalle
Cc: ath10k @ lists . infradead . org, Simon Wunderlich,
Thiagarajan, Vasanthakumar, Sebastian Gottschall, michal.kazior,
Mathias Kretschmer, linux-wireless, Benjamin Berg
In-Reply-To: <871t0tp1ad.fsf@kamboji.qca.qualcomm.com>
The patch "ath10k: allow setting coverage class" was missing a check for
CONFIG_ATH10K_DEBUGFS so it would try to use non-existing struct elements
in some configurations. Fix this by adding the appropriate ifdef.
Signed-off-by: Benjamin Berg <benjamin@sipsolutions.net>
---
Sorry, so turns out the kbuild test robot is correct and I forgot the ifdef
to check for CONFIG_ATH10K_DEBUGFS, so here a fixup commit for it.
Other than that everything looks good to me in the pending branch.
Benjamin
drivers/net/wireless/ath/ath10k/hw.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/hw.c b/drivers/net/wireless/ath/ath10k/hw.c
index c52b5f4..e182f09 100644
--- a/drivers/net/wireless/ath/ath10k/hw.c
+++ b/drivers/net/wireless/ath/ath10k/hw.c
@@ -338,8 +338,13 @@ static void ath10k_hw_qca988x_set_coverage_class(struct ath10k *ar,
* set the registers again if the firmware does an internal reset and
* this way we will be notified of the event.
*/
+#ifdef CONFIG_ATH10K_DEBUGFS
fw_dbglog_mask = ar->debug.fw_dbglog_mask;
fw_dbglog_level = ar->debug.fw_dbglog_level;
+#else
+ fw_dbglog_mask = 0;
+ fw_dbglog_level = 0;
+#endif
if (value > 0) {
if (fw_dbglog_level > ATH10K_DBGLOG_LEVEL_WARN)
--
2.9.3
^ permalink raw reply related
* [PATCH 15/15] iwlegacy: use IS_ENABLED() instead of checking for built-in or module
From: Javier Martinez Canillas @ 2016-09-12 14:03 UTC (permalink / raw)
To: linux-kernel
Cc: Javier Martinez Canillas, linux-wireless, netdev, Kalle Valo,
Stanislaw Gruszka
In-Reply-To: <1473689026-6983-1-git-send-email-javier@osg.samsung.com>
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either
built-in or as a module, use that macro instead of open coding the same.
Using the macro makes the code more readable by helping abstract away some
of the Kconfig built-in and module enable details.
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
drivers/net/wireless/intel/iwlegacy/common.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/common.h b/drivers/net/wireless/intel/iwlegacy/common.h
index 726ede391cb9..3bba521d2cd9 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.h
+++ b/drivers/net/wireless/intel/iwlegacy/common.h
@@ -1320,7 +1320,7 @@ struct il_priv {
u64 timestamp;
union {
-#if defined(CONFIG_IWL3945) || defined(CONFIG_IWL3945_MODULE)
+#if IS_ENABLED(CONFIG_IWL3945)
struct {
void *shared_virt;
dma_addr_t shared_phys;
@@ -1351,7 +1351,7 @@ struct il_priv {
} _3945;
#endif
-#if defined(CONFIG_IWL4965) || defined(CONFIG_IWL4965_MODULE)
+#if IS_ENABLED(CONFIG_IWL4965)
struct {
struct il_rx_phy_res last_phy_res;
bool last_phy_res_valid;
--
2.7.4
^ permalink raw reply related
* [PATCH 00/15] drivers: net: use IS_ENABLED() instead of checking for built-in or module
From: Javier Martinez Canillas @ 2016-09-12 14:03 UTC (permalink / raw)
To: linux-kernel
Cc: Javier Martinez Canillas, Arnd Bergmann, Sony Chacko,
Giuseppe Cavallaro, Solarflare linux maintainers,
Florian Westphal, Bert Kenward, Geert Uytterhoeven, Edward Cree,
Kalle Valo, Marcin Wojtas, Antonio Quartulli, intel-wired-lan,
Florian Fainelli, Felipe Balbi, Gregory CLEMENT,
Dept-HSGLinuxNICDev, Jeff Kirsher, Mugunthan V N, Andrew Lunn,
Steffen Klassert, Stanislaw Gruszka, Denis Kirjanov, Zhao Qiang,
netdev, Gerlando Falauto, linux-wireless, Ion Badulescu,
David S. Miller, Daniele Venzano, Alexandre Torgue
Hello David,
This trivial series is similar to [0] for net/ that you already merged, but
for drivers/net. The patches replaces the open coding to check for a Kconfig
symbol being built-in or module, with IS_ENABLED() macro that does the same.
Using the macro makes the code more readable by helping abstract away some
of the Kconfig built-in and module enable details.
[0]: https://lkml.org/lkml/2016/9/9/323
Best regards,
Javier
Javier Martinez Canillas (15):
3c59x: use IS_ENABLED() instead of checking for built-in or module
starfire: use IS_ENABLED() instead of checking for built-in or module
ethernet: amd: use IS_ENABLED() instead of checking for built-in or
module
bnx2: use IS_ENABLED() instead of checking for built-in or module
sundance: use IS_ENABLED() instead of checking for built-in or module
net/fsl_pq_mdio: use IS_ENABLED() instead of checking for built-in or
module
i825xx: use IS_ENABLED() instead of checking for built-in or module
ixgbe: use IS_ENABLED() instead of checking for built-in or module
net: mvneta: use IS_ENABLED() instead of checking for built-in or
module
natsemi: use IS_ENABLED() instead of checking for built-in or module
sfc: use IS_ENABLED() instead of checking for built-in or module
sis900: use IS_ENABLED() instead of checking for built-in or module
stmmac: use IS_ENABLED() instead of checking for built-in or module
hamradio: use IS_ENABLED() instead of checking for built-in or module
iwlegacy: use IS_ENABLED() instead of checking for built-in or module
drivers/net/ethernet/3com/3c59x.c | 2 +-
drivers/net/ethernet/adaptec/starfire.c | 2 +-
drivers/net/ethernet/amd/7990.c | 6 +++---
drivers/net/ethernet/amd/amd8111e.c | 2 +-
drivers/net/ethernet/broadcom/bnx2.c | 2 +-
drivers/net/ethernet/dlink/sundance.c | 2 +-
drivers/net/ethernet/freescale/fsl_pq_mdio.c | 8 ++++----
drivers/net/ethernet/i825xx/82596.c | 4 ++--
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 4 ++--
drivers/net/ethernet/marvell/mvneta_bm.h | 2 +-
drivers/net/ethernet/natsemi/ns83820.c | 2 +-
drivers/net/ethernet/sfc/falcon_boards.c | 4 ++--
drivers/net/ethernet/sis/sis900.c | 4 ++--
drivers/net/ethernet/sis/sis900.h | 2 +-
drivers/net/ethernet/stmicro/stmmac/common.h | 2 +-
drivers/net/hamradio/bpqether.c | 2 +-
drivers/net/wireless/intel/iwlegacy/common.h | 4 ++--
17 files changed, 27 insertions(+), 27 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH 00/26] constify local structures
From: Geert Uytterhoeven @ 2016-09-12 13:57 UTC (permalink / raw)
To: Felipe Balbi
Cc: Jarkko Sakkinen, Julia Lawall, Linux-Renesas, Joe Perches,
kernel-janitors@vger.kernel.org, Sergei Shtylyov, Linux PM list,
platform-driver-x86, Linux Media Mailing List, linux-can,
Tatyana Nikolova, Shiraz Saleem, Mustafa Ismail, Chien Tin Tung,
linux-rdma, netdev@vger.kernel.org, driverdevel,
ALSA Development Mailing List, linux-kernel@vger.kernel.org,
Linux Fbdev development list, linux-wireless, Jason Gunthorpe,
tpmdd-devel, scsi, linux-spi, USB list, ACPI Devel Maling List
In-Reply-To: <877fah5j35.fsf@linux.intel.com>
On Mon, Sep 12, 2016 at 3:43 PM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> writes:
>> On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
>>> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
>>> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
>>> > > Constify local structures.
>>> > >
>>> > > The semantic patch that makes this change is as follows:
>>> > > (http://coccinelle.lip6.fr/)
>>> >
>>> > Just my two cents but:
>>> >
>>> > 1. You *can* use a static analysis too to find bugs or other issues.
>>> > 2. However, you should manually do the commits and proper commit
>>> > messages to subsystems based on your findings. And I generally think
>>> > that if one contributes code one should also at least smoke test changes
>>> > somehow.
>>> >
>>> > I don't know if I'm alone with my opinion. I just think that one should
>>> > also do the analysis part and not blindly create and submit patches.
>>>
>>> All of the patches are compile tested. And the individual patches are
>>
>> Compile-testing is not testing. If you are not able to test a commit,
>> you should explain why.
>
> Dude, Julia has been doing semantic patching for years already and
> nobody has raised any concerns so far. There's already an expectation
> that Coccinelle *works* and Julia's sematic patches are sound.
+1
> Besides, adding 'const' is something that causes virtually no functional
> changes to the point that build-testing is really all you need. Any
> problems caused by adding 'const' to a definition will be seen by build
> errors or warnings.
Unfortunately in this particular case they could lead to failures that can only
be detected at runtime, when failing o write to a read-only piece of memory,
due to the casting away of the constness of the pointers later.
Fortunately this was detected during code review (doh...).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH v3] mac80211: Re-structure aqm debugfs output and keep CoDel stats per txq
From: Toke Høiland-Jørgensen @ 2016-09-12 13:55 UTC (permalink / raw)
To: make-wifi-fast, linux-wireless; +Cc: Toke Høiland-Jørgensen
In-Reply-To: <20160811151156.6485-1-toke@toke.dk>
Currently the 'aqm' stats in mac80211 only keeps overlimit drop stats,
not CoDel stats. This moves the CoDel stats into the txqi structure to
keep them per txq in order to show them in debugfs.
In addition, the aqm debugfs output is restructured by splitting it up
into three files: One global per phy, one per netdev and one per
station, in the appropriate directories. The files are all called aqm,
and are only created if the driver supports the wake_tx_queue op (rather
than emitting an error on open as previously).
Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk>
---
Changes since v2:
- Dynamically allocate buffer in per-station function.
- Rebase on current mac80211-next.
=20
net/mac80211/debugfs.c | 151 ++++++++----------------------------=
------
net/mac80211/debugfs_netdev.c | 37 ++++++++++-
net/mac80211/debugfs_sta.c | 52 +++++++++++++++
net/mac80211/ieee80211_i.h | 2 +-
net/mac80211/tx.c | 4 +-
5 files changed, 117 insertions(+), 129 deletions(-)
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 2906c10..5bbb470 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -71,138 +71,39 @@ DEBUGFS_READONLY_FILE(wep_iv, "%#08x",
DEBUGFS_READONLY_FILE(rate_ctrl_alg, "%s",
local->rate_ctrl ? local->rate_ctrl->ops->name : "hw/driver");
=20
-struct aqm_info {
- struct ieee80211_local *local;
- size_t size;
- size_t len;
- unsigned char buf[0];
-};
-
-#define AQM_HDR_LEN 200
-#define AQM_HW_ENTRY_LEN 40
-#define AQM_TXQ_ENTRY_LEN 110
-
-static int aqm_open(struct inode *inode, struct file *file)
+static ssize_t aqm_read(struct file *file,
+ char __user *user_buf,
+ size_t count,
+ loff_t *ppos)
{
- struct ieee80211_local *local =3D inode->i_private;
- struct ieee80211_sub_if_data *sdata;
- struct sta_info *sta;
- struct txq_info *txqi;
+ struct ieee80211_local *local =3D file->private_data;
struct fq *fq =3D &local->fq;
- struct aqm_info *info =3D NULL;
+ char buf[200];
int len =3D 0;
- int i;
-
- if (!local->ops->wake_tx_queue)
- return -EOPNOTSUPP;
-
- len +=3D AQM_HDR_LEN;
- len +=3D 6 * AQM_HW_ENTRY_LEN;
-
- rcu_read_lock();
- list_for_each_entry_rcu(sdata, &local->interfaces, list)
- len +=3D AQM_TXQ_ENTRY_LEN;
- list_for_each_entry_rcu(sta, &local->sta_list, list)
- len +=3D AQM_TXQ_ENTRY_LEN * ARRAY_SIZE(sta->sta.txq);
- rcu_read_unlock();
-
- info =3D vmalloc(len);
- if (!info)
- return -ENOMEM;
=20
spin_lock_bh(&local->fq.lock);
rcu_read_lock();
=20
- file->private_data =3D info;
- info->local =3D local;
- info->size =3D len;
- len =3D 0;
-
- len +=3D scnprintf(info->buf + len, info->size - len,
- "* hw\n"
- "access name value\n"
- "R fq_flows_cnt %u\n"
- "R fq_backlog %u\n"
- "R fq_overlimit %u\n"
- "R fq_collisions %u\n"
- "RW fq_limit %u\n"
- "RW fq_quantum %u\n",
- fq->flows_cnt,
- fq->backlog,
- fq->overlimit,
- fq->collisions,
- fq->limit,
- fq->quantum);
-
- len +=3D scnprintf(info->buf + len,
- info->size - len,
- "* vif\n"
- "ifname addr ac backlog-bytes backlog-packets flows overlimit collis=
ions tx-bytes tx-packets\n");
-
- list_for_each_entry_rcu(sdata, &local->interfaces, list) {
- txqi =3D to_txq_info(sdata->vif.txq);
- len +=3D scnprintf(info->buf + len, info->size - len,
- "%s %pM %u %u %u %u %u %u %u %u\n",
- sdata->name,
- sdata->vif.addr,
- txqi->txq.ac,
- txqi->tin.backlog_bytes,
- txqi->tin.backlog_packets,
- txqi->tin.flows,
- txqi->tin.overlimit,
- txqi->tin.collisions,
- txqi->tin.tx_bytes,
- txqi->tin.tx_packets);
- }
-
- len +=3D scnprintf(info->buf + len,
- info->size - len,
- "* sta\n"
- "ifname addr tid ac backlog-bytes backlog-packets flows overlimit co=
llisions tx-bytes tx-packets\n");
-
- list_for_each_entry_rcu(sta, &local->sta_list, list) {
- sdata =3D sta->sdata;
- for (i =3D 0; i < ARRAY_SIZE(sta->sta.txq); i++) {
- txqi =3D to_txq_info(sta->sta.txq[i]);
- len +=3D scnprintf(info->buf + len, info->size - len,
- "%s %pM %d %d %u %u %u %u %u %u %u\n",
- sdata->name,
- sta->sta.addr,
- txqi->txq.tid,
- txqi->txq.ac,
- txqi->tin.backlog_bytes,
- txqi->tin.backlog_packets,
- txqi->tin.flows,
- txqi->tin.overlimit,
- txqi->tin.collisions,
- txqi->tin.tx_bytes,
- txqi->tin.tx_packets);
- }
- }
-
- info->len =3D len;
+ len =3D scnprintf(buf, sizeof(buf),
+ "access name value\n"
+ "R fq_flows_cnt %u\n"
+ "R fq_backlog %u\n"
+ "R fq_overlimit %u\n"
+ "R fq_collisions %u\n"
+ "RW fq_limit %u\n"
+ "RW fq_quantum %u\n",
+ fq->flows_cnt,
+ fq->backlog,
+ fq->overlimit,
+ fq->collisions,
+ fq->limit,
+ fq->quantum);
=20
rcu_read_unlock();
spin_unlock_bh(&local->fq.lock);
=20
- return 0;
-}
-
-static int aqm_release(struct inode *inode, struct file *file)
-{
- vfree(file->private_data);
- return 0;
-}
-
-static ssize_t aqm_read(struct file *file,
- char __user *user_buf,
- size_t count,
- loff_t *ppos)
-{
- struct aqm_info *info =3D file->private_data;
-
return simple_read_from_buffer(user_buf, count, ppos,
- info->buf, info->len);
+ buf, len);
}
=20
static ssize_t aqm_write(struct file *file,
@@ -210,8 +111,7 @@ static ssize_t aqm_write(struct file *file,
size_t count,
loff_t *ppos)
{
- struct aqm_info *info =3D file->private_data;
- struct ieee80211_local *local =3D info->local;
+ struct ieee80211_local *local =3D file->private_data;
char buf[100];
size_t len;
=20
@@ -237,8 +137,7 @@ static ssize_t aqm_write(struct file *file,
static const struct file_operations aqm_ops =3D {
.write =3D aqm_write,
.read =3D aqm_read,
- .open =3D aqm_open,
- .release =3D aqm_release,
+ .open =3D simple_open,
.llseek =3D default_llseek,
};
=20
@@ -428,7 +327,9 @@ void debugfs_hw_add(struct ieee80211_local *local)
DEBUGFS_ADD(hwflags);
DEBUGFS_ADD(user_power);
DEBUGFS_ADD(power);
- DEBUGFS_ADD_MODE(aqm, 0600);
+
+ if (local->ops->wake_tx_queue)
+ DEBUGFS_ADD_MODE(aqm, 0600);
=20
statsd =3D debugfs_create_dir("statistics", phyd);
=20
diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.=
c
index a5ba739..5d35c0f 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -30,7 +30,7 @@ static ssize_t ieee80211_if_read(
size_t count, loff_t *ppos,
ssize_t (*format)(const struct ieee80211_sub_if_data *, char *, int))
{
- char buf[70];
+ char buf[200];
ssize_t ret =3D -EINVAL;
=20
read_lock(&dev_base_lock);
@@ -486,6 +486,38 @@ static ssize_t ieee80211_if_fmt_num_buffered_multica=
st(
}
IEEE80211_IF_FILE_R(num_buffered_multicast);
=20
+static ssize_t ieee80211_if_fmt_aqm(
+ const struct ieee80211_sub_if_data *sdata, char *buf, int buflen)
+{
+ struct ieee80211_local *local =3D sdata->local;
+ struct txq_info *txqi =3D to_txq_info(sdata->vif.txq);
+ int len;
+
+ spin_lock_bh(&local->fq.lock);
+ rcu_read_lock();
+
+ len =3D scnprintf(buf,
+ buflen,
+ "ac backlog-bytes backlog-packets new-flows drops marks overlimit col=
lisions tx-bytes tx-packets\n"
+ "%u %u %u %u %u %u %u %u %u %u\n",
+ txqi->txq.ac,
+ txqi->tin.backlog_bytes,
+ txqi->tin.backlog_packets,
+ txqi->tin.flows,
+ txqi->cstats.drop_count,
+ txqi->cstats.ecn_mark,
+ txqi->tin.overlimit,
+ txqi->tin.collisions,
+ txqi->tin.tx_bytes,
+ txqi->tin.tx_packets);
+
+ rcu_read_unlock();
+ spin_unlock_bh(&local->fq.lock);
+
+ return len;
+}
+IEEE80211_IF_FILE_R(aqm);
+
/* IBSS attributes */
static ssize_t ieee80211_if_fmt_tsf(
const struct ieee80211_sub_if_data *sdata, char *buf, int buflen)
@@ -618,6 +650,9 @@ static void add_common_files(struct ieee80211_sub_if_=
data *sdata)
DEBUGFS_ADD(rc_rateidx_vht_mcs_mask_2ghz);
DEBUGFS_ADD(rc_rateidx_vht_mcs_mask_5ghz);
DEBUGFS_ADD(hw_queues);
+
+ if (sdata->local->ops->wake_tx_queue)
+ DEBUGFS_ADD(aqm);
}
=20
static void add_sta_files(struct ieee80211_sub_if_data *sdata)
diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index fd33413..fb26935 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -133,6 +133,55 @@ static ssize_t sta_last_seq_ctrl_read(struct file *f=
ile, char __user *userbuf,
}
STA_OPS(last_seq_ctrl);
=20
+#define AQM_TXQ_ENTRY_LEN 130
+
+static ssize_t sta_aqm_read(struct file *file, char __user *userbuf,
+ size_t count, loff_t *ppos)
+{
+ struct sta_info *sta =3D file->private_data;
+ struct ieee80211_local *local =3D sta->local;
+ size_t bufsz =3D AQM_TXQ_ENTRY_LEN*(IEEE80211_NUM_TIDS+1);
+ char *buf =3D kzalloc(bufsz, GFP_KERNEL), *p =3D buf;
+ struct txq_info *txqi;
+ ssize_t rv;
+ int i;
+
+ if (!buf)
+ return -ENOMEM;
+
+ spin_lock_bh(&local->fq.lock);
+ rcu_read_lock();
+
+ p +=3D scnprintf(p,
+ bufsz+buf-p,
+ "tid ac backlog-bytes backlog-packets new-flows drops marks ove=
rlimit collisions tx-bytes tx-packets\n");
+
+ for (i =3D 0; i < IEEE80211_NUM_TIDS; i++) {
+ txqi =3D to_txq_info(sta->sta.txq[i]);
+ p +=3D scnprintf(p, bufsz+buf-p,
+ "%d %d %u %u %u %u %u %u %u %u %u\n",
+ txqi->txq.tid,
+ txqi->txq.ac,
+ txqi->tin.backlog_bytes,
+ txqi->tin.backlog_packets,
+ txqi->tin.flows,
+ txqi->cstats.drop_count,
+ txqi->cstats.ecn_mark,
+ txqi->tin.overlimit,
+ txqi->tin.collisions,
+ txqi->tin.tx_bytes,
+ txqi->tin.tx_packets);
+ }
+
+ rcu_read_unlock();
+ spin_unlock_bh(&local->fq.lock);
+
+ rv =3D simple_read_from_buffer(userbuf, count, ppos, buf, p - buf);
+ kfree(buf);
+ return rv;
+}
+STA_OPS(aqm);
+
static ssize_t sta_agg_status_read(struct file *file, char __user *userb=
uf,
size_t count, loff_t *ppos)
{
@@ -478,6 +527,9 @@ void ieee80211_sta_debugfs_add(struct sta_info *sta)
DEBUGFS_ADD_COUNTER(rx_fragments, rx_stats.fragments);
DEBUGFS_ADD_COUNTER(tx_filtered, status_stats.filtered);
=20
+ if (local->ops->wake_tx_queue)
+ DEBUGFS_ADD(aqm);
+
if (sizeof(sta->driver_buffered_tids) =3D=3D sizeof(u32))
debugfs_create_x32("driver_buffered_tids", 0400,
sta->debugfs_dir,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 7576168..c71c735 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -818,6 +818,7 @@ struct txq_info {
struct fq_tin tin;
struct fq_flow def_flow;
struct codel_vars def_cvars;
+ struct codel_stats cstats;
unsigned long flags;
=20
/* keep last! */
@@ -1117,7 +1118,6 @@ struct ieee80211_local {
struct fq fq;
struct codel_vars *cvars;
struct codel_params cparams;
- struct codel_stats cstats;
=20
const struct ieee80211_ops *ops;
=20
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index efc38e7..ee9e7d6 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1343,7 +1343,7 @@ static struct sk_buff *fq_tin_dequeue_func(struct f=
q *fq,
local =3D container_of(fq, struct ieee80211_local, fq);
txqi =3D container_of(tin, struct txq_info, tin);
cparams =3D &local->cparams;
- cstats =3D &local->cstats;
+ cstats =3D &txqi->cstats;
=20
if (flow =3D=3D &txqi->def_flow)
cvars =3D &txqi->def_cvars;
@@ -1403,6 +1403,7 @@ void ieee80211_txq_init(struct ieee80211_sub_if_dat=
a *sdata,
fq_tin_init(&txqi->tin);
fq_flow_init(&txqi->def_flow);
codel_vars_init(&txqi->def_cvars);
+ codel_stats_init(&txqi->cstats);
=20
txqi->txq.vif =3D &sdata->vif;
=20
@@ -1441,7 +1442,6 @@ int ieee80211_txq_setup_flows(struct ieee80211_loca=
l *local)
return ret;
=20
codel_params_init(&local->cparams);
- codel_stats_init(&local->cstats);
local->cparams.interval =3D MS2TIME(100);
local->cparams.target =3D MS2TIME(20);
local->cparams.ecn =3D true;
--=20
2.9.3
^ permalink raw reply related
* Re: [PATCH 00/26] constify local structures
From: Julia Lawall @ 2016-09-12 13:52 UTC (permalink / raw)
To: Felipe Balbi
Cc: Jarkko Sakkinen, Julia Lawall, linux-renesas-soc, joe,
kernel-janitors, Sergei Shtylyov, linux-pm, platform-driver-x86,
linux-media, linux-can, Tatyana Nikolova, Shiraz Saleem,
Mustafa Ismail, Chien Tin Tung, linux-rdma, netdev, devel,
alsa-devel, linux-kernel, linux-fbdev, linux-wireless,
Jason Gunthorpe, tpmdd-devel, linux-scsi, linux-spi, linux-usb,
linux-acpi
In-Reply-To: <877fah5j35.fsf@linux.intel.com>
On Mon, 12 Sep 2016, Felipe Balbi wrote:
>
> Hi,
>
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> writes:
> > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> >>
> >>
> >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> >>
> >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> >> > > Constify local structures.
> >> > >
> >> > > The semantic patch that makes this change is as follows:
> >> > > (http://coccinelle.lip6.fr/)
> >> >
> >> > Just my two cents but:
> >> >
> >> > 1. You *can* use a static analysis too to find bugs or other issues.
> >> > 2. However, you should manually do the commits and proper commit
> >> > messages to subsystems based on your findings. And I generally think
> >> > that if one contributes code one should also at least smoke test changes
> >> > somehow.
> >> >
> >> > I don't know if I'm alone with my opinion. I just think that one should
> >> > also do the analysis part and not blindly create and submit patches.
> >>
> >> All of the patches are compile tested. And the individual patches are
> >
> > Compile-testing is not testing. If you are not able to test a commit,
> > you should explain why.
>
> Dude, Julia has been doing semantic patching for years already and
> nobody has raised any concerns so far. There's already an expectation
> that Coccinelle *works* and Julia's sematic patches are sound.
>
> Besides, adding 'const' is something that causes virtually no functional
> changes to the point that build-testing is really all you need. Any
> problems caused by adding 'const' to a definition will be seen by build
> errors or warnings.
>
> Really, just stop with the pointless discussion and go read a bit about
> Coccinelle and what semantic patches are giving you. The work done by
> Julia and her peers are INRIA have measurable benefits.
>
> You're really making a thunderstorm in a glass of water.
Thanks for the defense, but since a lot of these patches torned out to be
wrong, due to an incorrect parse by Coccinelle, combined with an
unpleasantly lax compiler, Jarkko does have a point that I should have
looked at the patches more carefully. In any case, I have written to the
maintainers relevant to the patches that turned out to be incorrect.
julia
^ permalink raw reply
* Re: [PATCH 00/26] constify local structures
From: Felipe Balbi @ 2016-09-12 13:43 UTC (permalink / raw)
To: Jarkko Sakkinen, Julia Lawall
Cc: linux-renesas-soc, joe, kernel-janitors, Sergei Shtylyov,
linux-pm, platform-driver-x86, linux-media, linux-can,
Tatyana Nikolova, Shiraz Saleem, Mustafa Ismail, Chien Tin Tung,
linux-rdma, netdev, devel, alsa-devel, linux-kernel, linux-fbdev,
linux-wireless, Jason Gunthorpe, tpmdd-devel, linux-scsi,
linux-spi, linux-usb, linux-acpi
In-Reply-To: <20160912131625.GD957@intel.com>
[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]
Hi,
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> writes:
> On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
>>
>>
>> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
>>
>> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
>> > > Constify local structures.
>> > >
>> > > The semantic patch that makes this change is as follows:
>> > > (http://coccinelle.lip6.fr/)
>> >
>> > Just my two cents but:
>> >
>> > 1. You *can* use a static analysis too to find bugs or other issues.
>> > 2. However, you should manually do the commits and proper commit
>> > messages to subsystems based on your findings. And I generally think
>> > that if one contributes code one should also at least smoke test changes
>> > somehow.
>> >
>> > I don't know if I'm alone with my opinion. I just think that one should
>> > also do the analysis part and not blindly create and submit patches.
>>
>> All of the patches are compile tested. And the individual patches are
>
> Compile-testing is not testing. If you are not able to test a commit,
> you should explain why.
Dude, Julia has been doing semantic patching for years already and
nobody has raised any concerns so far. There's already an expectation
that Coccinelle *works* and Julia's sematic patches are sound.
Besides, adding 'const' is something that causes virtually no functional
changes to the point that build-testing is really all you need. Any
problems caused by adding 'const' to a definition will be seen by build
errors or warnings.
Really, just stop with the pointless discussion and go read a bit about
Coccinelle and what semantic patches are giving you. The work done by
Julia and her peers are INRIA have measurable benefits.
You're really making a thunderstorm in a glass of water.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply
* [PATCH 2/2] mwifiex: firmware name correction for usb8997 chipset
From: Amitkumar Karwar @ 2016-09-12 13:25 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, Ganapathi Bhat, Amitkumar Karwar
In-Reply-To: <1473686728-8101-1-git-send-email-akarwar@marvell.com>
From: Ganapathi Bhat <gbhat@marvell.com>
Similar to pcie8997 chipset, first firmware submitted for usb8997
chipset will be usbusb8997_combo_v4.bin. This patch corrects the
name used in driver.
Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
drivers/net/wireless/marvell/mwifiex/usb.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.h b/drivers/net/wireless/marvell/mwifiex/usb.h
index 1b49c52..30e8eb8 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.h
+++ b/drivers/net/wireless/marvell/mwifiex/usb.h
@@ -46,7 +46,7 @@
#define USB8766_DEFAULT_FW_NAME "mrvl/usb8766_uapsta.bin"
#define USB8797_DEFAULT_FW_NAME "mrvl/usb8797_uapsta.bin"
#define USB8801_DEFAULT_FW_NAME "mrvl/usb8801_uapsta.bin"
-#define USB8997_DEFAULT_FW_NAME "mrvl/usb8997_uapsta.bin"
+#define USB8997_DEFAULT_FW_NAME "mrvl/usbusb8997_combo_v4.bin"
#define FW_DNLD_TX_BUF_SIZE 620
#define FW_DNLD_RX_BUF_SIZE 2048
--
1.9.1
^ permalink raw reply related
* [PATCH 1/2] mwifiex: Command 7 handling for USB chipsets
From: Amitkumar Karwar @ 2016-09-12 13:25 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, Ganapathi Bhat, Amitkumar Karwar
In-Reply-To: <1473686728-8101-1-git-send-email-akarwar@marvell.com>
From: Ganapathi Bhat <gbhat@marvell.com>
Firmware image for newer USB chipsets starts with a command 7 block
(special command). It doesn't contain data length field. This patch adds
necessary handling.
Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
drivers/net/wireless/marvell/mwifiex/usb.c | 4 ++++
drivers/net/wireless/marvell/mwifiex/usb.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 9213516..8a20620 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -1026,6 +1026,10 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter,
dnld_cmd = le32_to_cpu(fwdata->fw_hdr.dnld_cmd);
tlen += sizeof(struct fw_header);
+ /* Command 7 doesn't have data length field */
+ if (dnld_cmd == FW_CMD_7)
+ dlen = 0;
+
memcpy(fwdata->data, &firmware[tlen], dlen);
fwdata->seq_num = cpu_to_le32(fw_seqnum);
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.h b/drivers/net/wireless/marvell/mwifiex/usb.h
index b4e9246..1b49c52 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.h
+++ b/drivers/net/wireless/marvell/mwifiex/usb.h
@@ -51,6 +51,7 @@
#define FW_DNLD_TX_BUF_SIZE 620
#define FW_DNLD_RX_BUF_SIZE 2048
#define FW_HAS_LAST_BLOCK 0x00000004
+#define FW_CMD_7 0x00000007
#define FW_DATA_XMIT_SIZE \
(sizeof(struct fw_header) + dlen + sizeof(u32))
--
1.9.1
^ permalink raw reply related
* [PATCH 0/2] mwifiex: usb8997 chipset support
From: Amitkumar Karwar @ 2016-09-12 13:25 UTC (permalink / raw)
To: linux-wireless; +Cc: Cathy Luo, Nishant Sarmukadam, Amitkumar Karwar
This patch series includes changes needed to support Marvell usb8997
chipset.
Ganapathi Bhat (2):
mwifiex: Command 7 handling for USB chipsets
mwifiex: firmware name correction for usb8997 chipset
drivers/net/wireless/marvell/mwifiex/usb.c | 4 ++++
drivers/net/wireless/marvell/mwifiex/usb.h | 3 ++-
2 files changed, 6 insertions(+), 1 deletion(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH 00/26] constify local structures
From: Julia Lawall @ 2016-09-12 13:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-renesas-soc, joe, kernel-janitors, Sergei Shtylyov,
linux-pm, platform-driver-x86, linux-media, linux-can,
Tatyana Nikolova, Shiraz Saleem, Mustafa Ismail, Chien Tin Tung,
linux-rdma, netdev, devel, alsa-devel, linux-kernel, linux-fbdev,
linux-wireless, Jason Gunthorpe, tpmdd-devel, linux-scsi,
linux-spi, linux-usb, linux-acpi
In-Reply-To: <20160912131625.GD957@intel.com>
On Mon, 12 Sep 2016, Jarkko Sakkinen wrote:
> On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
> >
> >
> > On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> >
> > > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > > > Constify local structures.
> > > >
> > > > The semantic patch that makes this change is as follows:
> > > > (http://coccinelle.lip6.fr/)
> > >
> > > Just my two cents but:
> > >
> > > 1. You *can* use a static analysis too to find bugs or other issues.
> > > 2. However, you should manually do the commits and proper commit
> > > messages to subsystems based on your findings. And I generally think
> > > that if one contributes code one should also at least smoke test changes
> > > somehow.
> > >
> > > I don't know if I'm alone with my opinion. I just think that one should
> > > also do the analysis part and not blindly create and submit patches.
> >
> > All of the patches are compile tested. And the individual patches are
>
> Compile-testing is not testing. If you are not able to test a commit,
> you should explain why.
>
> > submitted to the relevant maintainers. The individual commit messages
> > give a more detailed explanation of the strategy used to decide that the
> > structure was constifiable. It seemed redundant to put that in the cover
> > letter, which will not be committed anyway.
>
> I don't mean to be harsh but I do not care about your thought process
> *that much* when I review a commit (sometimes it might make sense to
> explain that but it depends on the context).
>
> I mostly only care why a particular change makes sense for this
> particular subsystem. The report given by a static analysis tool can
> be a starting point for making a commit but it's not sufficient.
> Based on the report you should look subsystems as individuals.
OK, thanks for the feedback.
julia
^ permalink raw reply
* Re: [PATCH v8] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue.
From: Johannes Berg @ 2016-09-12 13:19 UTC (permalink / raw)
To: Toke Høiland-Jørgensen; +Cc: make-wifi-fast, linux-wireless
In-Reply-To: <87wpihe045.fsf@toke.dk>
> Well, the TXQ already adds a lot of other overhead (hashing on the
> packet header, for one), so my guess would be that this would be
> negligible compared to all that?
>
> >
> > I suppose I don't have to care all that much about the TXQs, but
> > ...
> >
> > Then again, adding a field in the skb->cb for the sake of this? No,
> > not really either.
>
> So that's a "keep it", then? :)
Yeah I think so :)
> > > + ieee80211_xmit_fast_finish(sta->sdata, sta,
> > > pn_offs,
> > > + info->control.hw_key,
> > > skb);
> >
> > I don't see how keeping the info->control.hw_key pointer across the
> > TXQ/FQ/Codel queueing isn't a potential bug? Probably one that
> > already exists in your code today, before this patch, of course.
>
> You mean the key could get removed from the hardware while the packet
> was queued? Can certainly add a check for that. Under what conditions
> does that happen? Does it make sense to try to recover from it (I
> guess by calling tx_h_select_key), or is it rare enough that giving
> up and dropping the packet makes more sense?
Not just from the hardware, more importantly the whole key structure
can be kfree()d, leading to use-after-free here, no?
Fast-xmit solves this by invalidating the fast-xmit cache when the key
pointer changes/goes away and possibly punting some frames to the slow
path, but you've absolutely no protection on these pointers here within
the TXQs, afaict?
A similar situation occurs with other pointers, like stations and vifs,
but when those are removed then obviously the entire TXQs are flushed,
so they're not relevant.
With the key though, frames can be on the queue while a key is removed,
and even before this patch, drivers would consequently access an
invalid key pointer.
Mind you, as I just wrote I think that issue exists even before this
patch, so you should probably look at it separately. Felix might know
better too.
johannes
^ permalink raw reply
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