* [PATCH] firewire: core: extend card->lock in fw_core_handle_bus_reset
@ 2022-03-03 18:30 Niels Dossche
2022-03-04 10:25 ` Takashi Sakamoto
0 siblings, 1 reply; 2+ messages in thread
From: Niels Dossche @ 2022-03-03 18:30 UTC (permalink / raw)
To: linux1394-devel, linux-kernel; +Cc: Stefan Richter, Niels Dossche
card->local_node and card->bm_retries are both always accessed under
card->lock.
fw_core_handle_bus_reset has a check whose condition depends on
card->local_node and whose body writes to card->bm_retries.
Both of these accesses are not under card->lock. Move the lock acquiring
of card->lock to before this check such that these accesses do happen
when card->lock is held.
fw_destroy_nodes is called inside the check.
Since fw_destroy_nodes already acquires card->lock inside its function
body, move this out to the callsites of fw_destroy_nodes.
Also add a comment to indicate which locking is necessary when calling
fw_destroy_nodes.
Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
---
drivers/firewire/core-card.c | 3 +++
drivers/firewire/core-topology.c | 9 +++------
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 54be88167c60..f3b3953cac83 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -668,6 +668,7 @@ EXPORT_SYMBOL_GPL(fw_card_release);
void fw_core_remove_card(struct fw_card *card)
{
struct fw_card_driver dummy_driver = dummy_driver_template;
+ unsigned long flags;
card->driver->update_phy_reg(card, 4,
PHY_LINK_ACTIVE | PHY_CONTENDER, 0);
@@ -682,7 +683,9 @@ void fw_core_remove_card(struct fw_card *card)
dummy_driver.stop_iso = card->driver->stop_iso;
card->driver = &dummy_driver;
+ spin_lock_irqsave(&card->lock, flags);
fw_destroy_nodes(card);
+ spin_unlock_irqrestore(&card->lock, flags);
/* Wait for all users, especially device workqueue jobs, to finish. */
fw_card_put(card);
diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index b63d55f5ebd3..f40c81534381 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -375,16 +375,13 @@ static void report_found_node(struct fw_card *card,
card->bm_retries = 0;
}
+/* Must be called with card->lock held */
void fw_destroy_nodes(struct fw_card *card)
{
- unsigned long flags;
-
- spin_lock_irqsave(&card->lock, flags);
card->color++;
if (card->local_node != NULL)
for_each_fw_node(card, card->local_node, report_lost_node);
card->local_node = NULL;
- spin_unlock_irqrestore(&card->lock, flags);
}
static void move_tree(struct fw_node *node0, struct fw_node *node1, int port)
@@ -510,6 +507,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
struct fw_node *local_node;
unsigned long flags;
+ spin_lock_irqsave(&card->lock, flags);
+
/*
* If the selfID buffer is not the immediate successor of the
* previously processed one, we cannot reliably compare the
@@ -521,8 +520,6 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
card->bm_retries = 0;
}
- spin_lock_irqsave(&card->lock, flags);
-
card->broadcast_channel_allocated = card->broadcast_channel_auto_allocated;
card->node_id = node_id;
/*
--
2.35.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] firewire: core: extend card->lock in fw_core_handle_bus_reset
2022-03-03 18:30 [PATCH] firewire: core: extend card->lock in fw_core_handle_bus_reset Niels Dossche
@ 2022-03-04 10:25 ` Takashi Sakamoto
0 siblings, 0 replies; 2+ messages in thread
From: Takashi Sakamoto @ 2022-03-04 10:25 UTC (permalink / raw)
To: Niels Dossche; +Cc: linux1394-devel, linux-kernel
Hi,
On Thu, Mar 03, 2022 at 07:30:38PM +0100, Niels Dossche wrote:
> card->local_node and card->bm_retries are both always accessed under
> card->lock.
> fw_core_handle_bus_reset has a check whose condition depends on
> card->local_node and whose body writes to card->bm_retries.
> Both of these accesses are not under card->lock. Move the lock acquiring
> of card->lock to before this check such that these accesses do happen
> when card->lock is held.
> fw_destroy_nodes is called inside the check.
> Since fw_destroy_nodes already acquires card->lock inside its function
> body, move this out to the callsites of fw_destroy_nodes.
> Also add a comment to indicate which locking is necessary when calling
> fw_destroy_nodes.
>
> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> ---
> drivers/firewire/core-card.c | 3 +++
> drivers/firewire/core-topology.c | 9 +++------
> 2 files changed, 6 insertions(+), 6 deletions(-)
It looks good to me and is preferable as Niels said.
Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Tested-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
I think the patch is for minor code refactoring rather than bug fix
since it is unlikely that the rate condition occurs. The bus_reset_work
is synchronously canceled in the beginning of pci_remove().
```
pci_remove()
->cancel_work_sync(&ohci->bus_reset_work);
->fw_core_remove_card(&ohci->card);
->software_reset(ohci);
->free_irq(dev->irq, ohci);
```
But I have no confidence that the above calls are done in the order by
processor...
> diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
> index 54be88167c60..f3b3953cac83 100644
> --- a/drivers/firewire/core-card.c
> +++ b/drivers/firewire/core-card.c
> @@ -668,6 +668,7 @@ EXPORT_SYMBOL_GPL(fw_card_release);
> void fw_core_remove_card(struct fw_card *card)
> {
> struct fw_card_driver dummy_driver = dummy_driver_template;
> + unsigned long flags;
>
> card->driver->update_phy_reg(card, 4,
> PHY_LINK_ACTIVE | PHY_CONTENDER, 0);
> @@ -682,7 +683,9 @@ void fw_core_remove_card(struct fw_card *card)
> dummy_driver.stop_iso = card->driver->stop_iso;
> card->driver = &dummy_driver;
>
> + spin_lock_irqsave(&card->lock, flags);
> fw_destroy_nodes(card);
> + spin_unlock_irqrestore(&card->lock, flags);
>
> /* Wait for all users, especially device workqueue jobs, to finish. */
> fw_card_put(card);
> diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
> index b63d55f5ebd3..f40c81534381 100644
> --- a/drivers/firewire/core-topology.c
> +++ b/drivers/firewire/core-topology.c
> @@ -375,16 +375,13 @@ static void report_found_node(struct fw_card *card,
> card->bm_retries = 0;
> }
>
> +/* Must be called with card->lock held */
> void fw_destroy_nodes(struct fw_card *card)
> {
> - unsigned long flags;
> -
> - spin_lock_irqsave(&card->lock, flags);
> card->color++;
> if (card->local_node != NULL)
> for_each_fw_node(card, card->local_node, report_lost_node);
> card->local_node = NULL;
> - spin_unlock_irqrestore(&card->lock, flags);
> }
>
> static void move_tree(struct fw_node *node0, struct fw_node *node1, int port)
> @@ -510,6 +507,8 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
> struct fw_node *local_node;
> unsigned long flags;
>
> + spin_lock_irqsave(&card->lock, flags);
> +
> /*
> * If the selfID buffer is not the immediate successor of the
> * previously processed one, we cannot reliably compare the
> @@ -521,8 +520,6 @@ void fw_core_handle_bus_reset(struct fw_card *card, int node_id, int generation,
> card->bm_retries = 0;
> }
>
> - spin_lock_irqsave(&card->lock, flags);
> -
> card->broadcast_channel_allocated = card->broadcast_channel_auto_allocated;
> card->node_id = node_id;
> /*
> --
> 2.35.1
Regards
Takashi Sakamoto
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-03-04 10:31 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-03 18:30 [PATCH] firewire: core: extend card->lock in fw_core_handle_bus_reset Niels Dossche
2022-03-04 10:25 ` Takashi Sakamoto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).