Re: ip_auto_config() prevents network device to be registered
Hello, On 01/31/2017 02:49 PM, Javier Martinez Canillas wrote: > > The kernelci folks pointed out that a Samsung Exynos based board was failing > to boot when trying to mount the rootfs via NFS, due a networking issue [0]. > > I looked at the issue and it turned out to be a race between ip_auto_config() > and register_netdev() when using the ip=dhcp param in the kernel command line. > > The problem is that ip_auto_config() calls wait_for_devices() [1] and returns > as soon as it finds a network device registered. Then ic_open_devs() [2] is > called then to bring the network devs up and wait for their carrier signals. > > But ic_open_devs() grabs the rtnl_mutex lock [3] when doing this, which is the > same lock that register_netdev() [4] grabs before registering a network > device. > > And so if a network dev is found and wait_for_devices() returns, > ic_open_devs() > will be called and no new network dev could be registered in the meantime. > > So since ic_open_devs() waits up to CONF_CARRIER_TIMEOUT (120 secs) with this > lock held, if the network dev that's supposed to get its IP over DHCP isn't > the > first to be registered, the boot test job may timeout and be considered a > fail. > > A workaround is to use ip=:eth0:dhcp instead ip=dhcp, so > wait_for_devices() > waits for this specific device. Another workaround is to increase the timeout > for the job to be much bigger than CONF_CARRIER_TIMEOUT so ip_auto_config() > can > retry and the network devices can be registered between tries. > > But I wonder if someone can suggest a proper way to fix this. Grabbing a mutex > that prevents network devs to be registered for 120 secs doesn't sound > correct. > > Thanks a lot for your help and please let me know if I misunderstood > something. > > [0]: > https://storage.kernelci.org/mainline/v4.9/arm-exynos_defconfig/lab-collabora/boot-exynos5422-odroidxu3_rootfs:nfs.html > [1]: http://lxr.free-electrons.com/source/net/ipv4/ipconfig.c#L1368 > [2]: http://lxr.free-electrons.com/source/net/ipv4/ipconfig.c#L202 > [3]: http://lxr.free-electrons.com/source/net/core/rtnetlink.c#L68 > [4]: http://lxr.free-electrons.com/source/net/core/dev.c#L7326 > > Any comments on this? We are still seeing this problem with today's -next (20170310): https://storage.kernelci.org/next/next-20170310/arm-exynos_defconfig/lab-collabora/boot-exynos5422-odroidxu3.html Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
ip_auto_config() prevents network device to be registered
Hello, The kernelci folks pointed out that a Samsung Exynos based board was failing to boot when trying to mount the rootfs via NFS, due a networking issue [0]. I looked at the issue and it turned out to be a race between ip_auto_config() and register_netdev() when using the ip=dhcp param in the kernel command line. The problem is that ip_auto_config() calls wait_for_devices() [1] and returns as soon as it finds a network device registered. Then ic_open_devs() [2] is called then to bring the network devs up and wait for their carrier signals. But ic_open_devs() grabs the rtnl_mutex lock [3] when doing this, which is the same lock that register_netdev() [4] grabs before registering a network device. And so if a network dev is found and wait_for_devices() returns, ic_open_devs() will be called and no new network dev could be registered in the meantime. So since ic_open_devs() waits up to CONF_CARRIER_TIMEOUT (120 secs) with this lock held, if the network dev that's supposed to get its IP over DHCP isn't the first to be registered, the boot test job may timeout and be considered a fail. A workaround is to use ip=:eth0:dhcp instead ip=dhcp, so wait_for_devices() waits for this specific device. Another workaround is to increase the timeout for the job to be much bigger than CONF_CARRIER_TIMEOUT so ip_auto_config() can retry and the network devices can be registered between tries. But I wonder if someone can suggest a proper way to fix this. Grabbing a mutex that prevents network devs to be registered for 120 secs doesn't sound correct. Thanks a lot for your help and please let me know if I misunderstood something. [0]: https://storage.kernelci.org/mainline/v4.9/arm-exynos_defconfig/lab-collabora/boot-exynos5422-odroidxu3_rootfs:nfs.html [1]: http://lxr.free-electrons.com/source/net/ipv4/ipconfig.c#L1368 [2]: http://lxr.free-electrons.com/source/net/ipv4/ipconfig.c#L202 [3]: http://lxr.free-electrons.com/source/net/core/rtnetlink.c#L68 [4]: http://lxr.free-electrons.com/source/net/core/dev.c#L7326 Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
Re: [PATCH -next] net: wan: slic_ds26522: Remove .owner field for driver
Hello Wei, On 10/17/2016 11:51 AM, Wei Yongjun wrote: > From: Wei Yongjun > > Remove .owner field if calls are used which set it automatically. > > Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci > > Signed-off-by: Wei Yongjun > --- > drivers/net/wan/slic_ds26522.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/net/wan/slic_ds26522.c b/drivers/net/wan/slic_ds26522.c > index b776a0a..5bca31c 100644 > --- a/drivers/net/wan/slic_ds26522.c > +++ b/drivers/net/wan/slic_ds26522.c > @@ -241,7 +241,6 @@ static struct spi_driver slic_ds26522_driver = { > .driver = { > .name = "ds26522", > .bus = &spi_bus_type, > -.owner = THIS_MODULE, > .of_match_table = slic_ds26522_match, > }, > .probe = slic_ds26522_probe, > Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
Re: [PATCH -next] net: wan: slic_ds26522: Use module_spi_driver to simplify the code
Hello Wei, On 10/17/2016 11:50 AM, Wei Yongjun wrote: > From: Wei Yongjun > > module_spi_driver() makes the code simpler by eliminating > boilerplate code. > > Signed-off-by: Wei Yongjun > --- > drivers/net/wan/slic_ds26522.c | 13 + > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/drivers/net/wan/slic_ds26522.c b/drivers/net/wan/slic_ds26522.c > index b776a0a..8f782f1 100644 > --- a/drivers/net/wan/slic_ds26522.c > +++ b/drivers/net/wan/slic_ds26522.c > @@ -249,15 +249,4 @@ static struct spi_driver slic_ds26522_driver = { > .id_table = slic_ds26522_id, > }; > > -static int __init slic_ds26522_init(void) > -{ > - return spi_register_driver(&slic_ds26522_driver); > -} > - > -static void __exit slic_ds26522_exit(void) > -{ > - spi_unregister_driver(&slic_ds26522_driver); > -} > - > -module_init(slic_ds26522_init); > -module_exit(slic_ds26522_exit); > +module_spi_driver(slic_ds26522_driver); > Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
[PATCH 4/7] net: qcom/emac: Fix module autoload for OF registration
If the driver is built as a module, autoload won't work because the module alias information is not filled. So user-space can't match the registered device with the corresponding module. Export the module alias information using the MODULE_DEVICE_TABLE() macro. Before this patch: $ modinfo drivers/net/ethernet/qualcomm/emac/qcom-emac.ko | grep alias alias: platform:qcom-emac After this patch: $ modinfo drivers/net/ethernet/qualcomm/emac/qcom-emac.ko | grep alias alias: platform:qcom-emac alias: of:N*T*Cqcom,fsm9900-emacC* alias: of:N*T*Cqcom,fsm9900-emac Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/qualcomm/emac/emac.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c index 9bf3b2b82e95..4fede4b86538 100644 --- a/drivers/net/ethernet/qualcomm/emac/emac.c +++ b/drivers/net/ethernet/qualcomm/emac/emac.c @@ -575,6 +575,7 @@ static const struct of_device_id emac_dt_match[] = { }, {} }; +MODULE_DEVICE_TABLE(of, emac_dt_match); #if IS_ENABLED(CONFIG_ACPI) static const struct acpi_device_id emac_acpi_match[] = { -- 2.7.4
[PATCH 6/7] net: dsa: b53: Fix module autoload
If the driver is built as a module, autoload won't work because the module alias information is not filled. So user-space can't match the registered device with the corresponding module. Export the module alias information using the MODULE_DEVICE_TABLE() macro. Before this patch: $ modinfo drivers/net/dsa/b53/b53_mmap.ko | grep alias $ After this patch: $ modinfo drivers/net/dsa/b53/b53_mmap.ko | grep alias alias: of:N*T*Cbrcm,bcm63xx-switchC* alias: of:N*T*Cbrcm,bcm63xx-switch alias: of:N*T*Cbrcm,bcm6368-switchC* alias: of:N*T*Cbrcm,bcm6368-switch alias: of:N*T*Cbrcm,bcm6328-switchC* alias: of:N*T*Cbrcm,bcm6328-switch alias: of:N*T*Cbrcm,bcm3384-switchC* alias: of:N*T*Cbrcm,bcm3384-switch Signed-off-by: Javier Martinez Canillas --- drivers/net/dsa/b53/b53_mmap.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/dsa/b53/b53_mmap.c b/drivers/net/dsa/b53/b53_mmap.c index 76fb8552c9d9..ef63d24fef81 100644 --- a/drivers/net/dsa/b53/b53_mmap.c +++ b/drivers/net/dsa/b53/b53_mmap.c @@ -256,6 +256,7 @@ static const struct of_device_id b53_mmap_of_table[] = { { .compatible = "brcm,bcm63xx-switch" }, { /* sentinel */ }, }; +MODULE_DEVICE_TABLE(of, b53_mmap_of_table); static struct platform_driver b53_mmap_driver = { .probe = b53_mmap_probe, -- 2.7.4
[PATCH 7/7] net: dsa: bcm_sf2: Fix module autoload for OF registration
If the driver is built as a module, autoload won't work because the module alias information is not filled. So user-space can't match the registered device with the corresponding module. Export the module alias information using the MODULE_DEVICE_TABLE() macro. Before this patch: $ modinfo drivers/net/dsa/bcm_sf2.ko | grep alias alias: platform:brcm-sf2 After this patch: $ modinfo drivers/net/dsa/bcm_sf2.ko | grep alias alias: platform:brcm-sf2 alias: of:N*T*Cbrcm,bcm7445-switch-v4.0C* alias: of:N*T*Cbrcm,bcm7445-switch-v4.0 Signed-off-by: Javier Martinez Canillas --- drivers/net/dsa/bcm_sf2.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index e218887f18b7..0427009bc924 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -1158,6 +1158,7 @@ static const struct of_device_id bcm_sf2_of_match[] = { { .compatible = "brcm,bcm7445-switch-v4.0" }, { /* sentinel */ }, }; +MODULE_DEVICE_TABLE(of, bcm_sf2_of_match); static struct platform_driver bcm_sf2_driver = { .probe = bcm_sf2_sw_probe, -- 2.7.4
[PATCH 3/7] net: hns: Fix hns_dsaf module autoload for OF registration
If the driver is built as a module, autoload won't work because the module alias information is not filled. So user-space can't match the registered device with the corresponding module. Export the module alias information using the MODULE_DEVICE_TABLE() macro. Before this patch: $ modinfo drivers/net/ethernet/hisilicon/hns/hns_dsaf.ko | grep alias alias: acpi*:HISI00B2:* alias: acpi*:HISI00B1:* After this patch: $ modinfo drivers/net/ethernet/hisilicon/hns/hns_dsaf.ko | grep alias alias: acpi*:HISI00B2:* alias: acpi*:HISI00B1:* alias: of:N*T*Chisilicon,hns-dsaf-v2C* alias: of:N*T*Chisilicon,hns-dsaf-v2 alias: of:N*T*Chisilicon,hns-dsaf-v1C* alias: of:N*T*Chisilicon,hns-dsaf-v1 Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c index 8d70377f6624..8ea3d95fa483 100644 --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c @@ -2751,6 +2751,7 @@ static const struct of_device_id g_dsaf_match[] = { {.compatible = "hisilicon,hns-dsaf-v2"}, {} }; +MODULE_DEVICE_TABLE(of, g_dsaf_match); static struct platform_driver g_dsaf_driver = { .probe = hns_dsaf_probe, -- 2.7.4
[PATCH 1/7] net: nps_enet: Fix module autoload
If the driver is built as a module, autoload won't work because the module alias information is not filled. So user-space can't match the registered device with the corresponding module. Export the module alias information using the MODULE_DEVICE_TABLE() macro. Before this patch: $ modinfo drivers/net/ethernet/ezchip/nps_enet.ko | grep alias $ After this patch: $ modinfo drivers/net/ethernet/ezchip/nps_enet.ko | grep alias alias: of:N*T*Cezchip,nps-mgt-enetC* alias: of:N*T*Cezchip,nps-mgt-enet Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/ezchip/nps_enet.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/ezchip/nps_enet.c b/drivers/net/ethernet/ezchip/nps_enet.c index f928e6f79c89..223f35cc034c 100644 --- a/drivers/net/ethernet/ezchip/nps_enet.c +++ b/drivers/net/ethernet/ezchip/nps_enet.c @@ -669,6 +669,7 @@ static const struct of_device_id nps_enet_dt_ids[] = { { .compatible = "ezchip,nps-mgt-enet" }, { /* Sentinel */ } }; +MODULE_DEVICE_TABLE(of, nps_enet_dt_ids); static struct platform_driver nps_enet_driver = { .probe = nps_enet_probe, -- 2.7.4
[PATCH 5/7] net: hisilicon: Fix hns_mdio module autoload for OF registration
If the driver is built as a module, autoload won't work because the module alias information is not filled. So user-space can't match the registered device with the corresponding module. Export the module alias information using the MODULE_DEVICE_TABLE() macro. Before this patch: $ modinfo drivers/net/ethernet/hisilicon//hns_mdio.ko | grep alias alias: platform:Hi-HNS_MDIO alias: acpi*:HISI0141:* After this patch: $ modinfo drivers/net/ethernet/hisilicon//hns_mdio.ko | grep alias alias: platform:Hi-HNS_MDIO alias: of:N*T*Chisilicon,hns-mdioC* alias: of:N*T*Chisilicon,hns-mdio alias: of:N*T*Chisilicon,mdioC* alias: of:N*T*Chisilicon,mdio alias: acpi*:HISI0141:* Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/hisilicon/hns_mdio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/hisilicon/hns_mdio.c b/drivers/net/ethernet/hisilicon/hns_mdio.c index 33f4c483af0f..501eb2090ca6 100644 --- a/drivers/net/ethernet/hisilicon/hns_mdio.c +++ b/drivers/net/ethernet/hisilicon/hns_mdio.c @@ -563,6 +563,7 @@ static const struct of_device_id hns_mdio_match[] = { {.compatible = "hisilicon,hns-mdio"}, {} }; +MODULE_DEVICE_TABLE(of, hns_mdio_match); static const struct acpi_device_id hns_mdio_acpi_match[] = { { "HISI0141", 0 }, -- 2.7.4
[PATCH 2/7] net: ethernet: nb8800: Fix module autoload
If the driver is built as a module, autoload won't work because the module alias information is not filled. So user-space can't match the registered device with the corresponding module. Export the module alias information using the MODULE_DEVICE_TABLE() macro. Before this patch: $ $ modinfo drivers/net/ethernet/aurora/nb8800.ko | grep alias $ After this patch: $ modinfo drivers/net/ethernet/aurora/nb8800.ko | grep alias alias: of:N*T*Csigma,smp8734-ethernetC* alias: of:N*T*Csigma,smp8734-ethernet alias: of:N*T*Csigma,smp8642-ethernetC* alias: of:N*T*Csigma,smp8642-ethernet alias: of:N*T*Caurora,nb8800C* alias: of:N*T*Caurora,nb8800 Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/aurora/nb8800.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c index 453dc0967125..d5f2ad1a5a30 100644 --- a/drivers/net/ethernet/aurora/nb8800.c +++ b/drivers/net/ethernet/aurora/nb8800.c @@ -1357,6 +1357,7 @@ static const struct of_device_id nb8800_dt_ids[] = { }, { } }; +MODULE_DEVICE_TABLE(of, nb8800_dt_ids); static int nb8800_probe(struct platform_device *pdev) { -- 2.7.4
[PATCH 0/7] net: Fix module autoload for several platform drivers
Hello David, I noticed that module autoload won't be working in a bunch of platform drivers in the net subsystem and this patch series contains the fixes. Best regards, Javier Javier Martinez Canillas (7): net: nps_enet: Fix module autoload net: ethernet: nb8800: Fix module autoload net: hns: Fix hns_dsaf module autoload for OF registration net: qcom/emac: Fix module autoload for OF registration net: hisilicon: Fix hns_mdio module autoload for OF registration net: dsa: b53: Fix module autoload net: dsa: bcm_sf2: Fix module autoload for OF registration drivers/net/dsa/b53/b53_mmap.c | 1 + drivers/net/dsa/bcm_sf2.c | 1 + drivers/net/ethernet/aurora/nb8800.c | 1 + drivers/net/ethernet/ezchip/nps_enet.c | 1 + drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 1 + drivers/net/ethernet/hisilicon/hns_mdio.c | 1 + drivers/net/ethernet/qualcomm/emac/emac.c | 1 + 7 files changed, 7 insertions(+) -- 2.7.4
[PATCH] net: wan: slic_ds26522: Allow driver to built if COMPILE_TEST is enabled
The driver only has runtime but no build time dependency with FSL_SOC || ARCH_MXC || ARCH_LAYERSCAPE. So it can be built for testing purposes if the COMPILE_TEST option is enabled. This is useful to have more build coverage and make sure that the driver is not affected by changes that could cause build regressions. Signed-off-by: Javier Martinez Canillas --- drivers/net/wan/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wan/Kconfig b/drivers/net/wan/Kconfig index 33ab3345d333..4e9fe75d7067 100644 --- a/drivers/net/wan/Kconfig +++ b/drivers/net/wan/Kconfig @@ -294,7 +294,7 @@ config FSL_UCC_HDLC config SLIC_DS26522 tristate "Slic Maxim ds26522 card support" depends on SPI - depends on FSL_SOC || ARCH_MXC || ARCH_LAYERSCAPE + depends on FSL_SOC || ARCH_MXC || ARCH_LAYERSCAPE || COMPILE_TEST help This module initializes and configures the slic maxim card in T1 or E1 mode. -- 2.7.4
[PATCH 1/2] net: wan: slic_ds26522: add SPI device ID table to fix module autoload
If the driver is built as a module, module alias information isn't filled so the module won't be autoloaded. Add a SPI device ID table and use the MODULE_DEVICE_TABLE() macro so the information is exported in the module. Before this patch: $ modinfo drivers/net/wan/slic_ds26522.ko | grep alias $ After this patch: $ modinfo drivers/net/wan/slic_ds26522.ko | grep alias alias: spi:ds26522 Signed-off-by: Javier Martinez Canillas --- drivers/net/wan/slic_ds26522.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/net/wan/slic_ds26522.c b/drivers/net/wan/slic_ds26522.c index d06a887a2352..53366a2232f0 100644 --- a/drivers/net/wan/slic_ds26522.c +++ b/drivers/net/wan/slic_ds26522.c @@ -223,6 +223,12 @@ static int slic_ds26522_probe(struct spi_device *spi) return ret; } +static const struct spi_device_id slic_ds26522_id[] = { + { .name = "ds26522" }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(spi, slic_ds26522_id); + static const struct of_device_id slic_ds26522_match[] = { { .compatible = "maxim,ds26522", @@ -239,6 +245,7 @@ static struct spi_driver slic_ds26522_driver = { }, .probe = slic_ds26522_probe, .remove = slic_ds26522_remove, + .id_table = slic_ds26522_id, }; static int __init slic_ds26522_init(void) -- 2.7.4
[PATCH 2/2] net: wan: slic_ds26522: Export OF module alias information
When the device is registered via OF, the OF table is used to match the driver instead of the SPI device ID table, but the entries in the later are used as aliasses to load the module if the driver was not built-in. This is because the SPI core always reports an SPI module alias instead of an OF one, but that could change so it's better to always export it. Signed-off-by: Javier Martinez Canillas --- drivers/net/wan/slic_ds26522.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/wan/slic_ds26522.c b/drivers/net/wan/slic_ds26522.c index 53366a2232f0..b776a0ab106c 100644 --- a/drivers/net/wan/slic_ds26522.c +++ b/drivers/net/wan/slic_ds26522.c @@ -235,6 +235,7 @@ static const struct of_device_id slic_ds26522_match[] = { }, {}, }; +MODULE_DEVICE_TABLE(of, slic_ds26522_match); static struct spi_driver slic_ds26522_driver = { .driver = { -- 2.7.4
[PATCH 12/15] sis900: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/sis/sis900.c | 4 ++-- drivers/net/ethernet/sis/sis900.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/sis/sis900.c b/drivers/net/ethernet/sis/sis900.c index 95001ee408ab..6f85276376e8 100644 --- a/drivers/net/ethernet/sis/sis900.c +++ b/drivers/net/ethernet/sis/sis900.c @@ -1426,7 +1426,7 @@ static void sis900_set_mode(struct sis900_private *sp, int speed, int duplex) rx_flags |= RxATX; } -#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE) +#if IS_ENABLED(CONFIG_VLAN_8021Q) /* Can accept Jumbo packet */ rx_flags |= RxAJAB; #endif @@ -1750,7 +1750,7 @@ static int sis900_rx(struct net_device *net_dev) data_size = rx_status & DSIZE; rx_size = data_size - CRC_SIZE; -#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE) +#if IS_ENABLED(CONFIG_VLAN_8021Q) /* ``TOOLONG'' flag means jumbo packet received. */ if ((rx_status & TOOLONG) && data_size <= MAX_FRAME_SIZE) rx_status &= (~ ((unsigned int)TOOLONG)); diff --git a/drivers/net/ethernet/sis/sis900.h b/drivers/net/ethernet/sis/sis900.h index 7d430d322931..f0da3dc52c01 100644 --- a/drivers/net/ethernet/sis/sis900.h +++ b/drivers/net/ethernet/sis/sis900.h @@ -310,7 +310,7 @@ enum sis630_revision_id { #define CRC_SIZE4 #define MAC_HEADER_SIZE 14 -#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE) +#if IS_ENABLED(CONFIG_VLAN_8021Q) #define MAX_FRAME_SIZE (1518 + 4) #else #define MAX_FRAME_SIZE 1518 -- 2.7.4
[PATCH 06/15] net/fsl_pq_mdio: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/freescale/fsl_pq_mdio.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c index f3c63dce1e30..446c7b374ff5 100644 --- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c +++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c @@ -195,7 +195,7 @@ static int fsl_pq_mdio_reset(struct mii_bus *bus) return 0; } -#if defined(CONFIG_GIANFAR) || defined(CONFIG_GIANFAR_MODULE) +#if IS_ENABLED(CONFIG_GIANFAR) /* * Return the TBIPA address, starting from the address * of the mapped GFAR MDIO registers (struct gfar) @@ -228,7 +228,7 @@ static uint32_t __iomem *get_etsec_tbipa(void __iomem *p) } #endif -#if defined(CONFIG_UCC_GETH) || defined(CONFIG_UCC_GETH_MODULE) +#if IS_ENABLED(CONFIG_UCC_GETH) /* * Return the TBIPAR address for a QE MDIO node, starting from the address * of the mapped MII registers (struct fsl_pq_mii) @@ -306,7 +306,7 @@ static void ucc_configure(phys_addr_t start, phys_addr_t end) #endif static const struct of_device_id fsl_pq_mdio_match[] = { -#if defined(CONFIG_GIANFAR) || defined(CONFIG_GIANFAR_MODULE) +#if IS_ENABLED(CONFIG_GIANFAR) { .compatible = "fsl,gianfar-tbi", .data = &(struct fsl_pq_mdio_data) { @@ -344,7 +344,7 @@ static const struct of_device_id fsl_pq_mdio_match[] = { }, }, #endif -#if defined(CONFIG_UCC_GETH) || defined(CONFIG_UCC_GETH_MODULE) +#if IS_ENABLED(CONFIG_UCC_GETH) { .compatible = "fsl,ucc-mdio", .data = &(struct fsl_pq_mdio_data) { -- 2.7.4
[PATCH 02/15] starfire: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/adaptec/starfire.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/adaptec/starfire.c b/drivers/net/ethernet/adaptec/starfire.c index 1d1069641d81..8af2c88d5b33 100644 --- a/drivers/net/ethernet/adaptec/starfire.c +++ b/drivers/net/ethernet/adaptec/starfire.c @@ -66,7 +66,7 @@ */ #define ZEROCOPY -#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE) +#if IS_ENABLED(CONFIG_VLAN_8021Q) #define VLAN_SUPPORT #endif -- 2.7.4
[PATCH 04/15] bnx2: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/broadcom/bnx2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c index 8fc3f3c137f8..ecd357dbb1d4 100644 --- a/drivers/net/ethernet/broadcom/bnx2.c +++ b/drivers/net/ethernet/broadcom/bnx2.c @@ -50,7 +50,7 @@ #include #include -#if defined(CONFIG_CNIC) || defined(CONFIG_CNIC_MODULE) +#if IS_ENABLED(CONFIG_CNIC) #define BCM_CNIC 1 #include "cnic_if.h" #endif -- 2.7.4
[PATCH 05/15] sundance: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/dlink/sundance.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/dlink/sundance.c b/drivers/net/ethernet/dlink/sundance.c index 58c6338a839e..79d80090eac8 100644 --- a/drivers/net/ethernet/dlink/sundance.c +++ b/drivers/net/ethernet/dlink/sundance.c @@ -867,7 +867,7 @@ static int netdev_open(struct net_device *dev) /* Initialize other registers. */ __set_mac_addr(dev); -#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE) +#if IS_ENABLED(CONFIG_VLAN_8021Q) iowrite16(dev->mtu + 18, ioaddr + MaxFrameSize); #else iowrite16(dev->mtu + 14, ioaddr + MaxFrameSize); -- 2.7.4
[PATCH 08/15] ixgbe: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/intel/ixgbe/ixgbe.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h index 33c025055011..b06e32d0d22a 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h @@ -45,10 +45,10 @@ #include "ixgbe_type.h" #include "ixgbe_common.h" #include "ixgbe_dcb.h" -#if defined(CONFIG_FCOE) || defined(CONFIG_FCOE_MODULE) +#if IS_ENABLED(CONFIG_FCOE) #define IXGBE_FCOE #include "ixgbe_fcoe.h" -#endif /* CONFIG_FCOE or CONFIG_FCOE_MODULE */ +#endif /* IS_ENABLED(CONFIG_FCOE) */ #ifdef CONFIG_IXGBE_DCA #include #endif -- 2.7.4
[PATCH 03/15] ethernet: amd: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/amd/7990.c | 6 +++--- drivers/net/ethernet/amd/amd8111e.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/amd/7990.c b/drivers/net/ethernet/amd/7990.c index dcf2a1f3643d..dc57f2759f44 100644 --- a/drivers/net/ethernet/amd/7990.c +++ b/drivers/net/ethernet/amd/7990.c @@ -45,14 +45,14 @@ #define WRITERDP(lp, x)out_be16(lp->base + LANCE_RDP, (x)) #define READRDP(lp)in_be16(lp->base + LANCE_RDP) -#if defined(CONFIG_HPLANCE) || defined(CONFIG_HPLANCE_MODULE) +#if IS_ENABLED(CONFIG_HPLANCE) #include "hplance.h" #undef WRITERAP #undef WRITERDP #undef READRDP -#if defined(CONFIG_MVME147_NET) || defined(CONFIG_MVME147_NET_MODULE) +#if IS_ENABLED(CONFIG_MVME147_NET) /* Lossage Factor Nine, Mr Sulu. */ #define WRITERAP(lp, x)(lp->writerap(lp, x)) @@ -86,7 +86,7 @@ static inline __u16 READRDP(struct lance_private *lp) } #endif -#endif /* CONFIG_HPLANCE || CONFIG_HPLANCE_MODULE */ +#endif /* IS_ENABLED(CONFIG_HPLANCE) */ /* debugging output macros, various flavours */ /* #define TEST_HITS */ diff --git a/drivers/net/ethernet/amd/amd8111e.c b/drivers/net/ethernet/amd/amd8111e.c index 94960055fa1f..f92cc97151ec 100644 --- a/drivers/net/ethernet/amd/amd8111e.c +++ b/drivers/net/ethernet/amd/amd8111e.c @@ -89,7 +89,7 @@ Revision History: #include #include -#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE) +#if IS_ENABLED(CONFIG_VLAN_8021Q) #define AMD8111E_VLAN_TAG_USED 1 #else #define AMD8111E_VLAN_TAG_USED 0 -- 2.7.4
[PATCH 00/15] drivers: net: use IS_ENABLED() instead of checking for built-in or module
Hello David, This trivial series is similar to [0] for net/ that you already merged, but for drivers/net. The patches replaces the open coding to check for a Kconfig symbol being built-in or module, with IS_ENABLED() macro that does the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. [0]: https://lkml.org/lkml/2016/9/9/323 Best regards, Javier Javier Martinez Canillas (15): 3c59x: use IS_ENABLED() instead of checking for built-in or module starfire: use IS_ENABLED() instead of checking for built-in or module ethernet: amd: use IS_ENABLED() instead of checking for built-in or module bnx2: use IS_ENABLED() instead of checking for built-in or module sundance: use IS_ENABLED() instead of checking for built-in or module net/fsl_pq_mdio: use IS_ENABLED() instead of checking for built-in or module i825xx: use IS_ENABLED() instead of checking for built-in or module ixgbe: use IS_ENABLED() instead of checking for built-in or module net: mvneta: use IS_ENABLED() instead of checking for built-in or module natsemi: use IS_ENABLED() instead of checking for built-in or module sfc: use IS_ENABLED() instead of checking for built-in or module sis900: use IS_ENABLED() instead of checking for built-in or module stmmac: use IS_ENABLED() instead of checking for built-in or module hamradio: use IS_ENABLED() instead of checking for built-in or module iwlegacy: use IS_ENABLED() instead of checking for built-in or module drivers/net/ethernet/3com/3c59x.c| 2 +- drivers/net/ethernet/adaptec/starfire.c | 2 +- drivers/net/ethernet/amd/7990.c | 6 +++--- drivers/net/ethernet/amd/amd8111e.c | 2 +- drivers/net/ethernet/broadcom/bnx2.c | 2 +- drivers/net/ethernet/dlink/sundance.c| 2 +- drivers/net/ethernet/freescale/fsl_pq_mdio.c | 8 drivers/net/ethernet/i825xx/82596.c | 4 ++-- drivers/net/ethernet/intel/ixgbe/ixgbe.h | 4 ++-- drivers/net/ethernet/marvell/mvneta_bm.h | 2 +- drivers/net/ethernet/natsemi/ns83820.c | 2 +- drivers/net/ethernet/sfc/falcon_boards.c | 4 ++-- drivers/net/ethernet/sis/sis900.c| 4 ++-- drivers/net/ethernet/sis/sis900.h| 2 +- drivers/net/ethernet/stmicro/stmmac/common.h | 2 +- drivers/net/hamradio/bpqether.c | 2 +- drivers/net/wireless/intel/iwlegacy/common.h | 4 ++-- 17 files changed, 27 insertions(+), 27 deletions(-) -- 2.7.4
[PATCH 07/15] i825xx: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/i825xx/82596.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/i825xx/82596.c b/drivers/net/ethernet/i825xx/82596.c index befb4ac3e2b0..ce235b776793 100644 --- a/drivers/net/ethernet/i825xx/82596.c +++ b/drivers/net/ethernet/i825xx/82596.c @@ -89,10 +89,10 @@ static char version[] __initdata = #define DEB(x,y) if (i596_debug & (x)) y -#if defined(CONFIG_MVME16x_NET) || defined(CONFIG_MVME16x_NET_MODULE) +#if IS_ENABLED(CONFIG_MVME16x_NET) #define ENABLE_MVME16x_NET #endif -#if defined(CONFIG_BVME6000_NET) || defined(CONFIG_BVME6000_NET_MODULE) +#if IS_ENABLED(CONFIG_BVME6000_NET) #define ENABLE_BVME6000_NET #endif -- 2.7.4
[PATCH 09/15] net: mvneta: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/marvell/mvneta_bm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/marvell/mvneta_bm.h b/drivers/net/ethernet/marvell/mvneta_bm.h index e74fd44a92f7..a32de432800c 100644 --- a/drivers/net/ethernet/marvell/mvneta_bm.h +++ b/drivers/net/ethernet/marvell/mvneta_bm.h @@ -133,7 +133,7 @@ struct mvneta_bm_pool { void *mvneta_frag_alloc(unsigned int frag_size); void mvneta_frag_free(unsigned int frag_size, void *data); -#if defined(CONFIG_MVNETA_BM) || defined(CONFIG_MVNETA_BM_MODULE) +#if IS_ENABLED(CONFIG_MVNETA_BM) void mvneta_bm_pool_destroy(struct mvneta_bm *priv, struct mvneta_bm_pool *bm_pool, u8 port_map); void mvneta_bm_bufs_free(struct mvneta_bm *priv, struct mvneta_bm_pool *bm_pool, -- 2.7.4
[PATCH 11/15] sfc: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/sfc/falcon_boards.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/sfc/falcon_boards.c b/drivers/net/ethernet/sfc/falcon_boards.c index 1736f4b806af..f6883b2b5da3 100644 --- a/drivers/net/ethernet/sfc/falcon_boards.c +++ b/drivers/net/ethernet/sfc/falcon_boards.c @@ -64,7 +64,7 @@ #define LM87_ALARM_TEMP_INT0x10 #define LM87_ALARM_TEMP_EXT1 0x20 -#if defined(CONFIG_SENSORS_LM87) || defined(CONFIG_SENSORS_LM87_MODULE) +#if IS_ENABLED(CONFIG_SENSORS_LM87) static int efx_poke_lm87(struct i2c_client *client, const u8 *reg_values) { @@ -455,7 +455,7 @@ static int sfe4001_init(struct efx_nic *efx) struct falcon_board *board = falcon_board(efx); int rc; -#if defined(CONFIG_SENSORS_LM90) || defined(CONFIG_SENSORS_LM90_MODULE) +#if IS_ENABLED(CONFIG_SENSORS_LM90) board->hwmon_client = i2c_new_device(&board->i2c_adap, &sfe4001_hwmon_info); #else -- 2.7.4
[PATCH 13/15] stmmac: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/stmicro/stmmac/common.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h index 2533b91f1421..d3292c4a6eda 100644 --- a/drivers/net/ethernet/stmicro/stmmac/common.h +++ b/drivers/net/ethernet/stmicro/stmmac/common.h @@ -30,7 +30,7 @@ #include #include #include -#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE) +#if IS_ENABLED(CONFIG_VLAN_8021Q) #define STMMAC_VLAN_TAG_USED #include #endif -- 2.7.4
[PATCH 15/15] iwlegacy: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/intel/iwlegacy/common.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/intel/iwlegacy/common.h b/drivers/net/wireless/intel/iwlegacy/common.h index 726ede391cb9..3bba521d2cd9 100644 --- a/drivers/net/wireless/intel/iwlegacy/common.h +++ b/drivers/net/wireless/intel/iwlegacy/common.h @@ -1320,7 +1320,7 @@ struct il_priv { u64 timestamp; union { -#if defined(CONFIG_IWL3945) || defined(CONFIG_IWL3945_MODULE) +#if IS_ENABLED(CONFIG_IWL3945) struct { void *shared_virt; dma_addr_t shared_phys; @@ -1351,7 +1351,7 @@ struct il_priv { } _3945; #endif -#if defined(CONFIG_IWL4965) || defined(CONFIG_IWL4965_MODULE) +#if IS_ENABLED(CONFIG_IWL4965) struct { struct il_rx_phy_res last_phy_res; bool last_phy_res_valid; -- 2.7.4
[PATCH 14/15] hamradio: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/hamradio/bpqether.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/hamradio/bpqether.c b/drivers/net/hamradio/bpqether.c index d95a50ae996d..622ab3ab9e93 100644 --- a/drivers/net/hamradio/bpqether.c +++ b/drivers/net/hamradio/bpqether.c @@ -484,7 +484,7 @@ static void bpq_setup(struct net_device *dev) dev->flags = 0; dev->features = NETIF_F_LLTX; /* Allow recursion */ -#if defined(CONFIG_AX25) || defined(CONFIG_AX25_MODULE) +#if IS_ENABLED(CONFIG_AX25) dev->header_ops = &ax25_header_ops; #endif -- 2.7.4
[PATCH 10/15] natsemi: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/natsemi/ns83820.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c index eb807b0dc72a..569ade6cf85c 100644 --- a/drivers/net/ethernet/natsemi/ns83820.c +++ b/drivers/net/ethernet/natsemi/ns83820.c @@ -134,7 +134,7 @@ static int lnksts = 0; /* CFG_LNKSTS bit polarity */ /* tunables */ #define RX_BUF_SIZE1500/* 8192 */ -#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE) +#if IS_ENABLED(CONFIG_VLAN_8021Q) #define NS83820_VLAN_ACCEL_SUPPORT #endif -- 2.7.4
[PATCH 01/15] 3c59x: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/3com/3c59x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/3com/3c59x.c b/drivers/net/ethernet/3com/3c59x.c index 25c55ab05c7d..9133e7926da5 100644 --- a/drivers/net/ethernet/3com/3c59x.c +++ b/drivers/net/ethernet/3com/3c59x.c @@ -3089,7 +3089,7 @@ static void set_rx_mode(struct net_device *dev) iowrite16(new_mode, ioaddr + EL3_CMD); } -#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE) +#if IS_ENABLED(CONFIG_VLAN_8021Q) /* Setup the card so that it can receive frames with an 802.1q VLAN tag. Note that this must be done after each RxReset due to some backwards compatibility logic in the Cyclone and Tornado ASICs */ -- 2.7.4
[PATCH 2/8] lec: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- net/atm/lec.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/net/atm/lec.c b/net/atm/lec.c index e574a7e9db6f..5d2693826afb 100644 --- a/net/atm/lec.c +++ b/net/atm/lec.c @@ -31,7 +31,7 @@ #include /* Proxy LEC knows about bridging */ -#if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE) +#if IS_ENABLED(CONFIG_BRIDGE) #include "../bridge/br_private.h" static unsigned char bridge_ula_lec[] = { 0x01, 0x80, 0xc2, 0x00, 0x00 }; @@ -121,7 +121,7 @@ static unsigned char bus_mac[ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; /* Device structures */ static struct net_device *dev_lec[MAX_LEC_ITF]; -#if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE) +#if IS_ENABLED(CONFIG_BRIDGE) static void lec_handle_bridge(struct sk_buff *skb, struct net_device *dev) { char *buff; @@ -155,7 +155,7 @@ static void lec_handle_bridge(struct sk_buff *skb, struct net_device *dev) sk->sk_data_ready(sk); } } -#endif /* defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE) */ +#endif /* IS_ENABLED(CONFIG_BRIDGE) */ /* * Open/initialize the netdevice. This is called (in the current kernel) @@ -222,7 +222,7 @@ static netdev_tx_t lec_start_xmit(struct sk_buff *skb, pr_debug("skbuff head:%lx data:%lx tail:%lx end:%lx\n", (long)skb->head, (long)skb->data, (long)skb_tail_pointer(skb), (long)skb_end_pointer(skb)); -#if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE) +#if IS_ENABLED(CONFIG_BRIDGE) if (memcmp(skb->data, bridge_ula_lec, sizeof(bridge_ula_lec)) == 0) lec_handle_bridge(skb, dev); #endif @@ -426,7 +426,7 @@ static int lec_atm_send(struct atm_vcc *vcc, struct sk_buff *skb) (unsigned short)(0x & mesg->content.normal.flag); break; case l_should_bridge: -#if defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE) +#if IS_ENABLED(CONFIG_BRIDGE) { pr_debug("%s: bridge zeppelin asks about %pM\n", dev->name, mesg->content.proxy.mac_addr); @@ -452,7 +452,7 @@ static int lec_atm_send(struct atm_vcc *vcc, struct sk_buff *skb) sk->sk_data_ready(sk); } } -#endif /* defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE) */ +#endif /* IS_ENABLED(CONFIG_BRIDGE) */ break; default: pr_info("%s: Unknown message type %d\n", dev->name, mesg->type); -- 2.7.4
[PATCH 1/8] appletalk: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- net/appletalk/ddp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c index f066781be3c8..10d2bdce686e 100644 --- a/net/appletalk/ddp.c +++ b/net/appletalk/ddp.c @@ -1278,7 +1278,7 @@ out: return err; } -#if defined(CONFIG_IPDDP) || defined(CONFIG_IPDDP_MODULE) +#if IS_ENABLED(CONFIG_IPDDP) static __inline__ int is_ip_over_ddp(struct sk_buff *skb) { return skb->data[12] == 22; -- 2.7.4
[PATCH 4/8] ipv4: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- net/ipv4/ip_output.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index 65569274efb8..b913f5bf0757 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -490,7 +490,7 @@ static void ip_copy_metadata(struct sk_buff *to, struct sk_buff *from) to->tc_index = from->tc_index; #endif nf_copy(to, from); -#if defined(CONFIG_IP_VS) || defined(CONFIG_IP_VS_MODULE) +#if IS_ENABLED(CONFIG_IP_VS) to->ipvs_property = from->ipvs_property; #endif skb_copy_secmark(to, from); -- 2.7.4
[PATCH 0/8] net: use IS_ENABLED() instead of checking for built-in or module
Hello David, This trivial series replace the open coding to check for a Kconfig symbol being built-in or module, with IS_ENABLED() macro that does exactly that. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Best regards, Javier Javier Martinez Canillas (8): appletalk: use IS_ENABLED() instead of checking for built-in or module lec: use IS_ENABLED() instead of checking for built-in or module net: use IS_ENABLED() instead of checking for built-in or module ipv4: use IS_ENABLED() instead of checking for built-in or module l2tp: use IS_ENABLED() instead of checking for built-in or module net: sched: use IS_ENABLED() instead of checking for built-in or module sctp: use IS_ENABLED() instead of checking for built-in or module xfrm: use IS_ENABLED() instead of checking for built-in or module net/appletalk/ddp.c | 2 +- net/atm/lec.c| 12 ++-- net/core/dev.c | 3 +-- net/ipv4/ip_output.c | 2 +- net/l2tp/l2tp_core.h | 2 +- net/l2tp/l2tp_eth.c | 4 ++-- net/l2tp/l2tp_ppp.c | 4 ++-- net/sched/cls_flow.c | 6 +++--- net/sctp/auth.c | 2 +- net/xfrm/xfrm_algo.c | 2 +- 10 files changed, 19 insertions(+), 20 deletions(-) -- 2.7.4
[PATCH 8/8] xfrm: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- net/xfrm/xfrm_algo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/xfrm/xfrm_algo.c b/net/xfrm/xfrm_algo.c index 250e567ba3d6..44ac85fe2bc9 100644 --- a/net/xfrm/xfrm_algo.c +++ b/net/xfrm/xfrm_algo.c @@ -17,7 +17,7 @@ #include #include #include -#if defined(CONFIG_INET_ESP) || defined(CONFIG_INET_ESP_MODULE) || defined(CONFIG_INET6_ESP) || defined(CONFIG_INET6_ESP_MODULE) +#if IS_ENABLED(CONFIG_INET_ESP) || IS_ENABLED(CONFIG_INET6_ESP) #include #endif -- 2.7.4
[PATCH 5/8] l2tp: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- net/l2tp/l2tp_core.h | 2 +- net/l2tp/l2tp_eth.c | 4 ++-- net/l2tp/l2tp_ppp.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h index 5871537af387..2599af6378e4 100644 --- a/net/l2tp/l2tp_core.h +++ b/net/l2tp/l2tp_core.h @@ -139,7 +139,7 @@ struct l2tp_session { void (*session_close)(struct l2tp_session *session); void (*ref)(struct l2tp_session *session); void (*deref)(struct l2tp_session *session); -#if defined(CONFIG_L2TP_DEBUGFS) || defined(CONFIG_L2TP_DEBUGFS_MODULE) +#if IS_ENABLED(CONFIG_L2TP_DEBUGFS) void (*show)(struct seq_file *m, void *priv); #endif uint8_t priv[0];/* private data */ diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c index 57fc5a46ce06..ef2cd30ca06e 100644 --- a/net/l2tp/l2tp_eth.c +++ b/net/l2tp/l2tp_eth.c @@ -195,7 +195,7 @@ static void l2tp_eth_delete(struct l2tp_session *session) } } -#if defined(CONFIG_L2TP_DEBUGFS) || defined(CONFIG_L2TP_DEBUGFS_MODULE) +#if IS_ENABLED(CONFIG_L2TP_DEBUGFS) static void l2tp_eth_show(struct seq_file *m, void *arg) { struct l2tp_session *session = arg; @@ -268,7 +268,7 @@ static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 p priv->tunnel_sock = tunnel->sock; session->recv_skb = l2tp_eth_dev_recv; session->session_close = l2tp_eth_delete; -#if defined(CONFIG_L2TP_DEBUGFS) || defined(CONFIG_L2TP_DEBUGFS_MODULE) +#if IS_ENABLED(CONFIG_L2TP_DEBUGFS) session->show = l2tp_eth_show; #endif diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c index 34eff77982cf..41d47bfda15c 100644 --- a/net/l2tp/l2tp_ppp.c +++ b/net/l2tp/l2tp_ppp.c @@ -552,7 +552,7 @@ out: return error; } -#if defined(CONFIG_L2TP_DEBUGFS) || defined(CONFIG_L2TP_DEBUGFS_MODULE) +#if IS_ENABLED(CONFIG_L2TP_DEBUGFS) static void pppol2tp_show(struct seq_file *m, void *arg) { struct l2tp_session *session = arg; @@ -723,7 +723,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr, session->recv_skb = pppol2tp_recv; session->session_close = pppol2tp_session_close; -#if defined(CONFIG_L2TP_DEBUGFS) || defined(CONFIG_L2TP_DEBUGFS_MODULE) +#if IS_ENABLED(CONFIG_L2TP_DEBUGFS) session->show = pppol2tp_show; #endif -- 2.7.4
[PATCH 7/8] sctp: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- net/sctp/auth.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/sctp/auth.c b/net/sctp/auth.c index 912eb1685a5d..f99d4855d3de 100644 --- a/net/sctp/auth.c +++ b/net/sctp/auth.c @@ -48,7 +48,7 @@ static struct sctp_hmac sctp_hmac_list[SCTP_AUTH_NUM_HMACS] = { /* id 2 is reserved as well */ .hmac_id = SCTP_AUTH_HMAC_ID_RESERVED_2, }, -#if defined (CONFIG_CRYPTO_SHA256) || defined (CONFIG_CRYPTO_SHA256_MODULE) +#if IS_ENABLED(CONFIG_CRYPTO_SHA256) { .hmac_id = SCTP_AUTH_HMAC_ID_SHA256, .hmac_name = "hmac(sha256)", -- 2.7.4
[PATCH 6/8] net: sched: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- net/sched/cls_flow.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c index 2c1ae549edbf..a379bae1d74e 100644 --- a/net/sched/cls_flow.c +++ b/net/sched/cls_flow.c @@ -29,7 +29,7 @@ #include #include -#if defined(CONFIG_NF_CONNTRACK) || defined(CONFIG_NF_CONNTRACK_MODULE) +#if IS_ENABLED(CONFIG_NF_CONNTRACK) #include #endif @@ -125,14 +125,14 @@ static u32 flow_get_mark(const struct sk_buff *skb) static u32 flow_get_nfct(const struct sk_buff *skb) { -#if defined(CONFIG_NF_CONNTRACK) || defined(CONFIG_NF_CONNTRACK_MODULE) +#if IS_ENABLED(CONFIG_NF_CONNTRACK) return addr_fold(skb->nfct); #else return 0; #endif } -#if defined(CONFIG_NF_CONNTRACK) || defined(CONFIG_NF_CONNTRACK_MODULE) +#if IS_ENABLED(CONFIG_NF_CONNTRACK) #define CTTUPLE(skb, member) \ ({ \ enum ip_conntrack_info ctinfo; \ -- 2.7.4
[PATCH 3/8] net: use IS_ENABLED() instead of checking for built-in or module
The IS_ENABLED() macro checks if a Kconfig symbol has been enabled either built-in or as a module, use that macro instead of open coding the same. Using the macro makes the code more readable by helping abstract away some of the Kconfig built-in and module enable details. Signed-off-by: Javier Martinez Canillas --- net/core/dev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index c7d7db49826d..9e76d4df6ca0 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3904,8 +3904,7 @@ static __latent_entropy void net_tx_action(struct softirq_action *h) } } -#if (defined(CONFIG_BRIDGE) || defined(CONFIG_BRIDGE_MODULE)) && \ -(defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE)) +#if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_ATM_LANE) /* This hook is defined here for ATM LANE */ int (*br_fdb_test_addr_hook)(struct net_device *dev, unsigned char *addr) __read_mostly; -- 2.7.4
Re: mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()
Hello Amitkumar, On 09/08/2016 11:55 AM, Amitkumar Karwar wrote: > Hi Javier, > >> From: Javier Martinez Canillas [mailto:jav...@osg.samsung.com] >> Sent: Tuesday, September 06, 2016 5:43 PM >> To: Kalle Valo >> Cc: linux-ker...@vger.kernel.org; Amitkumar Karwar; >> netdev@vger.kernel.org; linux-wirel...@vger.kernel.org; Nishant >> Sarmukadam; Arend van Spriel >> Subject: Re: mwifiex: propagate error if IRQ request fails in >> mwifiex_sdio_of() >> >> Hello Kalle, >> >> On 09/03/2016 12:35 PM, Kalle Valo wrote: >>> Javier Martinez Canillas wrote: >>>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error >>>> message is printed but the actual error is not propagated to the >> caller function. >>>> >>>> Signed-off-by: Javier Martinez Canillas >>> >>> What's the conclusion with this patch? Should I drop it or take it? >>> >>> (The discussion is available from the patchwork link in the >>> signature.) >>> >> >> My understanding is that Arend agrees with the patch and that the >> question raised was caused by looking at an older kernel version. IOW, >> the patch is OK and should be picked. >> >> I'm adding Arend to cc, so can comment in case I misunderstood him >> though. >> > > This error doesn't affect actual wifi functionality. Only thing is wakeup on > interrupt when system is in suspended state won't work. > I think, we can make below change. > > -- > @@ -122,9 +122,11 @@ static int mwifiex_sdio_probe_of(struct device *dev, > struct sdio_mmc_card *card) >IRQF_TRIGGER_LOW, >"wifi_wake", cfg); >if (ret) { > -dev_err(dev, > +dev_dbg(dev, > "Failed to request irq_wifi %d (%d)\n", > cfg->irq_wifi, ret); > +card->plt_wake_cfg = NULL; > +return 0; > } I'm OK with that change. Feel free too add my Reviewed-by if you post it. > disable_irq(cfg->irq_wifi); > } > - > > Regards, > Amitkumar > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
Re: mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()
Hello Kalle, On 09/03/2016 12:35 PM, Kalle Valo wrote: > Javier Martinez Canillas wrote: >> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message >> is printed but the actual error is not propagated to the caller function. >> >> Signed-off-by: Javier Martinez Canillas > > What's the conclusion with this patch? Should I drop it or take it? > > (The discussion is available from the patchwork link in the signature.) > My understanding is that Arend agrees with the patch and that the question raised was caused by looking at an older kernel version. IOW, the patch is OK and should be picked. I'm adding Arend to cc, so can comment in case I misunderstood him though. Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
Re: [PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()
Hello Arend, On 08/18/2016 03:49 PM, Arend van Spriel wrote: > > > On 18-08-16 21:29, Javier Martinez Canillas wrote: >> Hello Arend, >> >> Thanks a lot for your feedback. >> >> On 08/18/2016 03:14 PM, Arend van Spriel wrote: >>> On 18-08-16 16:17, Javier Martinez Canillas wrote: >>>> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message >>>> is printed but the actual error is not propagated to the caller function. >>> >>> Hmm. The caller function, ie. mwifiex_sdio_probe(), does not seem to care. >>> >> >> Hmm, I'm not so sure about that. It's checking the wifiex_sdio_probe_of() >> return value. > > Ok. I looked at 4.7 sources on lxr [1]. > Oh, right. That was fixed quite recently indeed. >> If the IRQ request failing is not an error, then at the very least the call >> to disable_irq() should be avoided if request_irq() fails, and the message >> should be changed from dev_err() to dev_dgb() or dev_info(). > > agree. > >>> The device may still function without this wake interrupt. >>> >> >> That's correct, the binding says that the "interrupts" property in the child >> node is optional since is just a wakeup IRQ. Now the question is if should >> be an error if the IRQ is defined but fails to be requested. > > Clearly it indicates an error in the DT specification so behavior is not > as expected. Personally I would indeed consider it an error, but I was > just indicating that it might have done like this intentionally. > Yes, might had been done intentionally indeed but I don't think that is the case since the driver lacked error checking and propagation in many places. But if someone thinks that's better to not honor the DT and at least have the driver working without the wake up capability, then I'm happy to respin the patch and change the print log level to info/debug. > Regards, > Arend > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
Re: [PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()
Hello Arend, Thanks a lot for your feedback. On 08/18/2016 03:14 PM, Arend van Spriel wrote: > On 18-08-16 16:17, Javier Martinez Canillas wrote: >> If request_irq() fails in mwifiex_sdio_probe_of(), only an error message >> is printed but the actual error is not propagated to the caller function. > > Hmm. The caller function, ie. mwifiex_sdio_probe(), does not seem to care. > Hmm, I'm not so sure about that. It's checking the wifiex_sdio_probe_of() return value. If the IRQ request failing is not an error, then at the very least the call to disable_irq() should be avoided if request_irq() fails, and the message should be changed from dev_err() to dev_dgb() or dev_info(). > The device may still function without this wake interrupt. > That's correct, the binding says that the "interrupts" property in the child node is optional since is just a wakeup IRQ. Now the question is if should be an error if the IRQ is defined but fails to be requested. > Regards, > Arend > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
[PATCH] mwifiex: propagate error if IRQ request fails in mwifiex_sdio_of()
If request_irq() fails in mwifiex_sdio_probe_of(), only an error message is printed but the actual error is not propagated to the caller function. Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/sdio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index d3e1561ca075..00727936ad6e 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -125,6 +125,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card) dev_err(dev, "Failed to request irq_wifi %d (%d)\n", cfg->irq_wifi, ret); + return ret; } disable_irq(cfg->irq_wifi); } -- 2.5.5
Re: [PATCH] mwifiex: fix unconditional error return in .add_virtual_intf callback
Hello Kalle, On 07/05/2016 09:09 AM, Kalle Valo wrote: > Javier Martinez Canillas writes: > >> The commit 7311ea850079 ("mwifiex: fix AP start problem for newly added >> interface") attempted to fix an issue when a new AP interface is added. >> >> But the patch didn't check the return value of the functions doing the >> firmware calls and returned an error even if the functions didn't fail. >> >> This prevents the network device to be registered properly, so fix it. >> >> Fixes: commit 7311ea850079 ("mwifiex: fix AP start problem for newly added >> interface") >> Signed-off-by: Javier Martinez Canillas > > The fixes line should be: > > Fixes: 7311ea850079 ("mwifiex: fix AP start problem for newly added > interface") > > I can fix that before I apply the patch. > Sigh, it was a copy and paste error when I copied the SHA-1 from the commit message. Sorry about that and thanks for taking care of this. Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
[PATCH] mwifiex: fix unconditional error return in .add_virtual_intf callback
The commit 7311ea850079 ("mwifiex: fix AP start problem for newly added interface") attempted to fix an issue when a new AP interface is added. But the patch didn't check the return value of the functions doing the firmware calls and returned an error even if the functions didn't fail. This prevents the network device to be registered properly, so fix it. Fixes: commit 7311ea850079 ("mwifiex: fix AP start problem for newly added interface") Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/cfg80211.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c index 99e8cf1ad610..5de9f63e2c01 100644 --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c @@ -2865,9 +2865,11 @@ struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy, ret = mwifiex_send_cmd(priv, HostCmd_CMD_SET_BSS_MODE, HostCmd_ACT_GEN_SET, 0, NULL, true); + if (ret) return ERR_PTR(ret); ret = mwifiex_sta_init_cmd(priv, false, false); + if (ret) return ERR_PTR(ret); mwifiex_setup_ht_caps(&wiphy->bands[NL80211_BAND_2GHZ]->ht_cap, priv); -- 2.5.5
Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file
Hello Kalle, On 06/22/2016 02:17 AM, Kalle Valo wrote: > Javier Martinez Canillas writes: > >>>> Patch 3/3 applies cleanly even after dropping patch 2/3. >>>> Is that Ok for you or do you want me to re-resend a v3 >>>> with only patches 1/3 and 3/3? >>> >>> I can drop patch 2, no need to resend. Thanks. >>> >> >> I saw that you sent your pull request for v4.8 but the >> patches from this series were not included: >> >> https://lkml.org/lkml/2016/6/21/400 > > I had forgotten to change the state of patches 1 and 3 from 'Changes > Requested' back to 'New' and that's why I missed them. I did that now so > they are back in my queue: > > https://patchwork.kernel.org/patch/9158855/ > https://patchwork.kernel.org/patch/9158851/ > > Thanks for checking. > Ok, thanks for the information. Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file
Hello Kalle, On 06/10/2016 03:54 PM, Kalle Valo wrote: > Javier Martinez Canillas writes: > >>> This patch (2/3) is only for code rearrangement and adds an >>> unnecessary wrapper function. We can simply drop the patch. >> >> Agreed. >> >> Kalle, >> >> Patch 3/3 applies cleanly even after dropping patch 2/3. >> Is that Ok for you or do you want me to re-resend a v3 >> with only patches 1/3 and 3/3? > > I can drop patch 2, no need to resend. Thanks. > I saw that you sent your pull request for v4.8 but the patches from this series were not included: https://lkml.org/lkml/2016/6/21/400 Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file
Hello Amitkumar, On 06/10/2016 12:26 PM, Amitkumar Karwar wrote: > Hi Kalle/Javier, > >> From: Javier Martinez Canillas [mailto:jav...@osg.samsung.com] >> Sent: Friday, June 10, 2016 8:07 PM >> To: Kalle Valo >> Cc: linux-ker...@vger.kernel.org; Julian Calaby; Shengzhen Li; Enric >> Balletbo i Serra; Amitkumar Karwar; netdev@vger.kernel.org; linux- >> wirel...@vger.kernel.org; Nishant Sarmukadam >> Subject: Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station >> ioctl file >> >> Hello Kalle, >> >> On 06/10/2016 10:30 AM, Kalle Valo wrote: >>> Javier Martinez Canillas writes: >>> >>>> From: Shengzhen Li >>>> >>>> Most cfg80211 operations are just a wrappers to functions defined in >>>> the sta_ioctl.c file, so for consistency move the .get_tx_power logic >> there. >>>> >>>> Signed-off-by: Shengzhen Li >>>> Signed-off-by: Amitkumar Karwar >>>> [javier: update the subject line and commit message] >>>> Signed-off-by: Javier Martinez Canillas >>> >>> [...] >>> >>>> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c >>>> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c >>>> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy >> *wiphy, >>>> int *dbm) >>>> { >>>>struct mwifiex_adapter *adapter = >> mwifiex_cfg80211_get_adapter(wiphy); >>>> - struct mwifiex_private *priv = mwifiex_get_priv(adapter, >>>> - MWIFIEX_BSS_ROLE_ANY); >>>> - int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR, >>>> - HostCmd_ACT_GEN_GET, 0, NULL, true); >>>> - >>>> - if (ret < 0) >>>> - return ret; >>>> - >>>> - /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler >> */ >>>> - *dbm = priv->tx_power_level; >>>> + struct mwifiex_private *priv; >>>> >>>> - return 0; >>>> + priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); >>>> + return mwifiex_get_tx_power(priv, dbm); >>>> } >>> >>> So in patch 1 you added the patch and in patch 2 you move it to a >>> different location? That doesn't make any sense, can't you just fold >>> the two patches into one so that the function is added only once. >>> >> >> I posted this patch in v1 but then Amitkumar shared his patch with me >> that was very similar to mine, only that the logic was in a different >> location. >> >> So I included his delta as a separate patch to try keeping attribution >> as best as possible. >> > > This patch (2/3) is only for code rearrangement and adds an unnecessary > wrapper function. We can simply drop the patch. > Agreed. Kalle, Patch 3/3 applies cleanly even after dropping patch 2/3. Is that Ok for you or do you want me to re-resend a v3 with only patches 1/3 and 3/3? > Regards, > Amitkumar > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
Re: [PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file
Hello Kalle, On 06/10/2016 10:30 AM, Kalle Valo wrote: > Javier Martinez Canillas writes: > >> From: Shengzhen Li >> >> Most cfg80211 operations are just a wrappers to functions defined in the >> sta_ioctl.c file, so for consistency move the .get_tx_power logic there. >> >> Signed-off-by: Shengzhen Li >> Signed-off-by: Amitkumar Karwar >> [javier: update the subject line and commit message] >> Signed-off-by: Javier Martinez Canillas > > [...] > >> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c >> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c >> @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy, >>int *dbm) >> { >> struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy); >> -struct mwifiex_private *priv = mwifiex_get_priv(adapter, >> -MWIFIEX_BSS_ROLE_ANY); >> -int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR, >> - HostCmd_ACT_GEN_GET, 0, NULL, true); >> - >> -if (ret < 0) >> -return ret; >> - >> -/* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */ >> -*dbm = priv->tx_power_level; >> +struct mwifiex_private *priv; >> >> -return 0; >> +priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); >> +return mwifiex_get_tx_power(priv, dbm); >> } > > So in patch 1 you added the patch and in patch 2 you move it to a > different location? That doesn't make any sense, can't you just fold the > two patches into one so that the function is added only once. > I posted this patch in v1 but then Amitkumar shared his patch with me that was very similar to mine, only that the logic was in a different location. So I included his delta as a separate patch to try keeping attribution as best as possible. Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
[PATCH v2 3/3] mwifiex: add get_antenna support for cfg80211
From: Shengzhen Li Since commit de3bb771f471 ("cfg80211: add more warnings for inconsistent ops") the wireless core warns if a driver implements a cfg80211 callback but doesn't implements the inverse operation. The mwifiex driver defines a .set_antenna handler but not a .get_antenna so this not only makes the core to print a warning when creating a new wiphy but also the antenna isn't reported to user-space apps such as iw. This patch queries the antenna to the firmware so is properly reported to user-space. With this patch, the wireless core does not warn anymore and: $ iw phy phy0 info | grep Antennas Available Antennas: TX 0x3 RX 0x3 Configured Antennas: TX 0x3 RX 0x3 Signed-off-by: Shengzhen Li Signed-off-by: Amitkumar Karwar [javier: expand the commit message] Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/cfg80211.c| 16 +++ drivers/net/wireless/marvell/mwifiex/fw.h | 3 ++ drivers/net/wireless/marvell/mwifiex/init.c| 2 + drivers/net/wireless/marvell/mwifiex/main.h| 2 + drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 50 +++--- drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 10 +++-- 6 files changed, 64 insertions(+), 19 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c index ff3f63ed95e1..ff1eefe5087b 100644 --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c @@ -1819,6 +1819,21 @@ mwifiex_cfg80211_set_antenna(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant) HostCmd_ACT_GEN_SET, 0, &ant_cfg, true); } +static int +mwifiex_cfg80211_get_antenna(struct wiphy *wiphy, u32 *tx_ant, u32 *rx_ant) +{ + struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy); + struct mwifiex_private *priv = mwifiex_get_priv(adapter, + MWIFIEX_BSS_ROLE_ANY); + mwifiex_send_cmd(priv, HostCmd_CMD_RF_ANTENNA, +HostCmd_ACT_GEN_GET, 0, NULL, true); + + *tx_ant = priv->tx_ant; + *rx_ant = priv->rx_ant; + + return 0; +} + /* cfg80211 operation handler for stop ap. * Function stops BSS running at uAP interface. */ @@ -3962,6 +3977,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = { .change_beacon = mwifiex_cfg80211_change_beacon, .set_cqm_rssi_config = mwifiex_cfg80211_set_cqm_rssi_config, .set_antenna = mwifiex_cfg80211_set_antenna, + .get_antenna = mwifiex_cfg80211_get_antenna, .del_station = mwifiex_cfg80211_del_station, .sched_scan_start = mwifiex_cfg80211_sched_scan_start, .sched_scan_stop = mwifiex_cfg80211_sched_scan_stop, diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h index 8e4145abdbfa..cef72343f5b6 100644 --- a/drivers/net/wireless/marvell/mwifiex/fw.h +++ b/drivers/net/wireless/marvell/mwifiex/fw.h @@ -462,6 +462,9 @@ enum P2P_MODES { #define HostCmd_ACT_SET_RX 0x0001 #define HostCmd_ACT_SET_TX 0x0002 #define HostCmd_ACT_SET_BOTH0x0003 +#define HostCmd_ACT_GET_RX 0x0004 +#define HostCmd_ACT_GET_TX 0x0008 +#define HostCmd_ACT_GET_BOTH0x000c #define RF_ANTENNA_AUTO 0x diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c index 78c532f0d286..fbaf49056746 100644 --- a/drivers/net/wireless/marvell/mwifiex/init.c +++ b/drivers/net/wireless/marvell/mwifiex/init.c @@ -110,6 +110,8 @@ int mwifiex_init_priv(struct mwifiex_private *priv) priv->tx_power_level = 0; priv->max_tx_power_level = 0; priv->min_tx_power_level = 0; + priv->tx_ant = 0; + priv->rx_ant = 0; priv->tx_rate = 0; priv->rxpd_htinfo = 0; priv->rxpd_rate = 0; diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h index 79c28cfb7780..2ae7ff74e1c6 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -533,6 +533,8 @@ struct mwifiex_private { u16 tx_power_level; u8 max_tx_power_level; u8 min_tx_power_level; + u32 tx_ant; + u32 rx_ant; u8 tx_rate; u8 tx_htinfo; u8 rxpd_htinfo; diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c index e436574b1698..8c658495bf66 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c @@ -313,23 +313,41 @@ static int mwifiex_cmd_rf_antenna(struct mwifiex_private *priv, cmd->command = cpu_to_le16(HostCmd_CMD_RF_ANTENNA); - if (cmd_action != HostCmd_ACT_GEN_S
[PATCH v2 0/3] mwifiex: add .get_tx_power and .get_antenna cfg80211 operations
Hello, Since commit de3bb771f471 ("cfg80211: add more warnings for inconsistent ops") the wireless core warns if a driver implements a cfg80211 callback but doesn't implements the inverse operation. The mwifiex driver has two set operations defined without the get counterpat so warnings are shown: WARNING: CPU: 3 PID: 127 at net/wireless/core.c:366 wiphy_new_nm+0x66c/0x6ac Modules linked in: mwifiex_sdio mwifiex CPU: 3 PID: 127 Comm: kworker/3:1 Tainted: GW 4.7.0-rc1-next-20160531-6-g569df5b983f3 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) Workqueue: events request_firmware_work_func [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (dump_stack+0x88/0x9c) [] (dump_stack) from [] (__warn+0xe8/0x100) [] (__warn) from [] (warn_slowpath_null+0x20/0x28) [] (warn_slowpath_null) from [] (wiphy_new_nm+0x66c/0x6ac) [] (wiphy_new_nm) from [] (mwifiex_register_cfg80211+0x28/0x3f0 [mwifiex]) [] (mwifiex_register_cfg80211 [mwifiex]) from [] (mwifiex_fw_dpc+0x2b0/0x474 [mwifiex]) [] (mwifiex_fw_dpc [mwifiex]) from [] (request_firmware_work_func+0x30/0x58) [] (request_firmware_work_func) from [] (process_one_work+0x124/0x338) [] (process_one_work) from [] (worker_thread+0x38/0x4d4) [] (worker_thread) from [] (kthread+0xdc/0xf4) [] (kthread) from [] (ret_from_fork+0x14/0x3c) This patch series fixes the warnings by implementing callbacks for the missing get operations. Since the first version was posted [0], Amitkumar Karwar from Marvell provided me their patches that adds the same. The .get_tx_power patch was very similar to mine. So I kept my patch and added the delta as a follow up patch but discarded my .get_antenna patch since the one provided by Amitkumar queries the firmware so is the correct approach. Patches were tested on an Exynos5800 Chromebook that has a mwifiex chip. Changes since v1: - Drop patch that reports antenna cached values and use Marvell's patch instead. - Add patch that moves the .get_tx_power() logic to sta_ioctl.c file. [0]: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1160119.html Best regards, Javier Javier Martinez Canillas (1): mwifiex: add a cfg80211 .get_tx_power operation callback Shengzhen Li (2): mwifiex: move .get_tx_power logic to station ioctl file mwifiex: add get_antenna support for cfg80211 drivers/net/wireless/marvell/mwifiex/cfg80211.c| 32 ++ drivers/net/wireless/marvell/mwifiex/fw.h | 3 ++ drivers/net/wireless/marvell/mwifiex/init.c| 2 + drivers/net/wireless/marvell/mwifiex/main.h| 4 ++ drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 50 +++--- drivers/net/wireless/marvell/mwifiex/sta_cmdresp.c | 10 +++-- drivers/net/wireless/marvell/mwifiex/sta_ioctl.c | 18 7 files changed, 100 insertions(+), 19 deletions(-) -- 2.5.5
[PATCH v2 2/3] mwifiex: move .get_tx_power logic to station ioctl file
From: Shengzhen Li Most cfg80211 operations are just a wrappers to functions defined in the sta_ioctl.c file, so for consistency move the .get_tx_power logic there. Signed-off-by: Shengzhen Li Signed-off-by: Amitkumar Karwar [javier: update the subject line and commit message] Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/cfg80211.c | 14 +++--- drivers/net/wireless/marvell/mwifiex/main.h | 2 ++ drivers/net/wireless/marvell/mwifiex/sta_ioctl.c | 18 ++ 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c index b17f3d09a2c7..ff3f63ed95e1 100644 --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c @@ -385,18 +385,10 @@ mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy, int *dbm) { struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy); - struct mwifiex_private *priv = mwifiex_get_priv(adapter, - MWIFIEX_BSS_ROLE_ANY); - int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR, - HostCmd_ACT_GEN_GET, 0, NULL, true); - - if (ret < 0) - return ret; - - /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */ - *dbm = priv->tx_power_level; + struct mwifiex_private *priv; - return 0; + priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); + return mwifiex_get_tx_power(priv, dbm); } /* diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h index 0207af00be42..79c28cfb7780 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -1464,6 +1464,8 @@ int mwifiex_drv_get_driver_version(struct mwifiex_adapter *adapter, int mwifiex_set_tx_power(struct mwifiex_private *priv, struct mwifiex_power_cfg *power_cfg); +int mwifiex_get_tx_power(struct mwifiex_private *priv, int *dbm); + int mwifiex_main_process(struct mwifiex_adapter *); int mwifiex_queue_tx_pkt(struct mwifiex_private *priv, struct sk_buff *skb); diff --git a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c index 8e0862657122..70ff9b805b5b 100644 --- a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c +++ b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c @@ -775,6 +775,24 @@ int mwifiex_set_tx_power(struct mwifiex_private *priv, } /* + * IOCTL request handler to get tx power configuration. + * + * This function prepares the correct firmware command and + * issues it. + */ +int mwifiex_get_tx_power(struct mwifiex_private *priv, int *dbm) +{ + int ret; + + ret = mwifiex_send_cmd(priv, HostCmd_CMD_TXPWR_CFG, + HostCmd_ACT_GEN_GET, 0, NULL, true); + + *dbm = priv->tx_power_level; + + return ret; +} + +/* * IOCTL request handler to get power save mode. * * This function prepares the correct firmware command and -- 2.5.5
[PATCH v2 1/3] mwifiex: add a cfg80211 .get_tx_power operation callback
The mwifiex driver implements a cfg80211 .set_tx_power operation handler but doesn't have the inverse .get_tx_power callback. This not only has the effect that the Tx power can't be reported to user space tools such as iwconfig and iwlist but also that the wireless core prints a warning when a new wiphy is created due an cfg80211 operation being implemented without its counterpart. After this patch, the Tx power is properly reported to user-space tools: $ iwlist mlan0 txpower mlan0 unknown transmit-power information. Current Tx-Power=13 dBm (19 mW) and also the following warning isn't shown anymore on the driver probe: WARNING: CPU: 3 PID: 127 at net/wireless/core.c:366 wiphy_new_nm+0x66c/0x6ac Modules linked in: mwifiex_sdio mwifiex CPU: 3 PID: 127 Comm: kworker/3:1 Tainted: GW 4.7.0-rc1-next-20160531-6-g569df5b983f3 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) Workqueue: events request_firmware_work_func [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (dump_stack+0x88/0x9c) [] (dump_stack) from [] (__warn+0xe8/0x100) [] (__warn) from [] (warn_slowpath_null+0x20/0x28) [] (warn_slowpath_null) from [] (wiphy_new_nm+0x66c/0x6ac) [] (wiphy_new_nm) from [] (mwifiex_register_cfg80211+0x28/0x3f0 [mwifiex]) [] (mwifiex_register_cfg80211 [mwifiex]) from [] (mwifiex_fw_dpc+0x2b0/0x474 [mwifiex]) [] (mwifiex_fw_dpc [mwifiex]) from [] (request_firmware_work_func+0x30/0x58) [] (request_firmware_work_func) from [] (process_one_work+0x124/0x338) [] (process_one_work) from [] (worker_thread+0x38/0x4d4) [] (worker_thread) from [] (kthread+0xdc/0xf4) [] (kthread) from [] (ret_from_fork+0x14/0x3c) Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/cfg80211.c | 24 1 file changed, 24 insertions(+) diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c index ff948a92..b17f3d09a2c7 100644 --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c @@ -377,6 +377,29 @@ mwifiex_cfg80211_set_tx_power(struct wiphy *wiphy, } /* + * CFG802.11 operation handler to get Tx power. + */ +static int +mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy, + struct wireless_dev *wdev, + int *dbm) +{ + struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy); + struct mwifiex_private *priv = mwifiex_get_priv(adapter, + MWIFIEX_BSS_ROLE_ANY); + int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR, + HostCmd_ACT_GEN_GET, 0, NULL, true); + + if (ret < 0) + return ret; + + /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */ + *dbm = priv->tx_power_level; + + return 0; +} + +/* * CFG802.11 operation handler to set Power Save option. * * The timeout value, if provided, is currently ignored. @@ -3940,6 +3963,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = { .set_default_key = mwifiex_cfg80211_set_default_key, .set_power_mgmt = mwifiex_cfg80211_set_power_mgmt, .set_tx_power = mwifiex_cfg80211_set_tx_power, + .get_tx_power = mwifiex_cfg80211_get_tx_power, .set_bitrate_mask = mwifiex_cfg80211_set_bitrate_mask, .start_ap = mwifiex_cfg80211_start_ap, .stop_ap = mwifiex_cfg80211_stop_ap, -- 2.5.5
[PATCH 1/2] mwifiex: add a cfg80211 .get_tx_power operation callback
The mwifiex driver implements a cfg80211 .set_tx_power operation handler but doesn't have the inverse .get_tx_power callback. This not only has the effect that the Tx power can't be reported to user space tools such as iwconfig and iwlist but also that the wireless core prints a warning when a new wiphy is created due an cfg80211 operation being implemented without its counterpart. After this patch, the Tx power is properly reported to user-space tools: $ iwlist mlan0 txpower mlan0 unknown transmit-power information. Current Tx-Power=13 dBm (19 mW) and also the following warning isn't shown anymore on the driver probe: WARNING: CPU: 3 PID: 127 at net/wireless/core.c:366 wiphy_new_nm+0x66c/0x6ac Modules linked in: mwifiex_sdio mwifiex CPU: 3 PID: 127 Comm: kworker/3:1 Tainted: GW 4.7.0-rc1-next-20160531-6-g569df5b983f3 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) Workqueue: events request_firmware_work_func [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (dump_stack+0x88/0x9c) [] (dump_stack) from [] (__warn+0xe8/0x100) [] (__warn) from [] (warn_slowpath_null+0x20/0x28) [] (warn_slowpath_null) from [] (wiphy_new_nm+0x66c/0x6ac) [] (wiphy_new_nm) from [] (mwifiex_register_cfg80211+0x28/0x3f0 [mwifiex]) [] (mwifiex_register_cfg80211 [mwifiex]) from [] (mwifiex_fw_dpc+0x2b0/0x474 [mwifiex]) [] (mwifiex_fw_dpc [mwifiex]) from [] (request_firmware_work_func+0x30/0x58) [] (request_firmware_work_func) from [] (process_one_work+0x124/0x338) [] (process_one_work) from [] (worker_thread+0x38/0x4d4) [] (worker_thread) from [] (kthread+0xdc/0xf4) [] (kthread) from [] (ret_from_fork+0x14/0x3c) Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/cfg80211.c | 24 1 file changed, 24 insertions(+) diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c index ff948a92..b17f3d09a2c7 100644 --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c @@ -377,6 +377,29 @@ mwifiex_cfg80211_set_tx_power(struct wiphy *wiphy, } /* + * CFG802.11 operation handler to get Tx power. + */ +static int +mwifiex_cfg80211_get_tx_power(struct wiphy *wiphy, + struct wireless_dev *wdev, + int *dbm) +{ + struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy); + struct mwifiex_private *priv = mwifiex_get_priv(adapter, + MWIFIEX_BSS_ROLE_ANY); + int ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_TX_PWR, + HostCmd_ACT_GEN_GET, 0, NULL, true); + + if (ret < 0) + return ret; + + /* tx_power_level is set in HostCmd_CMD_RF_TX_PWR command handler */ + *dbm = priv->tx_power_level; + + return 0; +} + +/* * CFG802.11 operation handler to set Power Save option. * * The timeout value, if provided, is currently ignored. @@ -3940,6 +3963,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = { .set_default_key = mwifiex_cfg80211_set_default_key, .set_power_mgmt = mwifiex_cfg80211_set_power_mgmt, .set_tx_power = mwifiex_cfg80211_set_tx_power, + .get_tx_power = mwifiex_cfg80211_get_tx_power, .set_bitrate_mask = mwifiex_cfg80211_set_bitrate_mask, .start_ap = mwifiex_cfg80211_start_ap, .stop_ap = mwifiex_cfg80211_stop_ap, -- 2.5.5
[PATCH 0/2] mwifiex: add .get_tx_power and .get_antenna cfg80211 operations
Hello, Since commit de3bb771f471 ("cfg80211: add more warnings for inconsistent ops") the wireless core warns if a driver implements a cfg80211 callback but doesn't implements the inverse operation. The mwifiex driver has two set operations defined without the get counterpat so warnings are shown: WARNING: CPU: 3 PID: 127 at net/wireless/core.c:366 wiphy_new_nm+0x66c/0x6ac Modules linked in: mwifiex_sdio mwifiex CPU: 3 PID: 127 Comm: kworker/3:1 Tainted: GW 4.7.0-rc1-next-20160531-6-g569df5b983f3 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) Workqueue: events request_firmware_work_func [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (dump_stack+0x88/0x9c) [] (dump_stack) from [] (__warn+0xe8/0x100) [] (__warn) from [] (warn_slowpath_null+0x20/0x28) [] (warn_slowpath_null) from [] (wiphy_new_nm+0x66c/0x6ac) [] (wiphy_new_nm) from [] (mwifiex_register_cfg80211+0x28/0x3f0 [mwifiex]) [] (mwifiex_register_cfg80211 [mwifiex]) from [] (mwifiex_fw_dpc+0x2b0/0x474 [mwifiex]) [] (mwifiex_fw_dpc [mwifiex]) from [] (request_firmware_work_func+0x30/0x58) [] (request_firmware_work_func) from [] (process_one_work+0x124/0x338) [] (process_one_work) from [] (worker_thread+0x38/0x4d4) [] (worker_thread) from [] (kthread+0xdc/0xf4) [] (kthread) from [] (ret_from_fork+0x14/0x3c) This patch series fixes the warnings by implementing callbacks for the get operations. There isn't public documentation about the firmware interface so the data is only queried to the firmware if the driver already supports it as is the case of the Tx power. For the antenna the driver only supports the get command so the get returns the cached values that were previously set. This can be changed to query the firmware as a followup by someone with access to the docs, if this is supported. Best regards, Javier Javier Martinez Canillas (2): mwifiex: add a cfg80211 .get_tx_power operation callback mwifiex: add a cfg80211 .get_antenna operation callback drivers/net/wireless/marvell/mwifiex/cfg80211.c | 50 - drivers/net/wireless/marvell/mwifiex/main.h | 1 + 2 files changed, 49 insertions(+), 2 deletions(-) -- 2.5.5
[PATCH 2/2] mwifiex: add a cfg80211 .get_antenna operation callback
Since commit de3bb771f471 ("cfg80211: add more warnings for inconsistent ops") the wireless core warns if a driver implements a cfg80211 callback but doesn't implements the inverse operation. The mwifiex driver defines a .set_antenna handler but not a .get_antenna so this not only makes the core to print a warning when creating a new wiphy but also the antenna isn't reported to user-space apps such as iw. Unfortunately the driver doesn't have support to query the antena to the firmware and there isn't public documentation about the interface so this patch caches the values set in .set_antenna and reports those back in get. With this patch, the wireless core does not warn anymore and the antenna is properly reported once has been set: $ iw phy phy0 set antenna 0x1 $ iw phy phy0 info | grep Antennas Available Antennas: TX 0x3 RX 0x3 Configured Antennas: TX 0x1 RX 0x1 Signed-off-by: Javier Martinez Canillas --- Hello, Even though this approach prevents the warning and allows to reports the antenna values, it would be better if instead of caching the set values, these are asked to the firmware for each .get_antenna callback. But the driver currently only implements a set antenna command and the firmware interface documentation is not public so isn't clear if the firmware doesn't support or is just that the driver didn't implement it. Best regards, Javier drivers/net/wireless/marvell/mwifiex/cfg80211.c | 26 +++-- drivers/net/wireless/marvell/mwifiex/main.h | 1 + 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c index b17f3d09a2c7..e9efbc852d23 100644 --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c @@ -1771,6 +1771,7 @@ mwifiex_cfg80211_set_antenna(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant) struct mwifiex_private *priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY); struct mwifiex_ds_ant_cfg ant_cfg; + int ret; if (!tx_ant || !rx_ant) return -EOPNOTSUPP; @@ -1823,8 +1824,28 @@ mwifiex_cfg80211_set_antenna(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant) ant_cfg.tx_ant = tx_ant; ant_cfg.rx_ant = rx_ant; - return mwifiex_send_cmd(priv, HostCmd_CMD_RF_ANTENNA, - HostCmd_ACT_GEN_SET, 0, &ant_cfg, true); + ret = mwifiex_send_cmd(priv, HostCmd_CMD_RF_ANTENNA, + HostCmd_ACT_GEN_SET, 0, &ant_cfg, true); + + if (ret < 0) + return ret; + + priv->ant_cfg.tx_ant = tx_ant; + priv->ant_cfg.rx_ant = rx_ant; + + return 0; +} + +static int +mwifiex_cfg80211_get_antenna(struct wiphy *wiphy, u32 *tx_ant, u32 *rx_ant) +{ + struct mwifiex_adapter *adapter = mwifiex_cfg80211_get_adapter(wiphy); + struct mwifiex_private *priv = mwifiex_get_priv(adapter, + MWIFIEX_BSS_ROLE_ANY); + *tx_ant = priv->ant_cfg.tx_ant; + *rx_ant = priv->ant_cfg.rx_ant; + + return 0; } /* cfg80211 operation handler for stop ap. @@ -3970,6 +3991,7 @@ static struct cfg80211_ops mwifiex_cfg80211_ops = { .change_beacon = mwifiex_cfg80211_change_beacon, .set_cqm_rssi_config = mwifiex_cfg80211_set_cqm_rssi_config, .set_antenna = mwifiex_cfg80211_set_antenna, + .get_antenna = mwifiex_cfg80211_get_antenna, .del_station = mwifiex_cfg80211_del_station, .sched_scan_start = mwifiex_cfg80211_sched_scan_start, .sched_scan_stop = mwifiex_cfg80211_sched_scan_stop, diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h index 0207af00be42..b4fe10cbb34d 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.h +++ b/drivers/net/wireless/marvell/mwifiex/main.h @@ -668,6 +668,7 @@ struct mwifiex_private { struct delayed_work dfs_chan_sw_work; struct cfg80211_beacon_data beacon_after; struct mwifiex_11h_intf_state state_11h; + struct mwifiex_ds_ant_cfg ant_cfg; struct mwifiex_ds_mem_rw mem_rw; struct sk_buff_head bypass_txq; struct mwifiex_user_scan_chan hidden_chan[MWIFIEX_USER_SCAN_CHAN_MAX]; -- 2.5.5
Re: [PATCH 7/8] mwifiex: don't print an error if an optional DT property is missing
Hello Julian, Thanks a lot for your feedback and reviews. On 06/01/2016 12:20 AM, Julian Calaby wrote: > Hi All, > > On Sat, May 28, 2016 at 12:18 AM, Javier Martinez Canillas > wrote: >> The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT >> binding document say that the "interrupts" property in the child node is >> optional. So the property being missed shouldn't be treated as an error. > > Have you checked whether it is truly optional? I.e. nothing else > breaks if this property isn't set? > That's what the DT binding says and the IRQ is only used as a wakeup source during system suspend, it is not used during runtime. And that is why the mwifiex_sdio_probe_of() function does not fail if the IRQ is missing. Now, I just got to that conclusion by reading the binding docs, the message in the commits that introduced this and the driver code. Xinming Hu should comment on how critical this feature is for systems that needs to be wakeup. In any case I think that the code should be consistent with what the binding doc says and also the function does (i.e: dev_err only if returns an error). >> Signed-off-by: Javier Martinez Canillas > > Other than that, this looks sensible to me. > > Reviewed-by: Julian Calaby > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America
[PATCH 2/8] mwifiex: propagate sdio_enable_func() errno code in mwifiex_sdio_probe()
If the sdio_enable_func() function fails on .probe, the -EIO errno code is always returned but that could make more difficult to debug and find the cause of why the function actually failed. Since the driver/device core prints the value returned by .probe in its error message propagate what was returned by sdio_enable_func() at fail. Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index 285b1b68f7e9..ab64507c84e1 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -184,7 +184,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) if (ret) { pr_err("%s: failed to enable function\n", __func__); kfree(card); - return -EIO; + return ret; } /* device tree node parsing and platform specific configuration*/ -- 2.5.5
[PATCH 3/8] mwifiex: propagate mwifiex_add_card() errno code in mwifiex_sdio_probe()
There's only a check if mwifiex_add_card() returned a nonzero value, but the actual error code is neither stored nor propagated to the caller. So instead of always returning -1 (which is -EPERM and not a suitable errno code in this case), propagate the value returned by mwifiex_add_card(). Patch also removes the assignment of sdio_disable_func() returned value since it was overwritten anyways and what matters is to know the error value returned by the first function that failed. Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/sdio.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index ab64507c84e1..81003fbe5025 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -191,14 +191,14 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) if (func->dev.of_node) mwifiex_sdio_probe_of(&func->dev, card); - if (mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops, -MWIFIEX_SDIO)) { + ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops, + MWIFIEX_SDIO); + if (ret) { pr_err("%s: add card failed\n", __func__); kfree(card); sdio_claim_host(func); - ret = sdio_disable_func(func); + sdio_disable_func(func); sdio_release_host(func); - ret = -1; } return ret; -- 2.5.5
[PATCH 1/8] mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node
SDIO is an auto enumerable bus so the SDIO devices are matched using the sdio_device_id table and not using compatible strings from a OF id table. However, commit ce4f6f0c353b ("mwifiex: add platform specific wakeup interrupt support") allowed to match nodes defined as child of the SDIO host controller in the probe function using a compatible string to setup platform specific parameters in the DT. The problem is that the OF parse function is always called regardless if the SDIO dev has an OF node associated or not, and prints an error if it is not found. So, on a platform that doesn't have a node for a SDIO dev, the following misleading error message will be printed: [ 12.480042] mwifiex_sdio mmc2:0001:1: sdio platform data not available Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/sdio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index bdc51ffd43ec..285b1b68f7e9 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -102,8 +102,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card) struct mwifiex_plt_wake_cfg *cfg; int ret; - if (!dev->of_node || - !of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) { + if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) { dev_err(dev, "sdio platform data not available\n"); return -1; } @@ -189,7 +188,8 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) } /* device tree node parsing and platform specific configuration*/ - mwifiex_sdio_probe_of(&func->dev, card); + if (func->dev.of_node) + mwifiex_sdio_probe_of(&func->dev, card); if (mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops, MWIFIEX_SDIO)) { -- 2.5.5
[PATCH 0/8] mwifiex: Fix some error handling issues in mwifiex_sdio_probe() function
Hello, While booting a system with a mwifiex WiFi card, I noticed the following missleading error message: [ 12.480042] mwifiex_sdio mmc2:0001:1: sdio platform data not available This error only applies to platforms that define a child node for the SDIO device, but it's currently shown even in platforms that don't have a child node defined. So this series fixes this issue and others I found in the .probe function (mostly related to error handling and the error path) while looking at it. Best regards, Javier Javier Martinez Canillas (8): mwifiex: only call mwifiex_sdio_probe_of() if dev has an OF node mwifiex: propagate sdio_enable_func() errno code in mwifiex_sdio_probe() mwifiex: propagate mwifiex_add_card() errno code in mwifiex_sdio_probe() mwifiex: consolidate mwifiex_sdio_probe() error paths mwifiex: use dev_err() instead of pr_err() in mwifiex_sdio_probe() mwifiex: check if mwifiex_sdio_probe_of() fails and return error mwifiex: don't print an error if an optional DT property is missing mwifiex: use better message and error code when OF node doesn't match drivers/net/wireless/marvell/mwifiex/sdio.c | 46 ++--- 1 file changed, 28 insertions(+), 18 deletions(-) -- 2.5.5
[PATCH 4/8] mwifiex: consolidate mwifiex_sdio_probe() error paths
Instead of duplicating part of the cleanups needed in case of an error in .probe callback, have a single error path and use goto labels as is common practice in the kernel. This also has the nice side effect that the cleanup operations are made in the inverse order of their counterparts, which was not the case for the mwifiex_add_card() error path. Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/sdio.c | 17 +++-- 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index 81003fbe5025..7aeee88b858f 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -183,8 +183,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) if (ret) { pr_err("%s: failed to enable function\n", __func__); - kfree(card); - return ret; + goto err_free; } /* device tree node parsing and platform specific configuration*/ @@ -195,12 +194,18 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) MWIFIEX_SDIO); if (ret) { pr_err("%s: add card failed\n", __func__); - kfree(card); - sdio_claim_host(func); - sdio_disable_func(func); - sdio_release_host(func); + goto err_disable; } + return 0; + +err_disable: + sdio_claim_host(func); + sdio_disable_func(func); + sdio_release_host(func); +err_free: + kfree(card); + return ret; } -- 2.5.5
[PATCH 8/8] mwifiex: use better message and error code when OF node doesn't match
The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT binding document lists the possible compatible strings that a SDIO child node can have, so the driver checks if the defined in the node matches. But the error message when that's not the case is misleading, so change for one that makes clear what the error really is. Also, returning a -1 as errno code is not correct since that's -EPERM. A -EINVAL seems to be a more appropriate one. Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index 8b3292eaecb2..e6d56be04e08 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -103,8 +103,8 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card) int ret; if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) { - dev_err(dev, "sdio platform data not available\n"); - return -1; + dev_err(dev, "required compatible string missing\n"); + return -EINVAL; } card->plt_of_node = dev->of_node; -- 2.5.5
[PATCH 5/8] mwifiex: use dev_err() instead of pr_err() in mwifiex_sdio_probe()
It's better to have the device name prefixed in the error message. Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index 7aeee88b858f..1ffbb972318f 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -182,7 +182,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) sdio_release_host(func); if (ret) { - pr_err("%s: failed to enable function\n", __func__); + dev_err(&func->dev, "failed to enable function\n"); goto err_free; } @@ -193,7 +193,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops, MWIFIEX_SDIO); if (ret) { - pr_err("%s: add card failed\n", __func__); + dev_err(&func->dev, "add card failed\n"); goto err_disable; } -- 2.5.5
[PATCH 7/8] mwifiex: don't print an error if an optional DT property is missing
The Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt DT binding document say that the "interrupts" property in the child node is optional. So the property being missed shouldn't be treated as an error. Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/sdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index 1c17e624547a..8b3292eaecb2 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -114,7 +114,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card) if (cfg && card->plt_of_node) { cfg->irq_wifi = irq_of_parse_and_map(card->plt_of_node, 0); if (!cfg->irq_wifi) { - dev_err(dev, + dev_dbg(dev, "fail to parse irq_wifi from device tree\n"); } else { ret = devm_request_irq(dev, cfg->irq_wifi, -- 2.5.5
[PATCH 6/8] mwifiex: check if mwifiex_sdio_probe_of() fails and return error
The function can fail so the returned value should be checked and the error propagated to the caller in case of a failure. Signed-off-by: Javier Martinez Canillas --- drivers/net/wireless/marvell/mwifiex/sdio.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c index 1ffbb972318f..1c17e624547a 100644 --- a/drivers/net/wireless/marvell/mwifiex/sdio.c +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c @@ -187,8 +187,13 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id) } /* device tree node parsing and platform specific configuration*/ - if (func->dev.of_node) - mwifiex_sdio_probe_of(&func->dev, card); + if (func->dev.of_node) { + ret = mwifiex_sdio_probe_of(&func->dev, card); + if (ret) { + dev_err(&func->dev, "SDIO dt node parse failed\n"); + goto err_disable; + } + } ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops, MWIFIEX_SDIO); -- 2.5.5
Re: [RFC] Reset pins of phys and their representation in a device tree
Hello ChenYu On Thu, May 12, 2016 at 9:49 AM, Chen-Yu Tsai wrote: > Hi, > > On Thu, May 12, 2016 at 9:40 PM, Javier Martinez Canillas > wrote: >> [adding Krzysztof as cc] >> >> On Thu, May 12, 2016 at 8:25 AM, Sergei Shtylyov >> wrote: >>> Hello. >>> >>> >>> On 5/12/2016 10:15 AM, Uwe Kleine-König wrote: >>> >>>> I have a machine here where the reset pin of the phy is connected to a >>>> GPIO. >>>> >>>> There are different possibilities available today to handle this >>>> situation, here are the ones I'm aware of: >>>> >>>> - Use a gpio-hog to set the reset gpio to non-active >>>>This might result in dependency problems (and that's what I am >>>>currently faced with) because there is no connection in the device >>>>tree between the hog and the phy. >>>> >>>> - [Documentation/devicetree/bindings/net/fsl-fec.txt] >>>>The fec node supports properties >>>> >>>> phy-reset-gpios = <&gpio2 14 0>; >>>> phy-reset-duration = <200> /* milliseconds */; >>>> >>>>Something similar exists in TI's vendor kernel >>>> >>>> (http://git.ti.com/ti-linux-kernel/ti-linux-kernel/commit/17d192b999ee904ced223c16cef76111a51c461b) >>>>with different (and IMHO bader) naming. >>>>This is the wrong place to specify the gpios; they shouldn't be in the >>>>mac's node, but in a phy node instead. >>>> >>>> So what I actually want is to put the gpio specification in the right >>>> place and let it look as follows: >>>> >>>> mymdiobus { >>>> [...] >>>> myfirstphy: ethernet-phy@0 { >>>> compatible = "ethernet-phy-ieee802.3-c22"; >>>> reg = <0>; >>>> >>>> reset-gpios = <&gpio2 14 0>; >>>> reset-duration-ms = <200>; >>>> }; >>>> >>>> mysecondphy: ethernet-phy@2 { >>>> compatible = "ethernet-phy-ieee802.3-c22"; >>>> reg = <2>; >>>> >>>> reset-gpios = <&gpio3 10 0>; >>>> reset-duration-ms = <200>; >>>> }; >>>> }; >>>> >>>> And with this we could defer probe of &myfirstphy if &gpio2 isn't >>>> available yet. >>>> >>>> Does this sound sensible? Does something like that already exist which I >>>> missed? Any further ideas/comments? >>> >>> >>> http://patchwork.ozlabs.org/patch/616495/ >>> >>>I'll post a new version RSN. >>> >> >> This seems to be similar to what's needed by MMC/SD/SDIO devices (i.e: >> a WiFi chip) that needs some power sequence for reset and be >> enumerable. >> >> Krzysztof has been working to make the MMC pwrseq framework more >> generic [0] since he wants to use it also for built-in USB devices. >> >> From a quick look at the patches mentioned in this thread, it seems >> that the same framework can be used to reset the PHYs unless I'm >> missing something. Have you considered using this? It would be good if >> there is a consistent way to define the power sequence for devices >> across the different subsystems. >> >> [0]: https://lkml.org/lkml/2016/5/5/230 > > You probably missed the part that Rob nacked the bindings in that > series? > No, I didn't miss that. AFAICT the way forward is still being discussed as you can see from the last email in that thread from Ulf [0]. And yes, the DT bindings is likely to change (keeping the backward compat for MMC) which is good, but still the power sequence management code can be reused across subsystems. > Putting the reset gpios under the PHY nodes and handling it from > phylib is probably the way to go. I'd like to ask for regulator > and clock support as well, if possible. > A GPIO array, clock and regulator (after Krzysztof patch) is already supported by the pwrseq_simple provider for example. > Regards > ChenYu [0]: https://lkml.org/lkml/2016/5/10/320
Re: [RFC] Reset pins of phys and their representation in a device tree
[adding Krzysztof as cc] On Thu, May 12, 2016 at 8:25 AM, Sergei Shtylyov wrote: > Hello. > > > On 5/12/2016 10:15 AM, Uwe Kleine-König wrote: > >> I have a machine here where the reset pin of the phy is connected to a >> GPIO. >> >> There are different possibilities available today to handle this >> situation, here are the ones I'm aware of: >> >> - Use a gpio-hog to set the reset gpio to non-active >>This might result in dependency problems (and that's what I am >>currently faced with) because there is no connection in the device >>tree between the hog and the phy. >> >> - [Documentation/devicetree/bindings/net/fsl-fec.txt] >>The fec node supports properties >> >> phy-reset-gpios = <&gpio2 14 0>; >> phy-reset-duration = <200> /* milliseconds */; >> >>Something similar exists in TI's vendor kernel >> >> (http://git.ti.com/ti-linux-kernel/ti-linux-kernel/commit/17d192b999ee904ced223c16cef76111a51c461b) >>with different (and IMHO bader) naming. >>This is the wrong place to specify the gpios; they shouldn't be in the >>mac's node, but in a phy node instead. >> >> So what I actually want is to put the gpio specification in the right >> place and let it look as follows: >> >> mymdiobus { >> [...] >> myfirstphy: ethernet-phy@0 { >> compatible = "ethernet-phy-ieee802.3-c22"; >> reg = <0>; >> >> reset-gpios = <&gpio2 14 0>; >> reset-duration-ms = <200>; >> }; >> >> mysecondphy: ethernet-phy@2 { >> compatible = "ethernet-phy-ieee802.3-c22"; >> reg = <2>; >> >> reset-gpios = <&gpio3 10 0>; >> reset-duration-ms = <200>; >> }; >> }; >> >> And with this we could defer probe of &myfirstphy if &gpio2 isn't >> available yet. >> >> Does this sound sensible? Does something like that already exist which I >> missed? Any further ideas/comments? > > > http://patchwork.ozlabs.org/patch/616495/ > >I'll post a new version RSN. > This seems to be similar to what's needed by MMC/SD/SDIO devices (i.e: a WiFi chip) that needs some power sequence for reset and be enumerable. Krzysztof has been working to make the MMC pwrseq framework more generic [0] since he wants to use it also for built-in USB devices. >From a quick look at the patches mentioned in this thread, it seems that the same framework can be used to reset the PHYs unless I'm missing something. Have you considered using this? It would be good if there is a consistent way to define the power sequence for devices across the different subsystems. [0]: https://lkml.org/lkml/2016/5/5/230 Best regards, Javier
[PATCH 0/2] net: encx24j600: Fix SPI driver module autoload
Hello, Recently I've been trying to fix module autoloading for all SPI drivers and found that the encx24j600 driver does not fill module alias information due missing a MODULE_DEVICE_TABLE() so module autload won't work and the driver Kconfig symbol is tristate which means the driver can be built as a module. But also the SPI id table is not correctly defined so this series fixes both issues. The patches have been applied on top of the next-next tree master branch: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git master Best regards, Javier Javier Martinez Canillas (2): net: encx24j600: Fix SPI id table definition net: encx24j600: Export missing SPI module alias information drivers/net/ethernet/microchip/encx24j600.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] net: encx24j600: Fix SPI id table definition
A driver's SPI id table is expected to be an array of struct spi_device_id that ends with a zero-initialized sentinel entry. But this driver defines the table as a single struct spi_device_id and sets .id_table to a pointer to this struct. But spi_match_id() has a loop that iterates while the struct spi_device_id .name[0] is not NULL, so not having a sentinel can cause a NULL pointer deference error. This patch defines the SPI id table correctly as all other SPI drivers do. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/microchip/encx24j600.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/microchip/encx24j600.c b/drivers/net/ethernet/microchip/encx24j600.c index bf08ce2baf8d..c2dafc1c53de 100644 --- a/drivers/net/ethernet/microchip/encx24j600.c +++ b/drivers/net/ethernet/microchip/encx24j600.c @@ -1094,8 +1094,9 @@ static int encx24j600_spi_remove(struct spi_device *spi) return 0; } -static const struct spi_device_id encx24j600_spi_id_table = { - .name = "encx24j600" +static const struct spi_device_id encx24j600_spi_id_table[] = { + { .name = "encx24j600" }, + { /* sentinel */ } }; static struct spi_driver encx24j600_spi_net_driver = { @@ -1106,7 +1107,7 @@ static struct spi_driver encx24j600_spi_net_driver = { }, .probe = encx24j600_spi_probe, .remove = encx24j600_spi_remove, - .id_table = &encx24j600_spi_id_table, + .id_table = encx24j600_spi_id_table, }; static int __init encx24j600_init(void) -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] net: encx24j600: Export missing SPI module alias information
The driver Kconfig symbol is tristate which means that it can be built as a module but the module alias information is not added to the module info so module autoload won't work since user-space won't have the information. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/microchip/encx24j600.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/microchip/encx24j600.c b/drivers/net/ethernet/microchip/encx24j600.c index c2dafc1c53de..2056b719c262 100644 --- a/drivers/net/ethernet/microchip/encx24j600.c +++ b/drivers/net/ethernet/microchip/encx24j600.c @@ -1098,6 +1098,7 @@ static const struct spi_device_id encx24j600_spi_id_table[] = { { .name = "encx24j600" }, { /* sentinel */ } }; +MODULE_DEVICE_TABLE(spi, encx24j600_spi_id_table); static struct spi_driver encx24j600_spi_net_driver = { .driver = { -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RESEND PATCH] net: ks8851: Export OF module alias information
Drivers needs to export the OF id table and this be built into the module or udev won't have the necessary information to autoload the driver module when the device is registered via OF. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/micrel/ks8851.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c index 66d4ab703f45..60f43ec22175 100644 --- a/drivers/net/ethernet/micrel/ks8851.c +++ b/drivers/net/ethernet/micrel/ks8851.c @@ -1601,6 +1601,7 @@ static const struct of_device_id ks8851_match_table[] = { { .compatible = "micrel,ks8851" }, { } }; +MODULE_DEVICE_TABLE(of, ks8851_match_table); static struct spi_driver ks8851_driver = { .driver = { -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/18] Export SPI and OF module aliases in missing drivers
Hello Brian, On 08/20/2015 11:11 PM, Brian Norris wrote: > On Thu, Aug 20, 2015 at 09:07:13AM +0200, Javier Martinez Canillas wrote: >> Patches #1 and #2 solves a), patches #3 to #8 solves b) and patches > > ^^^ I'm dying to know how this sentence ends :) > Sigh, I did some last minute restructuring of the cover letter and seems I missed a sentence. I meant to said: "and patches #9 to #17 solves c)." >> Patch #18 changes the logic of spi_uevent() to report an OF modalias if >> the device was registered using OF. But this patch is included in the >> series only as an RFC for illustration purposes since changing that >> without first applying all the other patches in this series, will break >> module autoloading for the drivers of devices registered using OF but >> that lacks an of_match_table. I'll repost patch #18 once all the patches >> in this series have landed. > > On a more productive note, the patches I've looked at look good to me. > The missing aliases are a problem enough that should be fixed (i.e., > part (b)). I'll leave the SPI framework changes to others to comment on. > Great, thanks a lot for your feedback. > Brian > Best regards, -- Javier Martinez Canillas Open Source Group Samsung Research America -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/18] Export SPI and OF module aliases in missing drivers
Hello, Short version: This patch series is the SPI equivalent of the I2C one posted before [0]. This series add the missing MODULE_DEVICE_TABLE() for OF and SPI tables to export that information so modules have the correct aliases built-in and autoloading works correctly. Longer version: The SPI core always reports the MODALIAS uevent as "spi:" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table). But this means that OF-only drivers needs to have both OF and SPI id tables that have to be kept in sync and also the device node's compatible manufacturer prefix is stripped when reporting the MODALIAS. Which can lead to issues if two vendors use the same SPI device name for example. Also, there are many SPI drivers whose module auto-loading is not working because of this fact that the SPI core always reports the MODALIAS as spi: and many developers didn't expect this since is not how other subsystems behave. I've identified SPI drivers with 3 types of different issues: a) Those that have an spi_table but are not exported. The match works if the driver is built-in but since the ID table is not exported, module auto-load won't work. b) Those that have a of_table but are not exported. This is currently not an issue since even when the of_table is used to match the dev with the driver, an OF modalias is not reported by the SPI core. But if the SPI core is changed to report the MODALIAS of the form of:N*T*C as it's made by other subsystems, then module auto-load will break for these drivers. c) Those that don't have an of_table but should since are OF drivers with DT bindings doc for them. Since the SPI core does not report a OF modalias and since spi_match_device() fallbacks to match the device part of the compatible string with the SPI device ID table, many OF drivers don't have an of_table to match. After all having a SPI device ID table is mandatory so it works without a of_table. So, in order to not make mandatory to have a SPI device ID table, all these three kind of issues have to be addressed. This series does that. I split the changes so the patches in this series are independent and can be picked individually by subsystem maintainers. Patches #1 and #2 solves a), patches #3 to #8 solves b) and patches Patch #18 changes the logic of spi_uevent() to report an OF modalias if the device was registered using OF. But this patch is included in the series only as an RFC for illustration purposes since changing that without first applying all the other patches in this series, will break module autoloading for the drivers of devices registered using OF but that lacks an of_match_table. I'll repost patch #18 once all the patches in this series have landed. [0]: https://lkml.org/lkml/2015/7/30/519 Best regards, Javier Javier Martinez Canillas (18): iio: Export SPI module alias information in missing drivers staging: iio: hmc5843: Export missing SPI module alias information mtd: dataflash: Export OF module alias information OMAPDSS: panel-sony-acx565akm: Export OF module alias information mmc: mmc_spi: Export OF module alias information staging: mt29f_spinand: Export OF module alias information net: ks8851: Export OF module alias information [media] s5c73m3: Export OF module alias information mfd: cros_ec: spi: Add OF match table iio: dac: ad7303: Add OF match table iio: adc: max1027: Set struct spi_driver .of_match_table mfd: stmpe: Add OF match table iio: adc: mcp320x: Set struct spi_driver .of_match_table iio: as3935: Add OF match table iio: adc128s052: Add OF match table iio: frequency: adf4350: Add OF match table NFC: trf7970a: Add OF match table spi: (RFC, don't apply) report OF style modalias when probing using DT drivers/iio/adc/max1027.c | 1 + drivers/iio/adc/mcp320x.c | 1 + drivers/iio/adc/ti-adc128s052.c | 8 drivers/iio/amplifiers/ad8366.c | 1 + drivers/iio/dac/ad7303.c| 7 +++ drivers/iio/frequency/adf4350.c | 9 + drivers/iio/proximity/as3935.c | 7 +++ drivers/media/i2c/s5c73m3/s5c73m3-spi.c | 1 + drivers/mfd/cros_ec_spi.c | 7 +++ drivers/mfd/stmpe-spi.c | 13 + drivers/mmc/host/mmc_spi.c | 1 + drivers/mtd/devices/mtd_dataflash.c | 1 + drivers/net/ethernet/micrel/ks8851.c| 1 + drivers/nfc/trf7970a.c | 7 +++ drivers/spi/spi.c
[PATCH 07/18] net: ks8851: Export OF module alias information
The SPI core always reports the MODALIAS uevent as "spi:" regardless of the mechanism that was used to register the device (i.e: OF or board code) and the table that is used later to match the driver with the device (i.e: SPI id table or OF match table). So drivers needs to export the SPI id table and this be built into the module or udev won't have the necessary information to autoload the needed driver module when the device is added. But this means that OF-only drivers needs to have both OF and SPI id tables that have to be kept in sync and also the dev node compatible manufacturer prefix is stripped when reporting the MODALIAS. Which can lead to issues if two vendors use the same SPI device name for example. To avoid the above, the SPI core behavior may be changed in the future to not require an SPI device table for OF-only drivers and report the OF module alias. So, it's better to also export the OF table even when is unused now to prevent breaking module loading when the core changes. Signed-off-by: Javier Martinez Canillas --- drivers/net/ethernet/micrel/ks8851.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c index 66d4ab703f45..60f43ec22175 100644 --- a/drivers/net/ethernet/micrel/ks8851.c +++ b/drivers/net/ethernet/micrel/ks8851.c @@ -1601,6 +1601,7 @@ static const struct of_device_id ks8851_match_table[] = { { .compatible = "micrel,ks8851" }, { } }; +MODULE_DEVICE_TABLE(of, ks8851_match_table); static struct spi_driver ks8851_driver = { .driver = { -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html