The r8188eu kernel module does not depend on the rtlwifi/rtl8188eufw.bin firmware file

The r8188eu kernel module does not depend on the rtlwifi/rtl8188eufw.bin firmware file

[Grzegorz Szymaszek]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]

Dear r8188eu Maintainers,

The old rtl8188eu kernel module, removed in v5.15[1][2], indicated that
it requires the rtlwifi/rtl8188eufw.bin firmware file[3]. The new
r8188eu driver no longer does so.

I don’t know if it should be considered a regression or just a different
behaviour of the two drivers. I’ve noticed it[4] when I tried to use an
RTL8188EU‐based card in the initramfs of two different Ubuntu kernels:
v5.4 and v5.15. In v5.4, the firmware would be automatically included
when the (old) driver was included, whereas in v5.15 I would have to add
it manually so that the card actually worked. (One can verify the active
driver’s requirements using “modinfo -F firmware r8188eu”.)

If there are cards the new driver supports that do not need that
firmware file, it makes sense to not automatically include it. In
general, I don’t know the kernel policy on such dependencies.

[1]: commit 55dfa29b43d23bab37d98f087615ff46d38241df
[2]: https://lore.kernel.org/all/20210731133809.196681-1-phil@philpotter.co.uk/
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/rtl8188eu/os_dep/os_intfs.c?id=06889446a78fb9655332954a2288ecbacc7f0ff8#n22
[4]: https://answers.launchpad.net/ubuntu/+source/linux-meta-hwe-5.15/+question/702611

Best regards

-- 
Grzegorz Szymaszek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: The r8188eu kernel module does not depend on the rtlwifi/rtl8188eufw.bin firmware file

[Greg KH]

On Tue, Aug 02, 2022 at 03:17:58PM +0200, Grzegorz Szymaszek wrote:
> Dear r8188eu Maintainers,
> 
> The old rtl8188eu kernel module, removed in v5.15[1][2], indicated that
> it requires the rtlwifi/rtl8188eufw.bin firmware file[3]. The new
> r8188eu driver no longer does so.
> 
> I don’t know if it should be considered a regression or just a different
> behaviour of the two drivers. I’ve noticed it[4] when I tried to use an
> RTL8188EU‐based card in the initramfs of two different Ubuntu kernels:
> v5.4 and v5.15. In v5.4, the firmware would be automatically included
> when the (old) driver was included, whereas in v5.15 I would have to add
> it manually so that the card actually worked. (One can verify the active
> driver’s requirements using “modinfo -F firmware r8188eu”.)
> 
> If there are cards the new driver supports that do not need that
> firmware file, it makes sense to not automatically include it. In
> general, I don’t know the kernel policy on such dependencies.
> 
> [1]: commit 55dfa29b43d23bab37d98f087615ff46d38241df
> [2]: https://lore.kernel.org/all/20210731133809.196681-1-phil@philpotter.co.uk/
> [3]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/rtl8188eu/os_dep/os_intfs.c?id=06889446a78fb9655332954a2288ecbacc7f0ff8#n22
> [4]: https://answers.launchpad.net/ubuntu/+source/linux-meta-hwe-5.15/+question/702611

Looks like someone needs to add a line to the driver that looks like:
	MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
so that the tools will automatically pick it up properly going forward.

Can you make a patch that does this?

thanks,

greg k-h

Re: The r8188eu kernel module does not depend on the rtlwifi/rtl8188eufw.bin firmware file

[Grzegorz Szymaszek]

[-- Attachment #1: Type: text/plain, Size: 356 bytes --]

On Tue, Aug 02, 2022 at 04:07:25PM +0200, Greg KH wrote:
> Looks like someone needs to add a line to the driver that looks like:
> 	MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
> […] Can you make a patch that does this?

Sure, I will prepare and send one later (hopefully today), assuming no
one objects in the meantime. Thanks for quick response.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: The r8188eu kernel module does not depend on the rtlwifi/rtl8188eufw.bin firmware file

[Larry Finger]

On 8/2/22 09:17, Grzegorz Szymaszek wrote:
> On Tue, Aug 02, 2022 at 04:07:25PM +0200, Greg KH wrote:
>> Looks like someone needs to add a line to the driver that looks like:
>> 	MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
>> […] Can you make a patch that does this?
> 
> Sure, I will prepare and send one later (hopefully today), assuming no
> one objects in the meantime. Thanks for quick response.

There will be no objections. All 8188eu-based devices need that firmware. 
Omitting the MODULE_FIRMWARE macro was simply an oversight.

Larry

[PATCH] staging: r8188eu: add firmware dependency

[Grzegorz Szymaszek]

The old rtl8188eu module, removed in commit 55dfa29b43d2 ("staging:
rtl8188eu: remove rtl8188eu driver from staging dir") (Linux kernel
v5.15-rc1), required (through a MODULE_FIRMWARE call()) the
rtlwifi/rtl8188eufw.bin firmware file, which the new r8188eu driver no
longer requires.

I have tested a few RTL8188EUS-based Wi-Fi cards and, while supported by
both drivers, they do not work when using the new one and the firmware
wasn't manually loaded. According to Larry Finger, the module
maintainer, all such cards need the firmware and the driver should
depend on it (see the linked mails).

Add a proper MODULE_FIRMWARE() call, like it was done in the old driver.

Thanks to Greg Kroah-Hartman and Larry Finger for quick responses to my
questions.

Link: https://answers.launchpad.net/ubuntu/+source/linux-meta-hwe-5.15/+question/702611
Link: https://lore.kernel.org/lkml/YukkBu3TNODO3or9@nx64de-df6d00/
Signed-off-by: Grzegorz Szymaszek <gszymaszek@short.pl>
---
 drivers/staging/r8188eu/os_dep/os_intfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
index 891c85b088ca..5bd3022e4b40 100644
--- a/drivers/staging/r8188eu/os_dep/os_intfs.c
+++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
@@ -18,6 +18,7 @@ MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Realtek Wireless Lan Driver");
 MODULE_AUTHOR("Realtek Semiconductor Corp.");
 MODULE_VERSION(DRIVERVERSION);
+MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
 
 #define CONFIG_BR_EXT_BRNAME "br0"
 #define RTW_NOTCH_FILTER 0 /* 0:Disable, 1:Enable, */
-- 
2.35.1

Re: [PATCH] staging: r8188eu: add firmware dependency

[Larry Finger]

On 8/2/22 12:18, Grzegorz Szymaszek wrote:
> The old rtl8188eu module, removed in commit 55dfa29b43d2 ("staging:
> rtl8188eu: remove rtl8188eu driver from staging dir") (Linux kernel
> v5.15-rc1), required (through a MODULE_FIRMWARE call()) the
> rtlwifi/rtl8188eufw.bin firmware file, which the new r8188eu driver no
> longer requires.
> 
> I have tested a few RTL8188EUS-based Wi-Fi cards and, while supported by
> both drivers, they do not work when using the new one and the firmware
> wasn't manually loaded. According to Larry Finger, the module
> maintainer, all such cards need the firmware and the driver should
> depend on it (see the linked mails).
> 
> Add a proper MODULE_FIRMWARE() call, like it was done in the old driver.
> 
> Thanks to Greg Kroah-Hartman and Larry Finger for quick responses to my
> questions.
> 
> Link: https://answers.launchpad.net/ubuntu/+source/linux-meta-hwe-5.15/+question/702611
> Link: https://lore.kernel.org/lkml/YukkBu3TNODO3or9@nx64de-df6d00/
> Signed-off-by: Grzegorz Szymaszek <gszymaszek@short.pl>
> ---
>   drivers/staging/r8188eu/os_dep/os_intfs.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
> index 891c85b088ca..5bd3022e4b40 100644
> --- a/drivers/staging/r8188eu/os_dep/os_intfs.c
> +++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
> @@ -18,6 +18,7 @@ MODULE_LICENSE("GPL");
>   MODULE_DESCRIPTION("Realtek Wireless Lan Driver");
>   MODULE_AUTHOR("Realtek Semiconductor Corp.");
>   MODULE_VERSION(DRIVERVERSION);
> +MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
>   
>   #define CONFIG_BR_EXT_BRNAME "br0"
>   #define RTW_NOTCH_FILTER 0 /* 0:Disable, 1:Enable, */

Acked-by: Larry Finger <Larry.Finger@lwfinger.net>

Thanks,

Larry

Re: [PATCH] staging: r8188eu: add firmware dependency

[Phillip Potter]

On Tue, Aug 02, 2022 at 07:18:44PM +0200, Grzegorz Szymaszek wrote:
> The old rtl8188eu module, removed in commit 55dfa29b43d2 ("staging:
> rtl8188eu: remove rtl8188eu driver from staging dir") (Linux kernel
> v5.15-rc1), required (through a MODULE_FIRMWARE call()) the
> rtlwifi/rtl8188eufw.bin firmware file, which the new r8188eu driver no
> longer requires.
> 
> I have tested a few RTL8188EUS-based Wi-Fi cards and, while supported by
> both drivers, they do not work when using the new one and the firmware
> wasn't manually loaded. According to Larry Finger, the module
> maintainer, all such cards need the firmware and the driver should
> depend on it (see the linked mails).
> 
> Add a proper MODULE_FIRMWARE() call, like it was done in the old driver.
> 
> Thanks to Greg Kroah-Hartman and Larry Finger for quick responses to my
> questions.
> 
> Link: https://answers.launchpad.net/ubuntu/+source/linux-meta-hwe-5.15/+question/702611
> Link: https://lore.kernel.org/lkml/YukkBu3TNODO3or9@nx64de-df6d00/
> Signed-off-by: Grzegorz Szymaszek <gszymaszek@short.pl>
> ---
>  drivers/staging/r8188eu/os_dep/os_intfs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
> index 891c85b088ca..5bd3022e4b40 100644
> --- a/drivers/staging/r8188eu/os_dep/os_intfs.c
> +++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
> @@ -18,6 +18,7 @@ MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("Realtek Wireless Lan Driver");
>  MODULE_AUTHOR("Realtek Semiconductor Corp.");
>  MODULE_VERSION(DRIVERVERSION);
> +MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
>  
>  #define CONFIG_BR_EXT_BRNAME "br0"
>  #define RTW_NOTCH_FILTER 0 /* 0:Disable, 1:Enable, */
> -- 
> 2.35.1

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Thanks for the patch :-)

Regards,
Phil

Re: [PATCH] staging: r8188eu: add firmware dependency

[Greg KH]

On Tue, Aug 02, 2022 at 07:18:44PM +0200, Grzegorz Szymaszek wrote:
> The old rtl8188eu module, removed in commit 55dfa29b43d2 ("staging:
> rtl8188eu: remove rtl8188eu driver from staging dir") (Linux kernel
> v5.15-rc1), required (through a MODULE_FIRMWARE call()) the
> rtlwifi/rtl8188eufw.bin firmware file, which the new r8188eu driver no
> longer requires.
> 
> I have tested a few RTL8188EUS-based Wi-Fi cards and, while supported by
> both drivers, they do not work when using the new one and the firmware
> wasn't manually loaded. According to Larry Finger, the module
> maintainer, all such cards need the firmware and the driver should
> depend on it (see the linked mails).
> 
> Add a proper MODULE_FIRMWARE() call, like it was done in the old driver.
> 
> Thanks to Greg Kroah-Hartman and Larry Finger for quick responses to my
> questions.
> 
> Link: https://answers.launchpad.net/ubuntu/+source/linux-meta-hwe-5.15/+question/702611
> Link: https://lore.kernel.org/lkml/YukkBu3TNODO3or9@nx64de-df6d00/
> Signed-off-by: Grzegorz Szymaszek <gszymaszek@short.pl>
> ---
>  drivers/staging/r8188eu/os_dep/os_intfs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
> index 891c85b088ca..5bd3022e4b40 100644
> --- a/drivers/staging/r8188eu/os_dep/os_intfs.c
> +++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
> @@ -18,6 +18,7 @@ MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("Realtek Wireless Lan Driver");
>  MODULE_AUTHOR("Realtek Semiconductor Corp.");
>  MODULE_VERSION(DRIVERVERSION);
> +MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");

This looks good, and I'll apply it after 5.20-rc1 is out, but you might
want to send a follow-on patch that removes the hard-coded string in 2
places in the driver, and just puts it into a single define somewhere,
to make it a bit easier over time.  Most other drivers do this as well,
so there are examples to look at.

thanks,

greg k-h

Re: [PATCH] staging: r8188eu: add firmware dependency

[Grzegorz Szymaszek]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

On Wed, Aug 03, 2022 at 08:08:31AM +0200, Greg KH wrote:
> This looks good, and I'll apply it after 5.20-rc1 is out,

I didn’t Cc stable since the “Submitting patches” guide says it’s for
“severe bugs”, but would it be possible to backport the patch to the
older kernels?

> but you might want to send a follow-on patch that removes the hard-coded
> string in 2 places in the driver, and just puts it into a single define
> somewhere, to make it a bit easier over time.

Good idea, will do so. I didn’t check if the filename is already used.
Would something like “this patch depends on patch…” (again from the
guide) be enough (assuming I will send the new patch before this one
is applied)?

-- 
Grzegorz Szymaszek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] staging: r8188eu: add firmware dependency

[Greg KH]

On Wed, Aug 03, 2022 at 08:33:35AM +0200, Grzegorz Szymaszek wrote:
> On Wed, Aug 03, 2022 at 08:08:31AM +0200, Greg KH wrote:
> > This looks good, and I'll apply it after 5.20-rc1 is out,
> 
> I didn’t Cc stable since the “Submitting patches” guide says it’s for
> “severe bugs”, but would it be possible to backport the patch to the
> older kernels?

Yes, I will tag it for backporting.

> > but you might want to send a follow-on patch that removes the hard-coded
> > string in 2 places in the driver, and just puts it into a single define
> > somewhere, to make it a bit easier over time.
> 
> Good idea, will do so. I didn’t check if the filename is already used.
> Would something like “this patch depends on patch…” (again from the
> guide) be enough (assuming I will send the new patch before this one
> is applied)?

No need to add the "this patch depends on", I'll know that implicitly as
it was sent after this one :)

thanks,

greg k-h

[PATCH 1/3] staging: r8188eu: set firmware path in a macro

[Grzegorz Szymaszek]

The r8188eu driver requires a firmware file, the path of which was
hardcoded as constant strings in two places:
(1) in core/rtw_fw.c, in function load_firmware(),
(2) in os_dep/os_intfs.c, in the MODULE_FIRMWARE() call.

Declare the path using a macro, FW_RTL8188EU, and replace the above
constant strings with the macro. That's the way it is done in many other
drivers. The new macro is defined in include/drv_types.h, because that
file is already included by both of the above files (or at least their
headers) and because it already contains other driver constants, like
its name and version.

Link: https://lore.kernel.org/lkml/YuoQ37PIKzWO1zIY@kroah.com/
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Grzegorz Szymaszek <gszymaszek@short.pl>
---
 drivers/staging/r8188eu/core/rtw_fw.c       | 2 +-
 drivers/staging/r8188eu/include/drv_types.h | 1 +
 drivers/staging/r8188eu/os_dep/os_intfs.c   | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_fw.c b/drivers/staging/r8188eu/core/rtw_fw.c
index 0451e5177644..0fe6d4944694 100644
--- a/drivers/staging/r8188eu/core/rtw_fw.c
+++ b/drivers/staging/r8188eu/core/rtw_fw.c
@@ -209,7 +209,7 @@ static int load_firmware(struct rt_firmware *rtfw, struct device *device)
 {
 	int ret = _SUCCESS;
 	const struct firmware *fw;
-	const char *fw_name = "rtlwifi/rtl8188eufw.bin";
+	const char *fw_name = FW_RTL8188EU;
 	int err = request_firmware(&fw, fw_name, device);
 
 	if (err) {
diff --git a/drivers/staging/r8188eu/include/drv_types.h b/drivers/staging/r8188eu/include/drv_types.h
index bba88a0ede61..f51b83515953 100644
--- a/drivers/staging/r8188eu/include/drv_types.h
+++ b/drivers/staging/r8188eu/include/drv_types.h
@@ -37,6 +37,7 @@
 #include "rtw_fw.h"
 
 #define DRIVERVERSION	"v4.1.4_6773.20130222"
+#define FW_RTL8188EU	"rtlwifi/rtl8188eufw.bin"
 
 struct registry_priv {
 	u8	chip_version;
diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
index 5bd3022e4b40..5985054da935 100644
--- a/drivers/staging/r8188eu/os_dep/os_intfs.c
+++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
@@ -18,7 +18,7 @@ MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Realtek Wireless Lan Driver");
 MODULE_AUTHOR("Realtek Semiconductor Corp.");
 MODULE_VERSION(DRIVERVERSION);
-MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
+MODULE_FIRMWARE(FW_RTL8188EU);
 
 #define CONFIG_BR_EXT_BRNAME "br0"
 #define RTW_NOTCH_FILTER 0 /* 0:Disable, 1:Enable, */
-- 
2.35.1

[PATCH 2/3] staging: r8188eu: make use of the DRV_NAME macro

[Grzegorz Szymaszek]

The DRV_NAME macro is defined with the name of the r8188eu driver, but
it seems it wasn't actually used anywhere. Replace a hardcoded constant
string of the same value in the driver's struct rtw_usb_drv, field
.usbdrv.name. The affected file already includes include/drv_types.h,
where the macro is declared.

Signed-off-by: Grzegorz Szymaszek <gszymaszek@short.pl>
---
 drivers/staging/r8188eu/os_dep/usb_intf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
index 68869c5daeff..256b9045488e 100644
--- a/drivers/staging/r8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
@@ -54,7 +54,7 @@ struct rtw_usb_drv {
 };
 
 static struct rtw_usb_drv rtl8188e_usb_drv = {
-	.usbdrv.name = "r8188eu",
+	.usbdrv.name = DRV_NAME,
 	.usbdrv.probe = rtw_drv_init,
 	.usbdrv.disconnect = rtw_dev_remove,
 	.usbdrv.id_table = rtw_usb_id_tbl,
-- 
2.35.1

[PATCH 3/3] staging: r8188eu: make driver metadata macro names more consistent

[Grzegorz Szymaszek]

Rename DRIVERVERSION to DRV_VERSION so that it looks more alike the
other macros, DRV_NAME and FW_*, and matches the most popular (as it
seems from a quick review) conventions in other drivers.

Signed-off-by: Grzegorz Szymaszek <gszymaszek@short.pl>
---
 drivers/staging/r8188eu/include/drv_types.h | 5 ++---
 drivers/staging/r8188eu/os_dep/os_intfs.c   | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/r8188eu/include/drv_types.h b/drivers/staging/r8188eu/include/drv_types.h
index f51b83515953..3328c66d1ef1 100644
--- a/drivers/staging/r8188eu/include/drv_types.h
+++ b/drivers/staging/r8188eu/include/drv_types.h
@@ -10,8 +10,6 @@
 #ifndef __DRV_TYPES_H__
 #define __DRV_TYPES_H__
 
-#define DRV_NAME "r8188eu"
-
 #include "osdep_service.h"
 #include "wlan_bssdef.h"
 #include "rtw_ht.h"
@@ -36,7 +34,8 @@
 #include "rtl8188e_hal.h"
 #include "rtw_fw.h"
 
-#define DRIVERVERSION	"v4.1.4_6773.20130222"
+#define DRV_NAME	"r8188eu"
+#define DRV_VERSION	"v4.1.4_6773.20130222"
 #define FW_RTL8188EU	"rtlwifi/rtl8188eufw.bin"
 
 struct registry_priv {
diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
index 5985054da935..d9abd2a98e1b 100644
--- a/drivers/staging/r8188eu/os_dep/os_intfs.c
+++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
@@ -17,7 +17,7 @@
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Realtek Wireless Lan Driver");
 MODULE_AUTHOR("Realtek Semiconductor Corp.");
-MODULE_VERSION(DRIVERVERSION);
+MODULE_VERSION(DRV_VERSION);
 MODULE_FIRMWARE(FW_RTL8188EU);
 
 #define CONFIG_BR_EXT_BRNAME "br0"
-- 
2.35.1

Re: [PATCH 1/3] staging: r8188eu: set firmware path in a macro

[Philipp Hortmann]

On 8/4/22 00:28, Grzegorz Szymaszek wrote:
> diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
> index 5bd3022e4b40..5985054da935 100644
> --- a/drivers/staging/r8188eu/os_dep/os_intfs.c
> +++ b/drivers/staging/r8188eu/os_dep/os_intfs.c
> @@ -18,7 +18,7 @@ MODULE_LICENSE("GPL");
>   MODULE_DESCRIPTION("Realtek Wireless Lan Driver");
>   MODULE_AUTHOR("Realtek Semiconductor Corp.");
>   MODULE_VERSION(DRIVERVERSION);
> -MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
> +MODULE_FIRMWARE(FW_RTL8188EU);
>   
>   #define CONFIG_BR_EXT_BRNAME "br0"
>   #define RTW_NOTCH_FILTER 0 /* 0:Disable, 1:Enable, */


It failed to apply your patch as the following line:
MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
is not in the repo. Inserted line in my repo to apply patch.

Why is the coverletter missing?

Tested all three patches.

Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150

Re: [PATCH 1/3] staging: r8188eu: set firmware path in a macro

[Grzegorz Szymaszek]

[-- Attachment #1: Type: text/plain, Size: 1336 bytes --]

On Thu, Aug 04, 2022 at 10:11:58PM +0200, Philipp Hortmann wrote:
> On 8/4/22 00:28, Grzegorz Szymaszek wrote:
> > diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
> > -%<-
> > -MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
> > +MODULE_FIRMWARE(FW_RTL8188EU);
> 
> 
> It failed to apply your patch as the following line:
> MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
> is not in the repo. Inserted line in my repo to apply patch.

I’m sorry, I didn’t add the base tree reference. Right now, git
format-patch generates the following:

    base-commit: 9de1f9c8ca5100a02a2e271bdbde36202e251b4b
    prerequisite-patch-id: 79964bd0bcd260f1df53830a81e009c34993ee6f

The prerequisite patch is available at
<https://lore.kernel.org/lkml/YulcdKfhA8dPQ78s@nx64de-df6d00/>.

> Why is the coverletter missing?

I didn’t think it would be necessary, since the patches are quite
simple and there are just three of them. Again, I’m sorry, I don’t want
to make it harder for anyone to review my patches. Hopefully I will
learn more of the kernel development practises without wasting too much
time of patch reviewers.

Should I send an improved (with the base tree reference and with a short
cover letter) patch series?

> Tested all three patches.

Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] staging: r8188eu: set firmware path in a macro

[Greg KH]

On Fri, Aug 05, 2022 at 12:23:12AM +0200, Grzegorz Szymaszek wrote:
> On Thu, Aug 04, 2022 at 10:11:58PM +0200, Philipp Hortmann wrote:
> > On 8/4/22 00:28, Grzegorz Szymaszek wrote:
> > > diff --git a/drivers/staging/r8188eu/os_dep/os_intfs.c b/drivers/staging/r8188eu/os_dep/os_intfs.c
> > > -%<-
> > > -MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
> > > +MODULE_FIRMWARE(FW_RTL8188EU);
> > 
> > 
> > It failed to apply your patch as the following line:
> > MODULE_FIRMWARE("rtlwifi/rtl8188eufw.bin");
> > is not in the repo. Inserted line in my repo to apply patch.
> 
> I’m sorry, I didn’t add the base tree reference. Right now, git
> format-patch generates the following:
> 
>     base-commit: 9de1f9c8ca5100a02a2e271bdbde36202e251b4b
>     prerequisite-patch-id: 79964bd0bcd260f1df53830a81e009c34993ee6f
> 
> The prerequisite patch is available at
> <https://lore.kernel.org/lkml/YulcdKfhA8dPQ78s@nx64de-df6d00/>.
> 
> > Why is the coverletter missing?
> 
> I didn’t think it would be necessary, since the patches are quite
> simple and there are just three of them. Again, I’m sorry, I don’t want
> to make it harder for anyone to review my patches. Hopefully I will
> learn more of the kernel development practises without wasting too much
> time of patch reviewers.
> 
> Should I send an improved (with the base tree reference and with a short
> cover letter) patch series?

Nah, it's fine as-is, I will take it when my tree opens up again after
-rc1 is out in a week or so.

thanks,

greg k-h

Re: [PATCH 3/3] staging: r8188eu: make driver metadata macro names more consistent

[Greg KH]

On Thu, Aug 04, 2022 at 12:29:10AM +0200, Grzegorz Szymaszek wrote:
> Rename DRIVERVERSION to DRV_VERSION so that it looks more alike the
> other macros, DRV_NAME and FW_*, and matches the most popular (as it
> seems from a quick review) conventions in other drivers.
> 
> Signed-off-by: Grzegorz Szymaszek <gszymaszek@short.pl>
> ---
>  drivers/staging/r8188eu/include/drv_types.h | 5 ++---
>  drivers/staging/r8188eu/os_dep/os_intfs.c   | 2 +-
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/include/drv_types.h b/drivers/staging/r8188eu/include/drv_types.h
> index f51b83515953..3328c66d1ef1 100644
> --- a/drivers/staging/r8188eu/include/drv_types.h
> +++ b/drivers/staging/r8188eu/include/drv_types.h
> @@ -10,8 +10,6 @@
>  #ifndef __DRV_TYPES_H__
>  #define __DRV_TYPES_H__
>  
> -#define DRV_NAME "r8188eu"

This should just be KBUILD_MODNAME, no need to create yet-another-macro
for this one.

> -
>  #include "osdep_service.h"
>  #include "wlan_bssdef.h"
>  #include "rtw_ht.h"
> @@ -36,7 +34,8 @@
>  #include "rtl8188e_hal.h"
>  #include "rtw_fw.h"
>  
> -#define DRIVERVERSION	"v4.1.4_6773.20130222"
> +#define DRV_NAME	"r8188eu"

Again, KBUILD_MODNAME

> +#define DRV_VERSION	"v4.1.4_6773.20130222"

As the driver is now in the kernel, this "version" string can just go
away.  Can you redo this patch to do the DRV_NAME thing first, and then
drop the DRV_VERSION field after that?

thanks,

greg k-h

Re: [PATCH 2/3] staging: r8188eu: make use of the DRV_NAME macro

[Greg KH]

On Thu, Aug 04, 2022 at 12:29:01AM +0200, Grzegorz Szymaszek wrote:
> The DRV_NAME macro is defined with the name of the r8188eu driver, but
> it seems it wasn't actually used anywhere. Replace a hardcoded constant
> string of the same value in the driver's struct rtw_usb_drv, field
> .usbdrv.name. The affected file already includes include/drv_types.h,
> where the macro is declared.
> 
> Signed-off-by: Grzegorz Szymaszek <gszymaszek@short.pl>
> ---
>  drivers/staging/r8188eu/os_dep/usb_intf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
> index 68869c5daeff..256b9045488e 100644
> --- a/drivers/staging/r8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
> @@ -54,7 +54,7 @@ struct rtw_usb_drv {
>  };
>  
>  static struct rtw_usb_drv rtl8188e_usb_drv = {
> -	.usbdrv.name = "r8188eu",
> +	.usbdrv.name = DRV_NAME,

Should just be KBUILD_MODNAME.

thanks,

greg k-h