| Message ID | 20230724084457.64995-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/4] wifi: libertas: add missing calls to cancel_work_sync() | expand |
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?
On 7/24/23 11:54, Kalle Valo wrote: > > How have you tested these patches? > No physical hardware so compile tested only. Dmitry
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
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
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...".
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
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
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.
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) {
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(+)