Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-26 Thread Kalle Valo 
IgorMitsyanko  writes:

> On 09/26/2016 01:56 PM, Kalle Valo wrote:
>> IgorMitsyanko  writes:
>>> On 09/17/2016 04:46 PM, Kalle Valo wrote:
>>
>> For the initial submission please freeze the driver, otherwise it's pain
>> to review as the driver changes too much in-between review rounds. So at
>> this stage only minimal changes, please.
>>
>> You can continue adding new features and making changes, but do those as
>> follow up patches and use the initial submission as the baseline for the
>> new patches. Once the driver is applied you can submit the rest of the
>> patches adding new features and they will be reviewed similarly like all
>> other wireless patches.
>
> Ok, we will keep patch modification to minimum between revisions, but
> channels-related changes are something we would like to apply: we're
> setting SELF_MANAGED bit right now and do not handle regulatory hints
> from cfg80211, having all the regulatory-related info fixed on device
> itself (region, per-channel Tx powers, DFS requirements). This info is
> what was used to pass regulatory authorities certification, and
> considering seriousness of regulatory compliance requirement it's
> probably better to use the info that is known to be accepted.
> Would it be acceptable if we keep rates/capabilities logic intact, but
> will modify channel-related logic in next patch revision?

Sure, regulatory compliance is important and it's understandable that
you want to fix that. Just give a short summary in the change log what
you changed regarding regulatory to make it easier for the reviewers.

-- 
Kalle Valo

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-26 Thread IgorMitsyanko 


On 09/26/2016 01:56 PM, Kalle Valo wrote:

IgorMitsyanko  writes:


On 09/17/2016 04:46 PM, Kalle Valo wrote:

 writes:


+/* Supported rates to be advertised to the cfg80211 */
+static struct ieee80211_rate qtnf_rates[] = {
+   {.bitrate = 10, .hw_value = 2, },
+   {.bitrate = 20, .hw_value = 4, },
+   {.bitrate = 55, .hw_value = 11, },
+   {.bitrate = 110, .hw_value = 22, },
+   {.bitrate = 60, .hw_value = 12, },
+   {.bitrate = 90, .hw_value = 18, },
+   {.bitrate = 120, .hw_value = 24, },
+   {.bitrate = 180, .hw_value = 36, },
+   {.bitrate = 240, .hw_value = 48, },
+   {.bitrate = 360, .hw_value = 72, },
+   {.bitrate = 480, .hw_value = 96, },
+   {.bitrate = 540, .hw_value = 108, },
+};
+
+/* Channel definitions to be advertised to cfg80211 */
+static struct ieee80211_channel qtnf_channels_2ghz[] = {
+   {.center_freq = 2412, .hw_value = 1, },
+   {.center_freq = 2417, .hw_value = 2, },
+   {.center_freq = 2422, .hw_value = 3, },
+   {.center_freq = 2427, .hw_value = 4, },
+   {.center_freq = 2432, .hw_value = 5, },
+   {.center_freq = 2437, .hw_value = 6, },
+   {.center_freq = 2442, .hw_value = 7, },
+   {.center_freq = 2447, .hw_value = 8, },
+   {.center_freq = 2452, .hw_value = 9, },
+   {.center_freq = 2457, .hw_value = 10, },
+   {.center_freq = 2462, .hw_value = 11, },
+   {.center_freq = 2467, .hw_value = 12, },
+   {.center_freq = 2472, .hw_value = 13, },
+   {.center_freq = 2484, .hw_value = 14, },
+};

I guess some of these static variables could be also const, but didn't
check.

We did some changes to this code recently: will get bands info
(channel list, rates, capabilities etc) from wireless device itself,
it will be in next patch revision.

For the initial submission please freeze the driver, otherwise it's pain
to review as the driver changes too much in-between review rounds. So at
this stage only minimal changes, please.

You can continue adding new features and making changes, but do those as
follow up patches and use the initial submission as the baseline for the
new patches. Once the driver is applied you can submit the rest of the
patches adding new features and they will be reviewed similarly like all
other wireless patches.


Ok, we will keep patch modification to minimum between revisions, but 
channels-related changes are something we would like to apply: we're 
setting SELF_MANAGED bit right now and do not handle regulatory hints 
from cfg80211, having all the regulatory-related info fixed on device 
itself (region, per-channel Tx powers, DFS requirements). This info is 
what was used to pass regulatory authorities certification, and 
considering seriousness of regulatory compliance requirement it's 
probably better to use the info that is known to be accepted.
Would it be acceptable if we keep rates/capabilities logic intact, but 
will modify channel-related logic in next patch revision?


In a follow-up patches we're considering introducing closer and more 
flexible integration with cfg80211 regulatory logic, allowing host 
system itself to provide regulatory info. This will need further legal 
consideration though: FCC and European commission seem to stricken rules 
recently, maybe having region fixed on device is a more compliant solution.






+static int
+qtnf_event_handle_sta_assoc(struct qtnf_wmac *mac, struct qtnf_vif *vif,
+   const struct qlink_event_sta_assoc *sta_assoc,
+   u16 len)
+{
+   const u8 *sta_addr;
+   u16 frame_control;
+   struct station_info sinfo = { 0 };
+   size_t payload_len;
+   u16 tlv_type;
+   u16 tlv_value_len;
+   size_t tlv_full_len;
+   const struct qlink_tlv_hdr *tlv;
+
+   if (unlikely(len < sizeof(*sta_assoc))) {
+   pr_err("%s: payload is too short (%u < %zu)\n", __func__,
+  len, sizeof(*sta_assoc));
+   return -EINVAL;
+   }

I see unlikely() used a lot, I counted 145 times. Not a big issue but I
don't see the point. In hot path I understand using it, but not
everywhere.

Agree, but would you suggest that we remove the ones that we already
have but that are not really needed?

Up to you really. This isn't important, just something I found a bit
odd.

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-26 Thread Kalle Valo 
IgorMitsyanko  writes:

> On 09/17/2016 04:46 PM, Kalle Valo wrote:
>>  writes:
>>
>>> +/* Supported rates to be advertised to the cfg80211 */
>>> +static struct ieee80211_rate qtnf_rates[] = {
>>> +   {.bitrate = 10, .hw_value = 2, },
>>> +   {.bitrate = 20, .hw_value = 4, },
>>> +   {.bitrate = 55, .hw_value = 11, },
>>> +   {.bitrate = 110, .hw_value = 22, },
>>> +   {.bitrate = 60, .hw_value = 12, },
>>> +   {.bitrate = 90, .hw_value = 18, },
>>> +   {.bitrate = 120, .hw_value = 24, },
>>> +   {.bitrate = 180, .hw_value = 36, },
>>> +   {.bitrate = 240, .hw_value = 48, },
>>> +   {.bitrate = 360, .hw_value = 72, },
>>> +   {.bitrate = 480, .hw_value = 96, },
>>> +   {.bitrate = 540, .hw_value = 108, },
>>> +};
>>> +
>>> +/* Channel definitions to be advertised to cfg80211 */
>>> +static struct ieee80211_channel qtnf_channels_2ghz[] = {
>>> +   {.center_freq = 2412, .hw_value = 1, },
>>> +   {.center_freq = 2417, .hw_value = 2, },
>>> +   {.center_freq = 2422, .hw_value = 3, },
>>> +   {.center_freq = 2427, .hw_value = 4, },
>>> +   {.center_freq = 2432, .hw_value = 5, },
>>> +   {.center_freq = 2437, .hw_value = 6, },
>>> +   {.center_freq = 2442, .hw_value = 7, },
>>> +   {.center_freq = 2447, .hw_value = 8, },
>>> +   {.center_freq = 2452, .hw_value = 9, },
>>> +   {.center_freq = 2457, .hw_value = 10, },
>>> +   {.center_freq = 2462, .hw_value = 11, },
>>> +   {.center_freq = 2467, .hw_value = 12, },
>>> +   {.center_freq = 2472, .hw_value = 13, },
>>> +   {.center_freq = 2484, .hw_value = 14, },
>>> +};
>> I guess some of these static variables could be also const, but didn't
>> check.
>
> We did some changes to this code recently: will get bands info
> (channel list, rates, capabilities etc) from wireless device itself,
> it will be in next patch revision.

For the initial submission please freeze the driver, otherwise it's pain
to review as the driver changes too much in-between review rounds. So at
this stage only minimal changes, please.

You can continue adding new features and making changes, but do those as
follow up patches and use the initial submission as the baseline for the
new patches. Once the driver is applied you can submit the rest of the
patches adding new features and they will be reviewed similarly like all
other wireless patches.

>>> +static int
>>> +qtnf_event_handle_sta_assoc(struct qtnf_wmac *mac, struct qtnf_vif *vif,
>>> +   const struct qlink_event_sta_assoc *sta_assoc,
>>> +   u16 len)
>>> +{
>>> +   const u8 *sta_addr;
>>> +   u16 frame_control;
>>> +   struct station_info sinfo = { 0 };
>>> +   size_t payload_len;
>>> +   u16 tlv_type;
>>> +   u16 tlv_value_len;
>>> +   size_t tlv_full_len;
>>> +   const struct qlink_tlv_hdr *tlv;
>>> +
>>> +   if (unlikely(len < sizeof(*sta_assoc))) {
>>> +   pr_err("%s: payload is too short (%u < %zu)\n", __func__,
>>> +  len, sizeof(*sta_assoc));
>>> +   return -EINVAL;
>>> +   }
>>
>> I see unlikely() used a lot, I counted 145 times. Not a big issue but I
>> don't see the point. In hot path I understand using it, but not
>> everywhere.
>
> Agree, but would you suggest that we remove the ones that we already
> have but that are not really needed?

Up to you really. This isn't important, just something I found a bit
odd.

-- 
Kalle Valo

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-26 Thread Kalle Valo 
IgorMitsyanko  writes:

> On 09/17/2016 04:56 PM, Kalle Valo wrote:
>>  writes:
>>
>>> +/* FW names */
>>> +
>>> +#define QTN_PCI_FW_NAME"pearl-linux.lzma.img"
>>
>> The firmware name gives no indication what this file is about (remember
>> that linux-firmware.git has a lot of files). Please name it properly,
>> don't just use what is used in by firmware build scripts :) Take into
>> account also future hw support, all firmware files need to coexist in
>> the same repository without user invention. In a way the firmware
>> filename is part of kernel/userspace interface and needs to be stable.
>>
>> For example, you could use something like "qtnfmac/qsr10g.img" (assuming
>> qsr10g is the name of chip).
>
> Ok, we will reconsider our naming conventions, take into account more
> devices that we need to support in the future.
> I'm thinking about something like:
> qtn/fmac_qsr10g.img  < FullMAC QSR10G device
> qtn/fmac_qsr1000.img  < FullMAC QSR1000 device
> qtn/smac_qsr10g.img  < SoftMAC QSR10G
> qtn/smac_qsr1.img <- SoftMAC QSR1000
> etc

Looks good to me.

-- 
Kalle Valo

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-20 Thread IgorMitsyanko 


On 09/17/2016 04:46 PM, Kalle Valo wrote:

 writes:


From: Avinash Patil 

This patch adds support for new FullMAC WiFi driver for Quantenna
QSR10G chipsets.

QSR10G is Quantenna's 8x8, 160M, 11ac offering.
QSR10G supports 2 simultaneous WMACs- one 5G and one 2G. 5G WMAC
supports 160M, 8x8 configuration.
FW supports 8 concurrent virtual interfaces on each WMAC.

Patch introduces 2 new drivers- qtnfmac.ko for interfacing with
kernel/cfg80211 and qtnfmac_pcie.ko for PCIe bus interface.

Signed-off-by: Dmitrii Lebed 
Signed-off-by: Sergei Maksimenko 
Signed-off-by: Sergey Matyukevich 
Signed-off-by: Bindu Therthala 
Signed-off-by: Huizhao Wang 
Signed-off-by: Kamlesh Rath 
Signed-off-by: Avinash Patil 
Signed-off-by: Igor Mitsyanko 

I read this through and looking good. Below are some comments.

I also saw few warnings but I suspect they are because of cfg80211 API
changes (didn't bother to check that now):

drivers/net/wireless/quantenna/qtnfmac/event.c: In function 
`qtnf_event_handle_scan_complete':
drivers/net/wireless/quantenna/qtnfmac/event.c:342:2: warning: passing argument 
2 of `cfg80211_scan_done' makes pointer from integer without a cast [enabled by 
default]
./include/net/cfg80211.h:4104:6: note: expected `struct cfg80211_scan_info *' 
but argument is of type `u32'
drivers/net/wireless/quantenna/qtnfmac/cfg80211.c: In function 
`qtnf_virtual_intf_cleanup':
drivers/net/wireless/quantenna/qtnfmac/cfg80211.c:1093:4: warning: passing 
argument 2 of `cfg80211_scan_done' makes pointer from integer without a cast 
[enabled by default]
./include/net/cfg80211.h:4104:6: note: expected `struct cfg80211_scan_info *' 
but argument is of type `int'


We will rebase before resubmitting, and address other issues that 
builbot has reported.





Resend to correct email kv...@codeaurora.org.

BTW, no need to send me directly. I take patches directly from patchwork
so sending them to linux-wireless is enough. Doesn't do any harm to send
them to, I just immediately delete them from my INBOX :)


Noted)




Changelist V1->V2:
1. Get rid of confidentiality footer.
2. Rewrite qlink.h header to make it easier to use, add
documentation to most of qlink.h definitions.

  MAINTAINERS|8 +
  drivers/net/wireless/Kconfig   |1 +
  drivers/net/wireless/Makefile  |1 +
  drivers/net/wireless/quantenna/Kconfig |   16 +
  drivers/net/wireless/quantenna/Makefile|6 +
  drivers/net/wireless/quantenna/include/bus.h   |  195 ++
  .../wireless/quantenna/include/pcie_regs_pearl.h   |  353 
  drivers/net/wireless/quantenna/include/qlink.h |  939 ++
  .../net/wireless/quantenna/include/qtn_hw_ids.h|   34 +
  .../net/wireless/quantenna/include/shm_ipc_defs.h  |   46 +

Is there a particular reason why you have a separate include directory?
There's just one quantenna driver for now so the extra include directory
feels unnecessary. I would prefer to have that only once there are two
quantenna drivers and we know exactly what is shared between the two.


No particular reason, just prepared for any potential future additions, 
like different transport for example (USB, SDIO) or new driver.

Will drop separate include directory for the initial patch.




--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9151,6 +9151,14 @@ L:   qemu-de...@nongnu.org
  S:Maintained
  F:drivers/firmware/qemu_fw_cfg.c
  
+QUANTENNA QTNFMAC WIRELESS DRIVER

+M: Igor Mitsyanko 
+M: Avinash Patil 
+M: Sergey Matyukevich 
+L: linux-wireless@vger.kernel.org
+S: Maintained
+F: drivers/net/wireless/quantenna/qtnfmac

The include directory is not listed.


+ccflags-y += -D__CHECK_ENDIAN

Very good.


+/* Supported rates to be advertised to the cfg80211 */
+static struct ieee80211_rate qtnf_rates[] = {
+   {.bitrate = 10, .hw_value = 2, },
+   {.bitrate = 20, .hw_value = 4, },
+   {.bitrate = 55, .hw_value = 11, },
+   {.bitrate = 110, .hw_value = 22, },
+   {.bitrate = 60, .hw_value = 12, },
+   {.bitrate = 90, .hw_value = 18, },
+   {.bitrate = 120, .hw_value = 24, },
+   {.bitrate = 180, .hw_value = 36, },
+   {.bitrate = 240, .hw_value = 48, },
+   {.bitrate = 360, .hw_value = 72, },
+   {.bitrate = 480, .hw_value = 96, },
+   {.bitrate = 540, .hw_value = 108, },
+};
+
+/* Channel definitions to be advertised to cfg80211 */
+static struct ieee80211_channel qtnf_channels_2ghz[] = {
+   {.center_freq = 2412, .hw_value = 1, },
+   {.center_freq = 2417, .hw_value = 2, },
+   {.center_freq = 2422, .hw_value = 3, },
+

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-20 Thread IgorMitsyanko 


On 09/17/2016 04:56 PM, Kalle Valo wrote:

 writes:


From: Avinash Patil 

This patch adds support for new FullMAC WiFi driver for Quantenna
QSR10G chipsets.

QSR10G is Quantenna's 8x8, 160M, 11ac offering.
QSR10G supports 2 simultaneous WMACs- one 5G and one 2G. 5G WMAC
supports 160M, 8x8 configuration.
FW supports 8 concurrent virtual interfaces on each WMAC.

Patch introduces 2 new drivers- qtnfmac.ko for interfacing with
kernel/cfg80211 and qtnfmac_pcie.ko for PCIe bus interface.

Signed-off-by: Dmitrii Lebed 
Signed-off-by: Sergei Maksimenko 
Signed-off-by: Sergey Matyukevich 
Signed-off-by: Bindu Therthala 
Signed-off-by: Huizhao Wang 
Signed-off-by: Kamlesh Rath 
Signed-off-by: Avinash Patil 
Signed-off-by: Igor Mitsyanko 

More comments:


+/* FW names */
+
+#define QTN_PCI_FW_NAME"pearl-linux.lzma.img"

The firmware name gives no indication what this file is about (remember
that linux-firmware.git has a lot of files). Please name it properly,
don't just use what is used in by firmware build scripts :) Take into
account also future hw support, all firmware files need to coexist in
the same repository without user invention. In a way the firmware
filename is part of kernel/userspace interface and needs to be stable.

For example, you could use something like "qtnfmac/qsr10g.img" (assuming
qsr10g is the name of chip).


Ok, we will reconsider our naming conventions, take into account more 
devices that we need to support in the future.

I'm thinking about something like:
qtn/fmac_qsr10g.img  < FullMAC QSR10G device
qtn/fmac_qsr1000.img  < FullMAC QSR1000 device
qtn/smac_qsr10g.img  < SoftMAC QSR10G
qtn/smac_qsr1.img <- SoftMAC QSR1000
etc


I forgot already, is the firmware image ready for submission to
linux-firmware.git?
Yes, we have firmware binary license prepared by our legal department 
and its ready for submitting. Will send in parallel with next patch 
revision (only to linux-wireless for now, not to linux-firmware, as was 
discussed).





+   pr_info("%s: %sregistered mgmt frame type 0x%x\n", __func__,
+   reg ? "" : "un", frame_type);

The driver seems to be quite spammy with info messages:

qtnfmac/cfg80211.c: pr_info("%s: %sregistered mgmt frame type 0x%x\n", 
__func__,
qtnfmac/cfg80211.c: pr_info("QTNF: %s cipher=%x, idx=%u, pairwise=%u\n", 
__func__,
qtnfmac/cfg80211.c: pr_info("QTNF: %s idx=%u, pairwise=%u\n", __func__, 
key_index,
qtnfmac/cfg80211.c: pr_info("QTNF: %s idx=%u, unicast=%u, multicast=%u\n", 
__func__,
qtnfmac/cfg80211.c: pr_info("QTNF: %s idx=%u\n", __func__, key_index);
qtnfmac/cfg80211.c: pr_info("%s: initiator=%d, alpha=%c%c, macid=%d\n", 
__func__,
qtnfmac/cfg80211.c: pr_info("%s: MAX_IF: %zu; MODES: %.4X; RADAR WIDTHS: 
%.2X\n", __func__,
qtnfmac/cfg80211.c: pr_info("macid=%d, phymode=%#x\n", mac->macid, 
mac->macinfo.phymode);
qtnfmac/commands.c: pr_info("%s: unexpected TLV type: 
%.4X\n",
qtnfmac/commands.c: pr_info("%s: STA %pM not found\n", 
__func__, sta_mac);
qtnfmac/commands.c: pr_info("country-code from EP: %c%c\n", 
hwinfo->country_code[0],
qtnfmac/commands.c: pr_info("fw_version = %d, num_mac=%d, mac_bitmap=%#x\n",
qtnfmac/commands.c: pr_info("iface limit record 
count=%zu\n", record_count);
qtnfmac/commands.c: pr_info("MAC%d reported channels %d\n",
qtnfmac/init.c: pr_info("%s: macid=%d\n", __func__, macid);
qtnfmac/pcie.c: pr_info("enabled PCIE MSI interrupt\n");
qtnfmac/pcie.c: pr_info("%s: BAR[%u] vaddr=0x%p busaddr=0x%p len=%u\n",
qtnfmac/pcie.c: pr_info("%s: set mps to %d (was %d, max %d)\n",
qtnfmac/pcie.c: pr_info("fw download started: fw start addr = 0x%p, size=%d\n",
qtnfmac/pcie.c: pr_info("fw download completed: totally sent %d blocks\n", blk);
qtnfmac/pcie.c: pr_info("RC is ready to boot EP...\n");
qtnfmac/pcie.c: pr_info("starting download firmware %s...\n", bus->fwname);
qtnfmac/pcie.c: pr_info("successful init of PCI device %x\n", 
pdev->device);
qtnfmac/pcie.c: pr_info("Register Quantenna FullMAC PCIE driver\n");
qtnfmac/pcie.c: pr_info("Unregister Quantenna FullMAC PCIE driver\n");
qtnfmac/trans.c:pr_info("%s: interrupted\n", __func__);
qtnfmac/trans.c:pr_info("%s: skb dropped\n", __func__);

Usualle the preference is that driver is quiet until something goes
wrong. I hope some of these could be debug messages.


We will reduce noise generated by driver.




--- /dev/null
+++ b/drivers/net/wireless/quantenna/qtnfmac/pcie.c
@@ -0,0 +1,1374 @@
+/**
+ * Copyright (c) 2015-2016 Quantenna Communications, Inc.
+ * All rights reserved.
+ *
+ * This program is

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-20 Thread IgorMitsyanko 


On 09/17/2016 05:50 PM, Johannes Berg wrote:

drivers/net/wireless/quantenna/qtnfmac/event.c: In function
`qtnf_event_handle_scan_complete':
drivers/net/wireless/quantenna/qtnfmac/event.c:342:2: warning:
passing argument 2 of `cfg80211_scan_done' makes pointer from integer
without a cast [enabled by default]

Yes, cfg80211_scan_done() changed fairly recently for sure.


We will rebase before submitting next patch revision.




./include/net/cfg80211.h:4104:6: note: expected `struct
cfg80211_scan_info *' but argument is of type `u32'
drivers/net/wireless/quantenna/qtnfmac/cfg80211.c: In function
`qtnf_virtual_intf_cleanup':
drivers/net/wireless/quantenna/qtnfmac/cfg80211.c:1093:4: warning:
passing argument 2 of `cfg80211_scan_done' makes pointer from integer
without a cast [enabled by default]
./include/net/cfg80211.h:4104:6: note: expected `struct
cfg80211_scan_info *' but argument is of type `int'


These also seem related.


+F:   drivers/net/wireless/quantenna/qtnfmac

The include directory is not listed.

Should really just stop after quantenna/ I'd think? As long as it's
just a single driver, you might as well claim maintenance over
everything there :)


Yes, makes sense)




I guess some of these static variables could be also const, but
didn't check.

I think both bitrates and channels can't be, due to cfg80211 writing
some (global) flags there on init.

johannes

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-17 Thread Kalle Valo 
Johannes Berg  writes:

> On Sat, 2016-09-17 at 18:01 +0300, Kalle Valo wrote:
>> 
>> Sorry, I was unclear. I meant the whole file
> [...]

Argh, I meant the whole driver of course, not just the file.

> Ah, ok - yeah, I think you're right - I think the supported_band ones
> can be, for example, and probably others as well.

Maybe also some of the pci related.

-- 
Kalle Valo

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-17 Thread Johannes Berg 
On Sat, 2016-09-17 at 18:01 +0300, Kalle Valo wrote:
> 
> Sorry, I was unclear. I meant the whole file
[...]

Ah, ok - yeah, I think you're right - I think the supported_band ones
can be, for example, and probably others as well.

johannes

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-17 Thread Kalle Valo 
Johannes Berg  writes:

>> I guess some of these static variables could be also const, but
>> didn't check.
>
> I think both bitrates and channels can't be, due to cfg80211 writing
> some (global) flags there on init.

Sorry, I was unclear. I meant the whole file, not just what was in my
mail:

qtnfmac/cfg80211.c:static struct ieee80211_rate qtnf_rates[] = {
qtnfmac/cfg80211.c:static struct ieee80211_channel qtnf_channels_2ghz[] = {
qtnfmac/cfg80211.c:static struct ieee80211_supported_band qtnf_band_2ghz = {
qtnfmac/cfg80211.c:static struct ieee80211_channel qtnf_channels_5ghz[] = {
qtnfmac/cfg80211.c:static struct ieee80211_supported_band qtnf_band_5ghz = {
qtnfmac/cfg80211.c:static struct cfg80211_ops qtn_cfg80211_ops = {
qtnfmac/pcie.c:static struct qtnf_bus_ops qtnf_pcie_bus_ops = {
qtnfmac/pcie.c:static struct attribute *qtnf_sysfs_entries[] = {
qtnfmac/pcie.c:static struct attribute_group qtnf_attrs_group = {
qtnfmac/pcie.c:static struct pci_device_id qtnf_pcie_devid_table[] = {
qtnfmac/pcie.c:static struct pci_driver qtnf_pcie_drv_data = {

Anyway, nothing important. Just something which I started to wonder
while reading the code.
 
-- 
Kalle Valo

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-17 Thread Johannes Berg 

> drivers/net/wireless/quantenna/qtnfmac/event.c: In function
> `qtnf_event_handle_scan_complete':
> drivers/net/wireless/quantenna/qtnfmac/event.c:342:2: warning:
> passing argument 2 of `cfg80211_scan_done' makes pointer from integer
> without a cast [enabled by default]

Yes, cfg80211_scan_done() changed fairly recently for sure.

> ./include/net/cfg80211.h:4104:6: note: expected `struct
> cfg80211_scan_info *' but argument is of type `u32'
> drivers/net/wireless/quantenna/qtnfmac/cfg80211.c: In function
> `qtnf_virtual_intf_cleanup':
> drivers/net/wireless/quantenna/qtnfmac/cfg80211.c:1093:4: warning:
> passing argument 2 of `cfg80211_scan_done' makes pointer from integer
> without a cast [enabled by default]
> ./include/net/cfg80211.h:4104:6: note: expected `struct
> cfg80211_scan_info *' but argument is of type `int'
> 

These also seem related.

> > +F:   drivers/net/wireless/quantenna/qtnfmac

> The include directory is not listed.

Should really just stop after quantenna/ I'd think? As long as it's
just a single driver, you might as well claim maintenance over
everything there :)

> I guess some of these static variables could be also const, but
> didn't check.

I think both bitrates and channels can't be, due to cfg80211 writing
some (global) flags there on init.

johannes

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-17 Thread Kalle Valo 
Igor Mitsyanko  writes:

> thank for review and sorry for long delays in replies. A parallel
> thread with Dave a discussion was initiated regarding HW platform
> samples availability. Current driver implementation supports a newer
> QSR10G platform, which availability is limited due to the platform
> still being in development stage. We're working on supporting
> Quantenna's current QSR1000 platform too, what would be the best
> approach to this:
> - have QSR1000 platform support ready, amend it to current patch and
> post it as a new patch revision (there should be a certain amount of
> new code);
> - have current patch accepted by community first, and then post a new
> patch (adding new platform support) one on top of it.

I prefer the latter option. Let's first try to get the driver applied
and then you can submit more features as followup patches.

-- 
Kalle Valo

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-09-17 Thread Kalle Valo 
 writes:

> From: Avinash Patil 
>
> This patch adds support for new FullMAC WiFi driver for Quantenna
> QSR10G chipsets.
>
> QSR10G is Quantenna's 8x8, 160M, 11ac offering.
> QSR10G supports 2 simultaneous WMACs- one 5G and one 2G. 5G WMAC
> supports 160M, 8x8 configuration.
> FW supports 8 concurrent virtual interfaces on each WMAC.
>
> Patch introduces 2 new drivers- qtnfmac.ko for interfacing with
> kernel/cfg80211 and qtnfmac_pcie.ko for PCIe bus interface.
>
> Signed-off-by: Dmitrii Lebed 
> Signed-off-by: Sergei Maksimenko 
> Signed-off-by: Sergey Matyukevich 
> Signed-off-by: Bindu Therthala 
> Signed-off-by: Huizhao Wang 
> Signed-off-by: Kamlesh Rath 
> Signed-off-by: Avinash Patil 
> Signed-off-by: Igor Mitsyanko 

More comments:

> +/* FW names */
> +
> +#define QTN_PCI_FW_NAME  "pearl-linux.lzma.img"

The firmware name gives no indication what this file is about (remember
that linux-firmware.git has a lot of files). Please name it properly,
don't just use what is used in by firmware build scripts :) Take into
account also future hw support, all firmware files need to coexist in
the same repository without user invention. In a way the firmware
filename is part of kernel/userspace interface and needs to be stable.

For example, you could use something like "qtnfmac/qsr10g.img" (assuming
qsr10g is the name of chip).

I forgot already, is the firmware image ready for submission to
linux-firmware.git?

> + pr_info("%s: %sregistered mgmt frame type 0x%x\n", __func__,
> + reg ? "" : "un", frame_type);

The driver seems to be quite spammy with info messages:

qtnfmac/cfg80211.c: pr_info("%s: %sregistered mgmt frame type 0x%x\n", 
__func__,
qtnfmac/cfg80211.c: pr_info("QTNF: %s cipher=%x, idx=%u, pairwise=%u\n", 
__func__,
qtnfmac/cfg80211.c: pr_info("QTNF: %s idx=%u, pairwise=%u\n", __func__, 
key_index,
qtnfmac/cfg80211.c: pr_info("QTNF: %s idx=%u, unicast=%u, multicast=%u\n", 
__func__,
qtnfmac/cfg80211.c: pr_info("QTNF: %s idx=%u\n", __func__, key_index);
qtnfmac/cfg80211.c: pr_info("%s: initiator=%d, alpha=%c%c, macid=%d\n", 
__func__,
qtnfmac/cfg80211.c: pr_info("%s: MAX_IF: %zu; MODES: %.4X; RADAR WIDTHS: 
%.2X\n", __func__,
qtnfmac/cfg80211.c: pr_info("macid=%d, phymode=%#x\n", mac->macid, 
mac->macinfo.phymode);
qtnfmac/commands.c: pr_info("%s: unexpected TLV type: 
%.4X\n",
qtnfmac/commands.c: pr_info("%s: STA %pM not found\n", 
__func__, sta_mac);
qtnfmac/commands.c: pr_info("country-code from EP: %c%c\n", 
hwinfo->country_code[0],
qtnfmac/commands.c: pr_info("fw_version = %d, num_mac=%d, mac_bitmap=%#x\n",
qtnfmac/commands.c: pr_info("iface limit record 
count=%zu\n", record_count);
qtnfmac/commands.c: pr_info("MAC%d reported channels %d\n",
qtnfmac/init.c: pr_info("%s: macid=%d\n", __func__, macid);
qtnfmac/pcie.c: pr_info("enabled PCIE MSI interrupt\n");
qtnfmac/pcie.c: pr_info("%s: BAR[%u] vaddr=0x%p busaddr=0x%p len=%u\n",
qtnfmac/pcie.c: pr_info("%s: set mps to %d (was %d, max %d)\n",
qtnfmac/pcie.c: pr_info("fw download started: fw start addr = 0x%p, size=%d\n",
qtnfmac/pcie.c: pr_info("fw download completed: totally sent %d blocks\n", blk);
qtnfmac/pcie.c: pr_info("RC is ready to boot EP...\n");
qtnfmac/pcie.c: pr_info("starting download firmware %s...\n", bus->fwname);
qtnfmac/pcie.c: pr_info("successful init of PCI device %x\n", 
pdev->device);
qtnfmac/pcie.c: pr_info("Register Quantenna FullMAC PCIE driver\n");
qtnfmac/pcie.c: pr_info("Unregister Quantenna FullMAC PCIE driver\n");
qtnfmac/trans.c:pr_info("%s: interrupted\n", __func__);
qtnfmac/trans.c:pr_info("%s: skb dropped\n", __func__);

Usualle the preference is that driver is quiet until something goes
wrong. I hope some of these could be debug messages.

> --- /dev/null
> +++ b/drivers/net/wireless/quantenna/qtnfmac/pcie.c
> @@ -0,0 +1,1374 @@
> +/**
> + * Copyright (c) 2015-2016 Quantenna Communications, Inc.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + **/
> +
> +#undef DEBUG

Why?

-- 
Kalle Valo

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-08-01 Thread Johannes Berg 

> > 
> > > + /* bus private data */
> > > + char bus_priv[0];
> > You might want to - for future proofing - add some aligned()
> > attribute.
> > Otherwise, if struct mutex doesn't have a size that's a multiple of
> > the
> > pointer size, fields in here will not be aligned right.
> 
> Right, thanks for pointing this out. Probably even better approach
> here would be to take an object-oriented approach and make "bus" a
> base structure with pcie_bus an inheriting structure.

I think both ways to do composition are fine. It depends more on how
your other code is structured. I think in mac80211/cfg80211 we do both,
depending on where/how the allocation is done.

> > > +#define QLINK_HT_MCS_MASK_LEN10
> > > +#define QLINK_ETH_ALEN   6
> > > +#define QLINK_MAX_SSID_LEN   32
> > These seem a bit strange? Why bother? They are standard values.
> > (not entirely sure what the MCS_MASK_LEN is used for though)
> 
> The idea was to make protocol definition an independent entity, which
> does not depend on actual values which happened to be used in current
> kernel. And so that we can be sure that both sides of protocol 
> communication entities use the same length values.
> 
> We actually had discussions regarding this locally too: it was not
> clear whether we should use existing Linux definitions for standard
> things like MAC length, 802.11 header fields, IEs definitions etc. It
> does make a lot of sense to think that they will never change and can
> not be different on opposite sides of protocol, but we went with
> approach to define everything manually.

Well, I can kinda see the beginning of that argument "we should own
everything to make sure", but it doesn't really work that way. The max
SSID that's passed in, or the ETH_ALEN that's there - you're always
going to rely on the kernel's definition of this in other ways, say
when you alloc_ether_dev() [or whatever it's called], etc.

The end result is that you're never going to be able to change one of
the two and expect the combined system to still work. As a consequence,
I don't see any point in even trying to separate them.

> > > +struct qlink_vht_cap {

> Ok, that makes sense of course. Will discuss this locally.

Same argument applies here too, btw, and you might even have to
translate between this version and the one coming from say cfg80211 -
where the ieee80211.h one is used - and then you can realistically only
do a memcpy(), so you rely on them being the same anyway.

> > > + memcpy(_cfg->crypto, >crypto, sizeof(bss_cfg-
> > > > crypto));
> > This makes no sense at all - you have to convert the format of this
> > somehow to make it work - at least endianness has to be fixed, even
> > if
> > you copied all of the cfg80211 struct.
> 
> In this particular place we're making a local copy of cfg80211 struct
> to  local data structure (per each virtual interface). Conversion
> before  sending to another side is done in qtnf_cmd_send_connect(),
> it is converted into struct qlink_auth_encr.

Ok. Somehow I thought this (bss_cfg) was sent to the device.


johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-07-11 Thread Igor Mitsyanko 

Hi Johannes,
thank for review and sorry for long delays in replies. A parallel thread 
with Dave a discussion was initiated regarding HW platform samples 
availability. Current driver implementation supports a newer QSR10G 
platform, which availability is limited due to the platform still being 
in development stage. We're working on supporting Quantenna's current 
QSR1000 platform too, what would be the best approach to this:
- have QSR1000 platform support ready, amend it to current patch and 
post it as a new patch revision (there should be a certain amount of new 
code);
- have current patch accepted by community first, and then post a new 
patch (adding new platform support) one on top of it.


More answers bellow.

On 06/21/2016 03:22 PM, Johannes Berg wrote:

Very brief review:


+/* */

That seems slightly odd.


Will remove.




+   /* bus private data */
+   char bus_priv[0];

You might want to - for future proofing - add some aligned() attribute.
Otherwise, if struct mutex doesn't have a size that's a multiple of the
pointer size, fields in here will not be aligned right.


Right, thanks for pointing this out. Probably even better approach here 
would be to take an object-oriented approach and make "bus" a base 
structure with pcie_bus an inheriting structure.





+static inline void *get_bus_priv(struct qtnf_bus *bus)
+{
+   if (WARN_ON(!bus)) {
+   pr_err("qtnfmac: invalid bus pointer!\n");
+   return NULL;

Better to just use "WARN(!bus, "qtnfmac: invalid bus pointer!\n");"

Also, for pr_* the "qtnfmac: " prefix should be done with pr_fmt, not
manually.


Ok




+#define QLINK_HT_MCS_MASK_LEN  10
+#define QLINK_ETH_ALEN 6
+#define QLINK_MAX_SSID_LEN 32

These seem a bit strange? Why bother? They are standard values.
(not entirely sure what the MCS_MASK_LEN is used for though)


The idea was to make protocol definition an independent entity, which 
does not depend on actual values which happened to be used in current 
kernel. And so that we can be sure that both sides of protocol 
communication entities use the same length values.


We actually had discussions regarding this locally too: it was not clear 
whether we should use existing Linux definitions for standard things 
like MAC length, 802.11 header fields, IEs definitions etc. It does make 
a lot of sense to think that they will never change and can not be 
different on opposite sides of protocol, but we went with approach to 
define everything manually.





+/*
+ * struct qlink_ht_mcs_info - MCS information
+ *
+ * See  ieee80211_mcs_info.
+ */
+struct qlink_ht_mcs_info {
+   u8 rx_mask[QLINK_HT_MCS_MASK_LEN];
+   __le16 rx_highest;
+   u8 tx_params;
+   u8 reserved[3];
+} __packed;
+
+/*
+ * struct qlink_ht_cap - HT capabilities
+ *
+ * "HT capabilities element", see  ieee80211_ht_cap.
+ */
+struct qlink_ht_cap {
+   struct qlink_ht_mcs_info mcs;
+   __le32 tx_BF_cap_info;
+   __le16 cap_info;
+   __le16 extended_ht_cap_info;
+   u8 ampdu_params_info;
+   u8 antenna_selection_info;
+} __packed;
+
+/*
+ * struct qlink_vht_mcs_info - VHT MCS information
+ *
+ * See  ieee80211_vht_mcs_info.
+ */
+struct qlink_vht_mcs_info {
+   __le16 rx_mcs_map;
+   __le16 rx_highest;
+   __le16 tx_mcs_map;
+   __le16 tx_highest;
+} __packed;
+
+/*
+ * struct qlink_vht_cap - VHT capabilities
+ *
+ * "VHT capabilities element", see  ieee80211_vht_cap.
+ */
+struct qlink_vht_cap {
+   __le32 vht_cap_info;
+   struct qlink_vht_mcs_info supp_mcs;
+} __packed;


I think you shouldn't duplicate these, there's no sane way they can
ever be changed in ieee80211.h


Ok, that makes sense of course. Will discuss this locally.





+enum qlink_iface_type {
+   QLINK_IFTYPE_AP = BIT(0),
+   QLINK_IFTYPE_STATION= BIT(1),
+   QLINK_IFTYPE_ADHOC  = BIT(2),
+   QLINK_IFTYPE_MONITOR= BIT(3),
+   QLINK_IFTYPE_WDS= BIT(4),
+};

Not sure how you use these, but BIT() doesn't make a lot of sense for
something that's mutually exclusive?


You're right, will change to simple enumeration.




+/**
+ * Copyright (c) 2015-2016 Quantenna Communications, Inc.
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ **/

You should probably not have those double asterisks since they're
reserved for kernel-doc.


Ok




+   if (qtnf_cmd_send_start_ap(vif)) {
+   pr_err("failed to issue start AP command\n");
+   return

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-06-21 Thread Johannes Berg 
Very brief review:

> +/* */

That seems slightly odd.

> + /* bus private data */
> + char bus_priv[0];

You might want to - for future proofing - add some aligned() attribute.
Otherwise, if struct mutex doesn't have a size that's a multiple of the
pointer size, fields in here will not be aligned right.

> +static inline void *get_bus_priv(struct qtnf_bus *bus)
> +{
> + if (WARN_ON(!bus)) {
> + pr_err("qtnfmac: invalid bus pointer!\n");
> + return NULL;

Better to just use "WARN(!bus, "qtnfmac: invalid bus pointer!\n");"

Also, for pr_* the "qtnfmac: " prefix should be done with pr_fmt, not
manually.

> +#define QLINK_HT_MCS_MASK_LEN10
> +#define QLINK_ETH_ALEN   6
> +#define QLINK_MAX_SSID_LEN   32

These seem a bit strange? Why bother? They are standard values.
(not entirely sure what the MCS_MASK_LEN is used for though)

> +/*
> + * struct qlink_ht_mcs_info - MCS information
> + *
> + * See  ieee80211_mcs_info.
> + */
> +struct qlink_ht_mcs_info {
> + u8 rx_mask[QLINK_HT_MCS_MASK_LEN];
> + __le16 rx_highest;
> + u8 tx_params;
> + u8 reserved[3];
> +} __packed;
> +
> +/*
> + * struct qlink_ht_cap - HT capabilities
> + *
> + * "HT capabilities element", see  ieee80211_ht_cap.
> + */
> +struct qlink_ht_cap {
> + struct qlink_ht_mcs_info mcs;
> + __le32 tx_BF_cap_info;
> + __le16 cap_info;
> + __le16 extended_ht_cap_info;
> + u8 ampdu_params_info;
> + u8 antenna_selection_info;
> +} __packed;
> +
> +/*
> + * struct qlink_vht_mcs_info - VHT MCS information
> + *
> + * See  ieee80211_vht_mcs_info.
> + */
> +struct qlink_vht_mcs_info {
> + __le16 rx_mcs_map;
> + __le16 rx_highest;
> + __le16 tx_mcs_map;
> + __le16 tx_highest;
> +} __packed;
> +
> +/*
> + * struct qlink_vht_cap - VHT capabilities
> + *
> + * "VHT capabilities element", see  ieee80211_vht_cap.
> + */
> +struct qlink_vht_cap {
> + __le32 vht_cap_info;
> + struct qlink_vht_mcs_info supp_mcs;
> +} __packed;


I think you shouldn't duplicate these, there's no sane way they can
ever be changed in ieee80211.h


> +enum qlink_iface_type {
> + QLINK_IFTYPE_AP = BIT(0),
> + QLINK_IFTYPE_STATION= BIT(1),
> + QLINK_IFTYPE_ADHOC  = BIT(2),
> + QLINK_IFTYPE_MONITOR= BIT(3),
> + QLINK_IFTYPE_WDS= BIT(4),
> +};

Not sure how you use these, but BIT() doesn't make a lot of sense for
something that's mutually exclusive?

> +/**
> + * Copyright (c) 2015-2016 Quantenna Communications, Inc.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + **/

You should probably not have those double asterisks since they're
reserved for kernel-doc.

> + if (qtnf_cmd_send_start_ap(vif)) {
> + pr_err("failed to issue start AP command\n");
> + return -EFAULT;
> + }
> +
> + if (!(vif->bss_status & QTNF_STATE_AP_START)) {
> + pr_err("failed to start AP operations in FW\n");
> + return -EFAULT;
> + }

This is strange - I'd expect send_start_ap() to not actually just send
it, but also wait for a response, and then it can return an error if
the flag didn't get set. If it doesn't, then it's racy, no?

> +static int
> +qtnf_connect(struct wiphy *wiphy, struct net_device *dev,
> +  struct cfg80211_connect_params *sme)
> +{
> + struct qtnf_vif *vif;
> + struct qtnf_bss_config *bss_cfg;
> +
> + vif = qtnf_netdev_get_priv(dev);
> + if (!vif) {
> + pr_err("core_attach: could not get valid vif
> pointer\n");
> + return -EFAULT;
> + }

It seems that you're overdoing the error checks a bit - I don't see how
this could possibly fail?

> + memcpy(_cfg->crypto, >crypto, sizeof(bss_cfg-
> >crypto));

This makes no sense at all - you have to convert the format of this
somehow to make it work - at least endianness has to be fixed, even if
you copied all of the cfg80211 struct.

[snip - lots of stuff I didn't really look at]

> +/* sysfs knobs: stats and other diagnistics */

I think you should not have these - maybe add those with separate
patches later that really can't be done otherwise, and then give very
good rationale for it. Having driver-specific sysfs is not a good idea
in general.

> +static inline u64 qtnf_get_unaligned_le64(const __le64 *ptr)
> +{
> + return le64_to_cpu(get_unaligned(ptr));
> +}
> +
> +static inline u32 qtnf_get_unaligned_le32(const __le32 *ptr)
> +{
> + return

Re: [PATCH v2 RESEND] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets

2016-06-20 Thread kbuild test robot 
Hi,

[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on v4.7-rc4 next-20160620]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/igor-mitsyanko-os-quantenna-com/qtnfmac-announcement-of-new-FullMAC-driver-for-Quantenna-chipsets/20160621-061419
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git 
master
config: m32r-allyesconfig (attached as .config)
compiler: m32r-linux-gcc (GCC) 4.9.0
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=m32r 

All errors (new ones prefixed by >>):

   In file included from drivers/net/wireless/quantenna/qtnfmac/core.c:23:0:
   drivers/net/wireless/quantenna/qtnfmac/pcie.h: In function 'align_buf_dma':
>> drivers/net/wireless/quantenna/qtnfmac/pcie.h:114:9: error: implicit 
>> declaration of function 'dma_get_cache_alignment' 
>> [-Werror=implicit-function-declaration]
dma_get_cache_alignment());
^
   cc1: some warnings being treated as errors

vim +/dma_get_cache_alignment +114 drivers/net/wireless/quantenna/qtnfmac/pcie.h

   108  }
   109  
   110  static __always_inline void *
   111  align_buf_dma(void *addr)
   112  {
   113  return (void *)align_val_up((unsigned long)addr,
 > 114  dma_get_cache_alignment());
   115  }
   116  
   117  static __always_inline unsigned long

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data