LinuxLists.cc - [PATCH 1/4] wifi: libertas: add missing calls to cancel_work

2023-07-24 08:53:13

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

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

Signed-off-by: Dmitry Antipov <[email protected]>
---
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

2023-07-24 08:53:36

by Dmitry Antipov

[permalink] [raw]

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

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

Signed-off-by: Dmitry Antipov <[email protected]>
---
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

2023-07-24 08:53:37

by Dmitry Antipov

[permalink] [raw]

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

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 <[email protected]>
---
.../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

2023-07-24 08:54:02

by Dmitry Antipov

[permalink] [raw]

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

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 <[email protected]>
---
.../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

2023-07-24 08:57:48

by Kalle Valo

[permalink] [raw]

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

Dmitry Antipov <[email protected]> writes:

> Add missing 'cancel_work_sync()' in 'if_sdio_remove()'
> and on error handling path in 'if_sdio_probe()'.
>
> Signed-off-by: Dmitry Antipov <[email protected]>

How have you tested these patches?

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

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

2023-07-24 09:01:15

by Dmitry Antipov

[permalink] [raw]

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

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

>
> How have you tested these patches?
>

No physical hardware so compile tested only.

Dmitry

2023-07-24 21:42:11

by Dan Williams

[permalink] [raw]

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

On Mon, 2023-07-24 at 11:54 +0300, Kalle Valo wrote:
> Dmitry Antipov <[email protected]> writes:
>
> > Add missing 'cancel_work_sync()' in 'if_sdio_remove()'
> > and on error handling path in 'if_sdio_probe()'.
> >
> > Signed-off-by: Dmitry Antipov <[email protected]>
>
> 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

2023-07-25 05:23:55

by Dmitry Antipov

[permalink] [raw]

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

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

2023-07-25 06:24:13

by Dmitry Antipov

[permalink] [raw]

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

2023-07-25 06:24:33

by Dmitry Antipov

[permalink] [raw]

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

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 <[email protected]>
---
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

2023-07-25 06:26:01

by Dmitry Antipov

[permalink] [raw]

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

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 <[email protected]>
---
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

2023-07-25 06:26:11

by Dmitry Antipov

[permalink] [raw]

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

2023-07-25 06:26:14

by Dmitry Antipov

[permalink] [raw]

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

2023-07-25 06:26:22

by Dmitry Antipov

[permalink] [raw]

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

2023-07-31 13:54:06

by Dan Williams

[permalink] [raw]

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

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 <[email protected]>

For the v2 series:

Tested-by: Dan Williams <[email protected]>

> ---
> 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) {

2023-08-01 10:52:07

by Kalle Valo

[permalink] [raw]

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

Dmitry Antipov <[email protected]> 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

2023-08-01 12:20:06

by Dmitry Antipov

[permalink] [raw]

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

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

2023-08-01 14:15:35

by Kalle Valo

[permalink] [raw]

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

Dmitry Antipov <[email protected]> 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

2023-08-01 14:18:23

by Kalle Valo

[permalink] [raw]

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

Kalle Valo <[email protected]> writes:

> Dmitry Antipov <[email protected]> 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/[email protected]/

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

2023-08-01 15:38:21

by Kalle Valo

[permalink] [raw]

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

Dmitry Antipov <[email protected]> wrote:

> Add missing 'cancel_work_sync()' in 'if_sdio_remove()'
> and on error handling path in 'if_sdio_probe()'.
>
> Signed-off-by: Dmitry Antipov <[email protected]>
> Tested-by: Dan Williams <[email protected]>

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/[email protected]/

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