[PATCH 1/4] wifi: libertas: add missing calls to cancel_work_sync()

[PATCH 1/4] wifi: libertas: add missing calls to cancel_work_sync()

[Dmitry Antipov]

Add missing 'cancel_work_sync()' in 'if_sdio_remove()'
and on error handling path in 'if_sdio_probe()'.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/marvell/libertas/if_sdio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c b/drivers/net/wireless/marvell/libertas/if_sdio.c
index a63c5e622ee3..a35b33e84670 100644
--- a/drivers/net/wireless/marvell/libertas/if_sdio.c
+++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
@@ -1233,6 +1233,7 @@ static int if_sdio_probe(struct sdio_func *func,
 	flush_workqueue(card->workqueue);
 	lbs_remove_card(priv);
 free:
+	cancel_work_sync(&card->packet_worker);
 	destroy_workqueue(card->workqueue);
 err_queue:
 	while (card->packets) {
@@ -1277,6 +1278,7 @@ static void if_sdio_remove(struct sdio_func *func)
 	lbs_stop_card(card->priv);
 	lbs_remove_card(card->priv);
 
+	cancel_work_sync(&card->packet_worker);
 	destroy_workqueue(card->workqueue);
 
 	while (card->packets) {
-- 
2.41.0

[PATCH 2/4] wifi: libertas: use convenient lists to manage SDIO packets

[Dmitry Antipov]

Use convenient lists to manage SDIO packets, adjust
'struct if_sdio_packet', 'struct if_sdio_card' and
related code accordingly.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 .../net/wireless/marvell/libertas/if_sdio.c   | 37 +++++++------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c b/drivers/net/wireless/marvell/libertas/if_sdio.c
index a35b33e84670..c72081cf8a85 100644
--- a/drivers/net/wireless/marvell/libertas/if_sdio.c
+++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
@@ -101,7 +101,7 @@ MODULE_FIRMWARE("sd8688_helper.bin");
 MODULE_FIRMWARE("sd8688.bin");
 
 struct if_sdio_packet {
-	struct if_sdio_packet	*next;
+	struct list_head	list;
 	u16			nb;
 	u8			buffer[] __aligned(4);
 };
@@ -119,7 +119,7 @@ struct if_sdio_card {
 	u8			buffer[65536] __attribute__((aligned(4)));
 
 	spinlock_t		lock;
-	struct if_sdio_packet	*packets;
+	struct list_head	packets;
 
 	struct workqueue_struct	*workqueue;
 	struct work_struct	packet_worker;
@@ -404,9 +404,10 @@ static void if_sdio_host_to_card_worker(struct work_struct *work)
 
 	while (1) {
 		spin_lock_irqsave(&card->lock, flags);
-		packet = card->packets;
+		packet = list_first_entry_or_null(&card->packets,
+						  struct if_sdio_packet, list);
 		if (packet)
-			card->packets = packet->next;
+			list_del(&packet->list);
 		spin_unlock_irqrestore(&card->lock, flags);
 
 		if (!packet)
@@ -909,7 +910,7 @@ static int if_sdio_host_to_card(struct lbs_private *priv,
 {
 	int ret;
 	struct if_sdio_card *card;
-	struct if_sdio_packet *packet, *cur;
+	struct if_sdio_packet *packet;
 	u16 size;
 	unsigned long flags;
 
@@ -934,7 +935,6 @@ static int if_sdio_host_to_card(struct lbs_private *priv,
 		goto out;
 	}
 
-	packet->next = NULL;
 	packet->nb = size;
 
 	/*
@@ -949,14 +949,7 @@ static int if_sdio_host_to_card(struct lbs_private *priv,
 
 	spin_lock_irqsave(&card->lock, flags);
 
-	if (!card->packets)
-		card->packets = packet;
-	else {
-		cur = card->packets;
-		while (cur->next)
-			cur = cur->next;
-		cur->next = packet;
-	}
+	list_add_tail(&packet->list, &card->packets);
 
 	switch (type) {
 	case MVMS_CMD:
@@ -1137,7 +1130,7 @@ static int if_sdio_probe(struct sdio_func *func,
 	struct lbs_private *priv;
 	int ret, i;
 	unsigned int model;
-	struct if_sdio_packet *packet;
+	struct if_sdio_packet *packet, *tmp;
 
 	for (i = 0;i < func->card->num_info;i++) {
 		if (sscanf(func->card->info[i],
@@ -1178,6 +1171,8 @@ static int if_sdio_probe(struct sdio_func *func,
 	}
 
 	spin_lock_init(&card->lock);
+	INIT_LIST_HEAD(&card->packets);
+
 	card->workqueue = alloc_workqueue("libertas_sdio", WQ_MEM_RECLAIM, 0);
 	if (unlikely(!card->workqueue)) {
 		ret = -ENOMEM;
@@ -1236,11 +1231,8 @@ static int if_sdio_probe(struct sdio_func *func,
 	cancel_work_sync(&card->packet_worker);
 	destroy_workqueue(card->workqueue);
 err_queue:
-	while (card->packets) {
-		packet = card->packets;
-		card->packets = card->packets->next;
+	list_for_each_entry_safe(packet, tmp, &card->packets, list)
 		kfree(packet);
-	}
 
 	kfree(card);
 
@@ -1250,7 +1242,7 @@ static int if_sdio_probe(struct sdio_func *func,
 static void if_sdio_remove(struct sdio_func *func)
 {
 	struct if_sdio_card *card;
-	struct if_sdio_packet *packet;
+	struct if_sdio_packet *packet, *tmp;
 
 	card = sdio_get_drvdata(func);
 
@@ -1281,11 +1273,8 @@ static void if_sdio_remove(struct sdio_func *func)
 	cancel_work_sync(&card->packet_worker);
 	destroy_workqueue(card->workqueue);
 
-	while (card->packets) {
-		packet = card->packets;
-		card->packets = card->packets->next;
+	list_for_each_entry_safe(packet, tmp, &card->packets, list)
 		kfree(packet);
-	}
 
 	kfree(card);
 }
-- 
2.41.0

[PATCH 3/4] wifi: libertas: simplify list operations in free_if_spi_card()

[Dmitry Antipov]

Use 'list_for_each_entry_safe()' to simplify
list operations in 'free_if_spi_card()'.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/marvell/libertas/if_spi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/if_spi.c b/drivers/net/wireless/marvell/libertas/if_spi.c
index 1225fc0e3352..3d53e444ba19 100644
--- a/drivers/net/wireless/marvell/libertas/if_spi.c
+++ b/drivers/net/wireless/marvell/libertas/if_spi.c
@@ -76,16 +76,13 @@ struct if_spi_card {
 
 static void free_if_spi_card(struct if_spi_card *card)
 {
-	struct list_head *cursor, *next;
-	struct if_spi_packet *packet;
+	struct if_spi_packet *packet, *tmp;
 
-	list_for_each_safe(cursor, next, &card->cmd_packet_list) {
-		packet = container_of(cursor, struct if_spi_packet, list);
+	list_for_each_entry_safe(packet, tmp, &card->cmd_packet_list, list) {
 		list_del(&packet->list);
 		kfree(packet);
 	}
-	list_for_each_safe(cursor, next, &card->data_packet_list) {
-		packet = container_of(cursor, struct if_spi_packet, list);
+	list_for_each_entry_safe(packet, tmp, &card->data_packet_list, list) {
 		list_del(&packet->list);
 		kfree(packet);
 	}
-- 
2.41.0

[PATCH 4/4] wifi: libertas: cleanup SDIO reset

[Dmitry Antipov]

Embed SDIO reset worker in 'struct if_sdio_card' and so
drop 'reset_host' and 'card_reset_work' static variables,
adjust related code. Not sure whether it's possible to do
something useful on 'mmc_add_host()' error, so just add
'dev_err()' to emit an error message.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 .../net/wireless/marvell/libertas/if_sdio.c   | 34 ++++++++++++-------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c b/drivers/net/wireless/marvell/libertas/if_sdio.c
index c72081cf8a85..524034699972 100644
--- a/drivers/net/wireless/marvell/libertas/if_sdio.c
+++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
@@ -123,6 +123,7 @@ struct if_sdio_card {
 
 	struct workqueue_struct	*workqueue;
 	struct work_struct	packet_worker;
+	struct work_struct	reset_worker;
 
 	u8			rx_unit;
 };
@@ -1022,10 +1023,19 @@ static int if_sdio_reset_deep_sleep_wakeup(struct lbs_private *priv)
 
 }
 
-static struct mmc_host *reset_host;
-
 static void if_sdio_reset_card_worker(struct work_struct *work)
 {
+	int ret;
+	const char *name;
+	struct device *dev;
+	struct if_sdio_card *card;
+	struct mmc_host *reset_host;
+
+	card = container_of(work, struct if_sdio_card, reset_worker);
+	reset_host = card->func->card->host;
+	name = card->priv->dev->name;
+	dev = &card->func->dev;
+
 	/*
 	 * The actual reset operation must be run outside of lbs_thread. This
 	 * is because mmc_remove_host() will cause the device to be instantly
@@ -1036,21 +1046,19 @@ static void if_sdio_reset_card_worker(struct work_struct *work)
 	 * instance for that reason.
 	 */
 
-	pr_info("Resetting card...");
+	dev_info(dev, "resetting card %s...", name);
 	mmc_remove_host(reset_host);
-	mmc_add_host(reset_host);
+	ret = mmc_add_host(reset_host);
+	if (ret)
+		dev_err(dev, "%s: can't add mmc host, error %d\n", name, ret);
 }
-static DECLARE_WORK(card_reset_work, if_sdio_reset_card_worker);
 
 static void if_sdio_reset_card(struct lbs_private *priv)
 {
 	struct if_sdio_card *card = priv->card;
 
-	if (work_pending(&card_reset_work))
-		return;
-
-	reset_host = card->func->card->host;
-	schedule_work(&card_reset_work);
+	if (!work_pending(&card->reset_worker))
+		schedule_work(&card->reset_worker);
 }
 
 static int if_sdio_power_save(struct lbs_private *priv)
@@ -1178,6 +1186,8 @@ static int if_sdio_probe(struct sdio_func *func,
 		ret = -ENOMEM;
 		goto err_queue;
 	}
+
+	INIT_WORK(&card->reset_worker, if_sdio_reset_card_worker);
 	INIT_WORK(&card->packet_worker, if_sdio_host_to_card_worker);
 	init_waitqueue_head(&card->pwron_waitq);
 
@@ -1229,6 +1239,7 @@ static int if_sdio_probe(struct sdio_func *func,
 	lbs_remove_card(priv);
 free:
 	cancel_work_sync(&card->packet_worker);
+	cancel_work_sync(&card->reset_worker);
 	destroy_workqueue(card->workqueue);
 err_queue:
 	list_for_each_entry_safe(packet, tmp, &card->packets, list)
@@ -1271,6 +1282,7 @@ static void if_sdio_remove(struct sdio_func *func)
 	lbs_remove_card(card->priv);
 
 	cancel_work_sync(&card->packet_worker);
+	cancel_work_sync(&card->reset_worker);
 	destroy_workqueue(card->workqueue);
 
 	list_for_each_entry_safe(packet, tmp, &card->packets, list)
@@ -1394,8 +1406,6 @@ static void __exit if_sdio_exit_module(void)
 	/* Set the flag as user is removing this module. */
 	user_rmmod = 1;
 
-	cancel_work_sync(&card_reset_work);
-
 	sdio_unregister_driver(&if_sdio_driver);
 }
 
-- 
2.41.0

Re: [PATCH 1/4] wifi: libertas: add missing calls to cancel_work_sync()

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> Add missing 'cancel_work_sync()' in 'if_sdio_remove()'
> and on error handling path in 'if_sdio_probe()'.
>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

How have you tested these patches?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/4] wifi: libertas: add missing calls to cancel_work_sync()

[Dmitry Antipov]

On 7/24/23 11:54, Kalle Valo wrote:

> 
> How have you tested these patches?
> 

No physical hardware so compile tested only.

Dmitry

Re: [PATCH 1/4] wifi: libertas: add missing calls to cancel_work_sync()

[Dan Williams]

On Mon, 2023-07-24 at 11:54 +0300, Kalle Valo wrote:
> Dmitry Antipov <dmantipov@yandex.ru> writes:
> 
> > Add missing 'cancel_work_sync()' in 'if_sdio_remove()'
> > and on error handling path in 'if_sdio_probe()'.
> > 
> > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> 
> How have you tested these patches?
> 

I can try to give the SDIO bits a run this week on an 8686. I don't
have a SPI setup to test though.

Dan

Re: [PATCH 1/4] wifi: libertas: add missing calls to cancel_work_sync()

[Dmitry Antipov]

On 7/25/23 00:27, Dan Williams wrote:

> I can try to give the SDIO bits a run this week on an 8686. I don't
> have a SPI setup to test though.

It would be very helpful, thanks in advance.

Dmitry

[PATCH 1/6] [v2] wifi: libertas: add missing calls to cancel_work_sync()

[Dmitry Antipov]

Add missing 'cancel_work_sync()' in 'if_sdio_remove()'
and on error handling path in 'if_sdio_probe()'.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/marvell/libertas/if_sdio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c b/drivers/net/wireless/marvell/libertas/if_sdio.c
index a63c5e622ee3..a35b33e84670 100644
--- a/drivers/net/wireless/marvell/libertas/if_sdio.c
+++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
@@ -1233,6 +1233,7 @@ static int if_sdio_probe(struct sdio_func *func,
 	flush_workqueue(card->workqueue);
 	lbs_remove_card(priv);
 free:
+	cancel_work_sync(&card->packet_worker);
 	destroy_workqueue(card->workqueue);
 err_queue:
 	while (card->packets) {
@@ -1277,6 +1278,7 @@ static void if_sdio_remove(struct sdio_func *func)
 	lbs_stop_card(card->priv);
 	lbs_remove_card(card->priv);
 
+	cancel_work_sync(&card->packet_worker);
 	destroy_workqueue(card->workqueue);
 
 	while (card->packets) {
-- 
2.41.0

[PATCH 2/6] [v2] wifi: libertas: use convenient lists to manage SDIO packets

[Dmitry Antipov]

Use convenient lists to manage SDIO packets, adjust
'struct if_sdio_packet', 'struct if_sdio_card' and
related code accordingly.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 .../net/wireless/marvell/libertas/if_sdio.c   | 37 +++++++------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c b/drivers/net/wireless/marvell/libertas/if_sdio.c
index a35b33e84670..c72081cf8a85 100644
--- a/drivers/net/wireless/marvell/libertas/if_sdio.c
+++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
@@ -101,7 +101,7 @@ MODULE_FIRMWARE("sd8688_helper.bin");
 MODULE_FIRMWARE("sd8688.bin");
 
 struct if_sdio_packet {
-	struct if_sdio_packet	*next;
+	struct list_head	list;
 	u16			nb;
 	u8			buffer[] __aligned(4);
 };
@@ -119,7 +119,7 @@ struct if_sdio_card {
 	u8			buffer[65536] __attribute__((aligned(4)));
 
 	spinlock_t		lock;
-	struct if_sdio_packet	*packets;
+	struct list_head	packets;
 
 	struct workqueue_struct	*workqueue;
 	struct work_struct	packet_worker;
@@ -404,9 +404,10 @@ static void if_sdio_host_to_card_worker(struct work_struct *work)
 
 	while (1) {
 		spin_lock_irqsave(&card->lock, flags);
-		packet = card->packets;
+		packet = list_first_entry_or_null(&card->packets,
+						  struct if_sdio_packet, list);
 		if (packet)
-			card->packets = packet->next;
+			list_del(&packet->list);
 		spin_unlock_irqrestore(&card->lock, flags);
 
 		if (!packet)
@@ -909,7 +910,7 @@ static int if_sdio_host_to_card(struct lbs_private *priv,
 {
 	int ret;
 	struct if_sdio_card *card;
-	struct if_sdio_packet *packet, *cur;
+	struct if_sdio_packet *packet;
 	u16 size;
 	unsigned long flags;
 
@@ -934,7 +935,6 @@ static int if_sdio_host_to_card(struct lbs_private *priv,
 		goto out;
 	}
 
-	packet->next = NULL;
 	packet->nb = size;
 
 	/*
@@ -949,14 +949,7 @@ static int if_sdio_host_to_card(struct lbs_private *priv,
 
 	spin_lock_irqsave(&card->lock, flags);
 
-	if (!card->packets)
-		card->packets = packet;
-	else {
-		cur = card->packets;
-		while (cur->next)
-			cur = cur->next;
-		cur->next = packet;
-	}
+	list_add_tail(&packet->list, &card->packets);
 
 	switch (type) {
 	case MVMS_CMD:
@@ -1137,7 +1130,7 @@ static int if_sdio_probe(struct sdio_func *func,
 	struct lbs_private *priv;
 	int ret, i;
 	unsigned int model;
-	struct if_sdio_packet *packet;
+	struct if_sdio_packet *packet, *tmp;
 
 	for (i = 0;i < func->card->num_info;i++) {
 		if (sscanf(func->card->info[i],
@@ -1178,6 +1171,8 @@ static int if_sdio_probe(struct sdio_func *func,
 	}
 
 	spin_lock_init(&card->lock);
+	INIT_LIST_HEAD(&card->packets);
+
 	card->workqueue = alloc_workqueue("libertas_sdio", WQ_MEM_RECLAIM, 0);
 	if (unlikely(!card->workqueue)) {
 		ret = -ENOMEM;
@@ -1236,11 +1231,8 @@ static int if_sdio_probe(struct sdio_func *func,
 	cancel_work_sync(&card->packet_worker);
 	destroy_workqueue(card->workqueue);
 err_queue:
-	while (card->packets) {
-		packet = card->packets;
-		card->packets = card->packets->next;
+	list_for_each_entry_safe(packet, tmp, &card->packets, list)
 		kfree(packet);
-	}
 
 	kfree(card);
 
@@ -1250,7 +1242,7 @@ static int if_sdio_probe(struct sdio_func *func,
 static void if_sdio_remove(struct sdio_func *func)
 {
 	struct if_sdio_card *card;
-	struct if_sdio_packet *packet;
+	struct if_sdio_packet *packet, *tmp;
 
 	card = sdio_get_drvdata(func);
 
@@ -1281,11 +1273,8 @@ static void if_sdio_remove(struct sdio_func *func)
 	cancel_work_sync(&card->packet_worker);
 	destroy_workqueue(card->workqueue);
 
-	while (card->packets) {
-		packet = card->packets;
-		card->packets = card->packets->next;
+	list_for_each_entry_safe(packet, tmp, &card->packets, list)
 		kfree(packet);
-	}
 
 	kfree(card);
 }
-- 
2.41.0

[PATCH 3/6] [v2] wifi: libertas: simplify list operations in free_if_spi_card()

[Dmitry Antipov]

Use 'list_for_each_entry_safe()' to simplify
list operations in 'free_if_spi_card()'.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/marvell/libertas/if_spi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/if_spi.c b/drivers/net/wireless/marvell/libertas/if_spi.c
index 1225fc0e3352..3d53e444ba19 100644
--- a/drivers/net/wireless/marvell/libertas/if_spi.c
+++ b/drivers/net/wireless/marvell/libertas/if_spi.c
@@ -76,16 +76,13 @@ struct if_spi_card {
 
 static void free_if_spi_card(struct if_spi_card *card)
 {
-	struct list_head *cursor, *next;
-	struct if_spi_packet *packet;
+	struct if_spi_packet *packet, *tmp;
 
-	list_for_each_safe(cursor, next, &card->cmd_packet_list) {
-		packet = container_of(cursor, struct if_spi_packet, list);
+	list_for_each_entry_safe(packet, tmp, &card->cmd_packet_list, list) {
 		list_del(&packet->list);
 		kfree(packet);
 	}
-	list_for_each_safe(cursor, next, &card->data_packet_list) {
-		packet = container_of(cursor, struct if_spi_packet, list);
+	list_for_each_entry_safe(packet, tmp, &card->data_packet_list, list) {
 		list_del(&packet->list);
 		kfree(packet);
 	}
-- 
2.41.0

[PATCH 4/6] [v2] wifi: libertas: cleanup SDIO reset

[Dmitry Antipov]

Embed SDIO reset worker in 'struct if_sdio_card' and so
drop 'reset_host' and 'card_reset_work' static variables,
adjust related code. Not sure whether it's possible to do
something useful on 'mmc_add_host()' error, so just add
'dev_err()' to emit an error message.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 .../net/wireless/marvell/libertas/if_sdio.c   | 34 ++++++++++++-------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c b/drivers/net/wireless/marvell/libertas/if_sdio.c
index c72081cf8a85..524034699972 100644
--- a/drivers/net/wireless/marvell/libertas/if_sdio.c
+++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
@@ -123,6 +123,7 @@ struct if_sdio_card {
 
 	struct workqueue_struct	*workqueue;
 	struct work_struct	packet_worker;
+	struct work_struct	reset_worker;
 
 	u8			rx_unit;
 };
@@ -1022,10 +1023,19 @@ static int if_sdio_reset_deep_sleep_wakeup(struct lbs_private *priv)
 
 }
 
-static struct mmc_host *reset_host;
-
 static void if_sdio_reset_card_worker(struct work_struct *work)
 {
+	int ret;
+	const char *name;
+	struct device *dev;
+	struct if_sdio_card *card;
+	struct mmc_host *reset_host;
+
+	card = container_of(work, struct if_sdio_card, reset_worker);
+	reset_host = card->func->card->host;
+	name = card->priv->dev->name;
+	dev = &card->func->dev;
+
 	/*
 	 * The actual reset operation must be run outside of lbs_thread. This
 	 * is because mmc_remove_host() will cause the device to be instantly
@@ -1036,21 +1046,19 @@ static void if_sdio_reset_card_worker(struct work_struct *work)
 	 * instance for that reason.
 	 */
 
-	pr_info("Resetting card...");
+	dev_info(dev, "resetting card %s...", name);
 	mmc_remove_host(reset_host);
-	mmc_add_host(reset_host);
+	ret = mmc_add_host(reset_host);
+	if (ret)
+		dev_err(dev, "%s: can't add mmc host, error %d\n", name, ret);
 }
-static DECLARE_WORK(card_reset_work, if_sdio_reset_card_worker);
 
 static void if_sdio_reset_card(struct lbs_private *priv)
 {
 	struct if_sdio_card *card = priv->card;
 
-	if (work_pending(&card_reset_work))
-		return;
-
-	reset_host = card->func->card->host;
-	schedule_work(&card_reset_work);
+	if (!work_pending(&card->reset_worker))
+		schedule_work(&card->reset_worker);
 }
 
 static int if_sdio_power_save(struct lbs_private *priv)
@@ -1178,6 +1186,8 @@ static int if_sdio_probe(struct sdio_func *func,
 		ret = -ENOMEM;
 		goto err_queue;
 	}
+
+	INIT_WORK(&card->reset_worker, if_sdio_reset_card_worker);
 	INIT_WORK(&card->packet_worker, if_sdio_host_to_card_worker);
 	init_waitqueue_head(&card->pwron_waitq);
 
@@ -1229,6 +1239,7 @@ static int if_sdio_probe(struct sdio_func *func,
 	lbs_remove_card(priv);
 free:
 	cancel_work_sync(&card->packet_worker);
+	cancel_work_sync(&card->reset_worker);
 	destroy_workqueue(card->workqueue);
 err_queue:
 	list_for_each_entry_safe(packet, tmp, &card->packets, list)
@@ -1271,6 +1282,7 @@ static void if_sdio_remove(struct sdio_func *func)
 	lbs_remove_card(card->priv);
 
 	cancel_work_sync(&card->packet_worker);
+	cancel_work_sync(&card->reset_worker);
 	destroy_workqueue(card->workqueue);
 
 	list_for_each_entry_safe(packet, tmp, &card->packets, list)
@@ -1394,8 +1406,6 @@ static void __exit if_sdio_exit_module(void)
 	/* Set the flag as user is removing this module. */
 	user_rmmod = 1;
 
-	cancel_work_sync(&card_reset_work);
-
 	sdio_unregister_driver(&if_sdio_driver);
 }
 
-- 
2.41.0

[PATCH 5/6] [v2] wifi: libertas: handle possible spu_write_u16() errors

[Dmitry Antipov]

Check and handle (well, report at least, as it's done through the rest
of the module) possible 'spu_write_u16()' errors in 'if_spi_e2h()'.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/marvell/libertas/if_spi.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/if_spi.c b/drivers/net/wireless/marvell/libertas/if_spi.c
index 3d53e444ba19..8690b0114e23 100644
--- a/drivers/net/wireless/marvell/libertas/if_spi.c
+++ b/drivers/net/wireless/marvell/libertas/if_spi.c
@@ -826,11 +826,16 @@ static void if_spi_e2h(struct if_spi_card *card)
 		goto out;
 
 	/* re-enable the card event interrupt */
-	spu_write_u16(card, IF_SPI_HOST_INT_STATUS_REG,
-			~IF_SPI_HICU_CARD_EVENT);
+	err = spu_write_u16(card, IF_SPI_HOST_INT_STATUS_REG,
+			    ~IF_SPI_HICU_CARD_EVENT);
+	if (err)
+		goto out;
 
 	/* generate a card interrupt */
-	spu_write_u16(card, IF_SPI_CARD_INT_CAUSE_REG, IF_SPI_CIC_HOST_EVENT);
+	err = spu_write_u16(card, IF_SPI_CARD_INT_CAUSE_REG,
+			    IF_SPI_CIC_HOST_EVENT);
+	if (err)
+		goto out;
 
 	lbs_queue_event(priv, cause & 0xff);
 out:
-- 
2.41.0

[PATCH 6/6] [v2] wifi: libertas: prefer kstrtoX() for simple integer conversions

[Dmitry Antipov]

Prefer 'kstrtoX()' family of functions over 'sscanf()' to convert
strings to integers and always check results of the conversions.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/marvell/libertas/mesh.c | 51 +++++++++++++-------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/mesh.c b/drivers/net/wireless/marvell/libertas/mesh.c
index 90ffe8d1e0e8..2dd635935448 100644
--- a/drivers/net/wireless/marvell/libertas/mesh.c
+++ b/drivers/net/wireless/marvell/libertas/mesh.c
@@ -188,8 +188,11 @@ static ssize_t anycast_mask_store(struct device *dev,
 	uint32_t datum;
 	int ret;
 
+	ret = kstrtouint(buf, 16, &datum);
+	if (ret)
+		return ret;
+
 	memset(&mesh_access, 0, sizeof(mesh_access));
-	sscanf(buf, "%x", &datum);
 	mesh_access.data[0] = cpu_to_le32(datum);
 
 	ret = lbs_mesh_access(priv, CMD_ACT_MESH_SET_ANYCAST, &mesh_access);
@@ -241,15 +244,14 @@ static ssize_t prb_rsp_limit_store(struct device *dev,
 	int ret;
 	unsigned long retry_limit;
 
-	memset(&mesh_access, 0, sizeof(mesh_access));
-	mesh_access.data[0] = cpu_to_le32(CMD_ACT_SET);
-
 	ret = kstrtoul(buf, 10, &retry_limit);
 	if (ret)
 		return ret;
 	if (retry_limit > 15)
 		return -ENOTSUPP;
 
+	memset(&mesh_access, 0, sizeof(mesh_access));
+	mesh_access.data[0] = cpu_to_le32(CMD_ACT_SET);
 	mesh_access.data[1] = cpu_to_le32(retry_limit);
 
 	ret = lbs_mesh_access(priv, CMD_ACT_MESH_SET_GET_PRB_RSP_LIMIT,
@@ -285,9 +287,12 @@ static ssize_t lbs_mesh_store(struct device *dev,
 			      const char *buf, size_t count)
 {
 	struct lbs_private *priv = to_net_dev(dev)->ml_priv;
-	int enable;
+	int ret, enable;
+
+	ret = kstrtoint(buf, 16, &enable);
+	if (ret)
+		return ret;
 
-	sscanf(buf, "%x", &enable);
 	enable = !!enable;
 	if (enable == !!priv->mesh_dev)
 		return count;
@@ -387,11 +392,13 @@ static ssize_t bootflag_store(struct device *dev, struct device_attribute *attr,
 	uint32_t datum;
 	int ret;
 
-	memset(&cmd, 0, sizeof(cmd));
-	ret = sscanf(buf, "%d", &datum);
-	if ((ret != 1) || (datum > 1))
+	ret = kstrtouint(buf, 10, &datum);
+	if (ret)
+		return ret;
+	if (datum > 1)
 		return -EINVAL;
 
+	memset(&cmd, 0, sizeof(cmd));
 	*((__le32 *)&cmd.data[0]) = cpu_to_le32(!!datum);
 	cmd.length = cpu_to_le16(sizeof(uint32_t));
 	ret = lbs_mesh_config_send(priv, &cmd, CMD_ACT_MESH_CONFIG_SET,
@@ -438,11 +445,14 @@ static ssize_t boottime_store(struct device *dev,
 	uint32_t datum;
 	int ret;
 
-	memset(&cmd, 0, sizeof(cmd));
-	ret = sscanf(buf, "%d", &datum);
-	if ((ret != 1) || (datum > 255))
+	ret = kstrtouint(buf, 10, &datum);
+	if (ret)
+		return ret;
+	if (datum > 255)
 		return -EINVAL;
 
+	memset(&cmd, 0, sizeof(cmd));
+
 	/* A too small boot time will result in the device booting into
 	 * standalone (no-host) mode before the host can take control of it,
 	 * so the change will be hard to revert.  This may be a desired
@@ -497,11 +507,13 @@ static ssize_t channel_store(struct device *dev, struct device_attribute *attr,
 	uint32_t datum;
 	int ret;
 
-	memset(&cmd, 0, sizeof(cmd));
-	ret = sscanf(buf, "%d", &datum);
-	if (ret != 1 || datum < 1 || datum > 11)
+	ret = kstrtouint(buf, 10, &datum);
+	if (ret)
+		return ret;
+	if (datum < 1 || datum > 11)
 		return -EINVAL;
 
+	memset(&cmd, 0, sizeof(cmd));
 	*((__le16 *)&cmd.data[0]) = cpu_to_le16(datum);
 	cmd.length = cpu_to_le16(sizeof(uint16_t));
 	ret = lbs_mesh_config_send(priv, &cmd, CMD_ACT_MESH_CONFIG_SET,
@@ -626,11 +638,14 @@ static ssize_t protocol_id_store(struct device *dev,
 	uint32_t datum;
 	int ret;
 
-	memset(&cmd, 0, sizeof(cmd));
-	ret = sscanf(buf, "%d", &datum);
-	if ((ret != 1) || (datum > 255))
+	ret = kstrtouint(buf, 10, &datum);
+	if (ret)
+		return ret;
+	if (datum > 255)
 		return -EINVAL;
 
+	memset(&cmd, 0, sizeof(cmd));
+
 	/* fetch all other Information Element parameters */
 	ret = mesh_get_default_parameters(dev, &defs);
 
-- 
2.41.0

Re: [PATCH 1/6] [v2] wifi: libertas: add missing calls to cancel_work_sync()

[Dan Williams]

On Tue, 2023-07-25 at 09:04 +0300, Dmitry Antipov wrote:
> Add missing 'cancel_work_sync()' in 'if_sdio_remove()'
> and on error handling path in 'if_sdio_probe()'.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

For the v2 series:

Tested-by: Dan Williams <dcbw@redhat.com>

> ---
>  drivers/net/wireless/marvell/libertas/if_sdio.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/marvell/libertas/if_sdio.c
> b/drivers/net/wireless/marvell/libertas/if_sdio.c
> index a63c5e622ee3..a35b33e84670 100644
> --- a/drivers/net/wireless/marvell/libertas/if_sdio.c
> +++ b/drivers/net/wireless/marvell/libertas/if_sdio.c
> @@ -1233,6 +1233,7 @@ static int if_sdio_probe(struct sdio_func
> *func,
>         flush_workqueue(card->workqueue);
>         lbs_remove_card(priv);
>  free:
> +       cancel_work_sync(&card->packet_worker);
>         destroy_workqueue(card->workqueue);
>  err_queue:
>         while (card->packets) {
> @@ -1277,6 +1278,7 @@ static void if_sdio_remove(struct sdio_func
> *func)
>         lbs_stop_card(card->priv);
>         lbs_remove_card(card->priv);
>  
> +       cancel_work_sync(&card->packet_worker);
>         destroy_workqueue(card->workqueue);
>  
>         while (card->packets) {

Re: [PATCH 1/4] wifi: libertas: add missing calls to cancel_work_sync()

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> On 7/24/23 11:54, Kalle Valo wrote:
>
>> How have you tested these patches?
>> 
>
> No physical hardware so compile tested only.

In that case please always add "Compile tested only" to the commit log.
It's important for us to know if a (non-trivial) patch is tested on a
real hardware or just with a compiler.

Another problem I see that you don't always reply to review comments and
that gives an impression that the comments are ignored. Please always
try to reply something to the review comments, even if just a simple
"ok" or "I don't agree because...".

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/4] wifi: libertas: add missing calls to cancel_work_sync()

[Dmitry Antipov]

On 8/1/23 13:23, Kalle Valo wrote:

> In that case please always add "Compile tested only" to the commit log.
> It's important for us to know if a (non-trivial) patch is tested on a
> real hardware or just with a compiler.

OK.

> Another problem I see that you don't always reply to review comments and
> that gives an impression that the comments are ignored. Please always
> try to reply something to the review comments, even if just a simple
> "ok" or "I don't agree because...".

Looking through my e-mails for the previous month, I was unable to find an
unanswered review. Could you please provide an example? I'll fix it ASAP.

I don't want to speculate around the workflow of others and realize that someone
(especially the maintainer) may be overloaded and too busy. OTOH it's not quite clear
why the trivial things like https://marc.info/?l=linux-wireless&m=169030215701718&w=2
stalls for almost a week. Should I consider this as "ignored" too?

Dmitry

Re: [PATCH 1/4] wifi: libertas: add missing calls to cancel_work_sync()

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> On 8/1/23 13:23, Kalle Valo wrote:
>
>> Another problem I see that you don't always reply to review comments and
>> that gives an impression that the comments are ignored. Please always
>> try to reply something to the review comments, even if just a simple
>> "ok" or "I don't agree because...".
>
> Looking through my e-mails for the previous month, I was unable to find an
> unanswered review. Could you please provide an example? I'll fix it
> ASAP.

You have sent so many patches and I don't have time to start go through
them. Maybe I noticed that with some of mwifiex patches, not sure. But
that doesn't matter, I just hope that in the future you reply to
comments.

> I don't want to speculate around the workflow of others and realize
> that someone (especially the maintainer) may be overloaded and too
> busy. OTOH it's not quite clear why the trivial things like
> https://marc.info/?l=linux-wireless&m=169030215701718&w=2 stalls for
> almost a week. Should I consider this as "ignored" too?

A delay of week is business as usual, I have patches in queue which are
from last October:

https://patchwork.kernel.org/project/linux-wireless/list/?series=684424&state=*

Remember that this is not a 24/7 service and we just had a summer break.
I have 160 patches in patchwork right so expect long delays but you can
check the status from patchwork yourself:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#checking_state_of_patches_from_patchwork

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/4] wifi: libertas: add missing calls to cancel_work_sync()

[Kalle Valo]

Kalle Valo <kvalo@kernel.org> writes:

> Dmitry Antipov <dmantipov@yandex.ru> writes:
>
>> On 8/1/23 13:23, Kalle Valo wrote:
>>
>>> Another problem I see that you don't always reply to review comments and
>>> that gives an impression that the comments are ignored. Please always
>>> try to reply something to the review comments, even if just a simple
>>> "ok" or "I don't agree because...".
>>
>> Looking through my e-mails for the previous month, I was unable to find an
>> unanswered review. Could you please provide an example? I'll fix it
>> ASAP.
>
> You have sent so many patches and I don't have time to start go through
> them. Maybe I noticed that with some of mwifiex patches, not sure. But
> that doesn't matter, I just hope that in the future you reply to
> comments.

While going through patches in patchwork I noticed this dicussion:

https://patchwork.kernel.org/project/linux-wireless/patch/20230726072114.51964-2-dmantipov@yandex.ru/

As there's no reply to Brian it gives a feeling that you are ignoring
our comments.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/6] [v2] wifi: libertas: add missing calls to cancel_work_sync()

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> wrote:

> Add missing 'cancel_work_sync()' in 'if_sdio_remove()'
> and on error handling path in 'if_sdio_probe()'.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> Tested-by: Dan Williams <dcbw@redhat.com>

6 patches applied to wireless-next.git, thanks.

c1861ff1d63d wifi: libertas: add missing calls to cancel_work_sync()
ce44fdf9c9d2 wifi: libertas: use convenient lists to manage SDIO packets
2c531d28f8e9 wifi: libertas: simplify list operations in free_if_spi_card()
6c968e90198f wifi: libertas: cleanup SDIO reset
3e14212f79fd wifi: libertas: handle possible spu_write_u16() errors
f5343efdf5b5 wifi: libertas: prefer kstrtoX() for simple integer conversions

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230725060531.72968-1-dmantipov@yandex.ru/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches