RE: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
> Having the firmware files under plfxlc/ directory looks good to me. But I'm > not really a fan of upper case filenames, is there a reason for that? I would > prefer have all lowercase filenames. Thanks for your suggestions. We have renamed the firmware names to lower-case and will submit v14 Thanks Srini
RE: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
> That wasn't my point. My point was that the kernel code trusts the validity > of the firmware image, in the sense of e.g. this piece: >> + no_of_files = *(u32 *)_packed->data[0]; > If the firmware file was corrupted (intentionally/maliciously or not), this > could now be say 0x. Thanks for the clarification, We will submit next patch with additional validations to this > What are your reasons for piggy-backing on 2.4 GHz? Just practical "it's > there and we don't care"? As the LiFi is not standardised yet we are using the existing wireless frameworks. For now piggy-backing with 2.4GHz is seamless for users. We will undertake band and other wider change once IEEE 802.11bb is standardised. Thanks Srini
Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
On Fri, 2021-02-19 at 05:15 +, Srinivasan Raju wrote:
> Hi,
>
> Please find a few responses to the comments , We will fix rest of the
> comments and submit v14 of the patch.
>
> > Also, you *really* need some validation here, rather than blindly
> > trusting that the file is well-formed, otherwise you immediately have a
> > security issue here.
>
> The firmware is signed and the harware validates the signature , so we are
> not validating in the software.
That wasn't my point. My point was that the kernel code trusts the
validity of the firmware image, in the sense of e.g. this piece:
> + no_of_files = *(u32 *)_packed->data[0];
If the firmware file was corrupted (intentionally/maliciously or not), this
could now be say 0x.
> + for (step = 0; step < no_of_files; step++) {
> + start_addr = *(u32 *)_packed->data[4 + (step * 4)];
But you trust it completely and don't even check that "4 + (step * 4)"
fits into the length of the data!
That's what I meant. Don't allow this to crash the kernel.
> > > +static const struct plf_reg_alpha2_map reg_alpha2_map[] = {
> > > + { PLF_REGDOMAIN_FCC, "US" },
> > > + { PLF_REGDOMAIN_IC, "CA" },
> > > + { PLF_REGDOMAIN_ETSI, "DE" }, /* Generic ETSI, use most restrictive
> > > */
> > > + { PLF_REGDOMAIN_JAPAN, "JP" },
> > > + { PLF_REGDOMAIN_SPAIN, "ES" },
> > > + { PLF_REGDOMAIN_FRANCE, "FR" },
> > > +};
> > You actually have regulatory restrictions on this stuff?
>
> Currently, There are no regulatory restrictions applicable for LiFi ,
> regulatory_hint setting is only for wifi core
So why do you have PLF_REGDOMAIN_* things? What does that even mean
then?
> > OTOH, I can sort of see why/how you're reusing wifi functionality here,
> > very old versions of wifi even had an infrared PHY I think.
> >
> > Conceptually, it seems odd. Perhaps we should add a new band definition?
> >
> > And what I also asked above - how much of the rate stuff is completely
> > fake? Are you really doing CCK/OFDM in some (strange?) way?
>
> Yes, your understanding is correct, and we use OFDM.
> For now we will use the existing band definition.
OFDM over infrared, nice.
Still, I'm not convinced we should piggy-back this on 2.4 GHz.
NL80211_BAND_1THZ? ;-)
Ok, I don't know what wavelength you're using, of course...
But at least thinking about this, wouldn't we be better off if we define
NL80211_BAND_INFRARED or something? That way, at least we can tell that
it's not going to interoperate with real 2.4 GHz, etc.
OTOH, this is a bit of a slippery slow - what if somebody else designs
an *incompatible* infrared PHY? I guess this is not really standardised
at this point.
Not really sure about all this, but I guess we need to think about it
more.
What are your reasons for piggy-backing on 2.4 GHz? Just practical "it's
there and we don't care"?
johannes
Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
Hi,
Please find a few responses to the comments , We will fix rest of the comments
and submit v14 of the patch.
> Also, you *really* need some validation here, rather than blindly
> trusting that the file is well-formed, otherwise you immediately have a
> security issue here.
The firmware is signed and the harware validates the signature , so we are not
validating in the software.
>> +static const struct plf_reg_alpha2_map reg_alpha2_map[] = {
>> + { PLF_REGDOMAIN_FCC, "US" },
>> + { PLF_REGDOMAIN_IC, "CA" },
>> + { PLF_REGDOMAIN_ETSI, "DE" }, /* Generic ETSI, use most restrictive */
>> + { PLF_REGDOMAIN_JAPAN, "JP" },
>> + { PLF_REGDOMAIN_SPAIN, "ES" },
>> + { PLF_REGDOMAIN_FRANCE, "FR" },
>> +};
> You actually have regulatory restrictions on this stuff?
Currently, There are no regulatory restrictions applicable for LiFi ,
regulatory_hint setting is only for wifi core
>> +static const struct ieee80211_rate purelifi_rates[] = {
>> + { .bitrate = 10,
>> + .hw_value = PURELIFI_CCK_RATE_1M,
>> + .flags = 0 },
>> + { .bitrate = 20,
>> + .hw_value = PURELIFI_CCK_RATE_2M,
>> + .hw_value_short = PURELIFI_CCK_RATE_2M
>> + | PURELIFI_CCK_PREA_SHORT,
>> + .flags = IEEE80211_RATE_SHORT_PREAMBLE },
>> + { .bitrate = 55,
>> + .hw_value = PURELIFI_CCK_RATE_5_5M,
>> + .hw_value_short = PURELIFI_CCK_RATE_5_5M
>> + | PURELIFI_CCK_PREA_SHORT,
>> + .flags = IEEE80211_RATE_SHORT_PREAMBLE },
>> + { .bitrate = 110,
>> + .hw_value = PURELIFI_CCK_RATE_11M,
>> + .hw_value_short = PURELIFI_CCK_RATE_11M
>> + | PURELIFI_CCK_PREA_SHORT,
>> + .flags = IEEE80211_RATE_SHORT_PREAMBLE },
> So ... how much of that is completely fake? Are you _actually_ doing 1,
> 2, 5.5 etc. Mbps over the air, and you even have short and long
> preamble? Why do all of that legacy mess, when you're essentially
> greenfield??
Yes, this not used. We will test and remove the unused legacy settings
> OTOH, I can sort of see why/how you're reusing wifi functionality here,
> very old versions of wifi even had an infrared PHY I think.
>
> Conceptually, it seems odd. Perhaps we should add a new band definition?
>
> And what I also asked above - how much of the rate stuff is completely
> fake? Are you really doing CCK/OFDM in some (strange?) way?
Yes, your understanding is correct, and we use OFDM.
For now we will use the existing band definition.
Thanks
Srini
____________
From: Johannes Berg
Sent: Friday, February 12, 2021 7:14 PM
To: Srinivasan Raju
Cc: Mostafa Afgani; Kalle Valo; David S. Miller; Jakub Kicinski; open list;
open list:NETWORKING DRIVERS (WIRELESS); open list:NETWORKING DRIVERS
Subject: Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA
devices
Hi,
Thanks for your patience, and thanks for sticking around!
I'm sorry I haven't reviewed this again in a long time, but I was able
to today.
> +PUREILIFI USB DRIVER
Did you mistype "PURELIFI" here, or was that intentional?
> +PUREILIFI USB DRIVER
> +M: Srinivasan Raju
Probably would be good to have an "L" entry with the linux-wireless list
here.
> +if WLAN_VENDOR_PURELIFI
> +
> +source "drivers/net/wireless/purelifi/plfxlc/Kconfig"
Seems odd to have the Makefile under purelifi/ but the Kconfig is yet
another directory deeper?
> +++ b/drivers/net/wireless/purelifi/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_WLAN_VENDOR_PURELIFI) := plfxlc/
Although this one doesn't do anything, so all you did was save a level
of Kconfig inclusion I guess ... no real objection to that.
> diff --git a/drivers/net/wireless/purelifi/plfxlc/Kconfig
> b/drivers/net/wireless/purelifi/plfxlc/Kconfig
> new file mode 100644
> index ..07a65c0fce68
> --- /dev/null
> +++ b/drivers/net/wireless/purelifi/plfxlc/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config PLFXLC
> +
> + tristate "pureLiFi X, XL, XC device support"
extra blank line.
Also, maybe that should be a bit more verbose? PURELIFI_XLC or so? I
don't think it shows up in many places, but if you see "PLFXLC"
somewhere that's not very helpful.
> + depends on CFG80211 && MAC80211 && USB
> + help
> +This driver makes the adapter appear as a normal WLAN interface.
> +
> +The pureLiFi device requires external STA firmware to be
Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
Srinivasan Raju writes:
> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
> and LiFi-XL USB devices.
>
> This driver implementation has been based on the zd1211rw driver.
>
> Driver is based on 802.11 softMAC Architecture and uses
> native 802.11 for configuration and management.
>
> The driver is compiled and tested in ARM, x86 architectures and
> compiled in powerpc architecture.
>
> Signed-off-by: Srinivasan Raju
[...]
> + if ((le16_to_cpu(udev->descriptor.idVendor) ==
> + PURELIFI_X_VENDOR_ID_0) &&
> + (le16_to_cpu(udev->descriptor.idProduct) ==
> + PURELIFI_X_PRODUCT_ID_0)) {
> + fw_name = "plfxlc/LiFi-X.bin";
> + dev_dbg(>dev, "bin file for X selected\n");
> +
> + } else if ((le16_to_cpu(udev->descriptor.idVendor)) ==
> + PURELIFI_XC_VENDOR_ID_0 &&
> +(le16_to_cpu(udev->descriptor.idProduct) ==
> + PURELIFI_XC_PRODUCT_ID_0)) {
> + fw_name = "plfxlc/LiFi-XC.bin";
> + dev_dbg(>dev, "bin file for XC selected\n");
> +
> + } else {
> + r = -EINVAL;
> + goto error;
> + }
> +
> + r = request_firmware((const struct firmware **), fw_name,
> + >dev);
> + if (r) {
> + dev_err(>dev, "request_firmware failed (%d)\n", r);
> + goto error;
> + }
[...]
> + /* Code for single pack file download */
> +
> + fw_pack = "plfxlc/LiFi-XL.bin";
> +
> + r = request_firmware((const struct firmware **)_packed, fw_pack,
> + >dev);
> + if (r) {
> + dev_err(>dev, "request_firmware failed (%d)\n", r);
> + goto error;
> + }
Having the firmware files under plfxlc/ directory looks good to me. But
I'm not really a fan of upper case filenames, is there a reason for
that? I would prefer have all lowercase filenames.
Also the cast (const struct firmware **) looks very wrong, but I didn't
investigate why you do it. I'm sure there is a proper way to implement
whatever you need here, without the cast.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
> Ah, kbuild bot had already reported few issues: > https://patchwork.kernel.org/project/linux-wireless/patch/20210212115030.124490-1-srini.r...@purelifi.com/ > Please fix those and I recommend waiting few days in case the bot finds > more issues. After that you can submitt v14 fixing the comments you got > in this review round. Hi Kalle, Thanks for reviewing the patch. We will fix / address the comments and resubmit v14 Thanks Srini -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
Kalle Valo writes: > Srinivasan Raju writes: > >> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC >> and LiFi-XL USB devices. >> >> This driver implementation has been based on the zd1211rw driver. >> >> Driver is based on 802.11 softMAC Architecture and uses >> native 802.11 for configuration and management. >> >> The driver is compiled and tested in ARM, x86 architectures and >> compiled in powerpc architecture. >> >> Signed-off-by: Srinivasan Raju > > I pushed this now to the pending branch of wireless-drivers-next for > build testing, let's see what kind of results we get. Ah, kbuild bot had already reported few issues: https://patchwork.kernel.org/project/linux-wireless/patch/20210212115030.124490-1-srini.r...@purelifi.com/ Please fix those and I recommend waiting few days in case the bot finds more issues. After that you can submitt v14 fixing the comments you got in this review round. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
Srinivasan Raju writes: > This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC > and LiFi-XL USB devices. > > This driver implementation has been based on the zd1211rw driver. > > Driver is based on 802.11 softMAC Architecture and uses > native 802.11 for configuration and management. > > The driver is compiled and tested in ARM, x86 architectures and > compiled in powerpc architecture. > > Signed-off-by: Srinivasan Raju [...] > + send_vendor_command(udev, PLF_VNDR_FPGA_STATE_CMD, NULL, 0); > + > + msleep(200); There are multiple msleeps like this, please add a descriptive define for value 200. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
Johannes Berg writes: >> +++ b/drivers/net/wireless/purelifi/Makefile >> @@ -0,0 +1,2 @@ >> +# SPDX-License-Identifier: GPL-2.0-only >> +obj-$(CONFIG_WLAN_VENDOR_PURELIFI) := plfxlc/ > > Although this one doesn't do anything, so all you did was save a level > of Kconfig inclusion I guess ... no real objection to that. I think this should use obj-$(CONFIG_PLFXLC). -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
Srinivasan Raju writes: > This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC > and LiFi-XL USB devices. > > This driver implementation has been based on the zd1211rw driver. > > Driver is based on 802.11 softMAC Architecture and uses > native 802.11 for configuration and management. > > The driver is compiled and tested in ARM, x86 architectures and > compiled in powerpc architecture. > > Signed-off-by: Srinivasan Raju I pushed this now to the pending branch of wireless-drivers-next for build testing, let's see what kind of results we get. I didn't manage to do a thorough review yet, but few quick comments: > --- a/drivers/net/wireless/Kconfig > +++ b/drivers/net/wireless/Kconfig > @@ -35,6 +35,7 @@ source "drivers/net/wireless/st/Kconfig" > source "drivers/net/wireless/ti/Kconfig" > source "drivers/net/wireless/zydas/Kconfig" > source "drivers/net/wireless/quantenna/Kconfig" > +source "drivers/net/wireless/purelifi/Kconfig" This is supposed to be in alphabetical order. > --- a/drivers/net/wireless/Makefile > +++ b/drivers/net/wireless/Makefile > @@ -20,6 +20,7 @@ obj-$(CONFIG_WLAN_VENDOR_ST) += st/ > obj-$(CONFIG_WLAN_VENDOR_TI) += ti/ > obj-$(CONFIG_WLAN_VENDOR_ZYDAS) += zydas/ > obj-$(CONFIG_WLAN_VENDOR_QUANTENNA) += quantenna/ > +obj-$(CONFIG_WLAN_VENDOR_PURELIFI) += purelifi/ And this one as well. Clearly I missed these with quantenna, but no need to redo the same mistake with purelife. Also patches welcome to fix quantenna sorting. > --- /dev/null > +++ b/drivers/net/wireless/purelifi/plfxlc/Kconfig > @@ -0,0 +1,13 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +config PLFXLC > + > + tristate "pureLiFi X, XL, XC device support" > + depends on CFG80211 && MAC80211 && USB > + help > +This driver makes the adapter appear as a normal WLAN interface. > + > +The pureLiFi device requires external STA firmware to be loaded. > + The documentation here is not telling much. I think it goes without saying that a firmware is needed, so no need to mention it. And also "normal WLAN interface" is a standard upstream requirement so drop that as well. Instead describe here in detail what hardware this driver supports, for example exact product names, bus types, wifi/ieee generation etc. > +To compile this driver as a module, choose M here: the module will > +be called purelifi. Didn't you rename it to plfxlc? -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
Hi Srinivasan,
I love your patch! Yet something to improve:
[auto build test ERROR on wireless-drivers-next/master]
[also build test ERROR on wireless-drivers/master net-next/master net/master
linus/master sparc-next/master v5.11-rc7 next-20210211]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Srinivasan-Raju/wireless-Initial-driver-submission-for-pureLiFi-STA-devices/20210212-195451
base:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git
master
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/5d8fb5ce1f136940c10fd16bc96856d2f6ad2741
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Srinivasan-Raju/wireless-Initial-driver-submission-for-pureLiFi-STA-devices/20210212-195451
git checkout 5d8fb5ce1f136940c10fd16bc96856d2f6ad2741
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
In file included from include/linux/kernel.h:10,
from drivers/net/wireless/purelifi/plfxlc/usb.c:6:
include/linux/scatterlist.h: In function 'sg_set_buf':
arch/m68k/include/asm/page_mm.h:174:49: warning: ordered comparison of
pointer with null pointer [-Wextra]
174 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void
*)PAGE_OFFSET && (void *)(kaddr) < high_memory)
| ^~
include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
include/linux/scatterlist.h:137:2: note: in expansion of macro 'BUG_ON'
137 | BUG_ON(!virt_addr_valid(buf));
| ^~
include/linux/scatterlist.h:137:10: note: in expansion of macro
'virt_addr_valid'
137 | BUG_ON(!virt_addr_valid(buf));
| ^~~
drivers/net/wireless/purelifi/plfxlc/usb.c: At top level:
>> drivers/net/wireless/purelifi/plfxlc/usb.c:962:20: error: 'suspend'
>> undeclared here (not in a function)
962 | .suspend= suspend,
|^~~
>> drivers/net/wireless/purelifi/plfxlc/usb.c:963:20: error: 'resume'
>> undeclared here (not in a function); did you mean 'resource'?
963 | .resume = resume,
|^~
|resource
drivers/net/wireless/purelifi/plfxlc/usb.c:909:29: warning:
'get_purelifi_usb' defined but not used [-Wunused-function]
909 | static struct purelifi_usb *get_purelifi_usb(struct usb_interface
*intf)
| ^~~~
vim +/suspend +962 drivers/net/wireless/purelifi/plfxlc/usb.c
954
955 static struct usb_driver driver = {
956 .name= KBUILD_MODNAME,
957 .id_table= usb_ids,
958 .probe= probe,
959 .disconnect= disconnect,
960 .pre_reset= pre_reset,
961 .post_reset= post_reset,
> 962 .suspend= suspend,
> 963 .resume = resume,
964 .disable_hub_initiated_lpm = 1,
965 };
966
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
Hi Srinivasan,
I love your patch! Perhaps something to improve:
[auto build test WARNING on wireless-drivers-next/master]
[also build test WARNING on wireless-drivers/master net-next/master net/master
linus/master sparc-next/master v5.11-rc7 next-20210211]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Srinivasan-Raju/wireless-Initial-driver-submission-for-pureLiFi-STA-devices/20210212-195451
base:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git
master
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/5d8fb5ce1f136940c10fd16bc96856d2f6ad2741
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Srinivasan-Raju/wireless-Initial-driver-submission-for-pureLiFi-STA-devices/20210212-195451
git checkout 5d8fb5ce1f136940c10fd16bc96856d2f6ad2741
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
drivers/net/wireless/purelifi/plfxlc/mac.c: In function
'purelifi_mac_tx_status':
>> drivers/net/wireless/purelifi/plfxlc/mac.c:219:19: warning: variable 'retry'
>> set but not used [-Wunused-but-set-variable]
219 | int success = 1, retry = 1;
| ^
vim +/retry +219 drivers/net/wireless/purelifi/plfxlc/mac.c
198
199 /**
200 * purelifi_mac_tx_status - reports tx status of a packet if required
201 * @hw - a ieee80211_hw pointer
202 * @skb - a sk-buffer
203 * @flags: extra flags to set in the TX status info
204 * @ackssi: ACK signal strength
205 * @success - True for successful transmission of the frame
206 *
207 * This information calls ieee80211_tx_status_irqsafe() if required by
the
208 * control information. It copies the control information into the
status
209 * information.
210 *
211 * If no status information has been requested, the skb is freed.
212 */
213 static void purelifi_mac_tx_status(struct ieee80211_hw *hw,
214 struct sk_buff *skb,
215 int ackssi,
216 struct tx_status *tx_status)
217 {
218 struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> 219 int success = 1, retry = 1;
220
221 ieee80211_tx_info_clear_status(info);
222
223 if (tx_status) {
224 success = !tx_status->failure;
225 retry = tx_status->retry + success;
226 }
227
228 if (success)
229 info->flags |= IEEE80211_TX_STAT_ACK;
230 else
231 info->flags &= ~IEEE80211_TX_STAT_ACK;
232
233 info->status.ack_signal = 50;
234 ieee80211_tx_status_irqsafe(hw, skb);
235 }
236
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
Re: [PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
Hi,
Thanks for your patience, and thanks for sticking around!
I'm sorry I haven't reviewed this again in a long time, but I was able
to today.
> +PUREILIFI USB DRIVER
Did you mistype "PURELIFI" here, or was that intentional?
> +PUREILIFI USB DRIVER
> +M: Srinivasan Raju
Probably would be good to have an "L" entry with the linux-wireless list
here.
> +if WLAN_VENDOR_PURELIFI
> +
> +source "drivers/net/wireless/purelifi/plfxlc/Kconfig"
Seems odd to have the Makefile under purelifi/ but the Kconfig is yet
another directory deeper?
> +++ b/drivers/net/wireless/purelifi/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_WLAN_VENDOR_PURELIFI) := plfxlc/
Although this one doesn't do anything, so all you did was save a level
of Kconfig inclusion I guess ... no real objection to that.
> diff --git a/drivers/net/wireless/purelifi/plfxlc/Kconfig
> b/drivers/net/wireless/purelifi/plfxlc/Kconfig
> new file mode 100644
> index ..07a65c0fce68
> --- /dev/null
> +++ b/drivers/net/wireless/purelifi/plfxlc/Kconfig
> @@ -0,0 +1,13 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config PLFXLC
> +
> + tristate "pureLiFi X, XL, XC device support"
extra blank line.
Also, maybe that should be a bit more verbose? PURELIFI_XLC or so? I
don't think it shows up in many places, but if you see "PLFXLC"
somewhere that's not very helpful.
> + depends on CFG80211 && MAC80211 && USB
> + help
> +This driver makes the adapter appear as a normal WLAN interface.
> +
> +The pureLiFi device requires external STA firmware to be loaded.
> +
> +To compile this driver as a module, choose M here: the module will
> +be called purelifi.
But will it? Seems like it would be called "plfxlc"?
See here:
> +++ b/drivers/net/wireless/purelifi/plfxlc/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_PLFXLC) := plfxlc.o
> +plfxlc-objs += chip.o firmware.o usb.o mac.o
> +int purelifi_set_beacon_interval(struct purelifi_chip *chip, u16 interval,
> + u8 dtim_period, int type)
> +{
> + if (!interval ||
> + (chip->beacon_set && chip->beacon_interval == interval)) {
> + return 0;
> + }
Don't need braces here
> + chip->beacon_interval = interval;
> + chip->beacon_set = true;
> + return usb_write_req((const u8 *)>beacon_interval,
> + sizeof(chip->beacon_interval),
> + USB_REQ_BEACON_INTERVAL_WR);
There's clearly an endian problem hiding here somewhere. You should have
"chip->beacon_interval" be (probably) __le16 since you send it to the
device as a buffer, yet the parameter to this function is just "u16".
Therefore, a conversion is missing somewhere - likely you need it to be
__le16 in the chip struct since you can't send USB requests from stack.
You should add some annotations for things getting sent to the device
like this, and then you can run sparse to validate them all over the
code.
> +}
> +
> +int purelifi_chip_init_hw(struct purelifi_chip *chip)
> +{
> + u8 *addr = purelifi_mac_get_perm_addr(purelifi_chip_to_mac(chip));
> + struct usb_device *udev = interface_to_usbdev(chip->usb.intf);
> +
> + pr_info("purelifi chip %02x:%02x v%02x %02x-%02x-%02x %s\n",
"%04x:%04x" for the USB ID?
And you should probably use %pM for the MAC address - the format you
have there looks ... strange? Why only print the vendor OUI?
> +int purelifi_chip_switch_radio(struct purelifi_chip *chip, u32 value)
> +{
> + int r;
> +
> + r = usb_write_req((const u8 *), sizeof(value), USB_REQ_POWER_WR);
> + if (r)
> + dev_err(purelifi_chip_dev(chip), "POWER_WR failed (%d)\n", r);
Same endian problem here.
>
> +static inline void purelifi_mc_add_addr(struct purelifi_mc_hash *hash,
> + u8 *addr)
> +{
> + unsigned int i = addr[5] >> 2;
> +
> + if (i < 32)
> + hash->low |= 1 << i;
> + else
> + hash->high |= 1 << (i - 32);
That's UB if i == 31, you need 1U << i, or just use the BIT() macro.
> + r = request_firmware((const struct firmware **), fw_name,
Not sure why that cast should be required?
> + fpga_dmabuff = NULL;
> + fpga_dmabuff = kmalloc(PLF_FPGA_SLEN, GFP_KERNEL);
that NULL assignment is pointless :)
> +
> + if (!fpga_dmabuff) {
> + r = -ENOMEM;
> + goto error_free_fw;
> + }
> + send_vendor_request(udev, PLF_VNDR_FPGA_SET_REQ, fpga_dmabuff,
> sizeof(fpga_setting));
> + memcpy(fpga_setting, fpga_dmabuff, PLF_FPGA_SLEN);
> + kfree(fpga_dmabuff);
I'd say you should remove fpga_setting from the stack since you anyway
allocated it, save the memcpy() to the stack, and just use fpga_dmabuff
below - and then free it later?
> + for (tbuf_idx = 0; tbuf_idx < blk_tran_len; tbuf_idx++) {
> + /* u8
[PATCH] [v13] wireless: Initial driver submission for pureLiFi STA devices
This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC and LiFi-XL USB devices. This driver implementation has been based on the zd1211rw driver. Driver is based on 802.11 softMAC Architecture and uses native 802.11 for configuration and management. The driver is compiled and tested in ARM, x86 architectures and compiled in powerpc architecture. Signed-off-by: Srinivasan Raju --- v13 - Removed unused #defines v12: - Removed sysfs, procfs related code - Addressed race condition bug - Used macros instead of magic numbers in firmware.c - Added copyright in all files v11, v10: - Addressed review comment on readability - Changed firmware names to match products v9: - Addressed review comments on style and content defects - Used kmemdup instead of alloc and memcpy v7 , v8: - Magic numbers removed and used IEEE80211 macors - Other code style and timer function fixes (mod_timer) v6: - Code style fix patch from Joe Perches v5: - Code refactoring for clarity and redundnacy removal - Fix warnings from kernel test robot v4: - Code refactoring based on kernel code guidelines - Remove multi level macors and use kernel debug macros v3: - Code style fixes kconfig fix v2: - Driver submitted to wireless-next - Code style fixes and copyright statement fix v1: - Driver submitted to staging --- MAINTAINERS |5 + drivers/net/wireless/Kconfig |1 + drivers/net/wireless/Makefile |1 + drivers/net/wireless/purelifi/Kconfig | 17 + drivers/net/wireless/purelifi/Makefile|2 + drivers/net/wireless/purelifi/plfxlc/Kconfig | 13 + drivers/net/wireless/purelifi/plfxlc/Makefile |3 + drivers/net/wireless/purelifi/plfxlc/chip.c | 96 ++ drivers/net/wireless/purelifi/plfxlc/chip.h | 84 ++ .../net/wireless/purelifi/plfxlc/firmware.c | 290 + drivers/net/wireless/purelifi/plfxlc/intf.h | 41 + drivers/net/wireless/purelifi/plfxlc/mac.c| 876 ++ drivers/net/wireless/purelifi/plfxlc/mac.h| 192 drivers/net/wireless/purelifi/plfxlc/usb.c| 1009 + drivers/net/wireless/purelifi/plfxlc/usb.h| 177 +++ 15 files changed, 2807 insertions(+) create mode 100644 drivers/net/wireless/purelifi/Kconfig create mode 100644 drivers/net/wireless/purelifi/Makefile create mode 100644 drivers/net/wireless/purelifi/plfxlc/Kconfig create mode 100644 drivers/net/wireless/purelifi/plfxlc/Makefile create mode 100644 drivers/net/wireless/purelifi/plfxlc/chip.c create mode 100644 drivers/net/wireless/purelifi/plfxlc/chip.h create mode 100644 drivers/net/wireless/purelifi/plfxlc/firmware.c create mode 100644 drivers/net/wireless/purelifi/plfxlc/intf.h create mode 100644 drivers/net/wireless/purelifi/plfxlc/mac.c create mode 100644 drivers/net/wireless/purelifi/plfxlc/mac.h create mode 100644 drivers/net/wireless/purelifi/plfxlc/usb.c create mode 100644 drivers/net/wireless/purelifi/plfxlc/usb.h diff --git a/MAINTAINERS b/MAINTAINERS index 5c1a6ba5ef26..3178ded0a91e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14199,6 +14199,11 @@ T: git git://linuxtv.org/media_tree.git F: Documentation/admin-guide/media/pulse8-cec.rst F: drivers/media/cec/usb/pulse8/ +PUREILIFI USB DRIVER +M: Srinivasan Raju +S: Supported +F: drivers/net/wireless/purelifi + PVRUSB2 VIDEO4LINUX DRIVER M: Mike Isely L: pvru...@isely.net (subscribers-only) diff --git a/drivers/net/wireless/Kconfig b/drivers/net/wireless/Kconfig index 7add2002ff4c..3a1f5ef3aa43 100644 --- a/drivers/net/wireless/Kconfig +++ b/drivers/net/wireless/Kconfig @@ -35,6 +35,7 @@ source "drivers/net/wireless/st/Kconfig" source "drivers/net/wireless/ti/Kconfig" source "drivers/net/wireless/zydas/Kconfig" source "drivers/net/wireless/quantenna/Kconfig" +source "drivers/net/wireless/purelifi/Kconfig" config PCMCIA_RAYCS tristate "Aviator/Raytheon 2.4GHz wireless support" diff --git a/drivers/net/wireless/Makefile b/drivers/net/wireless/Makefile index 80b324499786..e9fc770026f0 100644 --- a/drivers/net/wireless/Makefile +++ b/drivers/net/wireless/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_WLAN_VENDOR_ST) += st/ obj-$(CONFIG_WLAN_VENDOR_TI) += ti/ obj-$(CONFIG_WLAN_VENDOR_ZYDAS) += zydas/ obj-$(CONFIG_WLAN_VENDOR_QUANTENNA) += quantenna/ +obj-$(CONFIG_WLAN_VENDOR_PURELIFI) += purelifi/ # 16-bit wireless PCMCIA client drivers obj-$(CONFIG_PCMCIA_RAYCS) += ray_cs.o diff --git a/drivers/net/wireless/purelifi/Kconfig b/drivers/net/wireless/purelifi/Kconfig new file mode 100644 index ..e39afec3dcae --- /dev/null +++ b/drivers/net/wireless/purelifi/Kconfig @@ -0,0 +1,17 @@ +# SPDX-License-Identifier: GPL-2.0-only +config WLAN_VENDOR_PURELIFI + bool "pureLiFi devices" + default y + help + If you have a pureLiFi device, say Y. + + Note that the answer to this question doesn't directly affect the + kernel: saying N will
